[PATCH] drivers/char/pcmcia/synclink_cs.c: BKL pushdown?

[PATCH] drivers/char/pcmcia/synclink_cs.c: BKL pushdown?

[lenrek]

I found the counterpart of function mgslpc_wait_until_sent
in drivers/char/synclinkmp.c (wait_until_sent) is modified to
issue (un)lock_kernel.  This patch does the same modification.

However, I'm afraid similar modifications are necessary further on  
functions
mgslpc_ioctl and mgslpc_write_room.



--- linux-2.6.31.1/drivers/char/pcmcia/synclink_cs.c	2009-10-02  
17:01:23.000000000 +0900
+++ linux/drivers/char/pcmcia/synclink_cs.c	2009-10-02  
17:23:59.000000000 +0900
@@ -2442,6 +2442,8 @@
  	if (!info )
  		return;

+	lock_kernel();
+
  	if (debug_level >= DEBUG_LEVEL_INFO)
  		printk("%s(%d):mgslpc_wait_until_sent(%s) entry\n",
  			 __FILE__,__LINE__, info->device_name );
@@ -2490,6 +2492,7 @@
  	}

  exit:
+	unlock_kernel();
  	if (debug_level >= DEBUG_LEVEL_INFO)
  		printk("%s(%d):mgslpc_wait_until_sent(%s) exit\n",
  			 __FILE__,__LINE__, info->device_name );

Re: [PATCH] drivers/char/pcmcia/synclink_cs.c: BKL pushdown?

[Alan Cox]

On Fri, 02 Oct 2009 19:37:21 +0900
lenrek@me.com wrote:

> I found the counterpart of function mgslpc_wait_until_sent
> in drivers/char/synclinkmp.c (wait_until_sent) is modified to
> issue (un)lock_kernel.  This patch does the same modification.
> 
> However, I'm afraid similar modifications are necessary further on  
> functions
> mgslpc_ioctl and mgslpc_write_room.

The push down work normally eliminated BKL calls that were demonstrably
not needed and left it in anywhere that needed thought. Do those
functions still really need the BKL ?

Re: [PATCH] drivers/char/pcmcia/synclink_cs.c: BKL pushdown?

[Paul Fulghum]

Alan Cox wrote:
> On Fri, 02 Oct 2009 19:37:21 +0900
> lenrek@me.com wrote:
> 
>> I found the counterpart of function mgslpc_wait_until_sent
>> in drivers/char/synclinkmp.c (wait_until_sent) is modified to
>> issue (un)lock_kernel.  This patch does the same modification.
>>
>> However, I'm afraid similar modifications are necessary further on  
>> functions
>> mgslpc_ioctl and mgslpc_write_room.
> 
> The push down work normally eliminated BKL calls that were demonstrably
> not needed and left it in anywhere that needed thought. Do those
> functions still really need the BKL ?

No, these functions use a device specific spinlock (info->lock)
when needed. Not even mgslpc_wait_until_sent needs BKL.

-- 
Paul Fulghum
MicroGate Systems, Ltd.
=Customer Driven, by Design=
(800)444-1982
(512)345-7791 (Direct)
(512)343-9046 (Fax)
Central Time Zone (GMT -5h)
www.microgate.com

Re: [PATCH] drivers/char/pcmcia/synclink_cs.c: BKL pushdown?

[lenrek]

On 2009/10/02, at 22:20, Paul Fulghum wrote:

> Alan Cox wrote:
>> On Fri, 02 Oct 2009 19:37:21 +0900
>> lenrek@me.com wrote:
>>
>>> I found the counterpart of function mgslpc_wait_until_sent
>>> in drivers/char/synclinkmp.c (wait_until_sent) is modified to
>>> issue (un)lock_kernel.  This patch does the same modification.
>>>
>>> However, I'm afraid similar modifications are necessary further on
>>> functions
>>> mgslpc_ioctl and mgslpc_write_room.
>>
>> The push down work normally eliminated BKL calls that were  
>> demonstrably
>> not needed and left it in anywhere that needed thought. Do those
>> functions still really need the BKL ?
>
> No, these functions use a device specific spinlock (info->lock)
> when needed. Not even mgslpc_wait_until_sent needs BKL.

How about the BKL calls in synclinkmp.c, synclink.c, and synclink_gt.c?
Can they be safely eliminated?

Re: [PATCH] drivers/char/pcmcia/synclink_cs.c: BKL pushdown?

[Paul Fulghum]

lenrek@me.com wrote:
> How about the BKL calls in synclinkmp.c, synclink.c, and synclink_gt.c?
> Can they be safely eliminated?

I looked those over and yes, the BKL can be removed
from those as well. All 4 synclink drivers are mostly
variations on the same code.

It looks like the lock_kernel calls were blindly pushed down
for safety during the initial transition, but all of the
functions covered either use device specific locking or
do not require locking.

-- 
Paul Fulghum
MicroGate Systems, Ltd.
=Customer Driven, by Design=
(800)444-1982
(512)345-7791 (Direct)
(512)343-9046 (Fax)
Central Time Zone (GMT -5h)
www.microgate.com

Re: [PATCH] drivers/char/pcmcia/synclink_cs.c: BKL pushdown?

[lenrek]

On 2009/10/03, at 0:33, Paul Fulghum wrote:

> lenrek@me.com wrote:
>> How about the BKL calls in synclinkmp.c, synclink.c, and  
>> synclink_gt.c?
>> Can they be safely eliminated?
>
> I looked those over and yes, the BKL can be removed
> from those as well. All 4 synclink drivers are mostly
> variations on the same code.
>
> It looks like the lock_kernel calls were blindly pushed down
> for safety during the initial transition, but all of the
> functions covered either use device specific locking or
> do not require locking.

