Re: "leds: trigger: use RCU to protect the led_cdevs list" triggers RCU error checks

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

> Sorry to bring the bearer of bad news, but your commit 2a5a8fa8b23
> ("leds: trigger: use RCU to protect the led_cdevs list"),
> is causing the following RCU warning when used with blinking
> triggers on I2C LED controllers which support hw blinking.

Err, well, surely that is a pre-existing driver bug then?

> The specific problem is drivers/leds/led-triggers.c:
> led_trigger_blink_setup() which does:
> 
>         rcu_read_lock();
>         list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) {
>                 if (oneshot)
>                         led_blink_set_oneshot(led_cdev, delay_on, delay_off,
>                                               invert);
>                 else
>                         led_blink_set(led_cdev, delay_on, delay_off);
>         }
>         rcu_read_unlock();
> 
> And that led_blink_set() call then hits this path:
> 
>         if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
>             led_cdev->blink_set &&
>             !led_cdev->blink_set(led_cdev, delay_on, delay_off))
>                 return;
> 
> Which calls directly into the LED controller driver

Sure, so far so good.

> which talks to the LED controller over I2C which may sleep.

Which seems to me was already wrong before my patch, since the code was:

       read_lock_irqsave(&trig->leddev_list_lock, flags);
       list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
               if (oneshot)
                       led_blink_set_oneshot(led_cdev, delay_on, delay_off,
                                             invert);
               else
                       led_blink_set(led_cdev, delay_on, delay_off);
       }
       read_unlock_irqrestore(&trig->leddev_list_lock, flags);


Surely, the code wasn't allowed to sleep in an _irqsave() section? You'd
just see a different check complain, rather than about RCU, I guess.


So maybe you're bringing bad news, but I don't think it's for me ;-) I
don't see cht_wc_leds_brightness_get() or a driver/module called
leds_cht_wcove even in linux-next, so I guess you should look wherever
you got _that_ from.

johannes