[PATCH v2 0/6] drm/panel: Handle the "panel is missing" case properly

[PATCH v2 0/6] drm/panel: Handle the "panel is missing" case properly

[Boris Brezillon]

Hello,

This is a new attempt at fixing the "panel is missing" issue (described
in this thread [1]). I lost track of Eric's proposal, but I recently
proposed to address this problem through a new ->detect() hook in the
panel_funcs interface [2], which was rejected.

So here is a new version based on the feedback I had from Daniel,
Thierry and Rob.

The idea is to allow of_drm_find_panel() to return -ENODEV and let the
DRM driver decide what to do with that (silently ignore the missing
component and register the DRM device, or fail to register the DRM
device).

Patch 1 is just a fix for an OF node ref leak I found in the tegra
driver while working on this patch series. It can be applied
independently but I send it here since patch 2 depends on it.

Patch 2 changes the semantic of of_drm_find_panel() so that it returns
an ERR_PTR() instead of NULL when the panel is not found. This way
we'll be able to differentiate the "panel is missing" from "panel has
not been probed yet" errors.

Patch 3 and 4 are adding new tests in of_drm_find_panel() and
drm_of_find_panel_or_bridge() to return -ENODEV when the status
property of the DT node is not set to "okay".

Patch 5 is patching the VC4 DSI encoder driver to gracefully handle the
-ENODEV case and allow the registration of the DRM device when the DSI
device is disabled.

And finally, patch 6 is modifying the rpi-touchscreen panel driver to
update the status property of the DT node when the device is not
reachable on the I2C bus. This way, the DSI encoder driver will get an
-ENODEV and the DRM device will be registered even if the DSI panel
is not connected.
Note that the status prop update is currently done in the I2C driver
instead of the I2C or device-model core because I think modifying the
behavior for all I2C devices (or all devices) is too risky.

Feel free to comment on this implementation

Thanks,

Boris

Changes since v1:
- Everything :-)

[1]https://lists.freedesktop.org/archives/dri-devel/2017-November/157688.html
[2]https://www.spinics.net/lists/dri-devel/msg174808.html

Boris Brezillon (6):
  drm/tegra: Fix a device_node leak when the DRM panel is not found
  drm/panel: Make of_drm_find_panel() return an ERR_PTR() instead of
    NULL
  drm/panel: Let of_drm_find_panel() return -ENODEV when the panel is
    disabled
  drm/of: Make drm_of_find_panel_or_bridge() fail when the device is
    disabled
  drm/vc4: Support the case where the DSI device is disabled
  drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

 drivers/gpu/drm/bridge/cdns-dsi.c                  |  2 +-
 drivers/gpu/drm/bridge/lvds-encoder.c              |  4 +--
 drivers/gpu/drm/drm_of.c                           | 13 ++++++--
 drivers/gpu/drm/drm_panel.c                        | 11 +++++--
 drivers/gpu/drm/exynos/exynos_dp.c                 |  9 ++++--
 drivers/gpu/drm/exynos/exynos_drm_dpi.c            |  9 ++++--
 drivers/gpu/drm/exynos/exynos_drm_dsi.c            |  6 ++--
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c          |  8 +++--
 drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c  |  4 +--
 .../gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c    | 10 +++++--
 drivers/gpu/drm/msm/dsi/dsi_host.c                 |  2 +-
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_lvds.c                |  9 ++++--
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c             |  2 +-
 drivers/gpu/drm/sti/sti_dvo.c                      |  7 +++--
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c             |  4 +--
 drivers/gpu/drm/tegra/dsi.c                        |  6 ++--
 drivers/gpu/drm/tegra/output.c                     | 17 ++++++-----
 drivers/gpu/drm/vc4/vc4_dsi.c                      | 15 ++++++++--
 include/drm/drm_panel.h                            |  2 +-
 20 files changed, 131 insertions(+), 44 deletions(-)

-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/6] drm/tegra: Fix a device_node leak when the DRM panel is not found

[Boris Brezillon]

of_node_put(panel) is not called when of_drm_find_panel(panel) returns
NULL, thus leaking the reference we hold on panel.

Fixes: 9be7d864cf07 ("drm/tegra: Implement panel support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/tegra/output.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index ffe34bd0bb9d..676fd394836f 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -110,10 +110,9 @@ int tegra_output_probe(struct tegra_output *output)
 	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
 	if (panel) {
 		output->panel = of_drm_find_panel(panel);
+		of_node_put(panel);
 		if (!output->panel)
 			return -EPROBE_DEFER;
-
-		of_node_put(panel);
 	}
 
 	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/6] drm/tegra: Fix a device_node leak when the DRM panel is not found

[Boris Brezillon]

of_node_put(panel) is not called when of_drm_find_panel(panel) returns
NULL, thus leaking the reference we hold on panel.

Fixes: 9be7d864cf07 ("drm/tegra: Implement panel support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/tegra/output.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index ffe34bd0bb9d..676fd394836f 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -110,10 +110,9 @@ int tegra_output_probe(struct tegra_output *output)
 	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
 	if (panel) {
 		output->panel = of_drm_find_panel(panel);
+		of_node_put(panel);
 		if (!output->panel)
 			return -EPROBE_DEFER;
-
-		of_node_put(panel);
 	}
 
 	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
-- 
2.14.1

Re: [PATCH v2 1/6] drm/tegra: Fix a device_node leak when the DRM panel is not found

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

On Thu, May 03, 2018 at 06:40:04PM +0200, Boris Brezillon wrote:
> of_node_put(panel) is not called when of_drm_find_panel(panel) returns
> NULL, thus leaking the reference we hold on panel.
> 
> Fixes: 9be7d864cf07 ("drm/tegra: Implement panel support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/tegra/output.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

I'm not sure this warrants a backport, it's a very minor reference leak
so doesn't really have an impact on system stability or functionality.

Since this patch is unrelated from the rest of the series, do you mind
if I pick this up into the drm/tegra tree?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/6] drm/tegra: Fix a device_node leak when the DRM panel is not found

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

On Fri, May 04, 2018 at 11:50:16AM +0200, Thierry Reding wrote:
> On Thu, May 03, 2018 at 06:40:04PM +0200, Boris Brezillon wrote:
> > of_node_put(panel) is not called when of_drm_find_panel(panel) returns
> > NULL, thus leaking the reference we hold on panel.
> > 
> > Fixes: 9be7d864cf07 ("drm/tegra: Implement panel support")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/tegra/output.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> I'm not sure this warrants a backport, it's a very minor reference leak
> so doesn't really have an impact on system stability or functionality.
> 
> Since this patch is unrelated from the rest of the series, do you mind
> if I pick this up into the drm/tegra tree?

Oh, nevermind, I just noticed that patch 2 depends on this, so:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/6] drm/tegra: Fix a device_node leak when the DRM panel is not found

[Boris Brezillon]

On Fri, 4 May 2018 11:50:16 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Thu, May 03, 2018 at 06:40:04PM +0200, Boris Brezillon wrote:
> > of_node_put(panel) is not called when of_drm_find_panel(panel) returns
> > NULL, thus leaking the reference we hold on panel.
> > 
> > Fixes: 9be7d864cf07 ("drm/tegra: Implement panel support")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/tegra/output.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)  
> 
> I'm not sure this warrants a backport, it's a very minor reference leak
> so doesn't really have an impact on system stability or functionality.
> 
> Since this patch is unrelated from the rest of the series, do you mind
> if I pick this up into the drm/tegra tree?

Sure, and feel free to remove the Cc-stable and Fixes tag if you think
they are not necessary.

[PATCH v2 2/6] drm/panel: Make of_drm_find_panel() return an ERR_PTR() instead of NULL

[Boris Brezillon]

Right now, the DRM panel logic returns NULL when a panel pointing to
the passed OF node is not present in the list of registered panels.

Most drivers interpret this NULL value as -EPROBE_DEFER, but we are
about to modify the semantic of of_drm_find_panel() and let the
framework return -ENODEV when the device node we're pointing to has
a status property that is not equal to "okay" or "ok".

Let's first patch the of_drm_find_panel() implementation to return
ERR_PTR(-EPROBE_DEFER) instead of NULL and patch all callers to replace
the '!panel' check by an 'IS_ERR(panel)' one.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/bridge/cdns-dsi.c                   |  2 +-
 drivers/gpu/drm/bridge/lvds-encoder.c               |  4 ++--
 drivers/gpu/drm/drm_of.c                            |  8 ++++++--
 drivers/gpu/drm/drm_panel.c                         |  6 ++++--
 drivers/gpu/drm/exynos/exynos_dp.c                  |  9 ++++++---
 drivers/gpu/drm/exynos/exynos_drm_dpi.c             |  9 ++++++---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c             |  6 ++++--
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c           |  8 +++++---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c   |  4 ++--
 drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 10 +++++++---
 drivers/gpu/drm/msm/dsi/dsi_host.c                  |  2 +-
 drivers/gpu/drm/rcar-du/rcar_lvds.c                 |  9 ++++++---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c              |  2 +-
 drivers/gpu/drm/sti/sti_dvo.c                       |  7 +++++--
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c              |  4 ++--
 drivers/gpu/drm/tegra/dsi.c                         |  6 ++++--
 drivers/gpu/drm/tegra/output.c                      | 18 +++++++++++-------
 include/drm/drm_panel.h                             |  2 +-
 18 files changed, 74 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
index c255fc3e1be5..2c5991cf5397 100644
--- a/drivers/gpu/drm/bridge/cdns-dsi.c
+++ b/drivers/gpu/drm/bridge/cdns-dsi.c
@@ -1152,7 +1152,7 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
 		np = of_node_get(dev->dev.of_node);
 
 	panel = of_drm_find_panel(np);
-	if (panel) {
+	if (!IS_ERR(panel)) {
 		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DSI);
 	} else {
 		bridge = of_drm_find_bridge(dev->dev.of_node);
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..f56c92f7af7c 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -68,9 +68,9 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 
 	panel = of_drm_find_panel(panel_node);
 	of_node_put(panel_node);
-	if (!panel) {
+	if (IS_ERR(panel)) {
 		dev_dbg(&pdev->dev, "panel not found, deferring probe\n");
-		return -EPROBE_DEFER;
+		return PTR_ERR(panel);
 	}
 
 	lvds_encoder->panel_bridge =
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 1fe122461298..f413fae6f6dc 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -239,9 +239,13 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 		return -ENODEV;
 
 	if (panel) {
-		*panel = of_drm_find_panel(remote);
-		if (*panel)
+		struct drm_panel *tmp_panel;
+
+		tmp_panel = of_drm_find_panel(remote);
+		if (!IS_ERR(tmp_panel)) {
+			*panel = tmp_panel;
 			ret = 0;
+		}
 	}
 
 	/* No panel found yet, check for a bridge next. */
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 308d442a531b..2d9e2684c6c8 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -135,7 +135,9 @@ EXPORT_SYMBOL(drm_panel_detach);
  * tree node. If a matching panel is found, return a pointer to it.
  *
  * Return: A pointer to the panel registered for the specified device tree
- * node or NULL if no panel matching the device tree node can be found.
+ * node or an ERR_PTR() if no panel matching the device tree node can be found.
+ * The only error that can be reported is -EPROBE_DEFER, meaning that the panel
+ * device has not been probed yet, and the caller should re-retry later.
  */
 struct drm_panel *of_drm_find_panel(const struct device_node *np)
 {
@@ -151,7 +153,7 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
 	}
 
 	mutex_unlock(&panel_lock);
-	return NULL;
+	return ERR_PTR(-EPROBE_DEFER);
 }
 EXPORT_SYMBOL(of_drm_find_panel);
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
index 86330f396784..962cad0276e5 100644
--- a/drivers/gpu/drm/exynos/exynos_dp.c
+++ b/drivers/gpu/drm/exynos/exynos_dp.c
@@ -231,10 +231,13 @@ static int exynos_dp_probe(struct platform_device *pdev)
 	/* This is for the backward compatibility. */
 	np = of_parse_phandle(dev->of_node, "panel", 0);
 	if (np) {
-		dp->plat_data.panel = of_drm_find_panel(np);
+		struct drm_panel *panel = of_drm_find_panel(np);
+
 		of_node_put(np);
-		if (!dp->plat_data.panel)
-			return -EPROBE_DEFER;
+		if (IS_ERR(panel))
+			return PTR_ERR(panel);
+
+		dp->plat_data.panel = panel;
 		goto out;
 	}
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
index 66945e0dc57f..e9c7d1c1cf8f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
@@ -239,9 +239,12 @@ struct drm_encoder *exynos_dpi_probe(struct device *dev)
 	}
 
 	if (ctx->panel_node) {
-		ctx->panel = of_drm_find_panel(ctx->panel_node);
-		if (!ctx->panel)
-			return ERR_PTR(-EPROBE_DEFER);
+		struct drm_panel *panel = of_drm_find_panel(ctx->panel_node);
+
+		if (IS_ERR(panel))
+			return ERR_CAST(panel);
+
+		ctx->panel = panel;
 	}
 
 	return &ctx->encoder;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 7904ffa9abfb..96206deb86a0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1520,6 +1520,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
 	struct drm_device *drm = dsi->connector.dev;
+	struct drm_panel *panel;
 
 	/*
 	 * This is a temporary solution and should be made by more generic way.
@@ -1538,8 +1539,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	dsi->lanes = device->lanes;
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
-	dsi->panel = of_drm_find_panel(device->dev.of_node);
-	if (dsi->panel) {
+	panel = of_drm_find_panel(device->dev.of_node);
+	if (!IS_ERR(panel)) {
+		dsi->panel = panel;
 		drm_panel_attach(dsi->panel, &dsi->connector);
 		dsi->connector.status = connector_status_connected;
 	}
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
index c54806d08dd7..7753b3093472 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
@@ -146,10 +146,12 @@ int fsl_dcu_create_outputs(struct fsl_dcu_drm_device *fsl_dev)
 	/* This is for backward compatibility */
 	panel_node = of_parse_phandle(fsl_dev->np, "fsl,panel", 0);
 	if (panel_node) {
-		fsl_dev->connector.panel = of_drm_find_panel(panel_node);
+		panel = of_drm_find_panel(panel_node);
 		of_node_put(panel_node);
-		if (!fsl_dev->connector.panel)
-			return -EPROBE_DEFER;
+		if (IS_ERR(panel))
+			return PTR_ERR(panel);
+
+		fsl_dev->connector.panel = panel;
 		return fsl_dcu_attach_panel(fsl_dev, fsl_dev->connector.panel);
 	}
 
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
index 4a645926edb7..2bfb39082f54 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
@@ -341,7 +341,7 @@ static void mdp4_lcdc_encoder_disable(struct drm_encoder *encoder)
 	mdp4_write(mdp4_kms, REG_MDP4_LCDC_ENABLE, 0);
 
 	panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node);
-	if (panel) {
+	if (!IS_ERR(panel)) {
 		drm_panel_disable(panel);
 		drm_panel_unprepare(panel);
 	}
@@ -410,7 +410,7 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder *encoder)
 		dev_err(dev->dev, "failed to enable lcdc_clk: %d\n", ret);
 
 	panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node);
-	if (panel) {
+	if (!IS_ERR(panel)) {
 		drm_panel_prepare(panel);
 		drm_panel_enable(panel);
 	}
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
index e3b1c86b7aae..c7dd72b428f8 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
@@ -34,9 +34,13 @@ static enum drm_connector_status mdp4_lvds_connector_detect(
 	struct mdp4_lvds_connector *mdp4_lvds_connector =
 			to_mdp4_lvds_connector(connector);
 
-	if (!mdp4_lvds_connector->panel)
-		mdp4_lvds_connector->panel =
-			of_drm_find_panel(mdp4_lvds_connector->panel_node);
+	if (!mdp4_lvds_connector->panel) {
+		struct drm_panel *panel;
+
+		panel = of_drm_find_panel(mdp4_lvds_connector->panel_node);
+		if (!IS_ERR(panel))
+			mdp4_lvds_connector->panel = panel;
+	}
 
 	return mdp4_lvds_connector->panel ?
 			connector_status_connected :
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 7a03a9489708..fffc80b73966 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1881,7 +1881,7 @@ int msm_dsi_host_register(struct mipi_dsi_host *host, bool check_defer)
 		 * output
 		 */
 		if (check_defer && msm_host->device_node) {
-			if (!of_drm_find_panel(msm_host->device_node))
+			if (IS_ERR(of_drm_find_panel(msm_host->device_node)))
 				if (!of_drm_find_bridge(msm_host->device_node))
 					return -EPROBE_DEFER;
 		}
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3d2d3bbd1342..d77b8f8bcf94 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -430,9 +430,12 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 		if (!lvds->next_bridge)
 			ret = -EPROBE_DEFER;
 	} else {
-		lvds->panel = of_drm_find_panel(remote);
-		if (!lvds->panel)
-			ret = -EPROBE_DEFER;
+		struct drm_panel *panel = of_drm_find_panel(remote);
+
+		if (IS_ERR(panel))
+			ret = PTR_ERR(panel);
+		else
+			lvds->panel = panel;
 	}
 
 done:
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index d53d5a09547f..01642aaf6127 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -595,7 +595,7 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
 	dsi->panel = of_drm_find_panel(device->dev.of_node);
-	if (dsi->panel)
+	if (!IS_ERR(dsi->panel))
 		return drm_panel_attach(dsi->panel, &dsi->connector);
 
 	return -EINVAL;
diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
index a5979cd25cc7..3579c0e1ca1c 100644
--- a/drivers/gpu/drm/sti/sti_dvo.c
+++ b/drivers/gpu/drm/sti/sti_dvo.c
@@ -386,9 +386,12 @@ sti_dvo_connector_detect(struct drm_connector *connector, bool force)
 	DRM_DEBUG_DRIVER("\n");
 
 	if (!dvo->panel) {
-		dvo->panel = of_drm_find_panel(dvo->panel_node);
-		if (dvo->panel)
+		struct drm_panel *panel = of_drm_find_panel(dvo->panel_node);
+
+		if (!IS_ERR(panel)) {
+			dvo->panel = panel;
 			drm_panel_attach(dvo->panel, connector);
+		}
 	}
 
 	if (dvo->panel)
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index bfbf761f0c1d..ce388d7cebaa 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -812,8 +812,8 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 
 	dsi->device = device;
 	dsi->panel = of_drm_find_panel(device->dev.of_node);
-	if (!dsi->panel)
-		return -EINVAL;
+	if (IS_ERR(dsi->panel))
+		return PTR_ERR(dsi->panel);
 
 	dev_info(host->dev, "Attached device %s\n", device->name);
 
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 87c5d89bc9ba..0b1eee4b2fb1 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -1409,9 +1409,11 @@ static int tegra_dsi_host_attach(struct mipi_dsi_host *host,
 	 */
 	if (!dsi->master) {
 		struct tegra_output *output = &dsi->output;
+		struct drm_panel *panel;
 
-		output->panel = of_drm_find_panel(device->dev.of_node);
-		if (output->panel && output->connector.dev) {
+		panel = of_drm_find_panel(device->dev.of_node);
+		if (!IS_ERR(panel) && output->connector.dev) {
+			output->panel = panel;
 			drm_panel_attach(output->panel, &output->connector);
 			drm_helper_hpd_irq_event(output->connector.dev);
 		}
diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 676fd394836f..21d8737f8b49 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -101,18 +101,22 @@ static irqreturn_t hpd_irq(int irq, void *data)
 
 int tegra_output_probe(struct tegra_output *output)
 {
-	struct device_node *ddc, *panel;
+	struct device_node *ddc, *panelnp;
 	int err, size;
 
 	if (!output->of_node)
 		output->of_node = output->dev->of_node;
 
-	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
-	if (panel) {
-		output->panel = of_drm_find_panel(panel);
-		of_node_put(panel);
-		if (!output->panel)
-			return -EPROBE_DEFER;
+	panelnp = of_parse_phandle(output->of_node, "nvidia,panel", 0);
+	if (panelnp) {
+		struct drm_panel *panel = of_drm_find_panel(panelnp);
+
+		of_node_put(panelnp);
+
+		if (IS_ERR(panel))
+			return PTR_ERR(panel);
+
+		output->panel = panel;
 	}
 
 	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 14ac240a1f64..3bb91519462e 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -199,7 +199,7 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np);
 #else
 static inline struct drm_panel *of_drm_find_panel(const struct device_node *np)
 {
-	return NULL;
+	return ERR_PTR(-ENOTSUPP);
 }
 #endif
 
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/6] drm/panel: Make of_drm_find_panel() return an ERR_PTR() instead of NULL

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 17497 bytes --]

On Thu, May 03, 2018 at 06:40:05PM +0200, Boris Brezillon wrote:
> Right now, the DRM panel logic returns NULL when a panel pointing to
> the passed OF node is not present in the list of registered panels.
> 
> Most drivers interpret this NULL value as -EPROBE_DEFER, but we are
> about to modify the semantic of of_drm_find_panel() and let the
> framework return -ENODEV when the device node we're pointing to has
> a status property that is not equal to "okay" or "ok".
> 
> Let's first patch the of_drm_find_panel() implementation to return
> ERR_PTR(-EPROBE_DEFER) instead of NULL and patch all callers to replace
> the '!panel' check by an 'IS_ERR(panel)' one.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/bridge/cdns-dsi.c                   |  2 +-
>  drivers/gpu/drm/bridge/lvds-encoder.c               |  4 ++--
>  drivers/gpu/drm/drm_of.c                            |  8 ++++++--
>  drivers/gpu/drm/drm_panel.c                         |  6 ++++--
>  drivers/gpu/drm/exynos/exynos_dp.c                  |  9 ++++++---
>  drivers/gpu/drm/exynos/exynos_drm_dpi.c             |  9 ++++++---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c             |  6 ++++--
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c           |  8 +++++---
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c   |  4 ++--
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 10 +++++++---
>  drivers/gpu/drm/msm/dsi/dsi_host.c                  |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_lvds.c                 |  9 ++++++---
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c              |  2 +-
>  drivers/gpu/drm/sti/sti_dvo.c                       |  7 +++++--
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c              |  4 ++--
>  drivers/gpu/drm/tegra/dsi.c                         |  6 ++++--
>  drivers/gpu/drm/tegra/output.c                      | 18 +++++++++++-------
>  include/drm/drm_panel.h                             |  2 +-
>  18 files changed, 74 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
> index c255fc3e1be5..2c5991cf5397 100644
> --- a/drivers/gpu/drm/bridge/cdns-dsi.c
> +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> @@ -1152,7 +1152,7 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
>  		np = of_node_get(dev->dev.of_node);
>  
>  	panel = of_drm_find_panel(np);
> -	if (panel) {
> +	if (!IS_ERR(panel)) {
>  		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DSI);
>  	} else {
>  		bridge = of_drm_find_bridge(dev->dev.of_node);
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> index 75b0d3f6e4de..f56c92f7af7c 100644
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> @@ -68,9 +68,9 @@ static int lvds_encoder_probe(struct platform_device *pdev)
>  
>  	panel = of_drm_find_panel(panel_node);
>  	of_node_put(panel_node);
> -	if (!panel) {
> +	if (IS_ERR(panel)) {
>  		dev_dbg(&pdev->dev, "panel not found, deferring probe\n");
> -		return -EPROBE_DEFER;
> +		return PTR_ERR(panel);
>  	}
>  
>  	lvds_encoder->panel_bridge =
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 1fe122461298..f413fae6f6dc 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -239,9 +239,13 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>  		return -ENODEV;
>  
>  	if (panel) {
> -		*panel = of_drm_find_panel(remote);
> -		if (*panel)
> +		struct drm_panel *tmp_panel;
> +
> +		tmp_panel = of_drm_find_panel(remote);
> +		if (!IS_ERR(tmp_panel)) {
> +			*panel = tmp_panel;
>  			ret = 0;
> +		}

I think the introduction of this temporary variable makes the code hard
to read and the diff difficult to understand. Why not just stick with
the original style and make this:

		*panel = of_drm_find_panel(remote);
		if (IS_ERR(*panel))
			*panel = NULL;
		else
			ret = 0;

?

>  	}
>  
>  	/* No panel found yet, check for a bridge next. */
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 308d442a531b..2d9e2684c6c8 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -135,7 +135,9 @@ EXPORT_SYMBOL(drm_panel_detach);
>   * tree node. If a matching panel is found, return a pointer to it.
>   *
>   * Return: A pointer to the panel registered for the specified device tree
> - * node or NULL if no panel matching the device tree node can be found.
> + * node or an ERR_PTR() if no panel matching the device tree node can be found.
> + * The only error that can be reported is -EPROBE_DEFER, meaning that the panel
> + * device has not been probed yet, and the caller should re-retry later.

I think you can drop the extra re- from re-retry.

>   */
>  struct drm_panel *of_drm_find_panel(const struct device_node *np)
>  {
> @@ -151,7 +153,7 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
>  	}
>  
>  	mutex_unlock(&panel_lock);
> -	return NULL;
> +	return ERR_PTR(-EPROBE_DEFER);
>  }
>  EXPORT_SYMBOL(of_drm_find_panel);
>  #endif
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index 86330f396784..962cad0276e5 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -231,10 +231,13 @@ static int exynos_dp_probe(struct platform_device *pdev)
>  	/* This is for the backward compatibility. */
>  	np = of_parse_phandle(dev->of_node, "panel", 0);
>  	if (np) {
> -		dp->plat_data.panel = of_drm_find_panel(np);
> +		struct drm_panel *panel = of_drm_find_panel(np);
> +
>  		of_node_put(np);
> -		if (!dp->plat_data.panel)
> -			return -EPROBE_DEFER;
> +		if (IS_ERR(panel))
> +			return PTR_ERR(panel);

I don't see the point in the extra variable here. dp is allocated using
devm_kzalloc(), so it will go away on the error return. You shouldn't
need to worry about keeping the dp->plat_data.panel untouched in case of
error.

> +
> +		dp->plat_data.panel = panel;
>  		goto out;
>  	}
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> index 66945e0dc57f..e9c7d1c1cf8f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> @@ -239,9 +239,12 @@ struct drm_encoder *exynos_dpi_probe(struct device *dev)
>  	}
>  
>  	if (ctx->panel_node) {
> -		ctx->panel = of_drm_find_panel(ctx->panel_node);
> -		if (!ctx->panel)
> -			return ERR_PTR(-EPROBE_DEFER);
> +		struct drm_panel *panel = of_drm_find_panel(ctx->panel_node);
> +
> +		if (IS_ERR(panel))
> +			return ERR_CAST(panel);
> +
> +		ctx->panel = panel;

Same here.

>  	}
>  
>  	return &ctx->encoder;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 7904ffa9abfb..96206deb86a0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1520,6 +1520,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>  {
>  	struct exynos_dsi *dsi = host_to_dsi(host);
>  	struct drm_device *drm = dsi->connector.dev;
> +	struct drm_panel *panel;
>  
>  	/*
>  	 * This is a temporary solution and should be made by more generic way.
> @@ -1538,8 +1539,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>  	dsi->lanes = device->lanes;
>  	dsi->format = device->format;
>  	dsi->mode_flags = device->mode_flags;
> -	dsi->panel = of_drm_find_panel(device->dev.of_node);
> -	if (dsi->panel) {
> +	panel = of_drm_find_panel(device->dev.of_node);
> +	if (!IS_ERR(panel)) {
> +		dsi->panel = panel;
>  		drm_panel_attach(dsi->panel, &dsi->connector);
>  		dsi->connector.status = connector_status_connected;
>  	}

It seems to be a potential problem here if dsi->panel is an ERR_PTR()-
encoded value, but I still think this becomes clearer if you do:

	dsi->panel = of_drm_find_panel(...);
	if (!IS_ERR(dsi->panel)) {
		...
	} else {
		dsi->panel = NULL;
	}

Or maybe even:

	dsi->panel = of_drm_find_panel(...);
	if (IS_ERR(dsi->panel))
		dsi->panel = NULL;

	if (dsi->panel) {
	}

That's more explicitly saying that the panel support is optional.

> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> index c54806d08dd7..7753b3093472 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> @@ -146,10 +146,12 @@ int fsl_dcu_create_outputs(struct fsl_dcu_drm_device *fsl_dev)
>  	/* This is for backward compatibility */
>  	panel_node = of_parse_phandle(fsl_dev->np, "fsl,panel", 0);
>  	if (panel_node) {
> -		fsl_dev->connector.panel = of_drm_find_panel(panel_node);
> +		panel = of_drm_find_panel(panel_node);
>  		of_node_put(panel_node);
> -		if (!fsl_dev->connector.panel)
> -			return -EPROBE_DEFER;
> +		if (IS_ERR(panel))
> +			return PTR_ERR(panel);
> +
> +		fsl_dev->connector.panel = panel;

Same here, fsl_dev goes away after the error return.

>  		return fsl_dcu_attach_panel(fsl_dev, fsl_dev->connector.panel);
>  	}
>  
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> index 4a645926edb7..2bfb39082f54 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> @@ -341,7 +341,7 @@ static void mdp4_lcdc_encoder_disable(struct drm_encoder *encoder)
>  	mdp4_write(mdp4_kms, REG_MDP4_LCDC_ENABLE, 0);
>  
>  	panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node);
> -	if (panel) {
> +	if (!IS_ERR(panel)) {
>  		drm_panel_disable(panel);
>  		drm_panel_unprepare(panel);
>  	}
> @@ -410,7 +410,7 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder *encoder)
>  		dev_err(dev->dev, "failed to enable lcdc_clk: %d\n", ret);
>  
>  	panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node);
> -	if (panel) {
> +	if (!IS_ERR(panel)) {
>  		drm_panel_prepare(panel);
>  		drm_panel_enable(panel);
>  	}
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> index e3b1c86b7aae..c7dd72b428f8 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> @@ -34,9 +34,13 @@ static enum drm_connector_status mdp4_lvds_connector_detect(
>  	struct mdp4_lvds_connector *mdp4_lvds_connector =
>  			to_mdp4_lvds_connector(connector);
>  
> -	if (!mdp4_lvds_connector->panel)
> -		mdp4_lvds_connector->panel =
> -			of_drm_find_panel(mdp4_lvds_connector->panel_node);
> +	if (!mdp4_lvds_connector->panel) {
> +		struct drm_panel *panel;
> +
> +		panel = of_drm_find_panel(mdp4_lvds_connector->panel_node);
> +		if (!IS_ERR(panel))
> +			mdp4_lvds_connector->panel = panel;
> +	}

Huh... this is hacky to begin with. Seems like this driver doesn't care
about waiting for the panel, it'll just retry probing the panel
everytime ->detect() is called on the connector until the panel has
shown up. That's not really how this is supposed to work, but it's been
there before your patch, so should be addressed separately.

>  
>  	return mdp4_lvds_connector->panel ?
>  			connector_status_connected :
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 7a03a9489708..fffc80b73966 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1881,7 +1881,7 @@ int msm_dsi_host_register(struct mipi_dsi_host *host, bool check_defer)
>  		 * output
>  		 */
>  		if (check_defer && msm_host->device_node) {
> -			if (!of_drm_find_panel(msm_host->device_node))
> +			if (IS_ERR(of_drm_find_panel(msm_host->device_node)))
>  				if (!of_drm_find_bridge(msm_host->device_node))
>  					return -EPROBE_DEFER;
>  		}

Again, pretty weird stuff going on here, prior to the patch. But I think
this needs to be changed to take the -ENODEV into account in the next
patch. As it is, this will continue to defer probe even if the panel
node is disabled.

> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 3d2d3bbd1342..d77b8f8bcf94 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -430,9 +430,12 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  		if (!lvds->next_bridge)
>  			ret = -EPROBE_DEFER;
>  	} else {
> -		lvds->panel = of_drm_find_panel(remote);
> -		if (!lvds->panel)
> -			ret = -EPROBE_DEFER;
> +		struct drm_panel *panel = of_drm_find_panel(remote);
> +
> +		if (IS_ERR(panel))
> +			ret = PTR_ERR(panel);
> +		else
> +			lvds->panel = panel;
>  	}

Similar to others above, I think this can easily be done without the
extra temporary variable.

>  
>  done:
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index d53d5a09547f..01642aaf6127 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -595,7 +595,7 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
>  	dsi->format = device->format;
>  	dsi->mode_flags = device->mode_flags;
>  	dsi->panel = of_drm_find_panel(device->dev.of_node);
> -	if (dsi->panel)
> +	if (!IS_ERR(dsi->panel))
>  		return drm_panel_attach(dsi->panel, &dsi->connector);
>  
>  	return -EINVAL;
> diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
> index a5979cd25cc7..3579c0e1ca1c 100644
> --- a/drivers/gpu/drm/sti/sti_dvo.c
> +++ b/drivers/gpu/drm/sti/sti_dvo.c
> @@ -386,9 +386,12 @@ sti_dvo_connector_detect(struct drm_connector *connector, bool force)
>  	DRM_DEBUG_DRIVER("\n");
>  
>  	if (!dvo->panel) {
> -		dvo->panel = of_drm_find_panel(dvo->panel_node);
> -		if (dvo->panel)
> +		struct drm_panel *panel = of_drm_find_panel(dvo->panel_node);
> +
> +		if (!IS_ERR(panel)) {
> +			dvo->panel = panel;
>  			drm_panel_attach(dvo->panel, connector);
> +		}
>  	}

Same here.

>  
>  	if (dvo->panel)
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index bfbf761f0c1d..ce388d7cebaa 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -812,8 +812,8 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  
>  	dsi->device = device;
>  	dsi->panel = of_drm_find_panel(device->dev.of_node);
> -	if (!dsi->panel)
> -		return -EINVAL;
> +	if (IS_ERR(dsi->panel))
> +		return PTR_ERR(dsi->panel);
>  
>  	dev_info(host->dev, "Attached device %s\n", device->name);
>  
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index 87c5d89bc9ba..0b1eee4b2fb1 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -1409,9 +1409,11 @@ static int tegra_dsi_host_attach(struct mipi_dsi_host *host,
>  	 */
>  	if (!dsi->master) {
>  		struct tegra_output *output = &dsi->output;
> +		struct drm_panel *panel;
>  
> -		output->panel = of_drm_find_panel(device->dev.of_node);
> -		if (output->panel && output->connector.dev) {
> +		panel = of_drm_find_panel(device->dev.of_node);
> +		if (!IS_ERR(panel) && output->connector.dev) {
> +			output->panel = panel;

And here.

>  			drm_panel_attach(output->panel, &output->connector);
>  			drm_helper_hpd_irq_event(output->connector.dev);
>  		}
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index 676fd394836f..21d8737f8b49 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -101,18 +101,22 @@ static irqreturn_t hpd_irq(int irq, void *data)
>  
>  int tegra_output_probe(struct tegra_output *output)
>  {
> -	struct device_node *ddc, *panel;
> +	struct device_node *ddc, *panelnp;
>  	int err, size;
>  
>  	if (!output->of_node)
>  		output->of_node = output->dev->of_node;
>  
> -	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
> -	if (panel) {
> -		output->panel = of_drm_find_panel(panel);
> -		of_node_put(panel);
> -		if (!output->panel)
> -			return -EPROBE_DEFER;
> +	panelnp = of_parse_phandle(output->of_node, "nvidia,panel", 0);
> +	if (panelnp) {
> +		struct drm_panel *panel = of_drm_find_panel(panelnp);
> +
> +		of_node_put(panelnp);
> +
> +		if (IS_ERR(panel))
> +			return PTR_ERR(panel);
> +
> +		output->panel = panel;
>  	}

And here.

>  	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 14ac240a1f64..3bb91519462e 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -199,7 +199,7 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np);
>  #else
>  static inline struct drm_panel *of_drm_find_panel(const struct device_node *np)
>  {
> -	return NULL;
> +	return ERR_PTR(-ENOTSUPP);
>  }
>  #endif

Maybe make this return ERR_PTR(-ENODEV) for consistency with the real
function? If we absolutely do need to differentiate, then you should
probably update the kerneldoc for of_drm_find_panel() to mention the
-ENOTSUPP case.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/6] drm/panel: Make of_drm_find_panel() return an ERR_PTR() instead of NULL

[Boris Brezillon]

Hi Thierry,

On Fri, 4 May 2018 12:18:52 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Thu, May 03, 2018 at 06:40:05PM +0200, Boris Brezillon wrote:
> > Right now, the DRM panel logic returns NULL when a panel pointing to
> > the passed OF node is not present in the list of registered panels.
> > 
> > Most drivers interpret this NULL value as -EPROBE_DEFER, but we are
> > about to modify the semantic of of_drm_find_panel() and let the
> > framework return -ENODEV when the device node we're pointing to has
> > a status property that is not equal to "okay" or "ok".
> > 
> > Let's first patch the of_drm_find_panel() implementation to return
> > ERR_PTR(-EPROBE_DEFER) instead of NULL and patch all callers to replace
> > the '!panel' check by an 'IS_ERR(panel)' one.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/bridge/cdns-dsi.c                   |  2 +-
> >  drivers/gpu/drm/bridge/lvds-encoder.c               |  4 ++--
> >  drivers/gpu/drm/drm_of.c                            |  8 ++++++--
> >  drivers/gpu/drm/drm_panel.c                         |  6 ++++--
> >  drivers/gpu/drm/exynos/exynos_dp.c                  |  9 ++++++---
> >  drivers/gpu/drm/exynos/exynos_drm_dpi.c             |  9 ++++++---
> >  drivers/gpu/drm/exynos/exynos_drm_dsi.c             |  6 ++++--
> >  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c           |  8 +++++---
> >  drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c   |  4 ++--
> >  drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 10 +++++++---
> >  drivers/gpu/drm/msm/dsi/dsi_host.c                  |  2 +-
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c                 |  9 ++++++---
> >  drivers/gpu/drm/rockchip/dw-mipi-dsi.c              |  2 +-
> >  drivers/gpu/drm/sti/sti_dvo.c                       |  7 +++++--
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c              |  4 ++--
> >  drivers/gpu/drm/tegra/dsi.c                         |  6 ++++--
> >  drivers/gpu/drm/tegra/output.c                      | 18 +++++++++++-------
> >  include/drm/drm_panel.h                             |  2 +-
> >  18 files changed, 74 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
> > index c255fc3e1be5..2c5991cf5397 100644
> > --- a/drivers/gpu/drm/bridge/cdns-dsi.c
> > +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> > @@ -1152,7 +1152,7 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
> >  		np = of_node_get(dev->dev.of_node);
> >  
> >  	panel = of_drm_find_panel(np);
> > -	if (panel) {
> > +	if (!IS_ERR(panel)) {
> >  		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DSI);
> >  	} else {
> >  		bridge = of_drm_find_bridge(dev->dev.of_node);
> > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> > index 75b0d3f6e4de..f56c92f7af7c 100644
> > --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> > @@ -68,9 +68,9 @@ static int lvds_encoder_probe(struct platform_device *pdev)
> >  
> >  	panel = of_drm_find_panel(panel_node);
> >  	of_node_put(panel_node);
> > -	if (!panel) {
> > +	if (IS_ERR(panel)) {
> >  		dev_dbg(&pdev->dev, "panel not found, deferring probe\n");
> > -		return -EPROBE_DEFER;
> > +		return PTR_ERR(panel);
> >  	}
> >  
> >  	lvds_encoder->panel_bridge =
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 1fe122461298..f413fae6f6dc 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -239,9 +239,13 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >  		return -ENODEV;
> >  
> >  	if (panel) {
> > -		*panel = of_drm_find_panel(remote);
> > -		if (*panel)
> > +		struct drm_panel *tmp_panel;
> > +
> > +		tmp_panel = of_drm_find_panel(remote);
> > +		if (!IS_ERR(tmp_panel)) {
> > +			*panel = tmp_panel;
> >  			ret = 0;
> > +		}  
> 
> I think the introduction of this temporary variable makes the code hard
> to read and the diff difficult to understand. Why not just stick with
> the original style and make this:
> 
> 		*panel = of_drm_find_panel(remote);
> 		if (IS_ERR(*panel))
> 			*panel = NULL;
> 		else
> 			ret = 0;
> 
> ?

Sure.

> 
> >  	}
> >  
> >  	/* No panel found yet, check for a bridge next. */
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index 308d442a531b..2d9e2684c6c8 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -135,7 +135,9 @@ EXPORT_SYMBOL(drm_panel_detach);
> >   * tree node. If a matching panel is found, return a pointer to it.
> >   *
> >   * Return: A pointer to the panel registered for the specified device tree
> > - * node or NULL if no panel matching the device tree node can be found.
> > + * node or an ERR_PTR() if no panel matching the device tree node can be found.
> > + * The only error that can be reported is -EPROBE_DEFER, meaning that the panel
> > + * device has not been probed yet, and the caller should re-retry later.  
> 
> I think you can drop the extra re- from re-retry.

Yep, will fix that.

> 
> >   */
> >  struct drm_panel *of_drm_find_panel(const struct device_node *np)
> >  {
> > @@ -151,7 +153,7 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
> >  	}
> >  
> >  	mutex_unlock(&panel_lock);
> > -	return NULL;
> > +	return ERR_PTR(-EPROBE_DEFER);
> >  }
> >  EXPORT_SYMBOL(of_drm_find_panel);
> >  #endif
> > diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> > index 86330f396784..962cad0276e5 100644
> > --- a/drivers/gpu/drm/exynos/exynos_dp.c
> > +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> > @@ -231,10 +231,13 @@ static int exynos_dp_probe(struct platform_device *pdev)
> >  	/* This is for the backward compatibility. */
> >  	np = of_parse_phandle(dev->of_node, "panel", 0);
> >  	if (np) {
> > -		dp->plat_data.panel = of_drm_find_panel(np);
> > +		struct drm_panel *panel = of_drm_find_panel(np);
> > +
> >  		of_node_put(np);
> > -		if (!dp->plat_data.panel)
> > -			return -EPROBE_DEFER;
> > +		if (IS_ERR(panel))
> > +			return PTR_ERR(panel);  
> 
> I don't see the point in the extra variable here. dp is allocated using
> devm_kzalloc(), so it will go away on the error return. You shouldn't
> need to worry about keeping the dp->plat_data.panel untouched in case of
> error.

Also true. I'll assign dp->plat_data.panel directly.

> 
> > +
> > +		dp->plat_data.panel = panel;
> >  		goto out;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> > index 66945e0dc57f..e9c7d1c1cf8f 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> > @@ -239,9 +239,12 @@ struct drm_encoder *exynos_dpi_probe(struct device *dev)
> >  	}
> >  
> >  	if (ctx->panel_node) {
> > -		ctx->panel = of_drm_find_panel(ctx->panel_node);
> > -		if (!ctx->panel)
> > -			return ERR_PTR(-EPROBE_DEFER);
> > +		struct drm_panel *panel = of_drm_find_panel(ctx->panel_node);
> > +
> > +		if (IS_ERR(panel))
> > +			return ERR_CAST(panel);
> > +
> > +		ctx->panel = panel;  
> 
> Same here.

Yep.

> 
> >  	}
> >  
> >  	return &ctx->encoder;
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index 7904ffa9abfb..96206deb86a0 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -1520,6 +1520,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
> >  {
> >  	struct exynos_dsi *dsi = host_to_dsi(host);
> >  	struct drm_device *drm = dsi->connector.dev;
> > +	struct drm_panel *panel;
> >  
> >  	/*
> >  	 * This is a temporary solution and should be made by more generic way.
> > @@ -1538,8 +1539,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
> >  	dsi->lanes = device->lanes;
> >  	dsi->format = device->format;
> >  	dsi->mode_flags = device->mode_flags;
> > -	dsi->panel = of_drm_find_panel(device->dev.of_node);
> > -	if (dsi->panel) {
> > +	panel = of_drm_find_panel(device->dev.of_node);
> > +	if (!IS_ERR(panel)) {
> > +		dsi->panel = panel;
> >  		drm_panel_attach(dsi->panel, &dsi->connector);
> >  		dsi->connector.status = connector_status_connected;
> >  	}  
> 
> It seems to be a potential problem here if dsi->panel is an ERR_PTR()-
> encoded value, but I still think this becomes clearer if you do:
> 
> 	dsi->panel = of_drm_find_panel(...);
> 	if (!IS_ERR(dsi->panel)) {
> 		...
> 	} else {
> 		dsi->panel = NULL;
> 	}
> 
> Or maybe even:
> 
> 	dsi->panel = of_drm_find_panel(...);
> 	if (IS_ERR(dsi->panel))
> 		dsi->panel = NULL;
> 
> 	if (dsi->panel) {
> 	}
> 
> That's more explicitly saying that the panel support is optional.

I'll go for the 2nd option.

> 
> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> > index c54806d08dd7..7753b3093472 100644
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> > @@ -146,10 +146,12 @@ int fsl_dcu_create_outputs(struct fsl_dcu_drm_device *fsl_dev)
> >  	/* This is for backward compatibility */
> >  	panel_node = of_parse_phandle(fsl_dev->np, "fsl,panel", 0);
> >  	if (panel_node) {
> > -		fsl_dev->connector.panel = of_drm_find_panel(panel_node);
> > +		panel = of_drm_find_panel(panel_node);
> >  		of_node_put(panel_node);
> > -		if (!fsl_dev->connector.panel)
> > -			return -EPROBE_DEFER;
> > +		if (IS_ERR(panel))
> > +			return PTR_ERR(panel);
> > +
> > +		fsl_dev->connector.panel = panel;  
> 
> Same here, fsl_dev goes away after the error return.

Okay, will assign fsl_dev->connector.panel directly.

> 
> >  		return fsl_dcu_attach_panel(fsl_dev, fsl_dev->connector.panel);
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> > index 4a645926edb7..2bfb39082f54 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> > @@ -341,7 +341,7 @@ static void mdp4_lcdc_encoder_disable(struct drm_encoder *encoder)
> >  	mdp4_write(mdp4_kms, REG_MDP4_LCDC_ENABLE, 0);
> >  
> >  	panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node);
> > -	if (panel) {
> > +	if (!IS_ERR(panel)) {
> >  		drm_panel_disable(panel);
> >  		drm_panel_unprepare(panel);
> >  	}
> > @@ -410,7 +410,7 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder *encoder)
> >  		dev_err(dev->dev, "failed to enable lcdc_clk: %d\n", ret);
> >  
> >  	panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node);
> > -	if (panel) {
> > +	if (!IS_ERR(panel)) {
> >  		drm_panel_prepare(panel);
> >  		drm_panel_enable(panel);
> >  	}
> > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> > index e3b1c86b7aae..c7dd72b428f8 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> > @@ -34,9 +34,13 @@ static enum drm_connector_status mdp4_lvds_connector_detect(
> >  	struct mdp4_lvds_connector *mdp4_lvds_connector =
> >  			to_mdp4_lvds_connector(connector);
> >  
> > -	if (!mdp4_lvds_connector->panel)
> > -		mdp4_lvds_connector->panel =
> > -			of_drm_find_panel(mdp4_lvds_connector->panel_node);
> > +	if (!mdp4_lvds_connector->panel) {
> > +		struct drm_panel *panel;
> > +
> > +		panel = of_drm_find_panel(mdp4_lvds_connector->panel_node);
> > +		if (!IS_ERR(panel))
> > +			mdp4_lvds_connector->panel = panel;
> > +	}  
> 
> Huh... this is hacky to begin with. Seems like this driver doesn't care
> about waiting for the panel, it'll just retry probing the panel
> everytime ->detect() is called on the connector until the panel has
> shown up. That's not really how this is supposed to work, but it's been
> there before your patch, so should be addressed separately.

Yes, I'm not trying to address this kind of issues here, and I fear
it's not the only driver to do that.

> 
> >  
> >  	return mdp4_lvds_connector->panel ?
> >  			connector_status_connected :
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 7a03a9489708..fffc80b73966 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -1881,7 +1881,7 @@ int msm_dsi_host_register(struct mipi_dsi_host *host, bool check_defer)
> >  		 * output
> >  		 */
> >  		if (check_defer && msm_host->device_node) {
> > -			if (!of_drm_find_panel(msm_host->device_node))
> > +			if (IS_ERR(of_drm_find_panel(msm_host->device_node)))
> >  				if (!of_drm_find_bridge(msm_host->device_node))
> >  					return -EPROBE_DEFER;
> >  		}  
> 
> Again, pretty weird stuff going on here, prior to the patch. But I think
> this needs to be changed to take the -ENODEV into account in the next
> patch. As it is, this will continue to defer probe even if the panel
> node is disabled.

Not sure this is a problem. I mean, the code was working before, and we
had no way to differentiate the -ENODEV vs -EPROBE_DEFER, which in turn
means that, any driver that assumed that the device would appear at some
point were just as broken as they are after this patch when the node
they're pointing to has its status set to "disabled".

I'm not trying to patch all drivers to take the return code into
account, just those that might take advantage of it.

> 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 3d2d3bbd1342..d77b8f8bcf94 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -430,9 +430,12 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >  		if (!lvds->next_bridge)
> >  			ret = -EPROBE_DEFER;
> >  	} else {
> > -		lvds->panel = of_drm_find_panel(remote);
> > -		if (!lvds->panel)
> > -			ret = -EPROBE_DEFER;
> > +		struct drm_panel *panel = of_drm_find_panel(remote);
> > +
> > +		if (IS_ERR(panel))
> > +			ret = PTR_ERR(panel);
> > +		else
> > +			lvds->panel = panel;
> >  	}  
> 
> Similar to others above, I think this can easily be done without the
> extra temporary variable.

Sure (same goes for all occurrences).

[...]

> 
> >  	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index 14ac240a1f64..3bb91519462e 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -199,7 +199,7 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np);
> >  #else
> >  static inline struct drm_panel *of_drm_find_panel(const struct device_node *np)
> >  {
> > -	return NULL;
> > +	return ERR_PTR(-ENOTSUPP);
> >  }
> >  #endif  
> 
> Maybe make this return ERR_PTR(-ENODEV) for consistency with the real
> function? If we absolutely do need to differentiate, then you should
> probably update the kerneldoc for of_drm_find_panel() to mention the
> -ENOTSUPP case.

Will use -ENODEV here.

Thanks for the review.

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/6] drm/panel: Make of_drm_find_panel() return an ERR_PTR() instead of NULL

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1746 bytes --]

On Fri, May 04, 2018 at 01:58:20PM +0200, Boris Brezillon wrote:
> On Fri, 4 May 2018 12:18:52 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Thu, May 03, 2018 at 06:40:05PM +0200, Boris Brezillon wrote:
[...]
> > >  	return mdp4_lvds_connector->panel ?
> > >  			connector_status_connected :
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > index 7a03a9489708..fffc80b73966 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > @@ -1881,7 +1881,7 @@ int msm_dsi_host_register(struct mipi_dsi_host *host, bool check_defer)
> > >  		 * output
> > >  		 */
> > >  		if (check_defer && msm_host->device_node) {
> > > -			if (!of_drm_find_panel(msm_host->device_node))
> > > +			if (IS_ERR(of_drm_find_panel(msm_host->device_node)))
> > >  				if (!of_drm_find_bridge(msm_host->device_node))
> > >  					return -EPROBE_DEFER;
> > >  		}  
> > 
> > Again, pretty weird stuff going on here, prior to the patch. But I think
> > this needs to be changed to take the -ENODEV into account in the next
> > patch. As it is, this will continue to defer probe even if the panel
> > node is disabled.
> 
> Not sure this is a problem. I mean, the code was working before, and we
> had no way to differentiate the -ENODEV vs -EPROBE_DEFER, which in turn
> means that, any driver that assumed that the device would appear at some
> point were just as broken as they are after this patch when the node
> they're pointing to has its status set to "disabled".
> 
> I'm not trying to patch all drivers to take the return code into
> account, just those that might take advantage of it.

Okay, fair enough.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/6] drm/panel: Let of_drm_find_panel() return -ENODEV when the panel is disabled

[Boris Brezillon]

DT nodes might be present in the DT but with a status property set to
"disabled" or "fail". In this case, we should not return -EPROBE_DEFER
when the caller ask for a drm_panel instance. Return -ENODEV instead.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/drm_panel.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 2d9e2684c6c8..15df28d20194 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -136,13 +136,18 @@ EXPORT_SYMBOL(drm_panel_detach);
  *
  * Return: A pointer to the panel registered for the specified device tree
  * node or an ERR_PTR() if no panel matching the device tree node can be found.
- * The only error that can be reported is -EPROBE_DEFER, meaning that the panel
- * device has not been probed yet, and the caller should re-retry later.
+ * Possible error codes returned by this function:
+ * - EPROBE_DEFER: the panel device has not been probed yet, and the caller
+ *   should re-retry later
+ * - ENODEV: the device is not available (status != "okay" or "ok")
  */
 struct drm_panel *of_drm_find_panel(const struct device_node *np)
 {
 	struct drm_panel *panel;
 
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
 	mutex_lock(&panel_lock);
 
 	list_for_each_entry(panel, &panel_list, list) {
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/panel: Let of_drm_find_panel() return -ENODEV when the panel is disabled

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 558 bytes --]

On Thu, May 03, 2018 at 06:40:06PM +0200, Boris Brezillon wrote:
> DT nodes might be present in the DT but with a status property set to
> "disabled" or "fail". In this case, we should not return -EPROBE_DEFER
> when the caller ask for a drm_panel instance. Return -ENODEV instead.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/drm_panel.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/6] drm/of: Make drm_of_find_panel_or_bridge() fail when the device is disabled

[Boris Brezillon]

There's no point searching for a drm_bridge or drm_panel if the OF node
we're pointing has a status property that is not "okay" or "ok". Just
return -ENODEV in this case.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/drm_of.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index f413fae6f6dc..f8c1da95c63f 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -238,6 +238,11 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 	if (!remote)
 		return -ENODEV;
 
+	if (!of_device_is_available(remote)) {
+		of_node_put(remote);
+		return -ENODEV;
+	}
+
 	if (panel) {
 		struct drm_panel *tmp_panel;
 
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/of: Make drm_of_find_panel_or_bridge() fail when the device is disabled

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]

On Thu, May 03, 2018 at 06:40:07PM +0200, Boris Brezillon wrote:
> There's no point searching for a drm_bridge or drm_panel if the OF node
> we're pointing has a status property that is not "okay" or "ok". Just
> return -ENODEV in this case.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/drm_of.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/6] drm/vc4: Support the case where the DSI device is disabled

[Boris Brezillon]

Having a device with a status property != "okay" in the DT is a valid
use case, and we should not prevent the registration of the DRM device
when the DSI device connected to the DSI controller is disabled.

Consider the ENODEV return code as a valid result and do not expose the
DSI encoder/connector when it happens.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 8aa897835118..db2f137f8b7b 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 
 	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
 					  &panel, &dsi->bridge);
-	if (ret)
+	if (ret) {
+		/* If the bridge or panel pointed by dev->of_node is not
+		 * enabled, just return 0 here so that we don't prevent the DRM
+		 * dev from being registered. Of course that means the DSI
+		 * encoder won't be exposed, but that's not a problem since
+		 * nothing is connected to it.
+		 */
+		if (ret == -ENODEV)
+			return 0;
+
 		return ret;
+	}
 
 	if (panel) {
 		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
@@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
 	struct vc4_dev *vc4 = to_vc4_dev(drm);
 	struct vc4_dsi *dsi = dev_get_drvdata(dev);
 
-	pm_runtime_disable(dev);
+	if (dsi->bridge)
+		pm_runtime_disable(dev);
 
 	vc4_dsi_encoder_destroy(dsi->encoder);
 
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/vc4: Support the case where the DSI device is disabled

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2095 bytes --]

On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote:
> Having a device with a status property != "okay" in the DT is a valid
> use case, and we should not prevent the registration of the DRM device
> when the DSI device connected to the DSI controller is disabled.
> 
> Consider the ENODEV return code as a valid result and do not expose the
> DSI encoder/connector when it happens.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 8aa897835118..db2f137f8b7b 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>  
>  	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
>  					  &panel, &dsi->bridge);
> -	if (ret)
> +	if (ret) {
> +		/* If the bridge or panel pointed by dev->of_node is not
> +		 * enabled, just return 0 here so that we don't prevent the DRM
> +		 * dev from being registered. Of course that means the DSI
> +		 * encoder won't be exposed, but that's not a problem since
> +		 * nothing is connected to it.
> +		 */
> +		if (ret == -ENODEV)
> +			return 0;
> +
>  		return ret;
> +	}
>  
>  	if (panel) {
>  		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
> @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
>  	struct vc4_dev *vc4 = to_vc4_dev(drm);
>  	struct vc4_dsi *dsi = dev_get_drvdata(dev);
>  
> -	pm_runtime_disable(dev);
> +	if (dsi->bridge)
> +		pm_runtime_disable(dev);

Is this safe? This uses component/master, so dsi->bridge is going to
remain valid until the driver's ->remove() is called. So technically you
could have a situation where drm_of_find_panel_or_bridge() returned some
error code that remains stored in dsi->bridge and cause the above
condition to be incorrectly true.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/vc4: Support the case where the DSI device is disabled

[Boris Brezillon]

On Fri, 4 May 2018 12:28:33 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote:
> > Having a device with a status property != "okay" in the DT is a valid
> > use case, and we should not prevent the registration of the DRM device
> > when the DSI device connected to the DSI controller is disabled.
> > 
> > Consider the ENODEV return code as a valid result and do not expose the
> > DSI encoder/connector when it happens.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > index 8aa897835118..db2f137f8b7b 100644
> > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
> >  
> >  	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> >  					  &panel, &dsi->bridge);
> > -	if (ret)
> > +	if (ret) {
> > +		/* If the bridge or panel pointed by dev->of_node is not
> > +		 * enabled, just return 0 here so that we don't prevent the DRM
> > +		 * dev from being registered. Of course that means the DSI
> > +		 * encoder won't be exposed, but that's not a problem since
> > +		 * nothing is connected to it.
> > +		 */
> > +		if (ret == -ENODEV)
> > +			return 0;
> > +
> >  		return ret;
> > +	}
> >  
> >  	if (panel) {
> >  		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
> > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
> >  	struct vc4_dev *vc4 = to_vc4_dev(drm);
> >  	struct vc4_dsi *dsi = dev_get_drvdata(dev);
> >  
> > -	pm_runtime_disable(dev);
> > +	if (dsi->bridge)
> > +		pm_runtime_disable(dev);  
> 
> Is this safe? This uses component/master, so dsi->bridge is going to
> remain valid until the driver's ->remove() is called. So technically you
> could have a situation where drm_of_find_panel_or_bridge() returned some
> error code that remains stored in dsi->bridge and cause the above
> condition to be incorrectly true.

No, because of_drm_find_bridge() (which is called from
drm_of_find_panel_or_bridge() returns either NULL or a valid bridge
pointer), so dsi->bridge either points to a valid bridge object or is
NULL. Am I missing something?

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/vc4: Support the case where the DSI device is disabled

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2823 bytes --]

On Fri, May 04, 2018 at 02:05:25PM +0200, Boris Brezillon wrote:
> On Fri, 4 May 2018 12:28:33 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote:
> > > Having a device with a status property != "okay" in the DT is a valid
> > > use case, and we should not prevent the registration of the DRM device
> > > when the DSI device connected to the DSI controller is disabled.
> > > 
> > > Consider the ENODEV return code as a valid result and do not expose the
> > > DSI encoder/connector when it happens.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > > index 8aa897835118..db2f137f8b7b 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
> > >  
> > >  	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> > >  					  &panel, &dsi->bridge);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		/* If the bridge or panel pointed by dev->of_node is not
> > > +		 * enabled, just return 0 here so that we don't prevent the DRM
> > > +		 * dev from being registered. Of course that means the DSI
> > > +		 * encoder won't be exposed, but that's not a problem since
> > > +		 * nothing is connected to it.
> > > +		 */
> > > +		if (ret == -ENODEV)
> > > +			return 0;
> > > +
> > >  		return ret;
> > > +	}
> > >  
> > >  	if (panel) {
> > >  		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
> > > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
> > >  	struct vc4_dev *vc4 = to_vc4_dev(drm);
> > >  	struct vc4_dsi *dsi = dev_get_drvdata(dev);
> > >  
> > > -	pm_runtime_disable(dev);
> > > +	if (dsi->bridge)
> > > +		pm_runtime_disable(dev);  
> > 
> > Is this safe? This uses component/master, so dsi->bridge is going to
> > remain valid until the driver's ->remove() is called. So technically you
> > could have a situation where drm_of_find_panel_or_bridge() returned some
> > error code that remains stored in dsi->bridge and cause the above
> > condition to be incorrectly true.
> 
> No, because of_drm_find_bridge() (which is called from
> drm_of_find_panel_or_bridge() returns either NULL or a valid bridge
> pointer), so dsi->bridge either points to a valid bridge object or is
> NULL. Am I missing something?

The return value of devm_drm_panel_bridge_add() is also assigned to
dsi->bridge later on (in the "if (panel)" conditional).

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/vc4: Support the case where the DSI device is disabled

[Boris Brezillon]

On Fri, 4 May 2018 15:29:15 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Fri, May 04, 2018 at 02:05:25PM +0200, Boris Brezillon wrote:
> > On Fri, 4 May 2018 12:28:33 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >   
> > > On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote:  
> > > > Having a device with a status property != "okay" in the DT is a valid
> > > > use case, and we should not prevent the registration of the DRM device
> > > > when the DSI device connected to the DSI controller is disabled.
> > > > 
> > > > Consider the ENODEV return code as a valid result and do not expose the
> > > > DSI encoder/connector when it happens.
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > ---
> > > >  drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++--
> > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > > > index 8aa897835118..db2f137f8b7b 100644
> > > > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > > > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > > > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
> > > >  
> > > >  	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> > > >  					  &panel, &dsi->bridge);
> > > > -	if (ret)
> > > > +	if (ret) {
> > > > +		/* If the bridge or panel pointed by dev->of_node is not
> > > > +		 * enabled, just return 0 here so that we don't prevent the DRM
> > > > +		 * dev from being registered. Of course that means the DSI
> > > > +		 * encoder won't be exposed, but that's not a problem since
> > > > +		 * nothing is connected to it.
> > > > +		 */
> > > > +		if (ret == -ENODEV)
> > > > +			return 0;
> > > > +
> > > >  		return ret;
> > > > +	}
> > > >  
> > > >  	if (panel) {
> > > >  		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
> > > > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
> > > >  	struct vc4_dev *vc4 = to_vc4_dev(drm);
> > > >  	struct vc4_dsi *dsi = dev_get_drvdata(dev);
> > > >  
> > > > -	pm_runtime_disable(dev);
> > > > +	if (dsi->bridge)
> > > > +		pm_runtime_disable(dev);    
> > > 
> > > Is this safe? This uses component/master, so dsi->bridge is going to
> > > remain valid until the driver's ->remove() is called. So technically you
> > > could have a situation where drm_of_find_panel_or_bridge() returned some
> > > error code that remains stored in dsi->bridge and cause the above
> > > condition to be incorrectly true.  
> > 
> > No, because of_drm_find_bridge() (which is called from
> > drm_of_find_panel_or_bridge() returns either NULL or a valid bridge
> > pointer), so dsi->bridge either points to a valid bridge object or is
> > NULL. Am I missing something?  
> 
> The return value of devm_drm_panel_bridge_add() is also assigned to
> dsi->bridge later on (in the "if (panel)" conditional).

But then we return an error code if IS_ERR(dsi->bridge) [1], which
should prevent the unbind function from being called, right?

[1]https://elixir.bootlin.com/linux/v4.17-rc3/source/drivers/gpu/drm/vc4/vc4_dsi.c#L1610
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/vc4: Support the case where the DSI device is disabled

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3424 bytes --]

