linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [GIT PULL] Preparation for BKL'ed ioctl removal
@ 2010-04-16  3:55 Frederic Weisbecker
  2010-04-16  3:58 ` Frederic Weisbecker
  2010-04-19 12:30 ` Jan Blunck
  0 siblings, 2 replies; 6+ messages in thread
From: Frederic Weisbecker @ 2010-04-16  3:55 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: LKML, Frederic Weisbecker, Arnd Bergmann, Thomas Gleixner,
	Al Viro, Jan Blunck, Ingo Molnar

Linus,

In order to stop the bkl contagion in ioctl and push the
bkl from Vfs to the drivers that don't implement a proper
unlock_ioctl, this patch proposes to add a new "locked_ioctl"
field in struct file_operation.

This field is never called from vfs and requires to assign a
new "deprecated_ioctl" helper to the unlocked_ioctl field.
This deprecated_ioctl() helper then calls the locked_ioctl()
callback with the bkl held.

The point of doing this is to change every users of fops::ioctl
to the new scheme. Once there is no more users of the ioctl field,
we'll then be able to remove it.

Also this change brings a new config BKL which is always
enabled for now. Every drivers that use the bkl through
explicit lock_kernel() calls or by using deprecated_ioctl()
will have to select CONFIG_BKL.

Once we get no more uses of the bkl from the core features but
only on individual drivers, config BKL can be turned off
by default and selected by those drivers if needed, relegating
the bkl as an obsolete library for poor old drivers.

And given the work happening currently (tty mutex, vfs pushdowns,
procfs bkl removal, llseek janitorials, etc...), this may happen
sooner than was expected.

Now the point of having this patch in 2.6.34 is to turn
the old ioctl implementations to the new scheme in a set
of patches that relevant maintainers can apply to their
tree (or apply by ourself for unmaintained areas) for .35
Otherwise we would need to queue everything in a single
tree that may bring conflicts in the next merge window.

This single patch has very few chances to bring any regressions.

Please pull the bkl/ioctl branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	bkl/ioctl

Thanks,
	Frederic
---

Arnd Bergmann (1):
      vfs: Introduce CONFIG_BKL and deprecated_ioctl


 fs/ioctl.c               |   22 ++++++++++++++++++++++
 include/linux/fs.h       |    3 +++
 include/linux/smp_lock.h |    4 ++++
 kernel/Kconfig.locks     |   10 ++++++++++
 4 files changed, 39 insertions(+), 0 deletions(-)

---
commit 588a2267f7b2919ae73380e0932cc6a202787036
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Thu Apr 1 14:42:38 2010 +0200

    vfs: Introduce CONFIG_BKL and deprecated_ioctl
    
    This is a preparation for the removal of the big kernel lock that
    introduces new interfaces for device drivers still using it.
    
    We can start marking those device drivers as 'depends on CONFIG_BKL'
    now, and make that symbol optional later, when the point has come
    at which we are able to build a kernel without the BKL.
    
    Similarly, device drivers that currently make use of the implicit
    BKL locking around the ioctl function can now get annotated by
    changing
    
    	.ioctl = foo_ioctl,
    
    to
    
    	.locked_ioctl = foo_ioctl,
    	.unlocked_ioctl = deprecated_ioctl,
    
    As soon as no driver remains using the old ioctl callback, it can
    get removed.
    
    [fweisbec: move config bkl from Kconfig.debug to Kconfig.locks,
    rename default_ioctl to deprecated_ioctl]
    
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6c75110..e6d6a75 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -58,6 +58,28 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
 	return error;
 }
 
