Re: [PATCH 02/13] [cris/arch-v10] changed ioctls to unlocked

[Stoyan Gaydarov]

On Tue, Mar 24, 2009 at 6:59 PM, Jonathan Corbet <corbet@xxxxxxx> wrote:
> On Tue, 24 Mar 2009 16:12:37 -0500
> stoyboyker@xxxxxxxxx wrote:
>
>> From: Stoyan Gaydarov <stoyboyker@xxxxxxxxx>
>>
>> Signed-off-by: Stoyan Gaydarov <stoyboyker@xxxxxxxxx>
>> ---
>> Âarch/cris/arch-v10/drivers/ds1302.c   Â|  59 +++++++++++++++++++++---------
>> Âarch/cris/arch-v10/drivers/gpio.c    Â|  28 ++++++++------
>> Âarch/cris/arch-v10/drivers/pcf8563.c   |  33 +++++++++++++----
>> Âarch/cris/arch-v10/drivers/sync_serial.c | Â 18 ++++++---
>> Â4 files changed, 95 insertions(+), 43 deletions(-)
>>
>> diff --git a/arch/cris/arch-v10/drivers/ds1302.c b/arch/cris/arch-v10/drivers/ds1302.c
>> index 77630df..0260599 100644
>> --- a/arch/cris/arch-v10/drivers/ds1302.c
>> +++ b/arch/cris/arch-v10/drivers/ds1302.c
>> @@ -238,21 +238,25 @@ 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 long
>> +rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> Â{
>> + Â Â lock_kernel();
>> +
>> Â Â Â unsigned long flags;
>
> Define the variable first, please.
>
>> Â Â Â switch(cmd) {
>> Â Â Â Â Â Â Â case RTC_RD_TIME: Â Â Â /* read the time/date from RTC Â*/
>> Â Â Â Â Â Â Â {
>> Â Â Â Â Â Â Â Â Â Â Â struct rtc_time rtc_tm;
>> -
>> +
>> Â Â Â Â Â Â Â Â Â Â Â memset(&rtc_tm, 0, sizeof (struct rtc_time));
>> - Â Â Â Â Â Â Â Â Â Â get_rtc_time(&rtc_tm);
>> - Â Â Â Â Â Â Â Â Â Â if (copy_to_user((struct rtc_time*)arg, &rtc_tm, sizeof(struct rtc_time)))
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EFAULT;
>> + Â Â Â Â Â Â Â Â Â Â get_rtc_time(&rtc_tm);
>> + Â Â Â Â Â Â Â Â Â Â if (copy_to_user((struct rtc_time*)arg, &rtc_tm, sizeof(struct rtc_time))) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EFAULT;
>> + Â Â Â Â Â Â Â Â Â Â }
>> + Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>
> Again, please use the more standard idiom:
>
> Â Â Â Âretval = -EFAULT;
> Â Â Â Âgoto out;
>
> or some such. ÂAll these middle-of-function returns will bite you.
>
>> Â Â Â Â Â Â Â Â Â Â Â return 0;
>> Â Â Â Â Â Â Â }
>>
>> @@ -262,11 +266,15 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>> Â Â Â Â Â Â Â Â Â Â Â unsigned char mon, day, hrs, min, sec, leap_yr;
>> Â Â Â Â Â Â Â Â Â Â Â unsigned int yrs;
>>
>> - Â Â Â Â Â Â Â Â Â Â if (!capable(CAP_SYS_TIME))
>> + Â Â Â Â Â Â Â Â Â Â if (!capable(CAP_SYS_TIME)) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EPERM;
>> + Â Â Â Â Â Â Â Â Â Â }
>>
>> - Â Â Â Â Â Â Â Â Â Â if (copy_from_user(&rtc_tm, (struct rtc_time*)arg, sizeof(struct rtc_time)))
>> + Â Â Â Â Â Â Â Â Â Â if (copy_from_user(&rtc_tm, (struct rtc_time*)arg, sizeof(struct rtc_time))) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EFAULT;
>> + Â Â Â Â Â Â Â Â Â Â }
>>
>> Â Â Â Â Â Â Â Â Â Â Â yrs = rtc_tm.tm_year + 1900;
>> Â Â Â Â Â Â Â Â Â Â Â mon = rtc_tm.tm_mon + 1; Â /* tm_mon starts at zero */
>> @@ -276,19 +284,27 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>> Â Â Â Â Â Â Â Â Â Â Â sec = rtc_tm.tm_sec;
>>
>>
>> - Â Â Â Â Â Â Â Â Â Â if ((yrs < 1970) || (yrs > 2069))
>> + Â Â Â Â Â Â Â Â Â Â if ((yrs < 1970) || (yrs > 2069)) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EINVAL;
>> + Â Â Â Â Â Â Â Â Â Â }
>>
>> Â Â Â Â Â Â Â Â Â Â Â leap_yr = ((!(yrs % 4) && (yrs % 100)) || !(yrs % 400));
>>
>> - Â Â Â Â Â Â Â Â Â Â if ((mon > 12) || (day == 0))
>> + Â Â Â Â Â Â Â Â Â Â if ((mon > 12) || (day == 0)) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EINVAL;
>> + Â Â Â Â Â Â Â Â Â Â }
>>
>> - Â Â Â Â Â Â Â Â Â Â if (day > (days_in_mo[mon] + ((mon == 2) && leap_yr)))
>> + Â Â Â Â Â Â Â Â Â Â if (day > (days_in_mo[mon] + ((mon == 2) && leap_yr))) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EINVAL;
>> + Â Â Â Â Â Â Â Â Â Â }
>>
>> - Â Â Â Â Â Â Â Â Â Â if ((hrs >= 24) || (min >= 60) || (sec >= 60))
>> + Â Â Â Â Â Â Â Â Â Â if ((hrs >= 24) || (min >= 60) || (sec >= 60)) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EINVAL;
>> + Â Â Â Â Â Â Â Â Â Â }
>>
>> Â Â Â Â Â Â Â Â Â Â Â if (yrs >= 2000)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â yrs -= 2000; Â Â/* RTC (0, 1, ... 69) */
>> @@ -316,6 +332,7 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>> Â Â Â Â Â Â Â Â Â Â Â Â* You need to set that separately with settimeofday
>> Â Â Â Â Â Â Â Â Â Â Â Â* or adjtimex.
>> Â Â Â Â Â Â Â Â Â Â Â Â*/
>> + Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â return 0;
>> Â Â Â Â Â Â Â }
>>
>> @@ -323,14 +340,19 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>> Â Â Â Â Â Â Â {
>> Â Â Â Â Â Â Â Â Â Â Â int tcs_val;
>>
>> - Â Â Â Â Â Â Â Â Â Â if (!capable(CAP_SYS_TIME))
>> + Â Â Â Â Â Â Â Â Â Â if (!capable(CAP_SYS_TIME)) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EPERM;
>> + Â Â Â Â Â Â Â Â Â Â }
>>
>> - Â Â Â Â Â Â Â Â Â Â if(copy_from_user(&tcs_val, (int*)arg, sizeof(int)))
>> + Â Â Â Â Â Â Â Â Â Â if(copy_from_user(&tcs_val, (int*)arg, sizeof(int))) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return -EFAULT;
>> + Â Â Â Â Â Â Â Â Â Â }
>>
>> Â Â Â Â Â Â Â Â Â Â Â tcs_val = RTC_TCR_PATTERN | (tcs_val & 0x0F);
>> Â Â Â Â Â Â Â Â Â Â Â ds1302_writereg(RTC_TRICKLECHARGER, tcs_val);
>
> This function clearly needs the BKL, incidentally; there doesn't appear to
> be any other locking going on.
>
>> + Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â return 0;
>> Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â case RTC_VL_READ:
>> @@ -340,6 +362,7 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>> Â Â Â Â Â Â Â Â Â Â Â Â*/
>> Â Â Â Â Â Â Â Â Â Â Â printk(KERN_WARNING "DS1302: RTC Voltage Low detection"
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â" is not supported\n");
>> + Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â return 0;
>> Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â case RTC_VL_CLR:
>> @@ -347,9 +370,11 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>> Â Â Â Â Â Â Â Â Â Â Â /* TODO:
>> Â Â Â Â Â Â Â Â Â Â Â Â* Nothing to do since Voltage Low detection is not supported
>> Â Â Â Â Â Â Â Â Â Â Â Â*/
>> + Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â return 0;
>> Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â default:
>> + Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â return -ENOIOCTLCMD;
>> Â Â Â }
>> Â}
>> @@ -375,8 +400,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_ioctl,
>> Â};
>>
>> Â/* Probe for the chip by writing something to its RAM and try reading it back. */
>> diff --git a/arch/cris/arch-v10/drivers/gpio.c b/arch/cris/arch-v10/drivers/gpio.c
>> index 4b0f65f..2199c08 100644
>> --- a/arch/cris/arch-v10/drivers/gpio.c
>> +++ b/arch/cris/arch-v10/drivers/gpio.c
>> @@ -46,8 +46,8 @@ static char gpio_name[] = "etrax gpio";
>> Âstatic wait_queue_head_t *gpio_wq;
>> Â#endif
>>
>> -static int gpio_ioctl(struct inode *inode, struct file *file,
>> - Â Â unsigned int cmd, unsigned long arg);
>> +static long gpio_ioctl(struct file *file, unsigned int cmd,
>> + Â Â unsigned long arg);
>> Âstatic ssize_t gpio_write(struct file *file, const char __user *buf,
>> Â Â Â size_t count, loff_t *off);
>> Âstatic int gpio_open(struct inode *inode, struct file *filp);
>> @@ -504,17 +504,20 @@ unsigned long inline setget_output(struct gpio_private *priv, unsigned long arg)
>> Âstatic int
>> Âgpio_leds_ioctl(unsigned int cmd, unsigned long arg);
>>
>> -static int
>> -gpio_ioctl(struct inode *inode, struct file *file,
>> - Â Â Â Âunsigned int cmd, unsigned long arg)
>> +static long
>> +gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> Â{
>> + Â Â lock_kernel();
>> +
>> Â Â Â unsigned long flags;
>> Â Â Â unsigned long val;
>> Â Â Â Â Âint ret = 0;
>>
>> Â Â Â struct gpio_private *priv = file->private_data;
>> - Â Â if (_IOC_TYPE(cmd) != ETRAXGPIO_IOCTYPE)
>> + Â Â if (_IOC_TYPE(cmd) != ETRAXGPIO_IOCTYPE) {
>> + Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â return -EINVAL;
>> + Â Â }
>
> lock_kernel should happen here.
>
>> Â Â Â spin_lock_irqsave(&gpio_lock, flags);
>
> But notice how this function has its own locking? ÂThat, alone, doesn't
> tell you that the BKL is not needed, but it's a good sign.

I had asked about this before but received no response, but your
comment here explains my questions.

>
> HOWEVER (getting off the topic of this patch, now), further into this
> function I see:
>
> Â Â Â Âcase IO_CLRALARM:
> Â Â Â Â Â Â Â Â// clear alarm for bits with 1 in arg
> Â Â Â Â Â Â Â Âpriv->highalarm &= ~arg;
> Â Â Â Â Â Â Â Âpriv->lowalarm Â&= ~arg;
> Â Â Â Â Â Â Â Â{
> Â Â Â Â Â Â Â Â Â Â Â Â/* Must update gpio_some_alarms */
> Â Â Â Â Â Â Â Â Â Â Â Âstruct gpio_private *p = alarmlist;
> Â Â Â Â Â Â Â Â Â Â Â Âint some_alarms;
> Â Â Â Â Â Â Â Â Â Â Â Âspin_lock_irq(&gpio_lock);
> Â Â Â Â Â Â Â Â Â Â Â Âp = alarmlist;
> Â Â Â Â Â Â Â Â Â Â Â Âsome_alarms = 0;
>
> But it already took gpio_lock! ÂSomebody needs to tell me how this could
> possibly not deadlock. ÂMaybe this code has never been run on an SMP
> system?
>
> Stoyan, as a developer working on locking fixes, you would inspire more
> confidence in your work if you would notice things like this. ÂIt's
> important to look at what's going on.
>
>> @@ -680,6 +683,7 @@ gpio_ioctl(struct inode *inode, struct file *file,
>> Â Â Â } /* switch */
>>
>> Â Â Â spin_unlock_irqrestore(&gpio_lock, flags);
>> + Â Â unlock_kernel();
>> Â Â Â return ret;
>> Â}
>>
>> @@ -713,12 +717,12 @@ gpio_leds_ioctl(unsigned int cmd, unsigned long arg)
>> Â}
>>
>> Âstatic const struct file_operations gpio_fops = {
>> -   .owner    = THIS_MODULE,
>> -   .poll    Â= gpio_poll,
>> -   .ioctl    = gpio_ioctl,
>> -   .write    = gpio_write,
>> -   .open    Â= gpio_open,
>> -   .release   = gpio_release,
>> +   .owner     Â= THIS_MODULE,
>> +   .poll      = gpio_poll,
>> + Â Â .unlocked_ioctl = gpio_ioctl,
>> +   .write     Â= gpio_write,
>> +   .open      = gpio_open,
>> +   .release    Â= gpio_release,
>> Â};
>>
>> Âstatic void ioif_watcher(const unsigned int gpio_in_available,
>> diff --git a/arch/cris/arch-v10/drivers/pcf8563.c b/arch/cris/arch-v10/drivers/pcf8563.c
>> index 1e90c1a..9a2b46e 100644
>> --- a/arch/cris/arch-v10/drivers/pcf8563.c
>> +++ b/arch/cris/arch-v10/drivers/pcf8563.c
>> @@ -53,7 +53,7 @@ static DEFINE_MUTEX(rtc_lock); /* Protect state etc */
>> Âstatic const unsigned char days_in_month[] =
>> Â Â Â { 0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
>>
>> -int pcf8563_ioctl(struct inode *, struct file *, unsigned int, unsigned long);
>> +long pcf8563_ioctl(struct file *, unsigned int, unsigned long);
>>
>> Â/* Cache VL bit value read at driver init since writing the RTC_SECOND
>> Â * register clears the VL status.
>> @@ -62,7 +62,7 @@ static int voltage_low;
>>
>> Âstatic const struct file_operations pcf8563_fops = {
>> Â Â Â .owner = THIS_MODULE,
>> - Â Â .ioctl = pcf8563_ioctl,
>> + Â Â .unlocked_ioctl = pcf8563_ioctl,
>> Â};
>>
>> Âunsigned char
>> @@ -212,8 +212,7 @@ pcf8563_exit(void)
>> Â * ioctl calls for this driver. Why return -ENOTTY upon error? Because
>> Â * POSIX says so!
>> Â */
>> -int pcf8563_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
>> - Â Â unsigned long arg)
>> +long pcf8563_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> Â{
>> Â Â Â /* Some sanity checks. */
>> Â Â Â if (_IOC_TYPE(cmd) != RTC_MAGIC)
>> @@ -222,6 +221,8 @@ int pcf8563_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
>> Â Â Â if (_IOC_NR(cmd) > RTC_MAX_IOCTL)
>> Â Â Â Â Â Â Â return -ENOTTY;
>>
>> + Â Â lock_kernel();
>> +
>
> This is the right place for lock_kernel(). ÂBut...
>
>> Â Â Â switch (cmd) {
>> Â Â Â case RTC_RD_TIME:
>> Â Â Â {
>> @@ -234,11 +235,13 @@ int pcf8563_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
>> Â Â Â Â Â Â Â if (copy_to_user((struct rtc_time *) arg, &tm,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âsizeof tm)) {
>> Â Â Â Â Â Â Â Â Â Â Â mutex_unlock(&rtc_lock);
>> + Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>
> again, we have a driver which appears to be doing its own locking. ÂThe
> author was pretty careful to acquire rtc_lock before messing with things.
> But... Â(skipping a bit) I find:
>
>
> Â Â Â Â Â Â Â Âmutex_lock(&rtc_lock);
>
> Â Â Â Â Â Â Â Ârtc_write(RTC_YEAR, tm.tm_year);
> Â Â Â Â Â Â Â Ârtc_write(RTC_MONTH, tm.tm_mon);
> Â Â Â Â Â Â Â Ârtc_write(RTC_WEEKDAY, tm.tm_wday); /* Not coded in BCD. */
> Â Â Â Â Â Â Â Ârtc_write(RTC_DAY_OF_MONTH, tm.tm_mday);
> Â Â Â Â Â Â Â Ârtc_write(RTC_HOURS, tm.tm_hour);
> Â Â Â Â Â Â Â Ârtc_write(RTC_MINUTES, tm.tm_min);
> Â Â Â Â Â Â Â Ârtc_write(RTC_SECONDS, tm.tm_sec);
>
> Â Â Â Â Â Â Â Âmutex_unlock(&rtc_lock);
>
> Â Â Â Â Â Â Â Âreturn 0;
> Â Â Â Â}
>
> Â Â Â Â/* [trimmed by jc] */
>
> Â Â Â Âcase RTC_VL_CLR:
> Â Â Â Â{
> Â Â Â Â Â Â Â Â/* Clear the VL bit in the seconds register in case
> Â Â Â Â Â Â Â Â * the time has not been set already (which would
> Â Â Â Â Â Â Â Â * have cleared it). This does not really matter
> Â Â Â Â Â Â Â Â * because of the cached voltage_low value but do it
> Â Â Â Â Â Â Â Â * anyway for consistency. */
>
> Â Â Â Â Â Â Â Âint ret = rtc_read(RTC_SECONDS);
>
> Â Â Â Â Â Â Â Ârtc_write(RTC_SECONDS, (ret & 0x7F));
>
> Notice how the first rtc_write(RTC_SECONDS...) is protected by rtc_lock,
> but the second is not? ÂThis function appears to be buggy too. ÂIt would be
> good to notice things like that.

I have added the locks around this to the patch

>
> [...]
>
>>
>> diff --git a/arch/cris/arch-v10/drivers/sync_serial.c b/arch/cris/arch-v10/drivers/sync_serial.c
>> index 6cc1a03..f66e79b 100644
>> --- a/arch/cris/arch-v10/drivers/sync_serial.c
>> +++ b/arch/cris/arch-v10/drivers/sync_serial.c
>> @@ -158,8 +158,8 @@ static int sync_serial_open(struct inode *inode, struct file *file);
>> Âstatic int sync_serial_release(struct inode *inode, struct file *file);
>> Âstatic unsigned int sync_serial_poll(struct file *filp, poll_table *wait);
>>
>> -static int sync_serial_ioctl(struct inode *inode, struct file *file,
>> - Â Â unsigned int cmd, unsigned long arg);
>> +static long sync_serial_ioctl(struct file *file, unsigned int cmd,
>> + Â Â unsigned long arg);
>> Âstatic ssize_t sync_serial_write(struct file *file, const char *buf,
>> Â Â Â size_t count, loff_t *ppos);
>> Âstatic ssize_t sync_serial_read(struct file *file, char *buf,
>> @@ -249,7 +249,7 @@ static struct file_operations sync_serial_fops = {
>>    .write  = sync_serial_write,
>>    .read  Â= sync_serial_read,
>>    .poll  Â= sync_serial_poll,
>> -   .ioctl  = sync_serial_ioctl,
>> +   .unlocked_ioctl  = sync_serial_ioctl,
>>    .open  Â= sync_serial_open,
>> Â Â Â .release = sync_serial_release
>> Â};
>> @@ -679,17 +679,20 @@ static unsigned int sync_serial_poll(struct file *file, poll_table *wait)
>> Â Â Â return mask;
>> Â}
>>
>> -static int sync_serial_ioctl(struct inode *inode, struct file *file,
>> - Â Â Â Â Â Â Â unsigned int cmd, unsigned long arg)
>> +static long sync_serial_ioctl(struct file *file, unsigned int cmd,
>> + Â Â Â Â Â Â Â unsigned long arg)
>> Â{
>> Â Â Â int return_val = 0;
>> Â Â Â unsigned long flags;
>>
>> + Â Â lock_kernel();
>> +
>> Â Â Â int dev = MINOR(file->f_dentry->d_inode->i_rdev);
>> Â Â Â struct sync_port *port;
>
>> Â Â Â if (dev < 0 || dev >= NUMBER_OF_PORTS || !ports[dev].enabled) {
>> Â Â Â Â Â Â Â DEBUG(printk(KERN_DEBUG "Invalid minor %d\n", dev));
>> + Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â return -1;
>> Â Â Â }
>
> lock_kernel() should move down here.
>
>> Â Â Â port = &ports[dev];
>> @@ -757,8 +760,10 @@ static int sync_serial_ioctl(struct inode *inode, struct file *file,
>> Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â break;
>> Â Â Â case SSP_MODE:
>> - Â Â Â Â Â Â if (arg > 5)
>> + Â Â Â Â Â Â if (arg > 5) {
>> + Â Â Â Â Â Â Â Â Â Â unlock_kernel();
>> Â Â Â Â Â Â Â Â Â Â Â return -EINVAL;
>> + Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â if (arg == MASTER_OUTPUT || arg == SLAVE_OUTPUT)
>> Â Â Â Â Â Â Â Â Â Â Â *R_IRQ_MASK1_CLR = 1 << port->data_avail_bit;
>> Â Â Â Â Â Â Â else if (!port->use_dma)
>> @@ -954,6 +959,7 @@ static int sync_serial_ioctl(struct inode *inode, struct file *file,
>> Â Â Â Â Â Â Â start_dma_in(port);
>> Â Â Â }
>> Â Â Â local_irq_restore(flags);
>> + Â Â unlock_kernel();
>
> This function appears to be using irq-disabling as its locking. ÂHmm.
>
> You missed one:
>
> Â Â Â Âcase SSP_INBUFCHUNK:
> #if 0
> Â Â Â Â Â Â Â Âif (arg > port->in_buffer_size/NUM_IN_DESCR)
> Â Â Â Â Â Â Â Â Â Â Â Âreturn -EINVAL;
>
> Yes, it's in "#if 0", but somebody might uncomment it someday. ÂIf you're
> fixing the code, you need to fix *all* the code.

Added to the patch

>
>> Â Â Â return return_val;
>> Â}
>>
>
> jon
>

I have made the modifications and will be re-submitting the patch

-- 

-Stoyan
N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i