[PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

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(-)

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)
-- 
2.17.1

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[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
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

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Michael Schmitz]

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
>

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

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.

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Michael Schmitz]

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.

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

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.

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[James Bottomley]

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

[PATCH v2] zorro_esp: Limit DMA transfers to 65536 bytes (except on Fastlane)

[Kars de Jong]

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.

The limit of 65536 bytes can be used by all boards except the Fastlane. The
old driver used a limit of 65532 bytes (0xfffc), which is reintroduced in
this patch.

Fixes: b7ded0e8b0d1 ("scsi: zorro_esp: Limit DMA transfers to 65535 bytes")
Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/zorro_esp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index ca8e3abeb2c7..a23a8e5794f5 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -218,7 +218,14 @@ 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 u32 fastlane_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
+					u32 dma_len)
+{
+	/* The old driver used 0xfffc as limit, so do that here too */
+	return dma_len > 0xfffc ? 0xfffc : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)
@@ -604,7 +611,7 @@ static const struct esp_driver_ops fastlane_esp_ops = {
 	.esp_write8		= zorro_esp_write8,
 	.esp_read8		= zorro_esp_read8,
 	.irq_pending		= fastlane_esp_irq_pending,
-	.dma_length_limit	= zorro_esp_dma_length_limit,
+	.dma_length_limit	= fastlane_esp_dma_length_limit,
 	.reset_dma		= zorro_esp_reset_dma,
 	.dma_drain		= zorro_esp_dma_drain,
 	.dma_invalidate		= fastlane_esp_dma_invalidate,
-- 
2.17.1

Re: [PATCH v2] zorro_esp: Limit DMA transfers to 65536 bytes (except on Fastlane)

[Finn Thain]

On Tue, 12 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.
> 
> 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.
> 
> The limit of 65536 bytes can be used by all boards except the Fastlane. The
> old driver used a limit of 65532 bytes (0xfffc), which is reintroduced in
> this patch.
> 
> Fixes: b7ded0e8b0d1 ("scsi: zorro_esp: Limit DMA transfers to 65535 bytes")
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>

Reviewed-by: Finn Thain <fthain@telegraphics.com.au>

> ---
>  drivers/scsi/zorro_esp.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> index ca8e3abeb2c7..a23a8e5794f5 100644
> --- a/drivers/scsi/zorro_esp.c
> +++ b/drivers/scsi/zorro_esp.c
> @@ -218,7 +218,14 @@ 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 u32 fastlane_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
> +					u32 dma_len)
> +{
> +	/* The old driver used 0xfffc as limit, so do that here too */
> +	return dma_len > 0xfffc ? 0xfffc : dma_len;
>  }
>  
>  static void zorro_esp_reset_dma(struct esp *esp)
> @@ -604,7 +611,7 @@ static const struct esp_driver_ops fastlane_esp_ops = {
>  	.esp_write8		= zorro_esp_write8,
>  	.esp_read8		= zorro_esp_read8,
>  	.irq_pending		= fastlane_esp_irq_pending,
> -	.dma_length_limit	= zorro_esp_dma_length_limit,
> +	.dma_length_limit	= fastlane_esp_dma_length_limit,
>  	.reset_dma		= zorro_esp_reset_dma,
>  	.dma_drain		= zorro_esp_dma_drain,
>  	.dma_invalidate		= fastlane_esp_dma_invalidate,
> 

Re: [PATCH v2] zorro_esp: Limit DMA transfers to 65536 bytes (except on Fastlane)

[Martin K. Petersen]

Kars,

> 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.

Applied to 5.5/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

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.

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Finn Thain]

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?

-- 

>  static void zorro_esp_reset_dma(struct esp *esp)
> 

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

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.