[lkml]   [2017]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [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;
> +
> + 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]
 \ /
  Last update: 2017-08-14 21:18    [W:0.040 / U:1.888 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site