On Fri, May 04, 2018 at 03:49:06PM +0200, Boris Brezillon wrote:
> On Fri, 4 May 2018 15:29:15 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Fri, May 04, 2018 at 02:05:25PM +0200, Boris Brezillon wrote:
> > > On Fri, 4 May 2018 12:28:33 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >   
> > > > On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote:  
> > > > > Having a device with a status property != "okay" in the DT is a valid
> > > > > use case, and we should not prevent the registration of the DRM device
> > > > > when the DSI device connected to the DSI controller is disabled.
> > > > > 
> > > > > Consider the ENODEV return code as a valid result and do not expose the
> > > > > DSI encoder/connector when it happens.
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > ---
> > > > >  drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++--
> > > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > > > > index 8aa897835118..db2f137f8b7b 100644
> > > > > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > > > > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > > > > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
> > > > >  
> > > > >  	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> > > > >  					  &panel, &dsi->bridge);
> > > > > -	if (ret)
> > > > > +	if (ret) {
> > > > > +		/* If the bridge or panel pointed by dev->of_node is not
> > > > > +		 * enabled, just return 0 here so that we don't prevent the DRM
> > > > > +		 * dev from being registered. Of course that means the DSI
> > > > > +		 * encoder won't be exposed, but that's not a problem since
> > > > > +		 * nothing is connected to it.
> > > > > +		 */
> > > > > +		if (ret == -ENODEV)
> > > > > +			return 0;
> > > > > +
> > > > >  		return ret;
> > > > > +	}
> > > > >  
> > > > >  	if (panel) {
> > > > >  		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
> > > > > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
> > > > >  	struct vc4_dev *vc4 = to_vc4_dev(drm);
> > > > >  	struct vc4_dsi *dsi = dev_get_drvdata(dev);
> > > > >  
> > > > > -	pm_runtime_disable(dev);
> > > > > +	if (dsi->bridge)
> > > > > +		pm_runtime_disable(dev);    
> > > > 
> > > > Is this safe? This uses component/master, so dsi->bridge is going to
> > > > remain valid until the driver's ->remove() is called. So technically you
> > > > could have a situation where drm_of_find_panel_or_bridge() returned some
> > > > error code that remains stored in dsi->bridge and cause the above
> > > > condition to be incorrectly true.  
> > > 
> > > No, because of_drm_find_bridge() (which is called from
> > > drm_of_find_panel_or_bridge() returns either NULL or a valid bridge
> > > pointer), so dsi->bridge either points to a valid bridge object or is
> > > NULL. Am I missing something?  
> > 
> > The return value of devm_drm_panel_bridge_add() is also assigned to
> > dsi->bridge later on (in the "if (panel)" conditional).
> 
> But then we return an error code if IS_ERR(dsi->bridge) [1], which
> should prevent the unbind function from being called, right?

Right, that should work. Seems safe, then.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/vc4: Support the case where the DSI device is disabled

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1406 bytes --]

On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote:
> Having a device with a status property != "okay" in the DT is a valid
> use case, and we should not prevent the registration of the DRM device
> when the DSI device connected to the DSI controller is disabled.
> 
> Consider the ENODEV return code as a valid result and do not expose the
> DSI encoder/connector when it happens.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 8aa897835118..db2f137f8b7b 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>  
>  	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
>  					  &panel, &dsi->bridge);
> -	if (ret)
> +	if (ret) {
> +		/* If the bridge or panel pointed by dev->of_node is not
> +		 * enabled, just return 0 here so that we don't prevent the DRM
> +		 * dev from being registered. Of course that means the DSI
> +		 * encoder won't be exposed, but that's not a problem since
> +		 * nothing is connected to it.
> +		 */

Also, nit: this isn't the correct style for block comments.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/vc4: Support the case where the DSI device is disabled

[Boris Brezillon]

On Fri, 4 May 2018 12:30:25 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote:
> > Having a device with a status property != "okay" in the DT is a valid
> > use case, and we should not prevent the registration of the DRM device
> > when the DSI device connected to the DSI controller is disabled.
> > 
> > Consider the ENODEV return code as a valid result and do not expose the
> > DSI encoder/connector when it happens.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > index 8aa897835118..db2f137f8b7b 100644
> > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
> >  
> >  	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> >  					  &panel, &dsi->bridge);
> > -	if (ret)
> > +	if (ret) {
> > +		/* If the bridge or panel pointed by dev->of_node is not
> > +		 * enabled, just return 0 here so that we don't prevent the DRM
> > +		 * dev from being registered. Of course that means the DSI
> > +		 * encoder won't be exposed, but that's not a problem since
> > +		 * nothing is connected to it.
> > +		 */  
> 
> Also, nit: this isn't the correct style for block comments.

Just trying to keep it consistent with the rest of the file.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Boris Brezillon]

