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@xxxxxxxxxxxxxxx>
>> --
>> CC: Ludovic Desroches <ludovic.desroches@xxxxxxxxx>
>> CC: Vinod Koul <vinod.koul@xxxxxxxxx>
>> CC: Dan Williams <dan.j.williams@xxxxxxxxx>
>> CC: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> CC: dmaengine@xxxxxxxxxxxxxxx
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
