Re: [PATCH 29/30] drm/bridge: add support for device links to bridge

[Mihail Atanassov <Mihail.Atanassov@arm.com>]

On Tuesday, 26 November 2019 14:35:34 GMT Daniel Vetter wrote:
> On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
> > From: Russell King <rmk+kernel@armlinux.org.uk>
> > 
> > Bridge devices have been a potential for kernel oops as their lifetime
> > is independent of the DRM device that they are bound to.  Hence, if a
> > bridge device is unbound while the parent DRM device is using them, the
> > parent happily continues to use the bridge device, calling the driver
> > and accessing its objects that have been freed.
> > 
> > This can cause kernel memory corruption and kernel oops.
> > 
> > To control this, use device links to ensure that the parent DRM device
> > is unbound when the bridge device is unbound, and when the bridge
> > device is re-bound, automatically rebind the parent DRM device.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Tested-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > [reworked to use drm_bridge_init() for setting bridge->device]
> > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> 
> So I thought the big plan was to put the device_link setup into
> drm_bridge_attach, so that it's done for everyone. And we could then
> slowly go through the existing drivers that use the component framework to
> get this handled correctly.
> 
> So my questions:
> - is there a problem if we add the device_link for everyone?

Possibly not, but I didn't want to stir the entire pot :). This is
safer in the sense that it's an opt-in, so a lower chance of
regressions in code and setups that I can't possibly test. If you
think it's worth sticking it in the existing code paths, I can
certainly do that too.

> - is there an issue if we only add it at drm_bridge_attach time? I kinda
>   assumed that it's not needed before that (EPROBE_DEFER should handle
>   load dependencies as before), but it could be that some drivers ask for
>   a bridge and then check more stuff and then drop the bridge without
>   calling drm_bridge_attach. We probably don't have a case like this yet,
>   but better robust than sorry.

I think there would be a race there:

- bridge driver calls drm_bridge_add() in their probe()
- client driver calls of_drm_find_bridge() in their probe()
- bridge driver gets removed, calls drm_bridge_remove()
- client driver uses the now invalid drm_bridge pointer from above to
  do drm_bridge_attach()

With of_drm_bridge_find_devlink(), you get the device_link inside the
bridge_lock so the reference to the drm_bridge will either be valid, or
your driver gets removed right after it's probed, so that the bridge
can be removed, too.

In patch 30/30 I use both the _devlink and the non-_devlink versions
of of_drm_find_bridge(), but I guess there's no harm adding another
refcount on the link, it'll get destroyed if the bridge is removed
regardless, although that may need a DL_FLAG_AUTOREMOVE_CONSUMER.

> 
> Anyway, I scrolled through the bridge patches, looked all good, huge
> thanks for tackling this! Once we have some agreement on the bigger
> questions here I'll try to go through them and review.
> 
> Cheers, Daniel
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++----------
> >  include/drm/drm_bridge.h     |  4 +++
> >  2 files changed, 40 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index cbe680aa6eac..e1f8db84651a 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"
> > @@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> >  	bridge->encoder = NULL;
> >  	bridge->next = NULL;
> >  
> > +	bridge->device = dev;
> >  #ifdef CONFIG_OF
> >  	bridge->of_node = dev->of_node;
> >  #endif
> > @@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,
> >  EXPORT_SYMBOL(drm_atomic_bridge_enable);
> >  
> >  #ifdef CONFIG_OF
> > +static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
> > +					  struct device_node *np, bool link)
> > +{
> > +	struct drm_bridge *bridge, *found = NULL;
> > +	struct device_link *dl;
> > +
> > +	mutex_lock(&bridge_lock);
> > +
> > +	list_for_each_entry(bridge, &bridge_list, list)
> > +		if (bridge->of_node == np) {
> > +			found = bridge;
> > +			break;
> > +		}
> > +
> > +	if (found && link) {
> > +		dl = device_link_add(dev->dev, found->device,
> > +				     DL_FLAG_AUTOPROBE_CONSUMER);
> > +		if (!dl)mutex
> > +			found = NULL;
> > +	}
> > +
> > +	mutex_unlock(&bridge_lock);
> > +
> > +	return found;
> > +}
> > +
> >  /**
> >   * of_drm_find_bridge - find the bridge corresponding to the device node in
> >   *			the global bridge list
> > @@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable);
> >   */
> >  struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> >  {
> > -	struct drm_bridge *bridge;
> > -
> > -	mutex_lock(&bridge_lock);
> > -
> > -	list_for_each_entry(bridge, &bridge_list, list) {
> > -		if (bridge->of_node == np) {
> > -			mutex_unlock(&bridge_lock);
> > -			return bridge;
> > -		}
> > -	}
> > -
> > -	mutex_unlock(&bridge_lock);
> > -	return NULL;
> > +	return drm_bridge_find(NULL, np, false);
> >  }
> >  EXPORT_SYMBOL(of_drm_find_bridge);
> > +
> > +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> > +					      struct device_node *np)
> > +{
> > +	return drm_bridge_find(dev, np, true);
> > +}
> > +EXPORT_SYMBOL(of_drm_find_bridge_devlink);
> >  #endif
> >  
> >  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index d6d9d5301551..68b27c69cc3d 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -382,6 +382,8 @@ struct drm_bridge {
> >  	struct drm_encoder *encoder;
> >  	/** @next: the next bridge in the encoder chain */
> >  	struct drm_bridge *next;
> > +	/** @device: Linux driver model device */
> > +	struct device *device;
> >  #ifdef CONFIG_OF
> >  	/** @of_node: device node pointer to the bridge */
> >  	struct device_node *of_node;
> > @@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> >  		     const struct drm_bridge_timings *timings,
> >  		     void *driver_private);
> >  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> > +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> > +					      struct device_node *np);
> >  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  		      struct drm_bridge *previous);
> >  
> 
> 


-- 
Mihail