g_NCR5380: PDMA fixes and cleanup

Ondrej, would you please test this new series?

Changed since v1:
- PDMA transfer residual is calculated earlier.
- End of DMA flag check is now polled (if there is any residual).

Changed since v2:
- Bail out of transfer loops when Gated IRQ gets asserted.
- Make udelay conditional on board type.
- Drop sg_tablesize patch due to performance regression.

Finn Thain (1):
g_NCR5380: Cleanup comments and whitespace

Ondrej Zary (3):
g_NCR5380: Fix PDMA transfer size
g_NCR5380: End PDMA transfer correctly on target disconnection
g_NCR5380: Re-work PDMA loops

 drivers/scsi/g_NCR5380.c | 239

+++++++++++++++++++++++------------------------ 1 file changed, 116 insertions(+), 123 deletions(-)

No apparent change in behavior, the first write test resulted in:

[  842.830802] sd 2:0:1:0: [sdb] tag#0 53c80 registers not accessible, device will be reset
[  842.830802] sd 2:0:1:0: [sdb] tag#0 switching to slow handshake

Checking for IRQ after poll_politely2 does not seem right because we can miss the buffer ready signal.

According to my tests, buffer ready signal is most important - if there is any data to read/write, do the transfer. If not, only then check why
- maybe we got an IRQ (that terminated PDMA). Or no IRQ, sometimes the wait for buffer ready times out - we need to terminate PDMA manually then (reset).

Then 53C80 registers should become ready.

This is a log from writing 230 MB file using my code with some debug prints, on a NCR53C400. No 53C80 timeouts, only disconnects and some host buffer timeouts (maybe the drive sometimes just slows down without disconnecting?)

[ 3378.503828] basr=0x10 csr=0xd5 at start=512
[ 3461.257973] w basr=0x10 csr=0x95 at start=3840
[ 3461.838225] w basr=0x10 csr=0x95 at start=3840
[ 3462.683446] w basr=0x10 csr=0x95 at start=3840
[ 3463.416911] w basr=0x10 csr=0x95 at start=3840
[ 3465.117804] scsi host2: 53C400w: host buffer not ready in time
[ 3465.276375] w basr=0x10 csr=0x95 at start=3328
[ 3466.457701] w basr=0x10 csr=0x95 at start=1792
[ 3467.019643] scsi host2: 53C400w: host buffer not ready in time
[ 3467.619058] scsi host2: 53C400w: host buffer not ready in time
[ 3467.799619] w basr=0x10 csr=0x95 at start=3840
[ 3552.123501] w basr=0x10 csr=0x95 at start=2304
[ 3552.771223] w basr=0x10 csr=0x95 at start=1280
[ 3554.556451] w basr=0x10 csr=0x95 at start=2816
[ 3555.229646] w basr=0x10 csr=0x95 at start=1792
[ 3555.630632] scsi host2: 53C400w: host buffer not ready in time
[ 3555.774560] w basr=0x10 csr=0x95 at start=768
[ 3625.541608] w basr=0x10 csr=0x95 at start=3328
[ 3640.099861] w basr=0x10 csr=0x95 at start=1792
[ 3641.442671] w basr=0x10 csr=0x95 at start=2816
[ 3641.865469] w basr=0x10 csr=0x95 at start=768
[ 3642.939223] w basr=0x10 csr=0x95 at start=1280
[ 3643.356858] w basr=0x10 csr=0x95 at start=3328
[ 3643.701636] w basr=0x10 csr=0x95 at start=3840
[ 3645.153405] w basr=0x10 csr=0x95 at start=2304
[ 3646.135642] w basr=0x10 csr=0x95 at start=1280
[ 3647.007321] w basr=0x10 csr=0x95 at start=2816
[ 3648.065874] w basr=0x10 csr=0x95 at start=3328
[ 3650.071961] w basr=0x10 csr=0x95 at start=1280
[ 3650.827630] w basr=0x10 csr=0x95 at start=1792
[ 3651.827011] w basr=0x10 csr=0x95 at start=2816
[ 3652.559984] w basr=0x10 csr=0x95 at start=2816
[ 3653.203566] w basr=0x10 csr=0x95 at start=3328
[ 3653.594376] w basr=0x10 csr=0x95 at start=1280
[ 3653.903437] w basr=0x10 csr=0x95 at start=3840
[ 3654.305753] w basr=0x10 csr=0x95 at start=1792
[ 3654.676009] w basr=0x10 csr=0x95 at start=2304
[ 3655.367686] w basr=0x10 csr=0x95 at start=2816
[ 3655.733854] w basr=0x10 csr=0x95 at start=768
[ 3656.075023] w basr=0x10 csr=0x95 at start=3328
[ 3656.493046] w basr=0x10 csr=0x95 at start=2816
[ 3657.208089] w basr=0x10 csr=0x95 at start=1280
[ 3657.537223] w basr=0x10 csr=0x95 at start=1280

And this is from reading the file back:

[ 3799.053067] basr=0x10 csr=0xd5 at start=512
[ 3801.056337] basr=0x10 csr=0xd5 at start=3584
[ 3976.323836] scsi host2: 53C400r: host buffer not ready in time
[ 3976.404699] basr=0x10 csr=0xd5 at start=512
[ 3977.800647] basr=0x10 csr=0xd5 at start=512
[ 3979.240611] scsi host2: 53C400r: host buffer not ready in time
[ 3979.320698] basr=0x10 csr=0xd5 at start=512
[ 3980.040220] scsi host2: 53C400r: host buffer not ready in time
[ 3980.096401] basr=0x10 csr=0xd5 at start=512
[ 3980.394854] scsi host2: 53C400r: host buffer not ready in time

Quoted 5 lines · Ondrej Zary

No apparent change in behavior, the first write test resulted in:

[  842.830802] sd 2:0:1:0: [sdb] tag#0 53c80 registers not accessible,

device will be reset [ 842.830802] sd 2:0:1:0: [sdb] tag#0 switching to slow handshake

Quoted 2 lines

Checking for IRQ after poll_politely2 does not seem right because we can miss the buffer ready signal.

How so? As long as there's no gated IRQ, we poll for buffer readiness until timeout. And when there is a gated IRQ, we break both the polling loop and the transfer loop immediately. Your code and mine are basically in agreement here.

Quoted 6 lines

According to my tests, buffer ready signal is most important - if there is any data to read/write, do the transfer. If not, only then check why
- maybe we got an IRQ (that terminated PDMA). Or no IRQ, sometimes the wait for buffer ready times out - we need to terminate PDMA manually then (reset).

Quoted 1 line

Then 53C80 registers should become ready.

You seem to be saying that we should ignore the IRQ signal if the buffers have become ready. Maybe so. Can we try simply resetting the block counter? (I could imagine that the 53c400 core might leave the 53c80 registers inaccessible unless we keep accessing the buffers in the 53c400 core until the transfer is done.)

BTW, with regard to your patch, note that this construct is race prone:

while (1) { /* monitor IRQ while waiting for host buffer */

	csr = NCR5380_read(hostdata->c400_ctl_status);
	if (!(csr & CSR_HOST_BUF_NOT_RDY))
		break;
	if (csr & CSR_GATED_53C80_IRQ) {
		basr = NCR5380_read(BUS_AND_STATUS_REG);
		if (!(basr & BASR_PHASE_MATCH) ||
			   (basr & BASR_BUSY_ERROR)) {
			printk("basr=0x%02x csr=0x%02x at start=%d\n", basr, csr, start);
			goto out_wait;
		}
	}
	if (retries-- < 1) {
		shost_printk(KERN_ERR, hostdata->host, "53C400r: host buffer not ready in
time\n"); NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
		NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
		goto out_wait;
	}
}

This code can "goto out_wait" when !(csr & CSR_HOST_BUF_NOT_RDY). It depends on timing. This would seem to be contrary to your stated aim.

Moreover, this code can also "break" when (csr & CSR_GATED_53C80_IRQ). That depends on timing too. But this may be an improvement on my code if it allows the 53c80 registers to become accessible, by allowing the block counter to be decremented.

The uncertainty here was one of the reasons I reworked this code.

Quoted 50 lines

This is a log from writing 230 MB file using my code with some debug prints, on a NCR53C400. No 53C80 timeouts, only disconnects and some host buffer timeouts (maybe the drive sometimes just slows down without disconnecting?)

[ 3378.503828] basr=0x10 csr=0xd5 at start=512
[ 3461.257973] w basr=0x10 csr=0x95 at start=3840
[ 3461.838225] w basr=0x10 csr=0x95 at start=3840
[ 3462.683446] w basr=0x10 csr=0x95 at start=3840
[ 3463.416911] w basr=0x10 csr=0x95 at start=3840
[ 3465.117804] scsi host2: 53C400w: host buffer not ready in time
[ 3465.276375] w basr=0x10 csr=0x95 at start=3328
[ 3466.457701] w basr=0x10 csr=0x95 at start=1792
[ 3467.019643] scsi host2: 53C400w: host buffer not ready in time
[ 3467.619058] scsi host2: 53C400w: host buffer not ready in time
[ 3467.799619] w basr=0x10 csr=0x95 at start=3840
[ 3552.123501] w basr=0x10 csr=0x95 at start=2304
[ 3552.771223] w basr=0x10 csr=0x95 at start=1280
[ 3554.556451] w basr=0x10 csr=0x95 at start=2816
[ 3555.229646] w basr=0x10 csr=0x95 at start=1792
[ 3555.630632] scsi host2: 53C400w: host buffer not ready in time
[ 3555.774560] w basr=0x10 csr=0x95 at start=768
[ 3625.541608] w basr=0x10 csr=0x95 at start=3328
[ 3640.099861] w basr=0x10 csr=0x95 at start=1792
[ 3641.442671] w basr=0x10 csr=0x95 at start=2816
[ 3641.865469] w basr=0x10 csr=0x95 at start=768
[ 3642.939223] w basr=0x10 csr=0x95 at start=1280
[ 3643.356858] w basr=0x10 csr=0x95 at start=3328
[ 3643.701636] w basr=0x10 csr=0x95 at start=3840
[ 3645.153405] w basr=0x10 csr=0x95 at start=2304
[ 3646.135642] w basr=0x10 csr=0x95 at start=1280
[ 3647.007321] w basr=0x10 csr=0x95 at start=2816
[ 3648.065874] w basr=0x10 csr=0x95 at start=3328
[ 3650.071961] w basr=0x10 csr=0x95 at start=1280
[ 3650.827630] w basr=0x10 csr=0x95 at start=1792
[ 3651.827011] w basr=0x10 csr=0x95 at start=2816
[ 3652.559984] w basr=0x10 csr=0x95 at start=2816
[ 3653.203566] w basr=0x10 csr=0x95 at start=3328
[ 3653.594376] w basr=0x10 csr=0x95 at start=1280
[ 3653.903437] w basr=0x10 csr=0x95 at start=3840
[ 3654.305753] w basr=0x10 csr=0x95 at start=1792
[ 3654.676009] w basr=0x10 csr=0x95 at start=2304
[ 3655.367686] w basr=0x10 csr=0x95 at start=2816
[ 3655.733854] w basr=0x10 csr=0x95 at start=768
[ 3656.075023] w basr=0x10 csr=0x95 at start=3328
[ 3656.493046] w basr=0x10 csr=0x95 at start=2816
[ 3657.208089] w basr=0x10 csr=0x95 at start=1280
[ 3657.537223] w basr=0x10 csr=0x95 at start=1280

And this is from reading the file back:

Quoted 10 lines

[ 3799.053067] basr=0x10 csr=0xd5 at start=512
[ 3801.056337] basr=0x10 csr=0xd5 at start=3584
[ 3976.323836] scsi host2: 53C400r: host buffer not ready in time
[ 3976.404699] basr=0x10 csr=0xd5 at start=512
[ 3977.800647] basr=0x10 csr=0xd5 at start=512
[ 3979.240611] scsi host2: 53C400r: host buffer not ready in time
[ 3979.320698] basr=0x10 csr=0xd5 at start=512
[ 3980.040220] scsi host2: 53C400r: host buffer not ready in time
[ 3980.096401] basr=0x10 csr=0xd5 at start=512
[ 3980.394854] scsi host2: 53C400r: host buffer not ready in time

The register values look normal (?)

Anyway, there are only a few material differences between your code and this patch series.

1) Your code does not break the transfer loop for any Gated IRQ, but only for a phase error IRQ. My version responds to any Gated IRQ, as per the algorithm in the datasheet. Your code seems to assume that the 53c80 registers are accessible whenever gated IRQ is set, which seems unlikely.

2) My code fails the transfer when the 53c80 registers don't become accessible, which is why you see a fallback to PIO (slow handshake).

3) We use different timeouts.

4) In my code, the race condition applies only after the timeout has already expired, so it's inconsequential.

Please apply the patch below (on top of this series) so we can perhaps narrow this down a bit.

Quoted 5 lines · On Tuesday 27 June 2017 03:49:16 Finn Thain wrote:

... As long as there's no gated IRQ, we poll for buffer readiness until timeout. And when there is a gated IRQ, we break both the polling loop and the transfer loop immediately. Your code and mine are basically in agreement here.

Yes, it stops transfer when an IRQ arrives. But the host buffer could be ready at the same time. The IRQ can be an "end-of-DMA" IRQ (IIRC DTC chips assert this earlier than 53C400). Or just a disconnect that occured right now but the chip already read a buffer full of data.

Quoted 8 lines

Quoted 7 lines

According to my tests, buffer ready signal is most important - if there is any data to read/write, do the transfer. If not, only then check why - maybe we got an IRQ (that terminated PDMA). Or no IRQ, sometimes the wait for buffer ready times out - we need to terminate PDMA manually then (reset).

Then 53C80 registers should become ready.

Quoted 5 lines

You seem to be saying that we should ignore the IRQ signal if the buffers have become ready. Maybe so. Can we try simply resetting the block counter? (I could imagine that the 53c400 core might leave the 53c80 registers inaccessible unless we keep accessing the buffers in the 53c400 core until the transfer is done.)

We can't reset the block counter because 0 means 256 blocks to transfer (page 13 in datasheet).

Yes, the 53C80 registers seem to become available only when the PDMA transfer ends by either:
1. transferring all blocks (block counter decrementing to zero)
2. IRQ

3. reset

Quoted 25 lines

BTW, with regard to your patch, note that this construct is race prone:

while (1) { /* monitor IRQ while waiting for host buffer */

	csr = NCR5380_read(hostdata->c400_ctl_status);
	if (!(csr & CSR_HOST_BUF_NOT_RDY))
		break;
	if (csr & CSR_GATED_53C80_IRQ) {
		basr = NCR5380_read(BUS_AND_STATUS_REG);
		if (!(basr & BASR_PHASE_MATCH) ||
			   (basr & BASR_BUSY_ERROR)) {
			printk("basr=0x%02x csr=0x%02x at start=%d\n", basr, csr, start);
			goto out_wait;
		}
	}
	if (retries-- < 1) {
		shost_printk(KERN_ERR, hostdata->host, "53C400r: host buffer not ready in
time\n"); NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
		NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
		goto out_wait;
	}
}

This code can "goto out_wait" when !(csr & CSR_HOST_BUF_NOT_RDY). It depends on timing. This would seem to be contrary to your stated aim.

Quoted 4 lines

Moreover, this code can also "break" when (csr & CSR_GATED_53C80_IRQ). That depends on timing too. But this may be an improvement on my code if it allows the 53c80 registers to become accessible, by allowing the block counter to be decremented.

Yes, it continue the transfer even if the IRQ is asserted - as long as the buffer is ready. That's intended.

Quoted 1 line

The uncertainty here was one of the reasons I reworked this code.

My version reads CSR only once per loop but that probably does not help at all as the HW state could change anytime. The chip's design seems to be very race-prone.

Quoted 66 lines

Quoted 50 lines

This is a log from writing 230 MB file using my code with some debug prints, on a NCR53C400. No 53C80 timeouts, only disconnects and some host buffer timeouts (maybe the drive sometimes just slows down without disconnecting?)

[ 3378.503828] basr=0x10 csr=0xd5 at start=512
[ 3461.257973] w basr=0x10 csr=0x95 at start=3840
[ 3461.838225] w basr=0x10 csr=0x95 at start=3840
[ 3462.683446] w basr=0x10 csr=0x95 at start=3840
[ 3463.416911] w basr=0x10 csr=0x95 at start=3840
[ 3465.117804] scsi host2: 53C400w: host buffer not ready in time
[ 3465.276375] w basr=0x10 csr=0x95 at start=3328
[ 3466.457701] w basr=0x10 csr=0x95 at start=1792
[ 3467.019643] scsi host2: 53C400w: host buffer not ready in time
[ 3467.619058] scsi host2: 53C400w: host buffer not ready in time
[ 3467.799619] w basr=0x10 csr=0x95 at start=3840
[ 3552.123501] w basr=0x10 csr=0x95 at start=2304
[ 3552.771223] w basr=0x10 csr=0x95 at start=1280
[ 3554.556451] w basr=0x10 csr=0x95 at start=2816
[ 3555.229646] w basr=0x10 csr=0x95 at start=1792
[ 3555.630632] scsi host2: 53C400w: host buffer not ready in time
[ 3555.774560] w basr=0x10 csr=0x95 at start=768
[ 3625.541608] w basr=0x10 csr=0x95 at start=3328
[ 3640.099861] w basr=0x10 csr=0x95 at start=1792
[ 3641.442671] w basr=0x10 csr=0x95 at start=2816
[ 3641.865469] w basr=0x10 csr=0x95 at start=768
[ 3642.939223] w basr=0x10 csr=0x95 at start=1280
[ 3643.356858] w basr=0x10 csr=0x95 at start=3328
[ 3643.701636] w basr=0x10 csr=0x95 at start=3840
[ 3645.153405] w basr=0x10 csr=0x95 at start=2304
[ 3646.135642] w basr=0x10 csr=0x95 at start=1280
[ 3647.007321] w basr=0x10 csr=0x95 at start=2816
[ 3648.065874] w basr=0x10 csr=0x95 at start=3328
[ 3650.071961] w basr=0x10 csr=0x95 at start=1280
[ 3650.827630] w basr=0x10 csr=0x95 at start=1792
[ 3651.827011] w basr=0x10 csr=0x95 at start=2816
[ 3652.559984] w basr=0x10 csr=0x95 at start=2816
[ 3653.203566] w basr=0x10 csr=0x95 at start=3328
[ 3653.594376] w basr=0x10 csr=0x95 at start=1280
[ 3653.903437] w basr=0x10 csr=0x95 at start=3840
[ 3654.305753] w basr=0x10 csr=0x95 at start=1792
[ 3654.676009] w basr=0x10 csr=0x95 at start=2304
[ 3655.367686] w basr=0x10 csr=0x95 at start=2816
[ 3655.733854] w basr=0x10 csr=0x95 at start=768
[ 3656.075023] w basr=0x10 csr=0x95 at start=3328
[ 3656.493046] w basr=0x10 csr=0x95 at start=2816
[ 3657.208089] w basr=0x10 csr=0x95 at start=1280
[ 3657.537223] w basr=0x10 csr=0x95 at start=1280

And this is from reading the file back:

Quoted 10 lines

[ 3799.053067] basr=0x10 csr=0xd5 at start=512
[ 3801.056337] basr=0x10 csr=0xd5 at start=3584
[ 3976.323836] scsi host2: 53C400r: host buffer not ready in time
[ 3976.404699] basr=0x10 csr=0xd5 at start=512
[ 3977.800647] basr=0x10 csr=0xd5 at start=512
[ 3979.240611] scsi host2: 53C400r: host buffer not ready in time
[ 3979.320698] basr=0x10 csr=0xd5 at start=512
[ 3980.040220] scsi host2: 53C400r: host buffer not ready in time
[ 3980.096401] basr=0x10 csr=0xd5 at start=512
[ 3980.394854] scsi host2: 53C400r: host buffer not ready in time

The register values look normal (?)

Anyway, there are only a few material differences between your code and this patch series.

Quoted 5 lines

1) Your code does not break the transfer loop for any Gated IRQ, but only for a phase error IRQ. My version responds to any Gated IRQ, as per the algorithm in the datasheet. Your code seems to assume that the 53c80 registers are accessible whenever gated IRQ is set, which seems unlikely.

