[PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Finn Thain]

The zorro_esp driver uses both PIO and DMA transfers. If a failed DMA
transfer happened to be followed by a PIO transfer, the TCLOW and TCMED
registers would not get cleared. It is theoretically possible that the
stale value from the transfer counter or the TCLOW/TCMED registers
could then be used by the controller and the driver. Avoid that by
clearing these registers before each PIO transfer.

Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: Kars de Jong <jongk@linux-m68k.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/esp_scsi.c | 3 +++
 drivers/scsi/mac_esp.c  | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bb88995a12c7..afbef83f5dd9 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2835,6 +2835,9 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
 	cmd &= ~ESP_CMD_DMA;
 	esp->send_cmd_error = 0;
 
+	esp_write8(0, ESP_TCLOW);
+	esp_write8(0, ESP_TCMED);
+
 	if (write) {
 		u8 *dst = (u8 *)addr;
 		u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : ESP_INTR_BSERV);
diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index 1c78bc10c790..797579247e47 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -361,8 +361,6 @@ static int esp_mac_probe(struct platform_device *dev)
 	esp->flags = ESP_FLAG_NO_DMA_MAP;
 	if (mep->pdma_io == NULL) {
 		printk(KERN_INFO PFX "using PIO for controller %d\n", dev->id);
-		esp_write8(0, ESP_TCLOW);
-		esp_write8(0, ESP_TCMED);
 		esp->flags |= ESP_FLAG_DISABLE_SYNC;
 		mac_esp_ops.send_dma_cmd = esp_send_pio_cmd;
 	} else {
-- 
2.23.0

Re: [PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Michael Schmitz]

Hi Finn,

Am 16.11.2019 um 16:36 schrieb Finn Thain:
> The zorro_esp driver uses both PIO and DMA transfers. If a failed DMA
> transfer happened to be followed by a PIO transfer, the TCLOW and TCMED
> registers would not get cleared. It is theoretically possible that the
> stale value from the transfer counter or the TCLOW/TCMED registers
> could then be used by the controller and the driver. Avoid that by
> clearing these registers before each PIO transfer.

I believe you also need to send a DMA NOP command to the ESP:

"Values
written to these two registers will be stored inter-
nally and loaded into the transfer counter by any
DMA command. These values remain unchanged
while the transfer counter decrements. Thus,
successive blocks of equal size may be transferred
without reprogramming the count. They may be
reprogrammed any time after the previous DMA
operation has started, whether it has finished or
not."

"Any DMA command will load the transfer count into
the counter. A DMA NOP (80h) will load the
counter while the non-DMA NOP (OOh) will not."

(53CF64_96-2 data book)

Without the DMA NOP, the transfer counts will remain as they were left 
by the prematurely terminated DMA command.

Cheers,

	Michael

>
> Cc: Michael Schmitz <schmitzmic@gmail.com>
> Cc: Kars de Jong <jongk@linux-m68k.org>
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> ---
>  drivers/scsi/esp_scsi.c | 3 +++
>  drivers/scsi/mac_esp.c  | 2 --
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index bb88995a12c7..afbef83f5dd9 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -2835,6 +2835,9 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
>  	cmd &= ~ESP_CMD_DMA;
>  	esp->send_cmd_error = 0;
>
> +	esp_write8(0, ESP_TCLOW);
> +	esp_write8(0, ESP_TCMED);
> +
>  	if (write) {
>  		u8 *dst = (u8 *)addr;
>  		u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : ESP_INTR_BSERV);
> diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
> index 1c78bc10c790..797579247e47 100644
> --- a/drivers/scsi/mac_esp.c
> +++ b/drivers/scsi/mac_esp.c
> @@ -361,8 +361,6 @@ static int esp_mac_probe(struct platform_device *dev)
>  	esp->flags = ESP_FLAG_NO_DMA_MAP;
>  	if (mep->pdma_io == NULL) {
>  		printk(KERN_INFO PFX "using PIO for controller %d\n", dev->id);
> -		esp_write8(0, ESP_TCLOW);
> -		esp_write8(0, ESP_TCMED);
>  		esp->flags |= ESP_FLAG_DISABLE_SYNC;
>  		mac_esp_ops.send_dma_cmd = esp_send_pio_cmd;
>  	} else {
>

Re: [PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Finn Thain]

On Sat, 16 Nov 2019, Michael Schmitz wrote:

> 
> I believe you also need to send a DMA NOP command to the ESP
> 

I believe you're right. I shall add the missing DMA NOP command. Thanks 
for your review!

-- 

Re: [PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Kars de Jong]

Hi Finn,

Op za 16 nov. 2019 om 04:36 schreef Finn Thain <fthain@telegraphics.com.au>:
>
> The zorro_esp driver uses both PIO and DMA transfers. If a failed DMA
> transfer happened to be followed by a PIO transfer, the TCLOW and TCMED
> registers would not get cleared. It is theoretically possible that the
> stale value from the transfer counter or the TCLOW/TCMED registers
> could then be used by the controller and the driver. Avoid that by
> clearing these registers before each PIO transfer.

Are you sure this is really needed?

The only place where the driver reads these registers is after a data
transfer. These are done using DMA on all Zorro boards, so I don’t
think there’s a risk of stale values from a PIO transfer there.

The only place the controller reads these registers is when a DMA
command is issued. The only place where that is done is in the
zorro_esp send_dma_command() functions. These all set both registers
explicitly before issuing the DMA command to the controller, so I
don’t think there’s a risk there either.

Kind regards,

Kars.

Re: [PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Kars de Jong]

Op zo 17 nov. 2019 om 07:26 schreef Kars de Jong <jongk@linux-m68k.org>:
> The only place the controller reads these registers is when a DMA
> command is issued.

The only time it does this, I mean.

Re: [PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Finn Thain]

On Sun, 17 Nov 2019, Kars de Jong wrote:

> Hi Finn,
> 
> Op za 16 nov. 2019 om 04:36 schreef Finn Thain 
> <fthain@telegraphics.com.au>:
> >
> > The zorro_esp driver uses both PIO and DMA transfers. If a failed DMA 
> > transfer happened to be followed by a PIO transfer, the TCLOW and 
> > TCMED registers would not get cleared. It is theoretically possible 
> > that the stale value from the transfer counter or the TCLOW/TCMED 
> > registers could then be used by the controller and the driver. Avoid 
> > that by clearing these registers before each PIO transfer.
> 
> Are you sure this is really needed?
> 

No. I think it improves robustness and correctness.

I would be interested to know whether there is any measurable performance 
impact on zorro_esp.

> The only [time when] the driver reads these registers is after a data 
> transfer. These are done using DMA on all Zorro boards, so I don't think 
> there's a risk of stale values from a PIO transfer there.
> 

I'm not entirely sure that the chip is unaffected by stale counter values. 

(Stale transfer counter values are distinct from stale transfer count 
register values. Both are addressed by the patch.)

If there are DMA controllers out there that can't do very short transfers 
then this objection would seem to be invalid, because the "DMA length is 
zero!" issue could be tackled using PIO.

> The only place the controller reads these registers is when a DMA 
> command is issued. The only place where that is done is in the zorro_esp 
> send_dma_command() functions. 

Aren't you overlooking all of the ESP_CMD_DMA flags in the core driver?

Thanks for your review.

-- 

> These all set both registers explicitly before issuing the DMA command 
> to the controller, so I don't think there's a risk there either.
> 
> Kind regards,
> 
> Kars.
> 

Re: [PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Finn Thain]

On Mon, 18 Nov 2019, Finn Thain wrote:

> On Sun, 17 Nov 2019, Kars de Jong wrote:
> 
> > The only [time when] the driver reads these registers is after a data 
> > transfer. These are done using DMA on all Zorro boards, so I don't 
> > think there's a risk of stale values from a PIO transfer there.
> > 
> 
> I'm not entirely sure that the chip is unaffected by stale counter 
> values.
> 
> (Stale transfer counter values are distinct from stale transfer count 
> register values. Both are addressed by the patch.)
> 

Sorry -- I should have said, "both were _intended_ to be addressed by the 
patch". But, as Michael pointed out, the DMA NOP command was missing from 
the v1 patch. Please see revised patch below.


diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bb88995a12c7..82d49f0f09df 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2835,6 +2835,10 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
 	cmd &= ~ESP_CMD_DMA;
 	esp->send_cmd_error = 0;
 
+	esp_write8(0, ESP_TCLOW);
+	esp_write8(0, ESP_TCMED);
+	scsi_esp_cmd(esp, ESP_CMD_NULL | ESP_CMD_DMA);
+
 	if (write) {
 		u8 *dst = (u8 *)addr;
 		u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : ESP_INTR_BSERV);
diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index 1c78bc10c790..797579247e47 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -361,8 +361,6 @@ static int esp_mac_probe(struct platform_device *dev)
 	esp->flags = ESP_FLAG_NO_DMA_MAP;
 	if (mep->pdma_io == NULL) {
 		printk(KERN_INFO PFX "using PIO for controller %d\n", dev->id);
-		esp_write8(0, ESP_TCLOW);
-		esp_write8(0, ESP_TCMED);
 		esp->flags |= ESP_FLAG_DISABLE_SYNC;
 		mac_esp_ops.send_dma_cmd = esp_send_pio_cmd;
 	} else {

Re: [PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Kars de Jong]

Hi Finn,

Op ma 18 nov. 2019 om 00:13 schreef Finn Thain <fthain@telegraphics.com.au>:
>
> On Sun, 17 Nov 2019, Kars de Jong wrote:
> > Are you sure this is really needed?
> >
>
> No. I think it improves robustness and correctness.
>
> I would be interested to know whether there is any measurable performance
> impact on zorro_esp.
>
> > The only [time when] the driver reads these registers is after a data
> > transfer. These are done using DMA on all Zorro boards, so I don't think
> > there's a risk of stale values from a PIO transfer there.
> >
>
> I'm not entirely sure that the chip is unaffected by stale counter values.
>
> (Stale transfer counter values are distinct from stale transfer count
> register values. Both are addressed by the patch.)

I still don't see the need to address that in the PIO transfer code.
The ESP (when in initiator mode)
doesn't use the transfer counter (registers) in PIO mode.

> If there are DMA controllers out there that can't do very short transfers
> then this objection would seem to be invalid, because the "DMA length is
> zero!" issue could be tackled using PIO.

That's the issue you fixed by limiting the transfer size to 65535
bytes, correct?
The SYM53CF92-X in my Blizzard didn't show this error for the 1 byte
transfers. It just hung up:

 sdb: RDSK (512) sdb1 (DOS^C)(res 2 spb 1) sdb2 (SWP^@)(res 0 spb 8)
sdb3 (LNX^@)(res 2 spb 1) sdb4 (LNX^@)(res 2 spb 1)
sd 1:0:0:0: [sdb] Attached SCSI disk
EXT4-fs (sdb3): mounting ext3 file system using the ext4 subsystem
scsi host1: Aborting command [(ptrval):28]
scsi host1: Current command [(ptrval):28]
scsi host1:  Active command [(ptrval):28]
scsi host1: Dumping command log
scsi host1: ent[6] CMD val[12] sreg[97] seqreg[9c] sreg2[00] ireg[08]
ss[00] event[0c]
scsi host1: ent[7] EVENT val[0d] sreg[91] seqreg[9c] sreg2[00]
ireg[08] ss[00] event[0c]
scsi host1: ent[8] EVENT val[03] sreg[91] seqreg[c4] sreg2[00]
ireg[10] ss[00] event[0d]
scsi host1: ent[9] CMD val[80] sreg[91] seqreg[c4] sreg2[00] ireg[10]
ss[00] event[03]
scsi host1: ent[10] CMD val[90] sreg[91] seqreg[c4] sreg2[00] ireg[10]
ss[00] event[03]
scsi host1: ent[11] EVENT val[05] sreg[91] seqreg[c4] sreg2[00]
ireg[10] ss[00] event[03]
scsi host1: ent[12] EVENT val[0d] sreg[93] seqreg[cc] sreg2[00]
ireg[10] ss[00] event[05]
scsi host1: ent[13] CMD val[01] sreg[93] seqreg[cc] sreg2[00] ireg[10]
ss[00] event[0d]
scsi host1: ent[14] CMD val[11] sreg[93] seqreg[cc] sreg2[00] ireg[10]
ss[00] event[0d]
scsi host1: ent[15] EVENT val[0b] sreg[93] seqreg[cc] sreg2[00]
ireg[10] ss[00] event[0d]
scsi host1: ent[16] CMD val[12] sreg[97] seqreg[cc] sreg2[00] ireg[08]
ss[00] event[0b]
scsi host1: ent[17] EVENT val[0c] sreg[97] seqreg[cc] sreg2[00]
ireg[08] ss[00] event[0b]
scsi host1: ent[18] CMD val[44] sreg[90] seqreg[cc] sreg2[00] ireg[20]
ss[00] event[0c]
scsi host1: ent[19] CMD val[01] sreg[90] seqreg[cc] sreg2[00] ireg[20]
ss[01] event[0c]
scsi host1: ent[20] CMD val[46] sreg[90] seqreg[cc] sreg2[00] ireg[20]
ss[01] event[0c]
scsi host1: ent[21] EVENT val[0d] sreg[97] seqreg[04] sreg2[00]
ireg[18] ss[00] event[0c]
scsi host1: ent[22] EVENT val[06] sreg[97] seqreg[04] sreg2[00]
ireg[18] ss[00] event[0d]
scsi host1: ent[23] CMD val[01] sreg[97] seqreg[04] sreg2[00] ireg[18]
ss[00] event[06]
scsi host1: ent[24] CMD val[10] sreg[97] seqreg[04] sreg2[00] ireg[18]
ss[00] event[06]
scsi host1: ent[25] EVENT val[0c] sreg[97] seqreg[8c] sreg2[00]
ireg[08] ss[00] event[06]
scsi host1: ent[26] CMD val[12] sreg[97] seqreg[8c] sreg2[00] ireg[08]
ss[00] event[0c]
scsi host1: ent[27] CMD val[44] sreg[90] seqreg[cc] sreg2[00] ireg[20]
ss[00] event[0c]
scsi host1: ent[28] CMD val[01] sreg[97] seqreg[9c] sreg2[00] ireg[0c]
ss[00] event[0c]
scsi host1: ent[29] CMD val[00] sreg[97] seqreg[9c] sreg2[00] ireg[0c]
ss[00] event[0c]
scsi host1: ent[30] CMD val[12] sreg[97] seqreg[9c] sreg2[00] ireg[0c]
ss[00] event[0c]
scsi host1: ent[31] CMD val[10] sreg[97] seqreg[9c] sreg2[00] ireg[10]
ss[00] event[0c]
scsi host1: ent[0] CMD val[12] sreg[97] seqreg[9c] sreg2[00] ireg[08]
ss[00] event[0c]
scsi host1: ent[1] EVENT val[0d] sreg[91] seqreg[9c] sreg2[00]
ireg[08] ss[00] event[0c]
scsi host1: ent[2] EVENT val[03] sreg[91] seqreg[c4] sreg2[00]
ireg[10] ss[00] event[0d]
scsi host1: ent[3] CMD val[80] sreg[91] seqreg[c4] sreg2[00] ireg[10]
ss[00] event[03]
scsi host1: ent[4] CMD val[90] sreg[91] seqreg[c4] sreg2[00] ireg[10]
ss[00] event[03]
scsi host1: ent[5] EVENT val[05] sreg[91] seqreg[c4] sreg2[00]
ireg[10] ss[00] event[03]

> > The only place the controller reads these registers is when a DMA
> > command is issued. The only place where that is done is in the zorro_esp
> > send_dma_command() functions.
>
> Aren't you overlooking all of the ESP_CMD_DMA flags in the core driver?

No, all those occurences are only used when calling
send_dma_command(), except the NULL/NOP DMA commands
directly after a chip reset.

Kind regards,

Kars.

Re: [PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Finn Thain]

On Mon, 18 Nov 2019, Kars de Jong wrote:

> Op ma 18 nov. 2019 om 00:13 schreef Finn Thain <fthain@telegraphics.com.au>:
> >
> > On Sun, 17 Nov 2019, Kars de Jong wrote:
> > > Are you sure this is really needed?
> > >
> >
> > No. I think it improves robustness and correctness.
> >
> > I would be interested to know whether there is any measurable performance
> > impact on zorro_esp.
> >
> > > The only [time when] the driver reads these registers is after a data
> > > transfer. These are done using DMA on all Zorro boards, so I don't think
> > > there's a risk of stale values from a PIO transfer there.
> > >
> >
> > I'm not entirely sure that the chip is unaffected by stale counter values.
> >
> > (Stale transfer counter values are distinct from stale transfer count
> > register values. Both are addressed by the patch.)
> 
> I still don't see the need to address that in the PIO transfer code.

Well, send_dma_cmd() always initializes those registers. That's fine in 
the DMA case and seems to be the least surprising and cleanest thing to do 
for the PIO case, I think.

> The ESP (when in initiator mode) doesn't use the transfer counter 
> (registers) in PIO mode.
> 
> > If there are DMA controllers out there that can't do very short 
> > transfers then this objection would seem to be invalid, because the 
> > "DMA length is zero!" issue could be tackled using PIO.
> 
> That's the issue you fixed by limiting the transfer size to 65535 bytes, 
> correct?

I was alluding to an unpatched theoretical failure.

The issue would arise when a board driver indicated to the core driver 
that the requested transfer was too small. I'm assuming that it would do 
this by returning zero from esp->ops->dma_length_limit(), which puts us in 
the "DMA length is zero" code path.

The actual failure (that you patched) shows that your board isn't able to 
do short DMA transfers and would require PIO for that.

But this is a theoretical problem so far. You may need to use sg_utils to 
generate a SCSI command like that.

(For a DMA controller subject to, say, 24-bit boundaries, there are 
additional ways to end up with short transfers.)

> The SYM53CF92-X in my Blizzard didn't show this error for the 1 byte
> transfers.
> It just hung up:
> 
>  sdb: RDSK (512) sdb1 (DOS^C)(res 2 spb 1) sdb2 (SWP^@)(res 0 spb 8)
> sdb3 (LNX^@)(res 2 spb 1) sdb4 (LNX^@)(res 2 spb 1)
> sd 1:0:0:0: [sdb] Attached SCSI disk
> EXT4-fs (sdb3): mounting ext3 file system using the ext4 subsystem
> scsi host1: Aborting command [(ptrval):28]
> scsi host1: Current command [(ptrval):28]
> scsi host1:  Active command [(ptrval):28]
> scsi host1: Dumping command log
> scsi host1: ent[6] CMD val[12] sreg[97] seqreg[9c] sreg2[00] ireg[08] ss[00] event[0c]
> scsi host1: ent[7] EVENT val[0d] sreg[91] seqreg[9c] sreg2[00] ireg[08] ss[00] event[0c]
> scsi host1: ent[8] EVENT val[03] sreg[91] seqreg[c4] sreg2[00] ireg[10] ss[00] event[0d]
> scsi host1: ent[9] CMD val[80] sreg[91] seqreg[c4] sreg2[00] ireg[10] ss[00] event[03]
> scsi host1: ent[10] CMD val[90] sreg[91] seqreg[c4] sreg2[00] ireg[10] ss[00] event[03]
> scsi host1: ent[11] EVENT val[05] sreg[91] seqreg[c4] sreg2[00] ireg[10] ss[00] event[03]
> scsi host1: ent[12] EVENT val[0d] sreg[93] seqreg[cc] sreg2[00] ireg[10] ss[00] event[05]
> scsi host1: ent[13] CMD val[01] sreg[93] seqreg[cc] sreg2[00] ireg[10] ss[00] event[0d]
> scsi host1: ent[14] CMD val[11] sreg[93] seqreg[cc] sreg2[00] ireg[10] ss[00] event[0d]
> scsi host1: ent[15] EVENT val[0b] sreg[93] seqreg[cc] sreg2[00] ireg[10] ss[00] event[0d]
> scsi host1: ent[16] CMD val[12] sreg[97] seqreg[cc] sreg2[00] ireg[08] ss[00] event[0b]
> scsi host1: ent[17] EVENT val[0c] sreg[97] seqreg[cc] sreg2[00] ireg[08] ss[00] event[0b]
> scsi host1: ent[18] CMD val[44] sreg[90] seqreg[cc] sreg2[00] ireg[20] ss[00] event[0c]
> scsi host1: ent[19] CMD val[01] sreg[90] seqreg[cc] sreg2[00] ireg[20] ss[01] event[0c]
> scsi host1: ent[20] CMD val[46] sreg[90] seqreg[cc] sreg2[00] ireg[20] ss[01] event[0c]
> scsi host1: ent[21] EVENT val[0d] sreg[97] seqreg[04] sreg2[00] ireg[18] ss[00] event[0c]
> scsi host1: ent[22] EVENT val[06] sreg[97] seqreg[04] sreg2[00] ireg[18] ss[00] event[0d]
> scsi host1: ent[23] CMD val[01] sreg[97] seqreg[04] sreg2[00] ireg[18] ss[00] event[06]
> scsi host1: ent[24] CMD val[10] sreg[97] seqreg[04] sreg2[00] ireg[18] ss[00] event[06]
> scsi host1: ent[25] EVENT val[0c] sreg[97] seqreg[8c] sreg2[00] ireg[08] ss[00] event[06]
> scsi host1: ent[26] CMD val[12] sreg[97] seqreg[8c] sreg2[00] ireg[08] ss[00] event[0c]
> scsi host1: ent[27] CMD val[44] sreg[90] seqreg[cc] sreg2[00] ireg[20] ss[00] event[0c]
> scsi host1: ent[28] CMD val[01] sreg[97] seqreg[9c] sreg2[00] ireg[0c] ss[00] event[0c]
> scsi host1: ent[29] CMD val[00] sreg[97] seqreg[9c] sreg2[00] ireg[0c] ss[00] event[0c]
> scsi host1: ent[30] CMD val[12] sreg[97] seqreg[9c] sreg2[00] ireg[0c] ss[00] event[0c]
> scsi host1: ent[31] CMD val[10] sreg[97] seqreg[9c] sreg2[00] ireg[10] ss[00] event[0c]
> scsi host1: ent[0] CMD val[12] sreg[97] seqreg[9c] sreg2[00] ireg[08] ss[00] event[0c]
> scsi host1: ent[1] EVENT val[0d] sreg[91] seqreg[9c] sreg2[00] ireg[08] ss[00] event[0c]
> scsi host1: ent[2] EVENT val[03] sreg[91] seqreg[c4] sreg2[00] ireg[10] ss[00] event[0d]
> scsi host1: ent[3] CMD val[80] sreg[91] seqreg[c4] sreg2[00] ireg[10] ss[00] event[03]
> scsi host1: ent[4] CMD val[90] sreg[91] seqreg[c4] sreg2[00] ireg[10] ss[00] event[03]
> scsi host1: ent[5] EVENT val[05] sreg[91] seqreg[c4] sreg2[00] ireg[10] ss[00] event[03]
> 
> > > The only place the controller reads these registers is when a DMA
> > > command is issued. The only place where that is done is in the zorro_esp
> > > send_dma_command() functions.
> >
> > Aren't you overlooking all of the ESP_CMD_DMA flags in the core driver?
> 
> No, all those occurences are only used when calling
> send_dma_command(), except the NULL/NOP DMA commands
> directly after a chip reset.
> 

Fair enough. That means a stale count could be used after a PIO transfer 
in DATA IN and DATA OUT phase that happened to follow a DMA transfer (in 
any phase).

I still think there is a opportunity to improve robustness and correctness 
(presuming that performance isn't impacted).

-- 

> Kind regards,
> 
> Kars.
> 

Re: [PATCH] esp_scsi: Clear Transfer Count registers before PIO transfers

[Finn Thain]

On Tue, 19 Nov 2019, I wrote:

> 
> But this is a theoretical problem so far. You may need to use sg_utils 
> to generate a SCSI command like that.
> 
> (For a DMA controller subject to, say, 24-bit boundaries, there are 
> additional ways to end up with short transfers.)
> 

On reflection these probably can't happen in practice. I was being 
over-cautious. So I'll leave this patch out-of-tree until there's a need 
for it (i.e. some driver that uses both DMA and PIO in data phases).

--