Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[John Kacur]

On Sat, 10 Oct 2009, Thomas Gleixner wrote:

> The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
> the BKL pushdown and it still uses the locked ioctl.
> 
> The BKL serialization in this driver is more than obscure and
> definitely does not cover all possible corner cases. Protect the
> access to the hardware with a local mutex and get rid of BKL and
> locked ioctl.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> Cc: linuxppc-dev@xxxxxxxxxx
> ---
>  drivers/macintosh/ans-lcd.c |   45 +++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> Index: linux-2.6-tip/drivers/macintosh/ans-lcd.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/macintosh/ans-lcd.c
> +++ linux-2.6-tip/drivers/macintosh/ans-lcd.c
> @@ -3,7 +3,6 @@
>   */
>  
>  #include <linux/types.h>
> -#include <linux/smp_lock.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
>  #include <linux/miscdevice.h>
> @@ -26,6 +25,7 @@
>  static unsigned long anslcd_short_delay = 80;
>  static unsigned long anslcd_long_delay = 3280;
>  static volatile unsigned char __iomem *anslcd_ptr;
> +static DEFINE_MUTEX(anslcd_mutex);
>  
>  #undef DEBUG
>  
> @@ -65,26 +65,31 @@ anslcd_write( struct file * file, const 
>  
>  	if (!access_ok(VERIFY_READ, buf, count))
>  		return -EFAULT;
> +
> +	mutex_lock(&anslcd_mutex);
>  	for ( i = *ppos; count > 0; ++i, ++p, --count ) 
>  	{
>  		char c;
>  		__get_user(c, p);
>  		anslcd_write_byte_data( c );
>  	}
> +	mutex_unlock(&anslcd_mutex);
>  	*ppos = i;
>  	return p - buf;
>  }
>  
> -static int
> -anslcd_ioctl( struct inode * inode, struct file * file,
> -				unsigned int cmd, unsigned long arg )
> +static long
> +anslcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	char ch, __user *temp;
> +	long ret = 0;
>  
>  #ifdef DEBUG
>  	printk(KERN_DEBUG "LCD: ioctl(%d,%d)\n",cmd,arg);
>  #endif
>  
> +	mutex_lock(&anslcd_mutex);
> +
>  	switch ( cmd )
>  	{
>  	case ANSLCD_CLEAR:
> @@ -93,7 +98,7 @@ anslcd_ioctl( struct inode * inode, stru
>  		anslcd_write_byte_ctrl ( 0x06 );
>  		anslcd_write_byte_ctrl ( 0x01 );
>  		anslcd_write_byte_ctrl ( 0x02 );
> -		return 0;
> +		break;
>  	case ANSLCD_SENDCTRL:
>  		temp = (char __user *) arg;
>  		__get_user(ch, temp);
> @@ -101,33 +106,37 @@ anslcd_ioctl( struct inode * inode, stru
>  			anslcd_write_byte_ctrl ( ch );
>  			__get_user(ch, temp);
>  		}
> -		return 0;
> +		break;
>  	case ANSLCD_SETSHORTDELAY:
>  		if (!capable(CAP_SYS_ADMIN))
> -			return -EACCES;
> -		anslcd_short_delay=arg;
> -		return 0;
> +			ret =-EACCES;
> +		else
> +			anslcd_short_delay=arg;
> +		break;
>  	case ANSLCD_SETLONGDELAY:
>  		if (!capable(CAP_SYS_ADMIN))
> -			return -EACCES;
> -		anslcd_long_delay=arg;
> -		return 0;
> +			ret = -EACCES;
> +		else
> +			anslcd_long_delay=arg;
> +		break;
>  	default:
> -		return -EINVAL;
> +		ret = -EINVAL;
>  	}
> +
> +	mutex_unlock(&anslcd_mutex);
> +	return ret;
>  }
>  
>  static int
>  anslcd_open( struct inode * inode, struct file * file )
>  {
> -	cycle_kernel_lock();
>  	return 0;
>  }
>  
>  const struct file_operations anslcd_fops = {
> -	.write	= anslcd_write,
> -	.ioctl	= anslcd_ioctl,
> -	.open	= anslcd_open,
> +	.write		= anslcd_write,
> +	.unlocked_ioctl	= anslcd_ioctl,
> +	.open		= anslcd_open,
>  };
>  
>  static struct miscdevice anslcd_dev = {
> @@ -168,6 +177,7 @@ anslcd_init(void)
>  	printk(KERN_DEBUG "LCD: init\n");
>  #endif
>  
> +	mutex_lock(&anslcd_mutex);
>  	anslcd_write_byte_ctrl ( 0x38 );
>  	anslcd_write_byte_ctrl ( 0x0c );
>  	anslcd_write_byte_ctrl ( 0x06 );
> @@ -176,6 +186,7 @@ anslcd_init(void)
>  	for(a=0;a<80;a++) {
>  		anslcd_write_byte_data(anslcd_logo[a]);
>  	}
> +	mutex_unlock(&anslcd_mutex);
>  	return 0;
>  }
>  
> 
> 
> 

There were 4 checkpatch errors on this patch, all of the type
ERROR: spaces required around that '=' (ctx:WxO)
#1466: FILE: drivers/macintosh/ans-lcd.c:112:
+			ret =-EACCES;

Cheers

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/