| Message ID | 20240917-hotplug-drm-bridge-v4-6-bc4dfee61be6@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for GE SUNH hot-pluggable connector | expand |
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
diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c index c7aefcd6e4e3..bfbd80728036 100644 --- a/drivers/video/backlight/led_bl.c +++ b/drivers/video/backlight/led_bl.c @@ -209,6 +209,19 @@ static int led_bl_probe(struct platform_device *pdev) return PTR_ERR(priv->bl_dev); } + for (i = 0; i < priv->nb_leds; i++) { + struct device_link *link; + + link = device_link_add(&pdev->dev, priv->leds[0]->dev->parent, + DL_FLAG_AUTOREMOVE_CONSUMER); + if (!link) { + dev_err(&pdev->dev, "Failed to add devlink (consumer %s, supplier %s)\n", + dev_name(&pdev->dev), dev_name(priv->leds[0]->dev->parent)); + backlight_device_unregister(priv->bl_dev); + return -EINVAL; + } + } + for (i = 0; i < priv->nb_leds; i++) { mutex_lock(&priv->leds[i]->led_access); led_sysfs_disable(priv->leds[i]);
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 Fix by adding a devlink between the consuming led-backlight device and the supplying LED device. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- This patch first appeared in v4. --- drivers/video/backlight/led_bl.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)