| Message ID | 20230412073954.20601-1-tony@atomide.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/omap: dsi: Fix deferred probe warnings | expand |
* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [230412 11:59]: > On Wed, Apr 12, 2023 at 11:55:34AM +0300, Tomi Valkeinen wrote: > > On 12/04/2023 11:50, Laurent Pinchart wrote: > > > Hi Tony, > > > > > > Thank you for the patch. > > > > > > On Wed, Apr 12, 2023 at 10:39:53AM +0300, Tony Lindgren wrote: > > >> We may not have dsi->dsidev initialized during probe, and that can > > >> lead into various dsi related warnings as omap_dsi_host_detach() gets > > >> called with dsi->dsidev set to NULL. > > >> > > >> The warnings can be "Fixed dependency cycle(s)" followed by a > > >> WARNING: CPU: 0 PID: 787 at drivers/gpu/drm/omapdrm/dss/dsi.c:4414. > > > > > > How can this happen ? I assume .detach() can't be called without a > > > priori successful call to .attach(), that that sets dsi->dsidev. > > > > I had a quick look, and the driver calls mipi_dsi_host_register() in > > probe, and mipi_dsi_host_unregister() in remove. > > > > mipi_dsi_host_unregister() always calls mipi_dsi_detach(), but I don't > > think mipi_dsi_host_register() always calls attach, which happens later > > when the peripheral probes. > > Is this something that should be addressed in the DRM MIPI DSI helpers, > to only detach after an attach ? Tomi, any comments on detach after attach? Regards, Tony > > >> Let's fix the warnings by checking for a valid dsi->dsidev. > > >> > > >> Signed-off-by: Tony Lindgren <tony@atomide.com> > > >> --- > > >> drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > > >> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > > >> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > > >> @@ -4411,7 +4411,7 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host, > > >> { > > >> struct dsi_data *dsi = host_to_omap(host); > > >> > > >> - if (WARN_ON(dsi->dsidev != client)) > > >> + if (dsi->dsidev && WARN_ON(dsi->dsidev != client)) > > >> return -EINVAL; > > >> > > >> cancel_delayed_work_sync(&dsi->dsi_disable_work); > > -- > Regards, > > Laurent Pinchart >
On 12/04/2023 10:39, Tony Lindgren wrote: > We may not have dsi->dsidev initialized during probe, and that can > lead into various dsi related warnings as omap_dsi_host_detach() gets > called with dsi->dsidev set to NULL. > > The warnings can be "Fixed dependency cycle(s)" followed by a > WARNING: CPU: 0 PID: 787 at drivers/gpu/drm/omapdrm/dss/dsi.c:4414. > > Let's fix the warnings by checking for a valid dsi->dsidev. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > @@ -4411,7 +4411,7 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host, > { > struct dsi_data *dsi = host_to_omap(host); > > - if (WARN_ON(dsi->dsidev != client)) > + if (dsi->dsidev && WARN_ON(dsi->dsidev != client)) > return -EINVAL; > > cancel_delayed_work_sync(&dsi->dsi_disable_work); Shouldn't this rather be if (!dsi->dsidev) return 0; before the if (WARN_ON(dsi->dsidev != client)) line? If dsi->dsidev is NULL, then attach hasn't been called, and we shouldn't do anything in the detach callback either. With your change we'll end up doing all the work in the detach callback, without ever doing their counterpart in the attach side. Tomi
On 13/09/2023 10:37, Tony Lindgren wrote: > * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [230412 11:59]: >> On Wed, Apr 12, 2023 at 11:55:34AM +0300, Tomi Valkeinen wrote: >>> On 12/04/2023 11:50, Laurent Pinchart wrote: >>>> Hi Tony, >>>> >>>> Thank you for the patch. >>>> >>>> On Wed, Apr 12, 2023 at 10:39:53AM +0300, Tony Lindgren wrote: >>>>> We may not have dsi->dsidev initialized during probe, and that can >>>>> lead into various dsi related warnings as omap_dsi_host_detach() gets >>>>> called with dsi->dsidev set to NULL. >>>>> >>>>> The warnings can be "Fixed dependency cycle(s)" followed by a >>>>> WARNING: CPU: 0 PID: 787 at drivers/gpu/drm/omapdrm/dss/dsi.c:4414. >>>> >>>> How can this happen ? I assume .detach() can't be called without a >>>> priori successful call to .attach(), that that sets dsi->dsidev. >>> >>> I had a quick look, and the driver calls mipi_dsi_host_register() in >>> probe, and mipi_dsi_host_unregister() in remove. >>> >>> mipi_dsi_host_unregister() always calls mipi_dsi_detach(), but I don't >>> think mipi_dsi_host_register() always calls attach, which happens later >>> when the peripheral probes. >> >> Is this something that should be addressed in the DRM MIPI DSI helpers, >> to only detach after an attach ? > > Tomi, any comments on detach after attach? I sent a comment to the patch. As Laurent said, I think this really should be fixed in the framework side. Calling detach in drivers without attach feels just plain wrong. However, I don't see any harm in the patch (but perhaps some changes needed as per my comments), and it will fix the issue for omapdrm, until someone has the time and energy to look at a real fix. Tomi
* Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> [230913 11:59]: > On 12/04/2023 10:39, Tony Lindgren wrote: > > We may not have dsi->dsidev initialized during probe, and that can > > lead into various dsi related warnings as omap_dsi_host_detach() gets > > called with dsi->dsidev set to NULL. > > > > The warnings can be "Fixed dependency cycle(s)" followed by a > > WARNING: CPU: 0 PID: 787 at drivers/gpu/drm/omapdrm/dss/dsi.c:4414. > > > > Let's fix the warnings by checking for a valid dsi->dsidev. > > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > --- > > drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > > @@ -4411,7 +4411,7 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host, > > { > > struct dsi_data *dsi = host_to_omap(host); > > - if (WARN_ON(dsi->dsidev != client)) > > + if (dsi->dsidev && WARN_ON(dsi->dsidev != client)) > > return -EINVAL; > > cancel_delayed_work_sync(&dsi->dsi_disable_work); > > Shouldn't this rather be > > if (!dsi->dsidev) > return 0; > > before the if (WARN_ON(dsi->dsidev != client)) line? > > If dsi->dsidev is NULL, then attach hasn't been called, and we shouldn't do > anything in the detach callback either. > > With your change we'll end up doing all the work in the detach callback, > without ever doing their counterpart in the attach side. Oops, I'll check that thanks. Tony
Hi Tomi and Tony, > Am 13.09.2023 um 13:59 schrieb Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>: > > On 12/04/2023 10:39, Tony Lindgren wrote: >> We may not have dsi->dsidev initialized during probe, and that can >> lead into various dsi related warnings as omap_dsi_host_detach() gets >> called with dsi->dsidev set to NULL. >> The warnings can be "Fixed dependency cycle(s)" followed by a >> WARNING: CPU: 0 PID: 787 at drivers/gpu/drm/omapdrm/dss/dsi.c:4414. >> Let's fix the warnings by checking for a valid dsi->dsidev. >> Signed-off-by: Tony Lindgren <tony@atomide.com> >> --- >> drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c >> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c >> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c >> @@ -4411,7 +4411,7 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host, >> { >> struct dsi_data *dsi = host_to_omap(host); >> - if (WARN_ON(dsi->dsidev != client)) >> + if (dsi->dsidev && WARN_ON(dsi->dsidev != client)) >> return -EINVAL; >> cancel_delayed_work_sync(&dsi->dsi_disable_work); > > Shouldn't this rather be > > if (!dsi->dsidev) > return 0; > > before the if (WARN_ON(dsi->dsidev != client)) line? Yes you are right. We have a different variant in our Pyra kernel: What we currently have in our Pyra tree is: https://git.goldelico.com/?p=letux-kernel.git;a=commitdiff;h=5bf7bd64eec1eb924e794e8d6600919f0dae8c5a;hp=27a0cd6263194d1465e9c53293d35f8c8c988f9d struct dsi_data *dsi = host_to_omap(host); - if (WARN_ON(dsi->dsidev != client)) +printk("%s\n", __func__); + + if (!dsi->dsidev || WARN_ON(dsi->dsidev != client)) return -EINVAL; cancel_delayed_work_sync(&dsi->dsi_disable_work); > > If dsi->dsidev is NULL, then attach hasn't been called, and we shouldn't do anything in the detach callback either. > > With your change we'll end up doing all the work in the detach callback, without ever doing their counterpart in the attach side. If useful, I can post above mentioned patch (without printk). BR and thanks, Nikolaus
* H. Nikolaus Schaller <hns@goldelico.com> [230916 12:50]: > Hi Tomi and Tony, > > > Am 13.09.2023 um 13:59 schrieb Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>: > > > > On 12/04/2023 10:39, Tony Lindgren wrote: > >> We may not have dsi->dsidev initialized during probe, and that can > >> lead into various dsi related warnings as omap_dsi_host_detach() gets > >> called with dsi->dsidev set to NULL. > >> The warnings can be "Fixed dependency cycle(s)" followed by a > >> WARNING: CPU: 0 PID: 787 at drivers/gpu/drm/omapdrm/dss/dsi.c:4414. > >> Let's fix the warnings by checking for a valid dsi->dsidev. > >> Signed-off-by: Tony Lindgren <tony@atomide.com> > >> --- > >> drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > >> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > >> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > >> @@ -4411,7 +4411,7 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host, > >> { > >> struct dsi_data *dsi = host_to_omap(host); > >> - if (WARN_ON(dsi->dsidev != client)) > >> + if (dsi->dsidev && WARN_ON(dsi->dsidev != client)) > >> return -EINVAL; > >> cancel_delayed_work_sync(&dsi->dsi_disable_work); > > > > Shouldn't this rather be > > > > if (!dsi->dsidev) > > return 0; > > > > before the if (WARN_ON(dsi->dsidev != client)) line? > > Yes you are right. We have a different variant in our Pyra kernel: > > What we currently have in our Pyra tree is: https://git.goldelico.com/?p=letux-kernel.git;a=commitdiff;h=5bf7bd64eec1eb924e794e8d6600919f0dae8c5a;hp=27a0cd6263194d1465e9c53293d35f8c8c988f9d > > struct dsi_data *dsi = host_to_omap(host); > > - if (WARN_ON(dsi->dsidev != client)) > +printk("%s\n", __func__); > + > + if (!dsi->dsidev || WARN_ON(dsi->dsidev != client)) > return -EINVAL; > > cancel_delayed_work_sync(&dsi->dsi_disable_work); > > > > > If dsi->dsidev is NULL, then attach hasn't been called, and we shouldn't do anything in the detach callback either. > > > > With your change we'll end up doing all the work in the detach callback, without ever doing their counterpart in the attach side. > > If useful, I can post above mentioned patch (without printk). Sounds good to me. Thanks, Tony
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -4411,7 +4411,7 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host, { struct dsi_data *dsi = host_to_omap(host); - if (WARN_ON(dsi->dsidev != client)) + if (dsi->dsidev && WARN_ON(dsi->dsidev != client)) return -EINVAL; cancel_delayed_work_sync(&dsi->dsi_disable_work);
We may not have dsi->dsidev initialized during probe, and that can lead into various dsi related warnings as omap_dsi_host_detach() gets called with dsi->dsidev set to NULL. The warnings can be "Fixed dependency cycle(s)" followed by a WARNING: CPU: 0 PID: 787 at drivers/gpu/drm/omapdrm/dss/dsi.c:4414. Let's fix the warnings by checking for a valid dsi->dsidev. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)