Re: dwctwo(4) fix panic
On Thu, Apr 14, 2022 at 10:00:35PM +0200, Mark Kettenis wrote:
> > Date: Thu, 14 Apr 2022 20:16:13 +0200
> > From: Marcus Glocker
> >
> > I did hit this panic when trying to stream audio through
> > uaudio(4) / dwctwo(4):
> >
> > panic: _dmamap_sync: ran off map!
> > Stopped at panic+0x160:cmp w21, #0x0
> > TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
> > *421282 69103 0 0x2 0x4000K ld
> > db_enter() at panic+0x15c
> > panic() at _dmamap_sync+0x10c
> > _dmamap_sync() at dwc2_xfercomp_isoc_split_in+0xe0
> > dwc2_xfercomp_isoc_split_in() at dwc2_hc_xfercomp_intr+0xf4
> > dwc2_hc_xfercomp_intr() at dwc2_hc_n_intr+0x1d4
> > dwc2_hc_n_intr() at dwc2_handle_hcd_intr+0x1e4
> > dwc2_handle_hcd_intr() at dwc2_intr+0xb8
> >
> > Obviously usb_syncmem() expects an integer offset as second argument,
> > not a memory address. Looks like a typo to me ...
> >
> > OK?
>
> Hmm, that doesn't look right.
>
> >
> >
> > Index: sys/dev/usb/dwc2/dwc2_hcdintr.c
> > ===
> > RCS file: /cvs/src/sys/dev/usb/dwc2/dwc2_hcdintr.c,v
> > retrieving revision 1.13
> > diff -u -p -u -p -r1.13 dwc2_hcdintr.c
> > --- sys/dev/usb/dwc2/dwc2_hcdintr.c 4 Sep 2021 10:19:28 - 1.13
> > +++ sys/dev/usb/dwc2/dwc2_hcdintr.c 14 Apr 2022 17:48:08 -
> > @@ -975,11 +975,11 @@ STATIC int dwc2_xfercomp_isoc_split_in(s
> >
> > if (chan->align_buf) {
> > dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
> > - usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma,
> > + usb_syncmem(qtd->urb->usbdma, 0,
> > chan->qh->dw_align_buf_size, BUS_DMASYNC_POSTREAD);
> > memcpy(qtd->urb->buf + frame_desc->offset +
> >qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
> > - usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma,
> > + usb_syncmem(qtd->urb->usbdma, 0,
> > chan->qh->dw_align_buf_size, BUS_DMASYNC_PREREAD);
> > }
>
> When chan->align_buf is set we're doing DMA into dw_align_buf, and I
> think that means we need to use dw_align_buf_usbdma as the first
> argument to usb_syncmem since we need to make sure that the memory
> we're doing DMA into is coherent with the cache. So I think this
> should be something like:
>
> if (chan->align_buf) {
> struct usb_dma *ud = >qh->dw_align_buf_usbdma;
>
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
> usb_syncmem(ud, 0, chan->qh->dw_align_buf_size,
> BUS_DMASYNC_POSTREAD);
> memcpy(qtd->urb->buf + frame_desc->offset +
> qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
> usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma,
> usb_syncmem(ud, 0, chan->qh->dw_align_buf_size,
> BUS_DMASYNC_PREREAD);
>
> I think that means the code in dwc2_update_uwb_state() is wrong too.
> But then I never fully understood this code...
I guess you're right. Also looking at the other usb_syncmem() calls
when dw_align_buf is involved. As well the comment for struct dwc2_qh
says:
* @dw_align_buf_size: Size of dw_align_buf
* @dw_align_buf_dma: DMA address for dw_align_buf
I've tested the following diff. Panic is gone, sound still crackles,
but still partially plays. I think there are other issues related to
the ISOC transfers which need to be investigated further.
Index: sys/dev/usb/dwc2/dwc2_hcdintr.c
===
RCS file: /cvs/src/sys/dev/usb/dwc2/dwc2_hcdintr.c,v
retrieving revision 1.13
diff -u -p -u -p -r1.13 dwc2_hcdintr.c
--- sys/dev/usb/dwc2/dwc2_hcdintr.c 4 Sep 2021 10:19:28 - 1.13
+++ sys/dev/usb/dwc2/dwc2_hcdintr.c 15 Apr 2022 07:25:42 -
@@ -493,13 +493,15 @@ STATIC int dwc2_update_urb_state(struct
/* Non DWORD-aligned buffer case handling */
if (chan->align_buf && xfer_length) {
dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
- usb_syncmem(urb->usbdma, 0, chan->qh->dw_align_buf_size,
+ struct usb_dma *ud = >qh->dw_align_buf_usbdma;
+
+ usb_syncmem(ud, 0, chan->qh->dw_align_buf_size,
chan->ep_is_in ?
BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
if (chan->ep_is_in)
memcpy(urb->buf + urb->actual_length,
chan->qh->dw_align_buf, xfer_length);
- usb_syncmem(urb->usbdma, 0, chan->qh->dw_align_buf_size,
+ usb_syncmem(ud, 0, chan->qh->dw_align_buf_size,
chan->ep_is_in ?
BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
}
@@ -975,12 +977,14 @@ STATIC int
Re: dwctwo(4) fix panic
> Date: Thu, 14 Apr 2022 20:16:13 +0200
> From: Marcus Glocker
>
> I did hit this panic when trying to stream audio through
> uaudio(4) / dwctwo(4):
>
> panic: _dmamap_sync: ran off map!
> Stopped at panic+0x160:cmp w21, #0x0
> TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
> *421282 69103 0 0x2 0x4000K ld
> db_enter() at panic+0x15c
> panic() at _dmamap_sync+0x10c
> _dmamap_sync() at dwc2_xfercomp_isoc_split_in+0xe0
> dwc2_xfercomp_isoc_split_in() at dwc2_hc_xfercomp_intr+0xf4
> dwc2_hc_xfercomp_intr() at dwc2_hc_n_intr+0x1d4
> dwc2_hc_n_intr() at dwc2_handle_hcd_intr+0x1e4
> dwc2_handle_hcd_intr() at dwc2_intr+0xb8
>
> Obviously usb_syncmem() expects an integer offset as second argument,
> not a memory address. Looks like a typo to me ...
>
> OK?
Hmm, that doesn't look right.
>
>
> Index: sys/dev/usb/dwc2/dwc2_hcdintr.c
> ===
> RCS file: /cvs/src/sys/dev/usb/dwc2/dwc2_hcdintr.c,v
> retrieving revision 1.13
> diff -u -p -u -p -r1.13 dwc2_hcdintr.c
> --- sys/dev/usb/dwc2/dwc2_hcdintr.c 4 Sep 2021 10:19:28 - 1.13
> +++ sys/dev/usb/dwc2/dwc2_hcdintr.c 14 Apr 2022 17:48:08 -
> @@ -975,11 +975,11 @@ STATIC int dwc2_xfercomp_isoc_split_in(s
>
> if (chan->align_buf) {
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
> - usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma,
> + usb_syncmem(qtd->urb->usbdma, 0,
> chan->qh->dw_align_buf_size, BUS_DMASYNC_POSTREAD);
> memcpy(qtd->urb->buf + frame_desc->offset +
> qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
> - usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma,
> + usb_syncmem(qtd->urb->usbdma, 0,
> chan->qh->dw_align_buf_size, BUS_DMASYNC_PREREAD);
> }
When chan->align_buf is set we're doing DMA into dw_align_buf, and I
think that means we need to use dw_align_buf_usbdma as the first
argument to usb_syncmem since we need to make sure that the memory
we're doing DMA into is coherent with the cache. So I think this
should be something like:
if (chan->align_buf) {
struct usb_dma *ud = >qh->dw_align_buf_usbdma;
dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
usb_syncmem(ud, 0, chan->qh->dw_align_buf_size,
BUS_DMASYNC_POSTREAD);
memcpy(qtd->urb->buf + frame_desc->offset +
qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma,
usb_syncmem(ud, 0, chan->qh->dw_align_buf_size,
BUS_DMASYNC_PREREAD);
I think that means the code in dwc2_update_uwb_state() is wrong too.
But then I never fully understood this code...
dwctwo(4) fix panic
I did hit this panic when trying to stream audio through
uaudio(4) / dwctwo(4):
panic: _dmamap_sync: ran off map!
Stopped at panic+0x160:cmp w21, #0x0
TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
*421282 69103 0 0x2 0x4000K ld
db_enter() at panic+0x15c
panic() at _dmamap_sync+0x10c
_dmamap_sync() at dwc2_xfercomp_isoc_split_in+0xe0
dwc2_xfercomp_isoc_split_in() at dwc2_hc_xfercomp_intr+0xf4
dwc2_hc_xfercomp_intr() at dwc2_hc_n_intr+0x1d4
dwc2_hc_n_intr() at dwc2_handle_hcd_intr+0x1e4
dwc2_handle_hcd_intr() at dwc2_intr+0xb8
Obviously usb_syncmem() expects an integer offset as second argument,
not a memory address. Looks like a typo to me ...
OK?
Index: sys/dev/usb/dwc2/dwc2_hcdintr.c
===
RCS file: /cvs/src/sys/dev/usb/dwc2/dwc2_hcdintr.c,v
retrieving revision 1.13
diff -u -p -u -p -r1.13 dwc2_hcdintr.c
--- sys/dev/usb/dwc2/dwc2_hcdintr.c 4 Sep 2021 10:19:28 - 1.13
+++ sys/dev/usb/dwc2/dwc2_hcdintr.c 14 Apr 2022 17:48:08 -
@@ -975,11 +975,11 @@ STATIC int dwc2_xfercomp_isoc_split_in(s
if (chan->align_buf) {
dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
- usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma,
+ usb_syncmem(qtd->urb->usbdma, 0,
chan->qh->dw_align_buf_size, BUS_DMASYNC_POSTREAD);
memcpy(qtd->urb->buf + frame_desc->offset +
qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
- usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma,
+ usb_syncmem(qtd->urb->usbdma, 0,
chan->qh->dw_align_buf_size, BUS_DMASYNC_PREREAD);
}
