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

[Michael Schmitz <schmitzmic@gmail.com>]

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>