usb: gadget: dummy_hcd: Fix regression due to hrtimer migration

PATCH In review Marcello Sylvester Bauer 2 participants 3 messages Jun 26, 2024 USB

Share & tools

Marcello Sylvester Bauer

author

2024-06-26 13:47 UTC #0 lore raw

cover usb: gadget: dummy_hcd: Fix regression due to hrtimer migration

The kernel CI bots such as syzbot and intel kernel bot reported a regression due to the migration of the transfare scheduler from timer list to hrtimer. The current assumption is that this is because timer list uses soft interrupt context. I have not been able to reproduce the regression consistently. So I'm submitting this patch in the hope that it solves the issue.

Do not apply the patch if any bot still reports the problem.

Marcello Sylvester Bauer (1):
usb: gadget: dummy_hcd: make hrtimer expire in soft irq context

 drivers/usb/gadget/udc/dummy_hcd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Signature · 1 line

2.45.2

Marcello Sylvester Bauer

author

2024-06-26 13:47 UTC #1 lore raw

PATCH 1/1 usb: gadget: dummy_hcd: make hrtimer expire in soft irq context

Since migrating to hrtime, some kernel test bots are reporting regressions like rcu stalls and inconsistent_lock_state. the listed cause is around usb_hcd_giveback_urb() and it's locking mechanism, which was not changed by the patch.

The main difference between the timer list timer and the current hrtimer integration is that the timer list timer callback runs in soft interrupt context.

This patch changes the hrtimer in the transfer scheduler to also use soft interrupt context.

Reported-by [email protected]

Reported-by kernel test robot <[email protected]>

Closes https://lore.kernel.org/oe-lkp/[email protected] open here

Fixes a7f3813e589f ("usb: gadget: dummy_hcd: Switch to hrtimer transfer scheduler")

Signed-off-by Marcello Sylvester Bauer <[email protected]>

---
 drivers/usb/gadget/udc/dummy_hcd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/usb/gadget/udc/dummy_hcd.c +6 -6

index f37b0d8386c1..cd25538e999e 100644
@@ -1304,7 +1304,7 @@ static int dummy_urb_enqueue(
 
 	/* kick the scheduler, it'll do the rest */
 	if (!hrtimer_active(&dum_hcd->timer))
-		hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS), HRTIMER_MODE_REL);
+		hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS), HRTIMER_MODE_REL_SOFT);
 
  done:
 	spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);

@@ -1325,7 +1325,7 @@ static int dummy_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	rc = usb_hcd_check_unlink_urb(hcd, urb, status);
 	if (!rc && dum_hcd->rh_state != DUMMY_RH_RUNNING &&
 			!list_empty(&dum_hcd->urbp_list))
-		hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL);
+		hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL_SOFT);
 
 	spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);
 	return rc;

@@ -1995,7 +1995,7 @@ static enum hrtimer_restart dummy_timer(struct hrtimer *t)
 		dum_hcd->udev = NULL;
 	} else if (dum_hcd->rh_state == DUMMY_RH_RUNNING) {
 		/* want a 1 msec delay here */
-		hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS), HRTIMER_MODE_REL);
+		hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS), HRTIMER_MODE_REL_SOFT);
 	}
 
 	spin_unlock_irqrestore(&dum->lock, flags);

@@ -2389,7 +2389,7 @@ static int dummy_bus_resume(struct usb_hcd *hcd)
 		dum_hcd->rh_state = DUMMY_RH_RUNNING;
 		set_link_state(dum_hcd);
 		if (!list_empty(&dum_hcd->urbp_list))
-			hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL);
+			hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL_SOFT);
 		hcd->state = HC_STATE_RUNNING;
 	}
 	spin_unlock_irq(&dum_hcd->dum->lock);

@@ -2467,7 +2467,7 @@ static DEVICE_ATTR_RO(urbs);
 
 static int dummy_start_ss(struct dummy_hcd *dum_hcd)
 {
-	hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	dum_hcd->timer.function = dummy_timer;
 	dum_hcd->rh_state = DUMMY_RH_RUNNING;
 	dum_hcd->stream_en_ep = 0;

@@ -2497,7 +2497,7 @@ static int dummy_start(struct usb_hcd *hcd)
 		return dummy_start_ss(dum_hcd);
 
 	spin_lock_init(&dum_hcd->dum->lock);
-	hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	dum_hcd->timer.function = dummy_timer;
 	dum_hcd->rh_state = DUMMY_RH_RUNNING;

Signature · 1 line

2.45.2

Alan Stern

2024-06-26 14:00 UTC #2 lore raw

In reply to Marcello Sylvester Bauer

Quoted 8 lines · Marcello Sylvester Bauer

The kernel CI bots such as syzbot and intel kernel bot reported a regression due to the migration of the transfare scheduler from timer list to hrtimer. The current assumption is that this is because timer list uses soft interrupt context. I have not been able to reproduce the regression consistently. So I'm submitting this patch in the hope that it solves the issue.
Do not apply the patch if any bot still reports the problem.

You should send the patch to syzbot and have it run its test. Then you'll know whether the patch works.

Alan Stern

Quoted 10 lines

Marcello Sylvester Bauer (1):
usb: gadget: dummy_hcd: make hrtimer expire in soft irq context
 drivers/usb/gadget/udc/dummy_hcd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Signature · 1 line
2.45.2