On Sat, Jul 26, 2025 at 04:44:42PM +0900, Tetsuo Handa wrote:
On 2025/07/26 15:36, Greg Kroah-Hartman wrote:
Why is this only a USB thing? What is unique about it to trigger this issue?
I couldn't catch your question. But the answer could be that
__usb_hcd_giveback_urb() is a function which is a USB thing
and
kcov_remote_start_usb_softirq() is calling local_irq_save() despite CONFIG_PREEMPT_RT=y
as shown below.
static void __usb_hcd_giveback_urb(struct urb *urb) { (...snipped...) kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum) { if (in_serving_softirq()) { local_irq_save(flags); // calling local_irq_save() is wrong if CONFIG_PREEMPT_RT=y kcov_remote_start_usb(id) { kcov_remote_start(id) { kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_USB, id)) { (...snipped...) local_lock_irqsave(&kcov_percpu_data.lock, flags) { __local_lock_irqsave(lock, flags) { #ifndef CONFIG_PREEMPT_RT https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/local_lock_i... #else https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/local_lock_i... // not calling local_irq_save(flags) #endif } } (...snipped...) spin_lock(&kcov_remote_lock) { #ifndef CONFIG_PREEMPT_RT https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/spinlock.h#L... #else https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/spinlock_rt.... // mapped to rt_mutex which might sleep #endif } (...snipped...) } } } } } (...snipped...) }
Ok, but then how does the big comment section for kcov_remote_start_usb_softirq() work, where it explicitly states:
* 2. Disables interrupts for the duration of the coverage collection section. * This allows avoiding nested remote coverage collection sections in the * softirq context (a softirq might occur during the execution of a work in * the BH workqueue, which runs with in_serving_softirq() > 0). * For example, usb_giveback_urb_bh() runs in the BH workqueue with * interrupts enabled, so __usb_hcd_giveback_urb() might be interrupted in * the middle of its remote coverage collection section, and the interrupt * handler might invoke __usb_hcd_giveback_urb() again.
You are removing half of this function entirely, which feels very wrong to me as any sort of solution, as you have just said that all of that documentation entry is now not needed.
Are you sure this is ok?
thanks,
greg k-h