[PATCH v2 0/3] drm: fix i2c adapter device driver user counter

Discussion:

Vladimir Zapolskiy

2015-09-22 21:46:37 UTC

of_find_i2c_adapter_by_node() call requires quite often missing
put_device(), and i2c_put_adapter() releases a device locked by
i2c_get_adapter() only.

Below is a common error reproduction scenario as a result of the
misusage described above (this is run on iMX6 platform with
HDMI and I2C bus drivers compiled as kernel modules for clearness):

***@mx6q:~# lsmod | grep i2c
i2c_imx 15348 0
***@mx6q:~# lsmod | grep dw_hdmi_imx
dw_hdmi_imx 3567 0
dw_hdmi 15850 1 dw_hdmi_imx
imxdrm 8610 3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
***@mx6q:~# rmmod dw_hdmi_imx
***@mx6q:~# lsmod | grep i2c
i2c_imx 15348 -1

^^^^^

***@mx6q:~# rmmod i2c_imx
rmmod: ERROR: Module i2c_imx is in use

To fix existing users of these interfaces use of_get_i2c_adapter_by_node()
interface, which is similar to i2c_get_adapter() in sense that an I2C bus
device driver found and locked by a user can be correctly unlocked by
i2c_put_adapter() call.

Changes from v1 to v2:
- none, this series is a straightforward bugfix, v1 was a blend of
I2C core changes, bugfixes and improvements

The change is based on dri/drm-next.

Vladimir Zapolskiy (3):
drm: dw_hdmi: use of_get_i2c_adapter_by_node interface
drm: tilcdc: use of_get_i2c_adapter_by_node interface
drm: tegra: use of_get_i2c_adapter_by_node interface

drivers/gpu/drm/bridge/dw_hdmi.c | 14 +++++++++-----
drivers/gpu/drm/tegra/output.c | 23 ++++++++++++-----------
drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 6 ++----
3 files changed, 23 insertions(+), 20 deletions(-)

--
2.5.0

Vladimir Zapolskiy

2015-09-22 21:48:10 UTC

Permalink

This change is needed to properly lock I2C bus driver, which serves DDC.

The change fixes an overflow over zero of I2C bus driver user counter:

***@mx6q:~# lsmod | grep i2c
i2c_imx 15348 0
***@mx6q:~# lsmod | grep dw_hdmi_imx
dw_hdmi_imx 3567 0
dw_hdmi 15850 1 dw_hdmi_imx
imxdrm 8610 3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
***@mx6q:~# rmmod dw_hdmi_imx
***@mx6q:~# lsmod | grep i2c
i2c_imx 15348 -1

^^^^^

***@mx6q:~# rmmod i2c_imx
rmmod: ERROR: Module i2c_imx is in use

Note that prior to this change put_device() coupled with
of_find_i2c_adapter_by_node() was missing on error path of
dw_hdmi_bind(), added i2c_put_adapter() there along with the change.

Signed-off-by: Vladimir Zapolskiy <***@mentor.com>
Cc: Russell King <rmk+***@arm.linux.org.uk>
Cc: Philipp Zabel <***@pengutronix.de>
Cc: Andy Yan <***@rock-chips.com>
---
Changes from v1 to v2:
- none

drivers/gpu/drm/bridge/dw_hdmi.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 0083d4e..c2d804f 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1638,7 +1638,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master,

ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
if (ddc_node) {
- hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
+ hdmi->ddc = of_get_i2c_adapter_by_node(ddc_node);
of_node_put(ddc_node);
if (!hdmi->ddc) {
dev_dbg(hdmi->dev, "failed to read ddc node\n");
@@ -1650,20 +1650,22 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
}

hdmi->regs = devm_ioremap_resource(dev, iores);
- if (IS_ERR(hdmi->regs))
- return PTR_ERR(hdmi->regs);
+ if (IS_ERR(hdmi->regs)) {
+ ret = PTR_ERR(hdmi->regs);
+ goto err_res;
+ }

hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr");
if (IS_ERR(hdmi->isfr_clk)) {
ret = PTR_ERR(hdmi->isfr_clk);
dev_err(hdmi->dev, "Unable to get HDMI isfr clk: %d\n", ret);
- return ret;
+ goto err_res;
}

ret = clk_prepare_enable(hdmi->isfr_clk);
if (ret) {
dev_err(hdmi->dev, "Cannot enable HDMI isfr clock: %d\n", ret);
- return ret;
+ goto err_res;
}

hdmi->iahb_clk = devm_clk_get(hdmi->dev, "iahb");
@@ -1729,6 +1731,8 @@ err_iahb:
clk_disable_unprepare(hdmi->iahb_clk);
err_isfr:
clk_disable_unprepare(hdmi->isfr_clk);
+err_res:
+ i2c_put_adapter(hdmi->ddc);

return ret;
}

--
2.5.0

Vladimir Zapolskiy

2015-10-12 13:14:01 UTC

Permalink

David, Russel,

ping.

Post by Vladimir Zapolskiy
This change is needed to properly lock I2C bus driver, which serves DDC.
i2c_imx 15348 0
dw_hdmi_imx 3567 0
dw_hdmi 15850 1 dw_hdmi_imx
imxdrm 8610 3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
i2c_imx 15348 -1
^^^^^
rmmod: ERROR: Module i2c_imx is in use
Note that prior to this change put_device() coupled with
of_find_i2c_adapter_by_node() was missing on error path of
dw_hdmi_bind(), added i2c_put_adapter() there along with the change.

--
With best wishes,
Vladimir

Vladimir Zapolskiy

2015-09-22 21:48:11 UTC

Permalink

This change is needed to properly lock I2C bus driver, which serves DDC.

