Re: Microsoft Surface: replace umstc(4) with ucc(4)
Visa Hankala writes: > On Fri, Nov 18, 2022 at 11:03:06AM -0500, Dave Voutila wrote: >> That fixes booting and the Surface Keyboard is usable, but I'm getting >> spurious faults coming from retpoline out of filt_wseventdetach if I >> detach and reattach the Surface Keyboard multiple times while running >> Xorg. >> >> It's not consistent, but I've been able to trigger it 3 times now. 2 via >> physical detach/reattach of the Surface Keyboard and another by just >> running `sysctl machdep.forceukdb=1` which apparently detaches/attaches >> behind the scenes. > > It seems that the detaching can leave behind dangling wsevent knotes. > They should be ferreted out before the device disappears. > > Does the following patch help? > > klist_invalidate() can block, so I put it before ev->q is freed and > set NULL. This tries to avoid causing new races with wsevent_init(). > > The detach logic is tangled and particularly so with ws devices, > so I am uncertain if tweaking just wsevent_fini() is enough. > I can't trigger the same fault, so that's a step forward. However with some rapid physical attach/detach I can trigger some very noticeable lag in devices coming back up and being responsive. Easily 5+ seconds on this machine. I also managed to trigger this kernel message once (no panic or halt): usb_transfer_complete: xfer=0xfd821190b580 not on queue I've found other issues while being abusive towards my USB devices on this machine, but they're reproducible on a fresh kernel from the tree so I believe unrelated to anton's diff. (I'll have to document and send out once I can reproduce them.) Not sure best way to proceed here. anton's (2nd) diff seems to work, but might be surfacing a bigger issue in wscons? Would be good to hear test reports from other Surface devices. -dv > Index: dev/wscons/wsevent.c > === > RCS file: src/sys/dev/wscons/wsevent.c,v > retrieving revision 1.26 > diff -u -p -r1.26 wsevent.c > --- dev/wscons/wsevent.c 2 Jul 2022 08:50:42 - 1.26 > +++ dev/wscons/wsevent.c 19 Nov 2022 15:05:26 - > @@ -131,6 +131,9 @@ wsevent_fini(struct wseventvar *ev) > #endif > return; > } > + > + klist_invalidate(&ev->sel.si_note); > + > free(ev->q, M_DEVBUF, WSEVENT_QSIZE * sizeof(struct wscons_event)); > ev->q = NULL; >
Re: Microsoft Surface: replace umstc(4) with ucc(4)
On Fri, Nov 18, 2022 at 11:03:06AM -0500, Dave Voutila wrote: > That fixes booting and the Surface Keyboard is usable, but I'm getting > spurious faults coming from retpoline out of filt_wseventdetach if I > detach and reattach the Surface Keyboard multiple times while running > Xorg. > > It's not consistent, but I've been able to trigger it 3 times now. 2 via > physical detach/reattach of the Surface Keyboard and another by just > running `sysctl machdep.forceukdb=1` which apparently detaches/attaches > behind the scenes. It seems that the detaching can leave behind dangling wsevent knotes. They should be ferreted out before the device disappears. Does the following patch help? klist_invalidate() can block, so I put it before ev->q is freed and set NULL. This tries to avoid causing new races with wsevent_init(). The detach logic is tangled and particularly so with ws devices, so I am uncertain if tweaking just wsevent_fini() is enough. Index: dev/wscons/wsevent.c === RCS file: src/sys/dev/wscons/wsevent.c,v retrieving revision 1.26 diff -u -p -r1.26 wsevent.c --- dev/wscons/wsevent.c2 Jul 2022 08:50:42 - 1.26 +++ dev/wscons/wsevent.c19 Nov 2022 15:05:26 - @@ -131,6 +131,9 @@ wsevent_fini(struct wseventvar *ev) #endif return; } + + klist_invalidate(&ev->sel.si_note); + free(ev->q, M_DEVBUF, WSEVENT_QSIZE * sizeof(struct wscons_event)); ev->q = NULL;
Microsoft Surface: replace umstc(4) with ucc(4)
Hi,
umstc is essentially a less capable version of ucc. Now that matthieu@
recently taught wskbd to adjust the brightness in process context, we
should be able to get rid of umstc in favor of ucc.
In order for ucc to be feature compatible with umstc, it must honor the
always open quirk as pointed out by jcs@ earlier.
I don't have access to any umstc hardware. Could someone with such
hardware try the following diff? Ideally, umstc should no longer attach
whereas ucc should. A dmesg before and after would be helpful.
diff --git sys/dev/usb/ucc.c sys/dev/usb/ucc.c
index acc14ce86cf..25b391fd9ee 100644
--- sys/dev/usb/ucc.c
+++ sys/dev/usb/ucc.c
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
#include
@@ -29,6 +30,7 @@
struct ucc_softc {
struct uhidevsc_hdev;
struct hidcc*sc_cc;
+ u_int32_tsc_quirks;
};
intucc_match(struct device *, void *, void *);
@@ -76,6 +78,8 @@ ucc_attach(struct device *parent, struct device *self, void
*aux)
void *desc;
int repid, size;
+ sc->sc_quirks = usbd_get_quirks(sc->sc_hdev.sc_udev)->uq_flags;
+
sc->sc_hdev.sc_intr = ucc_intr;
sc->sc_hdev.sc_parent = uha->parent;
sc->sc_hdev.sc_udev = uha->uaa->device;
@@ -98,6 +102,9 @@ ucc_attach(struct device *parent, struct device *self, void
*aux)
.arg= self,
};
sc->sc_cc = hidcc_attach(&hca);
+
+ if (sc->sc_quirks & UQ_ALWAYS_OPEN)
+ uhidev_open(&sc->sc_hdev);
}
int
@@ -127,6 +134,9 @@ ucc_enable(void *v, int on)
struct ucc_softc *sc = (struct ucc_softc *)v;
int error = 0;
+ if (sc->sc_quirks & UQ_ALWAYS_OPEN)
+ return 0;
+
if (on)
error = uhidev_open(&sc->sc_hdev);
else
diff --git sys/dev/usb/umstc.c sys/dev/usb/umstc.c
index 4bebeb41811..c3be6cce41f 100644
--- sys/dev/usb/umstc.c
+++ sys/dev/usb/umstc.c
@@ -80,6 +80,8 @@ umstc_match(struct device *parent, void *match, void *aux)
int size;
void *desc;
+ return (UMATCH_NONE);
+
if (UHIDEV_CLAIM_MULTIPLE_REPORTID(uha))
return (UMATCH_NONE);
