login
Search
These are bkl pushdowns from Arnd that conflicted with
Linus's "Preparation for BKL'ed ioctl removal patch". I fixed-up the merge
conflicts. In addition - during allyesconfig compile testing on x86_64 I found
a number of issues that I fixed-up
I pushed these to my own new linus-bkl tree.
git://www.kernel.org/pub/scm/linux/kernel/git/jkacur/jk-2.6.git linux-bkl
Frederic, if you want to grab those, pls do so. also, we might want to combine
my compile fixes with Arnd's push-down patches, for better git bisectability.
Presented separately here for everyone's easy perusal.
Arnd Bergmann (6):
dvb: push down BKL into ioctl functions
scsi: push down BKL into ioctl functions
isdn: push down BKL into ioctl functions
staging: push down BKL into ioctl functions
v4l: always use unlocked_ioctl
drivers: push down BKL into various drivers
John Kacur (4):
bkl: Fix up compile problems in megaraid from bkl push-down
bkl: Fix missing inode tw_chrdev_ioctl due to bkl pushdown
bkl: Fix missing inode in twl_chrdev_ioctl resulting from bkl
pushdown
bkl: Fix-up compile problems as a result of the bkl-pushdown.
drivers/block/pktcdvd.c | 13 ++++++--
drivers/char/apm-emulation.c | 8 +++--
drivers/char/applicom.c | 13 +++++---
drivers/char/ds1620.c | 16 ++++++++-
drivers/char/dtlk.c | 15 +++++----
drivers/char/generic_nvram.c | 17 ++++++++--
drivers/char/genrtc.c | 16 ++++++++-
drivers/char/hpet.c | 14 +++++---
drivers/char/i8k.c | 21 ++++++++++--
drivers/char/ipmi/ipmi_devintf.c | 26 +++++++++++++---
drivers/char/ipmi/ipmi_watchdog.c | 17 +++++++++-
drivers/char/nvram.c | 10 ++++--
drivers/char/nwflash.c | 7 +++-
drivers/char/raw.c | 42 ...From: Arnd Bergmann <arnd@arndb.de>
This requires changing all users of dvb_usercopy to
omit the inode argument.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixed merge conflicts that result from having applied
Linus's Preparation for BKL'ed ioctl removal patch first.
Signed-off-by: John Kacur <jkacur@redhat.com>
---
drivers/media/dvb/dvb-core/dmxdev.c | 31 +++++++++++++++++++-------
drivers/media/dvb/dvb-core/dvb_ca_en50221.c | 17 ++++++++++----
drivers/media/dvb/dvb-core/dvb_frontend.c | 30 +++++++++++++-------------
drivers/media/dvb/dvb-core/dvb_net.c | 15 +++++++++---
drivers/media/dvb/dvb-core/dvbdev.c | 17 +++++++++-----
drivers/media/dvb/dvb-core/dvbdev.h | 11 +++------
drivers/media/dvb/firewire/firedtv-ci.c | 5 +--
drivers/media/dvb/ttpci/av7110.c | 4 +-
drivers/media/dvb/ttpci/av7110_av.c | 8 +++---
drivers/media/dvb/ttpci/av7110_ca.c | 5 +--
10 files changed, 85 insertions(+), 58 deletions(-)
diff --git a/drivers/media/dvb/dvb-core/dmxdev.c b/drivers/media/dvb/dvb-core/dmxdev.c
index a6c4f2f..425862f 100644
--- a/drivers/media/dvb/dvb-core/dmxdev.c
+++ b/drivers/media/dvb/dvb-core/dmxdev.c
@@ -25,6 +25,7 @@
#include <linux/slab.h>
#include <linux/vmalloc.h>
#include <linux/module.h>
+#include <linux/smp_lock.h>
#include <linux/poll.h>
#include <linux/ioctl.h>
#include <linux/wait.h>
@@ -963,7 +964,7 @@ dvb_demux_read(struct file *file, char __user *buf, size_t count,
return ret;
}
-static int dvb_demux_do_ioctl(struct inode *inode, struct file *file,
+static int dvb_demux_do_ioctl(struct file *file,
unsigned int cmd, void *parg)
{
struct dmxdev_filter *dmxdevfilter = file->private_data;
@@ -1084,10 +1085,16 @@ static int dvb_demux_do_ioctl(struct inode *inode, struct file *file,
return ret;
}
-static int dvb_demux_ioctl(struct inode *inode, struct file *file,
- unsigned int cmd, unsigned long arg)
+static long ...Signed-off-by: John Kacur <jkacur@redhat.com> --- drivers/scsi/3w-xxxx.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c index 45a737c..2957691 100644 --- a/drivers/scsi/3w-xxxx.c +++ b/drivers/scsi/3w-xxxx.c @@ -890,6 +890,7 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a unsigned long data_buffer_length_adjusted = 0; unsigned long *cpu_addr; long timeout; + struct inode *inode = file->f_dentry->d_inode; TW_New_Ioctl *tw_ioctl; TW_Passthru *passthru; TW_Device_Extension *tw_dev = tw_device_extension_list[iminor(inode)]; -- 1.6.6.1 --
Fix missing inode in twl_chrdev_ioctl resulting from bkl pushdown, because the ioctl is now an unlocked ioctl. Signed-off-by: John Kacur <jkacur@redhat.com> --- drivers/scsi/3w-sas.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c index ab4ad09..216af13 100644 --- a/drivers/scsi/3w-sas.c +++ b/drivers/scsi/3w-sas.c @@ -756,6 +756,7 @@ static long twl_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long unsigned long *cpu_addr, data_buffer_length_adjusted = 0, flags = 0; dma_addr_t dma_handle; int request_id = 0; + struct inode *inode = file->f_dentry->d_inode; TW_Ioctl_Driver_Command driver_command; TW_Ioctl_Buf_Apache *tw_ioctl; TW_Command_Full *full_command_packet; -- 1.6.6.1 --
From: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixed merge conflicts that result from having applied
Linus's Preparation for BKL'ed ioctl removal patch first.
Signed-off-by: John Kacur <jkacur@redhat.com>
---
drivers/scsi/3w-9xxx.c | 10 +++++++---
drivers/scsi/3w-sas.c | 7 +++++--
drivers/scsi/3w-xxxx.c | 10 +++++++---
drivers/scsi/aacraid/linit.c | 11 ++++++++---
drivers/scsi/dpt_i2o.c | 20 +++++++++++++++++---
drivers/scsi/gdth.c | 20 +++++++++++++++-----
drivers/scsi/megaraid.c | 20 +++++++++++++++++---
drivers/scsi/megaraid/megaraid_mm.c | 22 +++++++++++++++++-----
drivers/scsi/osst.c | 14 ++++++++++----
drivers/scsi/sg.c | 17 ++++++++++++++---
10 files changed, 117 insertions(+), 34 deletions(-)
diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index 32da0ef..4f74850 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -123,7 +123,7 @@ static void twa_aen_queue_event(TW_Device_Extension *tw_dev, TW_Command_Apache_H
static int twa_aen_read_queue(TW_Device_Extension *tw_dev, int request_id);
static char *twa_aen_severity_lookup(unsigned char severity_code);
static void twa_aen_sync_time(TW_Device_Extension *tw_dev, int request_id);
-static int twa_chrdev_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg);
+static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
static int twa_chrdev_open(struct inode *inode, struct file *file);
static int twa_fill_sense(TW_Device_Extension *tw_dev, int request_id, int copy_sense, int print_host);
static void twa_free_request_id(TW_Device_Extension *tw_dev,int request_id);
@@ -218,7 +218,7 @@ static struct device_attribute *twa_host_attrs[] = {
/* File operations struct for character device */
static const struct file_operations twa_fops = {
.owner = ...From: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixed merge conflicts that result from having applied
Linus's Preparation for BKL'ed ioctl removal patch first.
Signed-off-by: John Kacur <jkacur@redhat.com>
---
drivers/isdn/capi/capi.c | 17 ++++++++++++++---
drivers/isdn/divert/divert_procfs.c | 19 ++++++++++++++++---
drivers/isdn/i4l/isdn_common.c | 18 +++++++++++++++---
drivers/isdn/mISDN/timerdev.c | 10 ++++++----
4 files changed, 51 insertions(+), 13 deletions(-)
diff --git a/drivers/isdn/capi/capi.c b/drivers/isdn/capi/capi.c
index 6bc9766..0cabe31 100644
--- a/drivers/isdn/capi/capi.c
+++ b/drivers/isdn/capi/capi.c
@@ -787,8 +787,7 @@ capi_poll(struct file *file, poll_table * wait)
}
static int
-capi_ioctl(struct inode *inode, struct file *file,
- unsigned int cmd, unsigned long arg)
+capi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
struct capidev *cdev = file->private_data;
capi_ioctl_struct data;
@@ -981,6 +980,18 @@ register_out:
}
}
+static long
+capi_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ int ret;
+
+ lock_kernel();
+ ret = capi_ioctl(file, cmd, arg);
+ unlock_kernel();
+
+ return ret;
+}
+
static int capi_open(struct inode *inode, struct file *file)
{
struct capidev *cdev;
@@ -1026,7 +1037,7 @@ static const struct file_operations capi_fops =
.read = capi_read,
.write = capi_write,
.poll = capi_poll,
- .bkl_ioctl = capi_ioctl,
+ .unlocked_ioctl = capi_unlocked_ioctl,
.open = capi_open,
.release = capi_release,
};
diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
index ae0244f..8084557 100644
--- a/drivers/isdn/divert/divert_procfs.c
+++ b/drivers/isdn/divert/divert_procfs.c
@@ -11,6 +11,7 @@
#include <linux/module.h>
#include <linux/poll.h>
+#include <linux/smp_lock.h>
#include <linux/slab.h>
#ifdef CONFIG_PROC_FS
#include ...From: Arnd Bergmann <arnd@arndb.de>
v4l drivers may still use a locked ioctl method,
but we now always export an unlocked_ioctl and
lock ourselves, so that the ->ioctl file operation
can get removed.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixed merge conflicts that result from having applied
Linus's Preparation for BKL'ed ioctl removal patch first.
Signed-off-by: John Kacur <jkacur@redhat.com>
---
drivers/media/video/v4l2-dev.c | 17 ++++++++++++++---
1 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index c48143a..3606694 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -22,6 +22,7 @@
#include <linux/mm.h>
#include <linux/string.h>
#include <linux/errno.h>
+#include <linux/smp_lock.h>
#include <linux/init.h>
#include <linux/kmod.h>
#include <linux/slab.h>
@@ -215,16 +216,21 @@ static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll)
return vdev->fops->poll(filp, poll);
}
-static int v4l2_ioctl(struct inode *inode, struct file *filp,
+static long v4l2_ioctl(struct file *filp,
unsigned int cmd, unsigned long arg)
{
struct video_device *vdev = video_devdata(filp);
+ int ret;
if (!vdev->fops->ioctl)
return -ENOTTY;
/* Allow ioctl to continue even if the device was unregistered.
Things like dequeueing buffers might still be useful. */
- return vdev->fops->ioctl(filp, cmd, arg);
+ lock_kernel();
+ ret = vdev->fops->ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg);
+ unlock_kernel();
+
+ return ret;
}
static long v4l2_unlocked_ioctl(struct file *filp,
@@ -323,6 +329,11 @@ static const struct file_operations v4l2_unlocked_fops = {
.llseek = no_llseek,
};
+/*
+ * Note: this should not be needed, just check
+ * both pointers in v4l2_ioctl, or kill
+ * fops->ioctl. -arnd
+ */
static const struct file_operations v4l2_fops = {
.owner = THIS_MODULE,
...From: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixed merge conflicts that result from having applied
Linus's Preparation for BKL'ed ioctl removal patch first.
Signed-off-by: John Kacur <jkacur@redhat.com>
---
drivers/staging/crystalhd/crystalhd_lnx.c | 13 +++++++++----
drivers/staging/dt3155/dt3155_drv.c | 24 ++++++++++++++++++------
drivers/staging/poch/poch.c | 17 +++++++++++++++--
drivers/staging/vme/devices/vme_user.c | 18 +++++++++++++++---
4 files changed, 57 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/crystalhd/crystalhd_lnx.c b/drivers/staging/crystalhd/crystalhd_lnx.c
index 3d6b773..3291e14 100644
--- a/drivers/staging/crystalhd/crystalhd_lnx.c
+++ b/drivers/staging/crystalhd/crystalhd_lnx.c
@@ -16,6 +16,7 @@
***************************************************************************/
#include <linux/version.h>
+#include <linux/smp_lock.h>
#include <linux/slab.h>
#include "crystalhd_lnx.h"
@@ -261,12 +262,12 @@ static int chd_dec_api_cmd(struct crystalhd_adp *adp, unsigned long ua,
}
/* API interfaces */
-static int chd_dec_ioctl(struct inode *in, struct file *fd,
- unsigned int cmd, unsigned long ua)
+static long chd_dec_ioctl(struct file *fd, unsigned int cmd, unsigned long ua)
{
struct crystalhd_adp *adp = chd_get_adp();
crystalhd_cmd_proc cproc;
struct crystalhd_user *uc;
+ int ret;
if (!adp || !fd) {
BCMLOG_ERR("Invalid adp\n");
@@ -279,13 +280,17 @@ static int chd_dec_ioctl(struct inode *in, struct file *fd,
return -ENODATA;
}
+ lock_kernel();
cproc = crystalhd_get_cmd_proc(&adp->cmds, cmd, uc);
if (!cproc) {
BCMLOG_ERR("Unhandled command: %d\n", cmd);
+ unlock_kernel();
return -EINVAL;
}
- return chd_dec_api_cmd(adp, ua, uc->uid, cmd, cproc);
+ ret = chd_dec_api_cmd(adp, ua, uc->uid, cmd, cproc);
+ unlock_kernel();
+ return ret;
}
static int chd_dec_open(struct inode *in, struct file *fd)
@@ ...Fix-up compile problems as a result of the bkl-pushdown. In particular, the v4l2_ioctl should call an unlocked_ioctl Signed-off-by: John Kacur <jkacur@redhat.com> --- drivers/media/video/v4l2-dev.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c index 3606694..8fbfa61 100644 --- a/drivers/media/video/v4l2-dev.c +++ b/drivers/media/video/v4l2-dev.c @@ -222,12 +222,12 @@ static long v4l2_ioctl(struct file *filp, struct video_device *vdev = video_devdata(filp); int ret; - if (!vdev->fops->ioctl) + if (!vdev->fops->unlocked_ioctl) return -ENOTTY; /* Allow ioctl to continue even if the device was unregistered. Things like dequeueing buffers might still be useful. */ lock_kernel(); - ret = vdev->fops->ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg); + ret = vdev->fops->unlocked_ioctl(filp, cmd, arg); unlock_kernel(); return ret; -- 1.6.6.1 --
No, this is what I thought as well, at first, but the version I posted is actually correct. vdev->fops is not struct file_operations but struct v4l2_file_operations, and the v4l2_ioctl function is used only when fops->ioctl is set, see __video_register_device. The v4l2 ioctl stuff probably can use a lot of cleanup itself, but so far I think we're not making it worse with my patch. Arnd --
Hi Arnd Well it is certainly possible that my fixup is not correct too - your patch cannot be correct, because it doesn't compile! Here is what I get when I apply your patch to a recent linus/master make O=/bld/arnd/ drivers/media/video/v4l2-dev.o -----CUT A BUNCH OF STUFF OUT --- CC drivers/media/video/v4l2-dev.o /home/jkacur/jk-2.6/drivers/media/video/v4l2-dev.c: In function ‘v4l2_ioctl’: /home/jkacur/jk-2.6/drivers/media/video/v4l2-dev.c:230: warning: passing argument 1 of ‘vdev->fops->ioctl’ from incompatible pointer type /home/jkacur/jk-2.6/drivers/media/video/v4l2-dev.c:230: note: expected ‘struct file *’ but argument is of type ‘struct inode *’ /home/jkacur/jk-2.6/drivers/media/video/v4l2-dev.c:230: warning: passing argument 2 of ‘vdev->fops->ioctl’ makes integer from pointer without a cast /home/jkacur/jk-2.6/drivers/media/video/v4l2-dev.c:230: note: expected ‘unsigned int’ but argument is of type ‘struct file *’ /home/jkacur/jk-2.6/drivers/media/video/v4l2-dev.c:230: error: too many arguments to function ‘vdev->fops->ioctl’ make[2]: *** [drivers/media/video/v4l2-dev.o] Error 1 make[1]: *** [drivers/media/video/v4l2-dev.o] Error 2 make: *** [sub-make] Error 2 Thanks
I didn't realize that the prototype for the locked ->ioctl function in
v4l2 does not take the inode argument, so that needs to be dropped from
my patch (i.e. not added).
Or we do it slightly better and clean up the code a bit in the process.
Mauro, does this patch make sense to you? It would be good to have your
Ack so we can queue this in the series leading to the removal of the
->ioctl file operation. We can also do the minimal change and let you
take care of fixing this up in a different way in your own tree.
---
Subject: v4l: convert v4l2-dev to unlocked_ioctl
v4l2 implements two separate file operations for drivers that use locked
and unlocked ioctl callbacks. Since we want to remove the ioctl file operation
in favor of the unlocked variant, this separation no longer seems helpful.
Unfortunately, there are still 77 drivers with locked ioctl functions in
their v4l2_file_operations, which need to be taken care of separately.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/video/v4l2-dev.c | 52 +++++++++++----------------------------
1 files changed, 15 insertions(+), 37 deletions(-)
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 7090699..ab198ba 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -25,6 +25,7 @@
#include <linux/init.h>
#include <linux/kmod.h>
#include <linux/slab.h>
+#include <linux/smp_lock.h>
#include <asm/uaccess.h>
#include <asm/system.h>
@@ -215,28 +216,24 @@ static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll)
return vdev->fops->poll(filp, poll);
}
-static int v4l2_ioctl(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg)
+static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
struct video_device *vdev = video_devdata(filp);
+ int ret;
- if (!vdev->fops->ioctl)
- return -ENOTTY;
/* Allow ioctl to continue even if the device was ...Hi Arnd, We had a similar discussion a while ago at linux-media. The idea is to do it on a deeper level, changing the drivers to not need to use KBL. Hans is working on those patches. Not sure about the current status. Anyway, I think that the better is to apply first your patch, to avoid breaking your patch series, and then ours, as we may otherwise have some conflicts between your tree and drivers/media git tree. So, -- Cheers, Mauro --
I'm waiting for the event patch series from Sakari to go in before I can I have no real problem with this patch, as long as people realize that this only moves the BKL from one place to another and does not really fix any drivers. Acked-by: Hans Verkuil <hverkuil@xs4all.nl> Regards, -- Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco --
Hi Hans, Seems that Arnd was more convincing than me. I've submitted the exact same patch a while ago :-) Of course this doesn't solve the BKL issue, but it pushes it one level down, closer to the drivers. Higher levels can then stop caring about it. Of course lower levels will still need to remove the BKL. -- Regards, Laurent Pinchart --
Yes. This patch doesn't change anything in practice, but it allows RT people to keep working on their series of changes, giving us a little more time to work on a definitive solution. If nacked, we would need to send an alternative patch for them, and, as this is not ready (and such patch applied via RT tree would likely cause merge conflicts with our patches), the better is to just ack and keep writing the patch that will solve the issue. Your patch were very useful, as it started the discussions for the BKL removal on drivers/media drivers. -- Cheers, Mauro --
[ Removed the '-' lines so you can see what the end result ends up being ] Please, if you do this for the V4L2 layer, then DO NOT make the same mistake we did with the vasic VFS layer. In other words, DO NOT keep the "bkl" version named just "ioctl". It was a horrible horrible mistake, and it has resulted in problems years afterwards. I realize that it's so easy to just add a new ".unlocked_ioctl" member, and then as people start using it, they get rid of the BKL. But it's a mistake. It was a mistake for the VFS layer, it would be a mistake for the V4L2 layer. Instead, spend the 15 minutes just renaming every current 'ioctl' user in the V4L2 layer. It's not that much work, the scripts I documented in my renaming patch do 95% of the work (you just need to change "file_operations" to "v4l2_file_operations"). It's not that painful. And then you don't just push the BKL down, you actually annotate the remaining users so that they can be grepped for. So please please please, don't make the same mistake we did long ago. Linus --
Hmm, there are 92 struct v4l2_file_operations::ioctl but actually a lot of duplicates ioctl. In fact there are just 26 ioctl functions. It's probably worth the whole pushdown instead of the rename. I'm going to do this. --
video_ioctl2 is a generic ioctl helper used by a lot of drivers.
Most of them put it as their bkl'ed .ioctl callback, then let's
pushdown the bkl inside but also provide an unlocked version for
those that use it as an unlocked ioctl.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
drivers/media/video/v4l2-ioctl.c | 17 +++++++++++++++--
include/media/v4l2-ioctl.h | 2 ++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 3da8d8f..0ff2595 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -16,6 +16,7 @@
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/kernel.h>
+#include <linux/smp_lock.h>
#define __OLD_VIDIOC_ /* To allow fixing old calls */
#include <linux/videodev.h>
@@ -2007,8 +2008,8 @@ static unsigned long cmd_input_size(unsigned int cmd)
}
}
-long video_ioctl2(struct file *file,
- unsigned int cmd, unsigned long arg)
+long video_ioctl2_unlocked(struct file *file,
+ unsigned int cmd, unsigned long arg)
{
char sbuf[128];
void *mbuf = NULL;
@@ -2102,4 +2103,16 @@ out:
kfree(mbuf);
return err;
}
+EXPORT_SYMBOL(video_ioctl2_unlocked);
+
+long video_ioctl2(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ long ret;
+
+ lock_kernel();
+ ret = video_ioctl2_unlocked(file, cmd, arg);
+ unlock_kernel();
+
+ return ret;
+}
EXPORT_SYMBOL(video_ioctl2);
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index e8ba0f2..08b3e42 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -316,5 +316,7 @@ extern long video_usercopy(struct file *file, unsigned int cmd,
/* Standard handlers for V4L ioctl's */
extern long video_ioctl2(struct file *file,
unsigned int cmd, unsigned long arg);
+extern long video_ioctl2_unlocked(struct file *file,
+ unsigned int cmd, unsigned long arg);
#endif /* _V4L2_IOCTL_H ...Now that video_ioctl2() got the bkl pushed down, update its users to use .unlocked_ioctl instead of ioctl. Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> --- drivers/media/common/saa7146_fops.c | 2 +- drivers/media/radio/dsbr100.c | 2 +- drivers/media/radio/radio-aimslab.c | 2 +- drivers/media/radio/radio-aztech.c | 2 +- drivers/media/radio/radio-cadet.c | 2 +- drivers/media/radio/radio-gemtek-pci.c | 2 +- drivers/media/radio/radio-gemtek.c | 2 +- drivers/media/radio/radio-maestro.c | 2 +- drivers/media/radio/radio-maxiradio.c | 2 +- drivers/media/radio/radio-miropcm20.c | 2 +- drivers/media/radio/radio-mr800.c | 2 +- drivers/media/radio/radio-rtrack2.c | 2 +- drivers/media/radio/radio-sf16fmi.c | 2 +- drivers/media/radio/radio-sf16fmr2.c | 2 +- drivers/media/radio/radio-si4713.c | 2 +- drivers/media/radio/radio-tea5764.c | 2 +- drivers/media/radio/radio-terratec.c | 2 +- drivers/media/radio/radio-timb.c | 2 +- drivers/media/radio/radio-trust.c | 2 +- drivers/media/radio/radio-typhoon.c | 2 +- drivers/media/radio/radio-zoltrix.c | 2 +- drivers/media/radio/si470x/radio-si470x-common.c | 2 +- drivers/media/video/arv.c | 2 +- drivers/media/video/au0828/au0828-video.c | 14 ++++++------ drivers/media/video/bt8xx/bttv-driver.c | 26 +++++++++++----------- drivers/media/video/cafe_ccic.c | 14 ++++++------ drivers/media/video/cx18/cx18-streams.c | 12 +++++----- drivers/media/video/cx231xx/cx231xx-video.c | 4 +- drivers/media/video/cx23885/cx23885-417.c | 2 +- drivers/media/video/cx23885/cx23885-video.c | ...
No more drivers use the bkl'ed callback, now drop it for good.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
drivers/media/video/v4l2-dev.c | 38 ++++----------------------------------
include/media/v4l2-dev.h | 1 -
2 files changed, 4 insertions(+), 35 deletions(-)
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 7090699..702a026 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -215,19 +215,7 @@ static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll)
return vdev->fops->poll(filp, poll);
}
-static int v4l2_ioctl(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg)
-{
- struct video_device *vdev = video_devdata(filp);
-
- if (!vdev->fops->ioctl)
- return -ENOTTY;
- /* Allow ioctl to continue even if the device was unregistered.
- Things like dequeueing buffers might still be useful. */
- return vdev->fops->ioctl(filp, cmd, arg);
-}
-
-static long v4l2_unlocked_ioctl(struct file *filp,
+static long v4l2_ioctl(struct file *filp,
unsigned int cmd, unsigned long arg)
{
struct video_device *vdev = video_devdata(filp);
@@ -307,22 +295,6 @@ static int v4l2_release(struct inode *inode, struct file *filp)
return ret;
}
-static const struct file_operations v4l2_unlocked_fops = {
- .owner = THIS_MODULE,
- .read = v4l2_read,
- .write = v4l2_write,
- .open = v4l2_open,
- .get_unmapped_area = v4l2_get_unmapped_area,
- .mmap = v4l2_mmap,
- .unlocked_ioctl = v4l2_unlocked_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = v4l2_compat_ioctl32,
-#endif
- .release = v4l2_release,
- .poll = v4l2_poll,
- .llseek = no_llseek,
-};
-
static const struct file_operations v4l2_fops = {
.owner = THIS_MODULE,
.read = v4l2_read,
@@ -330,7 +302,7 @@ static const struct file_operations v4l2_fops = {
.open = v4l2_open,
.get_unmapped_area = v4l2_get_unmapped_area,
.mmap = v4l2_mmap,
- .ioctl = ...These are the last remaining v4l drivers that implement the ioctl callback. Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> --- drivers/media/video/bw-qcam.c | 11 ++++++++- drivers/media/video/c-qcam.c | 11 ++++++++- drivers/media/video/cpia.c | 11 ++++++++- drivers/media/video/cpia2/cpia2_v4l.c | 11 ++++++++- drivers/media/video/et61x251/et61x251_core.c | 27 +++++++++++++++++++------ drivers/media/video/ov511.c | 15 ++++++++----- drivers/media/video/pvrusb2/pvrusb2-v4l2.c | 20 ++++++++++++------ drivers/media/video/pwc/pwc-if.c | 19 ++++++++++------- drivers/media/video/saa5246a.c | 11 ++++++--- drivers/media/video/saa5249.c | 6 ++++- drivers/media/video/se401.c | 20 ++++++++++++------ drivers/media/video/sn9c102/sn9c102_core.c | 27 +++++++++++++++++++------ drivers/media/video/stradis.c | 26 ++++++++++++++++++------ drivers/media/video/stv680.c | 20 ++++++++++++------ drivers/media/video/usbvideo/usbvideo.c | 21 +++++++++++++------ drivers/media/video/usbvideo/vicam.c | 14 ++++++++++++- drivers/media/video/uvc/uvc_v4l2.c | 11 ++++++++- drivers/media/video/vivi.c | 2 +- drivers/media/video/w9968cf.c | 25 ++++++++++++++++++----- drivers/media/video/zc0301/zc0301_core.c | 27 +++++++++++++++++++------ drivers/staging/cx25821/cx25821-audups11.c | 18 ++++++++++------ drivers/staging/cx25821/cx25821-videoioctl.c | 27 +++++++++++++++++++------ drivers/staging/cx25821/cx25821-vidups10.c | 19 +++++++++++------ drivers/staging/cx25821/cx25821-vidups9.c | 18 ++++++++++------ drivers/staging/dream/camera/msm_v4l2.c | 27 ++++++++++++++++++------- 25 files changed, 315 insertions(+), 129 deletions(-) diff --git a/drivers/media/video/bw-qcam.c ...
There are three drivers that use video_ioctl2() as an unlocked_ioctl,
let them use the new video_ioctl2_unlocked.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
drivers/media/video/davinci/vpfe_capture.c | 2 +-
drivers/media/video/gspca/gspca.c | 2 +-
drivers/media/video/hdpvr/hdpvr-video.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/video/davinci/vpfe_capture.c b/drivers/media/video/davinci/vpfe_capture.c
index aa1411f..61a3514 100644
--- a/drivers/media/video/davinci/vpfe_capture.c
+++ b/drivers/media/video/davinci/vpfe_capture.c
@@ -786,7 +786,7 @@ static const struct v4l2_file_operations vpfe_fops = {
.owner = THIS_MODULE,
.open = vpfe_open,
.release = vpfe_release,
- .unlocked_ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2_unlocked,
.mmap = vpfe_mmap,
.poll = vpfe_poll
};
diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 68a8431..a424c1d 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -2184,7 +2184,7 @@ static struct v4l2_file_operations dev_fops = {
.release = dev_close,
.read = dev_read,
.mmap = dev_mmap,
- .unlocked_ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2_unlocked,
.poll = dev_poll,
};
diff --git a/drivers/media/video/hdpvr/hdpvr-video.c b/drivers/media/video/hdpvr/hdpvr-video.c
index 196f82d..01b8a34 100644
--- a/drivers/media/video/hdpvr/hdpvr-video.c
+++ b/drivers/media/video/hdpvr/hdpvr-video.c
@@ -559,7 +559,7 @@ static const struct v4l2_file_operations hdpvr_fops = {
.release = hdpvr_release,
.read = hdpvr_read,
.poll = hdpvr_poll,
- .unlocked_ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2_unlocked,
};
/*=======================================================================*/
--
1.6.2.3
--
Hi,
Linus suggested to rename struct v4l2_file_operations::ioctl
into bkl_ioctl to eventually get something greppable and make
its background explicit.
While at it I thought it could be a good idea to just pushdown
the bkl to every v4l drivers that have an .ioctl, so that we
actually remove struct v4l2_file_operations::ioctl for good.
It passed make allyesconfig on sparc.
Please tell me what you think.
Thanks.
Frederic Weisbecker (5):
v4l: Pushdown bkl into video_ioctl2
v4l: Use video_ioctl2_unlocked from drivers that don't want the bkl
v4l: Change users of video_ioctl2 to use unlocked_ioctl
v4l: Pushdown bkl to drivers that implement their own ioctl
v4l: Remove struct v4l2_file_operations::ioctl
drivers/media/common/saa7146_fops.c | 2 +-
drivers/media/radio/dsbr100.c | 2 +-
drivers/media/radio/radio-aimslab.c | 2 +-
drivers/media/radio/radio-aztech.c | 2 +-
drivers/media/radio/radio-cadet.c | 2 +-
drivers/media/radio/radio-gemtek-pci.c | 2 +-
drivers/media/radio/radio-gemtek.c | 2 +-
drivers/media/radio/radio-maestro.c | 2 +-
drivers/media/radio/radio-maxiradio.c | 2 +-
drivers/media/radio/radio-miropcm20.c | 2 +-
drivers/media/radio/radio-mr800.c | 2 +-
drivers/media/radio/radio-rtrack2.c | 2 +-
drivers/media/radio/radio-sf16fmi.c | 2 +-
drivers/media/radio/radio-sf16fmr2.c | 2 +-
drivers/media/radio/radio-si4713.c | 2 +-
drivers/media/radio/radio-tea5764.c | 2 +-
drivers/media/radio/radio-terratec.c | 2 +-
drivers/media/radio/radio-timb.c | 2 +-
drivers/media/radio/radio-trust.c | 2 +-
drivers/media/radio/radio-typhoon.c | 2 +-
drivers/media/radio/radio-zoltrix.c | 2 ...Hi Hans, Every driver will need to be carefully checked to make sure the BKL can be replaced by a v4l2-dev global mutex. Why would it be more difficult to do so if the BKL is pushed down to the drivers ? -- Regards, Laurent Pinchart --
Note that you can completely skip the step of a v4l2-dev global mutex with Frederic's patch. This is the only use of the BKL in the common v4l2 code as far as I can tell, so instead of introducing yet another global lock, you can go straight to stage 4 and look at each driver separately, possibly introducing a per driver lock. Arnd --
The main reason is really that pushing the bkl into the v4l core makes it easier to review. I noticed for example that this patch series forgot to change the video_ioctl2 call in ivtv-ioctl.c to video_ioctl2_unlocked. And there may be other places as well that were missed. Having so many drivers changed also means a lot of careful reviewing. But I will not block this change. However, I do think it would be better to create a video_ioctl2_bkl rather than add a video_ioctl2_unlocked. The current video_ioctl2 function *is* already unlocked. So you are subtle changing the behavior of video_ioctl2. Not a good idea IMHO. And yes, grepping for video_ioctl2_bkl is also easy to do and makes it more obvious that the BKL is used in drivers that call this. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco --
Yes, that makes sense. It also allows working towards a goal of 'removing video_ioctl2_bkl', which is easier to understand than 'converting video_ioctl2 users to video_ioctl2_unlocked and later renaming that'. Arnd --
Indeed, that's because I did it in a half automated way and my script didn't took the direct calls to video_ioctl2() into account, so I had to check them manually and probably missed a few, I will fix this one and Totally agreed, will respin with this rename. Thanks. --
I much prefer to keep the bkl inside the v4l2 core. One reason is that I think that we can replace the bkl in the core with a mutex. Still not ideal of course, so the next step will be to implement proper locking in each driver. For this some additional v4l infrastructure work needs to be done. I couldn't proceed with that until the v4l events API patches went in, and that happened yesterday. So from my point of view the timeline is this: 1) I do the infrastructure work this weekend. This will make it much easier to convert drivers to do proper locking. And it will also simplify v4l2_priority handling, so I'm killing two birds with one stone :-) 2) Wait until Arnd's patch gets merged that pushes the bkl down to v4l2-dev.c 3) Investigate what needs to be done to replace the bkl with a v4l2-dev.c global mutex. Those drivers that call the bkl themselves should probably be converted to do proper locking, but there are only about 14 drivers that do this. The other 60 or so drivers should work fine if a v4l2-dev global lock is used. At this point the bkl is effectively removed from the v4l subsystem. 4) Work on the remaining 60 drivers to do proper locking and get rid of the v4l2-dev global lock. This is probably less work than it sounds. Since your patch moves everything down to the driver level it will actually make this work harder rather than easier. And it touches almost all drivers as well. Regards, -- Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco --
I did look at this a long time ago - it doesn't really work becaue the mutex you propose then has to be dropped and taken in the sleeping parts of each ioctl to avoid app problems and in some cases threaded apps deadlocking. I think Arnd is right on his approach to this, having tried the other way. --
From: Arnd Bergmann <arnd@arndb.de> These are the last remaining device drivers using the ->ioctl file operation in the drivers directory. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Fixed merge conflicts that result from having applied Linus's Preparation for BKL'ed ioctl removal patch first. Signed-off-by: John Kacur <jkacur@redhat.com> --- drivers/block/pktcdvd.c | 13 ++++++++-- drivers/char/apm-emulation.c | 8 ++++-- drivers/char/applicom.c | 13 ++++++---- drivers/char/ds1620.c | 16 +++++++++++- drivers/char/dtlk.c | 15 ++++++------ drivers/char/generic_nvram.c | 17 +++++++++++-- drivers/char/genrtc.c | 16 +++++++++++- drivers/char/hpet.c | 14 +++++++---- drivers/char/i8k.c | 21 ++++++++++++++--- drivers/char/ipmi/ipmi_devintf.c | 26 +++++++++++++++++---- drivers/char/ipmi/ipmi_watchdog.c | 17 ++++++++++++- drivers/char/nvram.c | 10 ++++++-- drivers/char/nwflash.c | 7 ++++- drivers/char/raw.c | 42 ++++++++++++++++++++--------------- drivers/hwmon/fschmd.c | 9 ++++--- drivers/hwmon/w83793.c | 10 +++++--- drivers/input/misc/hp_sdc_rtc.c | 34 ++++++++++++++++++++-------- drivers/macintosh/nvram.c | 2 +- drivers/macintosh/via-pmu.c | 17 +++++++++++-- drivers/mtd/mtdchar.c | 19 +++++++++++---- drivers/pcmcia/pcmcia_ioctl.c | 17 +++++++++++-- drivers/rtc/rtc-m41t80.c | 16 +++++++++++- drivers/sbus/char/openprom.c | 44 +++++++++++++++++++++---------------- drivers/usb/mon/mon_bin.c | 23 ++++++++++++++----- drivers/usb/mon/mon_stat.c | 3 +- 25 files changed, 307 insertions(+), 122 deletions(-) diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c index d0bb5c7..84a5658 100644 --- a/drivers/block/pktcdvd.c +++ b/drivers/block/pktcdvd.c @@ -55,6 +55,7 @@ ...
- Forward declaration missing ';' - Conflicting declaraction in megaraid.h changed Signed-off-by: John Kacur <jkacur@redhat.com> --- drivers/scsi/megaraid.c | 2 +- drivers/scsi/megaraid.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c index c20b621..0b6e322 100644 --- a/drivers/scsi/megaraid.c +++ b/drivers/scsi/megaraid.c @@ -92,7 +92,7 @@ static struct proc_dir_entry *mega_proc_dir_entry; static struct mega_hbas mega_hbas[MAX_CONTROLLERS]; static long -megadev_unlocked_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) +megadev_unlocked_ioctl(struct file *filep, unsigned int cmd, unsigned long arg); /* * The File Operations structure for the serial/ioctl interface of the driver diff --git a/drivers/scsi/megaraid.h b/drivers/scsi/megaraid.h index d310f49..b3d2949 100644 --- a/drivers/scsi/megaraid.h +++ b/drivers/scsi/megaraid.h @@ -1013,8 +1013,8 @@ static void mega_8_to_40ld (mraid_inquiry *inquiry, mega_inquiry3 *enquiry3, mega_product_info *); static int megadev_open (struct inode *, struct file *); -static int megadev_ioctl (struct inode *, struct file *, unsigned int, - unsigned long); +static int +megadev_ioctl(struct file *filep, unsigned int cmd, unsigned long arg); static int mega_m_to_n(void __user *, nitioctl_t *); static int mega_n_to_m(void __user *, megacmd_t *); -- 1.6.6.1 --
Thanks for catching the scsi bugs! It was getting late yesterday night, so Yes, your fixes (except the v4l2 patch, see comment there) should best be merged into the main patches. The idea was that if we manage to kill all fops->ioctl users right away, we wouldn't even need Linus' patch and move to dungeon level 2 directly. Either way works for me though. Should we just split up the rest? If you, Frederic and me each take one of these chunks, we're done. According to the diffstat from Linus' patch, this is what's left: a) arch specific drivers arch/cris/arch-v10/drivers/ds1302.c | 2 +- arch/cris/arch-v10/drivers/gpio.c | 2 +- arch/cris/arch-v10/drivers/i2c.c | 2 +- arch/cris/arch-v10/drivers/pcf8563.c | 2 +- arch/cris/arch-v10/drivers/sync_serial.c | 2 +- arch/cris/arch-v32/drivers/cryptocop.c | 2 +- arch/cris/arch-v32/drivers/i2c.c | 2 +- arch/cris/arch-v32/drivers/mach-a3/gpio.c | 2 +- arch/cris/arch-v32/drivers/mach-fs/gpio.c | 2 +- arch/cris/arch-v32/drivers/pcf8563.c | 2 +- arch/cris/arch-v32/drivers/sync_serial.c | 2 +- arch/ia64/kernel/perfmon.c | 2 +- arch/ia64/sn/kernel/sn2/sn_hwperf.c | 2 +- arch/m68k/bvme6000/rtc.c | 2 +- arch/m68k/mvme16x/rtc.c | 2 +- arch/um/drivers/harddog_kern.c | 2 +- arch/um/drivers/hostaudio_kern.c | 4 +- arch/um/drivers/mmapper_kern.c | 2 +- b) file systems (my other series has patches for some of these already) fs/autofs/root.c | 2 +- fs/autofs4/root.c | 2 +- fs/coda/pioctl.c | 2 +- fs/coda/psdev.c | 2 +- fs/fat/dir.c | 2 +- fs/fat/file.c | 2 +- fs/hfsplus/dir.c | 2 +- ...
Okay - good plan. I AM trying to balance doing my own day-to-day work, and writing slides for a conference but I'm sure you and Frederic are just as pressed for time as me, so lets go-ahead and divide it up! That will prevent us from doing redundant work. I assume the target is 2.6.35 so, we have a little bit of time anyway. I suggest we divide up a) and b) first and postone c) until the first parts are done. Arranging our names by alphabetical order and shuffling the files off one at a time (for variety), I get this. Is everybody happy with that? 36 files / 3 = 12 files each. Jan - speak up if you want in. Arnd ---- arch/cris/arch-v10/drivers/ds1302.c arch/cris/arch-v10/drivers/pcf8563.c arch/cris/arch-v32/drivers/i2c.c arch/cris/arch-v32/drivers/pcf8563.c arch/ia64/sn/kernel/sn2/sn_hwperf.c arch/m68k/mvme16x/rtc.c arch/um/drivers/mmapper_kern.c fs/coda/psdev.c fs/hfsplus/dir.c fs/logfs/file.c fs/ntfs/dir.c fs/smbfs/file.c Frederic -------- arch/cris/arch-v10/drivers/gpio.c arch/cris/arch-v10/drivers/sync_serial.c arch/cris/arch-v32/drivers/mach-a3/gpio.c arch/cris/arch-v32/drivers/sync_serial.c arch/m68k/bvme6000/rtc.c arch/um/drivers/harddog_kern.c fs/autofs/root.c fs/fat/dir.c fs/hfsplus/inode.c fs/ncpfs/dir.c fs/ntfs/file.c fs/udf/dir.c John ---- arch/cris/arch-v10/drivers/i2c.c arch/cris/arch-v32/drivers/cryptocop.c arch/cris/arch-v32/drivers/mach-fs/gpio.c arch/ia64/kernel/perfmon.c arch/m68k/bvme6000/rtc.c arch/um/drivers/hostaudio_kern.c fs/coda/pioctl.c fs/fat/file.c fs/logfs/dir.c fs/ncpfs/file.c fs/smbfs/dir.c --
done Arnd Bergmann (6): logfs: push down BKL into ioctl function hfsplus: push down BKL into ioctl function cris: push down BKL into some device drivers sn_hwperf: kill BKL usage um/mmapper: remove BKL usage coda/psdev: remove BKL from ioctl function arch/cris/arch-v10/drivers/ds1302.c | 20 +++++++++++++++----- arch/cris/arch-v10/drivers/pcf8563.c | 19 +++++++++++++++---- arch/cris/arch-v32/drivers/i2c.c | 22 ++++++++++++++-------- arch/cris/arch-v32/drivers/pcf8563.c | 21 ++++++++++++++++----- arch/ia64/sn/kernel/sn2/sn_hwperf.c | 9 ++------- arch/um/drivers/mmapper_kern.c | 5 ++--- fs/coda/psdev.c | 5 ++--- fs/hfsplus/dir.c | 2 +- fs/hfsplus/hfsplus_fs.h | 3 +-- fs/hfsplus/ioctl.c | 11 ++++++++--- fs/logfs/dir.c | 2 +- fs/logfs/file.c | 19 +++++++++++++++---- fs/logfs/logfs.h | 4 ++-- 13 files changed, 94 insertions(+), 48 deletions(-) --
HFS is one of the remaining users of the ->ioctl function, convert it
blindly to unlocked_ioctl by pushing down the BKL.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
fs/hfsplus/dir.c | 2 +-
fs/hfsplus/hfsplus_fs.h | 3 +--
fs/hfsplus/inode.c | 2 +-
fs/hfsplus/ioctl.c | 12 +++++++++---
4 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 5f40236..764fd1b 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -494,7 +494,7 @@ const struct inode_operations hfsplus_dir_inode_operations = {
const struct file_operations hfsplus_dir_operations = {
.read = generic_read_dir,
.readdir = hfsplus_readdir,
- .ioctl = hfsplus_ioctl,
+ .unlocked_ioctl = hfsplus_ioctl,
.llseek = generic_file_llseek,
.release = hfsplus_dir_release,
};
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 5c10d80..6505c30 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -337,8 +337,7 @@ struct inode *hfsplus_new_inode(struct super_block *, int);
void hfsplus_delete_inode(struct inode *);
/* ioctl.c */
-int hfsplus_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
- unsigned long arg);
+long hfsplus_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
int hfsplus_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags);
ssize_t hfsplus_getxattr(struct dentry *dentry, const char *name,
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 1bcf597..9bbb829 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -285,7 +285,7 @@ static const struct file_operations hfsplus_file_operations = {
.fsync = file_fsync,
.open = hfsplus_file_open,
.release = hfsplus_file_release,
- .ioctl = hfsplus_ioctl,
+ .unlocked_ioctl = hfsplus_ioctl,
};
struct inode *hfsplus_new_inode(struct super_block *sb, int mode)
diff --git a/fs/hfsplus/ioctl.c ...An empty function does not need the BKL, so just remove it.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
arch/um/drivers/mmapper_kern.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/um/drivers/mmapper_kern.c b/arch/um/drivers/mmapper_kern.c
index d22f9e5..7158393 100644
--- a/arch/um/drivers/mmapper_kern.c
+++ b/arch/um/drivers/mmapper_kern.c
@@ -46,8 +46,7 @@ static ssize_t mmapper_write(struct file *file, const char __user *buf,
return count;
}
-static int mmapper_ioctl(struct inode *inode, struct file *file,
- unsigned int cmd, unsigned long arg)
+static long mmapper_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
return -ENOIOCTLCMD;
}
@@ -90,7 +89,7 @@ static const struct file_operations mmapper_fops = {
.owner = THIS_MODULE,
.read = mmapper_read,
.write = mmapper_write,
- .ioctl = mmapper_ioctl,
+ .unlocked_ioctl = mmapper_ioctl,
.mmap = mmapper_mmap,
.open = mmapper_open,
.release = mmapper_release,
--
1.6.3.3
--
This driver always gave up the BKL in its ioctl function, so just
convert it to unlocked_ioctl and remove the BKL here.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
arch/ia64/sn/kernel/sn2/sn_hwperf.c | 9 ++-------
1 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/arch/ia64/sn/kernel/sn2/sn_hwperf.c b/arch/ia64/sn/kernel/sn2/sn_hwperf.c
index 55ac3c4..d68fe0f 100644
--- a/arch/ia64/sn/kernel/sn2/sn_hwperf.c
+++ b/arch/ia64/sn/kernel/sn2/sn_hwperf.c
@@ -30,7 +30,6 @@
#include <linux/miscdevice.h>
#include <linux/utsname.h>
#include <linux/cpumask.h>
-#include <linux/smp_lock.h>
#include <linux/nodemask.h>
#include <linux/smp.h>
#include <linux/mutex.h>
@@ -682,8 +681,7 @@ static int sn_hwperf_map_err(int hwperf_err)
/*
* ioctl for "sn_hwperf" misc device
*/
-static int
-sn_hwperf_ioctl(struct inode *in, struct file *fp, u32 op, unsigned long arg)
+static long sn_hwperf_ioctl(struct file *fp, u32 op, unsigned long arg)
{
struct sn_hwperf_ioctl_args a;
struct cpuinfo_ia64 *cdata;
@@ -699,8 +697,6 @@ sn_hwperf_ioctl(struct inode *in, struct file *fp, u32 op, unsigned long arg)
int i;
int j;
- unlock_kernel();
-
/* only user requests are allowed here */
if ((op & SN_HWPERF_OP_MASK) < 10) {
r = -EINVAL;
@@ -859,12 +855,11 @@ sn_hwperf_ioctl(struct inode *in, struct file *fp, u32 op, unsigned long arg)
error:
vfree(p);
- lock_kernel();
return r;
}
static const struct file_operations sn_hwperf_fops = {
- .ioctl = sn_hwperf_ioctl,
+ .unlocked_ioctl = sn_hwperf_ioctl,
};
static struct miscdevice sn_hwperf_dev = {
--
1.6.3.3
--
The ioctl function returns constant results, so it obviously
does not need the BKL and can be converted to unlocked_ioctl.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
fs/coda/psdev.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/coda/psdev.c b/fs/coda/psdev.c
index be4392c..66b9cf7 100644
--- a/fs/coda/psdev.c
+++ b/fs/coda/psdev.c
@@ -73,8 +73,7 @@ static unsigned int coda_psdev_poll(struct file *file, poll_table * wait)
return mask;
}
-static int coda_psdev_ioctl(struct inode * inode, struct file * filp,
- unsigned int cmd, unsigned long arg)
+static long coda_psdev_ioctl(struct file * filp, unsigned int cmd, unsigned long arg)
{
unsigned int data;
@@ -344,7 +343,7 @@ static const struct file_operations coda_psdev_fops = {
.read = coda_psdev_read,
.write = coda_psdev_write,
.poll = coda_psdev_poll,
- .ioctl = coda_psdev_ioctl,
+ .unlocked_ioctl = coda_psdev_ioctl,
.open = coda_psdev_open,
.release = coda_psdev_release,
};
--
1.6.3.3
--
A number of cris specific device drivers still use the
locked ->ioctl operation. Convert them to unlocked_ioctl
with explicit lock_kernel calls.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
arch/cris/arch-v10/drivers/ds1302.c | 20 +++++++++++++++-----
arch/cris/arch-v10/drivers/pcf8563.c | 19 +++++++++++++++----
arch/cris/arch-v32/drivers/i2c.c | 22 ++++++++++++++--------
arch/cris/arch-v32/drivers/pcf8563.c | 21 ++++++++++++++++-----
4 files changed, 60 insertions(+), 22 deletions(-)
diff --git a/arch/cris/arch-v10/drivers/ds1302.c b/arch/cris/arch-v10/drivers/ds1302.c
index 77630df..ffbf4d8 100644
--- a/arch/cris/arch-v10/drivers/ds1302.c
+++ b/arch/cris/arch-v10/drivers/ds1302.c
@@ -19,6 +19,7 @@
#include <linux/module.h>
#include <linux/miscdevice.h>
#include <linux/delay.h>
+#include <linux/smp_lock.h>
#include <linux/bcd.h>
#include <linux/capability.h>
@@ -238,9 +239,7 @@ static unsigned char days_in_mo[] =
/* ioctl that supports RTC_RD_TIME and RTC_SET_TIME (read and set time/date). */
-static int
-rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
- unsigned long arg)
+static int rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
unsigned long flags;
@@ -354,6 +353,17 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
}
}
+static long rtc_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ int ret;
+
+ lock_kernel();
+ ret = rtc_ioctl(file, cmd, arg);
+ unlock_kernel();
+
+ return ret;
+}
+
static void
print_rtc_status(void)
{
@@ -375,8 +385,8 @@ print_rtc_status(void)
/* The various file operations we support. */
static const struct file_operations rtc_fops = {
- .owner = THIS_MODULE,
- .ioctl = rtc_ioctl,
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = rtc_unlocked_ioctl,
};
/* Probe for the chip by writing something to its RAM and try reading it back. */
diff --git ...(Adding some more cris related Cc on this one) --
/^JN - Jesper Nilsson
--
Jesper Nilsson -- jesper.nilsson@axis.com
--
I'm sure that logfs doesn't rely on the BKL, but right now,
we're just pushing it down.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Jörn Engel <joern@logfs.org>
---
fs/logfs/dir.c | 2 +-
fs/logfs/file.c | 18 +++++++++++++++---
fs/logfs/logfs.h | 4 ++--
fs/smbfs/ioctl.c | 2 +-
4 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/fs/logfs/dir.c b/fs/logfs/dir.c
index 2396a85..a801b5e 100644
--- a/fs/logfs/dir.c
+++ b/fs/logfs/dir.c
@@ -821,7 +821,7 @@ const struct inode_operations logfs_dir_iops = {
};
const struct file_operations logfs_dir_fops = {
.fsync = logfs_fsync,
- .ioctl = logfs_ioctl,
+ .unlocked_ioctl = logfs_unlocked_ioctl,
.readdir = logfs_readdir,
.read = generic_read_dir,
};
diff --git a/fs/logfs/file.c b/fs/logfs/file.c
index 370f367..1dbc342 100644
--- a/fs/logfs/file.c
+++ b/fs/logfs/file.c
@@ -8,6 +8,7 @@
#include "logfs.h"
#include <linux/sched.h>
#include <linux/writeback.h>
+#include <linux/smp_lock.h>
static int logfs_write_begin(struct file *file, struct address_space *mapping,
loff_t pos, unsigned len, unsigned flags,
@@ -171,9 +172,9 @@ static int logfs_releasepage(struct page *page, gfp_t only_xfs_uses_this)
}
-int logfs_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
- unsigned long arg)
+static long logfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
+ struct inode *inode = file->f_path.dentry->d_inode;
struct logfs_inode *li = logfs_inode(inode);
unsigned int oldflags, flags;
int err;
@@ -209,6 +210,17 @@ int logfs_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
}
}
+long logfs_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ int ret;
+
+ lock_kernel();
+ ret = logfs_ioctl(file, cmd, arg);
+ unlock_kernel();
+
+ return ret;
+}
+
int logfs_fsync(struct file *file, struct dentry *dentry, int datasync)
{
struct super_block *sb = dentry->d_inode->i_sb;
@@ -243,7 ...This hunk obviously belongs to the smbfs patch, not the logfs patch. Sorry about that. Arnd --
So why do you create logfs_unlocked_ioctl in the first place? ;) Jörn -- Measure. Don't tune for speed until you've measured, and even then don't unless one part of the code overwhelms the rest. -- Rob Pike --
I don't want to get caught in discussions on whether any of my patches might introduce silent bugs in something I'm not maintaining. Also to put pressure on maintainers by threatening them to make their code ugly. If you just add a patch to convert to ->unlocked_ioctl without the BKL, we can drop this patch ;-) Arnd --
I guess that's threatening enough. Is this for the next merge window or still the current one? Jörn -- Everything should be made as simple as possible, but not simpler. -- Albert Einstein --
Hasn't the "current" merge window been over with for weeks? I was assuming that bkl push down, or other bkl patches are for the next merge window. You can ask Linus whether he'll take a convert unlocked_ioctl now or not. John
That's what I was thinking. But some of the bkl related discussion gave me the impression that such patches were still being merged. So I wanted to be sure. Jörn -- It is better to die of hunger having lived without grief and fear, than to live with a troubled spirit amid abundance. -- Epictetus --
Yes, just queue it for the 2.6.35-rc1 window and make sure it shows up in linux-next, or send a replacement patch with your Ack or S-o-b so we can put it into the BKL queue. Arnd --
I've applied this series in bkl/ioctl, it passed allyesconfig in sparc. I will apply the others from you and John tomorrow or so (and will enjoy my part as well). Jörn, please queue this patch if you want to, as you prefer, we can host it as well if necessary. Linus I hope you don't mind but I've dropped the .bkl_ioctl renaming. In fact it had conflicts with several trees in Linux-next and now that we are about to pushdown in every ioctl users, this would be an unnecessary step I think. Thanks. --
logfs does not need the BKL, so use ->unlocked_ioctl instead
of ->ioctl in file operations.
---
I believe that this is the one that Jörn would prefer to go in. I hope I
got it right by editing the patch.
diff --git a/fs/logfs/dir.c b/fs/logfs/dir.c
index 2396a85..a801b5e 100644
--- a/fs/logfs/dir.c
+++ b/fs/logfs/dir.c
@@ -821,7 +821,7 @@ const struct inode_operations logfs_dir_iops = {
};
const struct file_operations logfs_dir_fops = {
.fsync = logfs_fsync,
- .ioctl = logfs_ioctl,
+ .unlocked_ioctl = logfs_ioctl,
.readdir = logfs_readdir,
.read = generic_read_dir,
};
diff --git a/fs/logfs/file.c b/fs/logfs/file.c
index 370f367..1dbc342 100644
--- a/fs/logfs/file.c
+++ b/fs/logfs/file.c
@@ -171,9 +172,9 @@ static int logfs_releasepage(struct page *page, gfp_t only_xfs_uses_this)
}
-int logfs_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
- unsigned long arg)
+long logfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
+ struct inode *inode = file->f_path.dentry->d_inode;
struct logfs_inode *li = logfs_inode(inode);
unsigned int oldflags, flags;
int err;
@@ -243,7 +255,7 @@ const struct file_operations logfs_reg_fops = {
.aio_read = generic_file_aio_read,
.aio_write = generic_file_aio_write,
.fsync = logfs_fsync,
- .ioctl = logfs_ioctl,
+ .unlocked_ioctl = logfs_ioctl,
.llseek = generic_file_llseek,
.mmap = generic_file_readonly_mmap,
.open = generic_file_open,
diff --git a/fs/logfs/logfs.h b/fs/logfs/logfs.h
index 0a3df1a..8432c51 100644
--- a/fs/logfs/logfs.h
+++ b/fs/logfs/logfs.h
@@ -501,8 +501,8 @@ extern const struct inode_operations logfs_reg_iops;
extern const struct file_operations logfs_reg_fops;
extern const struct address_space_operations logfs_reg_aops;
int logfs_readpage(struct file *file, struct page *page);
-int logfs_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
- unsigned long arg);
+long logfs_ioctl(struct file *file, unsigned int cmd,
+ ...Jörn, any news about this patch? If you're fine with it, could you queue it for .35 ? --
Absolutely not. I could imagine merging something like just the bkl_ioctl rename: if it breaks, it won't compile, so there is little risk, and _if_ it helps the next merge window I could merge it early. But it looks like people are trying to avoid it entirely, and there is not a way in *hell* I will take some big and scary "push BKL down" patch that is clearly _not_ 99% mechanical and clearly _can_ introduce major and subtle bugs while compiling. Linus --
Heh, that's exactly what I thought you would say, but still good to clarify it for everyone. John --
I've included this part in the smbfs pushdown patch. Thanks. --
Converting from ->ioctl to ->unlocked_ioctl with explicit
lock_kernel lets us kill the ioctl operation.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
fs/smbfs/dir.c | 2 +-
fs/smbfs/file.c | 2 +-
fs/smbfs/ioctl.c | 8 +++++---
fs/smbfs/proto.h | 2 +-
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/fs/smbfs/dir.c b/fs/smbfs/dir.c
index 3e4803b..6c97842 100644
--- a/fs/smbfs/dir.c
+++ b/fs/smbfs/dir.c
@@ -39,7 +39,7 @@ const struct file_operations smb_dir_operations =
{
.read = generic_read_dir,
.readdir = smb_readdir,
- .ioctl = smb_ioctl,
+ .unlocked_ioctl = smb_ioctl,
.open = smb_dir_open,
};
diff --git a/fs/smbfs/file.c b/fs/smbfs/file.c
index dbf6548..84ecf0e 100644
--- a/fs/smbfs/file.c
+++ b/fs/smbfs/file.c
@@ -437,7 +437,7 @@ const struct file_operations smb_file_operations =
.aio_read = smb_file_aio_read,
.write = do_sync_write,
.aio_write = smb_file_aio_write,
- .ioctl = smb_ioctl,
+ .unlocked_ioctl = smb_ioctl,
.mmap = smb_file_mmap,
.open = smb_file_open,
.release = smb_file_release,
diff --git a/fs/smbfs/ioctl.c b/fs/smbfs/ioctl.c
index 910215b..0721531 100644
--- a/fs/smbfs/ioctl.c
+++ b/fs/smbfs/ioctl.c
@@ -13,6 +13,7 @@
#include <linux/time.h>
#include <linux/mm.h>
#include <linux/highuid.h>
+#include <linux/smp_lock.h>
#include <linux/net.h>
#include <linux/smb_fs.h>
@@ -22,14 +23,14 @@
#include "proto.h"
-int
-smb_ioctl(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg)
+long
+smb_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
struct smb_sb_info *server = server_from_inode(filp->f_path.dentry->d_inode);
struct smb_conn_opt opt;
int result = -EINVAL;
+ lock_kernel();
switch (cmd) {
uid16_t uid16;
uid_t uid32;
@@ -62,6 +63,7 @@ smb_ioctl(struct inode *inode, struct file *filp,
default:
break;
}
+ unlock_kernel();
return result;
}
diff --git a/fs/smbfs/proto.h ...Yeah. Since we are motivated enough to do the whole pushdown, I think we should forget the .bkl_ioctl renaming. This will let Greg take the staging part for example. I can take all the unmainted rest (basically most of the rest I guess). --
Don't waste your time on these: http://git.kernel.org/?p=linux/kernel/git/geert/linux-m68k.git;a=commit;h=ce266e954a1b... Gr{oetje,eeting}s, Geert (still has to update his for-next branch for 2.6.35). -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds --
Applied the series in bkl/ioctl, except the staging part that went to Greg's tree and the v4l part for which Linus had concerns. I haven't pushed your fixes separately but merged them into the concerned arnd's patches with your SOB added. It passed allyesconfig in sparc. Thanks. --
