[PATCH 3/3] dma: at_xdmac: make all descriptors little endian

[Ben Dooks]

On 26/03/15 17:05, Russell King - ARM Linux wrote:
> On Thu, Mar 26, 2015 at 01:06:31PM +0000, Ben Dooks wrote:
>> Always write the descriptors for the at_xdmac in little endian when
>> the processor is running big endian.
>>
>> Signed-off-by: Ben Dooks <ben.dooks at codethink.co.uk>
>> --
>> CC: Ludovic Desroches <ludovic.desroches at atmel.com>
>> CC: Vinod Koul <vinod.koul at intel.com>
>> CC: Dan Williams <dan.j.williams at intel.com>
>> CC: linux-arm-kernel at lists.infradead.org
>> CC: dmaengine at vger.kernel.org
>> ---
>>  drivers/dma/at_xdmac.c | 97 ++++++++++++++++++++++++++------------------------
>>  1 file changed, 51 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
>> index d9891d3..65a37be 100644
>> --- a/drivers/dma/at_xdmac.c
>> +++ b/drivers/dma/at_xdmac.c
>> @@ -232,10 +232,10 @@ struct at_xdmac {
>>  /* Linked List Descriptor */
>>  struct at_xdmac_lld {
>>  	dma_addr_t	mbr_nda;	/* Next Descriptor Member */
>> -	u32		mbr_ubc;	/* Microblock Control Member */
>> +	__le32		mbr_ubc;	/* Microblock Control Member */
>>  	dma_addr_t	mbr_sa;		/* Source Address Member */
>>  	dma_addr_t	mbr_da;		/* Destination Address Member */
>> -	u32		mbr_cfg;	/* Configuration Register */
>> +	__le32		mbr_cfg;	/* Configuration Register */
>>  };
> 
> This /really/ is not correct if this structure is describing something
> parsed by the hardware - I mean, your patch itself may be correct
> but it's showing that there's more problems here.
> 
> The reason is those dma_addr_t's.  dma_addr_t can be either 32-bit or
> 64-bit depending on the kernel configuration, and I really suspect that
> the hardware doesn't get to know how the kernel was configured.  That
> goes for any structure which is passed to hardware - dma_addr_t should
> never appear in it _anywhere_.
> 
> As you're converting it to __le32, I suspec those DMA addresses are
> also supposed to be __le32 quantities as well.
> 
>> +			desc->lld.mbr_sa = cpu_to_le32(atchan->per_src_addr);
>> +			desc->lld.mbr_da = cpu_to_le32(mem);
> 
> This kind'a confirms it - but what happens to the above if dma_addr_t
> is 64-bit and has some high bits set?  Should be silently truncate the
> value?

I thought that they may need changing, but this is a good reason to
go and change them from dma_addr_t to __le32 quantities.

>>  		dev_dbg(chan2dev(chan),
>>  			 "%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n",
>>  			 __func__, &desc->lld.mbr_sa, &desc->lld.mbr_da, desc->lld.mbr_ubc);
>>  
>>  		/* Chain lld. */
>>  		if (prev) {
>> -			prev->lld.mbr_nda = desc->tx_dma_desc.phys;
>> +			prev->lld.mbr_nda = cpu_to_le32(desc->tx_dma_desc.phys);
> 
> Another point to be raised with the original authors... get rid of this
> "phys" notation.  It's not physical.  It's an address which is specific
> to the DMA controller, but which _may_ happen to be the same as a
> physical address.

Thanks for the feedback. I'll look into how much of a change making
these be .dma_addr instead of .phys

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius