| Message ID | c56deeb735545c7942607a93f017bb536f581ae5.1559438652.git.fthain@telegraphics.com.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
Hi Finn, On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au> wrote: > A system bus error during a PDMA transfer can mess up the calculation of > the transfer residual (the PDMA handshaking hardware lacks a byte > counter). This results in data corruption. > > The algorithm in this patch anticipates a bus error by starting each > transfer with a MOVE.B instruction. If a bus error is caught the transfer > will be retried. If a bus error is caught later in the transfer (for a > MOVE.W instruction) the transfer gets failed and subsequent requests for > that target will use PIO instead of PDMA. > > This avoids the "!REQ and !ACK" error so the severity level of that > message is reduced to KERN_DEBUG. > > Cc: Michael Schmitz <schmitzmic@gmail.com> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: stable@vger.kernel.org # v4.14+ > Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation") > Reported-by: Chris Jones <chris@martin-jones.com> > Tested-by: Stan Johnson <userm57@yahoo.com> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au> Thanks for your patch! > --- > arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++ > drivers/scsi/mac_scsi.c | 201 ++++++++----------------------- Why have you moved the PDMA implementation to a header file under arch/m68k/? Do you intend to reuse it by other drivers? If not, please keep it in the driver, so (a) you don't need an ack from me ;-), and (b) your change may be easier to review. Thanks! Gr{oetje,eeting}s, Geert
On Sun, 2 Jun 2019, Geert Uytterhoeven wrote: > Hi Finn, > > On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au> > wrote: > > A system bus error during a PDMA transfer can mess up the calculation > > of the transfer residual (the PDMA handshaking hardware lacks a byte > > counter). This results in data corruption. > > > > The algorithm in this patch anticipates a bus error by starting each > > transfer with a MOVE.B instruction. If a bus error is caught the > > transfer will be retried. If a bus error is caught later in the > > transfer (for a MOVE.W instruction) the transfer gets failed and > > subsequent requests for that target will use PIO instead of PDMA. > > > > This avoids the "!REQ and !ACK" error so the severity level of that > > message is reduced to KERN_DEBUG. > > > > Cc: Michael Schmitz <schmitzmic@gmail.com> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Cc: stable@vger.kernel.org # v4.14+ > > Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation") > > Reported-by: Chris Jones <chris@martin-jones.com> > > Tested-by: Stan Johnson <userm57@yahoo.com> > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au> > > Thanks for your patch! > > > --- > > arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++ > > drivers/scsi/mac_scsi.c | 201 ++++++++----------------------- > > Why have you moved the PDMA implementation to a header file under > arch/m68k/? Do you intend to reuse it by other drivers? > There are a couple of reasons: the mac_esp driver also uses PDMA and the NuBus PowerMac port also uses mac_scsi.c. OTOH, the NuBus PowerMac port is still out-of-tree, and it is unclear whether the mac_esp driver will ever benefit from this code. > If not, please keep it in the driver, so (a) you don't need an ack from > me ;-), and (b) your change may be easier to review. > I take your wink to mean that you don't want to ask the SCSI maintainers to review m68k asm. Putting aside the code review process for a moment, do you have an opinion on the most logical way to organise this sort of code, from the point-of-view of maintainability, re-usability, readability etc.? Thanks.
Hi Finn, On Mon, Jun 3, 2019 at 1:32 AM Finn Thain <fthain@telegraphics.com.au> wrote: > On Sun, 2 Jun 2019, Geert Uytterhoeven wrote: > > On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au> > > wrote: > > > A system bus error during a PDMA transfer can mess up the calculation > > > of the transfer residual (the PDMA handshaking hardware lacks a byte > > > counter). This results in data corruption. > > > > > > The algorithm in this patch anticipates a bus error by starting each > > > transfer with a MOVE.B instruction. If a bus error is caught the > > > transfer will be retried. If a bus error is caught later in the > > > transfer (for a MOVE.W instruction) the transfer gets failed and > > > subsequent requests for that target will use PIO instead of PDMA. > > > > > > This avoids the "!REQ and !ACK" error so the severity level of that > > > message is reduced to KERN_DEBUG. > > > > > > Cc: Michael Schmitz <schmitzmic@gmail.com> > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > Cc: stable@vger.kernel.org # v4.14+ > > > Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation") > > > Reported-by: Chris Jones <chris@martin-jones.com> > > > Tested-by: Stan Johnson <userm57@yahoo.com> > > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au> > > > > Thanks for your patch! > > > > > --- > > > arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++ > > > drivers/scsi/mac_scsi.c | 201 ++++++++----------------------- > > > > Why have you moved the PDMA implementation to a header file under > > arch/m68k/? Do you intend to reuse it by other drivers? > > > > There are a couple of reasons: the mac_esp driver also uses PDMA and the > NuBus PowerMac port also uses mac_scsi.c. OTOH, the NuBus PowerMac port is > still out-of-tree, and it is unclear whether the mac_esp driver will ever > benefit from this code. So you do have future sharing in mind... > > If not, please keep it in the driver, so (a) you don't need an ack from > > me ;-), and (b) your change may be easier to review. > > I take your wink to mean that you don't want to ask the SCSI maintainers > to review m68k asm. Putting aside the code review process for a moment, do I meant that apart from the code containing m68k assembler source, it is not related to arch/m68k/, and thus belongs to the driver. There are several other drivers that contain pieces of assembler code. > you have an opinion on the most logical way to organise this sort of code, > from the point-of-view of maintainability, re-usability, readability etc.? If the code is used by multiple SCSI drivers, you can move it to a header file under drivers/scsi/. If the code is shared by drivers belonging to multiple subsystems, you can move it to a header file under include/linux/. Anyone who has a better solution? Thanks! Gr{oetje,eeting}s, Geert
On Mon, 3 Jun 2019, Geert Uytterhoeven wrote: > Hi Finn, > > On Mon, Jun 3, 2019 at 1:32 AM Finn Thain <fthain@telegraphics.com.au> wrote: > > On Sun, 2 Jun 2019, Geert Uytterhoeven wrote: > > > On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au> > > > wrote: > > > > A system bus error during a PDMA transfer can mess up the calculation > > > > of the transfer residual (the PDMA handshaking hardware lacks a byte > > > > counter). This results in data corruption. > > > > > > > > The algorithm in this patch anticipates a bus error by starting each > > > > transfer with a MOVE.B instruction. If a bus error is caught the > > > > transfer will be retried. If a bus error is caught later in the > > > > transfer (for a MOVE.W instruction) the transfer gets failed and > > > > subsequent requests for that target will use PIO instead of PDMA. > > > > > > > > This avoids the "!REQ and !ACK" error so the severity level of that > > > > message is reduced to KERN_DEBUG. > > > > > > > > Cc: Michael Schmitz <schmitzmic@gmail.com> > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Cc: stable@vger.kernel.org # v4.14+ > > > > Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation") > > > > Reported-by: Chris Jones <chris@martin-jones.com> > > > > Tested-by: Stan Johnson <userm57@yahoo.com> > > > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au> > > > > > > Thanks for your patch! > > > > > > > --- > > > > arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++ > > > > drivers/scsi/mac_scsi.c | 201 ++++++++----------------------- > > > > > > Why have you moved the PDMA implementation to a header file under > > > arch/m68k/? Do you intend to reuse it by other drivers? > > > > > > > There are a couple of reasons: the mac_esp driver also uses PDMA and the > > NuBus PowerMac port also uses mac_scsi.c. OTOH, the NuBus PowerMac port is > > still out-of-tree, and it is unclear whether the mac_esp driver will ever > > benefit from this code. > > So you do have future sharing in mind... > > > > If not, please keep it in the driver, so (a) you don't need an ack from > > > me ;-), and (b) your change may be easier to review. > > > > I take your wink to mean that you don't want to ask the SCSI maintainers > > to review m68k asm. Putting aside the code review process for a moment, do > > I meant that apart from the code containing m68k assembler source, it is > not related to arch/m68k/, and thus belongs to the driver. That criterion seems insufficient. It could describe most of arch/m68k/mac (which has headers in arch/m68k/include). > There are several other drivers that contain pieces of assembler code. > Does any driver contain assembler code for multiple architectures? I was trying to avoid that -- though admittedly I don't yet have actual code for the PDMA implementation for mac_scsi for Nubus PowerMacs. However, the existence of that out-of-tree port suggests to me that arch/powerpc/include/mac_scsi.h and arch/m68k/include/mac_scsi.h would be an appropriate layout. But if there's no clear policy then perhaps we should ignore the whole question until the driver code actually becomes shared code. I don't mind re-working the patch to combine the two files.
Hi Finn, On Mon, Jun 3, 2019 at 9:40 AM Finn Thain <fthain@telegraphics.com.au> wrote: > On Mon, 3 Jun 2019, Geert Uytterhoeven wrote: > > On Mon, Jun 3, 2019 at 1:32 AM Finn Thain <fthain@telegraphics.com.au> wrote: > > > On Sun, 2 Jun 2019, Geert Uytterhoeven wrote: > > > > On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au> > > > > wrote: > > > > > A system bus error during a PDMA transfer can mess up the calculation > > > > > of the transfer residual (the PDMA handshaking hardware lacks a byte > > > > > counter). This results in data corruption. > > > > > > > > > > The algorithm in this patch anticipates a bus error by starting each > > > > > transfer with a MOVE.B instruction. If a bus error is caught the > > > > > transfer will be retried. If a bus error is caught later in the > > > > > transfer (for a MOVE.W instruction) the transfer gets failed and > > > > > subsequent requests for that target will use PIO instead of PDMA. > > > > > > > > > > This avoids the "!REQ and !ACK" error so the severity level of that > > > > > message is reduced to KERN_DEBUG. > > > > > > > > > > Cc: Michael Schmitz <schmitzmic@gmail.com> > > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > Cc: stable@vger.kernel.org # v4.14+ > > > > > Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation") > > > > > Reported-by: Chris Jones <chris@martin-jones.com> > > > > > Tested-by: Stan Johnson <userm57@yahoo.com> > > > > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au> > > > > > > > > Thanks for your patch! > > > > > > > > > --- > > > > > arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++ > > > > > drivers/scsi/mac_scsi.c | 201 ++++++++----------------------- > > > > > > > > Why have you moved the PDMA implementation to a header file under > > > > arch/m68k/? Do you intend to reuse it by other drivers? > > > > > > > > > > There are a couple of reasons: the mac_esp driver also uses PDMA and the > > > NuBus PowerMac port also uses mac_scsi.c. OTOH, the NuBus PowerMac port is > > > still out-of-tree, and it is unclear whether the mac_esp driver will ever > > > benefit from this code. > > > > So you do have future sharing in mind... > > > > > > If not, please keep it in the driver, so (a) you don't need an ack from > > > > me ;-), and (b) your change may be easier to review. > > > > > > I take your wink to mean that you don't want to ask the SCSI maintainers > > > to review m68k asm. Putting aside the code review process for a moment, do > > > > I meant that apart from the code containing m68k assembler source, it is > > not related to arch/m68k/, and thus belongs to the driver. > > That criterion seems insufficient. It could describe most of arch/m68k/mac > (which has headers in arch/m68k/include). > > > There are several other drivers that contain pieces of assembler code. > > > > Does any driver contain assembler code for multiple architectures? I was > trying to avoid that -- though admittedly I don't yet have actual code for > the PDMA implementation for mac_scsi for Nubus PowerMacs. > > However, the existence of that out-of-tree port suggests to me that > arch/powerpc/include/mac_scsi.h and arch/m68k/include/mac_scsi.h would be > an appropriate layout. > > But if there's no clear policy then perhaps we should ignore the whole > question until the driver code actually becomes shared code. I don't mind > re-working the patch to combine the two files. Yep, can be handled when the need arises. Gr{oetje,eeting}s, Geert
Hi Finn, On 3/06/19 7:40 PM, Finn Thain wrote: > >> There are several other drivers that contain pieces of assembler code. >> > Does any driver contain assembler code for multiple architectures? I was > trying to avoid that -- though admittedly I don't yet have actual code for > the PDMA implementation for mac_scsi for Nubus PowerMacs. > I've seen that once, for one of the ESP drivers that were supported on both m68k and ppc (APUS, PPC upgrade to Amiga computers). But that driver was removed long ago (after 2.6?). In that case, the assembly file did reside in drivers/scsi/. That still appears to be current practice (see drivers/scsi/arm/acornscsi-io.S). Cheers, Michael
On Tue, 4 Jun 2019, Michael Schmitz wrote: > Hi Finn, > > On 3/06/19 7:40 PM, Finn Thain wrote: > > > > > There are several other drivers that contain pieces of assembler code. > > > > > Does any driver contain assembler code for multiple architectures? I was > > trying to avoid that -- though admittedly I don't yet have actual code for > > the PDMA implementation for mac_scsi for Nubus PowerMacs. > > > I've seen that once, for one of the ESP drivers that were supported on both > m68k and ppc (APUS, PPC upgrade to Amiga computers). But that driver was > removed long ago (after 2.6?). > > In that case, the assembly file did reside in drivers/scsi/. That still > appears to be current practice (see drivers/scsi/arm/acornscsi-io.S). > The presence of that file would be an argument for adding drivers/scsi/m68k/. This seems to be begging the question. Since there's no clear policy, I'll combine the two files and avoid the question for now.
diff --git a/arch/m68k/include/asm/mac_pdma.h b/arch/m68k/include/asm/mac_pdma.h new file mode 100644 index 000000000000..44e2fb6d4b1c --- /dev/null +++ b/arch/m68k/include/asm/mac_pdma.h @@ -0,0 +1,179 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ +/* Copyright (C) 2019 Finn Thain */ + +#ifndef _ASM_MAC_PDMA_H_ +#define _ASM_MAC_PDMA_H_ + +#include <linux/delay.h> + +/* + * According to "Inside Macintosh: Devices", Mac OS requires disk drivers to + * specify the number of bytes between the delays expected from a SCSI target. + * This allows the operating system to "prevent bus errors when a target fails + * to deliver the next byte within the processor bus error timeout period." + * Linux SCSI drivers lack knowledge of the timing behaviour of SCSI targets + * so bus errors are unavoidable. + * + * If a MOVE.B instruction faults, we assume that zero bytes were transferred + * and simply retry. That assumption probably depends on target behaviour but + * seems to hold up okay. The NOP provides synchronization: without it the + * fault can sometimes occur after the program counter has moved past the + * offending instruction. Post-increment addressing can't be used. + */ + +#define MOVE_BYTE(operands) \ + asm volatile ( \ + "1: moveb " operands " \n" \ + "11: nop \n" \ + " addq #1,%0 \n" \ + " subq #1,%1 \n" \ + "40: \n" \ + " \n" \ + ".section .fixup,\"ax\" \n" \ + ".even \n" \ + "90: movel #1, %2 \n" \ + " jra 40b \n" \ + ".previous \n" \ + " \n" \ + ".section __ex_table,\"a\" \n" \ + ".align 4 \n" \ + ".long 1b,90b \n" \ + ".long 11b,90b \n" \ + ".previous \n" \ + : "+a" (addr), "+r" (n), "+r" (result) : "a" (io)) + +/* + * If a MOVE.W (or MOVE.L) instruction faults, it cannot be retried because + * the residual byte count would be uncertain. In that situation the MOVE_WORD + * macro clears n in the fixup section to abort the transfer. + */ + +#define MOVE_WORD(operands) \ + asm volatile ( \ + "1: movew " operands " \n" \ + "11: nop \n" \ + " subq #2,%1 \n" \ + "40: \n" \ + " \n" \ + ".section .fixup,\"ax\" \n" \ + ".even \n" \ + "90: movel #0, %1 \n" \ + " movel #2, %2 \n" \ + " jra 40b \n" \ + ".previous \n" \ + " \n" \ + ".section __ex_table,\"a\" \n" \ + ".align 4 \n" \ + ".long 1b,90b \n" \ + ".long 11b,90b \n" \ + ".previous \n" \ + : "+a" (addr), "+r" (n), "+r" (result) : "a" (io)) + +#define MOVE_16_WORDS(operands) \ + asm volatile ( \ + "1: movew " operands " \n" \ + "2: movew " operands " \n" \ + "3: movew " operands " \n" \ + "4: movew " operands " \n" \ + "5: movew " operands " \n" \ + "6: movew " operands " \n" \ + "7: movew " operands " \n" \ + "8: movew " operands " \n" \ + "9: movew " operands " \n" \ + "10: movew " operands " \n" \ + "11: movew " operands " \n" \ + "12: movew " operands " \n" \ + "13: movew " operands " \n" \ + "14: movew " operands " \n" \ + "15: movew " operands " \n" \ + "16: movew " operands " \n" \ + "17: nop \n" \ + " subl #32,%1 \n" \ + "40: \n" \ + " \n" \ + ".section .fixup,\"ax\" \n" \ + ".even \n" \ + "90: movel #0, %1 \n" \ + " movel #2, %2 \n" \ + " jra 40b \n" \ + ".previous \n" \ + " \n" \ + ".section __ex_table,\"a\" \n" \ + ".align 4 \n" \ + ".long 1b,90b \n" \ + ".long 2b,90b \n" \ + ".long 3b,90b \n" \ + ".long 4b,90b \n" \ + ".long 5b,90b \n" \ + ".long 6b,90b \n" \ + ".long 7b,90b \n" \ + ".long 8b,90b \n" \ + ".long 9b,90b \n" \ + ".long 10b,90b \n" \ + ".long 11b,90b \n" \ + ".long 12b,90b \n" \ + ".long 13b,90b \n" \ + ".long 14b,90b \n" \ + ".long 15b,90b \n" \ + ".long 16b,90b \n" \ + ".long 17b,90b \n" \ + ".previous \n" \ + : "+a" (addr), "+r" (n), "+r" (result) : "a" (io)) + +#define MAC_PDMA_DELAY 32 + +static inline int mac_pdma_recv(void __iomem *io, unsigned char *start, int n) +{ + unsigned char *addr = start; + int result = 0; + + if (n >= 1) { + MOVE_BYTE("%3@,%0@"); + if (result) + goto out; + } + if (n >= 1 && ((unsigned long)addr & 1)) { + MOVE_BYTE("%3@,%0@"); + if (result) + goto out; + } + while (n >= 32) + MOVE_16_WORDS("%3@,%0@+"); + while (n >= 2) + MOVE_WORD("%3@,%0@+"); + if (result) + return start - addr; /* Negated to indicate uncertain length */ + if (n == 1) + MOVE_BYTE("%3@,%0@"); +out: + return addr - start; +} + +static inline int mac_pdma_send(unsigned char *start, void __iomem *io, int n) +{ + unsigned char *addr = start; + int result = 0; + + if (n >= 1) { + MOVE_BYTE("%0@,%3@"); + if (result) + goto out; + } + if (n >= 1 && ((unsigned long)addr & 1)) { + MOVE_BYTE("%0@,%3@"); + if (result) + goto out; + } + while (n >= 32) + MOVE_16_WORDS("%0@+,%3@"); + while (n >= 2) + MOVE_WORD("%0@+,%3@"); + if (result) + return start - addr; /* Negated to indicate uncertain length */ + if (n == 1) + MOVE_BYTE("%0@,%3@"); +out: + return addr - start; +} + +#endif /* _ASM_MAC_PDMA_H_ */ diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c index ba1afcaadae8..e83b47a7e4b5 100644 --- a/drivers/scsi/mac_scsi.c +++ b/drivers/scsi/mac_scsi.c @@ -21,6 +21,7 @@ #include <asm/hwtest.h> #include <asm/io.h> +#include <asm/mac_pdma.h> #include <asm/macints.h> #include <asm/setup.h> @@ -89,101 +90,47 @@ static int __init mac_scsi_setup(char *str) __setup("mac5380=", mac_scsi_setup); #endif /* !MODULE */ -/* Pseudo DMA asm originally by Ove Edlund */ - -#define CP_IO_TO_MEM(s,d,n) \ -__asm__ __volatile__ \ - (" cmp.w #4,%2\n" \ - " bls 8f\n" \ - " move.w %1,%%d0\n" \ - " neg.b %%d0\n" \ - " and.w #3,%%d0\n" \ - " sub.w %%d0,%2\n" \ - " bra 2f\n" \ - " 1: move.b (%0),(%1)+\n" \ - " 2: dbf %%d0,1b\n" \ - " move.w %2,%%d0\n" \ - " lsr.w #5,%%d0\n" \ - " bra 4f\n" \ - " 3: move.l (%0),(%1)+\n" \ - "31: move.l (%0),(%1)+\n" \ - "32: move.l (%0),(%1)+\n" \ - "33: move.l (%0),(%1)+\n" \ - "34: move.l (%0),(%1)+\n" \ - "35: move.l (%0),(%1)+\n" \ - "36: move.l (%0),(%1)+\n" \ - "37: move.l (%0),(%1)+\n" \ - " 4: dbf %%d0,3b\n" \ - " move.w %2,%%d0\n" \ - " lsr.w #2,%%d0\n" \ - " and.w #7,%%d0\n" \ - " bra 6f\n" \ - " 5: move.l (%0),(%1)+\n" \ - " 6: dbf %%d0,5b\n" \ - " and.w #3,%2\n" \ - " bra 8f\n" \ - " 7: move.b (%0),(%1)+\n" \ - " 8: dbf %2,7b\n" \ - " moveq.l #0, %2\n" \ - " 9: \n" \ - ".section .fixup,\"ax\"\n" \ - " .even\n" \ - "91: moveq.l #1, %2\n" \ - " jra 9b\n" \ - "94: moveq.l #4, %2\n" \ - " jra 9b\n" \ - ".previous\n" \ - ".section __ex_table,\"a\"\n" \ - " .align 4\n" \ - " .long 1b,91b\n" \ - " .long 3b,94b\n" \ - " .long 31b,94b\n" \ - " .long 32b,94b\n" \ - " .long 33b,94b\n" \ - " .long 34b,94b\n" \ - " .long 35b,94b\n" \ - " .long 36b,94b\n" \ - " .long 37b,94b\n" \ - " .long 5b,94b\n" \ - " .long 7b,91b\n" \ - ".previous" \ - : "=a"(s), "=a"(d), "=d"(n) \ - : "0"(s), "1"(d), "2"(n) \ - : "d0") - static inline int macscsi_pread(struct NCR5380_hostdata *hostdata, unsigned char *dst, int len) { u8 __iomem *s = hostdata->pdma_io + (INPUT_DATA_REG << 4); unsigned char *d = dst; - int n = len; - int transferred; + + hostdata->pdma_residual = len; while (!NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG, BASR_DRQ | BASR_PHASE_MATCH, BASR_DRQ | BASR_PHASE_MATCH, HZ / 64)) { - CP_IO_TO_MEM(s, d, n); + int bytes; + + bytes = mac_pdma_recv(s, d, min(hostdata->pdma_residual, 512)); - transferred = d - dst - n; - hostdata->pdma_residual = len - transferred; + if (bytes > 0) { + d += bytes; + hostdata->pdma_residual -= bytes; + } - /* No bus error. */ - if (n == 0) + if (hostdata->pdma_residual == 0) return 0; - /* Target changed phase early? */ if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ, - BUS_AND_STATUS_REG, BASR_ACK, BASR_ACK, HZ / 64) < 0) - scmd_printk(KERN_ERR, hostdata->connected, + BUS_AND_STATUS_REG, BASR_ACK, + BASR_ACK, HZ / 64) < 0) + scmd_printk(KERN_DEBUG, hostdata->connected, "%s: !REQ and !ACK\n", __func__); if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH)) return 0; + if (bytes == 0) + udelay(MAC_PDMA_DELAY); + + if (bytes >= 0) + continue; + dsprintk(NDEBUG_PSEUDO_DMA, hostdata->host, - "%s: bus error (%d/%d)\n", __func__, transferred, len); + "%s: bus error (%d/%d)\n", __func__, d - dst, len); NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host); - d = dst + transferred; - n = len - transferred; + return -1; } scmd_printk(KERN_ERR, hostdata->connected, @@ -192,93 +139,27 @@ static inline int macscsi_pread(struct NCR5380_hostdata *hostdata, return -1; } - -#define CP_MEM_TO_IO(s,d,n) \ -__asm__ __volatile__ \ - (" cmp.w #4,%2\n" \ - " bls 8f\n" \ - " move.w %0,%%d0\n" \ - " neg.b %%d0\n" \ - " and.w #3,%%d0\n" \ - " sub.w %%d0,%2\n" \ - " bra 2f\n" \ - " 1: move.b (%0)+,(%1)\n" \ - " 2: dbf %%d0,1b\n" \ - " move.w %2,%%d0\n" \ - " lsr.w #5,%%d0\n" \ - " bra 4f\n" \ - " 3: move.l (%0)+,(%1)\n" \ - "31: move.l (%0)+,(%1)\n" \ - "32: move.l (%0)+,(%1)\n" \ - "33: move.l (%0)+,(%1)\n" \ - "34: move.l (%0)+,(%1)\n" \ - "35: move.l (%0)+,(%1)\n" \ - "36: move.l (%0)+,(%1)\n" \ - "37: move.l (%0)+,(%1)\n" \ - " 4: dbf %%d0,3b\n" \ - " move.w %2,%%d0\n" \ - " lsr.w #2,%%d0\n" \ - " and.w #7,%%d0\n" \ - " bra 6f\n" \ - " 5: move.l (%0)+,(%1)\n" \ - " 6: dbf %%d0,5b\n" \ - " and.w #3,%2\n" \ - " bra 8f\n" \ - " 7: move.b (%0)+,(%1)\n" \ - " 8: dbf %2,7b\n" \ - " moveq.l #0, %2\n" \ - " 9: \n" \ - ".section .fixup,\"ax\"\n" \ - " .even\n" \ - "91: moveq.l #1, %2\n" \ - " jra 9b\n" \ - "94: moveq.l #4, %2\n" \ - " jra 9b\n" \ - ".previous\n" \ - ".section __ex_table,\"a\"\n" \ - " .align 4\n" \ - " .long 1b,91b\n" \ - " .long 3b,94b\n" \ - " .long 31b,94b\n" \ - " .long 32b,94b\n" \ - " .long 33b,94b\n" \ - " .long 34b,94b\n" \ - " .long 35b,94b\n" \ - " .long 36b,94b\n" \ - " .long 37b,94b\n" \ - " .long 5b,94b\n" \ - " .long 7b,91b\n" \ - ".previous" \ - : "=a"(s), "=a"(d), "=d"(n) \ - : "0"(s), "1"(d), "2"(n) \ - : "d0") - static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata, unsigned char *src, int len) { unsigned char *s = src; u8 __iomem *d = hostdata->pdma_io + (OUTPUT_DATA_REG << 4); - int n = len; - int transferred; + + hostdata->pdma_residual = len; while (!NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG, BASR_DRQ | BASR_PHASE_MATCH, BASR_DRQ | BASR_PHASE_MATCH, HZ / 64)) { - CP_MEM_TO_IO(s, d, n); + int bytes; - transferred = s - src - n; - hostdata->pdma_residual = len - transferred; + bytes = mac_pdma_send(s, d, min(hostdata->pdma_residual, 512)); - /* Target changed phase early? */ - if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ, - BUS_AND_STATUS_REG, BASR_ACK, BASR_ACK, HZ / 64) < 0) - scmd_printk(KERN_ERR, hostdata->connected, - "%s: !REQ and !ACK\n", __func__); - if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH)) - return 0; + if (bytes > 0) { + s += bytes; + hostdata->pdma_residual -= bytes; + } - /* No bus error. */ - if (n == 0) { + if (hostdata->pdma_residual == 0) { if (NCR5380_poll_politely(hostdata, TARGET_COMMAND_REG, TCR_LAST_BYTE_SENT, TCR_LAST_BYTE_SENT, HZ / 64) < 0) @@ -287,17 +168,29 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata, return 0; } + if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ, + BUS_AND_STATUS_REG, BASR_ACK, + BASR_ACK, HZ / 64) < 0) + scmd_printk(KERN_DEBUG, hostdata->connected, + "%s: !REQ and !ACK\n", __func__); + if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH)) + return 0; + + if (bytes == 0) + udelay(MAC_PDMA_DELAY); + + if (bytes >= 0) + continue; + dsprintk(NDEBUG_PSEUDO_DMA, hostdata->host, - "%s: bus error (%d/%d)\n", __func__, transferred, len); + "%s: bus error (%d/%d)\n", __func__, s - src, len); NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host); - s = src + transferred; - n = len - transferred; + return -1; } scmd_printk(KERN_ERR, hostdata->connected, "%s: phase mismatch or !DRQ\n", __func__); NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host); - return -1; }
A system bus error during a PDMA transfer can mess up the calculation of the transfer residual (the PDMA handshaking hardware lacks a byte counter). This results in data corruption. The algorithm in this patch anticipates a bus error by starting each transfer with a MOVE.B instruction. If a bus error is caught the transfer will be retried. If a bus error is caught later in the transfer (for a MOVE.W instruction) the transfer gets failed and subsequent requests for that target will use PIO instead of PDMA. This avoids the "!REQ and !ACK" error so the severity level of that message is reduced to KERN_DEBUG. Cc: Michael Schmitz <schmitzmic@gmail.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: stable@vger.kernel.org # v4.14+ Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation") Reported-by: Chris Jones <chris@martin-jones.com> Tested-by: Stan Johnson <userm57@yahoo.com> Signed-off-by: Finn Thain <fthain@telegraphics.com.au> --- arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++ drivers/scsi/mac_scsi.c | 201 ++++++++----------------------- 2 files changed, 226 insertions(+), 154 deletions(-) create mode 100644 arch/m68k/include/asm/mac_pdma.h