[PATCH] scsi: gvp11: add module parameter for DMA transfer bit mask

[PATCH] scsi: gvp11: add module parameter for DMA transfer bit mask

[Michael Schmitz]

Commit bfaa4a0ce1bb ("scsi: gvp11: Remove unused gvp11_setup()
function") removed the unused gvp11_setup() which had allowed
to override the default DMA transfer mask defined for GVP II
SCSI boards on Amiga. There now is no way to set a non-default
DMA mask on these boards.

Introduce a module parameter to allow users to override the
default DMA mask for the gvp11 driver.

Fixes: bfaa4a0ce1bb ("scsi: gvp11: Remove unused gvp11_setup() function")
Link: https://lore.kernel.org/r/20230810141947.1236730-12-arnd@kernel.org
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/scsi/gvp11.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/gvp11.c b/drivers/scsi/gvp11.c
index 0420bfe9bd42..12160d70a571 100644
--- a/drivers/scsi/gvp11.c
+++ b/drivers/scsi/gvp11.c
@@ -50,6 +50,9 @@ static irqreturn_t gvp11_intr(int irq, void *data)
 
 static int gvp11_xfer_mask = 0;
 
+module_param(gvp11_xfer_mask,  int, 0444);
+MODULE_PARM_DESC(gvp11_xfer_mask, "DMA mask (0xff000000 == 24 bit DMA)");
+
 static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 {
 	struct scsi_pointer *scsi_pointer = WD33C93_scsi_pointer(cmd);
-- 
2.17.1

Re: [PATCH] scsi: gvp11: add module parameter for DMA transfer bit mask

[Arnd Bergmann]

On Tue, Aug 29, 2023, at 17:45, Michael Schmitz wrote:
> SCSI boards on Amiga. There now is no way to set a non-default
> DMA mask on these boards.

It might help to mention here in which cases the default mask
is actually wrong.

> +module_param(gvp11_xfer_mask,  int, 0444);
> +MODULE_PARM_DESC(gvp11_xfer_mask, "DMA mask (0xff000000 == 24 bit DMA)");
> +

I think the comment is the wrong way round, it should be
0x00ffffff in this case, which also matches the default
mask for ZORRO_PROD_GVP_SERIES_II, in the match table:

static struct zorro_device_id gvp11_zorro_tbl[] = {
        { ZORRO_PROD_GVP_COMBO_030_R3_SCSI,     ~0x00ffffff },
        { ZORRO_PROD_GVP_SERIES_II,             ~0x00ffffff },
        { ZORRO_PROD_GVP_GFORCE_030_SCSI,       ~0x01ffffff },
        { ZORRO_PROD_GVP_A530_SCSI,             ~0x01ffffff },
        { ZORRO_PROD_GVP_COMBO_030_R4_SCSI,     ~0x01ffffff },
        { ZORRO_PROD_GVP_A1291,                 ~0x07ffffff },
        { ZORRO_PROD_GVP_GFORCE_040_SCSI_1,     ~0x07ffffff },
        { 0 }
};

      Arnd

Re: [PATCH] scsi: gvp11: add module parameter for DMA transfer bit mask

[Michael Schmitz]

Hi Arnd,

thanks for your comments!

On 30/08/23 10:05, Arnd Bergmann wrote:
> On Tue, Aug 29, 2023, at 17:45, Michael Schmitz wrote:
>> SCSI boards on Amiga. There now is no way to set a non-default
>> DMA mask on these boards.
> It might help to mention here in which cases the default mask
> is actually wrong.

All I have is:

Probably it's needed on A2000 with an accelerator card and GVP II SCSI,
to prevent DMA to RAM banks that do not support fast DMA cycles.

from Geert's reply. I can add that. It just did sound a shade 
speculative...
>
>> +module_param(gvp11_xfer_mask,  int, 0444);
>> +MODULE_PARM_DESC(gvp11_xfer_mask, "DMA mask (0xff000000 == 24 bit DMA)");
>> +
> I think the comment is the wrong way round, it should be
> 0x00ffffff in this case, which also matches the default
> mask for ZORRO_PROD_GVP_SERIES_II, in the match table:
>
> static struct zorro_device_id gvp11_zorro_tbl[] = {
>          { ZORRO_PROD_GVP_COMBO_030_R3_SCSI,     ~0x00ffffff },
>          { ZORRO_PROD_GVP_SERIES_II,             ~0x00ffffff },
>          { ZORRO_PROD_GVP_GFORCE_030_SCSI,       ~0x01ffffff },
>          { ZORRO_PROD_GVP_A530_SCSI,             ~0x01ffffff },
>          { ZORRO_PROD_GVP_COMBO_030_R4_SCSI,     ~0x01ffffff },
>          { ZORRO_PROD_GVP_A1291,                 ~0x07ffffff },
>          { ZORRO_PROD_GVP_GFORCE_040_SCSI_1,     ~0x07ffffff },
>          { 0 }
> };

gvp11_xfer_mask works inverse to what you'd expect (and inverse to what 
a DMA mask usually is defined as). DMA can _not_ be used if (address & 
gvp11_xfer_mask) isn't zero. See code in dma_setup() for details.

All those definitions have a '~' prefix, for that very reason.

I agree it isn't intuitive, and caused a little head scratching when 
preparing this patch. But I believe it is correct.

Now you could argue to shift the bit mask inversion to gvp11_probe() or 
even dma_setup() instead to rule out such confusion in future, but that 
would be an actual code change and would benefit from testing on at 
least one of these boards IMO. Not sure how easy that will be.

Cheers,

     Michael

>        Arnd

Re: [PATCH] scsi: gvp11: add module parameter for DMA transfer bit mask

[Arnd Bergmann]

On Tue, Aug 29, 2023, at 18:25, Michael Schmitz wrote:
>
>>> +module_param(gvp11_xfer_mask,  int, 0444);
>>> +MODULE_PARM_DESC(gvp11_xfer_mask, "DMA mask (0xff000000 == 24 bit DMA)");
>>> +
>> I think the comment is the wrong way round, it should be
>> 0x00ffffff in this case, which also matches the default
>> mask for ZORRO_PROD_GVP_SERIES_II, in the match table:
>>
>> static struct zorro_device_id gvp11_zorro_tbl[] = {
>>          { ZORRO_PROD_GVP_COMBO_030_R3_SCSI,     ~0x00ffffff },
>>          { ZORRO_PROD_GVP_SERIES_II,             ~0x00ffffff },
>>          { ZORRO_PROD_GVP_GFORCE_030_SCSI,       ~0x01ffffff },
>>          { ZORRO_PROD_GVP_A530_SCSI,             ~0x01ffffff },
>>          { ZORRO_PROD_GVP_COMBO_030_R4_SCSI,     ~0x01ffffff },
>>          { ZORRO_PROD_GVP_A1291,                 ~0x07ffffff },
>>          { ZORRO_PROD_GVP_GFORCE_040_SCSI_1,     ~0x07ffffff },
>>          { 0 }
>> };
>
> gvp11_xfer_mask works inverse to what you'd expect (and inverse to what 
> a DMA mask usually is defined as). DMA can _not_ be used if (address & 
> gvp11_xfer_mask) isn't zero. See code in dma_setup() for details.
>
> All those definitions have a '~' prefix, for that very reason.
>
> I agree it isn't intuitive, and caused a little head scratching when 
> preparing this patch. But I believe it is correct.
>
> Now you could argue to shift the bit mask inversion to gvp11_probe() or 
> even dma_setup() instead to rule out such confusion in future, but that 
> would be an actual code change and would benefit from testing on at 
> least one of these boards IMO. Not sure how easy that will be.

