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 <mich...@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
