| Message ID | 20191109191400.8999-1-jongk@linux-m68k.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | zorro_esp: increase maximum dma length to 65536 bytes | expand |
On Sat, 2019-11-09 at 20:14 +0100, Kars de Jong wrote: > When using this driver on a Blizzard 1260, there were failures > whenever DMA transfers from the SCSI bus to memory of 65535 bytes > were followed by a DMA transfer of 1 byte. This caused the byte at > offset 65535 to be overwritten with 0xff. The Blizzard hardware can't > handle single byte DMA transfers. > > Besides this issue, limiting the DMA length to something that is not > a multiple of the page size is very inefficient on most file systems. > > It seems this limit was chosen because the DMA transfer counter of > the ESP by default is 16 bits wide, thus limiting the length to 65535 > bytes. However, the value 0 means 65536 bytes, which is handled by > the ESP and the Blizzard just fine. It is also the default maximum > used by esp_scsi when drivers don't provide their own > dma_length_limit() function. Have you tested this on any other hardware? the reason most legacy hardware would have a setting like this is because they have a two byte transfer length register and zero doesn't mean 65536. If this is the case for any of the cards the zorro_esp drives, it might be better to lower the max length to 61440 (64k-4k) so the residual is a page. James
On Sat, 9 Nov 2019, Kars de Jong wrote: > When using this driver on a Blizzard 1260, there were failures whenever > DMA transfers from the SCSI bus to memory of 65535 bytes were followed by a > DMA transfer of 1 byte. This caused the byte at offset 65535 to be > overwritten with 0xff. The Blizzard hardware can't handle single byte DMA > transfers. > > Besides this issue, limiting the DMA length to something that is not a > multiple of the page size is very inefficient on most file systems. > Makes sense. You may want to add, Fixes: b7ded0e8b0d1 ("scsi: zorro_esp: Limit DMA transfers to 65535 bytes") > It seems this limit was chosen because the DMA transfer counter of the ESP > by default is 16 bits wide, thus limiting the length to 65535 bytes. > However, the value 0 means 65536 bytes, which is handled by the ESP and the > Blizzard just fine. It is also the default maximum used by esp_scsi when > drivers don't provide their own dma_length_limit() function. > > Signed-off-by: Kars de Jong <jongk@linux-m68k.org> > --- > drivers/scsi/zorro_esp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c > index ca8e3abeb2c7..4448567c495d 100644 > --- a/drivers/scsi/zorro_esp.c > +++ b/drivers/scsi/zorro_esp.c > @@ -218,7 +218,7 @@ static int fastlane_esp_irq_pending(struct esp *esp) > static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr, > u32 dma_len) > { > - return dma_len > 0xFFFF ? 0xFFFF : dma_len; > + return dma_len > (1U << 16) ? (1U << 16) : dma_len; > } > Would it be safer to simply remove this code and leave esp_driver_ops.dma_length_limit == NULL for all board types?
James, Am 10.11.2019 um 09:12 schrieb James Bottomley: > On Sat, 2019-11-09 at 20:14 +0100, Kars de Jong wrote: >> When using this driver on a Blizzard 1260, there were failures >> whenever DMA transfers from the SCSI bus to memory of 65535 bytes >> were followed by a DMA transfer of 1 byte. This caused the byte at >> offset 65535 to be overwritten with 0xff. The Blizzard hardware can't >> handle single byte DMA transfers. >> >> Besides this issue, limiting the DMA length to something that is not >> a multiple of the page size is very inefficient on most file systems. >> >> It seems this limit was chosen because the DMA transfer counter of >> the ESP by default is 16 bits wide, thus limiting the length to 65535 >> bytes. However, the value 0 means 65536 bytes, which is handled by >> the ESP and the Blizzard just fine. It is also the default maximum >> used by esp_scsi when drivers don't provide their own >> dma_length_limit() function. > > Have you tested this on any other hardware? the reason most legacy > hardware would have a setting like this is because they have a two byte > transfer length register and zero doesn't mean 65536. If this is the The data book for the NCR53FC94/NCR53FC96 (the 'fast' SCSI controller used on the board Kars tried) states that with the features enable bit clear (no 24 bit DMA counts used), zero does mean 64k indeed. The features enable bit is never set by esp_scsi.c. I chose the incorrect length limit without realizing this special case for the transfer count. and before we found out that 1-byte DMA just won't work. I need to confirm this from a data book of the older (pre-fast) revisions of this chip family. but since as Kars also states, the core driver default for the 16 bit transfer size is 64k as well, I very much suspect the same behaviour for the older revisions. All of the old board-specific drivers used a max transfer length of 0x1000000, only the fastlane driver used 0xfffc. That lower limit might be due to a DMA limitation on the fastlane board. We could accommodate the different limit for this board by using a board-specific dma_length_limit() callback... > case for any of the cards the zorro_esp drives, it might be better to > lower the max length to 61440 (64k-4k) so the residual is a page. For the benefit of keeping the code simple, and avoid retesting the fastlane board, that might indeed be the better solution. Cheers, Michael > > James >
Hi Michael, Op zo 10 nov. 2019 om 03:36 schreef Michael Schmitz <schmitzmic@gmail.com>: > All of the old board-specific drivers used a max transfer length of > 0x1000000, only the fastlane driver used 0xfffc. Yes, I also found this when checking the old drivers. > That lower limit might > be due to a DMA limitation on the fastlane board. We could accommodate > the different limit for this board by using a board-specific > dma_length_limit() callback... Yes, I think that's the best idea for now. Oktagon also used to have a different limit but that was never ported to the new ESP core. > > case for any of the cards the zorro_esp drives, it might be better to > > lower the max length to 61440 (64k-4k) so the residual is a page. > > For the benefit of keeping the code simple, and avoid retesting the > fastlane board, that might indeed be the better solution. But it's slower... :-P Also, I may be adding another board-specific version for the Blizzard 12x0 IV to enable 24-bit transfers, like the am53c974 driver does, in a later patch. Kind regards, Kars.
Hi Finn, Op za 9 nov. 2019 om 23:53 schreef Finn Thain <fthain@telegraphics.com.au>: > On Sat, 9 Nov 2019, Kars de Jong wrote: > > diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c > > index ca8e3abeb2c7..4448567c495d 100644 > > --- a/drivers/scsi/zorro_esp.c > > +++ b/drivers/scsi/zorro_esp.c > > @@ -218,7 +218,7 @@ static int fastlane_esp_irq_pending(struct esp *esp) > > static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr, > > u32 dma_len) > > { > > - return dma_len > 0xFFFF ? 0xFFFF : dma_len; > > + return dma_len > (1U << 16) ? (1U << 16) : dma_len; > > } > > > > Would it be safer to simply remove this code and leave > esp_driver_ops.dma_length_limit == NULL for all board types? I have considered that, but that version also imposes unneeded limits on crossing a 24-bit address boundary. The Zorro boards don't seem to need this. Kind regards, Kars.
Hi Kars, thanks for your patch! On 10/11/19 10:01 PM, Kars de Jong wrote: > Hi Michael, > > Op zo 10 nov. 2019 om 03:36 schreef Michael Schmitz <schmitzmic@gmail.com>: >> All of the old board-specific drivers used a max transfer length of >> 0x1000000, only the fastlane driver used 0xfffc. > Yes, I also found this when checking the old drivers. > >> That lower limit might >> be due to a DMA limitation on the fastlane board. We could accommodate >> the different limit for this board by using a board-specific >> dma_length_limit() callback... > Yes, I think that's the best idea for now. Oktagon also used to have a > different limit but that was never ported to the new ESP core. I can't remember the details, but as far as I recall it, the Oktagon used pseudo-DMA rather than hardware DMA. At the time I started porting Zorro ESP drivers to the new core, pseudo-DMA code was available for Mac only, and no PIO transfer for data phases at all, so I decided to leave that out altogether. Might be a lot easier now that Finn has moved the PIO support code into the core driver. Someone could start with a PIO mode driver and add PDMA later. >>> case for any of the cards the zorro_esp drives, it might be better to >>> lower the max length to 61440 (64k-4k) so the residual is a page. >> For the benefit of keeping the code simple, and avoid retesting the >> fastlane board, that might indeed be the better solution. > But it's slower... :-P I wonder what max. transfer size had been used so far, in the majority of cases. I hadn't observed this bug in my tests of the ESP driver on elgar. So it might not matter so much in practice. > Also, I may be adding another board-specific version for the Blizzard > 12x0 IV to enable 24-bit transfers, like the am53c974 driver does, in > a later patch. If we can differentiate between the Mark IV board and the Mark II board in a reliable way, fine. I can't remember whether I've had a report on that ever. I'd suggest to change the transfer size limit to 60k in the first instance, and add board-specific tweaks as needed when you add 24 bit DMA support for the Mark IV. Cheers, Michael > > Kind regards, > > Kars.
On Sun, 2019-11-10 at 15:36 +1300, Michael Schmitz wrote: > All of the old board-specific drivers used a max transfer length of > 0x1000000, only the fastlane driver used 0xfffc. That lower limit > might be due to a DMA limitation on the fastlane board. We could > accommodate the different limit for this board by using a board- > specific dma_length_limit() callback... Sure, that implies the minimum dma burst length is 4 bytes ... > > case for any of the cards the zorro_esp drives, it might be better > > to lower the max length to 61440 (64k-4k) so the residual is a > > page. > > For the benefit of keeping the code simple, and avoid retesting the > fastlane board, that might indeed be the better solution. ... which means you don't have to lower the limit by 4k as I suggested, because I was being incredibly conservative about what the dma burst length might be, just use 0xfffc as the default. I think raising to 0x1000000 for boards which can support it is fine too if you want to add the extra logic to the driver. James
Hi Michael, Op zo 10 nov. 2019 om 20:26 schreef Michael Schmitz <schmitzmic@gmail.com>: > >>> case for any of the cards the zorro_esp drives, it might be better to > >>> lower the max length to 61440 (64k-4k) so the residual is a page. > >> For the benefit of keeping the code simple, and avoid retesting the > >> fastlane board, that might indeed be the better solution. > > > > But it's slower... :-P > > > I wonder what max. transfer size had been used so far, in the majority > of cases. I hadn't observed this bug in my tests of the ESP driver on > elgar. So it might not matter so much in practice. Does Elgar indeed have a Blizzard 2060 as https://wiki.debian.org/M68k/Autobuilder says? If it does, it does surprise me that it works, since the DMA engine appears to be very much like the one of the Blizzard 1230 (including the >> 1 of the address). Even when just loading bash on my system, there were many 65535-and-then-1 byte transfers. It may of course depend on how fragmented your disk is. > > Also, I may be adding another board-specific version for the Blizzard > > 12x0 IV to enable 24-bit transfers, like the am53c974 driver does, in > > a later patch. > > If we can differentiate between the Mark IV board and the Mark II board > in a reliable way, fine. I can't remember whether I've had a report on > that ever. They have a different Zorro ID, so that should not be a problem. By the way, do you remember why you chose to not use the full Zorro IDs for the various SCSI boards asdefined in zorro_ids.h but only the manufacturer defines? Kind regards, Kars.
Hi Michael, Op zo 10 nov. 2019 om 03:36 schreef Michael Schmitz <schmitzmic@gmail.com>: > I need to confirm this from a data book of the older (pre-fast) > revisions of this chip family. but since as Kars also states, the core > driver default for the 16 bit transfer size is 64k as well, I very much > suspect the same behaviour for the older revisions. According to ftp://bitsavers.informatik.uni-stuttgart.de/components/ncr/scsi/NCR53C90ab.pdf, on the NCR 53C90A programming the transfer counter to 0 also means 64k. That's the oldest data book I could find. But the Zorro driver assumes a fast variant of the chip anyway (the clock frequency is hard coded to 40 MHz), so this point is a little moot. Kind regards, Kars.
diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c index ca8e3abeb2c7..4448567c495d 100644 --- a/drivers/scsi/zorro_esp.c +++ b/drivers/scsi/zorro_esp.c @@ -218,7 +218,7 @@ static int fastlane_esp_irq_pending(struct esp *esp) static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr, u32 dma_len) { - return dma_len > 0xFFFF ? 0xFFFF : dma_len; + return dma_len > (1U << 16) ? (1U << 16) : dma_len; } static void zorro_esp_reset_dma(struct esp *esp)
When using this driver on a Blizzard 1260, there were failures whenever DMA transfers from the SCSI bus to memory of 65535 bytes were followed by a DMA transfer of 1 byte. This caused the byte at offset 65535 to be overwritten with 0xff. The Blizzard hardware can't handle single byte DMA transfers. Besides this issue, limiting the DMA length to something that is not a multiple of the page size is very inefficient on most file systems. It seems this limit was chosen because the DMA transfer counter of the ESP by default is 16 bits wide, thus limiting the length to 65535 bytes. However, the value 0 means 65536 bytes, which is handled by the ESP and the Blizzard just fine. It is also the default maximum used by esp_scsi when drivers don't provide their own dma_length_limit() function. Signed-off-by: Kars de Jong <jongk@linux-m68k.org> --- drivers/scsi/zorro_esp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)