From: Stoyan Gaydarov <[email protected]>
Signed-off-by: Stoyan Gaydarov <[email protected]>
---
drivers/staging/epl/EplApiLinuxKernel.c | 10 ++-
drivers/staging/meilhaus/memain.c | 124 +++++++++++++++++++++----------
drivers/staging/poch/poch.c | 25 +++++--
3 files changed, 107 insertions(+), 52 deletions(-)
diff --git a/drivers/staging/epl/EplApiLinuxKernel.c b/drivers/staging/epl/EplApiLinuxKernel.c
index 05ca062..9517da2 100644
--- a/drivers/staging/epl/EplApiLinuxKernel.c
+++ b/drivers/staging/epl/EplApiLinuxKernel.c
@@ -219,7 +219,7 @@ static ssize_t EplLinRead(struct file *pInstance_p, char *pDstBuff_p,
size_t BuffSize_p, loff_t * pFileOffs_p);
static ssize_t EplLinWrite(struct file *pInstance_p, const char *pSrcBuff_p,
size_t BuffSize_p, loff_t * pFileOffs_p);
-static int EplLinIoctl(struct inode *pDeviceFile_p, struct file *pInstance_p,
+static long EplLinIoctl(struct file *pInstance_p,
unsigned int uiIoctlCmd_p, unsigned long ulArg_p);
//---------------------------------------------------------------------------
@@ -237,7 +237,7 @@ static struct file_operations EplLinFileOps_g = {
.release = EplLinRelease,
.read = EplLinRead,
.write = EplLinWrite,
- .ioctl = EplLinIoctl,
+ .unlocked_ioctl = EplLinIoctl,
};
@@ -551,12 +551,13 @@ static ssize_t EplLinWrite(struct file *pInstance_p, // information about driver
// -> ioctl(...)
//---------------------------------------------------------------------------
-static int EplLinIoctl(struct inode *pDeviceFile_p, // information about the device to open
- struct file *pInstance_p, // information about driver instance
+static long EplLinIoctl(struct file *pInstance_p, // information about driver instance
unsigned int uiIoctlCmd_p, // Ioctl command to execute
unsigned long ulArg_p) // Ioctl command specific argument/parameter
{
+ lock_kernel();
+
tEplKernel EplRet;
int iErr;
int iRet;
@@ -1148,6 +1149,7 @@ static int EplLinIoctl(struct inode *pDeviceFile_p, // information about the dev
Exit:
// TRACE1("EPL: - EplLinIoctl (iRet=%d)\n", iRet);
+ unlock_kernel();
return (iRet);
}
diff --git a/drivers/staging/meilhaus/memain.c b/drivers/staging/meilhaus/memain.c
index b09d1a6..9a1706b 100644
--- a/drivers/staging/meilhaus/memain.c
+++ b/drivers/staging/meilhaus/memain.c
@@ -95,7 +95,7 @@ static int replace_with_dummy(int vendor_id, int device_id, int serial_no);
static void clear_device_list(void);
static int me_open(struct inode *inode_ptr, struct file *filep);
static int me_release(struct inode *, struct file *);
-static int me_ioctl(struct inode *, struct file *, unsigned int, unsigned long);
+static long me_ioctl(struct file *, unsigned int, unsigned long);
//static int me_probe_usb(struct usb_interface *interface, const struct usb_device_id *id);
//static void me_disconnect_usb(struct usb_interface *interface);
@@ -109,7 +109,7 @@ static struct cdev *cdevp;
static struct file_operations me_file_operations = {
.owner = THIS_MODULE,
- .ioctl = me_ioctl,
+ .unlocked_ioctl = me_ioctl,
.open = me_open,
.release = me_release,
};
@@ -1751,14 +1751,17 @@ static void me_disconnect_usb(struct usb_interface *interface)
}
*/
-static int me_ioctl(struct inode *inodep,
- struct file *filep, unsigned int service, unsigned long arg)
+static long me_ioctl(struct file *filep, unsigned int service,
+ unsigned long arg)
{
+ lock_kernel();
+ long ret;
PDEBUG("executed.\n");
if (_IOC_TYPE(service) != MEMAIN_MAGIC) {
PERROR("Invalid magic number.\n");
+ unlock_kernel();
return -ENOTTY;
}
@@ -1766,147 +1769,186 @@ static int me_ioctl(struct inode *inodep,
switch (service) {
case ME_IO_IRQ_ENABLE:
- return me_io_irq_start(filep, (me_io_irq_start_t *) arg);
+ ret = me_io_irq_start(filep, (me_io_irq_start_t *) arg);
+ break;
case ME_IO_IRQ_WAIT:
- return me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);
+ ret = me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);
+ break;
case ME_IO_IRQ_DISABLE:
- return me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);
+ ret = me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);
+ break;
case ME_IO_RESET_DEVICE:
- return me_io_reset_device(filep, (me_io_reset_device_t *) arg);
+ ret = me_io_reset_device(filep, (me_io_reset_device_t *) arg);
+ break;
case ME_IO_RESET_SUBDEVICE:
- return me_io_reset_subdevice(filep,
+ ret = me_io_reset_subdevice(filep,
(me_io_reset_subdevice_t *) arg);
+ break;
case ME_IO_SINGLE_CONFIG:
- return me_io_single_config(filep,
+ ret = me_io_single_config(filep,
(me_io_single_config_t *) arg);
+ break;
case ME_IO_SINGLE:
- return me_io_single(filep, (me_io_single_t *) arg);
+ ret = me_io_single(filep, (me_io_single_t *) arg);
+ break;
case ME_IO_STREAM_CONFIG:
- return me_io_stream_config(filep,
+ ret = me_io_stream_config(filep,
(me_io_stream_config_t *) arg);
+ break;
case ME_IO_STREAM_NEW_VALUES:
- return me_io_stream_new_values(filep,
+ ret = me_io_stream_new_values(filep,
(me_io_stream_new_values_t *)
arg);
+ break;
case ME_IO_STREAM_READ:
- return me_io_stream_read(filep, (me_io_stream_read_t *) arg);
+ ret = me_io_stream_read(filep, (me_io_stream_read_t *) arg);
+ break;
case ME_IO_STREAM_START:
- return me_io_stream_start(filep, (me_io_stream_start_t *) arg);
+ ret = me_io_stream_start(filep, (me_io_stream_start_t *) arg);
+ break;
case ME_IO_STREAM_STATUS:
- return me_io_stream_status(filep,
+ ret = me_io_stream_status(filep,
(me_io_stream_status_t *) arg);
+ break;
case ME_IO_STREAM_STOP:
- return me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);
+ ret = me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);
+ break;
case ME_IO_STREAM_WRITE:
- return me_io_stream_write(filep, (me_io_stream_write_t *) arg);
+ ret = me_io_stream_write(filep, (me_io_stream_write_t *) arg);
+ break;
case ME_LOCK_DRIVER:
- return me_lock_driver(filep, (me_lock_driver_t *) arg);
+ ret = me_lock_driver(filep, (me_lock_driver_t *) arg);
+ break;
case ME_LOCK_DEVICE:
- return me_lock_device(filep, (me_lock_device_t *) arg);
+ ret = me_lock_device(filep, (me_lock_device_t *) arg);
+ break;
case ME_LOCK_SUBDEVICE:
- return me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);
+ ret = me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);
+ break;
case ME_QUERY_INFO_DEVICE:
- return me_query_info_device(filep,
+ ret = me_query_info_device(filep,
(me_query_info_device_t *) arg);
+ break;
case ME_QUERY_DESCRIPTION_DEVICE:
- return me_query_description_device(filep,
+ ret = me_query_description_device(filep,
(me_query_description_device_t
*) arg);
+ break;
case ME_QUERY_NAME_DEVICE:
- return me_query_name_device(filep,
+ ret = me_query_name_device(filep,
(me_query_name_device_t *) arg);
+ break;
case ME_QUERY_NAME_DEVICE_DRIVER:
- return me_query_name_device_driver(filep,
+ ret = me_query_name_device_driver(filep,
(me_query_name_device_driver_t
*) arg);
+ break;
case ME_QUERY_NUMBER_DEVICES:
- return me_query_number_devices(filep,
+ ret = me_query_number_devices(filep,
(me_query_number_devices_t *)
arg);
+ break;
case ME_QUERY_NUMBER_SUBDEVICES:
- return me_query_number_subdevices(filep,
+ ret = me_query_number_subdevices(filep,
(me_query_number_subdevices_t
*) arg);
+ break;
case ME_QUERY_NUMBER_CHANNELS:
- return me_query_number_channels(filep,
+ ret = me_query_number_channels(filep,
(me_query_number_channels_t *)
arg);
+ break;
case ME_QUERY_NUMBER_RANGES:
- return me_query_number_ranges(filep,
+ ret = me_query_number_ranges(filep,
(me_query_number_ranges_t *) arg);
+ break;
case ME_QUERY_RANGE_BY_MIN_MAX:
- return me_query_range_by_min_max(filep,
+ ret = me_query_range_by_min_max(filep,
(me_query_range_by_min_max_t *)
arg);
+ break;
case ME_QUERY_RANGE_INFO:
- return me_query_range_info(filep,
+ ret = me_query_range_info(filep,
(me_query_range_info_t *) arg);
+ break;
case ME_QUERY_SUBDEVICE_BY_TYPE:
- return me_query_subdevice_by_type(filep,
+ ret = me_query_subdevice_by_type(filep,
(me_query_subdevice_by_type_t
*) arg);
+ break;
case ME_QUERY_SUBDEVICE_TYPE:
- return me_query_subdevice_type(filep,
+ ret = me_query_subdevice_type(filep,
(me_query_subdevice_type_t *)
arg);
+ break;
case ME_QUERY_SUBDEVICE_CAPS:
- return me_query_subdevice_caps(filep,
+ ret = me_query_subdevice_caps(filep,
(me_query_subdevice_caps_t *)
arg);
+ break;
case ME_QUERY_SUBDEVICE_CAPS_ARGS:
- return me_query_subdevice_caps_args(filep,
+ ret = me_query_subdevice_caps_args(filep,
(me_query_subdevice_caps_args_t
*) arg);
+ break;
case ME_QUERY_TIMER:
- return me_query_timer(filep, (me_query_timer_t *) arg);
+ ret = me_query_timer(filep, (me_query_timer_t *) arg);
+ break;
case ME_QUERY_VERSION_MAIN_DRIVER:
- return me_query_version_main_driver(filep,
+ ret = me_query_version_main_driver(filep,
(me_query_version_main_driver_t
*) arg);
+ break;
case ME_QUERY_VERSION_DEVICE_DRIVER:
- return me_query_version_device_driver(filep,
+ ret = me_query_version_device_driver(filep,
(me_query_version_device_driver_t
*) arg);
+ break;
case ME_CONFIG_LOAD:
- return me_config_load(filep, (me_config_load_t *) arg);
+ ret = me_config_load(filep, (me_config_load_t *) arg);
+ break;
+ }
+ if(!ret) {
+ PERROR("Invalid ioctl number.\n");
+ ret = -ENOTTY;
}
- PERROR("Invalid ioctl number.\n");
- return -ENOTTY;
+ unlock_kernel();
+ return ret;
}
// Init and exit of module.
diff --git a/drivers/staging/poch/poch.c b/drivers/staging/poch/poch.c
index 0d111dd..03cc5b7 100644
--- a/drivers/staging/poch/poch.c
+++ b/drivers/staging/poch/poch.c
@@ -979,9 +979,10 @@ static unsigned int poch_poll(struct file *filp, poll_table *pt)
return ret;
}
-static int poch_ioctl(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg)
+static long poch_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
{
+ lock_kernel();
struct channel_info *channel = filp->private_data;
void __iomem *fpga = channel->fpga_iomem;
void __iomem *bridge = channel->bridge_iomem;
@@ -1026,8 +1027,10 @@ static int poch_ioctl(struct inode *inode, struct file *filp,
}
break;
case POCH_IOC_GET_COUNTERS:
- if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters)))
+ if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters))) {
+ unlock_kernel();
return -EFAULT;
+ }
spin_lock_irq(&channel->counters_lock);
counters = channel->counters;
@@ -1036,23 +1039,31 @@ static int poch_ioctl(struct inode *inode, struct file *filp,
ret = copy_to_user(argp, &counters,
sizeof(struct poch_counters));
- if (ret)
+ if (ret) {
+ unlock_kernel();
return ret;
+ }
break;
case POCH_IOC_SYNC_GROUP_FOR_USER:
case POCH_IOC_SYNC_GROUP_FOR_DEVICE:
vms = find_vma(current->mm, arg);
- if (!vms)
+ if (!vms) {
+ unlock_kernel();
/* Address not mapped. */
return -EINVAL;
- if (vms->vm_file != filp)
+ }
+ if (vms->vm_file != filp) {
+ unlock_kernel();
/* Address mapped from different device/file. */
return -EINVAL;
+ }
flush_cache_range(vms, arg, arg + channel->group_size);
break;
}
+
+ unlock_kernel();
return 0;
}
@@ -1060,7 +1071,7 @@ static struct file_operations poch_fops = {
.owner = THIS_MODULE,
.open = poch_open,
.release = poch_release,
- .ioctl = poch_ioctl,
+ .unlocked_ioctl = poch_ioctl,
.poll = poch_poll,
.mmap = poch_mmap
};
--
1.6.2
On Tue, Mar 24, 2009 at 04:12:36PM -0500, [email protected] wrote:
> From: Stoyan Gaydarov <[email protected]>
>
> Signed-off-by: Stoyan Gaydarov <[email protected]>
Hm, why? What does this patch accomplish becides just pushing the
lock_kernel into the driver? Shouldn't you fix the code to not need the
lock_kernel at all instead?
thanks,
greg k-h
On Tue, 24 Mar 2009 15:01:36 -0700
Greg KH <[email protected]> wrote:
> Hm, why? What does this patch accomplish becides just pushing the
> lock_kernel into the driver? Shouldn't you fix the code to not need
> the lock_kernel at all instead?
Having done a lot of this myself, I can attest to what is going on
here. If you're trying to push the BKL out of the core, there's not
much choice except to shove it down into the drivers and such below
it. In very simple cases you can tell that the lock isn't needed.
But going messing around with the locking in random drivers is a sure
way to create nasty subtle problems.
So, if a specific driver's situation is not obvious, or if it's clear
that some sort of fix is required, it's generally best to just make the
lock_kernel() (which has always been there) explicit. Then people who
actually know the driver and have the hardware to test changes can
eliminate it at their leisure. Pushing the BKL down gets it out of the
core, keeps the semantics the same, and sticks a red flag on code which
needs examination by suitably clueful maintainers.
I've not looked in depth at this series of patches, but I understand
where it's coming from. Stoyan, if you want, I'd be happy to take
these through the BKL-removal tree once the maintainers are happy with
it. OTOH, it looks like the locked ioctl() isn't going anywhere
anytime soon, so it might be best to just send these individually
through the appropriate subsystem trees.
jon
OK, looking at the patch now...
> drivers/staging/epl/EplApiLinuxKernel.c | 10 ++-
> drivers/staging/meilhaus/memain.c | 124 +++++++++++++++++++++----------
> drivers/staging/poch/poch.c | 25 +++++--
> 3 files changed, 107 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/staging/epl/EplApiLinuxKernel.c b/drivers/staging/epl/EplApiLinuxKernel.c
> index 05ca062..9517da2 100644
> --- a/drivers/staging/epl/EplApiLinuxKernel.c
> +++ b/drivers/staging/epl/EplApiLinuxKernel.c
> @@ -219,7 +219,7 @@ static ssize_t EplLinRead(struct file *pInstance_p, char *pDstBuff_p,
> size_t BuffSize_p, loff_t * pFileOffs_p);
> static ssize_t EplLinWrite(struct file *pInstance_p, const char *pSrcBuff_p,
> size_t BuffSize_p, loff_t * pFileOffs_p);
> -static int EplLinIoctl(struct inode *pDeviceFile_p, struct file *pInstance_p,
> +static long EplLinIoctl(struct file *pInstance_p,
> unsigned int uiIoctlCmd_p, unsigned long ulArg_p);
>
> //---------------------------------------------------------------------------
> @@ -237,7 +237,7 @@ static struct file_operations EplLinFileOps_g = {
> .release = EplLinRelease,
> .read = EplLinRead,
> .write = EplLinWrite,
> - .ioctl = EplLinIoctl,
> + .unlocked_ioctl = EplLinIoctl,
>
> };
>
> @@ -551,12 +551,13 @@ static ssize_t EplLinWrite(struct file *pInstance_p, // information about driver
> // -> ioctl(...)
> //---------------------------------------------------------------------------
>
> -static int EplLinIoctl(struct inode *pDeviceFile_p, // information about the device to open
> - struct file *pInstance_p, // information about driver instance
> +static long EplLinIoctl(struct file *pInstance_p, // information about driver instance
> unsigned int uiIoctlCmd_p, // Ioctl command to execute
> unsigned long ulArg_p) // Ioctl command specific argument/parameter
> {
>
> + lock_kernel();
> +
> tEplKernel EplRet;
> int iErr;
> int iRet;
> @@ -1148,6 +1149,7 @@ static int EplLinIoctl(struct inode *pDeviceFile_p, // information about the dev
> Exit:
>
> // TRACE1("EPL: - EplLinIoctl (iRet=%d)\n", iRet);
> + unlock_kernel();
> return (iRet);
>
> }
Gee what ugly code you're working on here. It's like...crap or
something...:)
You should avoid making it worse by putting lock_kernel() above the
variable declarations, though. That will set off alarms for sure, even in
-staging code.
> diff --git a/drivers/staging/meilhaus/memain.c b/drivers/staging/meilhaus/memain.c
> index b09d1a6..9a1706b 100644
> --- a/drivers/staging/meilhaus/memain.c
> +++ b/drivers/staging/meilhaus/memain.c
> @@ -95,7 +95,7 @@ static int replace_with_dummy(int vendor_id, int device_id, int serial_no);
> static void clear_device_list(void);
> static int me_open(struct inode *inode_ptr, struct file *filep);
> static int me_release(struct inode *, struct file *);
> -static int me_ioctl(struct inode *, struct file *, unsigned int, unsigned long);
> +static long me_ioctl(struct file *, unsigned int, unsigned long);
> //static int me_probe_usb(struct usb_interface *interface, const struct usb_device_id *id);
> //static void me_disconnect_usb(struct usb_interface *interface);
>
> @@ -109,7 +109,7 @@ static struct cdev *cdevp;
>
> static struct file_operations me_file_operations = {
> .owner = THIS_MODULE,
> - .ioctl = me_ioctl,
> + .unlocked_ioctl = me_ioctl,
> .open = me_open,
> .release = me_release,
> };
> @@ -1751,14 +1751,17 @@ static void me_disconnect_usb(struct usb_interface *interface)
> }
> */
>
> -static int me_ioctl(struct inode *inodep,
> - struct file *filep, unsigned int service, unsigned long arg)
> +static long me_ioctl(struct file *filep, unsigned int service,
> + unsigned long arg)
> {
> + lock_kernel();
> + long ret;
Please declare the variable first.
> PDEBUG("executed.\n");
>
> if (_IOC_TYPE(service) != MEMAIN_MAGIC) {
> PERROR("Invalid magic number.\n");
> + unlock_kernel();
> return -ENOTTY;
> }
You also clearly do not need to do lock_kernel() before this test, so you
could move it down here and avoid one unlock/return sequence.
> @@ -1766,147 +1769,186 @@ static int me_ioctl(struct inode *inodep,
>
> switch (service) {
> case ME_IO_IRQ_ENABLE:
> - return me_io_irq_start(filep, (me_io_irq_start_t *) arg);
> + ret = me_io_irq_start(filep, (me_io_irq_start_t *) arg);
> + break;
>
> case ME_IO_IRQ_WAIT:
> - return me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);
> + ret = me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);
> + break;
>
> case ME_IO_IRQ_DISABLE:
> - return me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);
> + ret = me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);
> + break;
>
> case ME_IO_RESET_DEVICE:
> - return me_io_reset_device(filep, (me_io_reset_device_t *) arg);
> + ret = me_io_reset_device(filep, (me_io_reset_device_t *) arg);
> + break;
>
> case ME_IO_RESET_SUBDEVICE:
> - return me_io_reset_subdevice(filep,
> + ret = me_io_reset_subdevice(filep,
> (me_io_reset_subdevice_t *) arg);
> + break;
>
> case ME_IO_SINGLE_CONFIG:
> - return me_io_single_config(filep,
> + ret = me_io_single_config(filep,
> (me_io_single_config_t *) arg);
> + break;
>
> case ME_IO_SINGLE:
> - return me_io_single(filep, (me_io_single_t *) arg);
> + ret = me_io_single(filep, (me_io_single_t *) arg);
> + break;
>
> case ME_IO_STREAM_CONFIG:
> - return me_io_stream_config(filep,
> + ret = me_io_stream_config(filep,
> (me_io_stream_config_t *) arg);
> + break;
>
> case ME_IO_STREAM_NEW_VALUES:
> - return me_io_stream_new_values(filep,
> + ret = me_io_stream_new_values(filep,
> (me_io_stream_new_values_t *)
> arg);
> + break;
>
> case ME_IO_STREAM_READ:
> - return me_io_stream_read(filep, (me_io_stream_read_t *) arg);
> + ret = me_io_stream_read(filep, (me_io_stream_read_t *) arg);
> + break;
>
> case ME_IO_STREAM_START:
> - return me_io_stream_start(filep, (me_io_stream_start_t *) arg);
> + ret = me_io_stream_start(filep, (me_io_stream_start_t *) arg);
> + break;
>
> case ME_IO_STREAM_STATUS:
> - return me_io_stream_status(filep,
> + ret = me_io_stream_status(filep,
> (me_io_stream_status_t *) arg);
> + break;
>
> case ME_IO_STREAM_STOP:
> - return me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);
> + ret = me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);
> + break;
>
> case ME_IO_STREAM_WRITE:
> - return me_io_stream_write(filep, (me_io_stream_write_t *) arg);
> + ret = me_io_stream_write(filep, (me_io_stream_write_t *) arg);
> + break;
>
> case ME_LOCK_DRIVER:
> - return me_lock_driver(filep, (me_lock_driver_t *) arg);
> + ret = me_lock_driver(filep, (me_lock_driver_t *) arg);
> + break;
>
> case ME_LOCK_DEVICE:
> - return me_lock_device(filep, (me_lock_device_t *) arg);
> + ret = me_lock_device(filep, (me_lock_device_t *) arg);
> + break;
>
> case ME_LOCK_SUBDEVICE:
> - return me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);
> + ret = me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);
> + break;
>
> case ME_QUERY_INFO_DEVICE:
> - return me_query_info_device(filep,
> + ret = me_query_info_device(filep,
> (me_query_info_device_t *) arg);
> + break;
>
> case ME_QUERY_DESCRIPTION_DEVICE:
> - return me_query_description_device(filep,
> + ret = me_query_description_device(filep,
> (me_query_description_device_t
> *) arg);
> + break;
>
> case ME_QUERY_NAME_DEVICE:
> - return me_query_name_device(filep,
> + ret = me_query_name_device(filep,
> (me_query_name_device_t *) arg);
> + break;
>
> case ME_QUERY_NAME_DEVICE_DRIVER:
> - return me_query_name_device_driver(filep,
> + ret = me_query_name_device_driver(filep,
> (me_query_name_device_driver_t
> *) arg);
> + break;
>
> case ME_QUERY_NUMBER_DEVICES:
> - return me_query_number_devices(filep,
> + ret = me_query_number_devices(filep,
> (me_query_number_devices_t *)
> arg);
> + break;
>
> case ME_QUERY_NUMBER_SUBDEVICES:
> - return me_query_number_subdevices(filep,
> + ret = me_query_number_subdevices(filep,
> (me_query_number_subdevices_t
> *) arg);
> + break;
>
> case ME_QUERY_NUMBER_CHANNELS:
> - return me_query_number_channels(filep,
> + ret = me_query_number_channels(filep,
> (me_query_number_channels_t *)
> arg);
> + break;
>
> case ME_QUERY_NUMBER_RANGES:
> - return me_query_number_ranges(filep,
> + ret = me_query_number_ranges(filep,
> (me_query_number_ranges_t *) arg);
> + break;
>
> case ME_QUERY_RANGE_BY_MIN_MAX:
> - return me_query_range_by_min_max(filep,
> + ret = me_query_range_by_min_max(filep,
> (me_query_range_by_min_max_t *)
> arg);
> + break;
>
> case ME_QUERY_RANGE_INFO:
> - return me_query_range_info(filep,
> + ret = me_query_range_info(filep,
> (me_query_range_info_t *) arg);
> + break;
>
> case ME_QUERY_SUBDEVICE_BY_TYPE:
> - return me_query_subdevice_by_type(filep,
> + ret = me_query_subdevice_by_type(filep,
> (me_query_subdevice_by_type_t
> *) arg);
> + break;
>
> case ME_QUERY_SUBDEVICE_TYPE:
> - return me_query_subdevice_type(filep,
> + ret = me_query_subdevice_type(filep,
> (me_query_subdevice_type_t *)
> arg);
> + break;
>
> case ME_QUERY_SUBDEVICE_CAPS:
> - return me_query_subdevice_caps(filep,
> + ret = me_query_subdevice_caps(filep,
> (me_query_subdevice_caps_t *)
> arg);
> + break;
>
> case ME_QUERY_SUBDEVICE_CAPS_ARGS:
> - return me_query_subdevice_caps_args(filep,
> + ret = me_query_subdevice_caps_args(filep,
> (me_query_subdevice_caps_args_t
> *) arg);
> + break;
>
> case ME_QUERY_TIMER:
> - return me_query_timer(filep, (me_query_timer_t *) arg);
> + ret = me_query_timer(filep, (me_query_timer_t *) arg);
> + break;
>
> case ME_QUERY_VERSION_MAIN_DRIVER:
> - return me_query_version_main_driver(filep,
> + ret = me_query_version_main_driver(filep,
> (me_query_version_main_driver_t
> *) arg);
> + break;
>
> case ME_QUERY_VERSION_DEVICE_DRIVER:
> - return me_query_version_device_driver(filep,
> + ret = me_query_version_device_driver(filep,
> (me_query_version_device_driver_t
> *) arg);
> + break;
>
> case ME_CONFIG_LOAD:
> - return me_config_load(filep, (me_config_load_t *) arg);
> + ret = me_config_load(filep, (me_config_load_t *) arg);
> + break;
> + }
> + if(!ret) {
> + PERROR("Invalid ioctl number.\n");
> + ret = -ENOTTY;
> }
Actually, you've changed the control flow a bit, and probably broken the
function too. If it's truly an invalid ioctl number, you'll just drop out
of the switch - there's no default branch. But ret will be a random value
in that case.
In every other case, you take a return code of zero (which means success)
and map it onto -ENOTTY. Probably not the right thing to do.
> - PERROR("Invalid ioctl number.\n");
> - return -ENOTTY;
> + unlock_kernel();
> + return ret;
> }
>
> // Init and exit of module.
> diff --git a/drivers/staging/poch/poch.c b/drivers/staging/poch/poch.c
> index 0d111dd..03cc5b7 100644
> --- a/drivers/staging/poch/poch.c
> +++ b/drivers/staging/poch/poch.c
> @@ -979,9 +979,10 @@ static unsigned int poch_poll(struct file *filp, poll_table *pt)
> return ret;
> }
>
> -static int poch_ioctl(struct inode *inode, struct file *filp,
> - unsigned int cmd, unsigned long arg)
> +static long poch_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> {
> + lock_kernel();
> struct channel_info *channel = filp->private_data;
> void __iomem *fpga = channel->fpga_iomem;
> void __iomem *bridge = channel->bridge_iomem;
After declarations, please. If you're not sure about accesses to channel,
you need to separate the declarations and the initializations of the
variables.
> @@ -1026,8 +1027,10 @@ static int poch_ioctl(struct inode *inode, struct file *filp,
> }
> break;
> case POCH_IOC_GET_COUNTERS:
> - if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters)))
> + if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters))) {
> + unlock_kernel();
> return -EFAULT;
> + }
In general, you want to have a single unlock/return at the end, and use
goto to get there from inside the function. Lots of returns from the
middle of a function which takes locks can lead to grief sooner or later.
If there's only one path out, things are harder to break.
> spin_lock_irq(&channel->counters_lock);
> counters = channel->counters;
> @@ -1036,23 +1039,31 @@ static int poch_ioctl(struct inode *inode, struct file *filp,
>
> ret = copy_to_user(argp, &counters,
> sizeof(struct poch_counters));
> - if (ret)
> + if (ret) {
> + unlock_kernel();
> return ret;
> + }
>
> break;
> case POCH_IOC_SYNC_GROUP_FOR_USER:
> case POCH_IOC_SYNC_GROUP_FOR_DEVICE:
> vms = find_vma(current->mm, arg);
> - if (!vms)
> + if (!vms) {
> + unlock_kernel();
> /* Address not mapped. */
> return -EINVAL;
> - if (vms->vm_file != filp)
> + }
> + if (vms->vm_file != filp) {
> + unlock_kernel();
> /* Address mapped from different device/file. */
> return -EINVAL;
> + }
>
> flush_cache_range(vms, arg, arg + channel->group_size);
> break;
> }
> +
> + unlock_kernel();
> return 0;
> }
This driver looks like it has other locking problems, incidentally; it has
a spinlock for its counters, but there's nothing serializing access to the
device registers.
jon
On Tue, Mar 24, 2009 at 6:40 PM, Jonathan Corbet <[email protected]> wrote:> OK, looking at the patch now...>>> drivers/staging/epl/EplApiLinuxKernel.c | 10 ++->> drivers/staging/meilhaus/memain.c | 124 +++++++++++++++++++++---------->> drivers/staging/poch/poch.c | 25 +++++-->> 3 files changed, 107 insertions(+), 52 deletions(-)>>>> diff --git a/drivers/staging/epl/EplApiLinuxKernel.c b/drivers/staging/epl/EplApiLinuxKernel.c>> index 05ca062..9517da2 100644>> --- a/drivers/staging/epl/EplApiLinuxKernel.c>> +++ b/drivers/staging/epl/EplApiLinuxKernel.c>> @@ -219,7 +219,7 @@ static ssize_t EplLinRead(struct file *pInstance_p, char *pDstBuff_p,>> size_t BuffSize_p, loff_t * pFileOffs_p);>> static ssize_t EplLinWrite(struct file *pInstance_p, const char *pSrcBuff_p,>> size_t BuffSize_p, loff_t * pFileOffs_p);>> -static int EplLinIoctl(struct inode *pDeviceFile_p, struct file *pInstance_p,>> +static long EplLinIoctl(struct file *pInstance_p,>> unsigned int uiIoctlCmd_p, unsigned long ulArg_p);>>>> //--------------------------------------------------------------------------->> @@ -237,7 +237,7 @@ static struct file_operations EplLinFileOps_g = {>> .release = EplLinRelease,>> .read = EplLinRead,>> .write = EplLinWrite,>> - .ioctl = EplLinIoctl,>> + .unlocked_ioctl = EplLinIoctl,>>>> };>>>> @@ -551,12 +551,13 @@ static ssize_t EplLinWrite(struct file *pInstance_p, // information about driver>> // -> ioctl(...)>> //--------------------------------------------------------------------------->>>> -static int EplLinIoctl(struct inode *pDeviceFile_p, // information about the device to open>> - struct file *pInstance_p, // information about driver instance>> +static long EplLinIoctl(struct file *pInstance_p, // information about driver instance>> unsigned int uiIoctlCmd_p, // Ioctl command to execute>> unsigned long ulArg_p) // Ioctl command specific argument/parameter>> {>>>> + lock_kernel();>> +>> tEplKernel EplRet;>> int iErr;>> int iRet;>> @@ -1148,6 +1149,7 @@ static int EplLinIoctl(struct inode *pDeviceFile_p, // information about the dev>> Exit:>>>> // TRACE1("EPL: - EplLinIoctl (iRet=%d)\n", iRet);>> + unlock_kernel();>> return (iRet);>>>> }>> Gee what ugly code you're working on here. It's like...crap or> something...:)>> You should avoid making it worse by putting lock_kernel() above the> variable declarations, though. That will set off alarms for sure, even in> -staging code.>>> diff --git a/drivers/staging/meilhaus/memain.c b/drivers/staging/meilhaus/memain.c>> index b09d1a6..9a1706b 100644>> --- a/drivers/staging/meilhaus/memain.c>> +++ b/drivers/staging/meilhaus/memain.c>> @@ -95,7 +95,7 @@ static int replace_with_dummy(int vendor_id, int device_id, int serial_no);>> static void clear_device_list(void);>> static int me_open(struct inode *inode_ptr, struct file *filep);>> static int me_release(struct inode *, struct file *);>> -static int me_ioctl(struct inode *, struct file *, unsigned int, unsigned long);>> +static long me_ioctl(struct file *, unsigned int, unsigned long);>> //static int me_probe_usb(struct usb_interface *interface, const struct usb_device_id *id);>> //static void me_disconnect_usb(struct usb_interface *interface);>>>> @@ -109,7 +109,7 @@ static struct cdev *cdevp;>>>> static struct file_operations me_file_operations = {>> .owner = THIS_MODULE,>> - .ioctl = me_ioctl,>> + .unlocked_ioctl = me_ioctl,>> .open = me_open,>> .release = me_release,>> };>> @@ -1751,14 +1751,17 @@ static void me_disconnect_usb(struct usb_interface *interface)>> }>> */>>>> -static int me_ioctl(struct inode *inodep,>> - struct file *filep, unsigned int service, unsigned long arg)>> +static long me_ioctl(struct file *filep, unsigned int service,>> + unsigned long arg)>> {>> + lock_kernel();>> + long ret;>> Please declare the variable first.>>> PDEBUG("executed.\n");>>>> if (_IOC_TYPE(service) != MEMAIN_MAGIC) {>> PERROR("Invalid magic number.\n");>> + unlock_kernel();>> return -ENOTTY;>> }>> You also clearly do not need to do lock_kernel() before this test, so you> could move it down here and avoid one unlock/return sequence.>>> @@ -1766,147 +1769,186 @@ static int me_ioctl(struct inode *inodep,>>>> switch (service) {>> case ME_IO_IRQ_ENABLE:>> - return me_io_irq_start(filep, (me_io_irq_start_t *) arg);>> + ret = me_io_irq_start(filep, (me_io_irq_start_t *) arg);>> + break;>>>> case ME_IO_IRQ_WAIT:>> - return me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);>> + ret = me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);>> + break;>>>> case ME_IO_IRQ_DISABLE:>> - return me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);>> + ret = me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);>> + break;>>>> case ME_IO_RESET_DEVICE:>> - return me_io_reset_device(filep, (me_io_reset_device_t *) arg);>> + ret = me_io_reset_device(filep, (me_io_reset_device_t *) arg);>> + break;>>>> case ME_IO_RESET_SUBDEVICE:>> - return me_io_reset_subdevice(filep,>> + ret = me_io_reset_subdevice(filep,>> (me_io_reset_subdevice_t *) arg);>> + break;>>>> case ME_IO_SINGLE_CONFIG:>> - return me_io_single_config(filep,>> + ret = me_io_single_config(filep,>> (me_io_single_config_t *) arg);>> + break;>>>> case ME_IO_SINGLE:>> - return me_io_single(filep, (me_io_single_t *) arg);>> + ret = me_io_single(filep, (me_io_single_t *) arg);>> + break;>>>> case ME_IO_STREAM_CONFIG:>> - return me_io_stream_config(filep,>> + ret = me_io_stream_config(filep,>> (me_io_stream_config_t *) arg);>> + break;>>>> case ME_IO_STREAM_NEW_VALUES:>> - return me_io_stream_new_values(filep,>> + ret = me_io_stream_new_values(filep,>> (me_io_stream_new_values_t *)>> arg);>> + break;>>>> case ME_IO_STREAM_READ:>> - return me_io_stream_read(filep, (me_io_stream_read_t *) arg);>> + ret = me_io_stream_read(filep, (me_io_stream_read_t *) arg);>> + break;>>>> case ME_IO_STREAM_START:>> - return me_io_stream_start(filep, (me_io_stream_start_t *) arg);>> + ret = me_io_stream_start(filep, (me_io_stream_start_t *) arg);>> + break;>>>> case ME_IO_STREAM_STATUS:>> - return me_io_stream_status(filep,>> + ret = me_io_stream_status(filep,>> (me_io_stream_status_t *) arg);>> + break;>>>> case ME_IO_STREAM_STOP:>> - return me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);>> + ret = me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);>> + break;>>>> case ME_IO_STREAM_WRITE:>> - return me_io_stream_write(filep, (me_io_stream_write_t *) arg);>> + ret = me_io_stream_write(filep, (me_io_stream_write_t *) arg);>> + break;>>>> case ME_LOCK_DRIVER:>> - return me_lock_driver(filep, (me_lock_driver_t *) arg);>> + ret = me_lock_driver(filep, (me_lock_driver_t *) arg);>> + break;>>>> case ME_LOCK_DEVICE:>> - return me_lock_device(filep, (me_lock_device_t *) arg);>> + ret = me_lock_device(filep, (me_lock_device_t *) arg);>> + break;>>>> case ME_LOCK_SUBDEVICE:>> - return me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);>> + ret = me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);>> + break;>>>> case ME_QUERY_INFO_DEVICE:>> - return me_query_info_device(filep,>> + ret = me_query_info_device(filep,>> (me_query_info_device_t *) arg);>> + break;>>>> case ME_QUERY_DESCRIPTION_DEVICE:>> - return me_query_description_device(filep,>> + ret = me_query_description_device(filep,>> (me_query_description_device_t>> *) arg);>> + break;>>>> case ME_QUERY_NAME_DEVICE:>> - return me_query_name_device(filep,>> + ret = me_query_name_device(filep,>> (me_query_name_device_t *) arg);>> + break;>>>> case ME_QUERY_NAME_DEVICE_DRIVER:>> - return me_query_name_device_driver(filep,>> + ret = me_query_name_device_driver(filep,>> (me_query_name_device_driver_t>> *) arg);>> + break;>>>> case ME_QUERY_NUMBER_DEVICES:>> - return me_query_number_devices(filep,>> + ret = me_query_number_devices(filep,>> (me_query_number_devices_t *)>> arg);>> + break;>>>> case ME_QUERY_NUMBER_SUBDEVICES:>> - return me_query_number_subdevices(filep,>> + ret = me_query_number_subdevices(filep,>> (me_query_number_subdevices_t>> *) arg);>> + break;>>>> case ME_QUERY_NUMBER_CHANNELS:>> - return me_query_number_channels(filep,>> + ret = me_query_number_channels(filep,>> (me_query_number_channels_t *)>> arg);>> + break;>>>> case ME_QUERY_NUMBER_RANGES:>> - return me_query_number_ranges(filep,>> + ret = me_query_number_ranges(filep,>> (me_query_number_ranges_t *) arg);>> + break;>>>> case ME_QUERY_RANGE_BY_MIN_MAX:>> - return me_query_range_by_min_max(filep,>> + ret = me_query_range_by_min_max(filep,>> (me_query_range_by_min_max_t *)>> arg);>> + break;>>>> case ME_QUERY_RANGE_INFO:>> - return me_query_range_info(filep,>> + ret = me_query_range_info(filep,>> (me_query_range_info_t *) arg);>> + break;>>>> case ME_QUERY_SUBDEVICE_BY_TYPE:>> - return me_query_subdevice_by_type(filep,>> + ret = me_query_subdevice_by_type(filep,>> (me_query_subdevice_by_type_t>> *) arg);>> + break;>>>> case ME_QUERY_SUBDEVICE_TYPE:>> - return me_query_subdevice_type(filep,>> + ret = me_query_subdevice_type(filep,>> (me_query_subdevice_type_t *)>> arg);>> + break;>>>> case ME_QUERY_SUBDEVICE_CAPS:>> - return me_query_subdevice_caps(filep,>> + ret = me_query_subdevice_caps(filep,>> (me_query_subdevice_caps_t *)>> arg);>> + break;>>>> case ME_QUERY_SUBDEVICE_CAPS_ARGS:>> - return me_query_subdevice_caps_args(filep,>> + ret = me_query_subdevice_caps_args(filep,>> (me_query_subdevice_caps_args_t>> *) arg);>> + break;>>>> case ME_QUERY_TIMER:>> - return me_query_timer(filep, (me_query_timer_t *) arg);>> + ret = me_query_timer(filep, (me_query_timer_t *) arg);>> + break;>>>> case ME_QUERY_VERSION_MAIN_DRIVER:>> - return me_query_version_main_driver(filep,>> + ret = me_query_version_main_driver(filep,>> (me_query_version_main_driver_t>> *) arg);>> + break;>>>> case ME_QUERY_VERSION_DEVICE_DRIVER:>> - return me_query_version_device_driver(filep,>> + ret = me_query_version_device_driver(filep,>> (me_query_version_device_driver_t>> *) arg);>> + break;>>>> case ME_CONFIG_LOAD:>> - return me_config_load(filep, (me_config_load_t *) arg);>> + ret = me_config_load(filep, (me_config_load_t *) arg);>> + break;>> + }>> + if(!ret) {>> + PERROR("Invalid ioctl number.\n");>> + ret = -ENOTTY;>> }>> Actually, you've changed the control flow a bit, and probably broken the> function too. If it's truly an invalid ioctl number, you'll just drop out> of the switch - there's no default branch. But ret will be a random value> in that case.>> In every other case, you take a return code of zero (which means success)> and map it onto -ENOTTY. Probably not the right thing to do.
I have changed this to add a default so if its invalid it will do whatits supposed to
>>> - PERROR("Invalid ioctl number.\n");>> - return -ENOTTY;>> + unlock_kernel();>> + return ret;>> }>>>> // Init and exit of module.>> diff --git a/drivers/staging/poch/poch.c b/drivers/staging/poch/poch.c>> index 0d111dd..03cc5b7 100644>> --- a/drivers/staging/poch/poch.c>> +++ b/drivers/staging/poch/poch.c>> @@ -979,9 +979,10 @@ static unsigned int poch_poll(struct file *filp, poll_table *pt)>> return ret;>> }>>>> -static int poch_ioctl(struct inode *inode, struct file *filp,>> - unsigned int cmd, unsigned long arg)>> +static long poch_ioctl(struct file *filp, unsigned int cmd,>> + unsigned long arg)>> {>> + lock_kernel();>> struct channel_info *channel = filp->private_data;>> void __iomem *fpga = channel->fpga_iomem;>> void __iomem *bridge = channel->bridge_iomem;>> After declarations, please. If you're not sure about accesses to channel,> you need to separate the declarations and the initializations of the> variables.>>> @@ -1026,8 +1027,10 @@ static int poch_ioctl(struct inode *inode, struct file *filp,>> }>> break;>> case POCH_IOC_GET_COUNTERS:>> - if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters)))>> + if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters))) {>> + unlock_kernel();>> return -EFAULT;>> + }>> In general, you want to have a single unlock/return at the end, and use> goto to get there from inside the function. Lots of returns from the> middle of a function which takes locks can lead to grief sooner or later.> If there's only one path out, things are harder to break.
I understand that but since this patch was just to move the BKL intoview, i wanted to change as little about the functions as possible,thats why the flow is not changed as much
>>> spin_lock_irq(&channel->counters_lock);>> counters = channel->counters;>> @@ -1036,23 +1039,31 @@ static int poch_ioctl(struct inode *inode, struct file *filp,>>>> ret = copy_to_user(argp, &counters,>> sizeof(struct poch_counters));>> - if (ret)>> + if (ret) {>> + unlock_kernel();>> return ret;>> + }>>>> break;>> case POCH_IOC_SYNC_GROUP_FOR_USER:>> case POCH_IOC_SYNC_GROUP_FOR_DEVICE:>> vms = find_vma(current->mm, arg);>> - if (!vms)>> + if (!vms) {>> + unlock_kernel();>> /* Address not mapped. */>> return -EINVAL;>> - if (vms->vm_file != filp)>> + }>> + if (vms->vm_file != filp) {>> + unlock_kernel();>> /* Address mapped from different device/file. */>> return -EINVAL;>> + }>>>> flush_cache_range(vms, arg, arg + channel->group_size);>> break;>> }>> +>> + unlock_kernel();>> return 0;>> }>> This driver looks like it has other locking problems, incidentally; it has> a spinlock for its counters, but there's nothing serializing access to the> device registers.>> jon>
I have made some modifications and will resubmit this patch.
--
-Stoyan????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?