Messages in this thread | | | Date | Fri, 23 May 2008 12:23:09 +0200 | From | Stefan Richter <> | Subject | Re: [PATCH] raw1394: Push the BKL down into the driver ioctls |
| |
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?
If not, we can immediately remove lock_kernel() from raw1394.
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.
I have another minor comment below:
> diff --git a/drivers/ieee1394/raw1394.c b/drivers/ieee1394/raw1394.c > index ec2a0ad..8ec0278 100644 > --- a/drivers/ieee1394/raw1394.c > +++ b/drivers/ieee1394/raw1394.c > @@ -2549,8 +2549,8 @@ static int raw1394_mmap(struct file *file, struct vm_area_struct *vma) > } > > /* ioctl is only used for rawiso operations */ > -static int raw1394_ioctl(struct inode *inode, struct file *file, > - unsigned int cmd, unsigned long arg) > +static long do_raw1394_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > { > struct file_info *fi = file->private_data; > void __user *argp = (void __user *)arg; > @@ -2656,6 +2656,16 @@ static int raw1394_ioctl(struct inode *inode, struct file *file, > return -EINVAL; > } > > +static long raw1394_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + long ret; > + lock_kernel(); > + ret = do_raw1394_ioctl(file, cmd, arg); > + unlock_kernel(); > + return ret; > +} > + > #ifdef CONFIG_COMPAT > struct raw1394_iso_packets32 { > __u32 n_packets; > @@ -2690,7 +2700,7 @@ static long raw1394_iso_xmit_recv_packets32(struct file *file, unsigned int cmd, > !copy_from_user(&infos32, &arg->infos, sizeof infos32)) { > infos = compat_ptr(infos32); > if (!copy_to_user(&dst->infos, &infos, sizeof infos)) > - err = raw1394_ioctl(NULL, file, cmd, (unsigned long)dst); > + err = do_raw1394_ioctl(file, cmd, (unsigned long)dst); > } > return err; > }
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.
> @@ -2984,7 +2994,7 @@ static const struct file_operations raw1394_fops = { > .read = raw1394_read, > .write = raw1394_write, > .mmap = raw1394_mmap, > - .ioctl = raw1394_ioctl, > + .unlocked_ioctl = raw1394_ioctl, > #ifdef CONFIG_COMPAT > .compat_ioctl = raw1394_compat_ioctl, > #endif
-- Stefan Richter -=====-==--- -=-= =-=== http://arcgraph.de/sr/
| |