Skip to contentSkip to conversation

drm/panel-edp: take i2c adapter module reference

PATCH Applied Johan Hovold 3 participants 9 messages Jul 17 – Jul 24

Subsystems (5)
Stable branch DRM drivers DRM misc DRM panel drivers +1 more subsystems
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

v2 cover drm/panel-edp: take i2c adapter module reference

The i2c subsystem currently blocks during adapter deregistration whenever there are consumers holding a reference.

Switch to using of_get_i2c_adapter_by_node() which also takes a reference to the adapter module so that an attempt to unload the module while in use fails gracefully instead of blocking uninterruptibly.

Johan

Changes in v2:
- fix a pre-existing leak in a corner case flagged by Sashiko that Doug thought was worth fixing
- rebase module reference patch on top of that fix

Johan Hovold (2):
drm/panel-edp: fix i2c adapter leak on probe failure drm/panel-edp: take i2c adapter module reference

 drivers/gpu/drm/panel/panel-edp.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)
Signature · 1 line
2.54.0
author
#1 lore raw

PATCH v2 1/2 drm/panel-edp: fix i2c adapter leak on probe failure

Make sure to drop the i2c adapter reference on probe failure (e.g. probe deferral) and on driver unbind also if a devicetree redundantly uses the 'ddc-i2c-bus' property to point to the aux ddc bus.

Fixes cc5a3fc041f0 ("drm/panel: panel-simple: Stash DP AUX bus; allow using it for DDC")
Cc [email protected] # 5.15
Reported-by Douglas Anderson <[email protected]>
Link https://lore.kernel.org/r/CAD=FV=VZPhzHU+Pet2m3L+Pqc7mOPfZC-f5p0OuNL79wNZPxRg@mail.gmail.com open here
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gpu/drm/panel/panel-edp.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/gpu/drm/panel/panel-edp.c +13 -6
index aa27d6cd932e..ac815729aa83 100644
@@ -831,6 +831,13 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
 	return 0;
 }
 
+static void panel_edp_put_adapter(void *_adap)
+{
+	struct i2c_adapter *adap = _adap;
+
+	put_device(&adap->dev);
+}
+
 static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
 			   struct drm_dp_aux *aux)
 {

@@ -878,6 +885,11 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
 
 		if (!panel->ddc)
 			return -EPROBE_DEFER;
+
+		err = devm_add_action_or_reset(dev, panel_edp_put_adapter,
+					       panel->ddc);
+		if (err)
+			return err;
 	} else if (aux) {
 		panel->ddc = &aux->ddc;
 	}

@@ -889,7 +901,7 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
 
 	err = drm_panel_of_backlight(&panel->base);
 	if (err)
