diff mbox

[1/3] drm/sti: do not remove the drm_bridge that was never added

Message ID 20180502074025.12421-2-peda@axentia.se (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Rosin May 2, 2018, 7:40 a.m. UTC
The more natural approach would perhaps be to add an drm_bridge_add,
but there are several other bridges that never call drm_bridge_add.
Just removing the drm_bridge_remove is the easier fix.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/sti/sti_hda.c  | 1 -
 drivers/gpu/drm/sti/sti_hdmi.c | 1 -
 2 files changed, 2 deletions(-)

Comments

Daniel Vetter May 3, 2018, 9:06 a.m. UTC | #1
On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> The more natural approach would perhaps be to add an drm_bridge_add,
> but there are several other bridges that never call drm_bridge_add.
> Just removing the drm_bridge_remove is the easier fix.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

This mess is much bigger. There's 2 pairs of bridge functions:

- drm_bridge_attach/detach. Those are meant to be called by the overall
  drm driver to connect/disconnect a drm_bridge.

- drm_bridge_add/remove. These are supposed to be called by the bridge
  driver itself to register/unregister itself. Maybe we should rename
  them, since the same issue happens with drm_panel, with the same
  confusion.

I thought someone was working on a cleanup series to fix this mess, but I
didn't find anything.
-Daniel

> ---
>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> index 67bbdb49fffc..199db13f565c 100644
> --- a/drivers/gpu/drm/sti/sti_hda.c
> +++ b/drivers/gpu/drm/sti/sti_hda.c
> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>  	return 0;
>  
>  err_sysfs:
> -	drm_bridge_remove(bridge);
>  	return -EINVAL;
>  }
>  
> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> index 58f431102512..932724784942 100644
> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>  	return 0;
>  
>  err_sysfs:
> -	drm_bridge_remove(bridge);
>  	hdmi->drm_connector = NULL;
>  	return -EINVAL;
>  }
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Peter Rosin May 3, 2018, 9:12 p.m. UTC | #2
On 2018-05-03 11:06, Daniel Vetter wrote:
> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>> The more natural approach would perhaps be to add an drm_bridge_add,
>> but there are several other bridges that never call drm_bridge_add.
>> Just removing the drm_bridge_remove is the easier fix.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> 
> This mess is much bigger. There's 2 pairs of bridge functions:
> 
> - drm_bridge_attach/detach. Those are meant to be called by the overall
>   drm driver to connect/disconnect a drm_bridge.
> 
> - drm_bridge_add/remove. These are supposed to be called by the bridge
>   driver itself to register/unregister itself. Maybe we should rename
>   them, since the same issue happens with drm_panel, with the same
>   confusion.
> 
> I thought someone was working on a cleanup series to fix this mess, but I
> didn't find anything.

Ok, I just spotted the imbalance and didn't really dig into what
actually happens in these error paths. Now that I have done so I
believe that the removed drm_bridge_remove calls causes NULL
dereferences if/when the error paths are triggered.

So, I don't think this can wait for some bigger cleanup.

drm_bridge_remove calls list_del_init calls __list_del_entry calls
__list_del with NULL in both prev and next since the list member
is never initialized. prev and next are dereferenced by __list_del
and you have *boom*

I recommend adding the tag

Fixes: 84601dbdea36 ("drm: sti: rework init sequence")

so that stable picks this one up.

Cheers,
Peter

> -Daniel
> 
>> ---
>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>  2 files changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>> index 67bbdb49fffc..199db13f565c 100644
>> --- a/drivers/gpu/drm/sti/sti_hda.c
>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>>  	return 0;
>>  
>>  err_sysfs:
>> -	drm_bridge_remove(bridge);
>>  	return -EINVAL;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>> index 58f431102512..932724784942 100644
>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>>  	return 0;
>>  
>>  err_sysfs:
>> -	drm_bridge_remove(bridge);
>>  	hdmi->drm_connector = NULL;
>>  	return -EINVAL;
>>  }
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Daniel Vetter May 7, 2018, 1:39 p.m. UTC | #3
On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> On 2018-05-03 11:06, Daniel Vetter wrote:
> > On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >> The more natural approach would perhaps be to add an drm_bridge_add,
> >> but there are several other bridges that never call drm_bridge_add.
> >> Just removing the drm_bridge_remove is the easier fix.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> > 
> > This mess is much bigger. There's 2 pairs of bridge functions:
> > 
> > - drm_bridge_attach/detach. Those are meant to be called by the overall
> >   drm driver to connect/disconnect a drm_bridge.
> > 
> > - drm_bridge_add/remove. These are supposed to be called by the bridge
> >   driver itself to register/unregister itself. Maybe we should rename
> >   them, since the same issue happens with drm_panel, with the same
> >   confusion.
> > 
> > I thought someone was working on a cleanup series to fix this mess, but I
> > didn't find anything.
> 
> Ok, I just spotted the imbalance and didn't really dig into what
> actually happens in these error paths. Now that I have done so I
> believe that the removed drm_bridge_remove calls causes NULL
> dereferences if/when the error paths are triggered.
> 
> So, I don't think this can wait for some bigger cleanup.
> 
> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> __list_del with NULL in both prev and next since the list member
> is never initialized. prev and next are dereferenced by __list_del
> and you have *boom*
> 
> I recommend adding the tag
> 
> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> 
> so that stable picks this one up.

