On Fri, 18 Aug 2023, Jacek Anaszewski wrote:
> Hi Marek,
>
> On 8/2/23 18:13, Marek Behún wrote:
> > On Wed, 2 Aug 2023 18:07:47 +0200
> > Marek Behún <kabel@xxxxxxxxxx> wrote:
> >
> > > +static int omnia_hwtrig_activate(struct led_classdev *cdev)
> > > +{
> > > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> > > + struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
> > > + struct omnia_led *led = to_omnia_led(mc_cdev);
> > > + int err = 0;
> > > +
> > > + mutex_lock(&leds->lock);
> > > +
> > > + if (!led->on) {
> > > + /*
> > > + * If the LED is off (brightness was set to 0), the last
> > > + * configured color was not necessarily sent to the MCU.
> > > + * Recompute with max_brightness and send if needed.
> > > + */
> > > + led_mc_calc_color_components(mc_cdev, cdev->max_brightness);
> > > +
> > > + if (omnia_led_channels_changed(led))
> > > + err = omnia_led_send_color_cmd(leds->client, led);
> > > + }
> > > +
> > > + if (!err) {
> > > + /* put the LED into MCU controlled mode */
> > > + err = omnia_cmd_write(leds->client, CMD_LED_MODE,
> > > + CMD_LED_MODE_LED(led->reg));
> > > + if (!err)
> > > + led->hwtrig = true;
> > > + }
> > > +
> > > + mutex_unlock(&leds->lock);
> > > +
> > > + return err;
> > > +}
> >
> > Pavel, Lee, here I wanted to ask: can the .activate() method of a LED
> > trigger sleep? The .brightness_set() method of a LED cannot sleep, and
> > therefore we have .brightness_set_blocking() method, which schedules
> > the potentially sleeping call into a work. But there is no such
> > mechanism for the LED trigger .activate() method.
> >
> > I looked at the LED core code, and it does not seem to me that
> > .activate() sleeping would cause issues, besides keeping trigger list
> > lock locked...
> >
> > Note that previously none of the LED triggers in drivers/leds/trigger
> > sleeped in .activate(), but recently the ethernet PHY subsystem was
> > wired into the netdev trigger, which may cause the .activate() method
> > to do a PHY transfer, which AFAIK may sleep...
>
> In general led_set_brightness() can't sleep because it is called from
> triggers in atomic contexts, which shouldn't be the case for activate().
>
> .activate() is called from led_trigger_{set|remove}() which is called from
> led_classdev_{register|unregister}) and from sysfs trigger attr's
> led_trigger_write() handler, so no risk here.
>
> Triggers already call e.g. kzalloc() in .activate() which may sleep.
Thanks for stepping in Jacek. I really appreciate your help.
--
Lee Jones [李琼斯]
