[PATCH] Remove cli()/sti() calls from HYSDN driver.

[PATCH] Remove cli()/sti() calls from HYSDN driver.

[Mark Asselstine]

>From looking at this driver the use of cli()/sti() within the do/while
was a way to ensure interrupts were only disabled for short periods of
time while the bulk of the time interrupts were free to occur. The
use of the spin lock has eliminated the need to play with interrupts
in this way while still allowing for IO to be protected.

The remaining 3 sti() calls seem unneeded now that at no other point
in the driver is there a call to cli().

Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
---
 drivers/isdn/hysdn/boardergo.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/isdn/hysdn/boardergo.c b/drivers/isdn/hysdn/boardergo.c
index 6cdbad3..3eb096f 100644
--- a/drivers/isdn/hysdn/boardergo.c
+++ b/drivers/isdn/hysdn/boardergo.c
@@ -64,10 +64,11 @@ ergo_interrupt(int intno, void *dev_id)
 }				/* ergo_interrupt */
 
 /******************************************************************************/
-/* ergo_irq_bh is the function called by the immediate kernel task list after */
-/* being activated with queue_task and no interrupts active. This task is the */
-/* only one handling data transfer from or to the card after booting. The task */
-/* may be queued from everywhere (interrupts included).                       */
+/* ergo_irq_bh will be called as part of the kernel clearing its shared work  */
+/* queue sometime after a call to schedule_work has been made passing our     */
+/* work_struct. This task is the only one handling data transfer from or to   */
+/* the card after booting. The task may be queued from everywhere             */
+/* (interrupts included).                                                     */
 /******************************************************************************/
 static void
 ergo_irq_bh(struct work_struct *ugli_api)
@@ -90,7 +91,6 @@ ergo_irq_bh(struct work_struct *ugli_api)
 	card->hw_lock = 1;	/* we now lock the hardware */
 
 	do {
-		sti();		/* reenable other ints */
 		again = 0;	/* assume loop not to be repeated */
 
 		if (!dpr->ToHyFlag) {
@@ -110,7 +110,6 @@ ergo_irq_bh(struct work_struct *ugli_api)
 				again = 1;	/* restart loop */
 			}
 		}		/* a message has arrived for us */
