diff mbox series

drm/omap: dsi: Fix deferred probe warnings

Message ID 20230412073954.20601-1-tony@atomide.com
State New
Headers
Series drm/omap: dsi: Fix deferred probe warnings |

Commit Message

Tony Lindgren April 12, 2023, 7:39 a.m. UTC 
  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(-)

Comments

Laurent Pinchart April 12, 2023, 8:50 a.m. UTC | #1 
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.

> 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);
Tomi Valkeinen April 12, 2023, 8:55 a.m. UTC | #2 
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.

  Tomi

>> 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);
>
Laurent Pinchart April 12, 2023, 8:59 a.m. UTC | #3 
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 ?

> >> 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);
H. Nikolaus Schaller April 12, 2023, 9 a.m. UTC | #4 
> Am 12.04.2023 um 10:50 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> 
> 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 have a similar patch (not submitted because it looks like a workaround) in our LetuxOS kernel:

https://git.goldelico.com/?p=letux-kernel.git;a=commitdiff;h=0656acb534439d4546b33826de62694e1df1b8ad

There I had commented:

dsi_init_output() called by dsi_probe() may fail. In that
case mipi_dsi_host_unregister() is called which may call
omap_dsi_host_detach() with uninitialized dsi->dsidev
because omap_dsi_host_attach() was never called before.

This happens if the panel driver asks for an EPROBE_DEFER.

So let's suppress the WARN() in this special case.

[    7.416759] WARNING: CPU: 0 PID: 32 at drivers/gpu/drm/omapdrm/dss/dsi.c:4419 omap_dsi_host_detach+0x3c/0xbc [omapdrm]
[    7.436053] Modules linked in: ina2xx_adc snd_soc_ts3a227e bq2429x_charger bq27xxx_battery_i2c(+) bq27xxx_battery ina2xx tca8418_keypad as5013(+) omapdrm hci_uart cec palmas_pwrbutton btbcm bmp280_spi palmas_gpadc bluetooth usb3503 ecdh_generic bmc150_accel_i2c bmg160_i2c ecc bmc150_accel_core bmg160_core bmc150_magn_i2c bmp280_i2c bmc150_magn bno055 industrialio_triggered_buffer bmp280 kfifo_buf snd_soc_omap_aess display_connector drm_kms_helper syscopyarea snd_soc_omap_mcbsp snd_soc_ti_sdma sysfillrect ti_tpd12s015 sysimgblt fb_sys_fops wwan_on_off snd_soc_gtm601 generic_adc_battery drm snd_soc_w2cbw003_bt industrialio drm_panel_orientation_quirks pwm_bl pwm_omap_dmtimer ip_tables x_tables ipv6 autofs4
[    7.507068] CPU: 0 PID: 32 Comm: kworker/u4:2 Tainted: G        W          6.1.0-rc3-letux-lpae+ #11107
[    7.516964] Hardware name: Generic OMAP5 (Flattened Device Tree)
[    7.523284] Workqueue: events_unbound deferred_probe_work_func
[    7.529456]  unwind_backtrace from show_stack+0x10/0x14
[    7.534972]  show_stack from dump_stack_lvl+0x40/0x4c
[    7.540315]  dump_stack_lvl from __warn+0xb0/0x164
[    7.545379]  __warn from warn_slowpath_fmt+0x70/0x9c
[    7.550625]  warn_slowpath_fmt from omap_dsi_host_detach+0x3c/0xbc [omapdrm]
[    7.558137]  omap_dsi_host_detach [omapdrm] from mipi_dsi_remove_device_fn+0x10/0x20
[    7.566376]  mipi_dsi_remove_device_fn from device_for_each_child+0x60/0x94
[    7.573729]  device_for_each_child from mipi_dsi_host_unregister+0x20/0x54
[    7.580992]  mipi_dsi_host_unregister from dsi_probe+0x5d8/0x744 [omapdrm]
[    7.588315]  dsi_probe [omapdrm] from platform_probe+0x58/0xa8
[    7.594542]  platform_probe from really_probe+0x144/0x2ac
[    7.600249]  really_probe from __driver_probe_device+0xc4/0xd8
[    7.606411]  __driver_probe_device from driver_probe_device+0x3c/0xb8
[    7.613216]  driver_probe_device from __device_attach_driver+0x58/0xbc
[    7.620115]  __device_attach_driver from bus_for_each_drv+0xa0/0xb4
[    7.626737]  bus_for_each_drv from __device_attach+0xdc/0x150
[    7.632808]  __device_attach from bus_probe_device+0x28/0x80
[    7.638792]  bus_probe_device from deferred_probe_work_func+0x84/0xa0
[    7.645595]  deferred_probe_work_func from process_one_work+0x1a4/0x2d8
[    7.652587]  process_one_work from worker_thread+0x214/0x2b8
[    7.658567]  worker_thread from kthread+0xe4/0xf0
[    7.663542]  kthread from ret_from_fork+0x14/0x1c
[    7.668515] Exception stack(0xf01b5fb0 to 0xf01b5ff8)
[    7.673827] 5fa0:                                     00000000 00000000 00000000 00000000
[    7.682435] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    7.691038] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000

Hope this helps to find the real cause.

BR,
Nikolaus
Tony Lindgren Sept. 13, 2023, 7:37 a.m. UTC | #5 
* 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
>
Tomi Valkeinen Sept. 13, 2023, 11:59 a.m. UTC | #6 
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
Tomi Valkeinen Sept. 13, 2023, 12:02 p.m. UTC | #7 
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
Tony Lindgren Sept. 13, 2023, 12:50 p.m. UTC | #8 
* 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
H. Nikolaus Schaller Sept. 16, 2023, 12:50 p.m. UTC | #9 
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
Tony Lindgren Sept. 18, 2023, 3:48 a.m. UTC | #10 
* 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 mbox series

Patch

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);