Re: [PATCH v2 2/8] IB/core: Replace semaphore sm_sem with completion

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday, October 25, 2016 9:48:22 AM CEST Jason Gunthorpe wrote:
> 
> Using a completion to model exclusive ownership seems convoluted to
> me - is that a thing now? What about an atomic?

I also totally missed how this is used. I initially mentioned to Binoy
that almost all semaphores can either be converted to a mutex or a
completion unless they rely on the counting behavior of the semaphore.

This driver clearly falls into another category and none of the above
apply here.

> open:
> 
> while (true) {
>    wait_event_interruptible(priv->queue,test_bit(CLAIMED_BIT, &priv->flags));
>    if (!test_and_set_bit(CLAIMED_BIT, &priv->flags))
>         break;
> }

I'd fold the test_and_set_bit() into the wait_event() and get rid of the
loop here, otherwise this looks nice.

I'm generally a bit suspicious of open() functions that try to
protect you from having multiple file descriptors open, as dup()
or fork() will also result in extra file descriptors, but this
use here seems harmless, as the device itself only supports
open() and close() and the only thing it does is to keep track
of whether it is open or not.

It's also interesting that the ib_modify_port() function also
keeps track of the a flag that is set in open() and cleared
in close(), so in principle we should be able to unify this
with the semaphore, but the wait_event() approach is certainly
much simpler.

	Arnd