Hi!
While looking at various drm bridge users, I came across these
issues.
Cheers,
Peter
Peter Rosin (3):
drm/sti: do not remove the drm_bridge that was never added
drm/rockchip: lvds: avoid duplicating drm_bridge_attach
drm/exynos: hdmi: avoid duplicating drm_bridge_attach
drivers/gpu/drm/exynos/exynos_hdmi.c | 2 --
drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
drivers/gpu/drm/sti/sti_hda.c | 1 -
drivers/gpu/drm/sti/sti_hdmi.c | 1 -
4 files changed, 6 deletions(-)
--
2.11.0
drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/gpu/drm/exynos/exynos_hdmi.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index abd84cbcf1c2..09c4bc0b1859 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -954,8 +954,6 @@ static int hdmi_create_connector(struct drm_encoder *encoder)
drm_mode_connector_attach_encoder(connector, encoder);
if (hdata->bridge) {
- encoder->bridge = hdata->bridge;
- hdata->bridge->encoder = encoder;
ret = drm_bridge_attach(encoder, hdata->bridge, NULL);
if (ret)
DRM_ERROR("Failed to attach bridge\n");
--
2.11.0
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 <[email protected]>
---
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
drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index e67f4ea28c0e..4bd94b167d2c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -446,14 +446,12 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
goto err_free_connector;
}
} else {
- lvds->bridge->encoder = encoder;
ret = drm_bridge_attach(encoder, lvds->bridge, NULL);
if (ret) {
DRM_DEV_ERROR(drm_dev->dev,
"failed to attach bridge: %d\n", ret);
goto err_free_encoder;
}
- encoder->bridge = lvds->bridge;
}
pm_runtime_enable(dev);
--
2.11.0
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 <[email protected]>
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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
>
> 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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
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 <[email protected]>
> >
> > 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
> >> [email protected]
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
>>>
>>> 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
>>>> [email protected]
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
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 <[email protected]>
>>>>
>>>> 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
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 <[email protected]>
> >>>
> >>> 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 <[email protected]>
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
> >>>> [email protected]
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> [email protected]
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
> >>>>
> >>>> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Am Mittwoch, 2. Mai 2018, 09:40:24 CEST schrieb Peter Rosin:
> drm_bridge_attach takes care of these assignments, so there is no need
> to open-code them a second time.
>
> Signed-off-by: Peter Rosin <[email protected]>
applied to drm-misc-next
Thanks
Heiko