+#ifdef CONFIG_BKL
+/*
+ * deprecated_ioctl - call locked_ioctl with BKL held
+ *
+ * Setting only the ioctl operation but not unlocked_ioctl will become
+ * invalid in the future, all drivers that are not converted to unlocked_ioctl
+ * should set .unlocked_ioctl = deprecated_ioctl now.
+ */
+long deprecated_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int error = -ENOTTY;
+	if (filp->f_op->locked_ioctl) {
+		lock_kernel();
+		error = filp->f_op->locked_ioctl(filp->f_path.dentry->d_inode,
+						 filp, cmd, arg);
+		unlock_kernel();
+	}
+	return error;
+}
+EXPORT_SYMBOL_GPL(default_ioctl);
+#endif
+
 static int ioctl_fibmap(struct file *filp, int __user *p)
 {
 	struct address_space *mapping = filp->f_mapping;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 39d57bc..6b65b26 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1492,6 +1492,9 @@ struct file_operations {
 	int (*readdir) (struct file *, void *, filldir_t);
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
 	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
+#ifdef CONFIG_BKL
+	int (*locked_ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
+#endif
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
diff --git a/include/linux/smp_lock.h b/include/linux/smp_lock.h
index 2ea1dd1..c8693e1 100644
--- a/include/linux/smp_lock.h
+++ b/include/linux/smp_lock.h
@@ -62,4 +62,8 @@ static inline void cycle_kernel_lock(void)
 #define kernel_locked()				1
 
 #endif /* CONFIG_LOCK_KERNEL */
+
+loff_t default_llseek(struct file *file, loff_t offset, int origin);
+long deprecated_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+
 #endif /* __LINUX_SMPLOCK_H */
diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index 88c92fb..9a80413 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -200,3 +200,13 @@ config INLINE_WRITE_UNLOCK_IRQRESTORE
 
 config MUTEX_SPIN_ON_OWNER
 	def_bool SMP && !DEBUG_MUTEXES && !HAVE_DEFAULT_NO_SPIN_MUTEXES
+
+config BKL
+	def_bool y
+	help
+	  This is the traditional lock that is used in old code instead
+	  of proper locking. All drivers that use the BKL should depend
+	  on this symbol.
+	  This configuration option will become user-selectable in the
+	  future, as soon as it is possible to build a kernel without
+	  it.

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [GIT PULL] Preparation for BKL'ed ioctl removal
  2010-04-16  3:55 [GIT PULL] Preparation for BKL'ed ioctl removal Frederic Weisbecker
@ 2010-04-16  3:58 ` Frederic Weisbecker
  2010-04-19 12:30 ` Jan Blunck
  1 sibling, 0 replies; 6+ messages in thread
From: Frederic Weisbecker @ 2010-04-16  3:58 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: LKML, Arnd Bergmann, Thomas Gleixner, Al Viro, Jan Blunck

Sorry,

I made a mistake with the recipient in my script :-s


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [GIT PULL] Preparation for BKL'ed ioctl removal
  2010-04-16  3:55 [GIT PULL] Preparation for BKL'ed ioctl removal Frederic Weisbecker
  2010-04-16  3:58 ` Frederic Weisbecker
@ 2010-04-19 12:30 ` Jan Blunck
  2010-04-19 12:52   ` fweisbec
  2010-04-19 14:27   ` Arnd Bergmann
  1 sibling, 2 replies; 6+ messages in thread
From: Jan Blunck @ 2010-04-19 12:30 UTC (permalink / raw)
  To: Frederic Weisbecker
  Cc: Ingo Molnar, LKML, Arnd Bergmann, Thomas Gleixner, Al Viro

On Fri, Apr 16, Frederic Weisbecker wrote:

> Linus,
> 
> In order to stop the bkl contagion in ioctl and push the
> bkl from Vfs to the drivers that don't implement a proper
> unlock_ioctl, this patch proposes to add a new "locked_ioctl"
> field in struct file_operation.
> 
> This field is never called from vfs and requires to assign a
> new "deprecated_ioctl" helper to the unlocked_ioctl field.
> This deprecated_ioctl() helper then calls the locked_ioctl()
> callback with the bkl held.
> 
> The point of doing this is to change every users of fops::ioctl
> to the new scheme. Once there is no more users of the ioctl field,
> we'll then be able to remove it.
> 
> Also this change brings a new config BKL which is always
> enabled for now. Every drivers that use the bkl through
> explicit lock_kernel() calls or by using deprecated_ioctl()
> will have to select CONFIG_BKL.
> 
> Once we get no more uses of the bkl from the core features but
> only on individual drivers, config BKL can be turned off
> by default and selected by those drivers if needed, relegating
> the bkl as an obsolete library for poor old drivers.
> 
> And given the work happening currently (tty mutex, vfs pushdowns,
> procfs bkl removal, llseek janitorials, etc...), this may happen
> sooner than was expected.
> 
> Now the point of having this patch in 2.6.34 is to turn
> the old ioctl implementations to the new scheme in a set
> of patches that relevant maintainers can apply to their
> tree (or apply by ourself for unmaintained areas) for .35
> Otherwise we would need to queue everything in a single
> tree that may bring conflicts in the next merge window.
> 
> This single patch has very few chances to bring any regressions.
> 
> Please pull the bkl/ioctl branch that can be found at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> 	bkl/ioctl
> 
> Thanks,
> 	Frederic
> ---
> 
> Arnd Bergmann (1):
>       vfs: Introduce CONFIG_BKL and deprecated_ioctl
> 
> 
>  fs/ioctl.c               |   22 ++++++++++++++++++++++
>  include/linux/fs.h       |    3 +++
>  include/linux/smp_lock.h |    4 ++++
>  kernel/Kconfig.locks     |   10 ++++++++++
>  4 files changed, 39 insertions(+), 0 deletions(-)
> 
> ---
> commit 588a2267f7b2919ae73380e0932cc6a202787036
> Author: Arnd Bergmann <arnd@arndb.de>
> Date:   Thu Apr 1 14:42:38 2010 +0200
> 
>     vfs: Introduce CONFIG_BKL and deprecated_ioctl
>     
>     This is a preparation for the removal of the big kernel lock that
>     introduces new interfaces for device drivers still using it.
>     
>     We can start marking those device drivers as 'depends on CONFIG_BKL'
>     now, and make that symbol optional later, when the point has come
>     at which we are able to build a kernel without the BKL.
>     
>     Similarly, device drivers that currently make use of the implicit
>     BKL locking around the ioctl function can now get annotated by
>     changing
>     
>     	.ioctl = foo_ioctl,
>     
>     to
>     
>     	.locked_ioctl = foo_ioctl,
>     	.unlocked_ioctl = deprecated_ioctl,
>     
>     As soon as no driver remains using the old ioctl callback, it can
>     get removed.
>     
>     [fweisbec: move config bkl from Kconfig.debug to Kconfig.locks,
>     rename default_ioctl to deprecated_ioctl]
>     
>     Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>     Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 6c75110..e6d6a75 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -58,6 +58,28 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
>  	return error;
>  }
>  
> +#ifdef CONFIG_BKL
> +/*
> + * deprecated_ioctl - call locked_ioctl with BKL held
> + *
> + * Setting only the ioctl operation but not unlocked_ioctl will become
> + * invalid in the future, all drivers that are not converted to unlocked_ioctl
> + * should set .unlocked_ioctl = deprecated_ioctl now.
> + */
> +long deprecated_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	int error = -ENOTTY;
> +	if (filp->f_op->locked_ioctl) {
> +		lock_kernel();
> +		error = filp->f_op->locked_ioctl(filp->f_path.dentry->d_inode,
> +						 filp, cmd, arg);
> +		unlock_kernel();
> +	}
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(default_ioctl);
> +#endif
> +
>  static int ioctl_fibmap(struct file *filp, int __user *p)
>  {
>  	struct address_space *mapping = filp->f_mapping;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 39d57bc..6b65b26 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1492,6 +1492,9 @@ struct file_operations {
>  	int (*readdir) (struct file *, void *, filldir_t);
>  	unsigned int (*poll) (struct file *, struct poll_table_struct *);
>  	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
> +#ifdef CONFIG_BKL
> +	int (*locked_ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
> +#endif
>  	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
>  	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
>  	int (*mmap) (struct file *, struct vm_area_struct *);
> diff --git a/include/linux/smp_lock.h b/include/linux/smp_lock.h
> index 2ea1dd1..c8693e1 100644
> --- a/include/linux/smp_lock.h
> +++ b/include/linux/smp_lock.h
> @@ -62,4 +62,8 @@ static inline void cycle_kernel_lock(void)
>  #define kernel_locked()				1
>  
>  #endif /* CONFIG_LOCK_KERNEL */
> +
> +loff_t default_llseek(struct file *file, loff_t offset, int origin);

Any specific reason why the default llseek() is in this patch?

Jan

> +long deprecated_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
> +
>  #endif /* __LINUX_SMPLOCK_H */
> diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
> index 88c92fb..9a80413 100644
> --- a/kernel/Kconfig.locks
> +++ b/kernel/Kconfig.locks
> @@ -200,3 +200,13 @@ config INLINE_WRITE_UNLOCK_IRQRESTORE
>  
>  config MUTEX_SPIN_ON_OWNER
>  	def_bool SMP && !DEBUG_MUTEXES && !HAVE_DEFAULT_NO_SPIN_MUTEXES
> +
> +config BKL
> +	def_bool y
> +	help
> +	  This is the traditional lock that is used in old code instead
> +	  of proper locking. All drivers that use the BKL should depend
> +	  on this symbol.
> +	  This configuration option will become user-selectable in the
> +	  future, as soon as it is possible to build a kernel without
> +	  it.
Regards,
	Jan

-- 
Jan Blunck <jblunck@suse.de>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [GIT PULL] Preparation for BKL'ed ioctl removal
  2010-04-19 12:30 ` Jan Blunck
@ 2010-04-19 12:52   ` fweisbec
  2010-04-19 14:27   ` Arnd Bergmann
  1 sibling, 0 replies; 6+ messages in thread
From: fweisbec @ 2010-04-19 12:52 UTC (permalink / raw)
  To: Jan Blunck; +Cc: Ingo Molnar, LKML, Arnd Bergmann, Thomas Gleixner, Al Viro

On Mon, Apr 19, 2010 at 02:30:03PM +0200, Jan Blunck wrote:
> On Fri, Apr 16, Frederic Weisbecker wrote:
> 
> > Linus,
> > 
> > In order to stop the bkl contagion in ioctl and push the
> > bkl from Vfs to the drivers that don't implement a proper
> > unlock_ioctl, this patch proposes to add a new "locked_ioctl"
> > field in struct file_operation.
> > 
> > This field is never called from vfs and requires to assign a
> > new "deprecated_ioctl" helper to the unlocked_ioctl field.
> > This deprecated_ioctl() helper then calls the locked_ioctl()
> > callback with the bkl held.
> > 
> > The point of doing this is to change every users of fops::ioctl
> > to the new scheme. Once there is no more users of the ioctl field,
> > we'll then be able to remove it.
> > 
> > Also this change brings a new config BKL which is always
> > enabled for now. Every drivers that use the bkl through
> > explicit lock_kernel() calls or by using deprecated_ioctl()
> > will have to select CONFIG_BKL.
> > 
> > Once we get no more uses of the bkl from the core features but
> > only on individual drivers, config BKL can be turned off
> > by default and selected by those drivers if needed, relegating
> > the bkl as an obsolete library for poor old drivers.
> > 
> > And given the work happening currently (tty mutex, vfs pushdowns,
> > procfs bkl removal, llseek janitorials, etc...), this may happen
> > sooner than was expected.
> > 
> > Now the point of having this patch in 2.6.34 is to turn
> > the old ioctl implementations to the new scheme in a set
> > of patches that relevant maintainers can apply to their
> > tree (or apply by ourself for unmaintained areas) for .35
> > Otherwise we would need to queue everything in a single
> > tree that may bring conflicts in the next merge window.
> > 
> > This single patch has very few chances to bring any regressions.
> > 
> > Please pull the bkl/ioctl branch that can be found at:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> > 	bkl/ioctl
> > 
> > Thanks,
> > 	Frederic
> > ---
> > 
> > Arnd Bergmann (1):
> >       vfs: Introduce CONFIG_BKL and deprecated_ioctl
> > 
> > 
> >  fs/ioctl.c               |   22 ++++++++++++++++++++++
> >  include/linux/fs.h       |    3 +++
> >  include/linux/smp_lock.h |    4 ++++
> >  kernel/Kconfig.locks     |   10 ++++++++++
> >  4 files changed, 39 insertions(+), 0 deletions(-)
> > 
> > ---
> > commit 588a2267f7b2919ae73380e0932cc6a202787036
> > Author: Arnd Bergmann <arnd@arndb.de>
> > Date:   Thu Apr 1 14:42:38 2010 +0200
> > 
> >     vfs: Introduce CONFIG_BKL and deprecated_ioctl
> >     
> >     This is a preparation for the removal of the big kernel lock that
> >     introduces new interfaces for device drivers still using it.
> >     
> >     We can start marking those device drivers as 'depends on CONFIG_BKL'
> >     now, and make that symbol optional later, when the point has come
> >     at which we are able to build a kernel without the BKL.
> >     
> >     Similarly, device drivers that currently make use of the implicit
> >     BKL locking around the ioctl function can now get annotated by
> >     changing
> >     
> >     	.ioctl = foo_ioctl,
> >     
> >     to
> >     
> >     	.locked_ioctl = foo_ioctl,
> >     	.unlocked_ioctl = deprecated_ioctl,
> >     
> >     As soon as no driver remains using the old ioctl callback, it can
> >     get removed.
> >     
> >     [fweisbec: move config bkl from Kconfig.debug to Kconfig.locks,
> >     rename default_ioctl to deprecated_ioctl]
> >     
> >     Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >     Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > 
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 6c75110..e6d6a75 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -58,6 +58,28 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
> >  	return error;
> >  }
> >  
> > +#ifdef CONFIG_BKL
> > +/*
> > + * deprecated_ioctl - call locked_ioctl with BKL held
> > + *
> > + * Setting only the ioctl operation but not unlocked_ioctl will become
> > + * invalid in the future, all drivers that are not converted to unlocked_ioctl
> > + * should set .unlocked_ioctl = deprecated_ioctl now.
> > + */
> > +long deprecated_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > +{
> > +	int error = -ENOTTY;
> > +	if (filp->f_op->locked_ioctl) {
> > +		lock_kernel();
> > +		error = filp->f_op->locked_ioctl(filp->f_path.dentry->d_inode,
> > +						 filp, cmd, arg);
> > +		unlock_kernel();
> > +	}
> > +	return error;
> > +}
> > +EXPORT_SYMBOL_GPL(default_ioctl);
> > +#endif
> > +
> >  static int ioctl_fibmap(struct file *filp, int __user *p)
> >  {
> >  	struct address_space *mapping = filp->f_mapping;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 39d57bc..6b65b26 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1492,6 +1492,9 @@ struct file_operations {
> >  	int (*readdir) (struct file *, void *, filldir_t);
> >  	unsigned int (*poll) (struct file *, struct poll_table_struct *);
> >  	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
> > +#ifdef CONFIG_BKL
> > +	int (*locked_ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
> > +#endif
> >  	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
> >  	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
> >  	int (*mmap) (struct file *, struct vm_area_struct *);
> > diff --git a/include/linux/smp_lock.h b/include/linux/smp_lock.h
> > index 2ea1dd1..c8693e1 100644
> > --- a/include/linux/smp_lock.h
> > +++ b/include/linux/smp_lock.h
> > @@ -62,4 +62,8 @@ static inline void cycle_kernel_lock(void)
> >  #define kernel_locked()				1
> >  
> >  #endif /* CONFIG_LOCK_KERNEL */
> > +
> > +loff_t default_llseek(struct file *file, loff_t offset, int origin);
> 
> Any specific reason why the default llseek() is in this patch?
> 
> Jan


Because this comes along the same kind of bkl preparation work
we want to do. The goal is to isolate default_llseek and deprecated_ioctl
and only enable them if CONFIG_BKL.

Hence we want their declaration in smp_lock.h

That said it is probably not needed to get the default_llseek declaration
here for this release.

I'm going to make a v2 and resend the pull request. Thanks.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [GIT PULL] Preparation for BKL'ed ioctl removal
  2010-04-19 12:30 ` Jan Blunck
  2010-04-19 12:52   ` fweisbec
@ 2010-04-19 14:27   ` Arnd Bergmann
  1 sibling, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2010-04-19 14:27 UTC (permalink / raw)
  To: Jan Blunck
  Cc: Frederic Weisbecker, Ingo Molnar, LKML, Thomas Gleixner, Al Viro

On Monday 19 April 2010, Jan Blunck wrote:
> > --- a/include/linux/smp_lock.h
> > +++ b/include/linux/smp_lock.h
> > @@ -62,4 +62,8 @@ static inline void cycle_kernel_lock(void)
> >  #define kernel_locked()                              1
> >  
> >  #endif /* CONFIG_LOCK_KERNEL */
> > +
> > +loff_t default_llseek(struct file *file, loff_t offset, int origin);
> 
> Any specific reason why the default llseek() is in this patch?

A later patch moves both default_llseek and deprecated_ioctl into the new
bkl.ko module, while this is just the minimal preparation for getting
all the users changed.

It was intentional but not important to do the move now.

	Arnd

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [GIT PULL] Preparation for BKL'ed ioctl removal
@ 2010-04-16  3:56 Frederic Weisbecker
  0 siblings, 0 replies; 6+ messages in thread
From: Frederic Weisbecker @ 2010-04-16  3:56 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: LKML, Frederic Weisbecker, Arnd Bergmann, Thomas Gleixner,
	Al Viro, Jan Blunck, Ingo Molnar

Linus,

In order to stop the bkl contagion in ioctl and push the
bkl from Vfs to the drivers that don't implement a proper
unlock_ioctl, this patch proposes to add a new "locked_ioctl"
field in struct file_operation.

This field is never called from vfs and requires to assign a
new "deprecated_ioctl" helper to the unlocked_ioctl field.
This deprecated_ioctl() helper then calls the locked_ioctl()
callback with the bkl held.

The point of doing this is to change every users of fops::ioctl
to the new scheme. Once there is no more users of the ioctl field,
we'll then be able to remove it.

Also this change brings a new config BKL which is always
enabled for now. Every drivers that use the bkl through
explicit lock_kernel() calls or by using deprecated_ioctl()
will have to select CONFIG_BKL.

Once we get no more uses of the bkl from the core features but
only on individual drivers, config BKL can be turned off
by default and selected by those drivers if needed, relegating
the bkl as an obsolete library for poor old drivers.

And given the work happening currently (tty mutex, vfs pushdowns,
procfs bkl removal, llseek janitorials, etc...), this may happen
sooner than was expected.

Now the point of having this patch in 2.6.34 is to turn
the old ioctl implementations to the new scheme in a set
of patches that relevant maintainers can apply to their
tree (or apply by ourself for unmaintained areas) for .35
Otherwise we would need to queue everything in a single
tree that may bring conflicts in the next merge window.

This single patch has very few chances to bring any regressions.

Please pull the bkl/ioctl branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	bkl/ioctl

Thanks,
	Frederic
---

Arnd Bergmann (1):
      vfs: Introduce CONFIG_BKL and deprecated_ioctl


 fs/ioctl.c               |   22 ++++++++++++++++++++++
 include/linux/fs.h       |    3 +++
 include/linux/smp_lock.h |    4 ++++
 kernel/Kconfig.locks     |   10 ++++++++++
 4 files changed, 39 insertions(+), 0 deletions(-)

---
commit 588a2267f7b2919ae73380e0932cc6a202787036
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Thu Apr 1 14:42:38 2010 +0200

    vfs: Introduce CONFIG_BKL and deprecated_ioctl
    
    This is a preparation for the removal of the big kernel lock that
    introduces new interfaces for device drivers still using it.
    
    We can start marking those device drivers as 'depends on CONFIG_BKL'
    now, and make that symbol optional later, when the point has come
    at which we are able to build a kernel without the BKL.
    
    Similarly, device drivers that currently make use of the implicit
    BKL locking around the ioctl function can now get annotated by
    changing
    
    	.ioctl = foo_ioctl,
    
    to
    
    	.locked_ioctl = foo_ioctl,
    	.unlocked_ioctl = deprecated_ioctl,
    
    As soon as no driver remains using the old ioctl callback, it can
    get removed.
    
    [fweisbec: move config bkl from Kconfig.debug to Kconfig.locks,
    rename default_ioctl to deprecated_ioctl]
    
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6c75110..e6d6a75 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -58,6 +58,28 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
 	return error;
 }
 
+#ifdef CONFIG_BKL
+/*
+ * deprecated_ioctl - call locked_ioctl with BKL held
+ *
+ * Setting only the ioctl operation but not unlocked_ioctl will become
+ * invalid in the future, all drivers that are not converted to unlocked_ioctl
+ * should set .unlocked_ioctl = deprecated_ioctl now.
+ */
+long deprecated_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int error = -ENOTTY;
+	if (filp->f_op->locked_ioctl) {
+		lock_kernel();
+		error = filp->f_op->locked_ioctl(filp->f_path.dentry->d_inode,
+						 filp, cmd, arg);
+		unlock_kernel();
+	}
+	return error;
+}
+EXPORT_SYMBOL_GPL(default_ioctl);
+#endif
+
 static int ioctl_fibmap(struct file *filp, int __user *p)
 {
 	struct address_space *mapping = filp->f_mapping;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 39d57bc..6b65b26 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1492,6 +1492,9 @@ struct file_operations {
 	int (*readdir) (struct file *, void *, filldir_t);
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
 	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
+#ifdef CONFIG_BKL
+	int (*locked_ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
+#endif
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
diff --git a/include/linux/smp_lock.h b/include/linux/smp_lock.h
index 2ea1dd1..c8693e1 100644
--- a/include/linux/smp_lock.h
+++ b/include/linux/smp_lock.h
@@ -62,4 +62,8 @@ static inline void cycle_kernel_lock(void)
 #define kernel_locked()				1
 
 #endif /* CONFIG_LOCK_KERNEL */
+
+loff_t default_llseek(struct file *file, loff_t offset, int origin);
+long deprecated_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+
 #endif /* __LINUX_SMPLOCK_H */
diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index 88c92fb..9a80413 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -200,3 +200,13 @@ config INLINE_WRITE_UNLOCK_IRQRESTORE
 
 config MUTEX_SPIN_ON_OWNER
 	def_bool SMP && !DEBUG_MUTEXES && !HAVE_DEFAULT_NO_SPIN_MUTEXES
+
+config BKL
+	def_bool y
+	help
+	  This is the traditional lock that is used in old code instead
+	  of proper locking. All drivers that use the BKL should depend
+	  on this symbol.
+	  This configuration option will become user-selectable in the
+	  future, as soon as it is possible to build a kernel without
+	  it.

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2010-04-19 14:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-16  3:55 [GIT PULL] Preparation for BKL'ed ioctl removal Frederic Weisbecker
2010-04-16  3:58 ` Frederic Weisbecker
2010-04-19 12:30 ` Jan Blunck
2010-04-19 12:52   ` fweisbec
2010-04-19 14:27   ` Arnd Bergmann
2010-04-16  3:56 Frederic Weisbecker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).