Re: [PATCH] dma: altera-msgdma: Replace memcpy with io32write in msgdma_copy_one

[NG, TZE YEE]

On 21/5/2026 5:29 am, Frank Li wrote:
> [Some people who received this message don't often get email from frank.li@xxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Mon, May 18, 2026 at 11:47:20PM -0700, tze.yee.ng@xxxxxxxxxx wrote:
>> From: Adrian Ng Ho Yin <adrianhoyin.ng@xxxxxxxxxx>
>>
>> The descriptor FIFO requires that all words of a descriptor are written
>> in order, with the control word written last to flush it into the DMA
>> engine. Using memcpy() is unsafe since it does not guarantee ordering of
>> MMIO writes across all architectures.
>>
>> Replace memcpy() with an explicit iowrite32() loop for each 32-bit word
>> (except the control word). The control word is still written separately,
>> with write barriers, to ensure it is always the final word pushed into
>> the FIFO.
>>
>> This makes the programming of descriptors fully deterministic and
>> portable across different architectures.
>>
>> Signed-off-by: Adrian Ng Ho Yin <adrianhoyin.ng@xxxxxxxxxx>
>> Signed-off-by: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
>> ---
>>   drivers/dma/altera-msgdma.c | 21 ++++++++++++---------
>>   1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
>> index b46999c81df0..5816973d2c70 100644
>> --- a/drivers/dma/altera-msgdma.c
>> +++ b/drivers/dma/altera-msgdma.c
>> @@ -495,6 +495,9 @@ static void msgdma_copy_one(struct msgdma_device *mdev,
>>                            struct msgdma_sw_desc *desc)
>>   {
>>        void __iomem *hw_desc = mdev->desc;
>> +     const u32 *src = (const u32 *)&desc->hw_desc;
>> +     unsigned int i, nwords = offsetof(struct msgdma_extended_desc, control) /
>> +                              sizeof(u32);
>>
>>        /*
>>         * Check if the DESC FIFO it not full. If its full, we need to wait
>> @@ -505,16 +508,16 @@ static void msgdma_copy_one(struct msgdma_device *mdev,
>>                mdelay(1);
>>
>>        /*
>> -      * The descriptor needs to get copied into the descriptor FIFO
>> -      * of the DMA controller. The descriptor will get flushed to the
>> -      * FIFO, once the last word (control word) is written. Since we
>> -      * are not 100% sure that memcpy() writes all word in the "correct"
>> -      * order (address from low to high) on all architectures, we make
>> -      * sure this control word is written last by single coding it and
>> -      * adding some write-barriers here.
>> +      * The descriptor must be written into the descriptor FIFO of the DMA
>> +      * controller. The FIFO is flushed and the descriptor becomes valid once
>> +      * the last word (the control word) is written. To guarantee the ordering
>> +      * of MMIO writes across all architectures, we write each 32-bit word
>> +      * individually using iowrite32(), and handle the control word separately
>> +      * at the end. This ensures the control word is always written last and
>> +      * prevents memcpy() or the compiler from reordering accesses.
>>         */
>> -     memcpy((void __force *)hw_desc, &desc->hw_desc,
>> -            sizeof(desc->hw_desc) - sizeof(u32));
>> +     for (i = 0; i < nwords; i++)
>> +             iowrite32(src[i], hw_desc + i * sizeof(u32));
> 
> why not use memcpy_toio()?
> 
> Frank
>>
>>        /* Write control word last to flush this descriptor into the FIFO */
>>        mdev->idle = false;
>> --
>> 2.43.7
>>
Hi Frank,

Good point — the issue with the original code was memcpy() with __force 
to __iomem, not bulk MMIO copies in general. memcpy_toio() is the right 
API for the descriptor body as long as we still omit the control word 
and write it last with wmb() + iowrite32(), which the hardware uses to 
commit the descriptor into the FIFO.

I'll submit the v2 patch with memcpy_toio().

Thanks,
Tze Yee