Source-Changes-D archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: CVS commit: src/sys/dev/usb
On 26/11/2025 03:12, Taylor R Campbell wrote:
Module Name: src
Committed By: manu
Date: Thu Oct 9 15:53:16 UTC 2025
Modified Files:
src/sys/dev/usb: usbdi.c
Log Message:
Adjust polling transition so that it can be compatible with DDB
Upcoming changes bring console and DDB support to ucom(4).
Approved by Nick Hudson
usbd_set_polling(struct usbd_device *dev, int on)
{
- mutex_enter(dev->ud_bus->ub_lock);
+ if (!db_active)
+ mutex_enter(dev->ud_bus->ub_lock);
...
+ * The softint routine is called after enabling polling
+ * and before disabling it, so that holding sc->sc_lock
+ * is not required. DDB needs this because it cannot wait
+ * to acquire sc->sc_lock from a suspended thread.
This change doesn't make sense; it fundamentally cannot work.
usbd_set_polling is used, via cnpollc, by ddb (and a few other things
like userconf) to switch from interrupt-based I/O to polling-based I/O
_when the system is guaranteed to be running a single thread_ either
because other threads haven't started or because ddb has already
suspended all the other CPUs.
There are two cases here:
(a) If the bus lock is not held, then there is nothing wrong with the
code that was there before.
(b) If the bus lock is held, then it is forbidden to touch any data
structures or hardware registers covered by the bus lock because
they all are currently in an unpredictable, inconsistent state in
the middle of an operation by a thread that assumes -- because it
holds a lock on the mutex -- that it has _exclusive_ access to
them.
Simply skipping the mutex_enter doesn't improve either situation: in
case (a), it doesn't achieve anything; in case (b), it turns a failure
or hang into a near-guarantee to corrupt some kernel data structures
and hardware registers.
So in the case where it appeared to make a difference, it made a
difference by removing the armored boot that protects you from
shooting yourself in the foot, and now you're just shooting yourself
in the foot.
I appreciate that it is frustrating to find that the lock may be held
by a thread that isn't running because ddb has suspended all the other
CPUs and the scheduler. But just skipping the mutex_enter can't fix
that. If you want to be able to interrupt logic in the USB stack at
any point and still use the HCI's hardware registers and the driver
data structures, you need a different approach.
I did this, but it really needs ukbd_cnpollc / usbd_set_polling to
return the error so that the heartbeat logic in cnpollc can handle the
error as well.
(aarch64 as an examble - others ports would need a similar change)
Thoughts?
diff --git a/sys/arch/aarch64/aarch64/db_machdep.c b/sys/arch/aarch64/aarch64/db_machdep.c
index b8e9e7559075..a08ac13ccd00 100644
--- a/sys/arch/aarch64/aarch64/db_machdep.c
+++ b/sys/arch/aarch64/aarch64/db_machdep.c
@@ -236,7 +236,8 @@ const struct db_variable db_regs[] = {
};
const struct db_variable * const db_eregs = db_regs + __arraycount(db_regs);
-int db_active;
+int db_active = 0;
+int db_set_polling_errors;
void
@@ -1215,7 +1216,12 @@ kdb_trap(int type, struct trapframe *tf)
const int s = splhigh();
db_active++;
+ db_set_polling_errors = 0;
cnpollc(true);
+ if (db_set_polling_errors != 0) {
+ printf("Failed to set polling\n");
+ break;
+ }
db_trap(type, 0/*code*/);
cnpollc(false);
db_active--;
diff --git a/sys/dev/usb/usbdi.c b/sys/dev/usb/usbdi.c
index d01d4d182662..cbddacdb763d 100644
--- a/sys/dev/usb/usbdi.c
+++ b/sys/dev/usb/usbdi.c
@@ -1368,41 +1367,58 @@ usbd_dopoll(struct usbd_interface *iface)
/*
* This is for keyboard driver as well, which only operates in polling
- * mode from the ask root, etc., prompt and from DDB.
+ * mode from the ask root, etc., prompt, and from DDB.
*/
void
usbd_set_polling(struct usbd_device *dev, int on)
{
- if (!db_active)
- mutex_enter(dev->ud_bus->ub_lock);
-
/*
- * We call softint routine on polling transitions, so
- * that completed/failed transfers have their callbacks
- * called. In-progress transfers started before transition
- * remain flying, and their completion after transition
- * must be taken into account.
+ * If polling is being enabled outside of DDB then we're safe to take
+ * the lock.
*
- * The softint routine is called after enabling polling
- * and before disabling it, so that holding sc->sc_lock
- * is not required. DDB needs this because it cannot wait
- * to acquire sc->sc_lock from a suspended thread.
+ * If polling is being enabled by DDB then fail if the lock is held.
+ * DDB must have suspended a CPU that was running a thread which holds
+ * the lock. The status of the USB controller and its data structures
+ * must not be changed in this case. That is, it's not safe to enter
+ * polling.
*/
+
+ if (!db_active) {
+ mutex_enter(dev->ud_bus->ub_lock);
+ } else {
+ if (mutex_owned(dev->ud_bus->ub_lock)) {
+ db_set_polling_error();
+ return;
+ }
+ }
if (on) {
+ /*
+ * Enabling polling. If we're enabling for the first time,
+ * call the softint routine on transition with polling marked
+ * as enabled, and the lock held in the non-DDB case.
+ *
+ * Once polling is enabled, we must not hold the lock when we
+ * call the softint routine.
+ */
KASSERT(dev->ud_bus->ub_usepolling < __type_max(char));
dev->ud_bus->ub_usepolling++;
if (dev->ud_bus->ub_usepolling == 1)
dev->ud_bus->ub_methods->ubm_softint(dev->ud_bus);
} else {
+ /*
+ * Disabling polling. If we're disabling polling for
+ * the last time, then call the softint routine with polling
+ * marked as enabled, and the lock held in the non-DDB case.
+ */
KASSERT(dev->ud_bus->ub_usepolling > 0);
if (dev->ud_bus->ub_usepolling == 1)
dev->ud_bus->ub_methods->ubm_softint(dev->ud_bus);
dev->ud_bus->ub_usepolling--;
}
-
- if (!db_active)
+ if (!db_active) {
mutex_exit(dev->ud_bus->ub_lock);
+ }
}
Home |
Main Index |
Thread Index |
Old Index