diff mbox series

[v2,1/6] zorro_esp: Limit DMA transfers to 65535 bytes

Message ID 022a8c581d228f3f01dfa783aadd183a53169daa.1539497520.git.fthain@telegraphics.com.au (mailing list archive)
State Changes Requested
Headers show
Series mac_esp, zorro_esp, esp_scsi: Various improvements | expand

Commit Message

Finn Thain Oct. 14, 2018, 6:12 a.m. UTC 
The core driver, esp_scsi, does not use the ESP_CONFIG2_FENAB bit, so
the chip's Transfer Counter register is only 16 bits wide (not 24).
A larger transfer cannot work and will theoretically result in a failed
command and a "DMA length is zero" error.

Fixes: 3109e5ae0311
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Cc: Michael Schmitz <schmitzmic@gmail.com>
Tested-by: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/scsi/zorro_esp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

Comments

Michael Schmitz Oct. 14, 2018, 7:35 a.m. UTC | #1 
Hi Finn,

thanks for spotting this!

Am 14.10.2018 um 19:12 schrieb Finn Thain:
> The core driver, esp_scsi, does not use the ESP_CONFIG2_FENAB bit, so
> the chip's Transfer Counter register is only 16 bits wide (not 24).
> A larger transfer cannot work and will theoretically result in a failed
> command and a "DMA length is zero" error.
>
> Fixes: 3109e5ae0311
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Cc: Michael Schmitz <schmitzmic@gmail.com>
> Tested-by: Michael Schmitz <schmitzmic@gmail.com>

Reviewed-by: Michael Schmitz <schmitzmic@gmail.com>

