| Message ID | 20170401065139.12084-1-eddie.cai.linux@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
On Samstag, 1. April 2017 08:51:39 CEST Eddie Cai wrote: > We should invalidate the dcache before starting the DMA. In case there are > any dirty lines from the DMA buffer in the cache, subsequent cache-line > replacements may corrupt the buffer in memory while the DMA is still going > on. Cache-line replacement can happen if the CPU tries to bring some other > memory locations into the cache while the DMA is going on. > > Signed-off-by: Eddie Cai <eddie.cai.linux@gmail.com> Can you please run the patch through checkpatch, I can at least spot some missing whitespace. You can add my Reviewed-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> to your v2. Kind regards, Stefan > --- > drivers/usb/host/dwc2.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c > index 5ac602e..a151ba6 100644 > --- a/drivers/usb/host/dwc2.c > +++ b/drivers/usb/host/dwc2.c > @@ -811,12 +811,17 @@ static int transfer_chunk(struct dwc2_hc_regs > *hc_regs, void *aligned_buffer, (*pid << DWC2_HCTSIZ_PID_OFFSET), > &hc_regs->hctsiz); > > - if (!in && xfer_len) { > - memcpy(aligned_buffer, buffer, xfer_len); > - > - flush_dcache_range((unsigned long)aligned_buffer, > - (unsigned long)aligned_buffer + > - roundup(xfer_len, ARCH_DMA_MINALIGN)); > + if (xfer_len) { > + if(in){ > + invalidate_dcache_range((unsigned long)aligned_buffer, > + (unsigned long)aligned_buffer + > + roundup(xfer_len, ARCH_DMA_MINALIGN)); > + }else{ > + memcpy(aligned_buffer, buffer, xfer_len); > + flush_dcache_range((unsigned long)aligned_buffer, > + (unsigned long)aligned_buffer + > + roundup(xfer_len, ARCH_DMA_MINALIGN)); > + } > } > > writel(phys_to_bus((unsigned long)aligned_buffer), &hc_regs->hcdma);
On 04/03/2017 04:43 PM, Brüns, Stefan wrote: > On Samstag, 1. April 2017 08:51:39 CEST Eddie Cai wrote: >> We should invalidate the dcache before starting the DMA. In case there are >> any dirty lines from the DMA buffer in the cache, subsequent cache-line >> replacements may corrupt the buffer in memory while the DMA is still going >> on. Cache-line replacement can happen if the CPU tries to bring some other >> memory locations into the cache while the DMA is going on. >> >> Signed-off-by: Eddie Cai <eddie.cai.linux@gmail.com> > > Can you please run the patch through checkpatch, I can at least spot some > missing whitespace. > > You can add my > > Reviewed-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > to your v2. Works for me too, looking forward to v2.
Hi Eddie,
We should only need to do only one time cache operation for a buffer
ready to do DMA transfer, so you need to remove another cache invalidate
operation for the same buffer in the same function.
Thanks,
- Kever
On 04/01/2017 02:51 PM, Eddie Cai wrote:
> We should invalidate the dcache before starting the DMA. In case there are
> any dirty lines from the DMA buffer in the cache, subsequent cache-line
> replacements may corrupt the buffer in memory while the DMA is still going on.
> Cache-line replacement can happen if the CPU tries to bring some other memory
> locations into the cache while the DMA is going on.
>
> Signed-off-by: Eddie Cai <eddie.cai.linux@gmail.com>
> ---
> drivers/usb/host/dwc2.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> index 5ac602e..a151ba6 100644
> --- a/drivers/usb/host/dwc2.c
> +++ b/drivers/usb/host/dwc2.c
> @@ -811,12 +811,17 @@ static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer,
> (*pid << DWC2_HCTSIZ_PID_OFFSET),
> &hc_regs->hctsiz);
>
> - if (!in && xfer_len) {
> - memcpy(aligned_buffer, buffer, xfer_len);
> -
> - flush_dcache_range((unsigned long)aligned_buffer,
> - (unsigned long)aligned_buffer +
> - roundup(xfer_len, ARCH_DMA_MINALIGN));
> + if (xfer_len) {
> + if(in){
> + invalidate_dcache_range((unsigned long)aligned_buffer,
> + (unsigned long)aligned_buffer +
> + roundup(xfer_len, ARCH_DMA_MINALIGN));
> + }else{
> + memcpy(aligned_buffer, buffer, xfer_len);
> + flush_dcache_range((unsigned long)aligned_buffer,
> + (unsigned long)aligned_buffer +
> + roundup(xfer_len, ARCH_DMA_MINALIGN));
> + }
> }
>
> writel(phys_to_bus((unsigned long)aligned_buffer), &hc_regs->hcdma);
Hi, On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: > Hi Eddie, > > > We should only need to do only one time cache operation for a buffer > > ready to do DMA transfer, so you need to remove another cache invalidate > > operation for the same buffer in the same function. I think this is a more general problem and might cause issues with other drivers. So I have sent this patch: http://patchwork.ozlabs.org/patch/746917/ Regards, Simon
On 04/05/2017 04:21 AM, Simon Glass wrote: > Hi, > > On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >> Hi Eddie, >> >> >> We should only need to do only one time cache operation for a buffer >> >> ready to do DMA transfer, so you need to remove another cache invalidate >> >> operation for the same buffer in the same function. > > I think this is a more general problem and might cause issues with > other drivers. So I have sent this patch: > > http://patchwork.ozlabs.org/patch/746917/ > This feels like papering over a problem though ... which will bite you later anyway.
Hi Marek, On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: > On 04/05/2017 04:21 AM, Simon Glass wrote: >> Hi, >> >> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>> Hi Eddie, >>> >>> >>> We should only need to do only one time cache operation for a buffer >>> >>> ready to do DMA transfer, so you need to remove another cache invalidate >>> >>> operation for the same buffer in the same function. >> >> I think this is a more general problem and might cause issues with >> other drivers. So I have sent this patch: >> >> http://patchwork.ozlabs.org/patch/746917/ >> > > This feels like papering over a problem though ... which will bite you > later anyway. I believe the problem only happens because we have cached zero bytes caused by this function. If the driver does the right thing (as dwc2.c already does) then everything should be find from then on. Notice that the problem does not happen without driver model, since non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with the cache off. Regards, Simon
On 04/05/2017 12:08 PM, Simon Glass wrote: > Hi Marek, > > On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >> On 04/05/2017 04:21 AM, Simon Glass wrote: >>> Hi, >>> >>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>> Hi Eddie, >>>> >>>> >>>> We should only need to do only one time cache operation for a buffer >>>> >>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>> >>>> operation for the same buffer in the same function. >>> >>> I think this is a more general problem and might cause issues with >>> other drivers. So I have sent this patch: >>> >>> http://patchwork.ozlabs.org/patch/746917/ >>> >> >> This feels like papering over a problem though ... which will bite you >> later anyway. > > I believe the problem only happens because we have cached zero bytes > caused by this function. If the driver does the right thing (as dwc2.c > already does) then everything should be find from then on. And I think the driver is where this should be fixed ? That is, the driver should do the right thing and flush/invalidate caches correctly. > Notice that the problem does not happen without driver model, since > non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with > the cache off.
+Tom Hi Marek, On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote: > On 04/05/2017 12:08 PM, Simon Glass wrote: >> Hi Marek, >> >> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >>> On 04/05/2017 04:21 AM, Simon Glass wrote: >>>> Hi, >>>> >>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>>> Hi Eddie, >>>>> >>>>> >>>>> We should only need to do only one time cache operation for a buffer >>>>> >>>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>>> >>>>> operation for the same buffer in the same function. >>>> >>>> I think this is a more general problem and might cause issues with >>>> other drivers. So I have sent this patch: >>>> >>>> http://patchwork.ozlabs.org/patch/746917/ >>>> >>> >>> This feels like papering over a problem though ... which will bite you >>> later anyway. >> >> I believe the problem only happens because we have cached zero bytes >> caused by this function. If the driver does the right thing (as dwc2.c >> already does) then everything should be find from then on. > > And I think the driver is where this should be fixed ? That is, the > driver should do the right thing and flush/invalidate caches correctly. > >> Notice that the problem does not happen without driver model, since >> non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with >> the cache off. I'm not sure if you read the long and windy thread with Stefan B but the upshot is that the driver is doing the right thing. If the driver were doing the memset() then you could make a case that we should change the driver, but since DM is doing it and DM is promising that DMA can be used on the buffer, I think the best place for the fix is in DM. The driver should not need to change and neither should any other driver when we convert it to DM. On that last point I really want to avoid having to change the caching behaviour of drivers just to work with DM! Regards, Simon
On 04/05/2017 05:03 PM, Simon Glass wrote: > +Tom > > Hi Marek, > > On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote: >> On 04/05/2017 12:08 PM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >>>> On 04/05/2017 04:21 AM, Simon Glass wrote: >>>>> Hi, >>>>> >>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>>>> Hi Eddie, >>>>>> >>>>>> >>>>>> We should only need to do only one time cache operation for a buffer >>>>>> >>>>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>>>> >>>>>> operation for the same buffer in the same function. >>>>> >>>>> I think this is a more general problem and might cause issues with >>>>> other drivers. So I have sent this patch: >>>>> >>>>> http://patchwork.ozlabs.org/patch/746917/ >>>>> >>>> >>>> This feels like papering over a problem though ... which will bite you >>>> later anyway. >>> >>> I believe the problem only happens because we have cached zero bytes >>> caused by this function. If the driver does the right thing (as dwc2.c >>> already does) then everything should be find from then on. >> >> And I think the driver is where this should be fixed ? That is, the >> driver should do the right thing and flush/invalidate caches correctly. >> >>> Notice that the problem does not happen without driver model, since >>> non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with >>> the cache off. > > I'm not sure if you read the long and windy thread with Stefan B but > the upshot is that the driver is doing the right thing. > > If the driver were doing the memset() then you could make a case that > we should change the driver, but since DM is doing it and DM is > promising that DMA can be used on the buffer, I think the best place > for the fix is in DM. The driver should not need to change and neither > should any other driver when we convert it to DM. > > On that last point I really want to avoid having to change the caching > behaviour of drivers just to work with DM! So will the driver work with your patch and without DM ? I don't think so, so I think what Eddie's patch does is correct. I'd really like him to send a new version and apply that. If this also needs to be fixed in DM, so be it. > Regards, > Simon >
Hi Marek, On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote: > On 04/05/2017 05:03 PM, Simon Glass wrote: >> +Tom >> >> Hi Marek, >> >> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote: >>> On 04/05/2017 12:08 PM, Simon Glass wrote: >>>> Hi Marek, >>>> >>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >>>>> On 04/05/2017 04:21 AM, Simon Glass wrote: >>>>>> Hi, >>>>>> >>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>>>>> Hi Eddie, >>>>>>> >>>>>>> >>>>>>> We should only need to do only one time cache operation for a buffer >>>>>>> >>>>>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>>>>> >>>>>>> operation for the same buffer in the same function. >>>>>> >>>>>> I think this is a more general problem and might cause issues with >>>>>> other drivers. So I have sent this patch: >>>>>> >>>>>> http://patchwork.ozlabs.org/patch/746917/ >>>>>> >>>>> >>>>> This feels like papering over a problem though ... which will bite you >>>>> later anyway. >>>> >>>> I believe the problem only happens because we have cached zero bytes >>>> caused by this function. If the driver does the right thing (as dwc2.c >>>> already does) then everything should be find from then on. >>> >>> And I think the driver is where this should be fixed ? That is, the >>> driver should do the right thing and flush/invalidate caches correctly. >>> >>>> Notice that the problem does not happen without driver model, since >>>> non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with >>>> the cache off. >> >> I'm not sure if you read the long and windy thread with Stefan B but >> the upshot is that the driver is doing the right thing. >> >> If the driver were doing the memset() then you could make a case that >> we should change the driver, but since DM is doing it and DM is >> promising that DMA can be used on the buffer, I think the best place >> for the fix is in DM. The driver should not need to change and neither >> should any other driver when we convert it to DM. >> >> On that last point I really want to avoid having to change the caching >> behaviour of drivers just to work with DM! > > So will the driver work with your patch and without DM ? I don't think > so, so I think what Eddie's patch does is correct. I'd really like him > to send a new version and apply that. Yes the driver work fine without DM and the code is correct. The buffer is in BSS in that case and we don't have the cache problem. I think we would have seen this problem before :-) > > If this also needs to be fixed in DM, so be it. OK. Regards, Simon
On 04/06/2017 03:24 AM, Simon Glass wrote: > Hi Marek, > > On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote: >> On 04/05/2017 05:03 PM, Simon Glass wrote: >>> +Tom >>> >>> Hi Marek, >>> >>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote: >>>> On 04/05/2017 12:08 PM, Simon Glass wrote: >>>>> Hi Marek, >>>>> >>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>>>>>> Hi Eddie, >>>>>>>> >>>>>>>> >>>>>>>> We should only need to do only one time cache operation for a buffer >>>>>>>> >>>>>>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>>>>>> >>>>>>>> operation for the same buffer in the same function. >>>>>>> >>>>>>> I think this is a more general problem and might cause issues with >>>>>>> other drivers. So I have sent this patch: >>>>>>> >>>>>>> http://patchwork.ozlabs.org/patch/746917/ >>>>>>> >>>>>> >>>>>> This feels like papering over a problem though ... which will bite you >>>>>> later anyway. >>>>> >>>>> I believe the problem only happens because we have cached zero bytes >>>>> caused by this function. If the driver does the right thing (as dwc2.c >>>>> already does) then everything should be find from then on. >>>> >>>> And I think the driver is where this should be fixed ? That is, the >>>> driver should do the right thing and flush/invalidate caches correctly. >>>> >>>>> Notice that the problem does not happen without driver model, since >>>>> non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with >>>>> the cache off. >>> >>> I'm not sure if you read the long and windy thread with Stefan B but >>> the upshot is that the driver is doing the right thing. >>> >>> If the driver were doing the memset() then you could make a case that >>> we should change the driver, but since DM is doing it and DM is >>> promising that DMA can be used on the buffer, I think the best place >>> for the fix is in DM. The driver should not need to change and neither >>> should any other driver when we convert it to DM. >>> >>> On that last point I really want to avoid having to change the caching >>> behaviour of drivers just to work with DM! >> >> So will the driver work with your patch and without DM ? I don't think >> so, so I think what Eddie's patch does is correct. I'd really like him >> to send a new version and apply that. > > Yes the driver work fine without DM and the code is correct. The > buffer is in BSS in that case and we don't have the cache problem. I > think we would have seen this problem before :-) I am seeing problems around this code and this patch makes sense to me, so I think this patch should go in as well ... >> >> If this also needs to be fixed in DM, so be it. > > OK. > > Regards, > Simon >
Hi Marek, On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote: > On 04/06/2017 03:24 AM, Simon Glass wrote: >> Hi Marek, >> >> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote: >>> On 04/05/2017 05:03 PM, Simon Glass wrote: >>>> +Tom >>>> >>>> Hi Marek, >>>> >>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote: >>>>> On 04/05/2017 12:08 PM, Simon Glass wrote: >>>>>> Hi Marek, >>>>>> >>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>>>>>>> Hi Eddie, >>>>>>>>> >>>>>>>>> >>>>>>>>> We should only need to do only one time cache operation for a buffer >>>>>>>>> >>>>>>>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>>>>>>> >>>>>>>>> operation for the same buffer in the same function. >>>>>>>> >>>>>>>> I think this is a more general problem and might cause issues with >>>>>>>> other drivers. So I have sent this patch: >>>>>>>> >>>>>>>> http://patchwork.ozlabs.org/patch/746917/ >>>>>>>> >>>>>>> >>>>>>> This feels like papering over a problem though ... which will bite you >>>>>>> later anyway. >>>>>> >>>>>> I believe the problem only happens because we have cached zero bytes >>>>>> caused by this function. If the driver does the right thing (as dwc2.c >>>>>> already does) then everything should be find from then on. >>>>> >>>>> And I think the driver is where this should be fixed ? That is, the >>>>> driver should do the right thing and flush/invalidate caches correctly. >>>>> >>>>>> Notice that the problem does not happen without driver model, since >>>>>> non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with >>>>>> the cache off. >>>> >>>> I'm not sure if you read the long and windy thread with Stefan B but >>>> the upshot is that the driver is doing the right thing. >>>> >>>> If the driver were doing the memset() then you could make a case that >>>> we should change the driver, but since DM is doing it and DM is >>>> promising that DMA can be used on the buffer, I think the best place >>>> for the fix is in DM. The driver should not need to change and neither >>>> should any other driver when we convert it to DM. >>>> >>>> On that last point I really want to avoid having to change the caching >>>> behaviour of drivers just to work with DM! >>> >>> So will the driver work with your patch and without DM ? I don't think >>> so, so I think what Eddie's patch does is correct. I'd really like him >>> to send a new version and apply that. >> >> Yes the driver work fine without DM and the code is correct. The >> buffer is in BSS in that case and we don't have the cache problem. I >> think we would have seen this problem before :-) > > I am seeing problems around this code and this patch makes sense to me, > so I think this patch should go in as well ... OK, well up to you. What sort of problems? > >>> >>> If this also needs to be fixed in DM, so be it. >> >> OK. >> >> Regards, >> Simon Regards, Simon
On 04/06/2017 04:40 AM, Simon Glass wrote: > Hi Marek, > > On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote: >> On 04/06/2017 03:24 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote: >>>> On 04/05/2017 05:03 PM, Simon Glass wrote: >>>>> +Tom >>>>> >>>>> Hi Marek, >>>>> >>>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote: >>>>>> On 04/05/2017 12:08 PM, Simon Glass wrote: >>>>>>> Hi Marek, >>>>>>> >>>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >>>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>>>>>>>> Hi Eddie, >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> We should only need to do only one time cache operation for a buffer >>>>>>>>>> >>>>>>>>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>>>>>>>> >>>>>>>>>> operation for the same buffer in the same function. >>>>>>>>> >>>>>>>>> I think this is a more general problem and might cause issues with >>>>>>>>> other drivers. So I have sent this patch: >>>>>>>>> >>>>>>>>> http://patchwork.ozlabs.org/patch/746917/ >>>>>>>>> >>>>>>>> >>>>>>>> This feels like papering over a problem though ... which will bite you >>>>>>>> later anyway. >>>>>>> >>>>>>> I believe the problem only happens because we have cached zero bytes >>>>>>> caused by this function. If the driver does the right thing (as dwc2.c >>>>>>> already does) then everything should be find from then on. >>>>>> >>>>>> And I think the driver is where this should be fixed ? That is, the >>>>>> driver should do the right thing and flush/invalidate caches correctly. >>>>>> >>>>>>> Notice that the problem does not happen without driver model, since >>>>>>> non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with >>>>>>> the cache off. >>>>> >>>>> I'm not sure if you read the long and windy thread with Stefan B but >>>>> the upshot is that the driver is doing the right thing. >>>>> >>>>> If the driver were doing the memset() then you could make a case that >>>>> we should change the driver, but since DM is doing it and DM is >>>>> promising that DMA can be used on the buffer, I think the best place >>>>> for the fix is in DM. The driver should not need to change and neither >>>>> should any other driver when we convert it to DM. >>>>> >>>>> On that last point I really want to avoid having to change the caching >>>>> behaviour of drivers just to work with DM! >>>> >>>> So will the driver work with your patch and without DM ? I don't think >>>> so, so I think what Eddie's patch does is correct. I'd really like him >>>> to send a new version and apply that. >>> >>> Yes the driver work fine without DM and the code is correct. The >>> buffer is in BSS in that case and we don't have the cache problem. I >>> think we would have seen this problem before :-) >> >> I am seeing problems around this code and this patch makes sense to me, >> so I think this patch should go in as well ... > > OK, well up to you. What sort of problems? Some sticks are not detected for me and adding a small delay here magically fixes it. I suspect I'm hitting this cache issue.
Hi Marek, On 6 April 2017 at 04:22, Marek Vasut <marex@denx.de> wrote: > On 04/06/2017 04:40 AM, Simon Glass wrote: >> Hi Marek, >> >> On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote: >>> On 04/06/2017 03:24 AM, Simon Glass wrote: >>>> Hi Marek, >>>> >>>> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote: >>>>> On 04/05/2017 05:03 PM, Simon Glass wrote: >>>>>> +Tom >>>>>> >>>>>> Hi Marek, >>>>>> >>>>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote: >>>>>>> On 04/05/2017 12:08 PM, Simon Glass wrote: >>>>>>>> Hi Marek, >>>>>>>> >>>>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >>>>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote: >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>>>>>>>>> Hi Eddie, >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> We should only need to do only one time cache operation for a buffer >>>>>>>>>>> >>>>>>>>>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>>>>>>>>> >>>>>>>>>>> operation for the same buffer in the same function. >>>>>>>>>> >>>>>>>>>> I think this is a more general problem and might cause issues with >>>>>>>>>> other drivers. So I have sent this patch: >>>>>>>>>> >>>>>>>>>> http://patchwork.ozlabs.org/patch/746917/ >>>>>>>>>> >>>>>>>>> >>>>>>>>> This feels like papering over a problem though ... which will bite you >>>>>>>>> later anyway. >>>>>>>> >>>>>>>> I believe the problem only happens because we have cached zero bytes >>>>>>>> caused by this function. If the driver does the right thing (as dwc2.c >>>>>>>> already does) then everything should be find from then on. >>>>>>> >>>>>>> And I think the driver is where this should be fixed ? That is, the >>>>>>> driver should do the right thing and flush/invalidate caches correctly. >>>>>>> >>>>>>>> Notice that the problem does not happen without driver model, since >>>>>>>> non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with >>>>>>>> the cache off. >>>>>> >>>>>> I'm not sure if you read the long and windy thread with Stefan B but >>>>>> the upshot is that the driver is doing the right thing. >>>>>> >>>>>> If the driver were doing the memset() then you could make a case that >>>>>> we should change the driver, but since DM is doing it and DM is >>>>>> promising that DMA can be used on the buffer, I think the best place >>>>>> for the fix is in DM. The driver should not need to change and neither >>>>>> should any other driver when we convert it to DM. >>>>>> >>>>>> On that last point I really want to avoid having to change the caching >>>>>> behaviour of drivers just to work with DM! >>>>> >>>>> So will the driver work with your patch and without DM ? I don't think >>>>> so, so I think what Eddie's patch does is correct. I'd really like him >>>>> to send a new version and apply that. >>>> >>>> Yes the driver work fine without DM and the code is correct. The >>>> buffer is in BSS in that case and we don't have the cache problem. I >>>> think we would have seen this problem before :-) >>> >>> I am seeing problems around this code and this patch makes sense to me, >>> so I think this patch should go in as well ... >> >> OK, well up to you. What sort of problems? > > Some sticks are not detected for me and adding a small delay here > magically fixes it. I suspect I'm hitting this cache issue. Is this with CONFIG_DM_USB or without? Also does your platform have a write-through or write-back cache? Regards, Simon
On 04/06/2017 04:06 PM, Simon Glass wrote: > Hi Marek, > > On 6 April 2017 at 04:22, Marek Vasut <marex@denx.de> wrote: >> On 04/06/2017 04:40 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote: >>>> On 04/06/2017 03:24 AM, Simon Glass wrote: >>>>> Hi Marek, >>>>> >>>>> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote: >>>>>> On 04/05/2017 05:03 PM, Simon Glass wrote: >>>>>>> +Tom >>>>>>> >>>>>>> Hi Marek, >>>>>>> >>>>>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote: >>>>>>>> On 04/05/2017 12:08 PM, Simon Glass wrote: >>>>>>>>> Hi Marek, >>>>>>>>> >>>>>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >>>>>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>>>>>>>>>> Hi Eddie, >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> We should only need to do only one time cache operation for a buffer >>>>>>>>>>>> >>>>>>>>>>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>>>>>>>>>> >>>>>>>>>>>> operation for the same buffer in the same function. >>>>>>>>>>> >>>>>>>>>>> I think this is a more general problem and might cause issues with >>>>>>>>>>> other drivers. So I have sent this patch: >>>>>>>>>>> >>>>>>>>>>> http://patchwork.ozlabs.org/patch/746917/ >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> This feels like papering over a problem though ... which will bite you >>>>>>>>>> later anyway. >>>>>>>>> >>>>>>>>> I believe the problem only happens because we have cached zero bytes >>>>>>>>> caused by this function. If the driver does the right thing (as dwc2.c >>>>>>>>> already does) then everything should be find from then on. >>>>>>>> >>>>>>>> And I think the driver is where this should be fixed ? That is, the >>>>>>>> driver should do the right thing and flush/invalidate caches correctly. >>>>>>>> >>>>>>>>> Notice that the problem does not happen without driver model, since >>>>>>>>> non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with >>>>>>>>> the cache off. >>>>>>> >>>>>>> I'm not sure if you read the long and windy thread with Stefan B but >>>>>>> the upshot is that the driver is doing the right thing. >>>>>>> >>>>>>> If the driver were doing the memset() then you could make a case that >>>>>>> we should change the driver, but since DM is doing it and DM is >>>>>>> promising that DMA can be used on the buffer, I think the best place >>>>>>> for the fix is in DM. The driver should not need to change and neither >>>>>>> should any other driver when we convert it to DM. >>>>>>> >>>>>>> On that last point I really want to avoid having to change the caching >>>>>>> behaviour of drivers just to work with DM! >>>>>> >>>>>> So will the driver work with your patch and without DM ? I don't think >>>>>> so, so I think what Eddie's patch does is correct. I'd really like him >>>>>> to send a new version and apply that. >>>>> >>>>> Yes the driver work fine without DM and the code is correct. The >>>>> buffer is in BSS in that case and we don't have the cache problem. I >>>>> think we would have seen this problem before :-) >>>> >>>> I am seeing problems around this code and this patch makes sense to me, >>>> so I think this patch should go in as well ... >>> >>> OK, well up to you. What sort of problems? >> >> Some sticks are not detected for me and adding a small delay here >> magically fixes it. I suspect I'm hitting this cache issue. > > Is this with CONFIG_DM_USB or without? With. > Also does your platform have a write-through or write-back cache? I think WT, but I'm not 100% sure . I can check when I have access to the board . It's SoCFPGA, C-A9 .
Hi Marek, On 6 April 2017 at 09:33, Marek Vasut <marex@denx.de> wrote: > On 04/06/2017 04:06 PM, Simon Glass wrote: >> Hi Marek, >> >> On 6 April 2017 at 04:22, Marek Vasut <marex@denx.de> wrote: >>> On 04/06/2017 04:40 AM, Simon Glass wrote: >>>> Hi Marek, >>>> >>>> On 5 April 2017 at 19:32, Marek Vasut <marex@denx.de> wrote: >>>>> On 04/06/2017 03:24 AM, Simon Glass wrote: >>>>>> Hi Marek, >>>>>> >>>>>> On 5 April 2017 at 15:34, Marek Vasut <marex@denx.de> wrote: >>>>>>> On 04/05/2017 05:03 PM, Simon Glass wrote: >>>>>>>> +Tom >>>>>>>> >>>>>>>> Hi Marek, >>>>>>>> >>>>>>>> On 5 April 2017 at 04:21, Marek Vasut <marex@denx.de> wrote: >>>>>>>>> On 04/05/2017 12:08 PM, Simon Glass wrote: >>>>>>>>>> Hi Marek, >>>>>>>>>> >>>>>>>>>> On 5 April 2017 at 03:35, Marek Vasut <marex@denx.de> wrote: >>>>>>>>>>> On 04/05/2017 04:21 AM, Simon Glass wrote: >>>>>>>>>>>> Hi, >>>>>>>>>>>> >>>>>>>>>>>> On 4 April 2017 at 19:26, Kever Yang <kever.yang@rock-chips.com> wrote: >>>>>>>>>>>>> Hi Eddie, >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> We should only need to do only one time cache operation for a buffer >>>>>>>>>>>>> >>>>>>>>>>>>> ready to do DMA transfer, so you need to remove another cache invalidate >>>>>>>>>>>>> >>>>>>>>>>>>> operation for the same buffer in the same function. >>>>>>>>>>>> >>>>>>>>>>>> I think this is a more general problem and might cause issues with >>>>>>>>>>>> other drivers. So I have sent this patch: >>>>>>>>>>>> >>>>>>>>>>>> http://patchwork.ozlabs.org/patch/746917/ >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> This feels like papering over a problem though ... which will bite you >>>>>>>>>>> later anyway. >>>>>>>>>> >>>>>>>>>> I believe the problem only happens because we have cached zero bytes >>>>>>>>>> caused by this function. If the driver does the right thing (as dwc2.c >>>>>>>>>> already does) then everything should be find from then on. >>>>>>>>> >>>>>>>>> And I think the driver is where this should be fixed ? That is, the >>>>>>>>> driver should do the right thing and flush/invalidate caches correctly. >>>>>>>>> >>>>>>>>>> Notice that the problem does not happen without driver model, since >>>>>>>>>> non-DM code in dwc2.c uses BSS for the buffers, which is zeroed with >>>>>>>>>> the cache off. >>>>>>>> >>>>>>>> I'm not sure if you read the long and windy thread with Stefan B but >>>>>>>> the upshot is that the driver is doing the right thing. >>>>>>>> >>>>>>>> If the driver were doing the memset() then you could make a case that >>>>>>>> we should change the driver, but since DM is doing it and DM is >>>>>>>> promising that DMA can be used on the buffer, I think the best place >>>>>>>> for the fix is in DM. The driver should not need to change and neither >>>>>>>> should any other driver when we convert it to DM. >>>>>>>> >>>>>>>> On that last point I really want to avoid having to change the caching >>>>>>>> behaviour of drivers just to work with DM! >>>>>>> >>>>>>> So will the driver work with your patch and without DM ? I don't think >>>>>>> so, so I think what Eddie's patch does is correct. I'd really like him >>>>>>> to send a new version and apply that. >>>>>> >>>>>> Yes the driver work fine without DM and the code is correct. The >>>>>> buffer is in BSS in that case and we don't have the cache problem. I >>>>>> think we would have seen this problem before :-) >>>>> >>>>> I am seeing problems around this code and this patch makes sense to me, >>>>> so I think this patch should go in as well ... >>>> >>>> OK, well up to you. What sort of problems? >>> >>> Some sticks are not detected for me and adding a small delay here >>> magically fixes it. I suspect I'm hitting this cache issue. >> >> Is this with CONFIG_DM_USB or without? > > With. OK, then this is no different from the rpi problem. My DM fix should fix it for you. The current driver (without DM) works fine. > >> Also does your platform have a write-through or write-back cache? > > I think WT, but I'm not 100% sure . I can check when I have access to > the board . It's SoCFPGA, C-A9 . I ask because with a WT cache the problem fixes itself after a short time (i.e. if you wait a bit before kicking the DMA the cache will have had time to flush). Regards, Simon
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 5ac602e..a151ba6 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -811,12 +811,17 @@ static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer, (*pid << DWC2_HCTSIZ_PID_OFFSET), &hc_regs->hctsiz); - if (!in && xfer_len) { - memcpy(aligned_buffer, buffer, xfer_len); - - flush_dcache_range((unsigned long)aligned_buffer, - (unsigned long)aligned_buffer + - roundup(xfer_len, ARCH_DMA_MINALIGN)); + if (xfer_len) { + if(in){ + invalidate_dcache_range((unsigned long)aligned_buffer, + (unsigned long)aligned_buffer + + roundup(xfer_len, ARCH_DMA_MINALIGN)); + }else{ + memcpy(aligned_buffer, buffer, xfer_len); + flush_dcache_range((unsigned long)aligned_buffer, + (unsigned long)aligned_buffer + + roundup(xfer_len, ARCH_DMA_MINALIGN)); + } } writel(phys_to_bus((unsigned long)aligned_buffer), &hc_regs->hcdma);
We should invalidate the dcache before starting the DMA. In case there are any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on. Cache-line replacement can happen if the CPU tries to bring some other memory locations into the cache while the DMA is going on. Signed-off-by: Eddie Cai <eddie.cai.linux@gmail.com> --- drivers/usb/host/dwc2.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)