Many thanks.  I enclose a patch against 2.6.31.1
which eliminates the BKL calls from the synclink drivers.


diff -ur linux-2.6.31.1/drivers/char/synclink.c linux/drivers/char/ 
synclink.c
--- linux-2.6.31.1/drivers/char/synclink.c	2009-09-25  
00:45:25.000000000 +0900
+++ linux/drivers/char/synclink.c	2009-10-03 00:35:31.000000000 +0900
@@ -2950,9 +2950,8 @@
  		    return -EIO;
  	}

-	lock_kernel();
  	ret = mgsl_ioctl_common(info, cmd, arg);
-	unlock_kernel();
+
  	return ret;
  }

@@ -3162,7 +3161,6 @@
  	 * Note: use tight timings here to satisfy the NIST-PCTS.
  	 */

-	lock_kernel();
  	if ( info->params.data_rate ) {
  	       	char_time = info->timeout/(32 * 5);
  		if (!char_time)
@@ -3192,7 +3190,6 @@
  				break;
  		}
  	}
-	unlock_kernel();

  exit:
  	if (debug_level >= DEBUG_LEVEL_INFO)
diff -ur linux-2.6.31.1/drivers/char/synclink_gt.c linux/drivers/char/ 
synclink_gt.c
--- linux-2.6.31.1/drivers/char/synclink_gt.c	2009-09-25  
00:45:25.000000000 +0900
+++ linux/drivers/char/synclink_gt.c	2009-10-03 00:35:57.000000000 +0900
@@ -928,8 +928,6 @@
  	 * Note: use tight timings here to satisfy the NIST-PCTS.
  	 */

-	lock_kernel();
-
  	if (info->params.data_rate) {
  	       	char_time = info->timeout/(32 * 5);
  		if (!char_time)
@@ -947,7 +945,6 @@
  		if (timeout && time_after(jiffies, orig_jiffies + timeout))
  			break;
  	}
-	unlock_kernel();

  exit:
  	DBGINFO(("%s wait_until_sent exit\n", info->device_name));
@@ -1073,7 +1070,6 @@
  		    return -EIO;
  	}

-	lock_kernel();

  	switch (cmd) {
  	case MGSL_IOCGPARAMS:
@@ -1143,7 +1139,7 @@
  	default:
  		ret = -ENOIOCTLCMD;
  	}
-	unlock_kernel();
+
  	return ret;
  }

diff -ur linux-2.6.31.1/drivers/char/synclinkmp.c linux/drivers/char/ 
synclinkmp.c
--- linux-2.6.31.1/drivers/char/synclinkmp.c	2009-09-25  
00:45:25.000000000 +0900
+++ linux/drivers/char/synclinkmp.c	2009-10-03 00:38:16.000000000 +0900
@@ -1062,7 +1062,6 @@
  	if (sanity_check(info, tty->name, "wait_until_sent"))
  		return;

-	lock_kernel();

  	if (!(info->port.flags & ASYNC_INITIALIZED))
  		goto exit;
@@ -1106,7 +1105,6 @@
  	}

  exit:
-	unlock_kernel();
  	if (debug_level >= DEBUG_LEVEL_INFO)
  		printk("%s(%d):%s wait_until_sent() exit\n",
  			 __FILE__,__LINE__, info->device_name );
@@ -1122,7 +1120,6 @@
  	if (sanity_check(info, tty->name, "write_room"))
  		return 0;

-	lock_kernel();
  	if (info->params.mode == MGSL_MODE_HDLC) {
  		ret = (info->tx_active) ? 0 : HDLC_MAX_FRAME_SIZE;
  	} else {
@@ -1130,7 +1127,6 @@
  		if (ret < 0)
  			ret = 0;
  	}
-	unlock_kernel();

  	if (debug_level >= DEBUG_LEVEL_INFO)
  		printk("%s(%d):%s write_room()=%d\n",
@@ -1251,7 +1247,7 @@
   *
   * Return Value:	0 if success, otherwise error code
   */
-static int do_ioctl(struct tty_struct *tty, struct file *file,
+static int ioctl(struct tty_struct *tty, struct file *file,
  		 unsigned int cmd, unsigned long arg)
  {
  	SLMP_INFO *info = tty->driver_data;
@@ -1341,16 +1337,6 @@
  	return 0;
  }

-static int ioctl(struct tty_struct *tty, struct file *file,
-		 unsigned int cmd, unsigned long arg)
-{
-	int ret;
-	lock_kernel();
-	ret = do_ioctl(tty, file, cmd, arg);
-	unlock_kernel();
-	return ret;
-}
-
  /*
   * /proc fs routines....
   */

Re: [PATCH] drivers/char/pcmcia/synclink_cs.c: BKL pushdown?

[Paul Fulghum]

lenrek@me.com wrote:
> On 2009/10/03, at 0:33, Paul Fulghum wrote:
> 
>> lenrek@me.com wrote:
>>> How about the BKL calls in synclinkmp.c, synclink.c, and  
>>> synclink_gt.c?
>>> Can they be safely eliminated?
>> I looked those over and yes, the BKL can be removed
>> from those as well. All 4 synclink drivers are mostly
>> variations on the same code.
>>
>> It looks like the lock_kernel calls were blindly pushed down
>> for safety during the initial transition, but all of the
>> functions covered either use device specific locking or
>> do not require locking.
> 
> Many thanks.  I enclose a patch against 2.6.31.1
> which eliminates the BKL calls from the synclink drivers.

Acked-by: Paul Fulghum <paulkf@microgate.com>

-- 
Paul Fulghum
MicroGate Systems, Ltd.
=Customer Driven, by Design=
(800)444-1982
(512)345-7791 (Direct)
(512)343-9046 (Fax)
Central Time Zone (GMT -5h)
www.microgate.com