The device might be described in the device tree but not connected to
the I2C bus. Update the status property so that the DRM panel logic
returns -ENODEV when someone tries to get the panel attached to this
DT node.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
index 2c9c9722734f..b8fcb1acef75 100644
--- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
 	.get_modes = rpi_touchscreen_get_modes,
 };
 
+static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
+{
+	struct property *newprop;
+
+	newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
+	if (!newprop)
+		return;
+
+	newprop->name = kstrdup("status", GFP_KERNEL);
+	if (!newprop->name)
+		goto err;
+
+	newprop->value = kstrdup("fail", GFP_KERNEL);
+	if (!newprop->value)
+		goto err;
+
+	newprop->length = sizeof("fail");
+
+	if (of_update_property(i2c->dev.of_node, newprop))
+		goto err;
+
+	/* We intentionally leak the memory we allocate here, because the new
+	 * OF property might live longer than the underlying dev, so no way
+	 * we can use devm_kzalloc() here.
+	 */
+	return;
+
+err:
+	kfree(newprop->value);
+	kfree(newprop->name);
+	kfree(newprop);
+}
+
 static int rpi_touchscreen_probe(struct i2c_client *i2c,
 				 const struct i2c_device_id *id)
 {
@@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
 
 	ver = rpi_touchscreen_i2c_read(ts, REG_ID);
 	if (ver < 0) {
+		rpi_touchscreen_set_status_fail(i2c);
 		dev_err(dev, "Atmel I2C read failed: %d\n", ver);
 		return -ENODEV;
 	}
@@ -391,6 +425,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
 	case 0xc3: /* ver 2 */
 		break;
 	default:
+		rpi_touchscreen_set_status_fail(i2c);
 		dev_err(dev, "Unknown Atmel firmware revision: 0x%02x\n", ver);
 		return -ENODEV;
 	}
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Rob Herring]

