diff mbox

[v2,26/26] drm/bridge: establish a link between the bridge supplier and consumer

Message ID 20180504135212.26977-27-peda@axentia.se (mailing list archive)
State Not Applicable
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Peter Rosin May 4, 2018, 1:52 p.m. UTC
If the bridge supplier is unbound, this will bring the bridge consumer
down along with the bridge. Thus, there will no longer linger any
dangling pointers from the bridge consumer (the drm_device) to some
non-existent bridge supplier.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
 include/drm/drm_bridge.h     |  2 ++
 2 files changed, 20 insertions(+)

Comments

Andrzej Hajda May 7, 2018, 12:59 p.m. UTC | #1
On 04.05.2018 15:52, Peter Rosin wrote:
> If the bridge supplier is unbound, this will bring the bridge consumer
> down along with the bridge. Thus, there will no longer linger any
> dangling pointers from the bridge consumer (the drm_device) to some
> non-existent bridge supplier.

I understand rationales behind this patch, but it is another step into
making drm_dev one big driver with subcomponents, where drm will work
only if every subcomponent is working/loaded. Do we need to go this way?
In case of many platforms such approach results in display turned on
very late on boot for example due to late initialization of some
regulator exposed by some i2c device, which is used by hdmi bridge. And
this hdmi bridge is just to provide alternative(rarely used) display
path, the main display path would work anyway.

So the main question to drm maintainers is about evolution of bridges,
if drm_bridges should become mandatory components of drm device or they
could be added/removed dynamically?

Regards
Andrzej


>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>  include/drm/drm_bridge.h     |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 78d186b6831b..0259f0a3ff27 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>  
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_device.h>
>  #include <drm/drm_encoder.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (bridge->dev)
>  		return -EBUSY;
>  
> +	if (encoder->dev->dev != bridge->odev) {
> +		bridge->link = device_link_add(encoder->dev->dev,
> +					       bridge->odev, 0);
> +		if (!bridge->link) {
> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
> +				dev_name(encoder->dev->dev));
> +			return -EINVAL;
> +		}
> +	}
> +
>  	bridge->dev = encoder->dev;
>  	bridge->encoder = encoder;
>  
>  	if (bridge->funcs->attach) {
>  		ret = bridge->funcs->attach(bridge);
>  		if (ret < 0) {
> +			if (bridge->link)
> +				device_link_del(bridge->link);
> +			bridge->link = NULL;
>  			bridge->dev = NULL;
>  			bridge->encoder = NULL;
>  			return ret;
> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	if (bridge->link)
> +		device_link_del(bridge->link);
> +	bridge->link = NULL;
> +
>  	bridge->dev = NULL;
>  }
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index b656e505d11e..804189c63a4c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> + * @link: drm consumer <-> bridge supplier
>   * @funcs: control functions
>   * @driver_private: pointer to the bridge driver's internal context
>   */
> @@ -271,6 +272,7 @@ struct drm_bridge {
>  	struct drm_bridge *next;
>  	struct list_head list;
>  	const struct drm_bridge_timings *timings;
> +	struct device_link *link;
>  
>  	const struct drm_bridge_funcs *funcs;
>  	void *driver_private;
Peter Rosin May 7, 2018, 1:43 p.m. UTC | #2
On 2018-05-07 14:59, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
> 
> I understand rationales behind this patch, but it is another step into
> making drm_dev one big driver with subcomponents, where drm will work
> only if every subcomponent is working/loaded.

The step is very small IMHO. Just a device-link, which is very easy to
remove once whatever other solution is ready.

>                                               Do we need to go this way?

If the drivers expect the parts to be there, and there is no other safety
net in place if they are not, what is the (short-term) alternative?

> In case of many platforms such approach results in display turned on
> very late on boot for example due to late initialization of some
> regulator exposed by some i2c device, which is used by hdmi bridge. And
> this hdmi bridge is just to provide alternative(rarely used) display
> path, the main display path would work anyway.

This patch does not contribute to any late init and any such delay is not
affected by this. At all.

> So the main question to drm maintainers is about evolution of bridges,
> if drm_bridges should become mandatory components of drm device or they
> could be added/removed dynamically?

That is a much bigger question than this patch/series. Conflating the
two is not fair IMHO. You could run this very same argument for every
driver that gets added, since any additional driver will just make it
harder to make everything dynamic. Should we stop development right
away?

Besides, as long as the drm devices are in fact acting as big static
drivers (built from smaller parts), this should be considered a bug-fix
that will prevent dereference of stale pointers.

Or will some other solution appear and magically make all bridges and
drm drivers capable of dynamic reconfiguration in the next few weeks?
Yeah, right...

Cheers,
Peter

> Regards
> Andrzej
> 
> 
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>  include/drm/drm_bridge.h     |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 78d186b6831b..0259f0a3ff27 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/mutex.h>
>>  
>>  #include <drm/drm_bridge.h>
>> +#include <drm/drm_device.h>
>>  #include <drm/drm_encoder.h>
>>  
>>  #include "drm_crtc_internal.h"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>  	if (bridge->dev)
>>  		return -EBUSY;
>>  
>> +	if (encoder->dev->dev != bridge->odev) {
>> +		bridge->link = device_link_add(encoder->dev->dev,
>> +					       bridge->odev, 0);
>> +		if (!bridge->link) {
>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>> +				dev_name(encoder->dev->dev));
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>>  	bridge->dev = encoder->dev;
>>  	bridge->encoder = encoder;
>>  
>>  	if (bridge->funcs->attach) {
>>  		ret = bridge->funcs->attach(bridge);
>>  		if (ret < 0) {
>> +			if (bridge->link)
>> +				device_link_del(bridge->link);
>> +			bridge->link = NULL;
>>  			bridge->dev = NULL;
>>  			bridge->encoder = NULL;
>>  			return ret;
>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>  	if (bridge->funcs->detach)
>>  		bridge->funcs->detach(bridge);
>>  
>> +	if (bridge->link)
>> +		device_link_del(bridge->link);
>> +	bridge->link = NULL;
>> +
>>  	bridge->dev = NULL;
>>  }
>>  
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index b656e505d11e..804189c63a4c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>   * @list: to keep track of all added bridges
>>   * @timings: the timing specification for the bridge, if any (may
>>   * be NULL)
>> + * @link: drm consumer <-> bridge supplier
>>   * @funcs: control functions
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>  	struct drm_bridge *next;
>>  	struct list_head list;
>>  	const struct drm_bridge_timings *timings;
>> +	struct device_link *link;
>>  
>>  	const struct drm_bridge_funcs *funcs;
>>  	void *driver_private;
> 
>
Daniel Vetter May 7, 2018, 1:53 p.m. UTC | #3
On Mon, May 07, 2018 at 02:59:45PM +0200, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
> > If the bridge supplier is unbound, this will bring the bridge consumer
> > down along with the bridge. Thus, there will no longer linger any
> > dangling pointers from the bridge consumer (the drm_device) to some
> > non-existent bridge supplier.
> 
> I understand rationales behind this patch, but it is another step into
> making drm_dev one big driver with subcomponents, where drm will work
> only if every subcomponent is working/loaded. Do we need to go this way?
> In case of many platforms such approach results in display turned on
> very late on boot for example due to late initialization of some
> regulator exposed by some i2c device, which is used by hdmi bridge. And
> this hdmi bridge is just to provide alternative(rarely used) display
> path, the main display path would work anyway.
> 
> So the main question to drm maintainers is about evolution of bridges,
> if drm_bridges should become mandatory components of drm device or they
> could be added/removed dynamically?

This is already the case. You currently cannot hotplug a drm_bridge,
everything must be present. I don't think it makes sense to change that
until we have physically hotpluggable drm_bridges in real hardware. I
definitely don't want to refcount stuff to work around driver load
bonghits on DT platforms, because refcounting is way too hard to get right
:-)

Afaik there's out-of-tree patches to solve 99% of the driver load fun on
DT platforms, but because it's not a 100% solution it's blocked since
forever.
-Daniel

> 
> Regards
> Andrzej
> 
> 
> >
> > Signed-off-by: Peter Rosin <peda@axentia.se>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
> >  include/drm/drm_bridge.h     |  2 ++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 78d186b6831b..0259f0a3ff27 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/mutex.h>
> >  
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_device.h>
> >  #include <drm/drm_encoder.h>
> >  
> >  #include "drm_crtc_internal.h"
> > @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  	if (bridge->dev)
> >  		return -EBUSY;
> >  
> > +	if (encoder->dev->dev != bridge->odev) {
> > +		bridge->link = device_link_add(encoder->dev->dev,
> > +					       bridge->odev, 0);
> > +		if (!bridge->link) {
> > +			dev_err(bridge->odev, "failed to link bridge to %s\n",
> > +				dev_name(encoder->dev->dev));
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	bridge->dev = encoder->dev;
> >  	bridge->encoder = encoder;
> >  
> >  	if (bridge->funcs->attach) {
> >  		ret = bridge->funcs->attach(bridge);
> >  		if (ret < 0) {
> > +			if (bridge->link)
> > +				device_link_del(bridge->link);
> > +			bridge->link = NULL;
> >  			bridge->dev = NULL;
> >  			bridge->encoder = NULL;
> >  			return ret;
> > @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >  	if (bridge->funcs->detach)
> >  		bridge->funcs->detach(bridge);
> >  
> > +	if (bridge->link)
> > +		device_link_del(bridge->link);
> > +	bridge->link = NULL;
> > +
> >  	bridge->dev = NULL;
> >  }
> >  
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index b656e505d11e..804189c63a4c 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -261,6 +261,7 @@ struct drm_bridge_timings {
> >   * @list: to keep track of all added bridges
> >   * @timings: the timing specification for the bridge, if any (may
> >   * be NULL)
> > + * @link: drm consumer <-> bridge supplier
> >   * @funcs: control functions
> >   * @driver_private: pointer to the bridge driver's internal context
> >   */
> > @@ -271,6 +272,7 @@ struct drm_bridge {
> >  	struct drm_bridge *next;
> >  	struct list_head list;
> >  	const struct drm_bridge_timings *timings;
> > +	struct device_link *link;
> >  
> >  	const struct drm_bridge_funcs *funcs;
> >  	void *driver_private;
> 
>
Andrzej Hajda May 8, 2018, 6:36 a.m. UTC | #4
On 07.05.2018 15:53, Daniel Vetter wrote:
> On Mon, May 07, 2018 at 02:59:45PM +0200, Andrzej Hajda wrote:
>> On 04.05.2018 15:52, Peter Rosin wrote:
>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>> down along with the bridge. Thus, there will no longer linger any
>>> dangling pointers from the bridge consumer (the drm_device) to some
>>> non-existent bridge supplier.
>> I understand rationales behind this patch, but it is another step into
>> making drm_dev one big driver with subcomponents, where drm will work
>> only if every subcomponent is working/loaded. Do we need to go this way?
>> In case of many platforms such approach results in display turned on
>> very late on boot for example due to late initialization of some
>> regulator exposed by some i2c device, which is used by hdmi bridge. And
>> this hdmi bridge is just to provide alternative(rarely used) display
>> path, the main display path would work anyway.
>>
>> So the main question to drm maintainers is about evolution of bridges,
>> if drm_bridges should become mandatory components of drm device or they
>> could be added/removed dynamically?
> This is already the case. You currently cannot hotplug a drm_bridge,
> everything must be present.

Are you sure? DRM core is changing quite fast, so maybe I have missed
something, but AFAIK core calls bridge code only if full display
pipeline is created and connector is in connected state.
So adding and removing bridges from inactive pipelines should work if
coded properly.

>  I don't think it makes sense to change that
> until we have physically hotpluggable drm_bridges in real hardware.

But kernel core already assumes that device drivers are hot-pluggable
:), even this patch is created to solve issues regarding driver hot
unplugging.

>  I
> definitely don't want to refcount stuff to work around driver load
> bonghits on DT platforms, because refcounting is way too hard to get right
> :-)

I am not sure about bridges, but I have successfully (IMO) experimented
with hot (un)plugging panel driver, see panel-samsung-s6e8aa0.c driver
and exynos_drm_dsi.c, panel driver can be safely
plugged/unplugged/replugged without any refcounting (but with help of
mipi_dsi attach/detach callbacks, which are not available for
non-mipi-dsi drivers).

Regards
Andrzej

>
> Afaik there's out-of-tree patches to solve 99% of the driver load fun on
> DT platforms, but because it's not a 100% solution it's blocked since
> forever.
> -Daniel
>
>> Regards
>> Andrzej
>>
>>
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>  include/drm/drm_bridge.h     |  2 ++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index 78d186b6831b..0259f0a3ff27 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -26,6 +26,7 @@
>>>  #include <linux/mutex.h>
>>>  
>>>  #include <drm/drm_bridge.h>
>>> +#include <drm/drm_device.h>
>>>  #include <drm/drm_encoder.h>
>>>  
>>>  #include "drm_crtc_internal.h"
>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>  	if (bridge->dev)
>>>  		return -EBUSY;
>>>  
>>> +	if (encoder->dev->dev != bridge->odev) {
>>> +		bridge->link = device_link_add(encoder->dev->dev,
>>> +					       bridge->odev, 0);
>>> +		if (!bridge->link) {
>>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>>> +				dev_name(encoder->dev->dev));
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>> +
>>>  	bridge->dev = encoder->dev;
>>>  	bridge->encoder = encoder;
>>>  
>>>  	if (bridge->funcs->attach) {
>>>  		ret = bridge->funcs->attach(bridge);
>>>  		if (ret < 0) {
>>> +			if (bridge->link)
>>> +				device_link_del(bridge->link);
>>> +			bridge->link = NULL;
>>>  			bridge->dev = NULL;
>>>  			bridge->encoder = NULL;
>>>  			return ret;
>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>  	if (bridge->funcs->detach)
>>>  		bridge->funcs->detach(bridge);
>>>  
>>> +	if (bridge->link)
>>> +		device_link_del(bridge->link);
>>> +	bridge->link = NULL;
>>> +
>>>  	bridge->dev = NULL;
>>>  }
>>>  
>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>> index b656e505d11e..804189c63a4c 100644
>>> --- a/include/drm/drm_bridge.h
>>> +++ b/include/drm/drm_bridge.h
>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>   * @list: to keep track of all added bridges
>>>   * @timings: the timing specification for the bridge, if any (may
>>>   * be NULL)
>>> + * @link: drm consumer <-> bridge supplier
>>>   * @funcs: control functions
>>>   * @driver_private: pointer to the bridge driver's internal context
>>>   */
>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>  	struct drm_bridge *next;
>>>  	struct list_head list;
>>>  	const struct drm_bridge_timings *timings;
>>> +	struct device_link *link;
>>>  
>>>  	const struct drm_bridge_funcs *funcs;
>>>  	void *driver_private;
>>
Andrzej Hajda May 8, 2018, 9:03 a.m. UTC | #5
On 07.05.2018 15:43, Peter Rosin wrote:
> On 2018-05-07 14:59, Andrzej Hajda wrote:
>> On 04.05.2018 15:52, Peter Rosin wrote:
>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>> down along with the bridge. Thus, there will no longer linger any
>>> dangling pointers from the bridge consumer (the drm_device) to some
>>> non-existent bridge supplier.
>> I understand rationales behind this patch, but it is another step into
>> making drm_dev one big driver with subcomponents, where drm will work
>> only if every subcomponent is working/loaded.
> The step is very small IMHO. Just a device-link, which is very easy to
> remove once whatever other solution is ready.
>
>>                                               Do we need to go this way?
> If the drivers expect the parts to be there, and there is no other safety
> net in place if they are not, what is the (short-term) alternative?
>
>> In case of many platforms such approach results in display turned on
>> very late on boot for example due to late initialization of some
>> regulator exposed by some i2c device, which is used by hdmi bridge. And
>> this hdmi bridge is just to provide alternative(rarely used) display
>> path, the main display path would work anyway.
> This patch does not contribute to any late init and any such delay is not
> affected by this. At all.
>
>> So the main question to drm maintainers is about evolution of bridges,
>> if drm_bridges should become mandatory components of drm device or they
>> could be added/removed dynamically?
> That is a much bigger question than this patch/series. Conflating the
> two is not fair IMHO. You could run this very same argument for every
> driver that gets added, since any additional driver will just make it
> harder to make everything dynamic. Should we stop development right
> away?
>
> Besides, as long as the drm devices are in fact acting as big static
> drivers (built from smaller parts), 

not true

> this should be considered a bug-fix
> that will prevent dereference of stale pointers.
>
> Or will some other solution appear and magically make all bridges and
> drm drivers capable of dynamic reconfiguration in the next few weeks?
> Yeah, right...

You are not changing single driver, you are changing framework and it
affects all the drivers using it, being more cautious about such patches
seems quite natural.

Anyway, I have realized that since drm_bridge_detach will remove the
link, so with properly written dynamic bridge removal, your patch should
not be a blocker.

Regards
Andrzej

>
> Cheers,
> Peter
>
>> Regards
>> Andrzej
>>
>>
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>  include/drm/drm_bridge.h     |  2 ++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index 78d186b6831b..0259f0a3ff27 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -26,6 +26,7 @@
>>>  #include <linux/mutex.h>
>>>  
>>>  #include <drm/drm_bridge.h>
>>> +#include <drm/drm_device.h>
>>>  #include <drm/drm_encoder.h>
>>>  
>>>  #include "drm_crtc_internal.h"
>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>  	if (bridge->dev)
>>>  		return -EBUSY;
>>>  
>>> +	if (encoder->dev->dev != bridge->odev) {
>>> +		bridge->link = device_link_add(encoder->dev->dev,
>>> +					       bridge->odev, 0);
>>> +		if (!bridge->link) {
>>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>>> +				dev_name(encoder->dev->dev));
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>> +
>>>  	bridge->dev = encoder->dev;
>>>  	bridge->encoder = encoder;
>>>  
>>>  	if (bridge->funcs->attach) {
>>>  		ret = bridge->funcs->attach(bridge);
>>>  		if (ret < 0) {
>>> +			if (bridge->link)
>>> +				device_link_del(bridge->link);
>>> +			bridge->link = NULL;
>>>  			bridge->dev = NULL;
>>>  			bridge->encoder = NULL;
>>>  			return ret;
>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>  	if (bridge->funcs->detach)
>>>  		bridge->funcs->detach(bridge);
>>>  
>>> +	if (bridge->link)
>>> +		device_link_del(bridge->link);
>>> +	bridge->link = NULL;
>>> +
>>>  	bridge->dev = NULL;
>>>  }
>>>  
>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>> index b656e505d11e..804189c63a4c 100644
>>> --- a/include/drm/drm_bridge.h
>>> +++ b/include/drm/drm_bridge.h
>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>   * @list: to keep track of all added bridges
>>>   * @timings: the timing specification for the bridge, if any (may
>>>   * be NULL)
>>> + * @link: drm consumer <-> bridge supplier
>>>   * @funcs: control functions
>>>   * @driver_private: pointer to the bridge driver's internal context
>>>   */
>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>  	struct drm_bridge *next;
>>>  	struct list_head list;
>>>  	const struct drm_bridge_timings *timings;
>>> +	struct device_link *link;
>>>  
>>>  	const struct drm_bridge_funcs *funcs;
>>>  	void *driver_private;
>>
>
>
>
Andrzej Hajda May 10, 2018, 8:10 a.m. UTC | #6
On 04.05.2018 15:52, Peter Rosin wrote:
> If the bridge supplier is unbound, this will bring the bridge consumer
> down along with the bridge. Thus, there will no longer linger any
> dangling pointers from the bridge consumer (the drm_device) to some
> non-existent bridge supplier.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>  include/drm/drm_bridge.h     |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 78d186b6831b..0259f0a3ff27 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>  
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_device.h>
>  #include <drm/drm_encoder.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (bridge->dev)
>  		return -EBUSY;
>  
> +	if (encoder->dev->dev != bridge->odev) {

I wonder why device_link_add does not handle this case (self dependency)
silently as noop, as it seems to be a correct behavior.

> +		bridge->link = device_link_add(encoder->dev->dev,
> +					       bridge->odev, 0);
> +		if (!bridge->link) {
> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
> +				dev_name(encoder->dev->dev));
> +			return -EINVAL;
> +		}
> +	}
> +
>  	bridge->dev = encoder->dev;
>  	bridge->encoder = encoder;
>  
>  	if (bridge->funcs->attach) {
>  		ret = bridge->funcs->attach(bridge);
>  		if (ret < 0) {
> +			if (bridge->link)
> +				device_link_del(bridge->link);
> +			bridge->link = NULL;
>  			bridge->dev = NULL;
>  			bridge->encoder = NULL;
>  			return ret;
> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	if (bridge->link)
> +		device_link_del(bridge->link);
> +	bridge->link = NULL;
> +
>  	bridge->dev = NULL;
>  }
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index b656e505d11e..804189c63a4c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> + * @link: drm consumer <-> bridge supplier

Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
to the bridge" would be better.

Anyway:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

>   * @funcs: control functions
>   * @driver_private: pointer to the bridge driver's internal context
>   */
> @@ -271,6 +272,7 @@ struct drm_bridge {
>  	struct drm_bridge *next;
>  	struct list_head list;
>  	const struct drm_bridge_timings *timings;
> +	struct device_link *link;
>  
>  	const struct drm_bridge_funcs *funcs;
>  	void *driver_private;
Peter Rosin May 11, 2018, 7:37 a.m. UTC | #7
On 2018-05-10 10:10, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>  include/drm/drm_bridge.h     |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 78d186b6831b..0259f0a3ff27 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/mutex.h>
>>  
>>  #include <drm/drm_bridge.h>
>> +#include <drm/drm_device.h>
>>  #include <drm/drm_encoder.h>
>>  
>>  #include "drm_crtc_internal.h"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>  	if (bridge->dev)
>>  		return -EBUSY;
>>  
>> +	if (encoder->dev->dev != bridge->odev) {
> 
> I wonder why device_link_add does not handle this case (self dependency)
> silently as noop, as it seems to be a correct behavior.

It's kind-of a silly corner-case though, so perfectly understandable
that it isn't handled.

>> +		bridge->link = device_link_add(encoder->dev->dev,
>> +					       bridge->odev, 0);
>> +		if (!bridge->link) {
>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>> +				dev_name(encoder->dev->dev));
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>>  	bridge->dev = encoder->dev;
>>  	bridge->encoder = encoder;
>>  
>>  	if (bridge->funcs->attach) {
>>  		ret = bridge->funcs->attach(bridge);
>>  		if (ret < 0) {
>> +			if (bridge->link)
>> +				device_link_del(bridge->link);
>> +			bridge->link = NULL;
>>  			bridge->dev = NULL;
>>  			bridge->encoder = NULL;
>>  			return ret;
>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>  	if (bridge->funcs->detach)
>>  		bridge->funcs->detach(bridge);
>>  
>> +	if (bridge->link)
>> +		device_link_del(bridge->link);
>> +	bridge->link = NULL;
>> +
>>  	bridge->dev = NULL;
>>  }
>>  
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index b656e505d11e..804189c63a4c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>   * @list: to keep track of all added bridges
>>   * @timings: the timing specification for the bridge, if any (may
>>   * be NULL)
>> + * @link: drm consumer <-> bridge supplier
> 
> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> to the bridge" would be better.

I meant "<->" to indicate that the link is bidirectional, not that the
relationship is in any way symmetric. I wasn't aware of any implication
of a symmetric relationship when using "<->", do you have a reference?
But I guess the different arrow notations in math are somewhat overloaded
and that someone at some point must have used "<->" to indicate a
symmetric relationship...

> Anyway:
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Thanks!

Cheers,
Peter

>  --
> Regards
> Andrzej
> 
>>   * @funcs: control functions
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>  	struct drm_bridge *next;
>>  	struct list_head list;
>>  	const struct drm_bridge_timings *timings;
>> +	struct device_link *link;
>>  
>>  	const struct drm_bridge_funcs *funcs;
>>  	void *driver_private;
> 
>
Daniel Vetter May 14, 2018, 4:28 p.m. UTC | #8
On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
> On 2018-05-10 10:10, Andrzej Hajda wrote:
> > On 04.05.2018 15:52, Peter Rosin wrote:
> >> If the bridge supplier is unbound, this will bring the bridge consumer
> >> down along with the bridge. Thus, there will no longer linger any
> >> dangling pointers from the bridge consumer (the drm_device) to some
> >> non-existent bridge supplier.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
> >>  include/drm/drm_bridge.h     |  2 ++
> >>  2 files changed, 20 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 78d186b6831b..0259f0a3ff27 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -26,6 +26,7 @@
> >>  #include <linux/mutex.h>
> >>  
> >>  #include <drm/drm_bridge.h>
> >> +#include <drm/drm_device.h>
> >>  #include <drm/drm_encoder.h>
> >>  
> >>  #include "drm_crtc_internal.h"
> >> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >>  	if (bridge->dev)
> >>  		return -EBUSY;
> >>  
> >> +	if (encoder->dev->dev != bridge->odev) {
> > 
> > I wonder why device_link_add does not handle this case (self dependency)
> > silently as noop, as it seems to be a correct behavior.
> 
> It's kind-of a silly corner-case though, so perfectly understandable
> that it isn't handled.
> 
> >> +		bridge->link = device_link_add(encoder->dev->dev,
> >> +					       bridge->odev, 0);
> >> +		if (!bridge->link) {
> >> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
> >> +				dev_name(encoder->dev->dev));
> >> +			return -EINVAL;
> >> +		}
> >> +	}
> >> +
> >>  	bridge->dev = encoder->dev;
> >>  	bridge->encoder = encoder;
> >>  
> >>  	if (bridge->funcs->attach) {
> >>  		ret = bridge->funcs->attach(bridge);
> >>  		if (ret < 0) {
> >> +			if (bridge->link)
> >> +				device_link_del(bridge->link);
> >> +			bridge->link = NULL;
> >>  			bridge->dev = NULL;
> >>  			bridge->encoder = NULL;
> >>  			return ret;
> >> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >>  	if (bridge->funcs->detach)
> >>  		bridge->funcs->detach(bridge);
> >>  
> >> +	if (bridge->link)
> >> +		device_link_del(bridge->link);
> >> +	bridge->link = NULL;
> >> +
> >>  	bridge->dev = NULL;
> >>  }
> >>  
> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >> index b656e505d11e..804189c63a4c 100644
> >> --- a/include/drm/drm_bridge.h
> >> +++ b/include/drm/drm_bridge.h
> >> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
> >>   * @list: to keep track of all added bridges
> >>   * @timings: the timing specification for the bridge, if any (may
> >>   * be NULL)
> >> + * @link: drm consumer <-> bridge supplier
> > 
> > Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> > to the bridge" would be better.
> 
> I meant "<->" to indicate that the link is bidirectional, not that the
> relationship is in any way symmetric. I wasn't aware of any implication
> of a symmetric relationship when using "<->", do you have a reference?
> But I guess the different arrow notations in math are somewhat overloaded
> and that someone at some point must have used "<->" to indicate a
> symmetric relationship...

Yeah I agree with Andrzej here, for me <-> implies a symmetric
relationship. Spelling it out like Andrzej suggested sounds like the
better idea.
-Daniel

> 
> > Anyway:
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> Thanks!
> 
> Cheers,
> Peter
> 
> >  --
> > Regards
> > Andrzej
> > 
> >>   * @funcs: control functions
> >>   * @driver_private: pointer to the bridge driver's internal context
> >>   */
> >> @@ -271,6 +272,7 @@ struct drm_bridge {
> >>  	struct drm_bridge *next;
> >>  	struct list_head list;
> >>  	const struct drm_bridge_timings *timings;
> >> +	struct device_link *link;
> >>  
> >>  	const struct drm_bridge_funcs *funcs;
> >>  	void *driver_private;
> > 
> > 
>
Peter Rosin May 14, 2018, 8:40 p.m. UTC | #9
On 2018-05-14 18:28, Daniel Vetter wrote:
> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>> On 04.05.2018 15:52, Peter Rosin wrote:
>>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>>> down along with the bridge. Thus, there will no longer linger any
>>>> dangling pointers from the bridge consumer (the drm_device) to some
>>>> non-existent bridge supplier.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>>  include/drm/drm_bridge.h     |  2 ++
>>>>  2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>> index 78d186b6831b..0259f0a3ff27 100644
>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>> @@ -26,6 +26,7 @@
>>>>  #include <linux/mutex.h>
>>>>  
>>>>  #include <drm/drm_bridge.h>
>>>> +#include <drm/drm_device.h>
>>>>  #include <drm/drm_encoder.h>
>>>>  
>>>>  #include "drm_crtc_internal.h"
>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>>  	if (bridge->dev)
>>>>  		return -EBUSY;
>>>>  
>>>> +	if (encoder->dev->dev != bridge->odev) {
>>>
>>> I wonder why device_link_add does not handle this case (self dependency)
>>> silently as noop, as it seems to be a correct behavior.
>>
>> It's kind-of a silly corner-case though, so perfectly understandable
>> that it isn't handled.
>>
>>>> +		bridge->link = device_link_add(encoder->dev->dev,
>>>> +					       bridge->odev, 0);
>>>> +		if (!bridge->link) {
>>>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>>>> +				dev_name(encoder->dev->dev));
>>>> +			return -EINVAL;
>>>> +		}
>>>> +	}
>>>> +
>>>>  	bridge->dev = encoder->dev;
>>>>  	bridge->encoder = encoder;
>>>>  
>>>>  	if (bridge->funcs->attach) {
>>>>  		ret = bridge->funcs->attach(bridge);
>>>>  		if (ret < 0) {
>>>> +			if (bridge->link)
>>>> +				device_link_del(bridge->link);
>>>> +			bridge->link = NULL;
>>>>  			bridge->dev = NULL;
>>>>  			bridge->encoder = NULL;
>>>>  			return ret;
>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>>  	if (bridge->funcs->detach)
>>>>  		bridge->funcs->detach(bridge);
>>>>  
>>>> +	if (bridge->link)
>>>> +		device_link_del(bridge->link);
>>>> +	bridge->link = NULL;
>>>> +
>>>>  	bridge->dev = NULL;
>>>>  }
>>>>  
>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>> index b656e505d11e..804189c63a4c 100644
>>>> --- a/include/drm/drm_bridge.h
>>>> +++ b/include/drm/drm_bridge.h
>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>>   * @list: to keep track of all added bridges
>>>>   * @timings: the timing specification for the bridge, if any (may
>>>>   * be NULL)
>>>> + * @link: drm consumer <-> bridge supplier
>>>
>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
>>> to the bridge" would be better.
>>
>> I meant "<->" to indicate that the link is bidirectional, not that the
>> relationship is in any way symmetric. I wasn't aware of any implication
>> of a symmetric relationship when using "<->", do you have a reference?
>> But I guess the different arrow notations in math are somewhat overloaded
>> and that someone at some point must have used "<->" to indicate a
>> symmetric relationship...
> 
> Yeah I agree with Andrzej here, for me <-> implies a symmetric
> relationship. Spelling it out like Andrzej suggested sounds like the
> better idea.
> -Daniel

Ok, I guess that means I have to do a v3 after all. Or can this
trivial documentation update be done by the committer? I hate to
spam everyone with another volley...

Or perhaps I should squash patches 2-23 that are all rather similar
and mechanic? I separated them to allow for easier review from
individual driver maintainers, but that didn't seem to happen
anyway...

Cheers,
Peter

> 
>>
>>> Anyway:
>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>
>> Thanks!
>>
>> Cheers,
>> Peter
>>
>>>  --
>>> Regards
>>> Andrzej
>>>
>>>>   * @funcs: control functions
>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>   */
>>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>>  	struct drm_bridge *next;
>>>>  	struct list_head list;
>>>>  	const struct drm_bridge_timings *timings;
>>>> +	struct device_link *link;
>>>>  
>>>>  	const struct drm_bridge_funcs *funcs;
>>>>  	void *driver_private;
>>>
>>>
>>
>
Daniel Vetter May 15, 2018, 10:22 a.m. UTC | #10
On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-14 18:28, Daniel Vetter wrote:
>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>>> On 04.05.2018 15:52, Peter Rosin wrote:
>>>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>>>> down along with the bridge. Thus, there will no longer linger any
>>>>> dangling pointers from the bridge consumer (the drm_device) to some
>>>>> non-existent bridge supplier.
>>>>>
>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>>>  include/drm/drm_bridge.h     |  2 ++
>>>>>  2 files changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>>> index 78d186b6831b..0259f0a3ff27 100644
>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>> @@ -26,6 +26,7 @@
>>>>>  #include <linux/mutex.h>
>>>>>
>>>>>  #include <drm/drm_bridge.h>
>>>>> +#include <drm/drm_device.h>
>>>>>  #include <drm/drm_encoder.h>
>>>>>
>>>>>  #include "drm_crtc_internal.h"
>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>>>    if (bridge->dev)
>>>>>            return -EBUSY;
>>>>>
>>>>> +  if (encoder->dev->dev != bridge->odev) {
>>>>
>>>> I wonder why device_link_add does not handle this case (self dependency)
>>>> silently as noop, as it seems to be a correct behavior.
>>>
>>> It's kind-of a silly corner-case though, so perfectly understandable
>>> that it isn't handled.
>>>
>>>>> +          bridge->link = device_link_add(encoder->dev->dev,
>>>>> +                                         bridge->odev, 0);
>>>>> +          if (!bridge->link) {
>>>>> +                  dev_err(bridge->odev, "failed to link bridge to %s\n",
>>>>> +                          dev_name(encoder->dev->dev));
>>>>> +                  return -EINVAL;
>>>>> +          }
>>>>> +  }
>>>>> +
>>>>>    bridge->dev = encoder->dev;
>>>>>    bridge->encoder = encoder;
>>>>>
>>>>>    if (bridge->funcs->attach) {
>>>>>            ret = bridge->funcs->attach(bridge);
>>>>>            if (ret < 0) {
>>>>> +                  if (bridge->link)
>>>>> +                          device_link_del(bridge->link);
>>>>> +                  bridge->link = NULL;
>>>>>                    bridge->dev = NULL;
>>>>>                    bridge->encoder = NULL;
>>>>>                    return ret;
>>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>>>    if (bridge->funcs->detach)
>>>>>            bridge->funcs->detach(bridge);
>>>>>
>>>>> +  if (bridge->link)
>>>>> +          device_link_del(bridge->link);
>>>>> +  bridge->link = NULL;
>>>>> +
>>>>>    bridge->dev = NULL;
>>>>>  }
>>>>>
>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>>> index b656e505d11e..804189c63a4c 100644
>>>>> --- a/include/drm/drm_bridge.h
>>>>> +++ b/include/drm/drm_bridge.h
>>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>>>   * @list: to keep track of all added bridges
>>>>>   * @timings: the timing specification for the bridge, if any (may
>>>>>   * be NULL)
>>>>> + * @link: drm consumer <-> bridge supplier
>>>>
>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
>>>> to the bridge" would be better.
>>>
>>> I meant "<->" to indicate that the link is bidirectional, not that the
>>> relationship is in any way symmetric. I wasn't aware of any implication
>>> of a symmetric relationship when using "<->", do you have a reference?
>>> But I guess the different arrow notations in math are somewhat overloaded
>>> and that someone at some point must have used "<->" to indicate a
>>> symmetric relationship...
>>
>> Yeah I agree with Andrzej here, for me <-> implies a symmetric
>> relationship. Spelling it out like Andrzej suggested sounds like the
>> better idea.
>> -Daniel
>
> Ok, I guess that means I have to do a v3 after all. Or can this
> trivial documentation update be done by the committer? I hate to
> spam everyone with another volley...
>
> Or perhaps I should squash patches 2-23 that are all rather similar
> and mechanic? I separated them to allow for easier review from
> individual driver maintainers, but that didn't seem to happen
> anyway...

Do another volley of the full set, or in-reply-to to just replace the
patch that needs to be respun (but some people don't like that).

When resending just make sure you're picking up all the acks/r-bs you
have already.
-Daniel
> Cheers,
> Peter
>
>>
>>>
>>>> Anyway:
>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>>
>>> Thanks!
>>>
>>> Cheers,
>>> Peter
>>>
>>>>  --
>>>> Regards
>>>> Andrzej
>>>>
>>>>>   * @funcs: control functions
>>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>>   */
>>>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>>>    struct drm_bridge *next;
>>>>>    struct list_head list;
>>>>>    const struct drm_bridge_timings *timings;
>>>>> +  struct device_link *link;
>>>>>
>>>>>    const struct drm_bridge_funcs *funcs;
>>>>>    void *driver_private;
>>>>
>>>>
>>>
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Peter Rosin May 15, 2018, 11:09 a.m. UTC | #11
On 2018-05-15 12:22, Daniel Vetter wrote:
> On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-14 18:28, Daniel Vetter wrote:
>>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>>>> On 04.05.2018 15:52, Peter Rosin wrote:
>>>>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>>>>> down along with the bridge. Thus, there will no longer linger any
>>>>>> dangling pointers from the bridge consumer (the drm_device) to some
>>>>>> non-existent bridge supplier.
>>>>>>
>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>>> ---
>>>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>>>>  include/drm/drm_bridge.h     |  2 ++
>>>>>>  2 files changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>>>> index 78d186b6831b..0259f0a3ff27 100644
>>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>>  #include <linux/mutex.h>
>>>>>>
>>>>>>  #include <drm/drm_bridge.h>
>>>>>> +#include <drm/drm_device.h>
>>>>>>  #include <drm/drm_encoder.h>
>>>>>>
>>>>>>  #include "drm_crtc_internal.h"
>>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>>>>    if (bridge->dev)
>>>>>>            return -EBUSY;
>>>>>>
>>>>>> +  if (encoder->dev->dev != bridge->odev) {
>>>>>
>>>>> I wonder why device_link_add does not handle this case (self dependency)
>>>>> silently as noop, as it seems to be a correct behavior.
>>>>
>>>> It's kind-of a silly corner-case though, so perfectly understandable
>>>> that it isn't handled.
>>>>
>>>>>> +          bridge->link = device_link_add(encoder->dev->dev,
>>>>>> +                                         bridge->odev, 0);
>>>>>> +          if (!bridge->link) {
>>>>>> +                  dev_err(bridge->odev, "failed to link bridge to %s\n",
>>>>>> +                          dev_name(encoder->dev->dev));
>>>>>> +                  return -EINVAL;
>>>>>> +          }
>>>>>> +  }
>>>>>> +
>>>>>>    bridge->dev = encoder->dev;
>>>>>>    bridge->encoder = encoder;
>>>>>>
>>>>>>    if (bridge->funcs->attach) {
>>>>>>            ret = bridge->funcs->attach(bridge);
>>>>>>            if (ret < 0) {
>>>>>> +                  if (bridge->link)
>>>>>> +                          device_link_del(bridge->link);
>>>>>> +                  bridge->link = NULL;
>>>>>>                    bridge->dev = NULL;
>>>>>>                    bridge->encoder = NULL;
>>>>>>                    return ret;
>>>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>>>>    if (bridge->funcs->detach)
>>>>>>            bridge->funcs->detach(bridge);
>>>>>>
>>>>>> +  if (bridge->link)
>>>>>> +          device_link_del(bridge->link);
>>>>>> +  bridge->link = NULL;
>>>>>> +
>>>>>>    bridge->dev = NULL;
>>>>>>  }
>>>>>>
>>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>>>> index b656e505d11e..804189c63a4c 100644
>>>>>> --- a/include/drm/drm_bridge.h
>>>>>> +++ b/include/drm/drm_bridge.h
>>>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>>>>   * @list: to keep track of all added bridges
>>>>>>   * @timings: the timing specification for the bridge, if any (may
>>>>>>   * be NULL)
>>>>>> + * @link: drm consumer <-> bridge supplier
>>>>>
>>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
>>>>> to the bridge" would be better.
>>>>
>>>> I meant "<->" to indicate that the link is bidirectional, not that the
>>>> relationship is in any way symmetric. I wasn't aware of any implication
>>>> of a symmetric relationship when using "<->", do you have a reference?
>>>> But I guess the different arrow notations in math are somewhat overloaded
>>>> and that someone at some point must have used "<->" to indicate a
>>>> symmetric relationship...
>>>
>>> Yeah I agree with Andrzej here, for me <-> implies a symmetric
>>> relationship. Spelling it out like Andrzej suggested sounds like the
>>> better idea.
>>> -Daniel
>>
>> Ok, I guess that means I have to do a v3 after all. Or can this
>> trivial documentation update be done by the committer? I hate to
>> spam everyone with another volley...
>>
>> Or perhaps I should squash patches 2-23 that are all rather similar
>> and mechanic? I separated them to allow for easier review from
>> individual driver maintainers, but that didn't seem to happen
>> anyway...
> 
> Do another volley of the full set, or in-reply-to to just replace the
> patch that needs to be respun (but some people don't like that).
> 
> When resending just make sure you're picking up all the acks/r-bs you
> have already.

Right, I always try to do that. One Ack that I did not include in v2
was the one you had on v1 24/24 (i.e. this patch). The reason I did
not add your Ack for v2 even on the patch where it obviously applied
was that I didn't know if you'd barf on the odev name.

But it was (and still is) a bit unclear if that was on Ack on the
last patch only, or if it was for the whole series? I think it might
have been for the whole series, but I'm not sure and I hate to be a
presumptuous idiot...

Cheers,
Peter

> -Daniel
>> Cheers,
>> Peter
>>
>>>
>>>>
>>>>> Anyway:
>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>
>>>> Thanks!
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>>  --
>>>>> Regards
>>>>> Andrzej
>>>>>
>>>>>>   * @funcs: control functions
>>>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>>>   */
>>>>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>>>>    struct drm_bridge *next;
>>>>>>    struct list_head list;
>>>>>>    const struct drm_bridge_timings *timings;
>>>>>> +  struct device_link *link;
>>>>>>
>>>>>>    const struct drm_bridge_funcs *funcs;
>>>>>>    void *driver_private;
>>>>>
>>>>>
>>>>
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
>
Daniel Vetter May 16, 2018, 9:31 a.m. UTC | #12
On Tue, May 15, 2018 at 01:09:59PM +0200, Peter Rosin wrote:
> On 2018-05-15 12:22, Daniel Vetter wrote:
> > On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote:
> >> On 2018-05-14 18:28, Daniel Vetter wrote:
> >>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
> >>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
> >>>>> On 04.05.2018 15:52, Peter Rosin wrote:
> >>>>>> If the bridge supplier is unbound, this will bring the bridge consumer
> >>>>>> down along with the bridge. Thus, there will no longer linger any
> >>>>>> dangling pointers from the bridge consumer (the drm_device) to some
> >>>>>> non-existent bridge supplier.
> >>>>>>
> >>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
> >>>>>>  include/drm/drm_bridge.h     |  2 ++
> >>>>>>  2 files changed, 20 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >>>>>> index 78d186b6831b..0259f0a3ff27 100644
> >>>>>> --- a/drivers/gpu/drm/drm_bridge.c
> >>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
> >>>>>> @@ -26,6 +26,7 @@
> >>>>>>  #include <linux/mutex.h>
> >>>>>>
> >>>>>>  #include <drm/drm_bridge.h>
> >>>>>> +#include <drm/drm_device.h>
> >>>>>>  #include <drm/drm_encoder.h>
> >>>>>>
> >>>>>>  #include "drm_crtc_internal.h"
> >>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >>>>>>    if (bridge->dev)
> >>>>>>            return -EBUSY;
> >>>>>>
> >>>>>> +  if (encoder->dev->dev != bridge->odev) {
> >>>>>
> >>>>> I wonder why device_link_add does not handle this case (self dependency)
> >>>>> silently as noop, as it seems to be a correct behavior.
> >>>>
> >>>> It's kind-of a silly corner-case though, so perfectly understandable
> >>>> that it isn't handled.
> >>>>
> >>>>>> +          bridge->link = device_link_add(encoder->dev->dev,
> >>>>>> +                                         bridge->odev, 0);
> >>>>>> +          if (!bridge->link) {
> >>>>>> +                  dev_err(bridge->odev, "failed to link bridge to %s\n",
> >>>>>> +                          dev_name(encoder->dev->dev));
> >>>>>> +                  return -EINVAL;
> >>>>>> +          }
> >>>>>> +  }
> >>>>>> +
> >>>>>>    bridge->dev = encoder->dev;
> >>>>>>    bridge->encoder = encoder;
> >>>>>>
> >>>>>>    if (bridge->funcs->attach) {
> >>>>>>            ret = bridge->funcs->attach(bridge);
> >>>>>>            if (ret < 0) {
> >>>>>> +                  if (bridge->link)
> >>>>>> +                          device_link_del(bridge->link);
> >>>>>> +                  bridge->link = NULL;
> >>>>>>                    bridge->dev = NULL;
> >>>>>>                    bridge->encoder = NULL;
> >>>>>>                    return ret;
> >>>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >>>>>>    if (bridge->funcs->detach)
> >>>>>>            bridge->funcs->detach(bridge);
> >>>>>>
> >>>>>> +  if (bridge->link)
> >>>>>> +          device_link_del(bridge->link);
> >>>>>> +  bridge->link = NULL;
> >>>>>> +
> >>>>>>    bridge->dev = NULL;
> >>>>>>  }
> >>>>>>
> >>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >>>>>> index b656e505d11e..804189c63a4c 100644
> >>>>>> --- a/include/drm/drm_bridge.h
> >>>>>> +++ b/include/drm/drm_bridge.h
> >>>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
> >>>>>>   * @list: to keep track of all added bridges
> >>>>>>   * @timings: the timing specification for the bridge, if any (may
> >>>>>>   * be NULL)
> >>>>>> + * @link: drm consumer <-> bridge supplier
> >>>>>
> >>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> >>>>> to the bridge" would be better.
> >>>>
> >>>> I meant "<->" to indicate that the link is bidirectional, not that the
> >>>> relationship is in any way symmetric. I wasn't aware of any implication
> >>>> of a symmetric relationship when using "<->", do you have a reference?
> >>>> But I guess the different arrow notations in math are somewhat overloaded
> >>>> and that someone at some point must have used "<->" to indicate a
> >>>> symmetric relationship...
> >>>
> >>> Yeah I agree with Andrzej here, for me <-> implies a symmetric
> >>> relationship. Spelling it out like Andrzej suggested sounds like the
> >>> better idea.
> >>> -Daniel
> >>
> >> Ok, I guess that means I have to do a v3 after all. Or can this
> >> trivial documentation update be done by the committer? I hate to
> >> spam everyone with another volley...
> >>
> >> Or perhaps I should squash patches 2-23 that are all rather similar
> >> and mechanic? I separated them to allow for easier review from
> >> individual driver maintainers, but that didn't seem to happen
> >> anyway...
> > 
> > Do another volley of the full set, or in-reply-to to just replace the
> > patch that needs to be respun (but some people don't like that).
> > 
> > When resending just make sure you're picking up all the acks/r-bs you
> > have already.
> 
> Right, I always try to do that. One Ack that I did not include in v2
> was the one you had on v1 24/24 (i.e. this patch). The reason I did
> not add your Ack for v2 even on the patch where it obviously applied
> was that I didn't know if you'd barf on the odev name.
> 
> But it was (and still is) a bit unclear if that was on Ack on the
> last patch only, or if it was for the whole series? I think it might
> have been for the whole series, but I'm not sure and I hate to be a
> presumptuous idiot...

Ack on the overall concept, and I'm ok with odev too. But definitely get
acks from relevant people (bridge/driver maintainers). In terms of
patches, I'd say patch 1 + 24-26 have my

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

-Daniel

> 
> Cheers,
> Peter
> 
> > -Daniel
> >> Cheers,
> >> Peter
> >>
> >>>
> >>>>
> >>>>> Anyway:
> >>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> >>>>
> >>>> Thanks!
> >>>>
> >>>> Cheers,
> >>>> Peter
> >>>>
> >>>>>  --
> >>>>> Regards
> >>>>> Andrzej
> >>>>>
> >>>>>>   * @funcs: control functions
> >>>>>>   * @driver_private: pointer to the bridge driver's internal context
> >>>>>>   */
> >>>>>> @@ -271,6 +272,7 @@ struct drm_bridge {
> >>>>>>    struct drm_bridge *next;
> >>>>>>    struct list_head list;
> >>>>>>    const struct drm_bridge_timings *timings;
> >>>>>> +  struct device_link *link;
> >>>>>>
> >>>>>>    const struct drm_bridge_funcs *funcs;
> >>>>>>    void *driver_private;
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > 
> > 
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 78d186b6831b..0259f0a3ff27 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@ 
 #include <linux/mutex.h>
 
 #include <drm/drm_bridge.h>
+#include <drm/drm_device.h>
 #include <drm/drm_encoder.h>
 
 #include "drm_crtc_internal.h"
@@ -127,12 +128,25 @@  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (bridge->dev)
 		return -EBUSY;
 
+	if (encoder->dev->dev != bridge->odev) {
+		bridge->link = device_link_add(encoder->dev->dev,
+					       bridge->odev, 0);
+		if (!bridge->link) {
+			dev_err(bridge->odev, "failed to link bridge to %s\n",
+				dev_name(encoder->dev->dev));
+			return -EINVAL;
+		}
+	}
+
 	bridge->dev = encoder->dev;
 	bridge->encoder = encoder;
 
 	if (bridge->funcs->attach) {
 		ret = bridge->funcs->attach(bridge);
 		if (ret < 0) {
+			if (bridge->link)
+				device_link_del(bridge->link);
+			bridge->link = NULL;
 			bridge->dev = NULL;
 			bridge->encoder = NULL;
 			return ret;
@@ -159,6 +173,10 @@  void drm_bridge_detach(struct drm_bridge *bridge)
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
+	if (bridge->link)
+		device_link_del(bridge->link);
+	bridge->link = NULL;
+
 	bridge->dev = NULL;
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index b656e505d11e..804189c63a4c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -261,6 +261,7 @@  struct drm_bridge_timings {
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
+ * @link: drm consumer <-> bridge supplier
  * @funcs: control functions
  * @driver_private: pointer to the bridge driver's internal context
  */
@@ -271,6 +272,7 @@  struct drm_bridge {
 	struct drm_bridge *next;
 	struct list_head list;
 	const struct drm_bridge_timings *timings;
+	struct device_link *link;
 
 	const struct drm_bridge_funcs *funcs;
 	void *driver_private;