[PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Jyri Sarha]

On 25/04/18 02:25, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 09:25:44PM +0300, Jyri Sarha wrote:
>> On 24/04/18 20:06, Russell King - ARM Linux wrote:
>>> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
 On 24/04/18 13:14, Peter Rosin wrote:
> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
 On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>  static int tda998x_remove(struct i2c_client *client)
>  {
> - component_del(>dev, _ops);
> + struct device *dev = >dev;
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> + drm_bridge_remove(>bridge);
> + component_del(dev, _ops);
> +

 I'd like to ask a rather fundamental question about DRM bridge support,
 because I suspect that there's a major fsckup here.

 The above is the function that deals with the TDA998x device being
 unbound from the driver.  With the component API, this results in the
 DRM device correctly being torn down, because one of the hardware
 devices has gone.

 With DRM bridge, the bridge is merely removed from the list of
 bridges:

 void drm_bridge_remove(struct drm_bridge *bridge)
 {
 mutex_lock(_lock);
 list_del_init(>list);
 mutex_unlock(_lock);
 }
 EXPORT_SYMBOL(drm_bridge_remove);

 and the memory backing the "struct tda998x_bridge" (which contains
 the struct drm_bridge) will be freed by the devm subsystem.

 However, there is no notification into the rest of the DRM subsystem
 that the device has gone away.  Worse, the memory that is still in
 use by DRM has now been freed, so further use of the DRM device
 results in a use-after-free bug.

 This is really not good, and to me looks like a fundamental problem
 with the DRM bridge code.  I see nothing in the DRM bridge code that
 deals with the lifetime of a "DRM bridge" or indeed the lifetime of
 the actual device itself.

 So, from what I can see, there seems to be a fundamental lifetime
 issue with the design of the DRM bridge code.  This needs to be
 fixed.
>>>
>>> Oh crap. A gigantic can of worms...
>>
>> Yes, it's especially annoying for me, having put the effort in to
>> the component helper to cover all these cases.
>>
>>> Would a patch (completely untested btw) along this line of thinking make
>>> any difference whatsoever?
>>
>> It looks interesting - from what I can see of the device links code,
>> it would have the effect of unbinding the DRM device just before
>> TDA998x is unbound, so that's an improvement.
>>
>> However, from what I can see, the link vanishes at that point (as
>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>> in nothing further happening - the link will be recreated, but there
>> appears to be nothing that triggers the "consumer" to rebind at that
>> point.  Maybe I've missed something?
>
> Right, auto-remove is a no-go. So, improving on the previous...
>
> (I think drm_panel might suffer from this issue too?)

 Yes it does and I took a shot at trying to fix it at the end of the
 previous merge window, but gave up as I run out of time. I re-spun the
 work now after reading this thread. I add you and Russell to cc.
>>>
>>> Right, and these exact problems are what the component helper is
>>> there to sort out, in a subsystem independent way.
>>>
>>> What is the problem with the component helper that people seem to
>>> be soo loathed to use it, instead preferring to come up with sub-
>>> standard and broken alternatives?
>>>
>>
>> Nothing but time. Embedding component helpers seamlessly into drm
>> framework does not sound like a couple of days job. Right now I simply
>> do not have time to take on a challenge like that. If someone does it I
>> am all for it.
>>
>> However, I would not call device links substandard. They are in the
>> device core after all.
> 
> Umm, no, I was not talking about the device links, but the tendency to
> have subsystem or component specific solutions to this problem.
> 

Oh yes. But in this case the substandard solution is already there and
it is already widely used, despite it being severely broken. I am merely
trying to fix the existing substandard solution.

I admit that a full integration with component helpers would probably be
more elegant solution to the problem, but the amount of work is just too
much. The change would impact the way all the master drm drivers pull
them selves together. The drivers 

[PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Jyri Sarha]

On 25/04/18 02:25, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 09:25:44PM +0300, Jyri Sarha wrote:
>> On 24/04/18 20:06, Russell King - ARM Linux wrote:
>>> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
 On 24/04/18 13:14, Peter Rosin wrote:
> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
 On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>  static int tda998x_remove(struct i2c_client *client)
>  {
> - component_del(>dev, _ops);
> + struct device *dev = >dev;
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> + drm_bridge_remove(>bridge);
> + component_del(dev, _ops);
> +

 I'd like to ask a rather fundamental question about DRM bridge support,
 because I suspect that there's a major fsckup here.

 The above is the function that deals with the TDA998x device being
 unbound from the driver.  With the component API, this results in the
 DRM device correctly being torn down, because one of the hardware
 devices has gone.

 With DRM bridge, the bridge is merely removed from the list of
 bridges:

 void drm_bridge_remove(struct drm_bridge *bridge)
 {
 mutex_lock(_lock);
 list_del_init(>list);
 mutex_unlock(_lock);
 }
 EXPORT_SYMBOL(drm_bridge_remove);

 and the memory backing the "struct tda998x_bridge" (which contains
 the struct drm_bridge) will be freed by the devm subsystem.

 However, there is no notification into the rest of the DRM subsystem
 that the device has gone away.  Worse, the memory that is still in
 use by DRM has now been freed, so further use of the DRM device
 results in a use-after-free bug.

 This is really not good, and to me looks like a fundamental problem
 with the DRM bridge code.  I see nothing in the DRM bridge code that
 deals with the lifetime of a "DRM bridge" or indeed the lifetime of
 the actual device itself.

 So, from what I can see, there seems to be a fundamental lifetime
 issue with the design of the DRM bridge code.  This needs to be
 fixed.
>>>
>>> Oh crap. A gigantic can of worms...
>>
>> Yes, it's especially annoying for me, having put the effort in to
>> the component helper to cover all these cases.
>>
>>> Would a patch (completely untested btw) along this line of thinking make
>>> any difference whatsoever?
>>
>> It looks interesting - from what I can see of the device links code,
>> it would have the effect of unbinding the DRM device just before
>> TDA998x is unbound, so that's an improvement.
>>
>> However, from what I can see, the link vanishes at that point (as
>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>> in nothing further happening - the link will be recreated, but there
>> appears to be nothing that triggers the "consumer" to rebind at that
>> point.  Maybe I've missed something?
>
> Right, auto-remove is a no-go. So, improving on the previous...
>
> (I think drm_panel might suffer from this issue too?)

 Yes it does and I took a shot at trying to fix it at the end of the
 previous merge window, but gave up as I run out of time. I re-spun the
 work now after reading this thread. I add you and Russell to cc.
>>>
>>> Right, and these exact problems are what the component helper is
>>> there to sort out, in a subsystem independent way.
>>>
>>> What is the problem with the component helper that people seem to
>>> be soo loathed to use it, instead preferring to come up with sub-
>>> standard and broken alternatives?
>>>
>>
>> Nothing but time. Embedding component helpers seamlessly into drm
>> framework does not sound like a couple of days job. Right now I simply
>> do not have time to take on a challenge like that. If someone does it I
>> am all for it.
>>
>> However, I would not call device links substandard. They are in the
>> device core after all.
> 
> Umm, no, I was not talking about the device links, but the tendency to
> have subsystem or component specific solutions to this problem.
> 

Oh yes. But in this case the substandard solution is already there and
it is already widely used, despite it being severely broken. I am merely
trying to fix the existing substandard solution.

I admit that a full integration with component helpers would probably be
more elegant solution to the problem, but the amount of work is just too
much. The change would impact the way all the master drm drivers pull
them selves together. The drivers 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-24 19:06, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
>> On 24/04/18 13:14, Peter Rosin wrote:
>>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
 On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>  static int tda998x_remove(struct i2c_client *client)
>>>  {
>>> -   component_del(>dev, _ops);
>>> +   struct device *dev = >dev;
>>> +   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>> +
>>> +   drm_bridge_remove(>bridge);
>>> +   component_del(dev, _ops);
>>> +
>>
>> I'd like to ask a rather fundamental question about DRM bridge support,
>> because I suspect that there's a major fsckup here.
>>
>> The above is the function that deals with the TDA998x device being
>> unbound from the driver.  With the component API, this results in the
>> DRM device correctly being torn down, because one of the hardware
>> devices has gone.
>>
>> With DRM bridge, the bridge is merely removed from the list of
>> bridges:
>>
>> void drm_bridge_remove(struct drm_bridge *bridge)
>> {
>> mutex_lock(_lock);
>> list_del_init(>list);
>> mutex_unlock(_lock);
>> }
>> EXPORT_SYMBOL(drm_bridge_remove);
>>
>> and the memory backing the "struct tda998x_bridge" (which contains
>> the struct drm_bridge) will be freed by the devm subsystem.
>>
>> However, there is no notification into the rest of the DRM subsystem
>> that the device has gone away.  Worse, the memory that is still in
>> use by DRM has now been freed, so further use of the DRM device
>> results in a use-after-free bug.
>>
>> This is really not good, and to me looks like a fundamental problem
>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>> the actual device itself.
>>
>> So, from what I can see, there seems to be a fundamental lifetime
>> issue with the design of the DRM bridge code.  This needs to be
>> fixed.
>
> Oh crap. A gigantic can of worms...

 Yes, it's especially annoying for me, having put the effort in to
 the component helper to cover all these cases.

> Would a patch (completely untested btw) along this line of thinking make
> any difference whatsoever?

 It looks interesting - from what I can see of the device links code,
 it would have the effect of unbinding the DRM device just before
 TDA998x is unbound, so that's an improvement.

 However, from what I can see, the link vanishes at that point (as
 DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
 in nothing further happening - the link will be recreated, but there
 appears to be nothing that triggers the "consumer" to rebind at that
 point.  Maybe I've missed something?
>>>
>>> Right, auto-remove is a no-go. So, improving on the previous...
>>>
>>> (I think drm_panel might suffer from this issue too?)
>>
>> Yes it does and I took a shot at trying to fix it at the end of the
>> previous merge window, but gave up as I run out of time. I re-spun the
>> work now after reading this thread. I add you and Russell to cc.
> 
> Right, and these exact problems are what the component helper is
> there to sort out, in a subsystem independent way.
> 
> What is the problem with the component helper that people seem to
> be soo loathed to use it, instead preferring to come up with sub-
> standard and broken alternatives?

I think the answer to that is rather obvious. If you design with these
components from the get-go, I see no problem with them, but it simply
seems way easier to retrofit device-links. Just take a look at my
untested patch and patch v3 2/2 from Jyri [1] for panels (that presumably
fix the big issue, namely leaving wild pointers). They either don't touch
neither suppliers nor consumers or are totally trivial (assigning a new
.owner field in suppliers). Compare that with adding a couple of dozen
boilerplate lines with hook functions etc to each and every drm_device,
drm_panel and drm_bridge.

Couple that with the fact that apparently the problem of unbinding
and leaving wild pointers hasn't been all that prevalent, implying that
the problem of rebinding can't be all that critical either.

But what do I know?

Cheers,
Peter

[1] I can't seem to find it in archives, so I'm including it here for
reference. It's small enough.

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 29d2c74..7474045 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -101,6 +102,13 @@ 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-24 19:06, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
>> On 24/04/18 13:14, Peter Rosin wrote:
>>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
 On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>  static int tda998x_remove(struct i2c_client *client)
>>>  {
>>> -   component_del(>dev, _ops);
>>> +   struct device *dev = >dev;
>>> +   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>> +
>>> +   drm_bridge_remove(>bridge);
>>> +   component_del(dev, _ops);
>>> +
>>
>> I'd like to ask a rather fundamental question about DRM bridge support,
>> because I suspect that there's a major fsckup here.
>>
>> The above is the function that deals with the TDA998x device being
>> unbound from the driver.  With the component API, this results in the
>> DRM device correctly being torn down, because one of the hardware
>> devices has gone.
>>
>> With DRM bridge, the bridge is merely removed from the list of
>> bridges:
>>
>> void drm_bridge_remove(struct drm_bridge *bridge)
>> {
>> mutex_lock(_lock);
>> list_del_init(>list);
>> mutex_unlock(_lock);
>> }
>> EXPORT_SYMBOL(drm_bridge_remove);
>>
>> and the memory backing the "struct tda998x_bridge" (which contains
>> the struct drm_bridge) will be freed by the devm subsystem.
>>
>> However, there is no notification into the rest of the DRM subsystem
>> that the device has gone away.  Worse, the memory that is still in
>> use by DRM has now been freed, so further use of the DRM device
>> results in a use-after-free bug.
>>
>> This is really not good, and to me looks like a fundamental problem
>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>> the actual device itself.
>>
>> So, from what I can see, there seems to be a fundamental lifetime
>> issue with the design of the DRM bridge code.  This needs to be
>> fixed.
>
> Oh crap. A gigantic can of worms...

 Yes, it's especially annoying for me, having put the effort in to
 the component helper to cover all these cases.

> Would a patch (completely untested btw) along this line of thinking make
> any difference whatsoever?

 It looks interesting - from what I can see of the device links code,
 it would have the effect of unbinding the DRM device just before
 TDA998x is unbound, so that's an improvement.

 However, from what I can see, the link vanishes at that point (as
 DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
 in nothing further happening - the link will be recreated, but there
 appears to be nothing that triggers the "consumer" to rebind at that
 point.  Maybe I've missed something?
>>>
>>> Right, auto-remove is a no-go. So, improving on the previous...
>>>
>>> (I think drm_panel might suffer from this issue too?)
>>
>> Yes it does and I took a shot at trying to fix it at the end of the
>> previous merge window, but gave up as I run out of time. I re-spun the
>> work now after reading this thread. I add you and Russell to cc.
> 
> Right, and these exact problems are what the component helper is
> there to sort out, in a subsystem independent way.
> 
> What is the problem with the component helper that people seem to
> be soo loathed to use it, instead preferring to come up with sub-
> standard and broken alternatives?

I think the answer to that is rather obvious. If you design with these
components from the get-go, I see no problem with them, but it simply
seems way easier to retrofit device-links. Just take a look at my
untested patch and patch v3 2/2 from Jyri [1] for panels (that presumably
fix the big issue, namely leaving wild pointers). They either don't touch
neither suppliers nor consumers or are totally trivial (assigning a new
.owner field in suppliers). Compare that with adding a couple of dozen
boilerplate lines with hook functions etc to each and every drm_device,
drm_panel and drm_bridge.

Couple that with the fact that apparently the problem of unbinding
and leaving wild pointers hasn't been all that prevalent, implying that
the problem of rebinding can't be all that critical either.

But what do I know?

Cheers,
Peter

[1] I can't seem to find it in archives, so I'm including it here for
reference. It's small enough.

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 29d2c74..7474045 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -101,6 +102,13 @@ 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 09:25:44PM +0300, Jyri Sarha wrote:
> On 24/04/18 20:06, Russell King - ARM Linux wrote:
> > On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
> >> On 24/04/18 13:14, Peter Rosin wrote:
> >>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>  On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> > On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> >> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >>>  static int tda998x_remove(struct i2c_client *client)
> >>>  {
> >>> - component_del(>dev, _ops);
> >>> + struct device *dev = >dev;
> >>> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> >>> +
> >>> + drm_bridge_remove(>bridge);
> >>> + component_del(dev, _ops);
> >>> +
> >>
> >> I'd like to ask a rather fundamental question about DRM bridge support,
> >> because I suspect that there's a major fsckup here.
> >>
> >> The above is the function that deals with the TDA998x device being
> >> unbound from the driver.  With the component API, this results in the
> >> DRM device correctly being torn down, because one of the hardware
> >> devices has gone.
> >>
> >> With DRM bridge, the bridge is merely removed from the list of
> >> bridges:
> >>
> >> void drm_bridge_remove(struct drm_bridge *bridge)
> >> {
> >> mutex_lock(_lock);
> >> list_del_init(>list);
> >> mutex_unlock(_lock);
> >> }
> >> EXPORT_SYMBOL(drm_bridge_remove);
> >>
> >> and the memory backing the "struct tda998x_bridge" (which contains
> >> the struct drm_bridge) will be freed by the devm subsystem.
> >>
> >> However, there is no notification into the rest of the DRM subsystem
> >> that the device has gone away.  Worse, the memory that is still in
> >> use by DRM has now been freed, so further use of the DRM device
> >> results in a use-after-free bug.
> >>
> >> This is really not good, and to me looks like a fundamental problem
> >> with the DRM bridge code.  I see nothing in the DRM bridge code that
> >> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> >> the actual device itself.
> >>
> >> So, from what I can see, there seems to be a fundamental lifetime
> >> issue with the design of the DRM bridge code.  This needs to be
> >> fixed.
> >
> > Oh crap. A gigantic can of worms...
> 
>  Yes, it's especially annoying for me, having put the effort in to
>  the component helper to cover all these cases.
> 
> > Would a patch (completely untested btw) along this line of thinking make
> > any difference whatsoever?
> 
>  It looks interesting - from what I can see of the device links code,
>  it would have the effect of unbinding the DRM device just before
>  TDA998x is unbound, so that's an improvement.
> 
>  However, from what I can see, the link vanishes at that point (as
>  DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>  in nothing further happening - the link will be recreated, but there
>  appears to be nothing that triggers the "consumer" to rebind at that
>  point.  Maybe I've missed something?
> >>>
> >>> Right, auto-remove is a no-go. So, improving on the previous...
> >>>
> >>> (I think drm_panel might suffer from this issue too?)
> >>
> >> Yes it does and I took a shot at trying to fix it at the end of the
> >> previous merge window, but gave up as I run out of time. I re-spun the
> >> work now after reading this thread. I add you and Russell to cc.
> > 
> > Right, and these exact problems are what the component helper is
> > there to sort out, in a subsystem independent way.
> > 
> > What is the problem with the component helper that people seem to
> > be soo loathed to use it, instead preferring to come up with sub-
> > standard and broken alternatives?
> > 
> 
> Nothing but time. Embedding component helpers seamlessly into drm
> framework does not sound like a couple of days job. Right now I simply
> do not have time to take on a challenge like that. If someone does it I
> am all for it.
> 
> However, I would not call device links substandard. They are in the
> device core after all.

Umm, no, I was not talking about the device links, but the tendency to
have subsystem or component specific solutions to this problem.

We're now heading down the path of trying to retrofit the functionality
that one expects and is provided by the component helpers into DRM
bridge and DRM panel by using device links, which appears to only
partially resolve the problem.

On the point of device links, I've been wondering whether the component
helpers could take advantage of the device links, but at the moment
that would cause a regression if there's no facility to re-probe the
"consumer" when a "supplier" returns.

As you bring that up, I 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 09:25:44PM +0300, Jyri Sarha wrote:
> On 24/04/18 20:06, Russell King - ARM Linux wrote:
> > On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
> >> On 24/04/18 13:14, Peter Rosin wrote:
> >>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>  On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> > On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> >> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >>>  static int tda998x_remove(struct i2c_client *client)
> >>>  {
> >>> - component_del(>dev, _ops);
> >>> + struct device *dev = >dev;
> >>> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> >>> +
> >>> + drm_bridge_remove(>bridge);
> >>> + component_del(dev, _ops);
> >>> +
> >>
> >> I'd like to ask a rather fundamental question about DRM bridge support,
> >> because I suspect that there's a major fsckup here.
> >>
> >> The above is the function that deals with the TDA998x device being
> >> unbound from the driver.  With the component API, this results in the
> >> DRM device correctly being torn down, because one of the hardware
> >> devices has gone.
> >>
> >> With DRM bridge, the bridge is merely removed from the list of
> >> bridges:
> >>
> >> void drm_bridge_remove(struct drm_bridge *bridge)
> >> {
> >> mutex_lock(_lock);
> >> list_del_init(>list);
> >> mutex_unlock(_lock);
> >> }
> >> EXPORT_SYMBOL(drm_bridge_remove);
> >>
> >> and the memory backing the "struct tda998x_bridge" (which contains
> >> the struct drm_bridge) will be freed by the devm subsystem.
> >>
> >> However, there is no notification into the rest of the DRM subsystem
> >> that the device has gone away.  Worse, the memory that is still in
> >> use by DRM has now been freed, so further use of the DRM device
> >> results in a use-after-free bug.
> >>
> >> This is really not good, and to me looks like a fundamental problem
> >> with the DRM bridge code.  I see nothing in the DRM bridge code that
> >> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> >> the actual device itself.
> >>
> >> So, from what I can see, there seems to be a fundamental lifetime
> >> issue with the design of the DRM bridge code.  This needs to be
> >> fixed.
> >
> > Oh crap. A gigantic can of worms...
> 
>  Yes, it's especially annoying for me, having put the effort in to
>  the component helper to cover all these cases.
> 
> > Would a patch (completely untested btw) along this line of thinking make
> > any difference whatsoever?
> 
>  It looks interesting - from what I can see of the device links code,
>  it would have the effect of unbinding the DRM device just before
>  TDA998x is unbound, so that's an improvement.
> 
>  However, from what I can see, the link vanishes at that point (as
>  DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>  in nothing further happening - the link will be recreated, but there
>  appears to be nothing that triggers the "consumer" to rebind at that
>  point.  Maybe I've missed something?
> >>>
> >>> Right, auto-remove is a no-go. So, improving on the previous...
> >>>
> >>> (I think drm_panel might suffer from this issue too?)
> >>
> >> Yes it does and I took a shot at trying to fix it at the end of the
> >> previous merge window, but gave up as I run out of time. I re-spun the
> >> work now after reading this thread. I add you and Russell to cc.
> > 
> > Right, and these exact problems are what the component helper is
> > there to sort out, in a subsystem independent way.
> > 
> > What is the problem with the component helper that people seem to
> > be soo loathed to use it, instead preferring to come up with sub-
> > standard and broken alternatives?
> > 
> 
> Nothing but time. Embedding component helpers seamlessly into drm
> framework does not sound like a couple of days job. Right now I simply
> do not have time to take on a challenge like that. If someone does it I
> am all for it.
> 
> However, I would not call device links substandard. They are in the
> device core after all.

Umm, no, I was not talking about the device links, but the tendency to
have subsystem or component specific solutions to this problem.

We're now heading down the path of trying to retrofit the functionality
that one expects and is provided by the component helpers into DRM
bridge and DRM panel by using device links, which appears to only
partially resolve the problem.

On the point of device links, I've been wondering whether the component
helpers could take advantage of the device links, but at the moment
that would cause a regression if there's no facility to re-probe the
"consumer" when a "supplier" returns.

As you bring that up, I 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Jyri Sarha]

On 24/04/18 20:06, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
>> On 24/04/18 13:14, Peter Rosin wrote:
>>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
 On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>  static int tda998x_remove(struct i2c_client *client)
>>>  {
>>> -   component_del(>dev, _ops);
>>> +   struct device *dev = >dev;
>>> +   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>> +
>>> +   drm_bridge_remove(>bridge);
>>> +   component_del(dev, _ops);
>>> +
>>
>> I'd like to ask a rather fundamental question about DRM bridge support,
>> because I suspect that there's a major fsckup here.
>>
>> The above is the function that deals with the TDA998x device being
>> unbound from the driver.  With the component API, this results in the
>> DRM device correctly being torn down, because one of the hardware
>> devices has gone.
>>
>> With DRM bridge, the bridge is merely removed from the list of
>> bridges:
>>
>> void drm_bridge_remove(struct drm_bridge *bridge)
>> {
>> mutex_lock(_lock);
>> list_del_init(>list);
>> mutex_unlock(_lock);
>> }
>> EXPORT_SYMBOL(drm_bridge_remove);
>>
>> and the memory backing the "struct tda998x_bridge" (which contains
>> the struct drm_bridge) will be freed by the devm subsystem.
>>
>> However, there is no notification into the rest of the DRM subsystem
>> that the device has gone away.  Worse, the memory that is still in
>> use by DRM has now been freed, so further use of the DRM device
>> results in a use-after-free bug.
>>
>> This is really not good, and to me looks like a fundamental problem
>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>> the actual device itself.
>>
>> So, from what I can see, there seems to be a fundamental lifetime
>> issue with the design of the DRM bridge code.  This needs to be
>> fixed.
>
> Oh crap. A gigantic can of worms...

 Yes, it's especially annoying for me, having put the effort in to
 the component helper to cover all these cases.

> Would a patch (completely untested btw) along this line of thinking make
> any difference whatsoever?

 It looks interesting - from what I can see of the device links code,
 it would have the effect of unbinding the DRM device just before
 TDA998x is unbound, so that's an improvement.

 However, from what I can see, the link vanishes at that point (as
 DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
 in nothing further happening - the link will be recreated, but there
 appears to be nothing that triggers the "consumer" to rebind at that
 point.  Maybe I've missed something?
>>>
>>> Right, auto-remove is a no-go. So, improving on the previous...
>>>
>>> (I think drm_panel might suffer from this issue too?)
>>
>> Yes it does and I took a shot at trying to fix it at the end of the
>> previous merge window, but gave up as I run out of time. I re-spun the
>> work now after reading this thread. I add you and Russell to cc.
> 
> Right, and these exact problems are what the component helper is
> there to sort out, in a subsystem independent way.
> 
> What is the problem with the component helper that people seem to
> be soo loathed to use it, instead preferring to come up with sub-
> standard and broken alternatives?
> 

Nothing but time. Embedding component helpers seamlessly into drm
framework does not sound like a couple of days job. Right now I simply
do not have time to take on a challenge like that. If someone does it I
am all for it.

However, I would not call device links substandard. They are in the
device core after all.

Best regards,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Jyri Sarha]

On 24/04/18 20:06, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
>> On 24/04/18 13:14, Peter Rosin wrote:
>>> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
 On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>>  static int tda998x_remove(struct i2c_client *client)
>>>  {
>>> -   component_del(>dev, _ops);
>>> +   struct device *dev = >dev;
>>> +   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>>> +
>>> +   drm_bridge_remove(>bridge);
>>> +   component_del(dev, _ops);
>>> +
>>
>> I'd like to ask a rather fundamental question about DRM bridge support,
>> because I suspect that there's a major fsckup here.
>>
>> The above is the function that deals with the TDA998x device being
>> unbound from the driver.  With the component API, this results in the
>> DRM device correctly being torn down, because one of the hardware
>> devices has gone.
>>
>> With DRM bridge, the bridge is merely removed from the list of
>> bridges:
>>
>> void drm_bridge_remove(struct drm_bridge *bridge)
>> {
>> mutex_lock(_lock);
>> list_del_init(>list);
>> mutex_unlock(_lock);
>> }
>> EXPORT_SYMBOL(drm_bridge_remove);
>>
>> and the memory backing the "struct tda998x_bridge" (which contains
>> the struct drm_bridge) will be freed by the devm subsystem.
>>
>> However, there is no notification into the rest of the DRM subsystem
>> that the device has gone away.  Worse, the memory that is still in
>> use by DRM has now been freed, so further use of the DRM device
>> results in a use-after-free bug.
>>
>> This is really not good, and to me looks like a fundamental problem
>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>> the actual device itself.
>>
>> So, from what I can see, there seems to be a fundamental lifetime
>> issue with the design of the DRM bridge code.  This needs to be
>> fixed.
>
> Oh crap. A gigantic can of worms...

 Yes, it's especially annoying for me, having put the effort in to
 the component helper to cover all these cases.

> Would a patch (completely untested btw) along this line of thinking make
> any difference whatsoever?

 It looks interesting - from what I can see of the device links code,
 it would have the effect of unbinding the DRM device just before
 TDA998x is unbound, so that's an improvement.

 However, from what I can see, the link vanishes at that point (as
 DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
 in nothing further happening - the link will be recreated, but there
 appears to be nothing that triggers the "consumer" to rebind at that
 point.  Maybe I've missed something?
>>>
>>> Right, auto-remove is a no-go. So, improving on the previous...
>>>
>>> (I think drm_panel might suffer from this issue too?)
>>
>> Yes it does and I took a shot at trying to fix it at the end of the
>> previous merge window, but gave up as I run out of time. I re-spun the
>> work now after reading this thread. I add you and Russell to cc.
> 
> Right, and these exact problems are what the component helper is
> there to sort out, in a subsystem independent way.
> 
> What is the problem with the component helper that people seem to
> be soo loathed to use it, instead preferring to come up with sub-
> standard and broken alternatives?
> 

Nothing but time. Embedding component helpers seamlessly into drm
framework does not sound like a couple of days job. Right now I simply
do not have time to take on a challenge like that. If someone does it I
am all for it.

However, I would not call device links substandard. They are in the
device core after all.

Best regards,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
> On 24/04/18 13:14, Peter Rosin wrote:
> > On 2018-04-24 10:08, Russell King - ARM Linux wrote:
> >> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> >>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>  On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >  static int tda998x_remove(struct i2c_client *client)
> >  {
> > -   component_del(>dev, _ops);
> > +   struct device *dev = >dev;
> > +   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> > +
> > +   drm_bridge_remove(>bridge);
> > +   component_del(dev, _ops);
> > +
> 
>  I'd like to ask a rather fundamental question about DRM bridge support,
>  because I suspect that there's a major fsckup here.
> 
>  The above is the function that deals with the TDA998x device being
>  unbound from the driver.  With the component API, this results in the
>  DRM device correctly being torn down, because one of the hardware
>  devices has gone.
> 
>  With DRM bridge, the bridge is merely removed from the list of
>  bridges:
> 
>  void drm_bridge_remove(struct drm_bridge *bridge)
>  {
>  mutex_lock(_lock);
>  list_del_init(>list);
>  mutex_unlock(_lock);
>  }
>  EXPORT_SYMBOL(drm_bridge_remove);
> 
>  and the memory backing the "struct tda998x_bridge" (which contains
>  the struct drm_bridge) will be freed by the devm subsystem.
> 
>  However, there is no notification into the rest of the DRM subsystem
>  that the device has gone away.  Worse, the memory that is still in
>  use by DRM has now been freed, so further use of the DRM device
>  results in a use-after-free bug.
> 
>  This is really not good, and to me looks like a fundamental problem
>  with the DRM bridge code.  I see nothing in the DRM bridge code that
>  deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>  the actual device itself.
> 
>  So, from what I can see, there seems to be a fundamental lifetime
>  issue with the design of the DRM bridge code.  This needs to be
>  fixed.
> >>>
> >>> Oh crap. A gigantic can of worms...
> >>
> >> Yes, it's especially annoying for me, having put the effort in to
> >> the component helper to cover all these cases.
> >>
> >>> Would a patch (completely untested btw) along this line of thinking make
> >>> any difference whatsoever?
> >>
> >> It looks interesting - from what I can see of the device links code,
> >> it would have the effect of unbinding the DRM device just before
> >> TDA998x is unbound, so that's an improvement.
> >>
> >> However, from what I can see, the link vanishes at that point (as
> >> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
> >> in nothing further happening - the link will be recreated, but there
> >> appears to be nothing that triggers the "consumer" to rebind at that
> >> point.  Maybe I've missed something?
> > 
> > Right, auto-remove is a no-go. So, improving on the previous...
> > 
> > (I think drm_panel might suffer from this issue too?)
> 
> Yes it does and I took a shot at trying to fix it at the end of the
> previous merge window, but gave up as I run out of time. I re-spun the
> work now after reading this thread. I add you and Russell to cc.

Right, and these exact problems are what the component helper is
there to sort out, in a subsystem independent way.

What is the problem with the component helper that people seem to
be soo loathed to use it, instead preferring to come up with sub-
standard and broken alternatives?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote:
> On 24/04/18 13:14, Peter Rosin wrote:
> > On 2018-04-24 10:08, Russell King - ARM Linux wrote:
> >> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> >>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>  On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >  static int tda998x_remove(struct i2c_client *client)
> >  {
> > -   component_del(>dev, _ops);
> > +   struct device *dev = >dev;
> > +   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> > +
> > +   drm_bridge_remove(>bridge);
> > +   component_del(dev, _ops);
> > +
> 
>  I'd like to ask a rather fundamental question about DRM bridge support,
>  because I suspect that there's a major fsckup here.
> 
>  The above is the function that deals with the TDA998x device being
>  unbound from the driver.  With the component API, this results in the
>  DRM device correctly being torn down, because one of the hardware
>  devices has gone.
> 
>  With DRM bridge, the bridge is merely removed from the list of
>  bridges:
> 
>  void drm_bridge_remove(struct drm_bridge *bridge)
>  {
>  mutex_lock(_lock);
>  list_del_init(>list);
>  mutex_unlock(_lock);
>  }
>  EXPORT_SYMBOL(drm_bridge_remove);
> 
>  and the memory backing the "struct tda998x_bridge" (which contains
>  the struct drm_bridge) will be freed by the devm subsystem.
> 
>  However, there is no notification into the rest of the DRM subsystem
>  that the device has gone away.  Worse, the memory that is still in
>  use by DRM has now been freed, so further use of the DRM device
>  results in a use-after-free bug.
> 
>  This is really not good, and to me looks like a fundamental problem
>  with the DRM bridge code.  I see nothing in the DRM bridge code that
>  deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>  the actual device itself.
> 
>  So, from what I can see, there seems to be a fundamental lifetime
>  issue with the design of the DRM bridge code.  This needs to be
>  fixed.
> >>>
> >>> Oh crap. A gigantic can of worms...
> >>
> >> Yes, it's especially annoying for me, having put the effort in to
> >> the component helper to cover all these cases.
> >>
> >>> Would a patch (completely untested btw) along this line of thinking make
> >>> any difference whatsoever?
> >>
> >> It looks interesting - from what I can see of the device links code,
> >> it would have the effect of unbinding the DRM device just before
> >> TDA998x is unbound, so that's an improvement.
> >>
> >> However, from what I can see, the link vanishes at that point (as
> >> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
> >> in nothing further happening - the link will be recreated, but there
> >> appears to be nothing that triggers the "consumer" to rebind at that
> >> point.  Maybe I've missed something?
> > 
> > Right, auto-remove is a no-go. So, improving on the previous...
> > 
> > (I think drm_panel might suffer from this issue too?)
> 
> Yes it does and I took a shot at trying to fix it at the end of the
> previous merge window, but gave up as I run out of time. I re-spun the
> work now after reading this thread. I add you and Russell to cc.

Right, and these exact problems are what the component helper is
there to sort out, in a subsystem independent way.

What is the problem with the component helper that people seem to
be soo loathed to use it, instead preferring to come up with sub-
standard and broken alternatives?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Jyri Sarha]

On 24/04/18 13:14, Peter Rosin wrote:
> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
 On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>  static int tda998x_remove(struct i2c_client *client)
>  {
> - component_del(>dev, _ops);
> + struct device *dev = >dev;
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> + drm_bridge_remove(>bridge);
> + component_del(dev, _ops);
> +

 I'd like to ask a rather fundamental question about DRM bridge support,
 because I suspect that there's a major fsckup here.

 The above is the function that deals with the TDA998x device being
 unbound from the driver.  With the component API, this results in the
 DRM device correctly being torn down, because one of the hardware
 devices has gone.

 With DRM bridge, the bridge is merely removed from the list of
 bridges:

 void drm_bridge_remove(struct drm_bridge *bridge)
 {
 mutex_lock(_lock);
 list_del_init(>list);
 mutex_unlock(_lock);
 }
 EXPORT_SYMBOL(drm_bridge_remove);

 and the memory backing the "struct tda998x_bridge" (which contains
 the struct drm_bridge) will be freed by the devm subsystem.

 However, there is no notification into the rest of the DRM subsystem
 that the device has gone away.  Worse, the memory that is still in
 use by DRM has now been freed, so further use of the DRM device
 results in a use-after-free bug.

 This is really not good, and to me looks like a fundamental problem
 with the DRM bridge code.  I see nothing in the DRM bridge code that
 deals with the lifetime of a "DRM bridge" or indeed the lifetime of
 the actual device itself.

 So, from what I can see, there seems to be a fundamental lifetime
 issue with the design of the DRM bridge code.  This needs to be
 fixed.
>>>
>>> Oh crap. A gigantic can of worms...
>>
>> Yes, it's especially annoying for me, having put the effort in to
>> the component helper to cover all these cases.
>>
>>> Would a patch (completely untested btw) along this line of thinking make
>>> any difference whatsoever?
>>
>> It looks interesting - from what I can see of the device links code,
>> it would have the effect of unbinding the DRM device just before
>> TDA998x is unbound, so that's an improvement.
>>
>> However, from what I can see, the link vanishes at that point (as
>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>> in nothing further happening - the link will be recreated, but there
>> appears to be nothing that triggers the "consumer" to rebind at that
>> point.  Maybe I've missed something?
> 
> Right, auto-remove is a no-go. So, improving on the previous...
> 
> (I think drm_panel might suffer from this issue too?)

Yes it does and I took a shot at trying to fix it at the end of the
previous merge window, but gave up as I run out of time. I re-spun the
work now after reading this thread. I add you and Russell to cc.

As far as I understand the re-binding problem can be solved with this patch:

https://lists.freedesktop.org/archives/dri-devel/2018-February/166907.html

The device_links are such a new concept that there is at least hope this
change won't have too unwanted side effects.

> > Cheers,
> Peter
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..b1365cfee445 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
> struct drm_bridge *bridge,
>   if (bridge->dev)
>   return -EBUSY;
>  
> + bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0);
> + if (!bridge->link)
> + return -EINVAL;
> +

At least this piece code should prepare for the bridge owner and the
master drm device being the same, I which case the link should not be
needed. This happens at least when a panel is attached using
drm_panel_bridge_add().

Best reagards,
Jyri

>   bridge->dev = encoder->dev;
>   bridge->encoder = encoder;
>  
>   if (bridge->funcs->attach) {
>   ret = bridge->funcs->attach(bridge);
>   if (ret < 0) {
> + device_link_del(bridge->link);
>   bridge->dev = NULL;
>   bridge->encoder = NULL;
>   return ret;
> @@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>   if (bridge->funcs->detach)
>   bridge->funcs->detach(bridge);
>  
> + device_link_del(bridge->link);
> +
>   bridge->dev = NULL;
>  }
>  
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index b8cb6237a38b..29eba4e9a39d 100644
> --- 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Jyri Sarha]

On 24/04/18 13:14, Peter Rosin wrote:
> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
 On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>  static int tda998x_remove(struct i2c_client *client)
>  {
> - component_del(>dev, _ops);
> + struct device *dev = >dev;
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> + drm_bridge_remove(>bridge);
> + component_del(dev, _ops);
> +

 I'd like to ask a rather fundamental question about DRM bridge support,
 because I suspect that there's a major fsckup here.

 The above is the function that deals with the TDA998x device being
 unbound from the driver.  With the component API, this results in the
 DRM device correctly being torn down, because one of the hardware
 devices has gone.

 With DRM bridge, the bridge is merely removed from the list of
 bridges:

 void drm_bridge_remove(struct drm_bridge *bridge)
 {
 mutex_lock(_lock);
 list_del_init(>list);
 mutex_unlock(_lock);
 }
 EXPORT_SYMBOL(drm_bridge_remove);

 and the memory backing the "struct tda998x_bridge" (which contains
 the struct drm_bridge) will be freed by the devm subsystem.

 However, there is no notification into the rest of the DRM subsystem
 that the device has gone away.  Worse, the memory that is still in
 use by DRM has now been freed, so further use of the DRM device
 results in a use-after-free bug.

 This is really not good, and to me looks like a fundamental problem
 with the DRM bridge code.  I see nothing in the DRM bridge code that
 deals with the lifetime of a "DRM bridge" or indeed the lifetime of
 the actual device itself.

 So, from what I can see, there seems to be a fundamental lifetime
 issue with the design of the DRM bridge code.  This needs to be
 fixed.
>>>
>>> Oh crap. A gigantic can of worms...
>>
>> Yes, it's especially annoying for me, having put the effort in to
>> the component helper to cover all these cases.
>>
>>> Would a patch (completely untested btw) along this line of thinking make
>>> any difference whatsoever?
>>
>> It looks interesting - from what I can see of the device links code,
>> it would have the effect of unbinding the DRM device just before
>> TDA998x is unbound, so that's an improvement.
>>
>> However, from what I can see, the link vanishes at that point (as
>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>> in nothing further happening - the link will be recreated, but there
>> appears to be nothing that triggers the "consumer" to rebind at that
>> point.  Maybe I've missed something?
> 
> Right, auto-remove is a no-go. So, improving on the previous...
> 
> (I think drm_panel might suffer from this issue too?)

Yes it does and I took a shot at trying to fix it at the end of the
previous merge window, but gave up as I run out of time. I re-spun the
work now after reading this thread. I add you and Russell to cc.

As far as I understand the re-binding problem can be solved with this patch:

https://lists.freedesktop.org/archives/dri-devel/2018-February/166907.html

The device_links are such a new concept that there is at least hope this
change won't have too unwanted side effects.

> > Cheers,
> Peter
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..b1365cfee445 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
> struct drm_bridge *bridge,
>   if (bridge->dev)
>   return -EBUSY;
>  
> + bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0);
> + if (!bridge->link)
> + return -EINVAL;
> +

At least this piece code should prepare for the bridge owner and the
master drm device being the same, I which case the link should not be
needed. This happens at least when a panel is attached using
drm_panel_bridge_add().

Best reagards,
Jyri

>   bridge->dev = encoder->dev;
>   bridge->encoder = encoder;
>  
>   if (bridge->funcs->attach) {
>   ret = bridge->funcs->attach(bridge);
>   if (ret < 0) {
> + device_link_del(bridge->link);
>   bridge->dev = NULL;
>   bridge->encoder = NULL;
>   return ret;
> @@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>   if (bridge->funcs->detach)
>   bridge->funcs->detach(bridge);
>  
> + device_link_del(bridge->link);
> +
>   bridge->dev = NULL;
>  }
>  
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index b8cb6237a38b..29eba4e9a39d 100644
> --- 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-24 12:14, Peter Rosin wrote:
> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
 On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>  static int tda998x_remove(struct i2c_client *client)
>  {
> - component_del(>dev, _ops);
> + struct device *dev = >dev;
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> + drm_bridge_remove(>bridge);
> + component_del(dev, _ops);
> +

 I'd like to ask a rather fundamental question about DRM bridge support,
 because I suspect that there's a major fsckup here.

 The above is the function that deals with the TDA998x device being
 unbound from the driver.  With the component API, this results in the
 DRM device correctly being torn down, because one of the hardware
 devices has gone.

 With DRM bridge, the bridge is merely removed from the list of
 bridges:

 void drm_bridge_remove(struct drm_bridge *bridge)
 {
 mutex_lock(_lock);
 list_del_init(>list);
 mutex_unlock(_lock);
 }
 EXPORT_SYMBOL(drm_bridge_remove);

 and the memory backing the "struct tda998x_bridge" (which contains
 the struct drm_bridge) will be freed by the devm subsystem.

 However, there is no notification into the rest of the DRM subsystem
 that the device has gone away.  Worse, the memory that is still in
 use by DRM has now been freed, so further use of the DRM device
 results in a use-after-free bug.

 This is really not good, and to me looks like a fundamental problem
 with the DRM bridge code.  I see nothing in the DRM bridge code that
 deals with the lifetime of a "DRM bridge" or indeed the lifetime of
 the actual device itself.

 So, from what I can see, there seems to be a fundamental lifetime
 issue with the design of the DRM bridge code.  This needs to be
 fixed.
>>>
>>> Oh crap. A gigantic can of worms...
>>
>> Yes, it's especially annoying for me, having put the effort in to
>> the component helper to cover all these cases.
>>
>>> Would a patch (completely untested btw) along this line of thinking make
>>> any difference whatsoever?
>>
>> It looks interesting - from what I can see of the device links code,
>> it would have the effect of unbinding the DRM device just before
>> TDA998x is unbound, so that's an improvement.
>>
>> However, from what I can see, the link vanishes at that point (as
>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>> in nothing further happening - the link will be recreated, but there
>> appears to be nothing that triggers the "consumer" to rebind at that
>> point.  Maybe I've missed something?
> 
> Right, auto-remove is a no-go. So, improving on the previous...

Heh, I didn't address the rebind triggering part at all, and while I'm by
no means responsible or have any deep knowledge, I thought this was true:

- driver .remove for the device owning the drm_bridge is what typically
  calls drm_bridge_remove()
- driver .remove is called as part of the device being unbound from the
  driver by the bus (i2c in this case)
- by registering a link to the consumer, this unbinding will trigger the
  removal of this main drm consumer device as part of the unbinding
- so everything aboput the drm device will be torn down, and everything
  will thus have to be reprobed to get things back

But I could easily have misunderstood just about everything in the above...

And maybe it's really inconvenient to have to trigger a reprobe of the
whole drm cluster? Maybe all drm driver parts should be components?

I have no idea.

Cheers,
Peter

PS. compile-tested the below and drm_bridge.c needs to
#include 

> (I think drm_panel might suffer from this issue too?)
> 
> Cheers,
> Peter
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..b1365cfee445 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
> struct drm_bridge *bridge,
>   if (bridge->dev)
>   return -EBUSY;
>  
> + bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0);
> + if (!bridge->link)
> + return -EINVAL;
> +
>   bridge->dev = encoder->dev;
>   bridge->encoder = encoder;
>  
>   if (bridge->funcs->attach) {
>   ret = bridge->funcs->attach(bridge);
>   if (ret < 0) {
> + device_link_del(bridge->link);
>   bridge->dev = NULL;
>   bridge->encoder = NULL;
>   return ret;
> @@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>   if (bridge->funcs->detach)
>   bridge->funcs->detach(bridge);
>  
> + 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-24 12:14, Peter Rosin wrote:
> On 2018-04-24 10:08, Russell King - ARM Linux wrote:
>> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
 On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>  static int tda998x_remove(struct i2c_client *client)
>  {
> - component_del(>dev, _ops);
> + struct device *dev = >dev;
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> + drm_bridge_remove(>bridge);
> + component_del(dev, _ops);
> +

 I'd like to ask a rather fundamental question about DRM bridge support,
 because I suspect that there's a major fsckup here.

 The above is the function that deals with the TDA998x device being
 unbound from the driver.  With the component API, this results in the
 DRM device correctly being torn down, because one of the hardware
 devices has gone.

 With DRM bridge, the bridge is merely removed from the list of
 bridges:

 void drm_bridge_remove(struct drm_bridge *bridge)
 {
 mutex_lock(_lock);
 list_del_init(>list);
 mutex_unlock(_lock);
 }
 EXPORT_SYMBOL(drm_bridge_remove);

 and the memory backing the "struct tda998x_bridge" (which contains
 the struct drm_bridge) will be freed by the devm subsystem.

 However, there is no notification into the rest of the DRM subsystem
 that the device has gone away.  Worse, the memory that is still in
 use by DRM has now been freed, so further use of the DRM device
 results in a use-after-free bug.

 This is really not good, and to me looks like a fundamental problem
 with the DRM bridge code.  I see nothing in the DRM bridge code that
 deals with the lifetime of a "DRM bridge" or indeed the lifetime of
 the actual device itself.

 So, from what I can see, there seems to be a fundamental lifetime
 issue with the design of the DRM bridge code.  This needs to be
 fixed.
>>>
>>> Oh crap. A gigantic can of worms...
>>
>> Yes, it's especially annoying for me, having put the effort in to
>> the component helper to cover all these cases.
>>
>>> Would a patch (completely untested btw) along this line of thinking make
>>> any difference whatsoever?
>>
>> It looks interesting - from what I can see of the device links code,
>> it would have the effect of unbinding the DRM device just before
>> TDA998x is unbound, so that's an improvement.
>>
>> However, from what I can see, the link vanishes at that point (as
>> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
>> in nothing further happening - the link will be recreated, but there
>> appears to be nothing that triggers the "consumer" to rebind at that
>> point.  Maybe I've missed something?
> 
> Right, auto-remove is a no-go. So, improving on the previous...

Heh, I didn't address the rebind triggering part at all, and while I'm by
no means responsible or have any deep knowledge, I thought this was true:

- driver .remove for the device owning the drm_bridge is what typically
  calls drm_bridge_remove()
- driver .remove is called as part of the device being unbound from the
  driver by the bus (i2c in this case)
- by registering a link to the consumer, this unbinding will trigger the
  removal of this main drm consumer device as part of the unbinding
- so everything aboput the drm device will be torn down, and everything
  will thus have to be reprobed to get things back

But I could easily have misunderstood just about everything in the above...

And maybe it's really inconvenient to have to trigger a reprobe of the
whole drm cluster? Maybe all drm driver parts should be components?

I have no idea.

Cheers,
Peter

PS. compile-tested the below and drm_bridge.c needs to
#include 

> (I think drm_panel might suffer from this issue too?)
> 
> Cheers,
> Peter
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..b1365cfee445 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
> struct drm_bridge *bridge,
>   if (bridge->dev)
>   return -EBUSY;
>  
> + bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0);
> + if (!bridge->link)
> + return -EINVAL;
> +
>   bridge->dev = encoder->dev;
>   bridge->encoder = encoder;
>  
>   if (bridge->funcs->attach) {
>   ret = bridge->funcs->attach(bridge);
>   if (ret < 0) {
> + device_link_del(bridge->link);
>   bridge->dev = NULL;
>   bridge->encoder = NULL;
>   return ret;
> @@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>   if (bridge->funcs->detach)
>   bridge->funcs->detach(bridge);
>  
> + 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-24 10:08, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
  static int tda998x_remove(struct i2c_client *client)
  {
 -  component_del(>dev, _ops);
 +  struct device *dev = >dev;
 +  struct tda998x_bridge *bridge = dev_get_drvdata(dev);
 +
 +  drm_bridge_remove(>bridge);
 +  component_del(dev, _ops);
 +
>>>
>>> I'd like to ask a rather fundamental question about DRM bridge support,
>>> because I suspect that there's a major fsckup here.
>>>
>>> The above is the function that deals with the TDA998x device being
>>> unbound from the driver.  With the component API, this results in the
>>> DRM device correctly being torn down, because one of the hardware
>>> devices has gone.
>>>
>>> With DRM bridge, the bridge is merely removed from the list of
>>> bridges:
>>>
>>> void drm_bridge_remove(struct drm_bridge *bridge)
>>> {
>>> mutex_lock(_lock);
>>> list_del_init(>list);
>>> mutex_unlock(_lock);
>>> }
>>> EXPORT_SYMBOL(drm_bridge_remove);
>>>
>>> and the memory backing the "struct tda998x_bridge" (which contains
>>> the struct drm_bridge) will be freed by the devm subsystem.
>>>
>>> However, there is no notification into the rest of the DRM subsystem
>>> that the device has gone away.  Worse, the memory that is still in
>>> use by DRM has now been freed, so further use of the DRM device
>>> results in a use-after-free bug.
>>>
>>> This is really not good, and to me looks like a fundamental problem
>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>>> the actual device itself.
>>>
>>> So, from what I can see, there seems to be a fundamental lifetime
>>> issue with the design of the DRM bridge code.  This needs to be
>>> fixed.
>>
>> Oh crap. A gigantic can of worms...
> 
> Yes, it's especially annoying for me, having put the effort in to
> the component helper to cover all these cases.
> 
>> Would a patch (completely untested btw) along this line of thinking make
>> any difference whatsoever?
> 
> It looks interesting - from what I can see of the device links code,
> it would have the effect of unbinding the DRM device just before
> TDA998x is unbound, so that's an improvement.
> 
> However, from what I can see, the link vanishes at that point (as
> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
> in nothing further happening - the link will be recreated, but there
> appears to be nothing that triggers the "consumer" to rebind at that
> point.  Maybe I've missed something?

Right, auto-remove is a no-go. So, improving on the previous...

(I think drm_panel might suffer from this issue too?)

Cheers,
Peter

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..b1365cfee445 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (bridge->dev)
return -EBUSY;
 
+   bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0);
+   if (!bridge->link)
+   return -EINVAL;
+
bridge->dev = encoder->dev;
bridge->encoder = encoder;
 
if (bridge->funcs->attach) {
ret = bridge->funcs->attach(bridge);
if (ret < 0) {
+   device_link_del(bridge->link);
bridge->dev = NULL;
bridge->encoder = NULL;
return ret;
@@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
if (bridge->funcs->detach)
bridge->funcs->detach(bridge);
 
+   device_link_del(bridge->link);
+
bridge->dev = NULL;
 }
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index b8cb6237a38b..29eba4e9a39d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1857,6 +1857,7 @@ tda998x_probe(struct i2c_client *client, const struct 
i2c_device_id *id)
bridge->dev = dev;
dev_set_drvdata(dev, bridge);
 
+   bridge->bridge.owner = dev;
bridge->bridge.funcs = _bridge_funcs;
 #ifdef CONFIG_OF
bridge->bridge.of_node = dev->of_node;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 682d01ba920c..b8f33aba3216 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -224,6 +224,8 @@ struct drm_bridge_funcs {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @owner: device that owns the bridge
+ * @link: drm consumer <-> bridge supplier
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-24 10:08, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
>> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
  static int tda998x_remove(struct i2c_client *client)
  {
 -  component_del(>dev, _ops);
 +  struct device *dev = >dev;
 +  struct tda998x_bridge *bridge = dev_get_drvdata(dev);
 +
 +  drm_bridge_remove(>bridge);
 +  component_del(dev, _ops);
 +
>>>
>>> I'd like to ask a rather fundamental question about DRM bridge support,
>>> because I suspect that there's a major fsckup here.
>>>
>>> The above is the function that deals with the TDA998x device being
>>> unbound from the driver.  With the component API, this results in the
>>> DRM device correctly being torn down, because one of the hardware
>>> devices has gone.
>>>
>>> With DRM bridge, the bridge is merely removed from the list of
>>> bridges:
>>>
>>> void drm_bridge_remove(struct drm_bridge *bridge)
>>> {
>>> mutex_lock(_lock);
>>> list_del_init(>list);
>>> mutex_unlock(_lock);
>>> }
>>> EXPORT_SYMBOL(drm_bridge_remove);
>>>
>>> and the memory backing the "struct tda998x_bridge" (which contains
>>> the struct drm_bridge) will be freed by the devm subsystem.
>>>
>>> However, there is no notification into the rest of the DRM subsystem
>>> that the device has gone away.  Worse, the memory that is still in
>>> use by DRM has now been freed, so further use of the DRM device
>>> results in a use-after-free bug.
>>>
>>> This is really not good, and to me looks like a fundamental problem
>>> with the DRM bridge code.  I see nothing in the DRM bridge code that
>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
>>> the actual device itself.
>>>
>>> So, from what I can see, there seems to be a fundamental lifetime
>>> issue with the design of the DRM bridge code.  This needs to be
>>> fixed.
>>
>> Oh crap. A gigantic can of worms...
> 
> Yes, it's especially annoying for me, having put the effort in to
> the component helper to cover all these cases.
> 
>> Would a patch (completely untested btw) along this line of thinking make
>> any difference whatsoever?
> 
> It looks interesting - from what I can see of the device links code,
> it would have the effect of unbinding the DRM device just before
> TDA998x is unbound, so that's an improvement.
> 
> However, from what I can see, the link vanishes at that point (as
> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
> in nothing further happening - the link will be recreated, but there
> appears to be nothing that triggers the "consumer" to rebind at that
> point.  Maybe I've missed something?

Right, auto-remove is a no-go. So, improving on the previous...

(I think drm_panel might suffer from this issue too?)

Cheers,
Peter

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..b1365cfee445 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (bridge->dev)
return -EBUSY;
 
+   bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0);
+   if (!bridge->link)
+   return -EINVAL;
+
bridge->dev = encoder->dev;
bridge->encoder = encoder;
 
if (bridge->funcs->attach) {
ret = bridge->funcs->attach(bridge);
if (ret < 0) {
+   device_link_del(bridge->link);
bridge->dev = NULL;
bridge->encoder = NULL;
return ret;
@@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
if (bridge->funcs->detach)
bridge->funcs->detach(bridge);
 
+   device_link_del(bridge->link);
+
bridge->dev = NULL;
 }
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index b8cb6237a38b..29eba4e9a39d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1857,6 +1857,7 @@ tda998x_probe(struct i2c_client *client, const struct 
i2c_device_id *id)
bridge->dev = dev;
dev_set_drvdata(dev, bridge);
 
+   bridge->bridge.owner = dev;
bridge->bridge.funcs = _bridge_funcs;
 #ifdef CONFIG_OF
bridge->bridge.of_node = dev->of_node;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 682d01ba920c..b8f33aba3216 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -224,6 +224,8 @@ struct drm_bridge_funcs {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @owner: device that owns the bridge
+ * @link: drm consumer <-> bridge supplier
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in 

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> > On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >>  static int tda998x_remove(struct i2c_client *client)
> >>  {
> >> -  component_del(>dev, _ops);
> >> +  struct device *dev = >dev;
> >> +  struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> >> +
> >> +  drm_bridge_remove(>bridge);
> >> +  component_del(dev, _ops);
> >> +
> > 
> > I'd like to ask a rather fundamental question about DRM bridge support,
> > because I suspect that there's a major fsckup here.
> > 
> > The above is the function that deals with the TDA998x device being
> > unbound from the driver.  With the component API, this results in the
> > DRM device correctly being torn down, because one of the hardware
> > devices has gone.
> > 
> > With DRM bridge, the bridge is merely removed from the list of
> > bridges:
> > 
> > void drm_bridge_remove(struct drm_bridge *bridge)
> > {
> > mutex_lock(_lock);
> > list_del_init(>list);
> > mutex_unlock(_lock);
> > }
> > EXPORT_SYMBOL(drm_bridge_remove);
> > 
> > and the memory backing the "struct tda998x_bridge" (which contains
> > the struct drm_bridge) will be freed by the devm subsystem.
> > 
> > However, there is no notification into the rest of the DRM subsystem
> > that the device has gone away.  Worse, the memory that is still in
> > use by DRM has now been freed, so further use of the DRM device
> > results in a use-after-free bug.
> > 
> > This is really not good, and to me looks like a fundamental problem
> > with the DRM bridge code.  I see nothing in the DRM bridge code that
> > deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> > the actual device itself.
> > 
> > So, from what I can see, there seems to be a fundamental lifetime
> > issue with the design of the DRM bridge code.  This needs to be
> > fixed.
> 
> Oh crap. A gigantic can of worms...

Yes, it's especially annoying for me, having put the effort in to
the component helper to cover all these cases.

> Would a patch (completely untested btw) along this line of thinking make
> any difference whatsoever?

It looks interesting - from what I can see of the device links code,
it would have the effect of unbinding the DRM device just before
TDA998x is unbound, so that's an improvement.

However, from what I can see, the link vanishes at that point (as
DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
in nothing further happening - the link will be recreated, but there
appears to be nothing that triggers the "consumer" to rebind at that
point.  Maybe I've missed something?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote:
> On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> > On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
> >>  static int tda998x_remove(struct i2c_client *client)
> >>  {
> >> -  component_del(>dev, _ops);
> >> +  struct device *dev = >dev;
> >> +  struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> >> +
> >> +  drm_bridge_remove(>bridge);
> >> +  component_del(dev, _ops);
> >> +
> > 
> > I'd like to ask a rather fundamental question about DRM bridge support,
> > because I suspect that there's a major fsckup here.
> > 
> > The above is the function that deals with the TDA998x device being
> > unbound from the driver.  With the component API, this results in the
> > DRM device correctly being torn down, because one of the hardware
> > devices has gone.
> > 
> > With DRM bridge, the bridge is merely removed from the list of
> > bridges:
> > 
> > void drm_bridge_remove(struct drm_bridge *bridge)
> > {
> > mutex_lock(_lock);
> > list_del_init(>list);
> > mutex_unlock(_lock);
> > }
> > EXPORT_SYMBOL(drm_bridge_remove);
> > 
> > and the memory backing the "struct tda998x_bridge" (which contains
> > the struct drm_bridge) will be freed by the devm subsystem.
> > 
> > However, there is no notification into the rest of the DRM subsystem
> > that the device has gone away.  Worse, the memory that is still in
> > use by DRM has now been freed, so further use of the DRM device
> > results in a use-after-free bug.
> > 
> > This is really not good, and to me looks like a fundamental problem
> > with the DRM bridge code.  I see nothing in the DRM bridge code that
> > deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> > the actual device itself.
> > 
> > So, from what I can see, there seems to be a fundamental lifetime
> > issue with the design of the DRM bridge code.  This needs to be
> > fixed.
> 
> Oh crap. A gigantic can of worms...

Yes, it's especially annoying for me, having put the effort in to
the component helper to cover all these cases.

> Would a patch (completely untested btw) along this line of thinking make
> any difference whatsoever?

It looks interesting - from what I can see of the device links code,
it would have the effect of unbinding the DRM device just before
TDA998x is unbound, so that's an improvement.

However, from what I can see, the link vanishes at that point (as
DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results
in nothing further happening - the link will be recreated, but there
appears to be nothing that triggers the "consumer" to rebind at that
point.  Maybe I've missed something?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>  static int tda998x_remove(struct i2c_client *client)
>>  {
>> -component_del(>dev, _ops);
>> +struct device *dev = >dev;
>> +struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>> +
>> +drm_bridge_remove(>bridge);
>> +component_del(dev, _ops);
>> +
> 
> I'd like to ask a rather fundamental question about DRM bridge support,
> because I suspect that there's a major fsckup here.
> 
> The above is the function that deals with the TDA998x device being
> unbound from the driver.  With the component API, this results in the
> DRM device correctly being torn down, because one of the hardware
> devices has gone.
> 
> With DRM bridge, the bridge is merely removed from the list of
> bridges:
> 
> void drm_bridge_remove(struct drm_bridge *bridge)
> {
> mutex_lock(_lock);
> list_del_init(>list);
> mutex_unlock(_lock);
> }
> EXPORT_SYMBOL(drm_bridge_remove);
> 
> and the memory backing the "struct tda998x_bridge" (which contains
> the struct drm_bridge) will be freed by the devm subsystem.
> 
> However, there is no notification into the rest of the DRM subsystem
> that the device has gone away.  Worse, the memory that is still in
> use by DRM has now been freed, so further use of the DRM device
> results in a use-after-free bug.
> 
> This is really not good, and to me looks like a fundamental problem
> with the DRM bridge code.  I see nothing in the DRM bridge code that
> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> the actual device itself.
> 
> So, from what I can see, there seems to be a fundamental lifetime
> issue with the design of the DRM bridge code.  This needs to be
> fixed.

Oh crap. A gigantic can of worms...

Would a patch (completely untested btw) along this line of thinking make
any difference whatsoever?

Yeah, I know, all other drm_bridges also need to fill in .owner, and
the .of_node member could probably be ditched from struct drm_device
etc, but this was just a quick sketch...

Cheers,
Peter

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..3577e50cc6c0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -138,6 +138,10 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
else
encoder->bridge = bridge;
 
+   if (!device_link_add(encoder->dev->dev, bridge->owner,
+DL_FLAG_AUTOREMOVE))
+   return -EINVAL;
+
return 0;
 }
 EXPORT_SYMBOL(drm_bridge_attach);
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index b8cb6237a38b..29eba4e9a39d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1857,6 +1857,7 @@ tda998x_probe(struct i2c_client *client, const struct 
i2c_device_id *id)
bridge->dev = dev;
dev_set_drvdata(dev, bridge);
 
+   bridge->bridge.owner = dev;
bridge->bridge.funcs = _bridge_funcs;
 #ifdef CONFIG_OF
bridge->bridge.of_node = dev->of_node;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 682d01ba920c..f0f8b2a85c28 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -224,6 +224,7 @@ struct drm_bridge_funcs {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @owner: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -233,6 +234,7 @@ struct drm_bridge_funcs {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+   struct device *owner;
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-23 18:08, Russell King - ARM Linux wrote:
> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>>  static int tda998x_remove(struct i2c_client *client)
>>  {
>> -component_del(>dev, _ops);
>> +struct device *dev = >dev;
>> +struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>> +
>> +drm_bridge_remove(>bridge);
>> +component_del(dev, _ops);
>> +
> 
> I'd like to ask a rather fundamental question about DRM bridge support,
> because I suspect that there's a major fsckup here.
> 
> The above is the function that deals with the TDA998x device being
> unbound from the driver.  With the component API, this results in the
> DRM device correctly being torn down, because one of the hardware
> devices has gone.
> 
> With DRM bridge, the bridge is merely removed from the list of
> bridges:
> 
> void drm_bridge_remove(struct drm_bridge *bridge)
> {
> mutex_lock(_lock);
> list_del_init(>list);
> mutex_unlock(_lock);
> }
> EXPORT_SYMBOL(drm_bridge_remove);
> 
> and the memory backing the "struct tda998x_bridge" (which contains
> the struct drm_bridge) will be freed by the devm subsystem.
> 
> However, there is no notification into the rest of the DRM subsystem
> that the device has gone away.  Worse, the memory that is still in
> use by DRM has now been freed, so further use of the DRM device
> results in a use-after-free bug.
> 
> This is really not good, and to me looks like a fundamental problem
> with the DRM bridge code.  I see nothing in the DRM bridge code that
> deals with the lifetime of a "DRM bridge" or indeed the lifetime of
> the actual device itself.
> 
> So, from what I can see, there seems to be a fundamental lifetime
> issue with the design of the DRM bridge code.  This needs to be
> fixed.

Oh crap. A gigantic can of worms...

Would a patch (completely untested btw) along this line of thinking make
any difference whatsoever?

Yeah, I know, all other drm_bridges also need to fill in .owner, and
the .of_node member could probably be ditched from struct drm_device
etc, but this was just a quick sketch...

Cheers,
Peter

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..3577e50cc6c0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -138,6 +138,10 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
else
encoder->bridge = bridge;
 
+   if (!device_link_add(encoder->dev->dev, bridge->owner,
+DL_FLAG_AUTOREMOVE))
+   return -EINVAL;
+
return 0;
 }
 EXPORT_SYMBOL(drm_bridge_attach);
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index b8cb6237a38b..29eba4e9a39d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1857,6 +1857,7 @@ tda998x_probe(struct i2c_client *client, const struct 
i2c_device_id *id)
bridge->dev = dev;
dev_set_drvdata(dev, bridge);
 
+   bridge->bridge.owner = dev;
bridge->bridge.funcs = _bridge_funcs;
 #ifdef CONFIG_OF
bridge->bridge.of_node = dev->of_node;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 682d01ba920c..f0f8b2a85c28 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -224,6 +224,7 @@ struct drm_bridge_funcs {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @owner: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -233,6 +234,7 @@ struct drm_bridge_funcs {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+   struct device *owner;
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>  static int tda998x_remove(struct i2c_client *client)
>  {
> - component_del(>dev, _ops);
> + struct device *dev = >dev;
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> + drm_bridge_remove(>bridge);
> + component_del(dev, _ops);
> +

I'd like to ask a rather fundamental question about DRM bridge support,
because I suspect that there's a major fsckup here.

The above is the function that deals with the TDA998x device being
unbound from the driver.  With the component API, this results in the
DRM device correctly being torn down, because one of the hardware
devices has gone.

With DRM bridge, the bridge is merely removed from the list of
bridges:

void drm_bridge_remove(struct drm_bridge *bridge)
{
mutex_lock(_lock);
list_del_init(>list);
mutex_unlock(_lock);
}
EXPORT_SYMBOL(drm_bridge_remove);

and the memory backing the "struct tda998x_bridge" (which contains
the struct drm_bridge) will be freed by the devm subsystem.

However, there is no notification into the rest of the DRM subsystem
that the device has gone away.  Worse, the memory that is still in
use by DRM has now been freed, so further use of the DRM device
results in a use-after-free bug.

This is really not good, and to me looks like a fundamental problem
with the DRM bridge code.  I see nothing in the DRM bridge code that
deals with the lifetime of a "DRM bridge" or indeed the lifetime of
the actual device itself.

So, from what I can see, there seems to be a fundamental lifetime
issue with the design of the DRM bridge code.  This needs to be
fixed.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote:
>  static int tda998x_remove(struct i2c_client *client)
>  {
> - component_del(>dev, _ops);
> + struct device *dev = >dev;
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> + drm_bridge_remove(>bridge);
> + component_del(dev, _ops);
> +

I'd like to ask a rather fundamental question about DRM bridge support,
because I suspect that there's a major fsckup here.

The above is the function that deals with the TDA998x device being
unbound from the driver.  With the component API, this results in the
DRM device correctly being torn down, because one of the hardware
devices has gone.

With DRM bridge, the bridge is merely removed from the list of
bridges:

void drm_bridge_remove(struct drm_bridge *bridge)
{
mutex_lock(_lock);
list_del_init(>list);
mutex_unlock(_lock);
}
EXPORT_SYMBOL(drm_bridge_remove);

and the memory backing the "struct tda998x_bridge" (which contains
the struct drm_bridge) will be freed by the devm subsystem.

However, there is no notification into the rest of the DRM subsystem
that the device has gone away.  Worse, the memory that is still in
use by DRM has now been freed, so further use of the DRM device
results in a use-after-free bug.

This is really not good, and to me looks like a fundamental problem
with the DRM bridge code.  I see nothing in the DRM bridge code that
deals with the lifetime of a "DRM bridge" or indeed the lifetime of
the actual device itself.

So, from what I can see, there seems to be a fundamental lifetime
issue with the design of the DRM bridge code.  This needs to be
fixed.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

This makes this driver work with all(?) drivers that are not
componentized and instead expect to connect to a panel/bridge. That
said, the only one tested is atmel-hlcdc.

This hooks the relevant work function previously called by the encoder
and the component also to the bridge, since the encoder goes away when
connecting to the bridge interface of the driver and the equivalent of
bind/unbind of the component is handled by bridge attach/detach.

The lifetime requirements of a bridge and a component are slightly
different, which is the reason for struct tda998x_bridge.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 170 --
 1 file changed, 143 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 53fd0be076b4..b8cb6237a38b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -36,6 +36,14 @@ struct tda998x_audio_port {
u8 config;  /* AP value */
 };
 
+struct tda998x_priv;
+
+struct tda998x_bridge {
+   struct tda998x_priv *priv;
+   struct device *dev;
+   struct drm_bridge bridge;
+};
+
 struct tda998x_priv {
struct i2c_client *cec;
struct i2c_client *hdmi;
@@ -63,6 +71,8 @@ struct tda998x_priv {
wait_queue_head_t edid_delay_waitq;
bool edid_delay_active;
 
+   struct tda998x_bridge *bridge;
+   bool local_encoder;
struct drm_encoder encoder;
struct drm_connector connector;
 
@@ -75,6 +85,9 @@ struct tda998x_priv {
 #define enc_to_tda998x_priv(x) \
container_of(x, struct tda998x_priv, encoder)
 
+#define bridge_to_tda998x_bridge(x) \
+   container_of(x, struct tda998x_bridge, bridge)
+
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
  * write a given register, we need to make sure CURPAGE register is set
@@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev, void 
*data,
   struct hdmi_codec_daifmt *daifmt,
   struct hdmi_codec_params *params)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
int i, ret;
struct tda998x_audio_params audio = {
.sample_width = params->sample_width,
@@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev, void 
*data,
 
 static void tda998x_audio_shutdown(struct device *dev, void *data)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
 
@@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev, void 
*data)
 
 int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
 
@@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void 
*data, bool enable)
 static int tda998x_audio_get_eld(struct device *dev, void *data,
 uint8_t *buf, size_t len)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
memcpy(buf, priv->connector.eld,
@@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector 
*connector)
 {
struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
 
-   return >encoder;
+   if (priv->local_encoder)
+   return >encoder;
+   else
+   return priv->bridge->bridge.encoder;
 }
 
 static
@@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv 
*priv,
  struct drm_device *drm)
 {
struct drm_connector *connector = >connector;
+   struct drm_encoder *encoder;
int ret;
 
connector->interlace_allowed = 1;
@@ -1156,7 +1177,8 @@ static int tda998x_connector_init(struct tda998x_priv 
*priv,
if (ret)
return ret;
 
-   drm_mode_connector_attach_encoder(>connector, >encoder);
+   encoder = tda998x_connector_best_encoder(>connector);
+   drm_mode_connector_attach_encoder(>connector, encoder);
 
return 0;
 }
@@ -1672,8 +1694,10 @@ static void tda998x_set_config(struct tda998x_priv *priv,
priv->audio_params = p->audio_params;
 }
 
-static int tda998x_init(struct device *dev, struct drm_device *drm)
+static int tda998x_init(struct device *dev, struct drm_device *drm,

[PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

This makes this driver work with all(?) drivers that are not
componentized and instead expect to connect to a panel/bridge. That
said, the only one tested is atmel-hlcdc.

This hooks the relevant work function previously called by the encoder
and the component also to the bridge, since the encoder goes away when
connecting to the bridge interface of the driver and the equivalent of
bind/unbind of the component is handled by bridge attach/detach.

The lifetime requirements of a bridge and a component are slightly
different, which is the reason for struct tda998x_bridge.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 170 --
 1 file changed, 143 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 53fd0be076b4..b8cb6237a38b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -36,6 +36,14 @@ struct tda998x_audio_port {
u8 config;  /* AP value */
 };
 
+struct tda998x_priv;
+
+struct tda998x_bridge {
+   struct tda998x_priv *priv;
+   struct device *dev;
+   struct drm_bridge bridge;
+};
+
 struct tda998x_priv {
struct i2c_client *cec;
struct i2c_client *hdmi;
@@ -63,6 +71,8 @@ struct tda998x_priv {
wait_queue_head_t edid_delay_waitq;
bool edid_delay_active;
 
+   struct tda998x_bridge *bridge;
+   bool local_encoder;
struct drm_encoder encoder;
struct drm_connector connector;
 
@@ -75,6 +85,9 @@ struct tda998x_priv {
 #define enc_to_tda998x_priv(x) \
container_of(x, struct tda998x_priv, encoder)
 
+#define bridge_to_tda998x_bridge(x) \
+   container_of(x, struct tda998x_bridge, bridge)
+
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
  * write a given register, we need to make sure CURPAGE register is set
@@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev, void 
*data,
   struct hdmi_codec_daifmt *daifmt,
   struct hdmi_codec_params *params)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
int i, ret;
struct tda998x_audio_params audio = {
.sample_width = params->sample_width,
@@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev, void 
*data,
 
 static void tda998x_audio_shutdown(struct device *dev, void *data)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
 
@@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev, void 
*data)
 
 int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
 
@@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void 
*data, bool enable)
 static int tda998x_audio_get_eld(struct device *dev, void *data,
 uint8_t *buf, size_t len)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
memcpy(buf, priv->connector.eld,
@@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector 
*connector)
 {
struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
 
-   return >encoder;
+   if (priv->local_encoder)
+   return >encoder;
+   else
+   return priv->bridge->bridge.encoder;
 }
 
 static
@@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv 
*priv,
  struct drm_device *drm)
 {
struct drm_connector *connector = >connector;
+   struct drm_encoder *encoder;
int ret;
 
connector->interlace_allowed = 1;
@@ -1156,7 +1177,8 @@ static int tda998x_connector_init(struct tda998x_priv 
*priv,
if (ret)
return ret;
 
-   drm_mode_connector_attach_encoder(>connector, >encoder);
+   encoder = tda998x_connector_best_encoder(>connector);
+   drm_mode_connector_attach_encoder(>connector, encoder);
 
return 0;
 }
@@ -1672,8 +1694,10 @@ static void tda998x_set_config(struct tda998x_priv *priv,
priv->audio_params = p->audio_params;
 }
 
-static int tda998x_init(struct device *dev, struct drm_device *drm)
+static int tda998x_init(struct device *dev, struct drm_device *drm,
+