Re: [PATCH] spi: spidev: add exclusive bus access lock via ioctls

[Mark Brown]

On Sat, Aug 12, 2017 at 05:24:03PM +0530, Vikram N wrote:

>  	else
> -		status = spi_sync(spi, message);
> +		status = spidev->bus_locked ? spi_sync_locked(spi, message) :
> +				spi_sync(spi, message);

Please don't abuse the ternery operator, people need to be able to read
the code.

> +	case SPI_IOC_BUS_LOCK:
> +		spi_bus_lock(spi->master);
> +		spidev->bus_locked = true;
> +		break;
> +
> +	case SPI_IOC_BUS_UNLOCK:
> +		spi_bus_unlock(spi->master);
> +		spidev->bus_locked = false;
> +		break;

I'm not super convinced that this API is a good idea in general - it
seems extremely niche to be using multiple userspace programs that don't
need to coordinate at all except for a single lock (which they will all
need to use to avoid just bouncing off with errors).  That all seems
very narrow.

I'm also worried that even if there is such a use case this code is very
fragile as it stands.  If an application crashes then nothing will free
a lock it holds and any application can through simple error drop locks
that are supposed to be held by other applications.  This isn't going to
be terribly robust.

Attachment: signature.asc
Description: PGP signature