autofs: Pushdown the bkl from ioctl
in [Kernel]
|
Prev: [PATCH] xen: ensure timer tick is resumed even on CPU driving the resume
Next: mm: Move ARCH_SLAB_MINALIGN and ARCH_KMALLOC_MINALIGN to <linux/slab_def.h>
From: H. Peter Anvin on 19 May 2010 14:10 On 05/19/2010 10:24 AM, Frederic Weisbecker wrote: > * generate kernel reactions > */ > -static int autofs_root_ioctl(struct inode *inode, struct file *filp, > +static int autofs_root_ioctl_unlocked(struct inode *inode, struct file *filp, > unsigned int cmd, unsigned long arg) > { > struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb); > @@ -579,3 +579,16 @@ static int autofs_root_ioctl(struct inode *inode, struct file *filp, > return -ENOSYS; > } > } > + > +static long autofs_root_ioctl(struct file *filp, > + unsigned int cmd, unsigned long arg) > +{ The choice of naming here seems reverse in my opinion. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 19 May 2010 14:10 On Wed, May 19, 2010 at 11:02:04AM -0700, H. Peter Anvin wrote: > On 05/19/2010 10:24 AM, Frederic Weisbecker wrote: > > * generate kernel reactions > > */ > > -static int autofs_root_ioctl(struct inode *inode, struct file *filp, > > +static int autofs_root_ioctl_unlocked(struct inode *inode, struct file *filp, > > unsigned int cmd, unsigned long arg) > > { > > struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb); > > @@ -579,3 +579,16 @@ static int autofs_root_ioctl(struct inode *inode, struct file *filp, > > return -ENOSYS; > > } > > } > > + > > +static long autofs_root_ioctl(struct file *filp, > > + unsigned int cmd, unsigned long arg) > > +{ > > The choice of naming here seems reverse in my opinion. Oh, why? The function that holds the bkl calls its unlocked version. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 19 May 2010 14:30 On 05/19/2010 11:08 AM, Frederic Weisbecker wrote: > On Wed, May 19, 2010 at 11:02:04AM -0700, H. Peter Anvin wrote: >> On 05/19/2010 10:24 AM, Frederic Weisbecker wrote: >>> * generate kernel reactions >>> */ >>> -static int autofs_root_ioctl(struct inode *inode, struct file *filp, >>> +static int autofs_root_ioctl_unlocked(struct inode *inode, struct file *filp, >>> unsigned int cmd, unsigned long arg) >>> { >>> struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb); >>> @@ -579,3 +579,16 @@ static int autofs_root_ioctl(struct inode *inode, struct file *filp, >>> return -ENOSYS; >>> } >>> } >>> + >>> +static long autofs_root_ioctl(struct file *filp, >>> + unsigned int cmd, unsigned long arg) >>> +{ >> >> The choice of naming here seems reverse in my opinion. > > > Oh, why? > > The function that holds the bkl calls its unlocked version. > But it's not ... it is locked at that point. It's not lock*ing*, but it is not *unlocked*, either. Furthermore, it is directly contradicting the naming scheme of the ops structure. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 19 May 2010 14:30 On Wed, May 19, 2010 at 11:13:50AM -0700, H. Peter Anvin wrote: > On 05/19/2010 11:08 AM, Frederic Weisbecker wrote: > > On Wed, May 19, 2010 at 11:02:04AM -0700, H. Peter Anvin wrote: > >> On 05/19/2010 10:24 AM, Frederic Weisbecker wrote: > >>> * generate kernel reactions > >>> */ > >>> -static int autofs_root_ioctl(struct inode *inode, struct file *filp, > >>> +static int autofs_root_ioctl_unlocked(struct inode *inode, struct file *filp, > >>> unsigned int cmd, unsigned long arg) > >>> { > >>> struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb); > >>> @@ -579,3 +579,16 @@ static int autofs_root_ioctl(struct inode *inode, struct file *filp, > >>> return -ENOSYS; > >>> } > >>> } > >>> + > >>> +static long autofs_root_ioctl(struct file *filp, > >>> + unsigned int cmd, unsigned long arg) > >>> +{ > >> > >> The choice of naming here seems reverse in my opinion. > > > > > > Oh, why? > > > > The function that holds the bkl calls its unlocked version. > > > > But it's not ... it is locked at that point. It's not lock*ing*, but it > is not *unlocked*, either. Furthermore, it is directly contradicting > the naming scheme of the ops structure. It depends on the point of view. The function itself doesn't lock, it is the "naked point", so if you take it, you need to lock before, that's what the name wants to tell. But may be that's the opposite point of view than the common one, for which I wouldn't be suprised as my brain tends to be upside down... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 19 May 2010 15:10
On Wed, May 19, 2010 at 11:13:50AM -0700, H. Peter Anvin wrote: > On 05/19/2010 11:08 AM, Frederic Weisbecker wrote: > > On Wed, May 19, 2010 at 11:02:04AM -0700, H. Peter Anvin wrote: > >> On 05/19/2010 10:24 AM, Frederic Weisbecker wrote: > >>> * generate kernel reactions > >>> */ > >>> -static int autofs_root_ioctl(struct inode *inode, struct file *filp, > >>> +static int autofs_root_ioctl_unlocked(struct inode *inode, struct file *filp, > >>> unsigned int cmd, unsigned long arg) > >>> { > >>> struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb); > >>> @@ -579,3 +579,16 @@ static int autofs_root_ioctl(struct inode *inode, struct file *filp, > >>> return -ENOSYS; > >>> } > >>> } > >>> + > >>> +static long autofs_root_ioctl(struct file *filp, > >>> + unsigned int cmd, unsigned long arg) > >>> +{ > >> > >> The choice of naming here seems reverse in my opinion. > > > > > > Oh, why? > > > > The function that holds the bkl calls its unlocked version. > > > > But it's not ... it is locked at that point. It's not lock*ing*, but it > is not *unlocked*, either. Furthermore, it is directly contradicting > the naming scheme of the ops structure. > > -hpa > Would you prefer me to resend a patch? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |