Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582971 > unrolled thread

Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches Ondrej Zary <linux@rainbow-software.org> - 2017-02-16 23:20 +0100
    Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor  patches Finn Thain <fthain@telegraphics.com.au> - 2017-02-17 23:40 +0100
      Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches Ondrej Zary <linux@rainbow-software.org> - 2017-02-18 12:20 +0100
        Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor  patches Finn Thain <fthain@telegraphics.com.au> - 2017-02-19 00:30 +0100
          Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches Ondrej Zary <linux@rainbow-software.org> - 2017-02-19 12:30 +0100
            Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor  patches Finn Thain <fthain@telegraphics.com.au> - 2017-02-20 00:20 +0100

#1582971 — Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
[...]
> Are you trying to figure out which commands are going to disconnect during
> a transfer? This is really a function of the firmware in the target; there
> are no good heuristics AFAICT, so the PDMA algorithm has to be robust.
> mac_scsi has to cope with this too.
>
> Does the problem go away when you assign no IRQ? When instance->irq ==
> NO_IRQ, the core driver will inhibit disconnect privileges.

Yes, it seems to run fine with IRQ/disconnect disabled.
With IRQ enabled, "dd if=/dev/sr0 of=anything" stops after a while. I get 
gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.

When I enable interrupts during PDMA (like the removed UNSAFE macro did), the 
problems go away. I see an IRQ after each pread call.
(had to disable "no 53C80 gated irq after transfer" and "no end dma signal" 
messages to reduce log spam)

-- 
Ondrej Zary

[toc] | [next] | [standalone]

#1583782 — Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

On Thu, 16 Feb 2017, Ondrej Zary wrote:

> On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
> [...]
> > Are you trying to figure out which commands are going to disconnect 
> > during a transfer? This is really a function of the firmware in the 
> > target; there are no good heuristics AFAICT, so the PDMA algorithm has 
> > to be robust. mac_scsi has to cope with this too.
> >
> > Does the problem go away when you assign no IRQ? When instance->irq == 
> > NO_IRQ, the core driver will inhibit disconnect privileges.
> 
> Yes, it seems to run fine with IRQ/disconnect disabled. With IRQ 
> enabled, "dd if=/dev/sr0 of=anything" stops after a while.

When you say "stops", do you mean an infinite loop in 
generic_NCR5380_pread or does the loop complete (which would presumably 
leave the target stuck in DATA IN phase, and a scsi command timeout would 
probably follow after 30 seconds...)

> I get gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.
> 

You can use NCR5380_print() to get a decoded register dump.

When I decode the above values I get,

BASR   = 0x10 = BASR_IRQ
MODE   = 0x0e = MR_ENABLE_EOP_INTR | MR_MONITOR_BSY | MR_DMA_MODE
STATUS = 0x7c = SR_BSY | SR_REQ | SR_MSG | SR_CD | SR_IO

Since BASR_PHASE_MATCH is not set, the interrupt is almost certainly a 
phase mismatch. The new phase is SR_MSG | SR_CD | SR_IO = PHASE_MSGIN, 
which shows that the target has given up on the DATA IN phase and is 
presumably trying to send a DISCONNECT message.

> When I enable interrupts during PDMA (like the removed UNSAFE macro 
> did), the problems go away. I see an IRQ after each pread call.

You shouldn't need an interrupt, because NCR5380_dma_complete() gets 
called regardless. AFAICT, the only difference is the timing, which 
becomes less predictable. Calling spinlock_irq_restore() before the 
transfer seems to create a race condition between hostdata->dma_len store 
and load.

I think that the pread call has not yet returned when the interrupt fires 
and NCR5380_dma_complete() is called. Hence hostdata->dma_len has not yet 
been initialized. So when NCR5380_dma_complete() is called by the 
interrupt handler, SCp.this_residual will not change at all because 
hostdata->dma_len is still zero.

> (had to disable "no 53C80 gated irq after transfer" and "no end dma 
> signal" messages to reduce log spam)
> 

That may provide a quick way to detect the failed PDMA transfer, at least 
for testing purposes. There may be a more conclusive test for a partial 
transfer.

We could to implement something like macscsi_dma_residual() to take care 
of a failed PDMA transfer. That way, the failure can be taken into account 
when NCR5380_dma_complete() is called at the end of the transfer.

See patch below for example. It should confirm the theory above though it 
really needs some timeouts added (NCR5380_poll_politely()). And perhaps we 
could do something more clever than retry indefinitely, though we can 
still rely on the command timeout.

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 6f9665d..75cfaf3 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -497,15 +497,17 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
 		blocks--;
 	}
 
