Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211016102232.202119-1-michael@amarulasolutions.com>
 <20211016102232.202119-6-michael@amarulasolutions.com> <YWrEqStMPYuzUoUn@ravnborg.org>
In-Reply-To: <YWrEqStMPYuzUoUn@ravnborg.org>
From:   Michael Nazzareno Trimarchi <michael@amarulasolutions.com>
Date:   Sat, 16 Oct 2021 15:32:28 +0200
Message-ID: <CAOf5uwm0iiSNvDF9ambZPQ9O6OCkWWAobxXvKYd6ieDscnNXVw@mail.gmail.com>
Subject: Re: [PATCH 5/5] drm/bridge: dw-mipi-dsi: Fix dsi registration during
 drm probing
To:     Sam Ravnborg <sam@ravnborg.org>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Rob Herring <robh+dt@kernel.org>,
        =?UTF-8?Q?Heiko_St=C3=BCbner?= <heiko@sntech.de>,
        devicetree <devicetree@vger.kernel.org>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Sam

On Sat, Oct 16, 2021 at 2:25 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> 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

Ok, it's wrong. I was not expecting that call.Anyway this was my path
on linux-next

dw_mipi_dsi_rockchip_probe
dw_mipi_dsi_probe -->start call

dw_mipi_dsi_rockchip_host_attach <-- this was not able to use dmd

dw_mipi_dsi_probe -> exit from the call

Michael

> here?
>
>         Sam


-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com