Re: [PATCH 01/13] [staging] changed ioctls to unlocked

[Stoyan Gaydarov]

On Tue, Mar 24, 2009 at 6:40 PM, Jonathan Corbet <corbet@xxxxxxx> 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 what
its 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 into
view, 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
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_