+	hostdata->pdma_residual = 0;
+
 	if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
-		printk("53C400r: no 53C80 gated irq after transfer");
+		hostdata->pdma_residual = hostdata->dma_len;
 
 	/* wait for 53C80 registers to be available */
 	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
 		;
 
 	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
-		printk(KERN_ERR "53C400r: no end dma signal\n");
+		hostdata->pdma_residual = hostdata->dma_len;
 		
 	return 0;
 }
@@ -576,12 +578,14 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 		/* FIXME - no timeout */
 	}
 
-	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER)) {
-		printk(KERN_ERR "53C400w: no end dma signal\n");
-	}
+	hostdata->pdma_residual = 0;
+
+	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
+		hostdata->pdma_residual = hostdata->dma_len;
 
 	while (!(NCR5380_read(TARGET_COMMAND_REG) & TCR_LAST_BYTE_SENT))
 		; 	// TIMEOUT
+
 	return 0;
 }
 
@@ -607,6 +611,11 @@ static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
 	return transfersize;
 }
 
+static int generic_NCR5380_dma_residual(struct NCR5380_hostdata *hostdata)
+{
+	return hostdata->pdma_residual;
+}
+
 /*
  *	Include the NCR5380 core code that we build our driver around	
  */
diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
index 81b22d9..08c91b7 100644
--- a/drivers/scsi/g_NCR5380.h
+++ b/drivers/scsi/g_NCR5380.h
@@ -26,7 +26,8 @@
 	int c400_ctl_status; \
 	int c400_blk_cnt; \
 	int c400_host_buf; \
-	int io_width;
+	int io_width; \
+	int pdma_residual;
 
 #define NCR53C400_mem_base 0x3880
 #define NCR53C400_host_buffer 0x3900
@@ -35,7 +36,7 @@
 #define NCR5380_dma_xfer_len		generic_NCR5380_dma_xfer_len
 #define NCR5380_dma_recv_setup		generic_NCR5380_pread
 #define NCR5380_dma_send_setup		generic_NCR5380_pwrite
-#define NCR5380_dma_residual		NCR5380_dma_residual_none
+#define NCR5380_dma_residual		generic_NCR5380_dma_residual
 
 #define NCR5380_intr generic_NCR5380_intr
 #define NCR5380_queue_command generic_NCR5380_queue_command

-- 

[toc] | [prev] | [next] | [standalone]

#1583883

On Friday 17 February 2017 23:38:12 Finn Thain wrote:
> On Thu, 16 Feb 2017, Ondrej Zary wrote:
> > On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
> > [...]
> >
> > > Are you trying to figure out which commands are going to disconnect
> > > during a transfer? This is really a function of the firmware in the
> > > target; there are no good heuristics AFAICT, so the PDMA algorithm has
> > > to be robust. mac_scsi has to cope with this too.
> > >
> > > Does the problem go away when you assign no IRQ? When instance->irq ==
> > > NO_IRQ, the core driver will inhibit disconnect privileges.
> >
> > Yes, it seems to run fine with IRQ/disconnect disabled. With IRQ
> > enabled, "dd if=/dev/sr0 of=anything" stops after a while.
>
> When you say "stops", do you mean an infinite loop in
> generic_NCR5380_pread or does the loop complete (which would presumably
> leave the target stuck in DATA IN phase, and a scsi command timeout would
> probably follow after 30 seconds...)

I've added timeouts to the possibly-infinite loops. It times out waiting for 
the host buffer to become ready. Then everything breaks.
Now I found why: pread() returns without waiting for the 53C80 registers to be 
ready. Adding the wait allows to continue in PIO mode with "tag#0 switching 
to slow handshake".

> > I get gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.
>
> You can use NCR5380_print() to get a decoded register dump.
>
> When I decode the above values I get,
>
> BASR   = 0x10 = BASR_IRQ
> MODE   = 0x0e = MR_ENABLE_EOP_INTR | MR_MONITOR_BSY | MR_DMA_MODE
> STATUS = 0x7c = SR_BSY | SR_REQ | SR_MSG | SR_CD | SR_IO
>
> Since BASR_PHASE_MATCH is not set, the interrupt is almost certainly a
> phase mismatch. The new phase is SR_MSG | SR_CD | SR_IO = PHASE_MSGIN,
> which shows that the target has given up on the DATA IN phase and is
> presumably trying to send a DISCONNECT message.