-		goto err_finished_ddc_init;
+		return err;
 
 	/*
 	 * We use runtime PM for prepare / unprepare since those power the panel

@@ -936,9 +948,6 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
 err_finished_pm_runtime:
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
-err_finished_ddc_init:
-	if (panel->ddc && (!panel->aux || panel->ddc != &panel->aux->ddc))
-		put_device(&panel->ddc->dev);
 
 	return err;
 }

@@ -982,8 +991,6 @@ static void panel_edp_remove(struct device *dev)
 
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
-	if (panel->ddc && (!panel->aux || panel->ddc != &panel->aux->ddc))
-		put_device(&panel->ddc->dev);
 
 	drm_edid_free(panel->drm_edid);
 	panel->drm_edid = NULL;
Signature · 1 line
2.54.0
AI review
#2 lore raw

Re: [PATCH v2 1/2] drm/panel-edp: fix i2c adapter leak on probe failure

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The device can be left permanently powered on if the driver is unbound while an asynchronous autosuspend is pending. --

--- Patch [1]: [PATCH v2 1/2] drm/panel-edp: fix i2c adapter leak on probe failure --- commit 471fbc83c1856bd9eec16a225d0ec84f3032c113
Author: Johan Hovold <[email protected]>

drm/panel-edp: fix i2c adapter leak on probe failure

This commit fixes a reference leak on an i2c adapter during probe failure or driver unbind. It introduces a devres action to automatically drop the adapter reference, streamlining the cleanup path and removing manual puts.

Link https://lore.kernel.org/r/CAD=FV=VZPhzHU+Pet2m3L+Pqc7mOPfZC-f5p0OuNL79wNZPxRg@mail.gmail.com open here
Quoted 4 lines
drivers/gpu/drm/panel/panel-edp.c +0 -0
index f66474d3ef65e..775bd6af31ae7 100644

[ ... ]

drivers/gpu/drm/panel/panel-edp.c static void panel_edp_remove(struct device *dev) · line 994

Quoted 4 lines
drivers/gpu/drm/panel/panel-edp.c +0 -0
@@ -983,8 +992,6 @@ static void panel_edp_remove(struct device *dev)
 
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);

[Severity: High]
This is a pre-existing issue, but does this sequence leave the device permanently powered on if unbound while an autosuspend is pending?

During probe, pm_runtime_get_sync() is followed by pm_runtime_put_autosuspend() (e.g., in generic_edp_panel_probe()), leaving the device in RPM_ACTIVE for 1000ms. If the driver is unbound before this timer fires, pm_runtime_disable() cancels the pending autosuspend work without suspending the device.

This can leave the panel regulators and GPIOs permanently enabled, and trigger a kernel WARN_ON from devres during regulator_put() due to an unbalanced enable count. Should there be a synchronous suspend call prior to disabling runtime PM in the removal path?

Quoted 5 lines
drivers/gpu/drm/panel/panel-edp.c +0 -2
-	if (panel->ddc && (!panel->aux || panel->ddc != &panel->aux->ddc))
-		put_device(&panel->ddc->dev);
 
 	drm_edid_free(panel->drm_edid);
 	panel->drm_edid = NULL;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
reviewer Reviewed-by on v2 1/2
#3 lore raw

Re: [PATCH v2 1/2] drm/panel-edp: fix i2c adapter leak on probe failure

In reply to Johan Hovold

Hi,

Quoted 33 lines · Johan Hovold

Make sure to drop the i2c adapter reference on probe failure (e.g. probe deferral) and on driver unbind also if a devicetree redundantly uses the 'ddc-i2c-bus' property to point to the aux ddc bus.

Fixes cc5a3fc041f0 ("drm/panel: panel-simple: Stash DP AUX bus; allow using it for DDC")
Cc [email protected] # 5.15
Reported-by Douglas Anderson <[email protected]>
Link https://lore.kernel.org/r/CAD=FV=VZPhzHU+Pet2m3L+Pqc7mOPfZC-f5p0OuNL79wNZPxRg@mail.gmail.com open here
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gpu/drm/panel/panel-edp.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/gpu/drm/panel/panel-edp.c +7 -0
index aa27d6cd932e..ac815729aa83 100644
@@ -831,6 +831,13 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
        return 0;
 }
 
+static void panel_edp_put_adapter(void *_adap)
+{
+       struct i2c_adapter *adap = _adap;
+
+       put_device(&adap->dev);
+}
+
 static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
                           struct drm_dp_aux *aux)
 {

drivers/gpu/drm/panel/panel-edp.c static int panel_edp_probe(struct device *dev, const struct panel_desc *desc, · line 892

Quoted 8 lines
drivers/gpu/drm/panel/panel-edp.c +5 -0
@@ -878,6 +885,11 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
 
                if (!panel->ddc)
                        return -EPROBE_DEFER;
+
+               err = devm_add_action_or_reset(dev, panel_edp_put_adapter,
+                                              panel->ddc);
+               if (err)
+                       return err;

Yeah, devm makes this so much nicer. Thanks for fixing!

Reviewed-by Douglas Anderson <[email protected]>

I'll plan to apply this, probably to drm-misc-next next week. Unless someone objects, I won't land it -fixes because it's such a corner case.

-Doug

author
#4 lore raw

Re: [PATCH v2 1/2] drm/panel-edp: fix i2c adapter leak on probe failure

Quoted 12 lines · Doug Anderson

drivers/gpu/drm/panel/panel-edp.c static int panel_edp_probe(struct device *dev, const struct panel_desc *desc, · line 892

Quoted 9 lines
drivers/gpu/drm/panel/panel-edp.c +5 -0
@@ -878,6 +885,11 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
 
                if (!panel->ddc)
                        return -EPROBE_DEFER;
+
+               err = devm_add_action_or_reset(dev, panel_edp_put_adapter,
+                                              panel->ddc);
+               if (err)
+                       return err;

Yeah, devm makes this so much nicer. Thanks for fixing!

Quoted 5 lines
Reviewed-by Douglas Anderson <[email protected]>

I'll plan to apply this, probably to drm-misc-next next week. Unless someone objects, I won't land it -fixes because it's such a corner case.

Sounds good. Thanks.

Johan

reviewer Applied
#5 lore raw

Re: [PATCH v2 1/2] drm/panel-edp: fix i2c adapter leak on probe failure

Hi,

Quoted 15 lines · Johan Hovold
Quoted 12 lines · Doug Anderson

drivers/gpu/drm/panel/panel-edp.c static int panel_edp_probe(struct device *dev, const struct panel_desc *desc, · line 892

Quoted 9 lines
drivers/gpu/drm/panel/panel-edp.c +5 -0
@@ -878,6 +885,11 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
 
                if (!panel->ddc)
                        return -EPROBE_DEFER;
+
+               err = devm_add_action_or_reset(dev, panel_edp_put_adapter,
+                                              panel->ddc);
+               if (err)
+                       return err;

Yeah, devm makes this so much nicer. Thanks for fixing!

Quoted 7 lines
Quoted 5 lines
Reviewed-by Douglas Anderson <[email protected]>

I'll plan to apply this, probably to drm-misc-next next week. Unless someone objects, I won't land it -fixes because it's such a corner case.

Sounds good. Thanks.

Pushed to drm-misc-next:

[1/2] drm/panel-edp: fix i2c adapter leak on probe failure

      commit: e2a9e291275a74e309a21cbb1def6296a72d6aed
author
#6 lore raw

PATCH v2 2/2 drm/panel-edp: take i2c adapter module reference

In reply to Johan Hovold

The i2c subsystem currently blocks during adapter deregistration whenever there are consumers holding a reference.

Switch to using of_get_i2c_adapter_by_node() which also takes a reference to the adapter module so that an attempt to unload the module while in use fails gracefully instead of blocking uninterruptibly.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gpu/drm/panel/panel-edp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/panel/panel-edp.c +2 -2
index ac815729aa83..522bc665a11a 100644
@@ -835,7 +835,7 @@ static void panel_edp_put_adapter(void *_adap)
 {
 	struct i2c_adapter *adap = _adap;
 
-	put_device(&adap->dev);
+	i2c_put_adapter(adap);
 }
 
 static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,

@@ -880,7 +880,7 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
 
 	ddc = of_parse_phandle(dev->of_node, "ddc-i2c-bus", 0);
 	if (ddc) {
-		panel->ddc = of_find_i2c_adapter_by_node(ddc);
+		panel->ddc = of_get_i2c_adapter_by_node(ddc);
 		of_node_put(ddc);
 
 		if (!panel->ddc)
Signature · 1 line
2.54.0
reviewer Reviewed-by on v2 2/2
#7 lore raw

Hi,

Quoted 4 lines · Johan Hovold

The i2c subsystem currently blocks during adapter deregistration whenever there are consumers holding a reference.

Quoted 8 lines

Switch to using of_get_i2c_adapter_by_node() which also takes a reference to the adapter module so that an attempt to unload the module while in use fails gracefully instead of blocking uninterruptibly.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gpu/drm/panel/panel-edp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by Douglas Anderson <[email protected]>
reviewer Applied
#8 lore raw

Hi,

Quoted 12 lines · Doug Anderson

Hi,

Quoted 8 lines · Johan Hovold

The i2c subsystem currently blocks during adapter deregistration whenever there are consumers holding a reference.

Switch to using of_get_i2c_adapter_by_node() which also takes a reference to the adapter module so that an attempt to unload the module while in use fails gracefully instead of blocking uninterruptibly.

Quoted 6 lines
Quoted 4 lines
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gpu/drm/panel/panel-edp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by Douglas Anderson <[email protected]>

Pushed to drm-misc-next:

[2/2] drm/panel-edp: take i2c adapter module reference

      commit: c7b77a538e01bb3289ae9eec70fc315f99798594