[U-Boot] [PATCH v2 5/7] usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds

[Marek Vasut]

On 06/07/2017 10:03 AM, Dr. Philipp Tomsich wrote:
> 
>> On 07 Jun 2017, at 09:53, Marek Vasut <marex at denx.de> wrote:
>>
>> On 06/07/2017 09:49 AM, Dr. Philipp Tomsich wrote:
>>>> On 07 Jun 2017, at 08:28, Marek Vasut <marex at denx.de
>>>> <mailto:marex at denx.de>> wrote:
>>>>
>>>> On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote:
>>>>>
>>>>>> On 06 Jun 2017, at 16:35, Marek Vasut <marex at denx.de
>>>>>> <mailto:marex at denx.de>> wrote:
>>>>>>
>>>>>> On 06/06/2017 03:42 PM, Philipp Tomsich wrote:
>>>>>>> For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable
>>>>>>> when (truncating and) writing into the controller's registers is
>>>>>>> unsafe and triggers the following compiler warning (thus failing by
>>>>>>> buildman tests):
>>>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx':
>>>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast
>>>>>>> from pointer to integer of different size [-Wpointer-to-int-cast]
>>>>>>>  writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>>>>>         ^
>>>>>>>
>>>>>>> This change fixes the warning and makes the code a bit more robust by
>>>>>>> doing the following:
>>>>>>> - we check whether ep->dma_buf can be safely truncated to 32 bits
>>>>>>> (i.e. whether dma_buf is the same value when masked to 32 bits) and
>>>>>>> emit an error message if that is not the case
>>>>>>> - we cast to a uintptr_t and let the writel macro truncate the
>>>>>>> uintptr_t (potentially a 64bit value) to 32bits
>>>>>>>
>>>>>>> Signed-off-by: Philipp Tomsich
>>>>>>> <philipp.tomsich at theobroma-systems.com
>>>>>>> <mailto:philipp.tomsich at theobroma-systems.com>>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - (new patch) fix warnings and add some robustness for the truncation
>>>>>>> of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
>>>>>>> for u-boot-rockchip/master at 2b19b2f
>>>>>>>
>>>>>>> drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++-
>>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>>> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>>> index 0d6d2fb..f5c926c 100644
>>>>>>> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>>> @@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep,
>>>>>>> struct dwc2_request *req)
>>>>>>> invalidate_dcache_range((unsigned long) ep->dma_buf,
>>>>>>> (unsigned long) ep->dma_buf + ep->len);
>>>>>>>
>>>>>>> -writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>>>>> +/* ensure that ep->dma_buf fits into 32 bits */
>>>>>>> +if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf)
>>>>>>> +error("ep->dma_buf does not fit into a 32 bits");
>>>>>>
>>>>>> You print an error, but still continue with the function ? That'll
>>>>>> probably lead to a crash, you should rather abort. In fact, I suspect
>>>>>> you can use bounce-buffer to assure the dma buffer is in 32bit space.
>>>>>
>>>>> This tries to stay close to the current behaviour (we have no
>>>>> hardware to test this,
>>>>> but I wanted to get it out of the way as it caused buildman-failures)
>>>>> and to provide
>>>>> diagnostics for whoever first encounters an issue here.
>>>>>
>>>>> Note that I would only expect a DMA error, if someone really hit this
>>>>> issue ...
>>>>
>>>> So this patch is just some hypothetical fix ? What triggered creation of
>>>> this patch ?
>>>
>>> Running buildman (w/ a GCC 6.3 configured as aarch-unknown-elf) against
>>> unrelated
>>> patch-series (for a Rockchip device that has a different USB controller)
>>> fails due to the
>>> warnings raised from this. Just casting those warnings away (without
>>> having an
>>> “assertion-like” diagnostic printed) doesn’t sound like the best plan
>>> either, though. 
>>>
>>> Details of the buildman-failure are in the above commit-message.
>>
>> Ah, hm. In that case, you should really implement the bounce buffer here
>> I think. Letting the transfer continue will lead to really weird
>> failures and/or memory corruption.
> 
> Alright. I’ll put it to (the back of) my queue and resubmit.
> Someone (with hardware) will need to test, once it’s submitted.

That's fine, thanks.

-- 
Best regards,
Marek Vasut