Fix mutex references and the global lock

[martin-hughes]

This fixes #305 by correcting the mutex acquisition and release. It also allows mutexes to be acquired and released by reference, since otherwise it is not possible to acquire (or release) _GL.

@IsaacWoods the main question I had was about where to keep track of the reentrant acquisition count of global_lock_mutex. Clearly the Handler should not release the actual mutex until the count reaches zero, but we don't know what the count is.

There were two options that I could think of:

1. Keep track of the count in Interpreter (which I chose), or
2. Require Handler to return a value indicating whether the mutex was fully released or not.

Option 2 would require an API change, and also relies on the implementer to correctly deal with reentrancy. Whereas option 1 allows the crate to deal with an inadequately implemented Handler but if Handler is wrong then the actual mutex may come out of sync with the firmware lock.

I would be happy to switch to option 2 if you prefer, Isaac.

[SnowCheetos]

Perhaps a syntax sugar closure like handler.with_lock(...) would a nice little addition to make life easier, smth like:

fn with_lock<R>(
    &self,
    mutex: Handle,
    timeout: u16,
    f: impl FnOnce() -> R,
) -> Result<R, AmlError> {
    self.acquire(mutex, timeout)?;

    struct Guard<'a, H: Handler> {
        handler: &'a H,
        mutex: Handle,
    }

    impl<H: Handler> Drop for Guard<'_, H> {
        fn drop(&mut self) {
            self.handler.release(self.mutex);
        }
    }

    let _guard = Guard {
        handler: self,
        mutex,
    };

    Ok(f())
}

Little helpers like this saved plenty of boot wedges for me

[martin-hughes]

Do you mean to add to this PR? If so would you show me an example of it being used?

I can think why it might be generally useful but I'm not sure about needing it here

[IsaacWoods]

Thanks for working on this Martin! I also have some _GL work locally that is partially orthogonal to this (I am wondering if we need to manually synchronise the hardware access between potential OS threads with a spinlock-type primitive too, plus moving some bits re the hardware locking into Facs) that I'll try to get in at some point. This seems like a strict improvement in the meantime, so thank you.

@IsaacWoods the main question I had was about where to keep track of the reentrant acquisition count of global_lock_mutex. Clearly the Handler should not release the actual mutex until the count reaches zero, but we don't know what the count is.

Yeah, we have to trust the Handler to be correct in some ways; I think either approach would be reasonable but I'm happy with keeping track of the count within Interpreter like this.

[IsaacWoods]

On second reading - are these orderings strong enough? This is an accursed area of knowledge for me, but I wonder if these should be Acquire and Release respectively? Other than that, very happy to merge.

[martin-hughes]

TL;DR: I've changed the memory ordering

Longer: I think Relaxed is OK because:

1. I expect that the global lock mutex would provide Acquire before we first access global_lock_acquisition_count, and Release after the last time
2. The global_lock_acquisition_count is only accessed in a single thread at a time (*) due to the global lock mutex.

But I'm not entirely certain that (1) is definitively, provably, correct so Acquire / Release is sensible. Plus, like you, memory orderings are a bit of a dark art from my point of view.

All in all: good shout.

(*) global_lock_acquisition_count is Atomic partly to provide mutability even in non-&mut Self functions without additional locking

[IsaacWoods]

Cool, thanks Martin! I agree with your reasoning in hindsight that it should be fine given it'll be strictly ordered in relation to the mutex, but I don't think there's any harm in having it like this.