Looks you're right. The transfersize is 4096, i.e. 2 CD-ROM sectors. When the 
2nd sector is not ready in the drive internal buffer, the drive probably 
disconnects which breaks the fragile pdma transfer. When the transfersize is 
limited to 2048 bytes, the problem goes away.

The problem also went away with enabled interrupts because I had some debug 
printks added which were slowing down the transfer enough for the drive (SONY 
CDU-55S) to keep up and not disconnect. Got the same result by inserting 
udelay(100) after each 128 byte block trasnferred in pread().

> > When I enable interrupts during PDMA (like the removed UNSAFE macro
> > did), the problems go away. I see an IRQ after each pread call.
>
> You shouldn't need an interrupt, because NCR5380_dma_complete() gets
> called regardless. AFAICT, the only difference is the timing, which
> becomes less predictable. Calling spinlock_irq_restore() before the
> transfer seems to create a race condition between hostdata->dma_len store
> and load.
>
> I think that the pread call has not yet returned when the interrupt fires
> and NCR5380_dma_complete() is called. Hence hostdata->dma_len has not yet
> been initialized. So when NCR5380_dma_complete() is called by the
> interrupt handler, SCp.this_residual will not change at all because
> hostdata->dma_len is still zero.
>
> > (had to disable "no 53C80 gated irq after transfer" and "no end dma
> > signal" messages to reduce log spam)
>
> That may provide a quick way to detect the failed PDMA transfer, at least
> for testing purposes. There may be a more conclusive test for a partial
> transfer.
>
> We could to implement something like macscsi_dma_residual() to take care
> of a failed PDMA transfer. That way, the failure can be taken into account
> when NCR5380_dma_complete() is called at the end of the transfer.
>
> See patch below for example. It should confirm the theory above though it
> really needs some timeouts added (NCR5380_poll_politely()). And perhaps we
> could do something more clever than retry indefinitely, though we can
> still rely on the command timeout.

Thanks for idea

> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 6f9665d..75cfaf3 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -497,15 +497,17 @@ static inline int generic_NCR5380_pread(struct
> NCR5380_hostdata *hostdata, blocks--;
>  	}
>
> +	hostdata->pdma_residual = 0;
> +
>  	if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
> -		printk("53C400r: no 53C80 gated irq after transfer");
> +		hostdata->pdma_residual = hostdata->dma_len;
>
>  	/* wait for 53C80 registers to be available */
>  	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
>  		;
>
>  	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
> -		printk(KERN_ERR "53C400r: no end dma signal\n");
> +		hostdata->pdma_residual = hostdata->dma_len;
>
>  	return 0;
>  }
> @@ -576,12 +578,14 @@ static inline int generic_NCR5380_pwrite(struct
> NCR5380_hostdata *hostdata, /* FIXME - no timeout */
>  	}
>
> -	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER)) {
> -		printk(KERN_ERR "53C400w: no end dma signal\n");
> -	}
> +	hostdata->pdma_residual = 0;
> +
> +	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
> +		hostdata->pdma_residual = hostdata->dma_len;
>
>  	while (!(NCR5380_read(TARGET_COMMAND_REG) & TCR_LAST_BYTE_SENT))
>  		; 	// TIMEOUT
> +
>  	return 0;
>  }
>
> @@ -607,6 +611,11 @@ static int generic_NCR5380_dma_xfer_len(struct
> NCR5380_hostdata *hostdata, return transfersize;
>  }
>
> +static int generic_NCR5380_dma_residual(struct NCR5380_hostdata *hostdata)
> +{
> +	return hostdata->pdma_residual;
> +}
> +
>  /*
>   *	Include the NCR5380 core code that we build our driver around
>   */
> diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
> index 81b22d9..08c91b7 100644
> --- a/drivers/scsi/g_NCR5380.h
> +++ b/drivers/scsi/g_NCR5380.h
> @@ -26,7 +26,8 @@
>  	int c400_ctl_status; \
>  	int c400_blk_cnt; \
>  	int c400_host_buf; \
> -	int io_width;
> +	int io_width; \
> +	int pdma_residual;
>
>  #define NCR53C400_mem_base 0x3880
>  #define NCR53C400_host_buffer 0x3900
> @@ -35,7 +36,7 @@
>  #define NCR5380_dma_xfer_len		generic_NCR5380_dma_xfer_len
>  #define NCR5380_dma_recv_setup		generic_NCR5380_pread
>  #define NCR5380_dma_send_setup		generic_NCR5380_pwrite
> -#define NCR5380_dma_residual		NCR5380_dma_residual_none
> +#define NCR5380_dma_residual		generic_NCR5380_dma_residual
>
>  #define NCR5380_intr generic_NCR5380_intr
>  #define NCR5380_queue_command generic_NCR5380_queue_command


