Re: [PATCH] raw1394: Push the BKL down into the driver ioctls

[Alan Cox <alan@lxorguk.ukuu.org.uk>]

On Fri, 23 May 2008 12:23:09 +0200
Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Alan Cox wrote:
> > Actually in this case wrap the function for now.
> > 
> > Signed-off-by: Alan Cox <alan@redhat.com>
> 
> Can an .unlocked_ioctl() preempt another .unlocked_ioctl() to the very
> same instance of struct file?

Yes. And this btw is true even with the old locked ioctl call if you ever
sleep (eg a copy_to/from_user).

> If yes, we need to serialize do_raw1394_ioctl against itself or come up
> with another protection against concurrent fi->iso_state switches before
> we can remove lock_kernel().  And if a .write() can preempt another
> .write() to the same instance of struct file, raw1394_write() already
> has a problem with concurrent fi->state switches.

Quite a few drivers end up with a private mutex and do mutex_lock/unlock
around the ioctl and write paths (and if the write path can be slow using
_trylock and O_NDELAY check when appropriate).

The goal of pushing it down is to enable driver authors to see and to do
the locking at a driver level instead - plus fix lots of bugs where the
BKL "sleep and drop" behaviour isn't anticipated.

> The same s/raw1394_ioctl/do_raw1394_ioctl/ should be done in
> raw1394_compat_ioctl().  But I suppose it doesn't really matter because
> lock_kernel() is allowed to nest.

Agreed.

Alan