Message ID | 1496756553-19901-6-git-send-email-philipp.tomsich@theobroma-systems.com |
---|---|
State | Changes Requested |
Delegated to: | Simon Glass |
Headers | show |
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, ®->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@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@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, ®->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. > + writel((uintptr_t)ep->dma_buf, ®->out_endp[ep_num].doepdma); > writel(DOEPT_SIZ_PKT_CNT(pktcnt) | DOEPT_SIZ_XFER_SIZE(length), > ®->out_endp[ep_num].doeptsiz); > writel(DEPCTL_EPENA|DEPCTL_CNAK|ctrl, ®->out_endp[ep_num].doepctl); >
Marek, > On 06 Jun 2017, at 16:35, Marek Vasut <marex@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, ®->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@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@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, ®->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 ... Regards, Philipp. >> + writel((uintptr_t)ep->dma_buf, ®->out_endp[ep_num].doepdma); >> writel(DOEPT_SIZ_PKT_CNT(pktcnt) | DOEPT_SIZ_XFER_SIZE(length), >> ®->out_endp[ep_num].doeptsiz); >> writel(DEPCTL_EPENA|DEPCTL_CNAK|ctrl, ®->out_endp[ep_num].doepctl); >> > > > -- > Best regards, > Marek Vasut
On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote: > Marek, > >> On 06 Jun 2017, at 16:35, Marek Vasut <marex@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, ®->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@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@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, ®->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 ?
> On 07 Jun 2017, at 08:28, Marek Vasut <marex@denx.de> wrote: > > On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote: >> >>> On 06 Jun 2017, at 16:35, Marek Vasut <marex@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, ®->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@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@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, ®->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. Regards, Philipp.
On 06/07/2017 09:49 AM, Dr. Philipp Tomsich wrote: >> On 07 Jun 2017, at 08:28, Marek Vasut <marex@denx.de >> <mailto:marex@denx.de>> wrote: >> >> On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote: >>> >>>> On 06 Jun 2017, at 16:35, Marek Vasut <marex@denx.de >>>> <mailto:marex@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, ®->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@theobroma-systems.com >>>>> <mailto:philipp.tomsich@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@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, ®->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.
> On 07 Jun 2017, at 09:53, Marek Vasut <marex@denx.de> wrote: > > On 06/07/2017 09:49 AM, Dr. Philipp Tomsich wrote: >>> On 07 Jun 2017, at 08:28, Marek Vasut <marex@denx.de >>> <mailto:marex@denx.de>> wrote: >>> >>> On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote: >>>> >>>>> On 06 Jun 2017, at 16:35, Marek Vasut <marex@denx.de >>>>> <mailto:marex@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, ®->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@theobroma-systems.com >>>>>> <mailto:philipp.tomsich@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@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, ®->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. Cheers, Philipp.
On 06/07/2017 10:03 AM, Dr. Philipp Tomsich wrote: > >> On 07 Jun 2017, at 09:53, Marek Vasut <marex@denx.de> wrote: >> >> On 06/07/2017 09:49 AM, Dr. Philipp Tomsich wrote: >>>> On 07 Jun 2017, at 08:28, Marek Vasut <marex@denx.de >>>> <mailto:marex@denx.de>> wrote: >>>> >>>> On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote: >>>>> >>>>>> On 06 Jun 2017, at 16:35, Marek Vasut <marex@denx.de >>>>>> <mailto:marex@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, ®->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@theobroma-systems.com >>>>>>> <mailto:philipp.tomsich@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@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, ®->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.
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, ®->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"); + + writel((uintptr_t)ep->dma_buf, ®->out_endp[ep_num].doepdma); writel(DOEPT_SIZ_PKT_CNT(pktcnt) | DOEPT_SIZ_XFER_SIZE(length), ®->out_endp[ep_num].doeptsiz); writel(DEPCTL_EPENA|DEPCTL_CNAK|ctrl, ®->out_endp[ep_num].doepctl);
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, ®->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@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@2b19b2f drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)