-- 
Ondrej Zary

[toc] | [prev] | [next] | [standalone]

#1584031 — Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

On Sat, 18 Feb 2017, Ondrej Zary wrote:

> On Friday 17 February 2017 23:38:12 Finn Thain wrote:
> > On Thu, 16 Feb 2017, Ondrej Zary wrote:
> > > On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
> > > [...]
> > >
> > > > Are you trying to figure out which commands are going to 
> > > > disconnect during a transfer? This is really a function of the 
> > > > firmware in the target; there are no good heuristics AFAICT, so 
> > > > the PDMA algorithm has to be robust. mac_scsi has to cope with 
> > > > this too.
> > > >
> > > > Does the problem go away when you assign no IRQ? When 
> > > > instance->irq == NO_IRQ, the core driver will inhibit disconnect 
> > > > privileges.
> > >
> > > Yes, it seems to run fine with IRQ/disconnect disabled. With IRQ 
> > > enabled, "dd if=/dev/sr0 of=anything" stops after a while.
> >
> > When you say "stops", do you mean an infinite loop in 
> > generic_NCR5380_pread or does the loop complete (which would 
> > presumably leave the target stuck in DATA IN phase, and a scsi command 
> > timeout would probably follow after 30 seconds...)
> 
> I've added timeouts to the possibly-infinite loops. It times out waiting 
> for the host buffer to become ready.

In mac_scsi you'll find that the PDMA loop exploits the 15ms poll_politely 
time limit to give the target device time to catch up. You might want to 
do something similar.

> Then everything breaks. Now I found why: pread() returns without waiting 
> for the 53C80 registers to be ready.

Ouch! You can't return control to the core driver when the 53C80 core is 
unavailable. That would need special handling: the core driver would have 
to fail the scsi command and reset the device (and bus), based on the 
result you return from NCR5380_dma_recv_setup/NCR5380_dma_send_setup.

> Adding the wait allows to continue in PIO mode with "tag#0 switching to 
> slow handshake".
> 

I don't think this is the code path you want. The target isn't really 
broken. But yes, we could use PIO as a slow workaround for fragile PDMA 
logic.

> > > I get gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.
> >
> > You can use NCR5380_print() to get a decoded register dump.
> >
> > When I decode the above values I get,
> >
> > BASR   = 0x10 = BASR_IRQ
> > MODE   = 0x0e = MR_ENABLE_EOP_INTR | MR_MONITOR_BSY | MR_DMA_MODE
> > STATUS = 0x7c = SR_BSY | SR_REQ | SR_MSG | SR_CD | SR_IO
> >
> > Since BASR_PHASE_MATCH is not set, the interrupt is almost certainly a 
> > phase mismatch. The new phase is SR_MSG | SR_CD | SR_IO = PHASE_MSGIN, 
> > which shows that the target has given up on the DATA IN phase and is 
> > presumably trying to send a DISCONNECT message.
> 
> Looks you're right. The transfersize is 4096, i.e. 2 CD-ROM sectors. 
> When the 2nd sector is not ready in the drive internal buffer, the drive 
> probably disconnects which breaks the fragile pdma transfer. When the 
> transfersize is limited to 2048 bytes, the problem goes away.
> 

OK.

> The problem also went away with enabled interrupts because I had some 
> debug printks added which were slowing down the transfer enough for the 
> drive (SONY CDU-55S) to keep up and not disconnect. Got the same result 
> by inserting udelay(100) after each 128 byte block trasnferred in 
> pread().
> 

Well, yeah, if you change the timing and omit to mention that, as well as 
changing the spinlock_irq_save/restore pairs, then it's going to be 
difficult for me to make sense of your results. Anyway, I'm glad that you 
got to the bottom of this mystery.

> > > When I enable interrupts during PDMA (like the removed UNSAFE macro 
> > > did), the problems go away. I see an IRQ after each pread call.
> >

-- 

[toc] | [prev] | [next] | [standalone]

#1584110

On Sunday 19 February 2017 00:27:55 Finn Thain wrote:
> On Sat, 18 Feb 2017, Ondrej Zary wrote:
> > On Friday 17 February 2017 23:38:12 Finn Thain wrote:
> > > On Thu, 16 Feb 2017, Ondrej Zary wrote:
> > > > On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
> > > > [...]
> > > >
> > > > > Are you trying to figure out which commands are going to
> > > > > disconnect during a transfer? This is really a function of the
> > > > > firmware in the target; there are no good heuristics AFAICT, so
> > > > > the PDMA algorithm has to be robust. mac_scsi has to cope with
> > > > > this too.
> > > > >
> > > > > Does the problem go away when you assign no IRQ? When
> > > > > instance->irq == NO_IRQ, the core driver will inhibit disconnect
> > > > > privileges.
> > > >
> > > > Yes, it seems to run fine with IRQ/disconnect disabled. With IRQ
> > > > enabled, "dd if=/dev/sr0 of=anything" stops after a while.
> > >
> > > When you say "stops", do you mean an infinite loop in
> > > generic_NCR5380_pread or does the loop complete (which would
> > > presumably leave the target stuck in DATA IN phase, and a scsi command
> > > timeout would probably follow after 30 seconds...)
> >
> > I've added timeouts to the possibly-infinite loops. It times out waiting
> > for the host buffer to become ready.
>
> In mac_scsi you'll find that the PDMA loop exploits the 15ms poll_politely
> time limit to give the target device time to catch up. You might want to
> do something similar.
>
> > Then everything breaks. Now I found why: pread() returns without waiting
> > for the 53C80 registers to be ready.
>
> Ouch! You can't return control to the core driver when the 53C80 core is
> unavailable. That would need special handling: the core driver would have
> to fail the scsi command and reset the device (and bus), based on the
> result you return from NCR5380_dma_recv_setup/NCR5380_dma_send_setup.
>
> > Adding the wait allows to continue in PIO mode with "tag#0 switching to
> > slow handshake".
>
> I don't think this is the code path you want. The target isn't really
> broken. But yes, we could use PIO as a slow workaround for fragile PDMA
> logic.

Yes, we don't want that.

> > > > I get gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.
> > >
> > > You can use NCR5380_print() to get a decoded register dump.
> > >
> > > When I decode the above values I get,
> > >
> > > BASR   = 0x10 = BASR_IRQ
> > > MODE   = 0x0e = MR_ENABLE_EOP_INTR | MR_MONITOR_BSY | MR_DMA_MODE
> > > STATUS = 0x7c = SR_BSY | SR_REQ | SR_MSG | SR_CD | SR_IO
> > >
> > > Since BASR_PHASE_MATCH is not set, the interrupt is almost certainly a
> > > phase mismatch. The new phase is SR_MSG | SR_CD | SR_IO = PHASE_MSGIN,
> > > which shows that the target has given up on the DATA IN phase and is
> > > presumably trying to send a DISCONNECT message.
> >
> > Looks you're right. The transfersize is 4096, i.e. 2 CD-ROM sectors.
> > When the 2nd sector is not ready in the drive internal buffer, the drive
> > probably disconnects which breaks the fragile pdma transfer. When the
> > transfersize is limited to 2048 bytes, the problem goes away.
>
> OK.
>
> > The problem also went away with enabled interrupts because I had some
> > debug printks added which were slowing down the transfer enough for the
> > drive (SONY CDU-55S) to keep up and not disconnect. Got the same result
> > by inserting udelay(100) after each 128 byte block trasnferred in
> > pread().
>
> Well, yeah, if you change the timing and omit to mention that, as well as
> changing the spinlock_irq_save/restore pairs, then it's going to be
> difficult for me to make sense of your results. Anyway, I'm glad that you
> got to the bottom of this mystery.
>
> > > > When I enable interrupts during PDMA (like the removed UNSAFE macro
> > > > did), the problems go away. I see an IRQ after each pread call.

These two patches are enough to make PDMA work with CD-ROM drives on
53C400(A), with IRQ enabled or not. They even improve transfer rates with
HDDs because of bigger block size.