I just wanted to correct your commit message text - the correct solution
is definitely _not_ for sti here to call drm_bridge_add. It should call
drm_bridge_attach/detach only, as a pair.

I didn't check whether you instead have a _detach call missing or what's
going on here.
-Daniel
> 
> Cheers,
> Peter
> 
> > -Daniel
> > 
> >> ---
> >>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
> >>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
> >>  2 files changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> >> index 67bbdb49fffc..199db13f565c 100644
> >> --- a/drivers/gpu/drm/sti/sti_hda.c
> >> +++ b/drivers/gpu/drm/sti/sti_hda.c
> >> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
> >>  	return 0;
> >>  
> >>  err_sysfs:
> >> -	drm_bridge_remove(bridge);
> >>  	return -EINVAL;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> >> index 58f431102512..932724784942 100644
> >> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
> >>  	return 0;
> >>  
> >>  err_sysfs:
> >> -	drm_bridge_remove(bridge);
> >>  	hdmi->drm_connector = NULL;
> >>  	return -EINVAL;
> >>  }
> >> -- 
> >> 2.11.0
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Peter Rosin May 7, 2018, 1:59 p.m. UTC | #4
On 2018-05-07 15:39, Daniel Vetter wrote:
> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>> but there are several other bridges that never call drm_bridge_add.
>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>
>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>
>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>   drm driver to connect/disconnect a drm_bridge.
>>>
>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>   driver itself to register/unregister itself. Maybe we should rename
>>>   them, since the same issue happens with drm_panel, with the same
>>>   confusion.
>>>
>>> I thought someone was working on a cleanup series to fix this mess, but I
>>> didn't find anything.
>>
>> Ok, I just spotted the imbalance and didn't really dig into what
>> actually happens in these error paths. Now that I have done so I
>> believe that the removed drm_bridge_remove calls causes NULL
>> dereferences if/when the error paths are triggered.
>>
>> So, I don't think this can wait for some bigger cleanup.
>>
>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>> __list_del with NULL in both prev and next since the list member
>> is never initialized. prev and next are dereferenced by __list_del
>> and you have *boom*
>>
>> I recommend adding the tag
>>
>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>
>> so that stable picks this one up.
> 
> I just wanted to correct your commit message text - the correct solution
> is definitely _not_ for sti here to call drm_bridge_add.

Ah, I see what you mean. Do you want me to respin?

>                                                          It should call
> drm_bridge_attach/detach only, as a pair.

Alas, the attach/detach functions are generally not called from the same
level. After the bridge has been attached to an encoder, it is detached
in the generic code shutting down the encoder, i.e. the bridge consumer
is not explicitly involved with bridge detaching.

> I didn't check whether you instead have a _detach call missing or what's
> going on here.

So, even though there is no _detach call, it is still not "missing" as
it is not supposed to be there...

Cheers,
Peter

> -Daniel
>>
>> Cheers,
>> Peter
>>
>>> -Daniel
>>>
>>>> ---
>>>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>>>  2 files changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>>>> index 67bbdb49fffc..199db13f565c 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hda.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>>>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>>>>  	return 0;
>>>>  
>>>>  err_sysfs:
>>>> -	drm_bridge_remove(bridge);
>>>>  	return -EINVAL;
>>>>  }
>>>>  
>>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> index 58f431102512..932724784942 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>>>>  	return 0;
>>>>  
>>>>  err_sysfs:
>>>> -	drm_bridge_remove(bridge);
>>>>  	hdmi->drm_connector = NULL;
>>>>  	return -EINVAL;
>>>>  }
>>>> -- 
>>>> 2.11.0
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Peter Rosin May 7, 2018, 2:24 p.m. UTC | #5
On 2018-05-07 15:59, Peter Rosin wrote:
> On 2018-05-07 15:39, Daniel Vetter wrote:
>> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>>> but there are several other bridges that never call drm_bridge_add.
>>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>>
>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>
>>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>>
>>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>>   drm driver to connect/disconnect a drm_bridge.
>>>>
>>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>>   driver itself to register/unregister itself. Maybe we should rename
>>>>   them, since the same issue happens with drm_panel, with the same
>>>>   confusion.
>>>>
>>>> I thought someone was working on a cleanup series to fix this mess, but I
>>>> didn't find anything.
>>>
>>> Ok, I just spotted the imbalance and didn't really dig into what
>>> actually happens in these error paths. Now that I have done so I
>>> believe that the removed drm_bridge_remove calls causes NULL
>>> dereferences if/when the error paths are triggered.
>>>
>>> So, I don't think this can wait for some bigger cleanup.
>>>
>>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>>> __list_del with NULL in both prev and next since the list member
>>> is never initialized. prev and next are dereferenced by __list_del
>>> and you have *boom*
>>>
>>> I recommend adding the tag
>>>
>>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>>
>>> so that stable picks this one up.
>>
>> I just wanted to correct your commit message text - the correct solution
>> is definitely _not_ for sti here to call drm_bridge_add.
> 
> Ah, I see what you mean. Do you want me to respin?

Hold on, no I don't agree. sti_hda.c does create a bridge for it's own
internal use. It does not drm_bridge_add it, because all that ever does
is adding the bridge to the global lost of bridges. But since this is
a bridge for internal use, there is little point in calling drm_bridge_add,
the driver currently gains nothing by doing so.

But, drm_bridge_add might be a good place to put common stuff for every
bridge in the system, so it might be worthwhile to start requiring all
bridges to be drm_bridge_add-ed. And IMHO, it would not be wrong to have
the sti-hda driver call drm_bridge_add on the bridge it creates.

Do you really think it is actively wrong to call drm_bridge_add for
internal bridges such as this?

Cheers,
Peter
Daniel Vetter May 8, 2018, 7:41 a.m. UTC | #6
On Mon, May 07, 2018 at 03:59:04PM +0200, Peter Rosin wrote:
> On 2018-05-07 15:39, Daniel Vetter wrote:
> > On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> >> On 2018-05-03 11:06, Daniel Vetter wrote:
> >>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >>>> The more natural approach would perhaps be to add an drm_bridge_add,
> >>>> but there are several other bridges that never call drm_bridge_add.
> >>>> Just removing the drm_bridge_remove is the easier fix.
> >>>>
> >>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>
> >>> This mess is much bigger. There's 2 pairs of bridge functions:
> >>>
> >>> - drm_bridge_attach/detach. Those are meant to be called by the overall
> >>>   drm driver to connect/disconnect a drm_bridge.
> >>>
> >>> - drm_bridge_add/remove. These are supposed to be called by the bridge
> >>>   driver itself to register/unregister itself. Maybe we should rename
> >>>   them, since the same issue happens with drm_panel, with the same
> >>>   confusion.
> >>>
> >>> I thought someone was working on a cleanup series to fix this mess, but I
> >>> didn't find anything.
> >>
> >> Ok, I just spotted the imbalance and didn't really dig into what
> >> actually happens in these error paths. Now that I have done so I
> >> believe that the removed drm_bridge_remove calls causes NULL
> >> dereferences if/when the error paths are triggered.
> >>
> >> So, I don't think this can wait for some bigger cleanup.
> >>
> >> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> >> __list_del with NULL in both prev and next since the list member
> >> is never initialized. prev and next are dereferenced by __list_del
> >> and you have *boom*
> >>
> >> I recommend adding the tag
> >>
> >> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> >>
> >> so that stable picks this one up.
> > 
> > I just wanted to correct your commit message text - the correct solution
> > is definitely _not_ for sti here to call drm_bridge_add.
> 
> Ah, I see what you mean. Do you want me to respin?
> 
> >                                                          It should call
> > drm_bridge_attach/detach only, as a pair.
> 
> Alas, the attach/detach functions are generally not called from the same
> level. After the bridge has been attached to an encoder, it is detached
> in the generic code shutting down the encoder, i.e. the bridge consumer
> is not explicitly involved with bridge detaching.
> 
> > I didn't check whether you instead have a _detach call missing or what's
> > going on here.
> 
> So, even though there is no _detach call, it is still not "missing" as
> it is not supposed to be there...

Oh, TIL. Totally missed that we've improved this to be closer to dwim()
semantics. I think your patch is correct as-is and has my

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

It'd be great to improve the kerneldoc for drm_bridge_attach though to
mention that bridges get auto-detached on encoder cleanup as don in
drm_encoder_cleanup(). Care to do that?

