[PATCH] drivers/scsi/mca_53c9x.c : save_flags()/cli() removal

[PATCH] drivers/scsi/mca_53c9x.c : save_flags()/cli() removal

[Amol Lad]

Replaced save_flags()/cli() with spin_lock alternatives

Signed-off-by: Amol Lad <amol@verismonetworks.com>
---
Andrew, 
Please add this to -mm
---
--- linux-2.6.19-rc4-orig/drivers/scsi/mca_53c9x.c	2006-08-24 02:46:33.000000000 +0530
+++ linux-2.6.19-rc4/drivers/scsi/mca_53c9x.c	2006-11-06 18:03:22.000000000 +0530
@@ -341,9 +341,7 @@ static void dma_init_read(struct NCR_ESP
 {
 	unsigned long flags;
 
-
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(esp->ehost->host_lock, flags);
 
 	mca_disable_dma(esp->dma);
 	mca_set_dma_mode(esp->dma, MCA_DMA_MODE_XFER | MCA_DMA_MODE_16 |
@@ -352,16 +350,14 @@ static void dma_init_read(struct NCR_ESP
 	mca_set_dma_count(esp->dma, length / 2); /* !!! */
 	mca_enable_dma(esp->dma);
 
-	restore_flags(flags);
+	spin_unlock_irqrestore(esp->ehost->host_lock, flags);
 }
 
 static void dma_init_write(struct NCR_ESP *esp, __u32 addr, int length)
 {
 	unsigned long flags;
 
-
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(esp->ehost->host_lock, flags);
 
 	mca_disable_dma(esp->dma);
 	mca_set_dma_mode(esp->dma, MCA_DMA_MODE_XFER | MCA_DMA_MODE_WRITE |
@@ -370,7 +366,7 @@ static void dma_init_write(struct NCR_ES
 	mca_set_dma_count(esp->dma, length / 2); /* !!! */
 	mca_enable_dma(esp->dma);
 
-	restore_flags(flags);
+	spin_unlock_irqrestore(esp->ehost->host_lock, flags);
 }
 
 static void dma_ints_off(struct NCR_ESP *esp)

Re: [PATCH] drivers/scsi/mca_53c9x.c : save_flags()/cli() removal

[Andrew Morton]

On Mon, 06 Nov 2006 18:12:11 +0530
Amol Lad <amol@verismonetworks.com> wrote:

> Replaced save_flags()/cli() with spin_lock alternatives
> 
> Signed-off-by: Amol Lad <amol@verismonetworks.com>
> ---
> Andrew, 
> Please add this to -mm
> ---
> --- linux-2.6.19-rc4-orig/drivers/scsi/mca_53c9x.c	2006-08-24 02:46:33.000000000 +0530
> +++ linux-2.6.19-rc4/drivers/scsi/mca_53c9x.c	2006-11-06 18:03:22.000000000 +0530
> @@ -341,9 +341,7 @@ static void dma_init_read(struct NCR_ESP
>  {
>  	unsigned long flags;
>  
> -
> -	save_flags(flags);
> -	cli();
> +	spin_lock_irqsave(esp->ehost->host_lock, flags);
>  
>  	mca_disable_dma(esp->dma);
>  	mca_set_dma_mode(esp->dma, MCA_DMA_MODE_XFER | MCA_DMA_MODE_16 |
> @@ -352,16 +350,14 @@ static void dma_init_read(struct NCR_ESP
>  	mca_set_dma_count(esp->dma, length / 2); /* !!! */
>  	mca_enable_dma(esp->dma);
>  
> -	restore_flags(flags);
> +	spin_unlock_irqrestore(esp->ehost->host_lock, flags);
>  }
>  
>  static void dma_init_write(struct NCR_ESP *esp, __u32 addr, int length)
>  {
>  	unsigned long flags;
>  
> -
> -	save_flags(flags);
> -	cli();
> +	spin_lock_irqsave(esp->ehost->host_lock, flags);
>  
>  	mca_disable_dma(esp->dma);
>  	mca_set_dma_mode(esp->dma, MCA_DMA_MODE_XFER | MCA_DMA_MODE_WRITE |
> @@ -370,7 +366,7 @@ static void dma_init_write(struct NCR_ES
>  	mca_set_dma_count(esp->dma, length / 2); /* !!! */
>  	mca_enable_dma(esp->dma);
>  
> -	restore_flags(flags);
> +	spin_unlock_irqrestore(esp->ehost->host_lock, flags);
>  }
>  
>  static void dma_ints_off(struct NCR_ESP *esp)

hm.  How do we find out if this works?

If it _does_ work then we can now remove the Kconfig BROKEN_ON_SMP dependency
for this driver.

Re: [PATCH] drivers/scsi/mca_53c9x.c : save_flags()/cli() removal

[James Bottomley]

On Tue, 2006-11-07 at 12:53 -0800, Andrew Morton wrote:

> hm.  How do we find out if this works?

I'm not sure anyone has this type of HW anymore.  It was the original
SCSI chip for the old MCA based IBM PC.  I can dig through my hardware
bins to see if I have a card, but I don't think so.

> If it _does_ work then we can now remove the Kconfig BROKEN_ON_SMP dependency
> for this driver.

Theoretically, yes ... practically I don't think there was an SMP box
produced with this chip.

James

Re: [PATCH] drivers/scsi/mca_53c9x.c : save_flags()/cli() removal

[Amol Lad]

On Wed, 2006-11-08 at 09:19 +0900, James Bottomley wrote:
> On Tue, 2006-11-07 at 12:53 -0800, Andrew Morton wrote:
> 
> > hm.  How do we find out if this works?
> 
> I'm not sure anyone has this type of HW anymore.  It was the original
> SCSI chip for the old MCA based IBM PC.  I can dig through my hardware
> bins to see if I have a card, but I don't think so.

The only way I see is the code review. 

Most of the drivers using save_flags()/cli() interfaces are not in use
anymore (such as drivers/char/riscom8.c). There are two possibilities:
1. Remove such drivers from kernel
2. Fix them

This would mean we can remove cli()/sti()/save_flags() interfaces from
the kernel so that any new development does not uses them accidently

> 
> > If it _does_ work then we can now remove the Kconfig BROKEN_ON_SMP dependency
> > for this driver.
> 
> Theoretically, yes ... practically I don't think there was an SMP box
> produced with this chip.
> 
> James
> 
> 
> 

Re: [PATCH] drivers/scsi/mca_53c9x.c : save_flags()/cli() removal

[Alan Cox]

Ar Mer, 2006-11-08 am 09:19 +0900, ysgrifennodd James Bottomley:
> On Tue, 2006-11-07 at 12:53 -0800, Andrew Morton wrote:
> 
> > hm.  How do we find out if this works?
> 
> I'm not sure anyone has this type of HW anymore.  It was the original
> SCSI chip for the old MCA based IBM PC.  I can dig through my hardware
> bins to see if I have a card, but I don't think so.

There was one in the pile of MCA cards I posted you I think.

Re: [PATCH] drivers/scsi/mca_53c9x.c : save_flags()/cli() removal

[Christoph Hellwig]

On Mon, Nov 06, 2006 at 06:12:11PM +0530, Amol Lad wrote:
> Replaced save_flags()/cli() with spin_lock alternatives

This patch has very little chance to work as-is because it only replaces
cli with spinlocks, but not the irq handler it's locking against.

Re: [PATCH] drivers/scsi/mca_53c9x.c : save_flags()/cli() removal

[Amol Lad]

On Tue, 2006-11-07 at 22:29 +0000, Christoph Hellwig wrote:
> On Mon, Nov 06, 2006 at 06:12:11PM +0530, Amol Lad wrote:
> > Replaced save_flags()/cli() with spin_lock alternatives
> 
> This patch has very little chance to work as-is because it only replaces
> cli with spinlocks, but not the irq handler it's locking against.
> 
It protects against irq handler also. 

drivers/scsi/NCR53C9x.c:

irqreturn_t esp_intr(int irq, void *dev_id)
{
    struct NCR_ESP *esp;
    unsigned long flags;
    int again;
    struct Scsi_Host *dev = dev_id;

    /* Handle all ESP interrupts showing at this IRQ level. */
    spin_lock_irqsave(dev->host_lock, flags);
...
...
}

[PATCH] drivers/scsi/mca_53c9x.c : save_flags()/cli() removal

[Amol Lad]

Replaced calls with save_flags()/cli() pair with
spi_lock_irqsave()/spin_unlock_irqrestore()

Tested:
1. compilation only 
2. Code review to verify that the change does not result in a recursive
locking

Signed-off-by: Amol Lad <amol@verismonetworks.com>
---
Andrew, 
Please add this to -mm
---
--- linux-2.6.19-rc4-orig/drivers/scsi/mca_53c9x.c	2006-08-24 02:46:33.000000000 +0530
+++ linux-2.6.19-rc4/drivers/scsi/mca_53c9x.c	2006-11-06 18:03:22.000000000 +0530
@@ -341,9 +341,7 @@ static void dma_init_read(struct NCR_ESP
 {
 	unsigned long flags;
 
-
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(esp->ehost->host_lock, flags);
 
 	mca_disable_dma(esp->dma);
 	mca_set_dma_mode(esp->dma, MCA_DMA_MODE_XFER | MCA_DMA_MODE_16 |
@@ -352,16 +350,14 @@ static void dma_init_read(struct NCR_ESP
 	mca_set_dma_count(esp->dma, length / 2); /* !!! */
 	mca_enable_dma(esp->dma);
 
-	restore_flags(flags);
+	spin_unlock_irqrestore(esp->ehost->host_lock, flags);
 }
 
 static void dma_init_write(struct NCR_ESP *esp, __u32 addr, int length)
 {
 	unsigned long flags;
 
-
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(esp->ehost->host_lock, flags);
 
 	mca_disable_dma(esp->dma);
 	mca_set_dma_mode(esp->dma, MCA_DMA_MODE_XFER | MCA_DMA_MODE_WRITE |
@@ -370,7 +366,7 @@ static void dma_init_write(struct NCR_ES
 	mca_set_dma_count(esp->dma, length / 2); /* !!! */
 	mca_enable_dma(esp->dma);
 
-	restore_flags(flags);
+	spin_unlock_irqrestore(esp->ehost->host_lock, flags);
 }
 
 static void dma_ints_off(struct NCR_ESP *esp)

[PATCH] drivers/scsi/mca_53c9x.c: save_flags()/cli() removal

[Amol Lad]

Replaced calls with save_flags()/cli() pair with
spi_lock_irqsave()/spin_unlock_irqrestore()

Tested:
1. compilation only 
2. Code review to verify that the change does not result in a recursive
locking

Signed-off-by: Amol Lad <amol@verismonetworks.com>
---
James,

Though I have done the review to check for recursive locking, can you
also have a look ?
---
--- linux-2.6.19-rc1-orig/drivers/scsi/mca_53c9x.c	2006-08-24 02:46:33.000000000 +0530
+++ linux-2.6.19-rc1/drivers/scsi/mca_53c9x.c	2006-10-16 11:47:45.000000000 +0530
@@ -342,9 +342,8 @@ static void dma_init_read(struct NCR_ESP
 	unsigned long flags;
 
 
-	save_flags(flags);
-	cli();
-
+	spin_lock_irqsave(esp->ehost->host_lock, flags);
+	
 	mca_disable_dma(esp->dma);
 	mca_set_dma_mode(esp->dma, MCA_DMA_MODE_XFER | MCA_DMA_MODE_16 |
 	  MCA_DMA_MODE_IO);
@@ -352,7 +351,7 @@ static void dma_init_read(struct NCR_ESP
 	mca_set_dma_count(esp->dma, length / 2); /* !!! */
 	mca_enable_dma(esp->dma);
 
-	restore_flags(flags);
+	spin_unlock_irqrestore(esp->ehost->host_lock, flags);
 }
 
 static void dma_init_write(struct NCR_ESP *esp, __u32 addr, int length)
@@ -360,8 +359,7 @@ static void dma_init_write(struct NCR_ES
 	unsigned long flags;
 
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(esp->ehost->host_lock, flags);
 
 	mca_disable_dma(esp->dma);
 	mca_set_dma_mode(esp->dma, MCA_DMA_MODE_XFER | MCA_DMA_MODE_WRITE |
@@ -370,7 +368,7 @@ static void dma_init_write(struct NCR_ES
 	mca_set_dma_count(esp->dma, length / 2); /* !!! */
 	mca_enable_dma(esp->dma);
 
-	restore_flags(flags);
+	spin_unlock_irqrestore(esp->ehost->host_lock, flags);
 }
 
 static void dma_ints_off(struct NCR_ESP *esp)