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

Hello Daniel,

I wonder whether you remember about this conversation...

On Fri, 20 Sep 2024 14:41:13 +0200
Luca Ceresoli [off-list ref] wrote:

Hello Daniel,

On Thu, 19 Sep 2024 14:43:23 +0200
Daniel Thompson [off-list ref] wrote:

quoted

On Tue, Sep 17, 2024 at 10:53:10AM +0200, Luca Ceresoli wrote:  

quoted

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/ (local)

Best regards,
Luca

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