Prior to this change i2c_put_adapter() is misused, which may lead
to an overflow over zero of I2C bus driver user counter.

Signed-off-by: Vladimir Zapolskiy <***@mentor.com>
---
Changes from v1 to v2:
- none

drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index 354c47c..4dc78c7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -348,15 +348,13 @@ static int tfp410_probe(struct platform_device *pdev)
goto fail;
}

- tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
+ tfp410_mod->i2c = of_get_i2c_adapter_by_node(i2c_node);
+ of_node_put(i2c_node);
if (!tfp410_mod->i2c) {
dev_err(&pdev->dev, "could not get i2c\n");
- of_node_put(i2c_node);
goto fail;
}

- of_node_put(i2c_node);
-
tfp410_mod->gpio = of_get_named_gpio_flags(node, "powerdn-gpio",
0, NULL);
if (IS_ERR_VALUE(tfp410_mod->gpio)) {

--
2.5.0

Vladimir Zapolskiy

2015-09-22 21:48:12 UTC

Permalink

This change is needed to properly lock I2C bus driver, which serves DDC.

On release of_get_i2c_adapter_by_node() requires i2c_put_adapter() call.

Note, that prior to the change put_device() coupled with
of_find_i2c_adapter_by_node() was missing on error path of
tegra_output_probe().

Signed-off-by: Vladimir Zapolskiy <***@mentor.com>
Cc: Thierry Reding <***@gmail.com>
Cc: Terje Bergström <***@nvidia.com>
---
Changes from v1 to v2:
- converted two of_node_put(ddc) calls into one

drivers/gpu/drm/tegra/output.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 46664b6..9f3cec5 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -120,14 +120,12 @@ int tegra_output_probe(struct tegra_output *output)

ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
if (ddc) {
- output->ddc = of_find_i2c_adapter_by_node(ddc);
+ output->ddc = of_get_i2c_adapter_by_node(ddc);
+ of_node_put(ddc);
if (!output->ddc) {
err = -EPROBE_DEFER;
- of_node_put(ddc);
return err;
}
-
- of_node_put(ddc);
}

output->hpd_gpio = of_get_named_gpio_flags(output->of_node,
@@ -140,14 +138,13 @@ int tegra_output_probe(struct tegra_output *output)
"HDMI hotplug detect");
if (err < 0) {
dev_err(output->dev, "gpio_request_one(): %d\n", err);
- return err;
+ goto i2c_release;
}

err = gpio_to_irq(output->hpd_gpio);
if (err < 0) {
dev_err(output->dev, "gpio_to_irq(): %d\n", err);
- gpio_free(output->hpd_gpio);
- return err;
+ goto gpio_release;
}

output->hpd_irq = err;
@@ -160,8 +157,7 @@ int tegra_output_probe(struct tegra_output *output)
if (err < 0) {
dev_err(output->dev, "failed to request IRQ#%u: %d\n",
output->hpd_irq, err);
- gpio_free(output->hpd_gpio);
- return err;
+ goto gpio_release;
}

output->connector.polled = DRM_CONNECTOR_POLL_HPD;
@@ -175,6 +171,12 @@ int tegra_output_probe(struct tegra_output *output)
}

return 0;
+
+ gpio_release:
+ gpio_free(output->hpd_gpio);
+ i2c_release:
+ i2c_put_adapter(output->ddc);
+ return err;
}

void tegra_output_remove(struct tegra_output *output)
@@ -184,8 +186,7 @@ void tegra_output_remove(struct tegra_output *output)
gpio_free(output->hpd_gpio);
}

- if (output->ddc)
- put_device(&output->ddc->dev);
+ i2c_put_adapter(output->ddc);
}

int tegra_output_init(struct drm_device *drm, struct tegra_output *output)

--
2.5.0

Vladimir Zapolskiy

2015-10-12 13:15:12 UTC

Permalink

David, Russell,

ping.

Post by Vladimir Zapolskiy
of_find_i2c_adapter_by_node() call requires quite often missing
put_device(), and i2c_put_adapter() releases a device locked by
i2c_get_adapter() only.
Below is a common error reproduction scenario as a result of the
misusage described above (this is run on iMX6 platform with
i2c_imx 15348 0
dw_hdmi_imx 3567 0
dw_hdmi 15850 1 dw_hdmi_imx
imxdrm 8610 3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
i2c_imx 15348 -1
^^^^^
rmmod: ERROR: Module i2c_imx is in use
To fix existing users of these interfaces use of_get_i2c_adapter_by_node()
interface, which is similar to i2c_get_adapter() in sense that an I2C bus
device driver found and locked by a user can be correctly unlocked by
i2c_put_adapter() call.
- none, this series is a straightforward bugfix, v1 was a blend of
I2C core changes, bugfixes and improvements
The change is based on dri/drm-next.
drm: dw_hdmi: use of_get_i2c_adapter_by_node interface
drm: tilcdc: use of_get_i2c_adapter_by_node interface
drm: tegra: use of_get_i2c_adapter_by_node interface
drivers/gpu/drm/bridge/dw_hdmi.c | 14 +++++++++-----
drivers/gpu/drm/tegra/output.c | 23 ++++++++++++-----------
drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 6 ++----
3 files changed, 23 insertions(+), 20 deletions(-)

--
With best wishes,
Vladimir

Vladimir Zapolskiy

2015-11-02 15:10:55 UTC

Permalink

David, Russell,

ping.

Post by Vladimir Zapolskiy
David, Russell,
ping.

--
With best wishes,
Vladimir

Vladimir Zapolskiy

2015-11-30 14:19:12 UTC

Permalink

David, Russell,

ping. No response for more than 2 months.

Post by Vladimir Zapolskiy
David, Russell,
ping.

--
With best wishes,
Vladimir