-		cli();		/* no further ints */
 		if (again) {
 			dpr->ToHyInt = 1;
 			dpr->ToPcInt = 1;	/* interrupt to E1 for all cards */
@@ -242,7 +241,6 @@ ergo_writebootimg(struct HYSDN_CARD *card, unsigned char *buf,
 		byteout(card->iobase + PCI9050_USER_IO, PCI9050_E1_RUN);	/* start E1 processor */
 		/* the interrupts are still masked */
 
-		sti();
 		msleep_interruptible(20);		/* Timeout 20ms */
 
 		if (((tDpramBootSpooler *) card->dpram)->Len != DPRAM_SPOOLER_DATA_SIZE) {
@@ -276,7 +274,6 @@ ergo_writebootseq(struct HYSDN_CARD *card, unsigned char *buf, int len)
 	dst = sp->Data;		/* point to data in spool structure */
 	buflen = sp->Len;	/* maximum len of spooled data */
 	wr_mirror = sp->WrPtr;	/* only once read */
-	sti();
 
 	/* try until all bytes written or error */
 	i = 0x1000;		/* timeout value */
@@ -380,7 +377,6 @@ ergo_waitpofready(struct HYSDN_CARD *card)
 #endif /* CONFIG_HYSDN_CAPI */
 			return (0);	/* success */
 		}		/* data has arrived */
-		sti();
 		msleep_interruptible(50);		/* Timeout 50ms */
 	}			/* wait until timeout */
 
-- 
1.5.5.1

Re: [PATCH] Remove cli()/sti() calls from HYSDN driver.

[Karsten Keil]

On Thu, May 01, 2008 at 04:16:52PM -0400, Mark Asselstine wrote:
> >From looking at this driver the use of cli()/sti() within the do/while
> was a way to ensure interrupts were only disabled for short periods of
> time while the bulk of the time interrupts were free to occur. The
> use of the spin lock has eliminated the need to play with interrupts
> in this way while still allowing for IO to be protected.
> 
> The remaining 3 sti() calls seem unneeded now that at no other point
> in the driver is there a call to cli().
> 
> Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
  Acked-by: Karsten Keil <kkeil@suse.de>
> ---
>  drivers/isdn/hysdn/boardergo.c |   14 +++++---------
>  1 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/isdn/hysdn/boardergo.c b/drivers/isdn/hysdn/boardergo.c
> index 6cdbad3..3eb096f 100644
> --- a/drivers/isdn/hysdn/boardergo.c
> +++ b/drivers/isdn/hysdn/boardergo.c
> @@ -64,10 +64,11 @@ ergo_interrupt(int intno, void *dev_id)
>  }				/* ergo_interrupt */
>  
>  /******************************************************************************/
> -/* ergo_irq_bh is the function called by the immediate kernel task list after */
> -/* being activated with queue_task and no interrupts active. This task is the */
> -/* only one handling data transfer from or to the card after booting. The task */
> -/* may be queued from everywhere (interrupts included).                       */
> +/* ergo_irq_bh will be called as part of the kernel clearing its shared work  */
> +/* queue sometime after a call to schedule_work has been made passing our     */
> +/* work_struct. This task is the only one handling data transfer from or to   */
> +/* the card after booting. The task may be queued from everywhere             */
> +/* (interrupts included).                                                     */
>  /******************************************************************************/
>  static void
>  ergo_irq_bh(struct work_struct *ugli_api)
> @@ -90,7 +91,6 @@ ergo_irq_bh(struct work_struct *ugli_api)
>  	card->hw_lock = 1;	/* we now lock the hardware */
>  
>  	do {
> -		sti();		/* reenable other ints */
>  		again = 0;	/* assume loop not to be repeated */
>  
>  		if (!dpr->ToHyFlag) {
> @@ -110,7 +110,6 @@ ergo_irq_bh(struct work_struct *ugli_api)
>  				again = 1;	/* restart loop */
>  			}
>  		}		/* a message has arrived for us */
> -		cli();		/* no further ints */
>  		if (again) {
>  			dpr->ToHyInt = 1;
>  			dpr->ToPcInt = 1;	/* interrupt to E1 for all cards */
> @@ -242,7 +241,6 @@ ergo_writebootimg(struct HYSDN_CARD *card, unsigned char *buf,
>  		byteout(card->iobase + PCI9050_USER_IO, PCI9050_E1_RUN);	/* start E1 processor */
>  		/* the interrupts are still masked */
>  
> -		sti();
>  		msleep_interruptible(20);		/* Timeout 20ms */
>  
>  		if (((tDpramBootSpooler *) card->dpram)->Len != DPRAM_SPOOLER_DATA_SIZE) {
> @@ -276,7 +274,6 @@ ergo_writebootseq(struct HYSDN_CARD *card, unsigned char *buf, int len)
>  	dst = sp->Data;		/* point to data in spool structure */
>  	buflen = sp->Len;	/* maximum len of spooled data */
>  	wr_mirror = sp->WrPtr;	/* only once read */
> -	sti();
>  
>  	/* try until all bytes written or error */
>  	i = 0x1000;		/* timeout value */
> @@ -380,7 +377,6 @@ ergo_waitpofready(struct HYSDN_CARD *card)
>  #endif /* CONFIG_HYSDN_CAPI */
>  			return (0);	/* success */
>  		}		/* data has arrived */
> -		sti();
>  		msleep_interruptible(50);		/* Timeout 50ms */
>  	}			/* wait until timeout */
>  
> -- 
> 1.5.5.1

-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: [PATCH] Remove cli()/sti() calls from HYSDN driver.

[M. Asselstine]

On Friday 02 May 2008, Karsten Keil wrote:
> On Thu, May 01, 2008 at 04:16:52PM -0400, Mark Asselstine wrote:
> > >From looking at this driver the use of cli()/sti() within the do/while
> >
> > was a way to ensure interrupts were only disabled for short periods of
> > time while the bulk of the time interrupts were free to occur. The
> > use of the spin lock has eliminated the need to play with interrupts
> > in this way while still allowing for IO to be protected.
> >
> > The remaining 3 sti() calls seem unneeded now that at no other point
> > in the driver is there a call to cli().
> >
> > Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
>
>   Acked-by: Karsten Keil <kkeil@suse.de>
>
Andrew, 

Would it be possible to include this commit in your tree. It is the last of 
the cli()/sti() calls and I would love to get his work completed. The patch 
has been vetted in kernel-janitors and netdev but no one has picked it up 
yet. unfortunately.

Thanks,
Mark
> > ---
> >  drivers/isdn/hysdn/boardergo.c |   14 +++++---------
> >  1 files changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/isdn/hysdn/boardergo.c
> > b/drivers/isdn/hysdn/boardergo.c index 6cdbad3..3eb096f 100644
> > --- a/drivers/isdn/hysdn/boardergo.c
> > +++ b/drivers/isdn/hysdn/boardergo.c
> > @@ -64,10 +64,11 @@ ergo_interrupt(int intno, void *dev_id)
> >  }				/* ergo_interrupt */
> >
> > 
> > /************************************************************************
> >******/ -/* ergo_irq_bh is the function called by the immediate kernel
> > task list after */ -/* being activated with queue_task and no interrupts
> > active. This task is the */ -/* only one handling data transfer from or
> > to the card after booting. The task */ -/* may be queued from everywhere
> > (interrupts included).                       */ +/* ergo_irq_bh will be
> > called as part of the kernel clearing its shared work  */ +/* queue
> > sometime after a call to schedule_work has been made passing our     */
> > +/* work_struct. This task is the only one handling data transfer from or
> > to   */ +/* the card after booting. The task may be queued from
> > everywhere             */ +/* (interrupts included).                     
> >                                */
> > /************************************************************************
> >******/ static void
> >  ergo_irq_bh(struct work_struct *ugli_api)
> > @@ -90,7 +91,6 @@ ergo_irq_bh(struct work_struct *ugli_api)
> >  	card->hw_lock = 1;	/* we now lock the hardware */
> >
> >  	do {
> > -		sti();		/* reenable other ints */
> >  		again = 0;	/* assume loop not to be repeated */
> >
> >  		if (!dpr->ToHyFlag) {
> > @@ -110,7 +110,6 @@ ergo_irq_bh(struct work_struct *ugli_api)
> >  				again = 1;	/* restart loop */
> >  			}
> >  		}		/* a message has arrived for us */
> > -		cli();		/* no further ints */
> >  		if (again) {
> >  			dpr->ToHyInt = 1;
> >  			dpr->ToPcInt = 1;	/* interrupt to E1 for all cards */
> > @@ -242,7 +241,6 @@ ergo_writebootimg(struct HYSDN_CARD *card, unsigned
> > char *buf, byteout(card->iobase + PCI9050_USER_IO, PCI9050_E1_RUN);	/*
> > start E1 processor */ /* the interrupts are still masked */
> >
> > -		sti();
> >  		msleep_interruptible(20);		/* Timeout 20ms */
> >
> >  		if (((tDpramBootSpooler *) card->dpram)->Len !=
> > DPRAM_SPOOLER_DATA_SIZE) { @@ -276,7 +274,6 @@ ergo_writebootseq(struct
> > HYSDN_CARD *card, unsigned char *buf, int len) dst = sp->Data;		/* point
> > to data in spool structure */
> >  	buflen = sp->Len;	/* maximum len of spooled data */
> >  	wr_mirror = sp->WrPtr;	/* only once read */
> > -	sti();
> >
> >  	/* try until all bytes written or error */
> >  	i = 0x1000;		/* timeout value */
> > @@ -380,7 +377,6 @@ ergo_waitpofready(struct HYSDN_CARD *card)
> >  #endif /* CONFIG_HYSDN_CAPI */
> >  			return (0);	/* success */
> >  		}		/* data has arrived */
> > -		sti();
> >  		msleep_interruptible(50);		/* Timeout 50ms */
> >  	}			/* wait until timeout */
> >
> > --
> > 1.5.5.1

Re: [PATCH] Remove cli()/sti() calls from HYSDN driver.

[Andrew Morton]

On Wed, 7 May 2008 12:25:23 -0400 "M. Asselstine" <Mark.Asselstine@windriver.com> wrote:

> On Friday 02 May 2008, Karsten Keil wrote:
> > On Thu, May 01, 2008 at 04:16:52PM -0400, Mark Asselstine wrote:
> > > >From looking at this driver the use of cli()/sti() within the do/while
> > >
> > > was a way to ensure interrupts were only disabled for short periods of
> > > time while the bulk of the time interrupts were free to occur. The
> > > use of the spin lock has eliminated the need to play with interrupts
> > > in this way while still allowing for IO to be protected.
> > >
> > > The remaining 3 sti() calls seem unneeded now that at no other point
> > > in the driver is there a call to cli().
> > >
> > > Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
> >
> >   Acked-by: Karsten Keil <kkeil@suse.de>
> >
> Andrew, 
> 
> Would it be possible to include this commit in your tree. It is the last of 
> the cli()/sti() calls and I would love to get his work completed.

Sure, I'll take a look at it.

> The patch 
> has been vetted in kernel-janitors and netdev but no one has picked it up 
> yet. unfortunately.

I don't troll netdev for unloved patches.  A Cc:linux-kernel is often a
good idea...

Re: [PATCH] Remove cli()/sti() calls from HYSDN driver.

[Andrew Morton]

On Thu,  1 May 2008 16:16:52 -0400
Mark Asselstine <mark.asselstine@windriver.com> wrote:

> >From looking at this driver the use of cli()/sti() within the do/while
> was a way to ensure interrupts were only disabled for short periods of
> time while the bulk of the time interrupts were free to occur. The
> use of the spin lock has eliminated the need to play with interrupts
> in this way while still allowing for IO to be protected.

OK.  If we still want the interrupt-latency optimisation then we should
switch over to using local_irq_disable().  But probably nobody cares.

> The remaining 3 sti() calls seem unneeded now that at no other point
> in the driver is there a call to cli().
> 
> Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
> ---
>  drivers/isdn/hysdn/boardergo.c |   14 +++++---------

So... can we now do this?


--- a/drivers/isdn/hysdn/Kconfig~a
+++ a/drivers/isdn/hysdn/Kconfig
@@ -3,7 +3,7 @@
 #
 config HYSDN
 	tristate "Hypercope HYSDN cards (Champ, Ergo, Metro) support (module only)"
-	depends on m && PROC_FS && PCI && BROKEN_ON_SMP
+	depends on m && PROC_FS && PCI
 	help
 	  Say Y here if you have one of Hypercope's active PCI ISDN cards
 	  Champ, Ergo and Metro. You will then get a module called hysdn.
_

Re: [PATCH] Remove cli()/sti() calls from HYSDN driver.

[Karsten Keil]

On Wed, May 07, 2008 at 03:38:00PM -0700, Andrew Morton wrote:
> On Thu,  1 May 2008 16:16:52 -0400
> Mark Asselstine <mark.asselstine@windriver.com> wrote:
> 
> > >From looking at this driver the use of cli()/sti() within the do/while
> > was a way to ensure interrupts were only disabled for short periods of
> > time while the bulk of the time interrupts were free to occur. The
> > use of the spin lock has eliminated the need to play with interrupts
> > in this way while still allowing for IO to be protected.
> 
> OK.  If we still want the interrupt-latency optimisation then we should
> switch over to using local_irq_disable().  But probably nobody cares.
> 
> > The remaining 3 sti() calls seem unneeded now that at no other point
> > in the driver is there a call to cli().
> > 
> > Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
> > ---
> >  drivers/isdn/hysdn/boardergo.c |   14 +++++---------
> 
> So... can we now do this?
> 

I think so.

> 
> --- a/drivers/isdn/hysdn/Kconfig~a
> +++ a/drivers/isdn/hysdn/Kconfig
> @@ -3,7 +3,7 @@
>  #
>  config HYSDN
>  	tristate "Hypercope HYSDN cards (Champ, Ergo, Metro) support (module only)"
> -	depends on m && PROC_FS && PCI && BROKEN_ON_SMP
> +	depends on m && PROC_FS && PCI
>  	help
>  	  Say Y here if you have one of Hypercope's active PCI ISDN cards
>  	  Champ, Ergo and Metro. You will then get a module called hysdn.
> _

-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: [PATCH] Remove cli()/sti() calls from HYSDN driver.

[Mark Asselstine]

On Thursday 08 May 2008 04:33:53 Karsten Keil wrote:
> On Wed, May 07, 2008 at 03:38:00PM -0700, Andrew Morton wrote:
> > On Thu,  1 May 2008 16:16:52 -0400
> >
> > Mark Asselstine <mark.asselstine@windriver.com> wrote:
> > > >From looking at this driver the use of cli()/sti() within the do/while
> > >
> > > was a way to ensure interrupts were only disabled for short periods of
> > > time while the bulk of the time interrupts were free to occur. The
> > > use of the spin lock has eliminated the need to play with interrupts
> > > in this way while still allowing for IO to be protected.
> >
> > OK.  If we still want the interrupt-latency optimisation then we should
> > switch over to using local_irq_disable().  But probably nobody cares.
> >

I had thought about using local_irq_disable() but it never progressed past a 
thought.

> > > The remaining 3 sti() calls seem unneeded now that at no other point
> > > in the driver is there a call to cli().
> > >
> > > Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
> > > ---
> > >  drivers/isdn/hysdn/boardergo.c |   14 +++++---------
> >
> > So... can we now do this?
>
> I think so.
>

I am in agreement here too. From my poking around this driver I see no reason 
why not.

Regards,
Mark

> > --- a/drivers/isdn/hysdn/Kconfig~a
> > +++ a/drivers/isdn/hysdn/Kconfig
> > @@ -3,7 +3,7 @@
> >  #
> >  config HYSDN
> >  	tristate "Hypercope HYSDN cards (Champ, Ergo, Metro) support (module
> > only)" -	depends on m && PROC_FS && PCI && BROKEN_ON_SMP
> > +	depends on m && PROC_FS && PCI
> >  	help
> >  	  Say Y here if you have one of Hypercope's active PCI ISDN cards
> >  	  Champ, Ergo and Metro. You will then get a module called hysdn.
> > _