On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> The device might be described in the device tree but not connected to
> the I2C bus. Update the status property so that the DRM panel logic
> returns -ENODEV when someone tries to get the panel attached to this
> DT node.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> index 2c9c9722734f..b8fcb1acef75 100644
> --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
>         .get_modes = rpi_touchscreen_get_modes,
>  };
>
> +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> +{
> +       struct property *newprop;
> +
> +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> +       if (!newprop)
> +               return;
> +
> +       newprop->name = kstrdup("status", GFP_KERNEL);
> +       if (!newprop->name)
> +               goto err;
> +
> +       newprop->value = kstrdup("fail", GFP_KERNEL);
> +       if (!newprop->value)
> +               goto err;
> +
> +       newprop->length = sizeof("fail");
> +
> +       if (of_update_property(i2c->dev.of_node, newprop))
> +               goto err;
> +

As I mentioned on irc, can you make this a common DT function.

I'm not sure if it matters that we set status to fail vs. disabled. I
somewhat prefer the latter as we already have other cases and I'd
rather the api not pass a string in. I can't think of any reason to
distinguish the difference between fail and disabled.

> +       /* We intentionally leak the memory we allocate here, because the new
> +        * OF property might live longer than the underlying dev, so no way
> +        * we can use devm_kzalloc() here.
> +        */
> +       return;
> +
> +err:
> +       kfree(newprop->value);
> +       kfree(newprop->name);
> +       kfree(newprop);
> +}
> +
>  static int rpi_touchscreen_probe(struct i2c_client *i2c,
>                                  const struct i2c_device_id *id)
>  {
> @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
>
>         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
>         if (ver < 0) {
> +               rpi_touchscreen_set_status_fail(i2c);

I've thought some more about this and I still think this should be
handled in the driver core or i2c core.

The reason is simple. I think the state of the system should be the
same after this as if you booted with 'status = "disabled"' for this
node. And that means the device should be removed completely because
we don't create struct device's for disabled nodes.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Boris Brezillon]

Hi Rob,

On Thu, 3 May 2018 12:12:39 -0500
Rob Herring <robh+dt@kernel.org> wrote:

> On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > The device might be described in the device tree but not connected to
> > the I2C bus. Update the status property so that the DRM panel logic
> > returns -ENODEV when someone tries to get the panel attached to this
> > DT node.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > index 2c9c9722734f..b8fcb1acef75 100644
> > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
> >         .get_modes = rpi_touchscreen_get_modes,
> >  };
> >
> > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> > +{
> > +       struct property *newprop;
> > +
> > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > +       if (!newprop)
> > +               return;
> > +
> > +       newprop->name = kstrdup("status", GFP_KERNEL);
> > +       if (!newprop->name)
> > +               goto err;
> > +
> > +       newprop->value = kstrdup("fail", GFP_KERNEL);
> > +       if (!newprop->value)
> > +               goto err;
> > +
> > +       newprop->length = sizeof("fail");
> > +
> > +       if (of_update_property(i2c->dev.of_node, newprop))
> > +               goto err;
> > +  
> 
> As I mentioned on irc, can you make this a common DT function.

Yep, will move that to drivers/of/base.c and make it generic.

> 
> I'm not sure if it matters that we set status to fail vs. disabled. I
> somewhat prefer the latter as we already have other cases and I'd
> rather the api not pass a string in. I can't think of any reason to
> distinguish the difference between fail and disabled.

Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
and "fail" seemed like a good match for what we are trying to express
here: "we failed to communicate with the device in the probe function
and want to mark it unusable", which is a bit different from "the
device was explicitly disabled by the user".

Anyway, if you think "disabled" is more appropriate, I'll use that.

> 
> > +       /* We intentionally leak the memory we allocate here, because the new
> > +        * OF property might live longer than the underlying dev, so no way
> > +        * we can use devm_kzalloc() here.
> > +        */
> > +       return;
> > +
> > +err:
> > +       kfree(newprop->value);
> > +       kfree(newprop->name);
> > +       kfree(newprop);
> > +}
> > +
> >  static int rpi_touchscreen_probe(struct i2c_client *i2c,
> >                                  const struct i2c_device_id *id)
> >  {
> > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
> >
> >         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> >         if (ver < 0) {
> > +               rpi_touchscreen_set_status_fail(i2c);  
> 
> I've thought some more about this and I still think this should be
> handled in the driver core or i2c core.
> 
> The reason is simple. I think the state of the system should be the
> same after this as if you booted with 'status = "disabled"' for this
> node. And that means the device should be removed completely because
> we don't create struct device's for disabled nodes.

That was my feeling to when first discussing the issue with Daniel and
Thierry on IRC, but after digging a bit in the code I'm no longer sure
this is a good idea. At least, I don't think basing the decision to
disable the device (or mark it unusable) based on the return value of
the probe function is a good idea. What I can do is:

1/ provide a function to change the status prop in of.h
2/ let each driver call this function if they want to
3/ let the I2C core test the status prop again after the probe function
   has returned an error to determine whether the device (I mean struct
   i2c_client/device object) should be removed

Would that work for you?

Regards,

Boris

[1]https://elinux.org/images/c/cf/Power_ePAPR_APPROVED_v1.1.pdf
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4728 bytes --]

On Fri, May 04, 2018 at 10:06:53AM +0200, Boris Brezillon wrote:
> Hi Rob,
> 
> On Thu, 3 May 2018 12:12:39 -0500
> Rob Herring <robh+dt@kernel.org> wrote:
> 
> > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > > The device might be described in the device tree but not connected to
> > > the I2C bus. Update the status property so that the DRM panel logic
> > > returns -ENODEV when someone tries to get the panel attached to this
> > > DT node.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > ---
> > >  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > index 2c9c9722734f..b8fcb1acef75 100644
> > > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
> > >         .get_modes = rpi_touchscreen_get_modes,
> > >  };
> > >
> > > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> > > +{
> > > +       struct property *newprop;
> > > +
> > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > > +       if (!newprop)
> > > +               return;
> > > +
> > > +       newprop->name = kstrdup("status", GFP_KERNEL);
> > > +       if (!newprop->name)
> > > +               goto err;
> > > +
> > > +       newprop->value = kstrdup("fail", GFP_KERNEL);
> > > +       if (!newprop->value)
> > > +               goto err;
> > > +
> > > +       newprop->length = sizeof("fail");
> > > +
> > > +       if (of_update_property(i2c->dev.of_node, newprop))
> > > +               goto err;
> > > +  
> > 
> > As I mentioned on irc, can you make this a common DT function.
> 
> Yep, will move that to drivers/of/base.c and make it generic.
> 
> > 
> > I'm not sure if it matters that we set status to fail vs. disabled. I
> > somewhat prefer the latter as we already have other cases and I'd
> > rather the api not pass a string in. I can't think of any reason to
> > distinguish the difference between fail and disabled.
> 
> Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
> and "fail" seemed like a good match for what we are trying to express
> here: "we failed to communicate with the device in the probe function
> and want to mark it unusable", which is a bit different from "the
> device was explicitly disabled by the user".
> 
> Anyway, if you think "disabled" is more appropriate, I'll use that.
> 
> > 
> > > +       /* We intentionally leak the memory we allocate here, because the new
> > > +        * OF property might live longer than the underlying dev, so no way
> > > +        * we can use devm_kzalloc() here.
> > > +        */
> > > +       return;
> > > +
> > > +err:
> > > +       kfree(newprop->value);
> > > +       kfree(newprop->name);
> > > +       kfree(newprop);
> > > +}
> > > +
> > >  static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > >                                  const struct i2c_device_id *id)
> > >  {
> > > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > >
> > >         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> > >         if (ver < 0) {
> > > +               rpi_touchscreen_set_status_fail(i2c);  
> > 
> > I've thought some more about this and I still think this should be
> > handled in the driver core or i2c core.
> > 
> > The reason is simple. I think the state of the system should be the
> > same after this as if you booted with 'status = "disabled"' for this
> > node. And that means the device should be removed completely because
> > we don't create struct device's for disabled nodes.
> 
> That was my feeling to when first discussing the issue with Daniel and
> Thierry on IRC, but after digging a bit in the code I'm no longer sure
> this is a good idea. At least, I don't think basing the decision to
> disable the device (or mark it unusable) based on the return value of
> the probe function is a good idea.

I'm not so sure about that. -ENODEV seems like a very suitable error
code to base that decision on. A random sampling of a handful of drivers
confirms that this is primarily used to report situations where it is
impossible for the device to ever be probed successfully, so might as
well just remove it.

At the very least I think it is worth proposing the patch and let Greg
and others weigh in.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Boris Brezillon]

On Fri, 4 May 2018 11:47:48 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Fri, May 04, 2018 at 10:06:53AM +0200, Boris Brezillon wrote:
> > Hi Rob,
> > 
> > On Thu, 3 May 2018 12:12:39 -0500
> > Rob Herring <robh+dt@kernel.org> wrote:
> >   
> > > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> > > <boris.brezillon@bootlin.com> wrote:  
> > > > The device might be described in the device tree but not connected to
> > > > the I2C bus. Update the status property so that the DRM panel logic
> > > > returns -ENODEV when someone tries to get the panel attached to this
> > > > DT node.
> > > >
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > ---
> > > >  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
> > > >  1 file changed, 35 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > index 2c9c9722734f..b8fcb1acef75 100644
> > > > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
> > > >         .get_modes = rpi_touchscreen_get_modes,
> > > >  };
> > > >
> > > > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> > > > +{
> > > > +       struct property *newprop;
> > > > +
> > > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > > > +       if (!newprop)
> > > > +               return;
> > > > +
> > > > +       newprop->name = kstrdup("status", GFP_KERNEL);
> > > > +       if (!newprop->name)
> > > > +               goto err;
> > > > +
> > > > +       newprop->value = kstrdup("fail", GFP_KERNEL);
> > > > +       if (!newprop->value)
> > > > +               goto err;
> > > > +
> > > > +       newprop->length = sizeof("fail");
> > > > +
> > > > +       if (of_update_property(i2c->dev.of_node, newprop))
> > > > +               goto err;
> > > > +    
> > > 
> > > As I mentioned on irc, can you make this a common DT function.  
> > 
> > Yep, will move that to drivers/of/base.c and make it generic.
> >   
> > > 
> > > I'm not sure if it matters that we set status to fail vs. disabled. I
> > > somewhat prefer the latter as we already have other cases and I'd
> > > rather the api not pass a string in. I can't think of any reason to
> > > distinguish the difference between fail and disabled.  
> > 
> > Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
> > and "fail" seemed like a good match for what we are trying to express
> > here: "we failed to communicate with the device in the probe function
> > and want to mark it unusable", which is a bit different from "the
> > device was explicitly disabled by the user".
> > 
> > Anyway, if you think "disabled" is more appropriate, I'll use that.
> >   
> > >   
> > > > +       /* We intentionally leak the memory we allocate here, because the new
> > > > +        * OF property might live longer than the underlying dev, so no way
> > > > +        * we can use devm_kzalloc() here.
> > > > +        */
> > > > +       return;
> > > > +
> > > > +err:
> > > > +       kfree(newprop->value);
> > > > +       kfree(newprop->name);
> > > > +       kfree(newprop);
> > > > +}
> > > > +
> > > >  static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > >                                  const struct i2c_device_id *id)
> > > >  {
> > > > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > >
> > > >         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> > > >         if (ver < 0) {
> > > > +               rpi_touchscreen_set_status_fail(i2c);    
> > > 
> > > I've thought some more about this and I still think this should be
> > > handled in the driver core or i2c core.
> > > 
> > > The reason is simple. I think the state of the system should be the
> > > same after this as if you booted with 'status = "disabled"' for this
> > > node. And that means the device should be removed completely because
> > > we don't create struct device's for disabled nodes.  
> > 
> > That was my feeling to when first discussing the issue with Daniel and
> > Thierry on IRC, but after digging a bit in the code I'm no longer sure
> > this is a good idea. At least, I don't think basing the decision to
> > disable the device (or mark it unusable) based on the return value of
> > the probe function is a good idea.  
> 
> I'm not so sure about that. -ENODEV seems like a very suitable error
> code to base that decision on. A random sampling of a handful of drivers
> confirms that this is primarily used to report situations where it is
> impossible for the device to ever be probed successfully, so might as
> well just remove it.

It's not that easy. It has to be done from the I2C core since it's the
only one who can call device_unregister() and cleanup the other bits
associated with an I2C device (see i2c_unregister_device()). Now, the
i2c_driver->probe() function is called from a context where I'm almost
sure device_unregister() can't be called since we might still be in the
device_register() path. The solution would be to queue the
unregistration work to a workqueue, but I'm not even sure this is safe
to do that. What if the I2C adapter exposing the device is removed in
the meantime? Of course, all of this can be addressed, I'm just
wondering if it's really worth the trouble (we're likely to introduce
new races or other kind of bugs while doing that), especially since
placing the device in a "fail" state and still keeping it around would
solve the problem without requiring all the extra cleanup we're talking
about here.

> 
> At the very least I think it is worth proposing the patch and let Greg
> and others weigh in.

Well, if it was an easy thing to do, I guess I would have gone for this
approach from the beginning, but I fear doing that will be much more
complicated than we think it is (maybe I'm wrong).
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Daniel Vetter]

On Fri, May 04, 2018 at 02:17:49PM +0200, Boris Brezillon wrote:
> On Fri, 4 May 2018 11:47:48 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Fri, May 04, 2018 at 10:06:53AM +0200, Boris Brezillon wrote:
> > > Hi Rob,
> > > 
> > > On Thu, 3 May 2018 12:12:39 -0500
> > > Rob Herring <robh+dt@kernel.org> wrote:
> > >   
> > > > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> > > > <boris.brezillon@bootlin.com> wrote:  
> > > > > The device might be described in the device tree but not connected to
> > > > > the I2C bus. Update the status property so that the DRM panel logic
> > > > > returns -ENODEV when someone tries to get the panel attached to this
> > > > > DT node.
> > > > >
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > ---
> > > > >  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
> > > > >  1 file changed, 35 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > > index 2c9c9722734f..b8fcb1acef75 100644
> > > > > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
> > > > >         .get_modes = rpi_touchscreen_get_modes,
> > > > >  };
> > > > >
> > > > > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> > > > > +{
> > > > > +       struct property *newprop;
> > > > > +
> > > > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > > > > +       if (!newprop)
> > > > > +               return;
> > > > > +
> > > > > +       newprop->name = kstrdup("status", GFP_KERNEL);
> > > > > +       if (!newprop->name)
> > > > > +               goto err;
> > > > > +
> > > > > +       newprop->value = kstrdup("fail", GFP_KERNEL);
> > > > > +       if (!newprop->value)
> > > > > +               goto err;
> > > > > +
> > > > > +       newprop->length = sizeof("fail");
> > > > > +
> > > > > +       if (of_update_property(i2c->dev.of_node, newprop))
> > > > > +               goto err;
> > > > > +    
> > > > 
> > > > As I mentioned on irc, can you make this a common DT function.  
> > > 
> > > Yep, will move that to drivers/of/base.c and make it generic.
> > >   
> > > > 
> > > > I'm not sure if it matters that we set status to fail vs. disabled. I
> > > > somewhat prefer the latter as we already have other cases and I'd
> > > > rather the api not pass a string in. I can't think of any reason to
> > > > distinguish the difference between fail and disabled.  
> > > 
> > > Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
> > > and "fail" seemed like a good match for what we are trying to express
> > > here: "we failed to communicate with the device in the probe function
> > > and want to mark it unusable", which is a bit different from "the
> > > device was explicitly disabled by the user".
> > > 
> > > Anyway, if you think "disabled" is more appropriate, I'll use that.
> > >   
> > > >   
> > > > > +       /* We intentionally leak the memory we allocate here, because the new
> > > > > +        * OF property might live longer than the underlying dev, so no way
> > > > > +        * we can use devm_kzalloc() here.
> > > > > +        */
> > > > > +       return;
> > > > > +
> > > > > +err:
> > > > > +       kfree(newprop->value);
> > > > > +       kfree(newprop->name);
> > > > > +       kfree(newprop);
> > > > > +}
> > > > > +
> > > > >  static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > > >                                  const struct i2c_device_id *id)
> > > > >  {
> > > > > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > > >
> > > > >         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> > > > >         if (ver < 0) {
> > > > > +               rpi_touchscreen_set_status_fail(i2c);    
> > > > 
> > > > I've thought some more about this and I still think this should be
> > > > handled in the driver core or i2c core.
> > > > 
> > > > The reason is simple. I think the state of the system should be the
> > > > same after this as if you booted with 'status = "disabled"' for this
> > > > node. And that means the device should be removed completely because
> > > > we don't create struct device's for disabled nodes.  
> > > 
> > > That was my feeling to when first discussing the issue with Daniel and
> > > Thierry on IRC, but after digging a bit in the code I'm no longer sure
> > > this is a good idea. At least, I don't think basing the decision to
> > > disable the device (or mark it unusable) based on the return value of
> > > the probe function is a good idea.  
> > 
> > I'm not so sure about that. -ENODEV seems like a very suitable error
> > code to base that decision on. A random sampling of a handful of drivers
> > confirms that this is primarily used to report situations where it is
> > impossible for the device to ever be probed successfully, so might as
> > well just remove it.
> 
> It's not that easy. It has to be done from the I2C core since it's the
> only one who can call device_unregister() and cleanup the other bits
> associated with an I2C device (see i2c_unregister_device()). Now, the
> i2c_driver->probe() function is called from a context where I'm almost
> sure device_unregister() can't be called since we might still be in the
> device_register() path. The solution would be to queue the
> unregistration work to a workqueue, but I'm not even sure this is safe
> to do that. What if the I2C adapter exposing the device is removed in
> the meantime? Of course, all of this can be addressed, I'm just
> wondering if it's really worth the trouble (we're likely to introduce
> new races or other kind of bugs while doing that), especially since
> placing the device in a "fail" state and still keeping it around would
> solve the problem without requiring all the extra cleanup we're talking
> about here.

I think you have to put the device status into "fail" immediately,
otherwise there's a race with deferred probing. Scenario:

1. vc4 loads, panel isn't there yet -> EPROBE_DEFER.
2. rpi driver loads, notices panel isn't there, returns -ENODEV
3. i2c core fires off the worker and finishes it's ->probe callback.
4. device core starts a reprobe trigger
5. vc4 gets loaded, does the of_device_is_available check, but since
that's not yet update it doesn't get the ENODEV, but still EPROBE_DEFER.
6. i2c worker disables the device and unregisters it.

-> vc4 fails to load since nothing triggers another reprobe anymore.

At least afaics device removal does not trigger a reprobe.
-Daniel

> 
> > 
> > At the very least I think it is worth proposing the patch and let Greg
> > and others weigh in.
> 
> Well, if it was an easy thing to do, I guess I would have gone for this
> approach from the beginning, but I fear doing that will be much more
> complicated than we think it is (maybe I'm wrong).

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Boris Brezillon]

On Fri, 4 May 2018 16:20:17 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, May 04, 2018 at 02:17:49PM +0200, Boris Brezillon wrote:
> > On Fri, 4 May 2018 11:47:48 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >   
> > > On Fri, May 04, 2018 at 10:06:53AM +0200, Boris Brezillon wrote:  
> > > > Hi Rob,
> > > > 
> > > > On Thu, 3 May 2018 12:12:39 -0500
> > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > >     
> > > > > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> > > > > <boris.brezillon@bootlin.com> wrote:    
> > > > > > The device might be described in the device tree but not connected to
> > > > > > the I2C bus. Update the status property so that the DRM panel logic
> > > > > > returns -ENODEV when someone tries to get the panel attached to this
> > > > > > DT node.
> > > > > >
> > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > > ---
> > > > > >  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
> > > > > >  1 file changed, 35 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > > > index 2c9c9722734f..b8fcb1acef75 100644
> > > > > > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > > > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > > > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
> > > > > >         .get_modes = rpi_touchscreen_get_modes,
> > > > > >  };
> > > > > >
> > > > > > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> > > > > > +{
> > > > > > +       struct property *newprop;
> > > > > > +
> > > > > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > > > > > +       if (!newprop)
> > > > > > +               return;
> > > > > > +
> > > > > > +       newprop->name = kstrdup("status", GFP_KERNEL);
> > > > > > +       if (!newprop->name)
> > > > > > +               goto err;
> > > > > > +
> > > > > > +       newprop->value = kstrdup("fail", GFP_KERNEL);
> > > > > > +       if (!newprop->value)
> > > > > > +               goto err;
> > > > > > +
> > > > > > +       newprop->length = sizeof("fail");
> > > > > > +
> > > > > > +       if (of_update_property(i2c->dev.of_node, newprop))
> > > > > > +               goto err;
> > > > > > +      
> > > > > 
> > > > > As I mentioned on irc, can you make this a common DT function.    
> > > > 
> > > > Yep, will move that to drivers/of/base.c and make it generic.
> > > >     
> > > > > 
> > > > > I'm not sure if it matters that we set status to fail vs. disabled. I
> > > > > somewhat prefer the latter as we already have other cases and I'd
> > > > > rather the api not pass a string in. I can't think of any reason to
> > > > > distinguish the difference between fail and disabled.    
> > > > 
> > > > Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
> > > > and "fail" seemed like a good match for what we are trying to express
> > > > here: "we failed to communicate with the device in the probe function
> > > > and want to mark it unusable", which is a bit different from "the
> > > > device was explicitly disabled by the user".
> > > > 
> > > > Anyway, if you think "disabled" is more appropriate, I'll use that.
> > > >     
> > > > >     
> > > > > > +       /* We intentionally leak the memory we allocate here, because the new
> > > > > > +        * OF property might live longer than the underlying dev, so no way
> > > > > > +        * we can use devm_kzalloc() here.
> > > > > > +        */
> > > > > > +       return;
> > > > > > +
> > > > > > +err:
> > > > > > +       kfree(newprop->value);
> > > > > > +       kfree(newprop->name);
> > > > > > +       kfree(newprop);
> > > > > > +}
> > > > > > +
> > > > > >  static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > > > >                                  const struct i2c_device_id *id)
> > > > > >  {
> > > > > > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > > > >
> > > > > >         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> > > > > >         if (ver < 0) {
> > > > > > +               rpi_touchscreen_set_status_fail(i2c);      
> > > > > 
> > > > > I've thought some more about this and I still think this should be
> > > > > handled in the driver core or i2c core.
> > > > > 
> > > > > The reason is simple. I think the state of the system should be the
> > > > > same after this as if you booted with 'status = "disabled"' for this
> > > > > node. And that means the device should be removed completely because
> > > > > we don't create struct device's for disabled nodes.    
> > > > 
> > > > That was my feeling to when first discussing the issue with Daniel and
> > > > Thierry on IRC, but after digging a bit in the code I'm no longer sure
> > > > this is a good idea. At least, I don't think basing the decision to
> > > > disable the device (or mark it unusable) based on the return value of
> > > > the probe function is a good idea.    
> > > 
> > > I'm not so sure about that. -ENODEV seems like a very suitable error
> > > code to base that decision on. A random sampling of a handful of drivers
> > > confirms that this is primarily used to report situations where it is
> > > impossible for the device to ever be probed successfully, so might as
> > > well just remove it.  
> > 
> > It's not that easy. It has to be done from the I2C core since it's the
> > only one who can call device_unregister() and cleanup the other bits
> > associated with an I2C device (see i2c_unregister_device()). Now, the
> > i2c_driver->probe() function is called from a context where I'm almost
> > sure device_unregister() can't be called since we might still be in the
> > device_register() path. The solution would be to queue the
> > unregistration work to a workqueue, but I'm not even sure this is safe
> > to do that. What if the I2C adapter exposing the device is removed in
> > the meantime? Of course, all of this can be addressed, I'm just
> > wondering if it's really worth the trouble (we're likely to introduce
> > new races or other kind of bugs while doing that), especially since
> > placing the device in a "fail" state and still keeping it around would
> > solve the problem without requiring all the extra cleanup we're talking
> > about here.  
> 
> I think you have to put the device status into "fail" immediately,
> otherwise there's a race with deferred probing. Scenario:
> 
> 1. vc4 loads, panel isn't there yet -> EPROBE_DEFER.
> 2. rpi driver loads, notices panel isn't there, returns -ENODEV
> 3. i2c core fires off the worker and finishes it's ->probe callback.
> 4. device core starts a reprobe trigger
> 5. vc4 gets loaded, does the of_device_is_available check, but since
> that's not yet update it doesn't get the ENODEV, but still EPROBE_DEFER.
> 6. i2c worker disables the device and unregisters it.
> 
> -> vc4 fails to load since nothing triggers another reprobe anymore.  
> 
> At least afaics device removal does not trigger a reprobe.

Yep, you're correct. See, one more reason to keep the logic simple and
let each driver change the status prop in their ->probe() function.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Boris Brezillon]

