Re: [PATCH v1 2/3] scsi - a2091.c: convert m68k WD33C93 drivers to DMA API

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Arnd,

thanks for your review!

Am 29.06.2022 um 18:06 schrieb Arnd Bergmann:
> On Wed, Jun 29, 2022 at 3:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>
>> Use dma_map_single() for a2091 driver (leave bounce buffer
>> logic unchanged).
>>
>> Use dma_set_mask_and_coherent() to avoid explicit cache
>> flushes.
>>
>> Compile-tested only.
>>
>> CC: linux-scsi@vger.kernel.org
>> Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>
>> +
>> +       addr = dma_map_single(hdata->dev, scsi_pointer->ptr,
>> +                             len, DMA_DIR(dir_in));
>> +       if (dma_mapping_error(hdata->dev, addr)) {
>> +               dev_warn(hdata->dev, "cannot map SCSI data block %p\n",
>> +                        scsi_pointer->ptr);
>> +               return 1;
>> +       }
>> +       scsi_pointer->dma_handle = addr;
>>
>>         /* don't allow DMA if the physical address is bad */
>>         if (addr & A2091_XFER_MASK) {
>> +               /* drop useless mapping */
>> +               dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
>> +                                scsi_pointer->this_residual,
>> +                                DMA_DIR(dir_in));
>
> I think you could save the extra map/unmap here if you wanted, but that
> would risk introducing bugs since it requires a larger rework.

Not sure how to do that ...

>> +               scsi_pointer->dma_handle = (dma_addr_t) NULL;
>> +
>>                 wh->dma_bounce_len = (scsi_pointer->this_residual + 511) & ~0x1ff;
>>                 wh->dma_bounce_buffer = kmalloc(wh->dma_bounce_len,
>>                                                 GFP_KERNEL);
>
> Not your bug, but if there is memory above the A2091_XFER_MASK limit,
> this would need to use GFP_DMA instead of GFP_KERNEL.

Same argument goes for gvp11 - though last time I checked (and certainly 
at the time these drivers were written), there really was no difference 
between using GFP_DMA and GFP_KERNEL on m68k, hence the need to check 
the allocated buffer is indeed suitable for DMA, and perhaps revert to 
chip RAM (slow, useless for most other purposes but definitely below 16 
MB) if that fails (as the gvp11 driver does).

Most users will opt for loading the kernel to a RAM chunk at a higher 
physical address, making the lower chunk unusable (except as chip RAM), 
meaning allocating a bounce buffer within the first 16 MB will most 
likely fail anyway AIUI (but Geert would know that for sure).

Cheers,

	Michael

>
>          Arnd
>