But DTC436 breaks with CDU-55S, immediately after modprobe.
1) Seems that IRQ timing is slightly different so I rewrote the wait for
the host buffer to include check for CSR_GATED_53C80_IRQ. This allows some
data to be transferred but
2) DTC436 likes to hang (return only 0xff from all registers - like it's
not even present on the bus) on repeating CSR register reads and maybe some
other actions too. This problem already appeared before in pwrite() where
I added the udelay(4) workaround. Now I added udelay(100) to the waits but
it still crashes sometimes (randomly after tenths or hundreds of MB).

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 6f9665d..9d0cfd4 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -585,26 +585,20 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 	return 0;
 }
 
+#define G_NCR5380_DMA_MAX_SIZE	32768
 static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
                                         struct scsi_cmnd *cmd)
 {
-	int transfersize = cmd->transfersize;
+	int transfersize = cmd->SCp.this_residual;
 
 	if (hostdata->flags & FLAG_NO_PSEUDO_DMA)
 		return 0;
 
-	/* Limit transfers to 32K, for xx400 & xx406
-	 * pseudoDMA that transfers in 128 bytes blocks.
-	 */
-	if (transfersize > 32 * 1024 && cmd->SCp.this_residual &&
-	    !(cmd->SCp.this_residual % transfersize))
-		transfersize = 32 * 1024;
-
 	/* 53C400 datasheet: non-modulo-128-byte transfers should use PIO */
 	if (transfersize % 128)
 		transfersize = 0;
 
-	return transfersize;
+	return min(transfersize, G_NCR5380_DMA_MAX_SIZE);
 }
 
 /*

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 9d0cfd4..797115e 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -453,15 +453,15 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
 	int blocks = len / 128;
 	int start = 0;
 
+	hostdata->pdma_residual = 0;
+
 	NCR5380_write(hostdata->c400_ctl_status, CSR_BASE | CSR_TRANS_DIR);
 	NCR5380_write(hostdata->c400_blk_cnt, blocks);
 	while (1) {
 		if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
 			break;
-		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ) {
-			printk(KERN_ERR "53C400r: Got 53C80_IRQ start=%d, blocks=%d\n", start, blocks);
-			return -1;
-		}
+		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)
+			goto out_wait;
 		while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY)
 			; /* FIXME - no timeout */
 
@@ -499,13 +499,13 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
 
 	if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
 		printk("53C400r: no 53C80 gated irq after transfer");
-
+out_wait:
 	/* wait for 53C80 registers to be available */
 	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
 		;
 
 	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
-		printk(KERN_ERR "53C400r: no end dma signal\n");
+		hostdata->pdma_residual = blocks * 128;
 		
 	return 0;
 }
@@ -526,13 +526,13 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 	int blocks = len / 128;
 	int start = 0;
 
+	hostdata->pdma_residual = 0;
+
 	NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
 	NCR5380_write(hostdata->c400_blk_cnt, blocks);
 	while (1) {
-		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ) {
-			printk(KERN_ERR "53C400w: Got 53C80_IRQ start=%d, blocks=%d\n", start, blocks);
-			return -1;
-		}
+		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)
+			goto out_wait;
 
 		if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
 			break;
@@ -569,16 +569,15 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 		start += 128;
 		blocks--;
 	}
-
+out_wait:
 	/* wait for 53C80 registers to be available */
 	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG)) {
 		udelay(4); /* DTC436 chip hangs without this */
 		/* FIXME - no timeout */
 	}
 
-	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER)) {
-		printk(KERN_ERR "53C400w: no end dma signal\n");
-	}
+	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
+		hostdata->pdma_residual = blocks * 128;
 
 	while (!(NCR5380_read(TARGET_COMMAND_REG) & TCR_LAST_BYTE_SENT))
 		; 	// TIMEOUT
@@ -601,6 +600,11 @@ static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
 	return min(transfersize, G_NCR5380_DMA_MAX_SIZE);
 }
 
+static int generic_NCR5380_dma_residual(struct NCR5380_hostdata *hostdata)
+{
+	return hostdata->pdma_residual;
+}
+
 /*
  *	Include the NCR5380 core code that we build our driver around	
  */
diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
index 81b22d9..08c91b7 100644
--- a/drivers/scsi/g_NCR5380.h
+++ b/drivers/scsi/g_NCR5380.h
@@ -26,7 +26,8 @@
 	int c400_ctl_status; \
 	int c400_blk_cnt; \
 	int c400_host_buf; \