> ---
>  drivers/scsi/zorro_esp.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> index bb70882e6b56..be79127db594 100644
> --- a/drivers/scsi/zorro_esp.c
> +++ b/drivers/scsi/zorro_esp.c
> @@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
>  static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
>  					u32 dma_len)
>  {
> -	return dma_len > 0xFFFFFF ? 0xFFFFFF : dma_len;
> +	return dma_len > 0xFFFF ? 0xFFFF : dma_len;
>  }
>
>  static void zorro_esp_reset_dma(struct esp *esp)
> @@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
>  	scsi_esp_cmd(esp, ESP_CMD_DMA);
>  	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
>  	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> -	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
>
>  	scsi_esp_cmd(esp, cmd);
>  }
> @@ -529,7 +528,6 @@ static void zorro_esp_send_blz1230II_dma_cmd(struct esp *esp, u32 addr,
>  	scsi_esp_cmd(esp, ESP_CMD_DMA);
>  	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
>  	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> -	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
>
>  	scsi_esp_cmd(esp, cmd);
>  }
> @@ -574,7 +572,6 @@ static void zorro_esp_send_blz2060_dma_cmd(struct esp *esp, u32 addr,
>  	scsi_esp_cmd(esp, ESP_CMD_DMA);
>  	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
>  	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> -	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
>
>  	scsi_esp_cmd(esp, cmd);
>  }
> @@ -599,7 +596,6 @@ static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, u32 addr,
>
>  	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
>  	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> -	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
>
>  	if (write) {
>  		/* DMA receive */
> @@ -649,7 +645,6 @@ static void zorro_esp_send_cyberII_dma_cmd(struct esp *esp, u32 addr,
>
>  	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
>  	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> -	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
>
>  	if (write) {
>  		/* DMA receive */
> @@ -691,7 +686,6 @@ static void zorro_esp_send_fastlane_dma_cmd(struct esp *esp, u32 addr,
>
>  	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
>  	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> -	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
>
>  	if (write) {
>  		/* DMA receive */
>
Geert Uytterhoeven Oct. 14, 2018, 10:55 a.m. UTC | #2 
Hi Finn,

On Sun, Oct 14, 2018 at 8:36 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> The core driver, esp_scsi, does not use the ESP_CONFIG2_FENAB bit, so
> the chip's Transfer Counter register is only 16 bits wide (not 24).
> A larger transfer cannot work and will theoretically result in a failed
> command and a "DMA length is zero" error.

Thanks for your patch!

> Fixes: 3109e5ae0311

Fixes: 3109e5ae0311 ("scsi: zorro_esp: New driver for Amiga Zorro
NCR53C9x boards")

$ git help fixes
'fixes' is aliased to 'show --format='Fixes: %h ("%s")' -s'

BTW, if you use vim, add

    noremap ;gs "zyiw:exe "new \| setlocal buftype=nofile
bufhidden=hide noswapfile syntax=git \| 0r ! git show ".@z."" \|
:0<CR><CR>

to .vimrc, and type ";gs" when the cursor is positioned on a commit ID.

> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Cc: Michael Schmitz <schmitzmic@gmail.com>
> Tested-by: Michael Schmitz <schmitzmic@gmail.com>

> --- a/drivers/scsi/zorro_esp.c
> +++ b/drivers/scsi/zorro_esp.c
> @@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
>  static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
>                                         u32 dma_len)
>  {
> -       return dma_len > 0xFFFFFF ? 0xFFFFFF : dma_len;
> +       return dma_len > 0xFFFF ? 0xFFFF : dma_len;
>  }
>
>  static void zorro_esp_reset_dma(struct esp *esp)
> @@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
>         scsi_esp_cmd(esp, ESP_CMD_DMA);
>         zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
>         zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> -       zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);

Since all these sub-drivers seem to support 24-bit transfers, perhaps this is
something to be fixed in the esp_scsi core?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Finn Thain Oct. 14, 2018, 10 p.m. UTC | #3 
On Sun, 14 Oct 2018, Geert Uytterhoeven wrote:

> 
> > Fixes: 3109e5ae0311
> 
> Fixes: 3109e5ae0311 ("scsi: zorro_esp: New driver for Amiga Zorro
> NCR53C9x boards")
> 

OK.

> $ git help fixes
> 'fixes' is aliased to 'show --format='Fixes: %h ("%s")' -s'
> 
> BTW, if you use vim, add
> 
>     noremap ;gs "zyiw:exe "new \| setlocal buftype=nofile
> bufhidden=hide noswapfile syntax=git \| 0r ! git show ".@z."" \|
> :0<CR><CR>
> 
> to .vimrc, and type ";gs" when the cursor is positioned on a commit ID.
> 

Thanks.

> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > Cc: Michael Schmitz <schmitzmic@gmail.com>
> > Tested-by: Michael Schmitz <schmitzmic@gmail.com>
> 
> > --- a/drivers/scsi/zorro_esp.c
> > +++ b/drivers/scsi/zorro_esp.c
> > @@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
> >  static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
> >                                         u32 dma_len)
> >  {
> > -       return dma_len > 0xFFFFFF ? 0xFFFFFF : dma_len;
> > +       return dma_len > 0xFFFF ? 0xFFFF : dma_len;
> >  }
> >
> >  static void zorro_esp_reset_dma(struct esp *esp)
> > @@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
> >         scsi_esp_cmd(esp, ESP_CMD_DMA);
> >         zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
> >         zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> > -       zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
> 
> Since all these sub-drivers seem to support 24-bit transfers, perhaps this is
> something to be fixed in the esp_scsi core?
> 

I don't think there is anything to fix in the esp_scsi core. The fact that 
no-one saw the error indicates that large transfers don't occur in 
practice.

If you said there is an opportunity for an enhancement, I could agree, 
assuming that you also found a way to produce large IO requests. But I 
doubt that such an enhancement would make a measurable improvement.

Even if the benefit was measurable, the fix under review would still be 
needed for stable kernels.

BTW, Michael and I already discussed all this (it probably should have 
happened on the linux-m68k list).

Please see,

$ grep -lr ESP_CONFIG2_FENAB drivers/scsi/
drivers/scsi/am53c974.c
drivers/scsi/esp_scsi.c
drivers/scsi/esp_scsi.h

The authors of am53c974.c obviously decided it wasn't wise to make this 
feature mandatory (which suggests that perhaps it shouldn't go into common 
code).

The comments in esp_scsi.c say that ESP_CONFIG2_FENAB has undesirable 
consequences. There is information in the datasheets on this point but I 
didn't follow it up because I don't see a performance issue.

--
diff mbox series

Patch

diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index bb70882e6b56..be79127db594 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -245,7 +245,7 @@  static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
 					u32 dma_len)
 {
-	return dma_len > 0xFFFFFF ? 0xFFFFFF : dma_len;
+	return dma_len > 0xFFFF ? 0xFFFF : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)
@@ -484,7 +484,6 @@  static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
 	scsi_esp_cmd(esp, ESP_CMD_DMA);
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	scsi_esp_cmd(esp, cmd);
 }
@@ -529,7 +528,6 @@  static void zorro_esp_send_blz1230II_dma_cmd(struct esp *esp, u32 addr,
 	scsi_esp_cmd(esp, ESP_CMD_DMA);
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	scsi_esp_cmd(esp, cmd);
 }
@@ -574,7 +572,6 @@  static void zorro_esp_send_blz2060_dma_cmd(struct esp *esp, u32 addr,
 	scsi_esp_cmd(esp, ESP_CMD_DMA);
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	scsi_esp_cmd(esp, cmd);
 }
@@ -599,7 +596,6 @@  static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, u32 addr,
 
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	if (write) {
 		/* DMA receive */
@@ -649,7 +645,6 @@  static void zorro_esp_send_cyberII_dma_cmd(struct esp *esp, u32 addr,
 
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	if (write) {
 		/* DMA receive */
@@ -691,7 +686,6 @@  static void zorro_esp_send_fastlane_dma_cmd(struct esp *esp, u32 addr,
 
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	if (write) {
 		/* DMA receive */