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(-)
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);
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
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
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.
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
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;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;
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);
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);elselvds->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
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;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;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);
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);elselvds->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
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
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) {
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
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;
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
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);
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
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?
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
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_ds...
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
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
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.
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; }
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
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
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
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).
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).
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:
- vc4 loads, panel isn't there yet -> EPROBE_DEFER.
- rpi driver loads, notices panel isn't there, returns -ENODEV
- i2c core fires off the worker and finishes it's ->probe callback.
- device core starts a reprobe trigger
- 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.
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:
- vc4 loads, panel isn't there yet -> EPROBE_DEFER.
- rpi driver loads, notices panel isn't there, returns -ENODEV
- i2c core fires off the worker and finishes it's ->probe callback.
- device core starts a reprobe trigger
- 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.
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:
- vc4 loads, panel isn't there yet -> EPROBE_DEFER.
- 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?
- i2c core fires off the worker and finishes it's ->probe callback.
- device core starts a reprobe trigger
- 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
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:
- vc4 loads, panel isn't there yet -> EPROBE_DEFER.
- 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@lists.freedesktop.org
-
Boris Brezillon -
Daniel Vetter -
Rob Herring -
Thierry Reding