I'm picking up this series by Guenter Roeck as he stated he has no time
for it ATM. It was found to solve some unaligned DMA access issues on
Raspberry Pi 3. You can find the original discussion here:
https://lore.kernel.org/linux-usb/[email protected]/
I removed the fist patch from the original series as it turned out to be
contententious and needs more in-depth testing. Following is the edited
origin series description. Note that extra testing was performed on
RPi3:
"This series addresses the following problems:
- Fix receive transfers with 0 byte transfer length
- Abort transactions after unknown receive errors
if the receive buffer is full
- Reduce "trimming xfer length" logging noise
The problems fixed with this series were observed when connecting
a DM9600 Ethernet adapter to Veyron Chromebooks such as the ASUS
Chromebook C201PA. The series was tested extensively with this and
other adapters.
The observed problems are also reported when tethering various
phones, so test coverage with such phones would be very appreciated."
---
Guenter Roeck (3):
usb: dwc2: Do not update data length if it is 0 on inbound transfers
usb: dwc2: Abort transaction after errors with unknown reason
usb: dwc2: Make "trimming xfer length" a debug message
drivers/usb/dwc2/hcd.c | 15 ++++++++-------
drivers/usb/dwc2/hcd_intr.c | 14 +++++++++++++-
2 files changed, 21 insertions(+), 8 deletions(-)
--
2.29.2
Hi,
On Wed, Jan 13, 2021 at 3:21 AM Nicolas Saenz Julienne
<[email protected]> wrote:
>
> I'm picking up this series by Guenter Roeck as he stated he has no time
> for it ATM. It was found to solve some unaligned DMA access issues on
> Raspberry Pi 3. You can find the original discussion here:
> https://lore.kernel.org/linux-usb/[email protected]/
>
> I removed the fist patch from the original series as it turned out to be
> contententious and needs more in-depth testing. Following is the edited
> origin series description. Note that extra testing was performed on
> RPi3:
>
> "This series addresses the following problems:
>
> - Fix receive transfers with 0 byte transfer length
> - Abort transactions after unknown receive errors
> if the receive buffer is full
> - Reduce "trimming xfer length" logging noise
>
> The problems fixed with this series were observed when connecting
> a DM9600 Ethernet adapter to Veyron Chromebooks such as the ASUS
> Chromebook C201PA. The series was tested extensively with this and
> other adapters.
>
> The observed problems are also reported when tethering various
> phones, so test coverage with such phones would be very appreciated."
>
> ---
>
> Guenter Roeck (3):
> usb: dwc2: Do not update data length if it is 0 on inbound transfers
> usb: dwc2: Abort transaction after errors with unknown reason
> usb: dwc2: Make "trimming xfer length" a debug message
>
> drivers/usb/dwc2/hcd.c | 15 ++++++++-------
> drivers/usb/dwc2/hcd_intr.c | 14 +++++++++++++-
> 2 files changed, 21 insertions(+), 8 deletions(-)
It's been long enough ago that I've forgotten where this was left off,
but IIRC the 3 patches that you have here are all fine to land (and
have my Reviewed-by tag). However, I think Guenter was still tracking
down additional problems. Guenter: does that match your recollection?
It looks like there are still bugs open for this on our public bug tracker:
https://issuetracker.google.com/issues/172208170
https://issuetracker.google.com/issues/172216241
...but, as Guenter said, I don't think there's anyone actively working on them.
I'm not really doing too much with dwc2 these days either and don't
currently have good HW setup for testing, so for the most part I'll
leave it to you. I wanted to at least summarize what I remembered,
though! :-)
-Doug
On Wed, Jan 13, 2021 at 03:20:55PM -0800, Doug Anderson wrote:
> Hi,
>
[ ... ]
>
> It's been long enough ago that I've forgotten where this was left off,
> but IIRC the 3 patches that you have here are all fine to land (and
> have my Reviewed-by tag). However, I think Guenter was still tracking
> down additional problems. Guenter: does that match your recollection?
>
> It looks like there are still bugs open for this on our public bug tracker:
>
> https://issuetracker.google.com/issues/172208170
> https://issuetracker.google.com/issues/172216241
>
> ...but, as Guenter said, I don't think there's anyone actively working on them.
>
> I'm not really doing too much with dwc2 these days either and don't
> currently have good HW setup for testing, so for the most part I'll
> leave it to you. I wanted to at least summarize what I remembered,
> though! :-)
>
The patches in this series still match what I had in my latest test code,
so it makes sense to move forward with them. I don't think I ever found
an acceptable version of the DMA alignment code.
Guenter
Hi Guenter, Doug, thanks for having a look at this.
On Wed, 2021-01-13 at 19:07 -0800, Guenter Roeck wrote:
> On Wed, Jan 13, 2021 at 03:20:55PM -0800, Doug Anderson wrote:
> > Hi,
> >
> [ ... ]
> >
> > It's been long enough ago that I've forgotten where this was left off,
> > but IIRC the 3 patches that you have here are all fine to land (and
> > have my Reviewed-by tag). However, I think Guenter was still tracking
> > down additional problems. Guenter: does that match your recollection?
> >
> > It looks like there are still bugs open for this on our public bug tracker:
> >
> > https://issuetracker.google.com/issues/172208170
> > https://issuetracker.google.com/issues/172216241
> >
> > ...but, as Guenter said, I don't think there's anyone actively working on them.
> >
> > I'm not really doing too much with dwc2 these days either and don't
> > currently have good HW setup for testing, so for the most part I'll
> > leave it to you. I wanted to at least summarize what I remembered,
> > though! :-)
> >
>
> The patches in this series still match what I had in my latest test code,
> so it makes sense to move forward with them. I don't think I ever found
> an acceptable version of the DMA alignment code.
As for the alignment code rework, can you recall the underlying issue that
warranted it?
Regards,
Nicolas
On Thu, Jan 14, 2021 at 10:26:25AM +0100, Nicolas Saenz Julienne wrote:
> Hi Guenter, Doug, thanks for having a look at this.
>
> On Wed, 2021-01-13 at 19:07 -0800, Guenter Roeck wrote:
> > On Wed, Jan 13, 2021 at 03:20:55PM -0800, Doug Anderson wrote:
> > > Hi,
> > >
> > [ ... ]
> > >
> > > It's been long enough ago that I've forgotten where this was left off,
> > > but IIRC the 3 patches that you have here are all fine to land (and
> > > have my Reviewed-by tag). However, I think Guenter was still tracking
> > > down additional problems. Guenter: does that match your recollection?
> > >
> > > It looks like there are still bugs open for this on our public bug tracker:
> > >
> > > https://issuetracker.google.com/issues/172208170
> > > https://issuetracker.google.com/issues/172216241
> > >
> > > ...but, as Guenter said, I don't think there's anyone actively working on them.
> > >
> > > I'm not really doing too much with dwc2 these days either and don't
> > > currently have good HW setup for testing, so for the most part I'll
> > > leave it to you. I wanted to at least summarize what I remembered,
> > > though! :-)
> > >
> >
> > The patches in this series still match what I had in my latest test code,
> > so it makes sense to move forward with them. I don't think I ever found
> > an acceptable version of the DMA alignment code.
>
> As for the alignment code rework, can you recall the underlying issue that
> warranted it?
>
See
https://patchwork.kernel.org/project/linux-usb/patch/[email protected]/
for details. It isn't up to date - it says that buffer alignment to
DWC2_USB_DMA_ALIGN would be acceptable. However, it turned out in testing
that buffers do have to be aligned to dma_get_cache_alignment(), at least
on some mips systems.
My latest work-in-progress patch describes the changes made as:
To simplify the code, move the old data pointer back to the beginning of
the new buffer, restoring most of the original commit. Increase buffer
alignment to dma_get_cache_alignment(). Ensure that the data pointer is
DMA aligned by using ____cacheline_aligned instead of realigning it after
allocation. Ensure that the allocated buffer is a multiple of
wMaxPacketSize to guarantee that the chip does not write beyond the end
of the buffer.
I can provide that version of the patch in case someone wants to pick it up,
but it would need thorough testing on a variety of systems before it is
applied.
Guenter