On Mon, 5 Mar 2018, Michael Schmitz wrote:
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.
It did show up in PIO mode so it's a matter of having the right target
to trigger it. dmesg on elgar says:
[ 26.960000] scsi host1: esp
[ 27.270000] scsi 1:0:1:0: Direct-Access IBMRAID DFHSS2F9337 4I4I PQ: 0 ANSI: 2
[ 27.280000] scsi target1:0:1: Beginning Domain Validation
[ 27.360000] scsi target1:0:1: FAST-10 SCSI 10.0 MB/s ST (100 ns, offset 15)
[ 27.380000] scsi target1:0:1: Domain Validation skipping write tests
[ 27.390000] scsi target1:0:1: Ending Domain Validation
[ 27.450000] scsi 1:0:2:0: CD-ROM SONY CD-ROM CDU-561 1.9m PQ: 0 ANSI: 2 CCS
[ 27.460000] scsi target1:0:2: Beginning Domain Validation
[ 27.580000] scsi target1:0:2: FAST-5 SCSI 4.0 MB/s ST (248 ns, offset 15)
[ 27.600000] scsi target1:0:2: Domain Validation skipping write tests
[ 27.620000] scsi target1:0:2: Ending Domain Validation
[ 27.750000] scsi host1: scsi scan: INQUIRY result too short (5), using 36
To the best of my knowledge, the CD-ROM triggered that error. Looking at
the NCR53CF94_96 databook, select with attention and stop is used when
multiple message bytes need to be transferred (example: sync negotiation
- that's when esp->flags |= ESP_FLAG_DOING_SLOWCMD is used in the ESP
core which switches to using this selection mode).
One message byte is transferred before the ESP raises both
ESP_INTR_FDONE and ESP_INTR_BSERV. From all appearances, the bus will
still be in message out phase at that stage.
u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : (phase == ESP_MOP ?
ESP_INTR_FDONE|ESP_INTR_BSERV : ESP_INTR_BSERV) );
would set the correct interrupt status mask to avoid detecting this
condition as error.
Well, it is consistent with zorro_esp_send_pio_cmd as far as ESP_MOP is
concerned.
Doing sync negotiation is fairly basic - I wonder why you haven't seen
this before. Ah - the Mac driver supports sync transfers only when using
PDMA, that's why.
I suspect that if you try PIO for all transfers, not just Message In,
you'll need to drop async support too. But I don't recall why async was a
problem back when I was developing mac_esp. It was too long ago.
The description of 'select with attention' also states the same two
interrupt bits are set, so maybe the bug is elsewhere. According to the
manual, only the first message byte should be loaded into the FIFO when
stopping for additional message bytes. We load the entire message
instead...
FDONE is not a problem once all the bytes are in the fifo. The mac_esp
algorithm already accomodates that.
Which manual is this, BTW?
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.
Deciding whether an error exists just based on the phase might be
impossible now I come to think of it - the combination of
ESP_INTR_FDONE|ESP_INTR_BSERV is only legitimate for selection commands.
Well, I'm not trying to decide whether any error exists, I'm just trying
to detect an unexpected interrupt in the middle of ESP_DOP. Hence my
suggestion to add a test for ESP_MOP.
We might need to look at additional information to figure out whether
getting ESP_INTR_FDONE in message out phase was an error.
Your algorithm is pretty clear on that point: FDONE is not an error during
message out... but I'm beginning to think that it really is an error
during message out, except at the end of the transfer.
This is getting a little academic for zorro_esp - if a particular board
needs to do PIO all the time, we need a way to set the ESP_FLAG_USE_FIFO
flag in the core driver instead. Much safer.
Unfortunately, that flag doesn't always do what is needed for PIO.
Anyway, this discussion is completely academic when you consider this:
if (phase == ESP_MIP) {
zorro_esp_send_pio_cmd(esp, addr, esp_count,
dma_count, write, cmd);
return;
}
Based on this, you should be able to use the algorithm from
mac_esp_send_pio_cmd() unaltered, since your change to the !write branch
is irrelevant during Message In phase.
My main concern here is to avoid two slightly different copies of that
code.
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.
Reading that thread, leaving BUG_ON would make Linus pretty angry.
Maybe remove it if you think it can't fire.
Setting zep->error and letting the core driver deal with it might be
safer.
True enough ;-)
--
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
