Messages in this thread | | | Date | Mon, 14 Aug 2017 19:38:57 +0100 | From | Mark Brown <> | Subject | Re: [PATCH] spi: spidev: add exclusive bus access lock via ioctls |
| |
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. [unhandled content-type:application/pgp-signature] | |