| Message ID | 20230829214517.14448-1-schmitzmic@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | scsi: gvp11: add module parameter for DMA transfer bit mask | expand |
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
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
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>
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>
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
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 >
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);
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(+)