Re: [PATCH v4 6/8] backlight: led-backlight: add devlink to supplier LEDs

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Daniel,

I wonder whether you remember about this conversation...

On Fri, 20 Sep 2024 14:41:13 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> Hello Daniel,
> 
> On Thu, 19 Sep 2024 14:43:23 +0200
> Daniel Thompson <daniel.thompson@linaro.org> wrote:
> 
> > On Tue, Sep 17, 2024 at 10:53:10AM +0200, Luca Ceresoli wrote:  
> > > led-backlight is a consumer of one or multiple LED class devices, but no
> > > devlink is created for such supplier-producer relationship. One consequence
> > > is that removal ordered is not correctly enforced.
> > >
> > > Issues happen for example with the following sections in a device tree
> > > overlay:
> > >
> > >     // An LED driver chip
> > >     pca9632@62 {
> > >         compatible = "nxp,pca9632";
> > >         reg = <0x62>;
> > >
> > > 	// ...
> > >
> > >         addon_led_pwm: led-pwm@3 {
> > >             reg = <3>;
> > >             label = "addon:led:pwm";
> > >         };
> > >     };
> > >
> > >     backlight-addon {
> > >         compatible = "led-backlight";
> > >         leds = <&addon_led_pwm>;
> > >         brightness-levels = <255>;
> > >         default-brightness-level = <255>;
> > >     };
> > >
> > > On removal of the above overlay, the LED driver can be removed before the
> > > backlight device, resulting in:
> > >
> > >     Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> > >     ...
> > >     Call trace:
> > >      led_put+0xe0/0x140
> > >      devm_led_release+0x6c/0x98    
> > 
> > This looks like the object became invalid whilst we were holding a reference
> > to it. Is that reasonable? Put another way, is using devlink here fixing a
> > bug or merely hiding one?  
> 
> Thanks for your comment.
> 
> Hervé and I just had a look at the code and there actually might be a
> bug here, which we will be investigating (probably next week).
> 
> Still I think the devlink needs to be added to describe the
> relationship between the supplier (LED) and consumer (backlight).

It took "slightly more" than "next week", but we are here finally. In
reality this topics went pretty much forgotten until Alexander
Sverdlin's feedback [0].

About your concern, I'm not totally sure devlink is the tool expected
to solve this issue, but if it isn't I don't know any other tool that
should.

In other words, because devlink is exactly meant to represent
supplier-consumer relationships and enforce them to be respected, it
seems the appropriate tool. Moreover devlink already handles such
relationships quite well in many cases, and takes care of removing
consumers before their suppliers, when suppliers get removed.

One missing piece in devlink is it doesn't (yet) handle class devices
correctly. When the supplier is a class device (such as the LED device
in this case), then devlink creates a link to the parent of the
supplier, and not the supplier itself.

This problem is well known and it is under Saravana's radar. Adding
such devlinks at the device core level would be of course be the best
and most generic solution, but it seems to be much more tricky that it
may look. So other drivers and subsystems are "manually" creating
devlinks, to have the right links in place until devlink can figure
them out automatically. Some examples ('git grep device_link_add' for
more):

  https://elixir.bootlin.com/linux/v6.14.7/source/drivers/pwm/core.c#L1660
  https://elixir.bootlin.com/linux/v6.14.7/source/drivers/iio/industrialio-backend.c#L710
  https://elixir.bootlin.com/linux/v6.14.7/source/drivers/pmdomain/imx/gpc.c#L204

I hope this clarifies the need for this patch.

I am going to send this patch alone in a moment, detached from the
entire series because it is orthogonal.

[0]
https://lore.kernel.org/all/fa87471d31a62017067d4c3ba559cf79d6c3afec.camel@siemens.com/

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com