DMA_MEMSET definition confusion

DMA_MEMSET definition confusion

[Dave Jiang]

Hi Vinod, we are looking at implementing support for DMA memset in idxd driver and looking at the existing code, there seems to be some confusion as to the
expectation of the implementation. The input parameter for the pattern of ->device_prep_dma_memset() is an int, which mimics the POSIX memset() call.
And the way dmatest implemented, it passes in an u8 value as to 'int value' when calling ->device_prep_dma_memset().

I see 3 implementations in 3 of the in tree DMA drivers.

at_hdmac:
at_hdmac.c:
atc_prep_dma_memset()
Allocates a u32 DMA buffer and copies the 'int value' into the u32 memory.

at_xdmac:
at_xdmac.c:
at_xdmac_prep_dma_memset() -> at_xdmac_memset_create_desc()
assign value to desc->ldd.mbr_ds, which is a u32 data stride register

So these 2 drivers basically treats 'int value' as a 32bit pattern value.

hidma:
drivers/dma/qcom/hidma.c
hidma_prep_dma_set() -> hidma_ll_set_transfer_params() <== parm value passed in as third parameter

drivers/dma/qcom/hidma_ll.c:
hidma_llset_transfer_params(lldev, tre_ch, src, ....) <== int value casted to 'dma_addr_t src'
The function then calls lower_32_bits(src) and upper_32_bits(src) to program the DMA device. That just looks wrong.

None of these implementations would pass the dmatest data verification from inspecting the code AFAICT, and they deviate from the original intent of the
memset call IMHO.

So at this point given we have 2 implementations that expects a u32 value, should we just change 'int value' to 'u32 value', fix dmatest to a 32bit
pattern? Or do we introduce a memset32() call for those 2 implementations and specifically use a u32, and also a memset64() call for hidma so at least
it looks sane, and drop the original memset code since there would be no actual implementation for it? Or some other solution....?

For DSA, it takes a 64bit pattern in the descriptor. So we need to decide whether to go with replicating u8 to u64 or u32 to u64.

Thanks.

Re: DMA_MEMSET definition confusion

[Vinod Koul]

On 19-01-22, 12:53, Dave Jiang wrote:
> Hi Vinod, we are looking at implementing support for DMA memset in idxd driver and looking at the existing code, there seems to be some confusion as to the
> expectation of the implementation. The input parameter for the pattern of ->device_prep_dma_memset() is an int, which mimics the POSIX memset() call.
> And the way dmatest implemented, it passes in an u8 value as to 'int value' when calling ->device_prep_dma_memset().
> 
> I see 3 implementations in 3 of the in tree DMA drivers.
> 
> at_hdmac:
> at_hdmac.c:
> atc_prep_dma_memset()
> Allocates a u32 DMA buffer and copies the 'int value' into the u32 memory.
> 
> at_xdmac:
> at_xdmac.c:
> at_xdmac_prep_dma_memset() -> at_xdmac_memset_create_desc()
> assign value to desc->ldd.mbr_ds, which is a u32 data stride register
> 
> So these 2 drivers basically treats 'int value' as a 32bit pattern value.

IMO that is incorrect and should be fixed, the intent of memset should
is to provide an signed int value.

> 
> hidma:
> drivers/dma/qcom/hidma.c
> hidma_prep_dma_set() -> hidma_ll_set_transfer_params() <== parm value passed in as third parameter
> 
> drivers/dma/qcom/hidma_ll.c:
> hidma_llset_transfer_params(lldev, tre_ch, src, ....) <== int value casted to 'dma_addr_t src'
> The function then calls lower_32_bits(src) and upper_32_bits(src) to program the DMA device. That just looks wrong.
> 
> None of these implementations would pass the dmatest data verification from inspecting the code AFAICT, and they deviate from the original intent of the
> memset call IMHO.
> 
> So at this point given we have 2 implementations that expects a u32 value, should we just change 'int value' to 'u32 value', fix dmatest to a 32bit
> pattern? Or do we introduce a memset32() call for those 2 implementations and specifically use a u32, and also a memset64() call for hidma so at least
> it looks sane, and drop the original memset code since there would be no actual implementation for it? Or some other solution....?

we should have memset() take int and fix the users.

> For DSA, it takes a 64bit pattern in the descriptor. So we need to decide whether to go with replicating u8 to u64 or u32 to u64.
> 
> Thanks.
> 

-- 
~Vinod