Re: [PATCH 5/5] drm/bridge: dw-mipi-dsi: Fix dsi registration during drm probing

[Sam Ravnborg <sam@ravnborg.org>]

Hi Michael,

I fail to follow the logic in this patch.


On Sat, Oct 16, 2021 at 10:22:32AM +0000, Michael Trimarchi wrote:
> The dsi registration is implemented in rockchip platform driver.
> The attach can be called before the probe is terminated and therefore
> we need to be sure that corresponding entry during attach is valid
> 
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c   |  8 +++++++-
>  drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 12 ++++++++----
>  include/drm/bridge/dw_mipi_dsi.h                |  2 +-
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index e44e18a0112a..44b211be15fc 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -362,8 +362,14 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
>  		dsi->device_found = true;
>  	}
>  
> +	/*
> +	 * NOTE: the dsi registration is implemented in
> +	 * platform driver, that to say dsi would be exist after
> +	 * probe is terminated. The call is done before the end of probe
> +	 * so we need to pass the dsi to the platform driver.
> +	 */
>  	if (pdata->host_ops && pdata->host_ops->attach) {
> -		ret = pdata->host_ops->attach(pdata->priv_data, device);
> +		ret = pdata->host_ops->attach(pdata->priv_data, device, dsi);
>  		if (ret < 0)
>  			return ret;
>  	}
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> index a2262bee5aa4..32ddc8642ec1 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> @@ -997,7 +997,8 @@ static const struct component_ops dw_mipi_dsi_rockchip_ops = {
>  };
>  
>  static int dw_mipi_dsi_rockchip_host_attach(void *priv_data,
> -					    struct mipi_dsi_device *device)
> +					    struct mipi_dsi_device *device,
> +					    struct dw_mipi_dsi *dmd)
>  {
>  	struct dw_mipi_dsi_rockchip *dsi = priv_data;
>  	struct device *second;
> @@ -1005,6 +1006,8 @@ static int dw_mipi_dsi_rockchip_host_attach(void *priv_data,
>  
>  	mutex_lock(&dsi->usage_mutex);
>  
> +	dsi->dmd = dmd;
> +
>  	if (dsi->usage_mode != DW_DSI_USAGE_IDLE) {
>  		DRM_DEV_ERROR(dsi->dev, "dsi controller already in use\n");
>  		mutex_unlock(&dsi->usage_mutex);
> @@ -1280,6 +1283,7 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> +	struct dw_mipi_dsi *dmd;
>  	struct dw_mipi_dsi_rockchip *dsi;
>  	struct phy_provider *phy_provider;
>  	struct resource *res;
> @@ -1391,9 +1395,9 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
>  	if (IS_ERR(phy_provider))
>  		return PTR_ERR(phy_provider);
>  
> -	dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> -	if (IS_ERR(dsi->dmd)) {
> -		ret = PTR_ERR(dsi->dmd);
> +	dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> +	if (IS_ERR(dmd)) {
> +		ret = PTR_ERR(dmd);

The memory pointed to by dmd is allocated in dw_mipi_dsi_probe(), but
the pointer is not saved here.
We rely on the attach operation to save the dmd pointer.


In other words - the attach operation must be called before we call
dw_mipi_dsi_rockchip_remove(), which uses the dmd member.

This all looks wrong to me - are we papering over some other issue
here?

	Sam