| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3f72a209-55b0-c361-2737-ad4cb811053b@samsung.com>
Date: Thu, 29 Aug 2019 16:57:48 +0200
From: Andrzej Hajda <a.hajda@...sung.com>
To: John Stultz <john.stultz@...aro.org>,
lkml <linux-kernel@...r.kernel.org>
Cc: Matt Redfearn <matt.redfearn@...nci.com>,
Xinliang Liu <z.liuxinliang@...ilicon.com>,
Rongrong Zou <zourongrong@...il.com>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Neil Armstrong <narmstrong@...libre.com>,
Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...l.net>,
Thierry Reding <thierry.reding@...il.com>,
David Airlie <airlied@...ux.ie>,
Sean Paul <seanpaul@...omium.org>,
Sam Ravnborg <sam@...nborg.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic
On 29.08.2019 08:05, John Stultz wrote:
> Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
> host at probe time") landed in -next the HiKey board would fail
> to boot, looping:
>
> adv7511 2-0039: failed to find dsi host
>
> messages over and over. Andrzej Hajda suggested this is due to a
> circular dependency issue, and that the adv7511 change is
> correcting the improper order used earlier.
>
> Unfortunately this means the DSI drivers that use adv7511 need
> to also need to be updated to use the proper ordering to
> continue to work.
>
> This patch tries to reorder the initialization to register the
> dsi_host first, and then call component_add via dsi_host_attach,
> instead of doing that at probe time.
>
> This seems to resolve the issue with the HiKey board.
>
> Cc: Andrzej Hajda <a.hajda@...sung.com>
> Cc: Matt Redfearn <matt.redfearn@...nci.com>
> Cc: Xinliang Liu <z.liuxinliang@...ilicon.com>
> Cc: Rongrong Zou <zourongrong@...il.com>
> Cc: Laurent Pinchart <Laurent.pinchart@...asonboard.com>
> Cc: Neil Armstrong <narmstrong@...libre.com>
> Cc: Jonas Karlman <jonas@...boo.se>
> Cc: Jernej Skrabec <jernej.skrabec@...l.net>
> Cc: Thierry Reding <thierry.reding@...il.com>
> Cc: David Airlie <airlied@...ux.ie>,
> Cc: Sean Paul <seanpaul@...omium.org>
> Cc: Sam Ravnborg <sam@...nborg.org>
> Cc: "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
> Fixes: 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI host at probe time")
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
> Note: I'm really not super familiar with the DSI code here,
> and am mostly just trying to refactor the existing code in a
> similar fashion to the suggested dw-mipi-dsi-rockchip.c
> implementation. Careful review would be greatly appreciated!
>
> Also there is an outstanding regression on the db410c since it
> similarly uses the adv7511 and probably needs a similar rework.
> ---
> drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 111 ++++++++++---------
> 1 file changed, 56 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> index 5bf8138941de..696cee1a1219 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> @@ -79,6 +79,7 @@ struct dsi_hw_ctx {
> };
>
> struct dw_dsi {
> + struct device *dev;
> struct drm_encoder encoder;
> struct drm_bridge *bridge;
> struct mipi_dsi_host host;
> @@ -724,51 +725,6 @@ static int dw_drm_encoder_init(struct device *dev,
> return 0;
> }
>
> -static int dsi_host_attach(struct mipi_dsi_host *host,
> - struct mipi_dsi_device *mdsi)
> -{
> - struct dw_dsi *dsi = host_to_dsi(host);
> -
> - if (mdsi->lanes < 1 || mdsi->lanes > 4) {
> - DRM_ERROR("dsi device params invalid\n");
> - return -EINVAL;
> - }
> -
> - dsi->lanes = mdsi->lanes;
> - dsi->format = mdsi->format;
> - dsi->mode_flags = mdsi->mode_flags;
> -
> - return 0;
> -}
> -
> -static int dsi_host_detach(struct mipi_dsi_host *host,
> - struct mipi_dsi_device *mdsi)
> -{
> - /* do nothing */
> - return 0;
> -}
> -
> -static const struct mipi_dsi_host_ops dsi_host_ops = {
> - .attach = dsi_host_attach,
> - .detach = dsi_host_detach,
> -};
> -
> -static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
> -{
> - struct mipi_dsi_host *host = &dsi->host;
> - int ret;
> -
> - host->dev = dev;
> - host->ops = &dsi_host_ops;
> - ret = mipi_dsi_host_register(host);
> - if (ret) {
> - DRM_ERROR("failed to register dsi host\n");
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> static int dsi_bridge_init(struct drm_device *dev, struct dw_dsi *dsi)
> {
> struct drm_encoder *encoder = &dsi->encoder;
> @@ -796,10 +752,6 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
> if (ret)
> return ret;
>
> - ret = dsi_host_init(dev, dsi);
> - if (ret)
> - return ret;
> -
> ret = dsi_bridge_init(drm_dev, dsi);
> if (ret)
> return ret;
> @@ -817,13 +769,22 @@ static const struct component_ops dsi_ops = {
> .unbind = dsi_unbind,
> };
>
> -static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
> +static int dsi_host_attach(struct mipi_dsi_host *host,
> + struct mipi_dsi_device *mdsi)
> {
> - struct dsi_hw_ctx *ctx = dsi->ctx;
> - struct device_node *np = pdev->dev.of_node;
> - struct resource *res;
> + struct dw_dsi *dsi = host_to_dsi(host);
> + struct device_node *np = dsi->dev->of_node;
> int ret;
>
> + if (mdsi->lanes < 1 || mdsi->lanes > 4) {
> + DRM_ERROR("dsi device params invalid\n");
> + return -EINVAL;
> + }
> +
> + dsi->lanes = mdsi->lanes;
> + dsi->format = mdsi->format;
> + dsi->mode_flags = mdsi->mode_flags;
> +
> /*
> * Get the endpoint node. In our case, dsi has one output port1
> * to which the external HDMI bridge is connected.
> @@ -832,6 +793,42 @@ static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
> if (ret)
> return ret;
>
> + return component_add(dsi->dev, &dsi_ops);
> +}
> +
> +static int dsi_host_detach(struct mipi_dsi_host *host,
> + struct mipi_dsi_device *mdsi)
> +{
> + /* do nothing */
> + return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops dsi_host_ops = {
> + .attach = dsi_host_attach,
> + .detach = dsi_host_detach,
> +};
> +
> +static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
> +{
> + struct mipi_dsi_host *host = &dsi->host;
> + int ret;
> +
> + host->dev = dev;
> + host->ops = &dsi_host_ops;
> + ret = mipi_dsi_host_register(host);
> + if (ret) {
> + DRM_ERROR("failed to register dsi host\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
> +{
> + struct dsi_hw_ctx *ctx = dsi->ctx;
> + struct resource *res;
> +
> ctx->pclk = devm_clk_get(&pdev->dev, "pclk");
> if (IS_ERR(ctx->pclk)) {
> DRM_ERROR("failed to get pclk clock\n");
> @@ -862,15 +859,19 @@ static int dsi_probe(struct platform_device *pdev)
> }
> dsi = &data->dsi;
> ctx = &data->ctx;
> + dsi->dev = &pdev->dev;
> dsi->ctx = ctx;
>
> ret = dsi_parse_dt(pdev, dsi);
> if (ret)
> return ret;
>
> - platform_set_drvdata(pdev, data);
> + ret = dsi_host_init(&pdev->dev, dsi);
> + if (ret)
> + return ret;
>
> - return component_add(&pdev->dev, &dsi_ops);
> + platform_set_drvdata(pdev, data);
I suspect platform_set_drvdata should be before dsi_host_init.
Imagine dsi_probe calls dsi_host_init it registers dsi_host and
instantly dsi_device appears, so dsi_host_attach is called, it calls
component_add and dsi_bind is called, which requires valid drvdata.
Beside this it looks correct, but it would be good to test it.
So after fixing above you can add my R-B.
Regards
Andrzej
> + return 0;
> }
>
> static int dsi_remove(struct platform_device *pdev)
Powered by blists - more mailing lists