Ok, I see now. Let's leave the patch as it is then.

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] scsi: gvp11: add module parameter for DMA transfer bit mask

[Michael Schmitz]

Thanks Arnd!

Cheers,

     Michael

On 30/08/23 12:14, Arnd Bergmann wrote:
> On Tue, Aug 29, 2023, at 18:25, Michael Schmitz wrote:
>>>> +module_param(gvp11_xfer_mask,  int, 0444);
>>>> +MODULE_PARM_DESC(gvp11_xfer_mask, "DMA mask (0xff000000 == 24 bit DMA)");
>>>> +
>>> I think the comment is the wrong way round, it should be
>>> 0x00ffffff in this case, which also matches the default
>>> mask for ZORRO_PROD_GVP_SERIES_II, in the match table:
>>>
>>> static struct zorro_device_id gvp11_zorro_tbl[] = {
>>>           { ZORRO_PROD_GVP_COMBO_030_R3_SCSI,     ~0x00ffffff },
>>>           { ZORRO_PROD_GVP_SERIES_II,             ~0x00ffffff },
>>>           { ZORRO_PROD_GVP_GFORCE_030_SCSI,       ~0x01ffffff },
>>>           { ZORRO_PROD_GVP_A530_SCSI,             ~0x01ffffff },
>>>           { ZORRO_PROD_GVP_COMBO_030_R4_SCSI,     ~0x01ffffff },
>>>           { ZORRO_PROD_GVP_A1291,                 ~0x07ffffff },
>>>           { ZORRO_PROD_GVP_GFORCE_040_SCSI_1,     ~0x07ffffff },
>>>           { 0 }
>>> };
>> gvp11_xfer_mask works inverse to what you'd expect (and inverse to what
>> a DMA mask usually is defined as). DMA can _not_ be used if (address &
>> gvp11_xfer_mask) isn't zero. See code in dma_setup() for details.
>>
>> All those definitions have a '~' prefix, for that very reason.
>>
>> I agree it isn't intuitive, and caused a little head scratching when
>> preparing this patch. But I believe it is correct.
>>
>> Now you could argue to shift the bit mask inversion to gvp11_probe() or
>> even dma_setup() instead to rule out such confusion in future, but that
>> would be an actual code change and would benefit from testing on at
>> least one of these boards IMO. Not sure how easy that will be.
> Ok, I see now. Let's leave the patch as it is then.
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] scsi: gvp11: add module parameter for DMA transfer bit mask

[Geert Uytterhoeven]

Hi Michael,

