On Mon, 3 Jun 2019, Geert Uytterhoeven wrote:
Hi Finn,
On Mon, Jun 3, 2019 at 1:32 AM Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
On Sun, 2 Jun 2019, Geert Uytterhoeven wrote:
On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxx>
Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx # v4.14+
Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation")
Reported-by: Chris Jones <chris@xxxxxxxxxxxxxxxx>
Tested-by: Stan Johnson <userm57@xxxxxxxxx>
Signed-off-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
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.
--
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
