Re: [PATCH] kcov, usb: Fix invalid context sleep in softirq path on PREEMPT_RT

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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_internal.h#L125
>                 #else
>                   https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/local_lock_internal.h#L235 // 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#L351
>               #else
>                 https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/spinlock_rt.h#L42 // 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