[U-Boot] [PATCH 2/3] bouncebuf: Add DMA validation check to addr_aligned().

["Christoph Müllner" <christoph.muellner@theobroma-systems.com>]

On 07.05.19 17:56, Marek Vasut wrote:
> On 5/7/19 5:22 PM, Christoph Müllner wrote:
>>
>>
>> On 07.05.19 17:04, Marek Vasut wrote:
>>> On 5/7/19 4:01 PM, Christoph Müllner wrote:
>>>>
>>>>
>>>> On 07.05.19 15:53, Marek Vasut wrote:
>>>>> On 5/7/19 3:52 PM, Christoph Müllner wrote:
>>>>>>
>>>>>>
>>>>>> On 5/7/19 3:48 PM, Christoph Müllner wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 07.05.19 15:05, Marek Vasut wrote:
>>>>>>>> On 5/7/19 11:05 AM, Christoph Muellner wrote:
>>>>>>>>> Currently addr_aligned() performs an alignment and a length check
>>>>>>>>> to validate the DMA address. However, some machines have stricter
>>>>>>>>> restrictions of DMA-able addresses.
>>>>>>>>>
>>>>>>>>> This patch adds a call to mach_addr_is_dmaable() to honor this
>>>>>>>>> machine specific restrictions.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  common/bouncebuf.c | 6 ++++++
>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
>>>>>>>>> index a7098e2caf..26ddf30ea2 100644
>>>>>>>>> --- a/common/bouncebuf.c
>>>>>>>>> +++ b/common/bouncebuf.c
>>>>>>>>> @@ -26,6 +26,12 @@ static int addr_aligned(struct bounce_buffer *state)
>>>>>>>>>  		return 0;
>>>>>>>>>  	}
>>>>>>>>>  
>>>>>>>>> +	/* Check if valid DMA address. */
>>>>>>>>> +	if (!mach_addr_is_dmaable((ulong)state->user_buffer)) {
>>>>>>>>
>>>>>>>> Is the cast necessary ?
>>>>>>>
>>>>>>> Of course not.
>>>>>>> Will be fixed in v2.
>>>>>>>
>>>>>>> Thanks!
>>>>>>
>>>>>> I take that back.
>>>>>> The cast is needed.
>>>>>> (And also done several times in this file)
>>>>>>
>>>>>> Otherwise you will get:
>>>>>>
>>>>>> common/bouncebuf.c: In function ‘addr_aligned’:
>>>>>> common/bouncebuf.c:35:33: warning: passing argument 1 of ‘mach_addr_is_dmaable’ makes integer from pointer without a cast [-Wint-conversion]
>>>>>>   if (!mach_addr_is_dmaable(state->user_buffer)) {
>>>>>>                             ~~~~~^~~~~~~~~~~~~
>>>>>> In file included from include/common.h:64,
>>>>>>                  from common/bouncebuf.c:8:
>>>>>> include/init.h:128:40: note: expected ‘long unsigned int’ but argument is of type ‘void *’
>>>>>>  int mach_addr_is_dmaable(unsigned long addr);
>>>>>>                           ~~~~~~~~~~~~~~^~~~
>>>>>
>>>>> Shouldn't the function use void __iomem * in the first place ?
>>>>
>>>> IMHO Matter of taste.
>>>> If you prefer it this way, then I can change.
>>>> Doing grep "unsigned long addr" gave me enough confidence to use that.
>>>> But I leave this up to you.
>>>
>>> At least that's what Linux uses for mapped addresses.
>>
>> In Linux two versions of that function are needed: one for phys_addr_t and one for void *.
>> The linear mapping in U-Boot allows to combine both.
>> Therefore I see it as a matter of taste.
>>
>> I will change this to "void __iomem *" and cast to uintptr_t in the rk3399
>> implementation of that function in a v2 series.
> 
> Wait for more feedback please. Maybe someone has a more compelling
> argument for one or the other.

Too late, but I have no fear from a v3.