[U-Boot] [PATCH v4 1/3] net: designware: fix descriptor layout and warnings on 64-bit archs

[Alexander Graf <agraf@suse.de>]

On 04/18/2016 12:57 PM, Beniamino Galvani wrote:
> On Sun, Apr 17, 2016 at 10:59:11PM +0200, Marek Vasut wrote:
>> On 04/17/2016 01:14 PM, Beniamino Galvani wrote:
>>> On Sun, Apr 17, 2016 at 11:56:58AM +0200, Marek Vasut wrote:
>>>>> -		desc_p->dmamac_addr = &txbuffs[idx * CONFIG_ETH_BUFSIZE];
>>>>> -		desc_p->dmamac_next = &desc_table_p[idx + 1];
>>>>> +		desc_p->dmamac_addr = (ulong)&txbuffs[idx * CONFIG_ETH_BUFSIZE];
>>>>> +		desc_p->dmamac_next = (ulong)&desc_table_p[idx + 1];
>>>> Why don't you use u32 instead of ulong ? The u32 is well defined.
>>>> DTTO all over the place.
>>> &txbuffs[idx * CONFIG_ETH_BUFSIZE] is a pointer (and hence has the
>>> size of a ulong) and casting it to u32 would give a warning on 64 bit
>>> archs ("cast from pointer to integer of different size").
>> Will cast to uintptr_t and then to u32 help ?
> Note that uintptr_t is defined as ulong and the second cast to u32 is
> not needed because C does not require casts between arithmetic
> types. So I don't see much difference.
>
>> It's just a feeling, but casting to ulong just to circumvent compiler
>> warning does not sound right.
> It seems fine to me, the (ulong) is needed to cast the pointer to an
> arithmetic type of equivalent size which then can be assigned to an
> u32 variable.
>
>>>> btw just curious, but what will happen if the descriptors get allocated
>>>> in area above 4GiB ? Will the code silently corrupt memory by discarding
>>>> the top bits in the descriptor pointer?
>>> No, if the driver private structure (which contains buffers and
>>> descriptors) is above 4GiB, designware_initialize() will complain and
>>> return an error.
>> Which code checks that ?
>   +       if ((unsigned long long)priv + sizeof(*priv) > (1ULL << 32)) {
>   +               printf("designware: buffers are outside DMA memory\n");
>   +               return -EINVAL;
>   +       }
>   +

Hmm, this is going to get very interesting with efi_loader support. By 
default we allocate memory at the highest possible free address, so 
payloads will probably (unless they specify limits) have their buffers 
above 32bit on this platform. If we now deny any DMA to them, we 
basically break I/O access.

Could you by any chance just use a bounce buffer?


Alex