Re: [PATCH] spi: spi_amd: Add HIDDMA basic write support

["Rangoju, Raju" <raju.rangoju@amd.com>]

On 5/13/2025 12:04 AM, Geert Uytterhoeven wrote:
> Hi Raju,
> 
> On Mon, 12 May 2025 at 19:55, Rangoju, Raju <raju.rangoju@amd.com> wrote:
>> On 5/12/2025 7:47 PM, Geert Uytterhoeven wrote:
>>> On Mon, 12 May 2025 at 09:29, Rangoju, Raju <raju.rangoju@amd.com> wrote:
>>>> On 5/11/2025 3:51 AM, kernel test robot wrote:
>>>>> kernel test robot noticed the following build warnings:
>>>>>
>>>>> [auto build test WARNING on v6.15-rc5]
>>>>> [also build test WARNING on linus/master]
>>>>> [cannot apply to broonie-spi/for-next next-20250509]
>>>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>>>> And when submitting patch, we suggest to use '--base' as documented in
>>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>>
>>>>> url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Rangoju/spi-spi_amd-Add-HIDDMA-basic-write-support/20250510-021954
>>>>> base:   v6.15-rc5
>>>>> patch link:    https://lore.kernel.org/r/20250509181737.997167-1-Raju.Rangoju%40amd.com
>>>>> patch subject: [PATCH] spi: spi_amd: Add HIDDMA basic write support
>>>>> config: m68k-randconfig-r111-20250511 (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/config)
>>>>> compiler: m68k-linux-gcc (GCC) 14.2.0
>>>>
>>>> Thanks for reporting this. We do not support m68k.
>>>
>>> All write[bwlq]() functions take a volatile void __iomem pointer
>>> (https://elixir.bootlin.com/linux/v6.14.6/source/include/asm-generic/io.h#L174)
>>> while you are passing a void *, so sparse should complain about this
>>> on all architectures.
>>
>> My bad, with the following flags included, sparse now complains this on
>> all architectures.
>>
>> -fmax-errors=unlimited -fmax-warnings=unlimited
>>
>> And sparse is right, this driver is using MMIO
>>> accessors on allocated DMA memory, which is just plain wrong:
>>>
>>>       amd_spi->dma_virt_addr = dma_alloc_coherent(dev, AMD_SPI_HID2_DMA_SIZE,
>>>               &amd_spi->phy_dma_buf, GFP_KERNEL);
>>>
>>>        for (i = 0; left_data >= 8; i++, left_data -= 8)
>>>               *buf_64++ = readq((u8 __iomem *)amd_spi->dma_virt_addr + (i * 8));
>>>
>>>> Will re-spin v2 with necessary changes in Kconfig.
>>>
>>> Please fix the real issue instead ;-)
>>
>> We are using read*/write* calls instead of memcpy to copy data to/from
>> DMA memory due to performance concerns, as we observed better throughput
>> during continuous read/write compared to the memcpy functions.
>
Hi Geert,

> Perhaps your memcpy() copies backwards?

Nope. The Source and destinations are in different address range, so we 
do not do backward copying.

> https://lwn.net/Articles/1016300/
> 
> There is no guarantee that read*/write* calls work on normal RAM on
> all architectures. It may just crash, as some architectures return
> cookies instead of real pointers when mapping MMIO.

Okay. We will copy the data to/from the DMA buffer by iterating 
(ensuring that memory access is safe) by avoiding MMIO accessor usage 
(`read*`/`write*`).

For example:

u64 *dma_buff = (u64 *)amd_spi->dma_virt_addr;
...
for (i = 0; i < nbytes / 8; i++)
     *dma_buff++ = *buf_64++;

We will re-spin V2 with these changes.

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