On Sun, 4 Mar 2018, Michael Schmitz wrote:
> Am 04.03.2018 um 15:55 schrieb Finn Thain:
> >> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
> >> + *
> >> + * Copyright (C) 1996 Jesper Skov (js...@cygnus.co.uk)
> >> + *
> >> + * Copyright (C) 2011,2018 Michael Schmitz (schm...@debian.org) for
> >> + * migration to ESP SCSI core
> >
> > You can blame me for some of this ;-)
>
> Oops - did acknowledge you in the commit message but forgot to do it in
> the code ... or did you mean something else?
>
Right. The commit message is sufficient credit for my needs.
> >> +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \
> >> + &zorro_esp_private_data[(esp->host->host_no)])
> >> +
> >
> > How do you know that host_no won't exceed the array bounds?
> > Why not use dev_{set,get}_drvdata(esp->dev)? -- much as mac_esp uses
> > platform_{set,get}_drvdata() here.
>
> I don't think you can have more than 8 Zorro cards in an Amiga.
The host_no range is not limited to just zorro cards. It's also libata
hosts, USB Attached Storage hosts, scsi_debug etc.
> >
> > I suspect that the mac_esp PIO code should work fine here unchanged.
> > If so, let's avoid a new variation on that code if that's possible (?)
>
> The Mac PIO code did not work unchanged here, that's why I checked the
> interrupt bit raised and changed the mask. But that was only for one
> quirky target (CD-ROM, SCSI-1 CCS IIRC) and only when testing the entire
> driver without DMA. Doesn't happen in DMA mode.
>
> You would have seen the error only with such a special target, and
> probably not in PDMA mode...
>
The TCQ issue showed up on the AV Quadras becasue mac_esp doesn't know how
to do PDMA or DMA on that hardware, and so it always uses PIO. It sounds
like this bug would show up too given the right kind of target. If so, I
will need to patch mac_esp too.
> Regarding your other comment on this code: I'll have to review what
> phase was present when the select with attention and stop condition
> occurred,
Thanks. That's what I'd prefer to use in mac_esp (if it works).
> but setting the mask up front according to the bus phase would make a
> lot of sense.
>
The idea is that ESP_INTR_FDONE is an error when the phase is not
appropriate. In the version you sent, the loop permits ESP_INTR_BSERV |
ESP_INTR_FDONE regardless of phase.
> > WARN_ON is preferred. Please see Johannes Thumshirn's message from last
> > week,
> > https://www.spinics.net/lists/linux-scsi/msg117919.html
>
> Well, should we ever see this condition trigger (I have never!), I don't
> think there is a way we can recover from it. We expect a DMA command
> (DMA i.e. physical address), how can we reliably determine whether the
> calling code just forgot to set the bit, or really passed us a kernel
> virtual address?
>
Thinking about this a bit more, BUG_ON may be the right thing to do here.
If the core driver will not catch the DMA error, that might lead to data
corruption. You'd better check this before switching to WARN_ON. We can't
assume that end users will notice the warning in the logs.
> >> +static int zorro_esp_dma_error(struct esp *esp)
> >> +{
> >> + struct zorro_esp_priv *zep = ZORRO_ESP_GET_PRIV(esp);
> >> + u8 phase = esp->sreg & ESP_STAT_PMASK;
> >> +
> >> + /* if doing PIO, check for error */
> >> + if (phase == ESP_MIP && zep->error == 1)
> >> + return 1;
> >
> > Don't check the bus phase here, the target controls that. Just make sure
> > zep->error gets set to zero when not using PIO; that's what
> > mac_esp_send_pdma_cmd() does.
>
> You mean before sending each DMA command? Couldn't find a way to figure
> out whether we're been doing DMA or PIO otherwise.
>
Both mac_esp_send_pdma_cmd() and mac_esp_send_pio_cmd() just initialize
this to zero at the outset. Any time that mep->error is set, it means the
most recent transfer failed, which I think is what matters here.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