On Wed, Aug 30, 2023 at 12:26 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> On 30/08/23 10:05, Arnd Bergmann wrote:
> > On Tue, Aug 29, 2023, at 17:45, Michael Schmitz wrote:
> >> SCSI boards on Amiga. There now is no way to set a non-default
> >> DMA mask on these boards.
> > It might help to mention here in which cases the default mask
> > is actually wrong.
>
> All I have is:
>
> Probably it's needed on A2000 with an accelerator card and GVP II SCSI,
> to prevent DMA to RAM banks that do not support fast DMA cycles.
>
> from Geert's reply. I can add that. It just did sound a shade
> speculative...

Apparently gvp11_setup() became unused in 2.3.13pre2 (in 1999), when all
*_setup() functions were removed from init/main.c, and some of them were
reimplemented using __setup() in the driver sources where they belonged.

> >> +module_param(gvp11_xfer_mask,  int, 0444);
> >> +MODULE_PARM_DESC(gvp11_xfer_mask, "DMA mask (0xff000000 == 24 bit DMA)");
> >> +
> > I think the comment is the wrong way round, it should be
> > 0x00ffffff in this case, which also matches the default
> > mask for ZORRO_PROD_GVP_SERIES_II, in the match table:
> >
> > static struct zorro_device_id gvp11_zorro_tbl[] = {
> >          { ZORRO_PROD_GVP_COMBO_030_R3_SCSI,     ~0x00ffffff },
> >          { ZORRO_PROD_GVP_SERIES_II,             ~0x00ffffff },
> >          { ZORRO_PROD_GVP_GFORCE_030_SCSI,       ~0x01ffffff },
> >          { ZORRO_PROD_GVP_A530_SCSI,             ~0x01ffffff },
> >          { ZORRO_PROD_GVP_COMBO_030_R4_SCSI,     ~0x01ffffff },
> >          { ZORRO_PROD_GVP_A1291,                 ~0x07ffffff },
> >          { ZORRO_PROD_GVP_GFORCE_040_SCSI_1,     ~0x07ffffff },
> >          { 0 }
> > };

The default masks above were added (in some other form) in 2.1.91pre1
(in 1998).  Before, people had to use gvp11_setup() to do that.

So I think it is safe to assume there is no longer a need to configure
this manually.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] scsi: gvp11: add module parameter for DMA transfer bit mask

[Michael Schmitz]

Hi Geert

Am 30.08.2023 um 19:32 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Wed, Aug 30, 2023 at 12:26 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> On 30/08/23 10:05, Arnd Bergmann wrote:
>>> On Tue, Aug 29, 2023, at 17:45, Michael Schmitz wrote:
>>>> SCSI boards on Amiga. There now is no way to set a non-default
>>>> DMA mask on these boards.
>>> It might help to mention here in which cases the default mask
>>> is actually wrong.
>>
>> All I have is:
>>
>> Probably it's needed on A2000 with an accelerator card and GVP II SCSI,
>> to prevent DMA to RAM banks that do not support fast DMA cycles.
>>
>> from Geert's reply. I can add that. It just did sound a shade
>> speculative...
>
> Apparently gvp11_setup() became unused in 2.3.13pre2 (in 1999), when all
> *_setup() functions were removed from init/main.c, and some of them were
> reimplemented using __setup() in the driver sources where they belonged.

But that wasn't done for gvp11_setup() ...

>>>> +module_param(gvp11_xfer_mask,  int, 0444);
>>>> +MODULE_PARM_DESC(gvp11_xfer_mask, "DMA mask (0xff000000 == 24 bit DMA)");
>>>> +
>>> I think the comment is the wrong way round, it should be
>>> 0x00ffffff in this case, which also matches the default
>>> mask for ZORRO_PROD_GVP_SERIES_II, in the match table:
>>>
>>> static struct zorro_device_id gvp11_zorro_tbl[] = {
>>>          { ZORRO_PROD_GVP_COMBO_030_R3_SCSI,     ~0x00ffffff },
>>>          { ZORRO_PROD_GVP_SERIES_II,             ~0x00ffffff },
>>>          { ZORRO_PROD_GVP_GFORCE_030_SCSI,       ~0x01ffffff },
>>>          { ZORRO_PROD_GVP_A530_SCSI,             ~0x01ffffff },
>>>          { ZORRO_PROD_GVP_COMBO_030_R4_SCSI,     ~0x01ffffff },
>>>          { ZORRO_PROD_GVP_A1291,                 ~0x07ffffff },
>>>          { ZORRO_PROD_GVP_GFORCE_040_SCSI_1,     ~0x07ffffff },
>>>          { 0 }
>>> };
>
> The default masks above were added (in some other form) in 2.1.91pre1
> (in 1998).  Before, people had to use gvp11_setup() to do that.

... because gvp11_setup() was already obsolete.

Thanks for dredging up that bit of history!

> So I think it is safe to assume there is no longer a need to configure
> this manually.

I'll take your word for that. No need to apply this patch, then!

(Apologies for the noise, Arnd ...)

Cheers,

	Michael




> Gr{oetje,eeting}s,
>
>                         Geert
>