And on that note I've again realized that most drivers totally get this
wrong when they set their ->destroy callback to drm_encoder_cleanup
(similar for other kms objects), because that one does _not_ do the final
kfree. Oh well.
-Daniel

> 
> Cheers,
> Peter
> 
> > -Daniel
> >>
> >> Cheers,
> >> Peter
> >>
> >>> -Daniel
> >>>
> >>>> ---
> >>>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
> >>>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
> >>>>  2 files changed, 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> >>>> index 67bbdb49fffc..199db13f565c 100644
> >>>> --- a/drivers/gpu/drm/sti/sti_hda.c
> >>>> +++ b/drivers/gpu/drm/sti/sti_hda.c
> >>>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
> >>>>  	return 0;
> >>>>  
> >>>>  err_sysfs:
> >>>> -	drm_bridge_remove(bridge);
> >>>>  	return -EINVAL;
> >>>>  }
> >>>>  
> >>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> index 58f431102512..932724784942 100644
> >>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
> >>>>  	return 0;
> >>>>  
> >>>>  err_sysfs:
> >>>> -	drm_bridge_remove(bridge);
> >>>>  	hdmi->drm_connector = NULL;
> >>>>  	return -EINVAL;
> >>>>  }
> >>>> -- 
> >>>> 2.11.0
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter May 8, 2018, 7:51 a.m. UTC | #7
On Mon, May 07, 2018 at 04:24:43PM +0200, Peter Rosin wrote:
> On 2018-05-07 15:59, Peter Rosin wrote:
> > On 2018-05-07 15:39, Daniel Vetter wrote:
> >> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> >>> On 2018-05-03 11:06, Daniel Vetter wrote:
> >>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >>>>> The more natural approach would perhaps be to add an drm_bridge_add,
> >>>>> but there are several other bridges that never call drm_bridge_add.
> >>>>> Just removing the drm_bridge_remove is the easier fix.
> >>>>>
> >>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>>
> >>>> This mess is much bigger. There's 2 pairs of bridge functions:
> >>>>
> >>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
> >>>>   drm driver to connect/disconnect a drm_bridge.
> >>>>
> >>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
> >>>>   driver itself to register/unregister itself. Maybe we should rename
> >>>>   them, since the same issue happens with drm_panel, with the same
> >>>>   confusion.
> >>>>
> >>>> I thought someone was working on a cleanup series to fix this mess, but I
> >>>> didn't find anything.
> >>>
> >>> Ok, I just spotted the imbalance and didn't really dig into what
> >>> actually happens in these error paths. Now that I have done so I
> >>> believe that the removed drm_bridge_remove calls causes NULL
> >>> dereferences if/when the error paths are triggered.
> >>>
> >>> So, I don't think this can wait for some bigger cleanup.
> >>>
> >>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> >>> __list_del with NULL in both prev and next since the list member
> >>> is never initialized. prev and next are dereferenced by __list_del
> >>> and you have *boom*
> >>>
> >>> I recommend adding the tag
> >>>
> >>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> >>>
> >>> so that stable picks this one up.
> >>
> >> I just wanted to correct your commit message text - the correct solution
> >> is definitely _not_ for sti here to call drm_bridge_add.
> > 
> > Ah, I see what you mean. Do you want me to respin?
> 
> Hold on, no I don't agree. sti_hda.c does create a bridge for it's own
> internal use. It does not drm_bridge_add it, because all that ever does
> is adding the bridge to the global lost of bridges. But since this is
> a bridge for internal use, there is little point in calling drm_bridge_add,
> the driver currently gains nothing by doing so.
> 
> But, drm_bridge_add might be a good place to put common stuff for every
> bridge in the system, so it might be worthwhile to start requiring all
> bridges to be drm_bridge_add-ed. And IMHO, it would not be wrong to have
> the sti-hda driver call drm_bridge_add on the bridge it creates.
> 
> Do you really think it is actively wrong to call drm_bridge_add for
> internal bridges such as this?

If we want to share bridge init code, then I think we need a
drm_bridge_init(). Not overload drm_bridge_add (which really should be
drm_bridge_register I think, but oh well, it's at least consistent with
drm_panel_add).
-Daniel

> 
> Cheers,
> Peter
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 67bbdb49fffc..199db13f565c 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -721,7 +721,6 @@  static int sti_hda_bind(struct device *dev, struct device *master, void *data)
 	return 0;
 
 err_sysfs:
-	drm_bridge_remove(bridge);
 	return -EINVAL;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 58f431102512..932724784942 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -1315,7 +1315,6 @@  static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
 	return 0;
 
 err_sysfs:
-	drm_bridge_remove(bridge);
 	hdmi->drm_connector = NULL;
 	return -EINVAL;
 }