BASR seems to be always readable. Nothing in the datasheet about that but I needed it to determine the IRQ type and it works.

Quoted 15 lines · On Monday 26 June 2017 09:30:33 Finn Thain wrote:

Ondrej, would you please test this new series?

Changed since v1:
- PDMA transfer residual is calculated earlier.
- End of DMA flag check is now polled (if there is any residual).

Changed since v2:
- Bail out of transfer loops when Gated IRQ gets asserted.
- Make udelay conditional on board type.
- Drop sg_tablesize patch due to performance regression.

Finn Thain (1):
g_NCR5380: Cleanup comments and whitespace

Quoted 8 lines

Ondrej Zary (3):
g_NCR5380: Fix PDMA transfer size
g_NCR5380: End PDMA transfer correctly on target disconnection
g_NCR5380: Re-work PDMA loops

 drivers/scsi/g_NCR5380.c | 239

+++++++++++++++++++++++------------------------ 1 file changed, 116 insertions(+), 123 deletions(-)

Quoted 16 lines · On Monday 26 June 2017 09:30:33 Finn Thain wrote:

Quoted 15 lines

Ondrej, would you please test this new series?

Changed since v1:
- PDMA transfer residual is calculated earlier.
- End of DMA flag check is now polled (if there is any residual).

Changed since v2:
- Bail out of transfer loops when Gated IRQ gets asserted.
- Make udelay conditional on board type.
- Drop sg_tablesize patch due to performance regression.

Finn Thain (1):
g_NCR5380: Cleanup comments and whitespace

Quoted 8 lines

Quoted 8 lines

Ondrej Zary (3):
g_NCR5380: Fix PDMA transfer size
g_NCR5380: End PDMA transfer correctly on target disconnection
g_NCR5380: Re-work PDMA loops

 drivers/scsi/g_NCR5380.c | 239

+++++++++++++++++++++++------------------------ 1 file changed, 116 insertions(+), 123 deletions(-)

BTW. I've probably found the DTC write corruption.
Added the following check (13 is host buffer index register) - and it triggers
sometimes: the value is 1 instead of 0. As we use only 16-bit writes, I don't see how the value could ever be odd. Looks like a bug in the chip. The index register corrupts during the transfer, not after IRQ or timeout. The same check at beginning of pwrite() did not trigger.

The index register is not writable so we must(?) reset the PDMA engine to recover. However, this quick attempt to fix does not work, maybe we should reload the block count and continue?

drivers/scsi/g_NCR5380.c +7 -1

@@ -595,7 +603,13 @@ static inline int generic_NCR5380_pwrite(struct 
 NCR5380_hostdata *hostdata,
                                goto out_wait;
                        }
                }
-
+               idx = NCR5380_read(13);
+               if (idx != 0) {
+                       printk("host idx=%d, start=%d\n", idx, start);
+                       NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
+                       NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
+                       goto out_wait;
+               }
                if (hostdata->io_port && hostdata->io_width == 2)
                        outsw(hostdata->io_port + hostdata->c400_host_buf,

                                                        src + start, 64);

Ondrej,

could this be a partial write (target did not transfer the last byte)?

One would suppose the chip posts a phase mismatch in that case ...

Cheers,

Michael

BTW. I've probably found the DTC write corruption. Added the following check (13 is host buffer index register) -

and it triggers sometimes: the value is 1 instead of 0. As we use only 16-bit writes, I don't see how the value could ever be odd. Looks like a bug in the chip. The index register corrupts during the transfer, not after IRQ or timeout. The same check at beginning of pwrite() did not trigger.

The index register is not writable so we must(?) reset the PDMA engine to recover. However, this quick attempt to fix does not work, maybe we should reload the block count and continue?

drivers/scsi/g_NCR5380.c +7 -1

@@ -595,7 +603,13 @@ static inline int generic_NCR5380_pwrite(struct 
 NCR5380_hostdata *hostdata,
                                goto out_wait;
                        }
                }
-
+               idx = NCR5380_read(13);
+               if (idx != 0) {
+                       printk("host idx=%d, start=%d\n", idx, start);
+                       NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
+                       NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
+                       goto out_wait;
+               }
                if (hostdata->io_port && hostdata->io_width == 2)
                        outsw(hostdata->io_port + hostdata->c400_host_buf,

                                                        src + start, 64);

Quoted 2 lines · Ondrej Zary

BTW. I've probably found the DTC write corruption. Added the following check (13 is host buffer index register) -

That register is not mentioned in my 53c400 datasheet.

Quoted 5 lines

and it triggers sometimes: the value is 1 instead of 0. As we use only 16-bit writes, I don't see how the value could ever be odd. Looks like a bug in the chip. The index register corrupts during the transfer, not after IRQ or timeout. The same check at beginning of pwrite() did not trigger.

Are you reading this register at the right moment? Have you tried waiting for it to reach zero, as in,

	if (NCR5380_poll_politely(hostdata, 13, 0xff, 0, HZ / 64) < 0)
		/* printk, reset etc */;

Even if this is a reliable way to detect a short transfer, it would be nice to know the root cause. But I'm being unrealistic: the DTC436 vendor never responded to my requests for technical documentation.

Quoted 3 lines

The index register is not writable so we must(?) reset the PDMA engine to recover. However, this quick attempt to fix does not work, maybe we should reload the block count and continue?

I don't know if it is possible to recover. If the last byte never reached the scsi bus, then once you reset the 53c400 core, you need the driver to perform a single-byte PIO transfer after the short PDMA transfer. This would require that you set the residual appropriately (though in my experience that may not be sufficient).

It may be better to simply limit the transfer to 512 bytes instead of attempting to recover based on an undocumented (?) register, etc. Seems like a bit of a hack.

Quoted 16 lines

drivers/scsi/g_NCR5380.c +4 -1

@@ -595,7 +603,13 @@ static inline int generic_NCR5380_pwrite(struct
 NCR5380_hostdata *hostdata,
                                goto out_wait;
                        }
                }
-
+               idx = NCR5380_read(13);
+               if (idx != 0) {
+                       printk("host idx=%d, start=%d\n", idx, start);
+                       NCR5380_write(hostdata->c400_ctl_status,

CSR_RESET); + NCR5380_write(hostdata->c400_ctl_status, CSR_BASE); +
goto out_wait;
+ }

Quoted 3 lines

                if (hostdata->io_port && hostdata->io_width == 2)
                        outsw(hostdata->io_port +

hostdata->c400_host_buf, src + start, 64);

I find it hard to reason about this code. For example, out_wait is to be removed. Let's get the preceding patches working and signed-off. Please go ahead and use a 512 B transfer for DTC436 testing if that will help get this patch series over the line.

Quoted 6 lines · On Tuesday 27 June 2017 14:42:29 Finn Thain wrote:

Quoted 5 lines

... it triggers sometimes: the value is 1 instead of 0. As we use only 16-bit writes, I don't see how the value could ever be odd. Looks like a bug in the chip. The index register corrupts during the transfer, not after IRQ or timeout. The same check at beginning of pwrite() did not trigger.

Quoted 5 lines

Are you reading this register at the right moment? Have you tried waiting for it to reach zero, as in,

	if (NCR5380_poll_politely(hostdata, 13, 0xff, 0, HZ / 64) < 0)
		/* printk, reset etc */;

I have not but will try (expecting that it will not change by itself).

Quoted 3 lines

Even if this is a reliable way to detect a short transfer, it would be nice to know the root cause. But I'm being unrealistic: the DTC436 vendor never responded to my requests for technical documentation.

According to the data corruption observed, it's not a short transfer. The corruption is always the same: one byte missing at the beginning of a 128 B block. It happens only with slow Quantum LPS 240 drive, not with faster IBM DORS-32160.