On Fri, 4 May 2018 16:24:04 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Fri, 4 May 2018 16:20:17 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, May 04, 2018 at 02:17:49PM +0200, Boris Brezillon wrote:  
> > > On Fri, 4 May 2018 11:47:48 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >     
> > > > On Fri, May 04, 2018 at 10:06:53AM +0200, Boris Brezillon wrote:    
> > > > > Hi Rob,
> > > > > 
> > > > > On Thu, 3 May 2018 12:12:39 -0500
> > > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > > >       
> > > > > > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> > > > > > <boris.brezillon@bootlin.com> wrote:      
> > > > > > > The device might be described in the device tree but not connected to
> > > > > > > the I2C bus. Update the status property so that the DRM panel logic
> > > > > > > returns -ENODEV when someone tries to get the panel attached to this
> > > > > > > DT node.
> > > > > > >
> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > > > ---
> > > > > > >  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
> > > > > > >  1 file changed, 35 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > > > > index 2c9c9722734f..b8fcb1acef75 100644
> > > > > > > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > > > > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > > > > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
> > > > > > >         .get_modes = rpi_touchscreen_get_modes,
> > > > > > >  };
> > > > > > >
> > > > > > > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> > > > > > > +{
> > > > > > > +       struct property *newprop;
> > > > > > > +
> > > > > > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > > > > > > +       if (!newprop)
> > > > > > > +               return;
> > > > > > > +
> > > > > > > +       newprop->name = kstrdup("status", GFP_KERNEL);
> > > > > > > +       if (!newprop->name)
> > > > > > > +               goto err;
> > > > > > > +
> > > > > > > +       newprop->value = kstrdup("fail", GFP_KERNEL);
> > > > > > > +       if (!newprop->value)
> > > > > > > +               goto err;
> > > > > > > +
> > > > > > > +       newprop->length = sizeof("fail");
> > > > > > > +
> > > > > > > +       if (of_update_property(i2c->dev.of_node, newprop))
> > > > > > > +               goto err;
> > > > > > > +        
> > > > > > 
> > > > > > As I mentioned on irc, can you make this a common DT function.      
> > > > > 
> > > > > Yep, will move that to drivers/of/base.c and make it generic.
> > > > >       
> > > > > > 
> > > > > > I'm not sure if it matters that we set status to fail vs. disabled. I
> > > > > > somewhat prefer the latter as we already have other cases and I'd
> > > > > > rather the api not pass a string in. I can't think of any reason to
> > > > > > distinguish the difference between fail and disabled.      
> > > > > 
> > > > > Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
> > > > > and "fail" seemed like a good match for what we are trying to express
> > > > > here: "we failed to communicate with the device in the probe function
> > > > > and want to mark it unusable", which is a bit different from "the
> > > > > device was explicitly disabled by the user".
> > > > > 
> > > > > Anyway, if you think "disabled" is more appropriate, I'll use that.
> > > > >       
> > > > > >       
> > > > > > > +       /* We intentionally leak the memory we allocate here, because the new
> > > > > > > +        * OF property might live longer than the underlying dev, so no way
> > > > > > > +        * we can use devm_kzalloc() here.
> > > > > > > +        */
> > > > > > > +       return;
> > > > > > > +
> > > > > > > +err:
> > > > > > > +       kfree(newprop->value);
> > > > > > > +       kfree(newprop->name);
> > > > > > > +       kfree(newprop);
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > > > > >                                  const struct i2c_device_id *id)
> > > > > > >  {
> > > > > > > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > > > > >
> > > > > > >         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> > > > > > >         if (ver < 0) {
> > > > > > > +               rpi_touchscreen_set_status_fail(i2c);        
> > > > > > 
> > > > > > I've thought some more about this and I still think this should be
> > > > > > handled in the driver core or i2c core.
> > > > > > 
> > > > > > The reason is simple. I think the state of the system should be the
> > > > > > same after this as if you booted with 'status = "disabled"' for this
> > > > > > node. And that means the device should be removed completely because
> > > > > > we don't create struct device's for disabled nodes.      
> > > > > 
> > > > > That was my feeling to when first discussing the issue with Daniel and
> > > > > Thierry on IRC, but after digging a bit in the code I'm no longer sure
> > > > > this is a good idea. At least, I don't think basing the decision to
> > > > > disable the device (or mark it unusable) based on the return value of
> > > > > the probe function is a good idea.      
> > > > 
> > > > I'm not so sure about that. -ENODEV seems like a very suitable error
> > > > code to base that decision on. A random sampling of a handful of drivers
> > > > confirms that this is primarily used to report situations where it is
> > > > impossible for the device to ever be probed successfully, so might as
> > > > well just remove it.    
> > > 
> > > It's not that easy. It has to be done from the I2C core since it's the
> > > only one who can call device_unregister() and cleanup the other bits
> > > associated with an I2C device (see i2c_unregister_device()). Now, the
> > > i2c_driver->probe() function is called from a context where I'm almost
> > > sure device_unregister() can't be called since we might still be in the
> > > device_register() path. The solution would be to queue the
> > > unregistration work to a workqueue, but I'm not even sure this is safe
> > > to do that. What if the I2C adapter exposing the device is removed in
> > > the meantime? Of course, all of this can be addressed, I'm just
> > > wondering if it's really worth the trouble (we're likely to introduce
> > > new races or other kind of bugs while doing that), especially since
> > > placing the device in a "fail" state and still keeping it around would
> > > solve the problem without requiring all the extra cleanup we're talking
> > > about here.    
> > 
> > I think you have to put the device status into "fail" immediately,
> > otherwise there's a race with deferred probing. Scenario:
> > 
> > 1. vc4 loads, panel isn't there yet -> EPROBE_DEFER.
> > 2. rpi driver loads, notices panel isn't there, returns -ENODEV
> > 3. i2c core fires off the worker and finishes it's ->probe callback.
> > 4. device core starts a reprobe trigger
> > 5. vc4 gets loaded, does the of_device_is_available check, but since
> > that's not yet update it doesn't get the ENODEV, but still EPROBE_DEFER.
> > 6. i2c worker disables the device and unregisters it.
> >   
> > -> vc4 fails to load since nothing triggers another reprobe anymore.    
> > 
> > At least afaics device removal does not trigger a reprobe.  
> 
> Yep, you're correct. See, one more reason to keep the logic simple and
> let each driver change the status prop in their ->probe() function.

Hm, actually it does not work even with my solution because the only
thing that forces a new attempt on all deferred-probe devices is when a
new device is bound to a driver, which will not happen if the rpi-panel
->probe() function returns -ENODEV.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Rob Herring]