-	int io_width;
+	int io_width; \
+	int pdma_residual;
 
 #define NCR53C400_mem_base 0x3880
 #define NCR53C400_host_buffer 0x3900
@@ -35,7 +36,7 @@
 #define NCR5380_dma_xfer_len		generic_NCR5380_dma_xfer_len
 #define NCR5380_dma_recv_setup		generic_NCR5380_pread
 #define NCR5380_dma_send_setup		generic_NCR5380_pwrite
-#define NCR5380_dma_residual		NCR5380_dma_residual_none
+#define NCR5380_dma_residual		generic_NCR5380_dma_residual
 
 #define NCR5380_intr generic_NCR5380_intr
 #define NCR5380_queue_command generic_NCR5380_queue_command


-- 
Ondrej Zary

[toc] | [prev] | [next] | [standalone]

#1584285 — Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

On Sun, 19 Feb 2017, Ondrej Zary wrote:

> 
> These two patches are enough to make PDMA work with CD-ROM drives on 
> 53C400(A), with IRQ enabled or not. They even improve transfer rates 
> with HDDs because of bigger block size.
> 

Nice! This is a great improvement.

> But DTC436 breaks with CDU-55S, immediately after modprobe.
> 1) Seems that IRQ timing is slightly different so I rewrote the wait for
> the host buffer to include check for CSR_GATED_53C80_IRQ. This allows some
> data to be transferred but
> 2) DTC436 likes to hang (return only 0xff from all registers - like it's
> not even present on the bus) on repeating CSR register reads and maybe some
> other actions too. This problem already appeared before in pwrite() where
> I added the udelay(4) workaround. Now I added udelay(100) to the waits but
> it still crashes sometimes (randomly after tenths or hundreds of MB).
> 

That is not a regression, right? If there are no regressions, I'd like to 
merge this patch (with some minor tweaks, rebased on the latest driver).

The problem with these Domex chips is that the manufacturer doesn't 
respond to repeated requests for information.

If you want to pursue this bug, when working on undocumented proprietary 
hardware from manufacturers like this one, what I recommend is to look for 
clues in the code for alternative implementations (NetBSD etc) and try 
wiggling parameters until the behaviour changes (e.g. reduce the transfer 
size to 128 bytes, introduce delays at key places. Probably the same sort 
of thing you already tried.) Snooping the device register accessess made 
by a DOS/Windows driver is another approach.

> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 6f9665d..9d0cfd4 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -585,26 +585,20 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
>  	return 0;
>  }
>  
> +#define G_NCR5380_DMA_MAX_SIZE	32768

Please put this at the top with the other definitions. (As you will 
recall, the macro definitions were moved around in the latest driver.)

>  static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
>                                          struct scsi_cmnd *cmd)
>  {
> -	int transfersize = cmd->transfersize;
> +	int transfersize = cmd->SCp.this_residual;
>  
>  	if (hostdata->flags & FLAG_NO_PSEUDO_DMA)
>  		return 0;
>  
> -	/* Limit transfers to 32K, for xx400 & xx406
> -	 * pseudoDMA that transfers in 128 bytes blocks.
> -	 */
> -	if (transfersize > 32 * 1024 && cmd->SCp.this_residual &&
> -	    !(cmd->SCp.this_residual % transfersize))
> -		transfersize = 32 * 1024;
> -
>  	/* 53C400 datasheet: non-modulo-128-byte transfers should use PIO */
>  	if (transfersize % 128)
>  		transfersize = 0;
>  
> -	return transfersize;
> +	return min(transfersize, G_NCR5380_DMA_MAX_SIZE);
>  }
>  
>  /*
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 9d0cfd4..797115e 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -453,15 +453,15 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
>  	int blocks = len / 128;
>  	int start = 0;
>  
> +	hostdata->pdma_residual = 0;
> +
>  	NCR5380_write(hostdata->c400_ctl_status, CSR_BASE | CSR_TRANS_DIR);
>  	NCR5380_write(hostdata->c400_blk_cnt, blocks);
>  	while (1) {
>  		if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
>  			break;
> -		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ) {
> -			printk(KERN_ERR "53C400r: Got 53C80_IRQ start=%d, blocks=%d\n", start, blocks);
> -			return -1;
> -		}
> +		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)
> +			goto out_wait;

This algorithm basically says that, if the IRQ is late (or missing) we 
will go through the main loop, information transfer and DMA setup steps 
again. That's fine, but it means that we have to exit the pread/pwrite 
routines with the chip in a suitable state for back-to-back PDMA 
transfers.

NCR5380_dma_complete() takes care of things for the 53C80 core, but maybe 
the DTC436 logic might need a bit more work (?) For example, if we exit 
with a partially filled buffer, will the buffer switching logic break? The 
53C400 datasheet mentions a "Resume Transfer Register" which makes me a 
bit suspicious about chip state after a DISCONNECT... just a thought.

>  		while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY)
>  			; /* FIXME - no timeout */
>  
> @@ -499,13 +499,13 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
>  
>  	if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
>  		printk("53C400r: no 53C80 gated irq after transfer");
> -
> +out_wait:
>  	/* wait for 53C80 registers to be available */
>  	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
>  		;
>  
>  	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
> -		printk(KERN_ERR "53C400r: no end dma signal\n");
> +		hostdata->pdma_residual = blocks * 128;


Can we unconditionally assign pdma_residual? For example,

 	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
-		printk(KERN_ERR "53C400r: no end dma signal\n");
+		pr_err("%s: No end dma signal (%d/%d)\n", __func__, start, len);

+	hostdata->pdma_residual = len - start;
 
 	return 0;
 }

Since the new 'goto' can cause the while loop to terminate early, it is 
easier to reason about driver behaviour if this assignment is not 
conditional on the End of DMA signal.

> @@ -526,13 +526,13 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
>  	int blocks = len / 128;
>  	int start = 0;
>  
> +	hostdata->pdma_residual = 0;
> +
>  	NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
>  	NCR5380_write(hostdata->c400_blk_cnt, blocks);
>  	while (1) {
> -		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ) {
> -			printk(KERN_ERR "53C400w: Got 53C80_IRQ start=%d, blocks=%d\n", start, blocks);
> -			return -1;
> -		}
> +		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)
> +			goto out_wait;
>  
>  		if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
>  			break;
> @@ -569,16 +569,15 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
>  		start += 128;
>  		blocks--;
>  	}
> -
> +out_wait:
>  	/* wait for 53C80 registers to be available */
>  	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG)) {
>  		udelay(4); /* DTC436 chip hangs without this */
>  		/* FIXME - no timeout */
>  	}
>  
> -	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER)) {
> -		printk(KERN_ERR "53C400w: no end dma signal\n");
> -	}
> +	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
> +		hostdata->pdma_residual = blocks * 128;
>  

Same here.


>  	while (!(NCR5380_read(TARGET_COMMAND_REG) & TCR_LAST_BYTE_SENT))
>  		; 	// TIMEOUT
> @@ -601,6 +600,11 @@ static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
>  	return min(transfersize, G_NCR5380_DMA_MAX_SIZE);
>  }
>  
> +static int generic_NCR5380_dma_residual(struct NCR5380_hostdata *hostdata)
> +{
> +	return hostdata->pdma_residual;
> +}
> +
>  /*
>   *	Include the NCR5380 core code that we build our driver around	
>   */
> diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
> index 81b22d9..08c91b7 100644
> --- a/drivers/scsi/g_NCR5380.h
> +++ b/drivers/scsi/g_NCR5380.h
> @@ -26,7 +26,8 @@
>  	int c400_ctl_status; \
>  	int c400_blk_cnt; \
>  	int c400_host_buf; \
> -	int io_width;
> +	int io_width; \
> +	int pdma_residual;
>  
>  #define NCR53C400_mem_base 0x3880
>  #define NCR53C400_host_buffer 0x3900
> @@ -35,7 +36,7 @@
>  #define NCR5380_dma_xfer_len		generic_NCR5380_dma_xfer_len
>  #define NCR5380_dma_recv_setup		generic_NCR5380_pread
>  #define NCR5380_dma_send_setup		generic_NCR5380_pwrite
> -#define NCR5380_dma_residual		NCR5380_dma_residual_none
> +#define NCR5380_dma_residual		generic_NCR5380_dma_residual
>  
>  #define NCR5380_intr generic_NCR5380_intr
>  #define NCR5380_queue_command generic_NCR5380_queue_command
> 
> 
> 

Thanks.

-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web