Date: Sat, 19 Sep 2020 12:36:10 -0700 From: Mark Millard <marklmi@yahoo.com> To: Hans Petter Selasky <hps@selasky.org> Cc: freebsd-arm <freebsd-arm@freebsd.org> Subject: Re: Comment #135 for bugzilla 237666 : a USB3-handling problem with a investigatory fix for a cortex-a72 context Message-ID: <D28FEE99-A2CA-484E-A5E9-312334CF3FE3@yahoo.com> In-Reply-To: <565258A0-BEE1-48F8-9851-E6C7CF7ADAE7@yahoo.com> References: <6E618C3D-12DF-429E-A249-5BAB90FC6B15.ref@yahoo.com> <6E618C3D-12DF-429E-A249-5BAB90FC6B15@yahoo.com> <866bd652-5a8b-7500-b1e0-7528c035048b@selasky.org> <F9E8A6A0-619B-4C6F-8485-032B8358B780@yahoo.com> <578faf26-6042-91ec-c639-2bca17105fae@selasky.org> <723E6915-94F5-417C-B4AF-EEEBFBDF6162@yahoo.com> <565258A0-BEE1-48F8-9851-E6C7CF7ADAE7@yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 2020-Sep-19, at 12:18, Mark Millard <marklmi@yahoo.com> wrote:
>=20
> In the below I've cut out text without always indicating
> where so that code looks complete that is complete. I've
> also included some text from another reply of yours that
> fills in some specifics of what you were after.
>=20
> On 2020-Sep-19, at 04:46, Hans Petter Selasky <hps at selasky.org> =
wrote:
>>=20
>>>>> . . .
>>>>>=20
>>>>> Your finding indicate a problem in usb_pc_cpu_flush() and
>>>>>=20
>>>>> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
>>>>>=20
>>>>> Try to put the dsb only after dmamap_sync.
>>>>>=20
>>>>> void
>>>>> usb_pc_cpu_flush(struct usb_page_cache *pc)
>>>>> {
>>>>> if (pc->page_offset_end =3D=3D pc->page_offset_buf) {
>>>>> /* nothing has been loaded into this page cache! */
>>>>> return;
>>>>> }
>>>>> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
>>>>> }
>>>>> . . .
>>>> After adding a couple of more DSB ST instructions and rebuilding
>>>> (cross build), installing, and booting, I started a self-hosted,
>>>> from-scratch, buildworld buildlkernel and intend to installkernel
>>>> installworld and reboot if it appears to go well. (Better than
>>>> just a boot test for if things seem at least sufficient, even if
>>>> overkill.) But it will be hours before buildworld build kernel is
>>>> done (-j3). (I'll sleep during much of that time.)
>=20
> The buildworld buildkernel, installkernel installworld, reboot
> sequence seems to have gone fine.
>=20
>>>> . . .
>>>> phwr =3D buf_res.buffer;
>>>> addr =3D buf_res.physaddr;
>>>> addr +=3D (uintptr_t)&((struct xhci_hw_root =
*)0)->hwr_events[0];
>>>> /* reset hardware root structure */
>>>> memset(phwr, 0, sizeof(*phwr));
>>>> phwr->hwr_ring_seg[0].qwEvrsTablePtr =3D =
htole64(addr);
>>>> phwr->hwr_ring_seg[0].dwEvrsTableSize =3D =
htole32(XHCI_MAX_EVENTS);
>>>> DPRINTF("ERDP(0)=3D0x%016llx\n", (unsigned long =
long)addr);
>>>> #ifdef __aarch64__
>>>> __asm __volatile("dsb st" : : : "memory");
>>>> #endif
>>>> XWRITE4(sc, runt, XHCI_ERDP_LO(0), (uint32_t)addr);
>>>> XWRITE4(sc, runt, XHCI_ERDP_HI(0), (uint32_t)(addr >> 32));
>>>> addr =3D buf_res.physaddr;
>>>> DPRINTF("ERSTBA(0)=3D0x%016llx\n", (unsigned long =
long)addr);
>>>> XWRITE4(sc, runt, XHCI_ERSTBA_LO(0), (uint32_t)addr);
>>>> XWRITE4(sc, runt, XHCI_ERSTBA_HI(0), (uint32_t)(addr >> 32));
>>> . . .
>>> Probably same bug in the USB invalidate function.
>=20
>>> On 2020-09-19 11:31, Mark Millard wrote:
>>>> #ifdef __aarch64__
>>>> __asm __volatile("dsb ld" : : : "memory");
>>>> #endif
>>>> temp =3D le32toh(phwr->hwr_events[i].dwTrb3);
>>>>=20
>>>=20
>>>=20
>>> If you look a few lines up you'll find the:
>>>=20
>>> usb_pc_cpu_invalidate(&sc->sc_hw.root_pc);
>>>=20
>>> That's where this instruction belongs.
>=20
>>> Try to patch those generic flush/invalidate functions instead. I've =
been very careful about those.
>>=20
>> Sure.
>>=20
>>>> temp =3D le32toh(phwr->hwr_events[i].dwTrb3);
>>>> k =3D (temp & XHCI_TRB_3_CYCLE_BIT) ? 1 : 0;
>>>> if (j !=3D k)
>>>> break;
>>>> always took the break because temp was always zero, k was always =
zero,
>>>> and j was 1. I expect this is because the wrong qwEvrsTablePtr and
>>>> dwEvrsTableSize content was in use by the xhci (fixed by the DSB =
ST).
>>>> . . .
>>>=20
>>> It smells like a generic problem in the busdma synchronize code.
>>>=20
>>>> (Pe means "executing processor".)
>>>> I expect the removal of that specific DSB ST to result in
>>>> xhci_interrupt_poll's code again taking the break every time,
>>>> just based on event ring behavior alone.
>>>=20
>>> Again, try to patch both
>>> usb_pc_cpu_flush()
>>> and
>>> usb_pc_cpu_invalidate()
>>=20
>> Sure.
>>=20
>=20
> Unfortunately, it fails the same old way as far as
> what is seen on the console.
>=20
> So the steps used were:
>=20
> I reverted /usr/src/sys/dev/usb/controller/xhci.c .
>=20
> Then changed things so that:
>=20
> # svnlite diff /usr/src/sys/dev/usb/usb_busdma.c
> Index: /usr/src/sys/dev/usb/usb_busdma.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- /usr/src/sys/dev/usb/usb_busdma.c (revision 363590)
> +++ /usr/src/sys/dev/usb/usb_busdma.c (working copy)
> @@ -737,6 +737,9 @@
> */
> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_POSTREAD);
> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREREAD);
> +#ifdef __aarch64__
> +__asm __volatile("dsb ld" : : : "memory");
> +#endif
> }
>=20
> =
/*------------------------------------------------------------------------=
*
> @@ -750,6 +753,9 @@
> return;
> }
> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
> +#ifdef __aarch64__
> +__asm __volatile("dsb st" : : : "memory");
> +#endif
> }
>=20
> =
/*------------------------------------------------------------------------=
*
>=20
>=20
> As I expected, the resulting build fails to boot the same old
> way: a sequence messages that repeats indefinitely and
> involves "xhci0: Resetting controller" and no root file system
> mount happens.
>=20
> So I put back the just last change that had previously lead to
> the successful booting (given the prior DSB ST / DSB LD
> additions). I did so exactly as I originally had it:
>=20
> # svnlite diff /usr/src/sys/dev/usb/controller/xhci.c
> Index: /usr/src/sys/dev/usb/controller/xhci.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- /usr/src/sys/dev/usb/controller/xhci.c (revision 363590)
> +++ /usr/src/sys/dev/usb/controller/xhci.c (working copy)
> @@ -406,6 +406,9 @@
>=20
> addr =3D buf_res.physaddr;
>=20
> +#ifdef __aarch64__
> +__asm __volatile("dsb st" : : : "memory");
> +#endif
> XWRITE4(sc, oper, XHCI_DCBAAP_LO, (uint32_t)addr);
> XWRITE4(sc, oper, XHCI_DCBAAP_HI, (uint32_t)(addr >> 32));
> XWRITE4(sc, oper, XHCI_DCBAAP_LO, (uint32_t)addr);
>=20
> The result still fails. So more is required.
The controller/xhci.c change above was the wrong one: I grabbed
the wrong text. Retrying with:
# svnlite diff /usr/src/sys/dev/usb/controller/xhci.c
Index: /usr/src/sys/dev/usb/controller/xhci.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- /usr/src/sys/dev/usb/controller/xhci.c (revision 363590)
+++ /usr/src/sys/dev/usb/controller/xhci.c (working copy)
@@ -441,6 +441,9 @@
=20
DPRINTF("ERSTBA(0)=3D0x%016llx\n", (unsigned long long)addr);
=20
+#ifdef __aarch64__
+__asm __volatile("dsb st" : : : "memory");
+#endif
XWRITE4(sc, runt, XHCI_ERSTBA_LO(0), (uint32_t)addr);
XWRITE4(sc, runt, XHCI_ERSTBA_HI(0), (uint32_t)(addr >> 32));
=20
booted just fine. This is what I expected relative to
initializing for the event ring.
=3D=3D=3D
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D28FEE99-A2CA-484E-A5E9-312334CF3FE3>