On Fri, May 4, 2018 at 9:20 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, May 04, 2018 at 02:17:49PM +0200, Boris Brezillon wrote:
>> On Fri, 4 May 2018 11:47:48 +0200
>> Thierry Reding <thierry.reding@gmail.com> wrote:
>>
>> > On Fri, May 04, 2018 at 10:06:53AM +0200, Boris Brezillon wrote:
>> > > Hi Rob,
>> > >
>> > > On Thu, 3 May 2018 12:12:39 -0500
>> > > Rob Herring <robh+dt@kernel.org> wrote:
>> > >
>> > > > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
>> > > > <boris.brezillon@bootlin.com> wrote:
>> > > > > The device might be described in the device tree but not connected to
>> > > > > the I2C bus. Update the status property so that the DRM panel logic
>> > > > > returns -ENODEV when someone tries to get the panel attached to this
>> > > > > DT node.
>> > > > >
>> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>> > > > > ---
>> > > > >  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
>> > > > >  1 file changed, 35 insertions(+)
>> > > > >
>> > > > > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
>> > > > > index 2c9c9722734f..b8fcb1acef75 100644
>> > > > > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
>> > > > > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
>> > > > > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
>> > > > >         .get_modes = rpi_touchscreen_get_modes,
>> > > > >  };
>> > > > >
>> > > > > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
>> > > > > +{
>> > > > > +       struct property *newprop;
>> > > > > +
>> > > > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
>> > > > > +       if (!newprop)
>> > > > > +               return;
>> > > > > +
>> > > > > +       newprop->name = kstrdup("status", GFP_KERNEL);
>> > > > > +       if (!newprop->name)
>> > > > > +               goto err;
>> > > > > +
>> > > > > +       newprop->value = kstrdup("fail", GFP_KERNEL);
>> > > > > +       if (!newprop->value)
>> > > > > +               goto err;
>> > > > > +
>> > > > > +       newprop->length = sizeof("fail");
>> > > > > +
>> > > > > +       if (of_update_property(i2c->dev.of_node, newprop))
>> > > > > +               goto err;
>> > > > > +
>> > > >
>> > > > As I mentioned on irc, can you make this a common DT function.
>> > >
>> > > Yep, will move that to drivers/of/base.c and make it generic.
>> > >
>> > > >
>> > > > I'm not sure if it matters that we set status to fail vs. disabled. I
>> > > > somewhat prefer the latter as we already have other cases and I'd
>> > > > rather the api not pass a string in. I can't think of any reason to
>> > > > distinguish the difference between fail and disabled.
>> > >
>> > > Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
>> > > and "fail" seemed like a good match for what we are trying to express
>> > > here: "we failed to communicate with the device in the probe function
>> > > and want to mark it unusable", which is a bit different from "the
>> > > device was explicitly disabled by the user".
>> > >
>> > > Anyway, if you think "disabled" is more appropriate, I'll use that.
>> > >
>> > > >
>> > > > > +       /* We intentionally leak the memory we allocate here, because the new
>> > > > > +        * OF property might live longer than the underlying dev, so no way
>> > > > > +        * we can use devm_kzalloc() here.
>> > > > > +        */
>> > > > > +       return;
>> > > > > +
>> > > > > +err:
>> > > > > +       kfree(newprop->value);
>> > > > > +       kfree(newprop->name);
>> > > > > +       kfree(newprop);
>> > > > > +}
>> > > > > +
>> > > > >  static int rpi_touchscreen_probe(struct i2c_client *i2c,
>> > > > >                                  const struct i2c_device_id *id)
>> > > > >  {
>> > > > > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
>> > > > >
>> > > > >         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
>> > > > >         if (ver < 0) {
>> > > > > +               rpi_touchscreen_set_status_fail(i2c);
>> > > >
>> > > > I've thought some more about this and I still think this should be
>> > > > handled in the driver core or i2c core.
>> > > >
>> > > > The reason is simple. I think the state of the system should be the
>> > > > same after this as if you booted with 'status = "disabled"' for this
>> > > > node. And that means the device should be removed completely because
>> > > > we don't create struct device's for disabled nodes.
>> > >
>> > > That was my feeling to when first discussing the issue with Daniel and
>> > > Thierry on IRC, but after digging a bit in the code I'm no longer sure
>> > > this is a good idea. At least, I don't think basing the decision to
>> > > disable the device (or mark it unusable) based on the return value of
>> > > the probe function is a good idea.
>> >
>> > I'm not so sure about that. -ENODEV seems like a very suitable error
>> > code to base that decision on. A random sampling of a handful of drivers
>> > confirms that this is primarily used to report situations where it is
>> > impossible for the device to ever be probed successfully, so might as
>> > well just remove it.
>>
>> It's not that easy. It has to be done from the I2C core since it's the
>> only one who can call device_unregister() and cleanup the other bits
>> associated with an I2C device (see i2c_unregister_device()). Now, the
>> i2c_driver->probe() function is called from a context where I'm almost
>> sure device_unregister() can't be called since we might still be in the
>> device_register() path. The solution would be to queue the
>> unregistration work to a workqueue, but I'm not even sure this is safe
>> to do that. What if the I2C adapter exposing the device is removed in
>> the meantime? Of course, all of this can be addressed, I'm just
>> wondering if it's really worth the trouble (we're likely to introduce
>> new races or other kind of bugs while doing that), especially since
>> placing the device in a "fail" state and still keeping it around would
>> solve the problem without requiring all the extra cleanup we're talking
>> about here.
>
> I think you have to put the device status into "fail" immediately,
> otherwise there's a race with deferred probing. Scenario:
>
> 1. vc4 loads, panel isn't there yet -> EPROBE_DEFER.
> 2. rpi driver loads, notices panel isn't there, returns -ENODEV

Step 3a: i2c core updates device DT status property
Step 3b: i2c core fires off worker for device_unregister

That solves the race, right?

> 3. i2c core fires off the worker and finishes it's ->probe callback.
> 4. device core starts a reprobe trigger
> 5. vc4 gets loaded, does the of_device_is_available check, but since
> that's not yet update it doesn't get the ENODEV, but still EPROBE_DEFER.
> 6. i2c worker disables the device and unregisters it.
>
> -> vc4 fails to load since nothing triggers another reprobe anymore.
>
> At least afaics device removal does not trigger a reprobe.
> -Daniel
>
>>
>> >
>> > At the very least I think it is worth proposing the patch and let Greg
>> > and others weigh in.
>>
>> Well, if it was an easy thing to do, I guess I would have gone for this
>> approach from the beginning, but I fear doing that will be much more
>> complicated than we think it is (maybe I'm wrong).
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Boris Brezillon]

On Fri, 4 May 2018 14:29:27 -0500
Rob Herring <robh+dt@kernel.org> wrote:

> On Fri, May 4, 2018 at 9:20 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, May 04, 2018 at 02:17:49PM +0200, Boris Brezillon wrote:  
> >> On Fri, 4 May 2018 11:47:48 +0200
> >> Thierry Reding <thierry.reding@gmail.com> wrote:
> >>  
> >> > On Fri, May 04, 2018 at 10:06:53AM +0200, Boris Brezillon wrote:  
> >> > > Hi Rob,
> >> > >
> >> > > On Thu, 3 May 2018 12:12:39 -0500
> >> > > Rob Herring <robh+dt@kernel.org> wrote:
> >> > >  
> >> > > > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> >> > > > <boris.brezillon@bootlin.com> wrote:  
> >> > > > > The device might be described in the device tree but not connected to
> >> > > > > the I2C bus. Update the status property so that the DRM panel logic
> >> > > > > returns -ENODEV when someone tries to get the panel attached to this
> >> > > > > DT node.
> >> > > > >
> >> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >> > > > > ---
> >> > > > >  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
> >> > > > >  1 file changed, 35 insertions(+)
> >> > > > >
> >> > > > > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> >> > > > > index 2c9c9722734f..b8fcb1acef75 100644
> >> > > > > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> >> > > > > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> >> > > > > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
> >> > > > >         .get_modes = rpi_touchscreen_get_modes,
> >> > > > >  };
> >> > > > >
> >> > > > > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> >> > > > > +{
> >> > > > > +       struct property *newprop;
> >> > > > > +
> >> > > > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> >> > > > > +       if (!newprop)
> >> > > > > +               return;
> >> > > > > +
> >> > > > > +       newprop->name = kstrdup("status", GFP_KERNEL);
> >> > > > > +       if (!newprop->name)
> >> > > > > +               goto err;
> >> > > > > +
> >> > > > > +       newprop->value = kstrdup("fail", GFP_KERNEL);
> >> > > > > +       if (!newprop->value)
> >> > > > > +               goto err;
> >> > > > > +
> >> > > > > +       newprop->length = sizeof("fail");
> >> > > > > +
> >> > > > > +       if (of_update_property(i2c->dev.of_node, newprop))
> >> > > > > +               goto err;
> >> > > > > +  
> >> > > >
> >> > > > As I mentioned on irc, can you make this a common DT function.  
> >> > >
> >> > > Yep, will move that to drivers/of/base.c and make it generic.
> >> > >  
> >> > > >
> >> > > > I'm not sure if it matters that we set status to fail vs. disabled. I
> >> > > > somewhat prefer the latter as we already have other cases and I'd
> >> > > > rather the api not pass a string in. I can't think of any reason to
> >> > > > distinguish the difference between fail and disabled.  
> >> > >
> >> > > Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
> >> > > and "fail" seemed like a good match for what we are trying to express
> >> > > here: "we failed to communicate with the device in the probe function
> >> > > and want to mark it unusable", which is a bit different from "the
> >> > > device was explicitly disabled by the user".
> >> > >
> >> > > Anyway, if you think "disabled" is more appropriate, I'll use that.
> >> > >  
> >> > > >  
> >> > > > > +       /* We intentionally leak the memory we allocate here, because the new
> >> > > > > +        * OF property might live longer than the underlying dev, so no way
> >> > > > > +        * we can use devm_kzalloc() here.
> >> > > > > +        */
> >> > > > > +       return;
> >> > > > > +
> >> > > > > +err:
> >> > > > > +       kfree(newprop->value);
> >> > > > > +       kfree(newprop->name);
> >> > > > > +       kfree(newprop);
> >> > > > > +}
> >> > > > > +
> >> > > > >  static int rpi_touchscreen_probe(struct i2c_client *i2c,
> >> > > > >                                  const struct i2c_device_id *id)
> >> > > > >  {
> >> > > > > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
> >> > > > >
> >> > > > >         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> >> > > > >         if (ver < 0) {
> >> > > > > +               rpi_touchscreen_set_status_fail(i2c);  
> >> > > >
> >> > > > I've thought some more about this and I still think this should be
> >> > > > handled in the driver core or i2c core.
> >> > > >
> >> > > > The reason is simple. I think the state of the system should be the
> >> > > > same after this as if you booted with 'status = "disabled"' for this
> >> > > > node. And that means the device should be removed completely because
> >> > > > we don't create struct device's for disabled nodes.  
> >> > >
> >> > > That was my feeling to when first discussing the issue with Daniel and
> >> > > Thierry on IRC, but after digging a bit in the code I'm no longer sure
> >> > > this is a good idea. At least, I don't think basing the decision to
> >> > > disable the device (or mark it unusable) based on the return value of
> >> > > the probe function is a good idea.  
> >> >
> >> > I'm not so sure about that. -ENODEV seems like a very suitable error
> >> > code to base that decision on. A random sampling of a handful of drivers
> >> > confirms that this is primarily used to report situations where it is
> >> > impossible for the device to ever be probed successfully, so might as
> >> > well just remove it.  
> >>
> >> It's not that easy. It has to be done from the I2C core since it's the
> >> only one who can call device_unregister() and cleanup the other bits
> >> associated with an I2C device (see i2c_unregister_device()). Now, the
> >> i2c_driver->probe() function is called from a context where I'm almost
> >> sure device_unregister() can't be called since we might still be in the
> >> device_register() path. The solution would be to queue the
> >> unregistration work to a workqueue, but I'm not even sure this is safe
> >> to do that. What if the I2C adapter exposing the device is removed in
> >> the meantime? Of course, all of this can be addressed, I'm just
> >> wondering if it's really worth the trouble (we're likely to introduce
> >> new races or other kind of bugs while doing that), especially since
> >> placing the device in a "fail" state and still keeping it around would
> >> solve the problem without requiring all the extra cleanup we're talking
> >> about here.  
> >
> > I think you have to put the device status into "fail" immediately,
> > otherwise there's a race with deferred probing. Scenario:
> >
> > 1. vc4 loads, panel isn't there yet -> EPROBE_DEFER.
> > 2. rpi driver loads, notices panel isn't there, returns -ENODEV  
> 
> Step 3a: i2c core updates device DT status property
> Step 3b: i2c core fires off worker for device_unregister
> 
> That solves the race, right?

Yep, that should solve this particular race, but we still have a
problem with the deferred-probe trigger. The only thing that forces all
devices that have seen -EPROBE_DEFER to be probed again is when a
device is bound to a driver, which in our case won't happen because we
return -ENODEV. If we're lucky, something else in the system will
trigger that after the I2C core has changed the status prop, and if
we're not, the DRM dev will never but probed again.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel