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

Michael Schmitz
2023-08-30T00:48:20Z

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>