[PATCH] drm/drm_bridge: adjust bridge module's refcount

[Daniel Vetter]

On Wed, Nov 30, 2016 at 12:06:25AM +0200, Laurent Pinchart wrote:
> Hi Jyri,
> 
> Thank you for the patch.
> 
> On Tuesday 29 Nov 2016 23:12:56 Jyri Sarha wrote:
> > Store the module that provides the bridge and adjust its refcount
> > accordingly. The bridge module unload should not be allowed while the
> > bridge is attached.
> > 
> > Signed-off-by: Jyri Sarha <jsarha at ti.com>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 15 ++++++++++++---
> >  include/drm/drm_bridge.h     | 11 ++++++++++-
> >  2 files changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 0ee052b..36d427b 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -61,22 +61,26 @@
> >  static LIST_HEAD(bridge_list);
> > 
> >  /**
> > - * drm_bridge_add - add the given bridge to the global bridge list
> > + * __drm_bridge_add - add the given bridge to the global bridge list
> >   *
> >   * @bridge: bridge control structure
> > + * @module: module that provides this bridge
> >   *
> >   * RETURNS:
> >   * Unconditionally returns Zero.
> >   */
> > -int drm_bridge_add(struct drm_bridge *bridge)
> > +int __drm_bridge_add(struct drm_bridge *bridge, struct module *module)
> >  {
> > +#ifdef MODULE
> > +	bridge->module = module;
> > +#endif
> >  	mutex_lock(&bridge_lock);
> >  	list_add_tail(&bridge->list, &bridge_list);
> >  	mutex_unlock(&bridge_lock);
> > 
> >  	return 0;
> >  }
> > -EXPORT_SYMBOL(drm_bridge_add);
> > +EXPORT_SYMBOL(__drm_bridge_add);
> > 
> >  /**
> >   * drm_bridge_remove - remove the given bridge from the global bridge list
> > @@ -114,6 +118,9 @@ int drm_bridge_attach(struct drm_device *dev, struct
> > drm_bridge *bridge) if (bridge->dev)
> >  		return -EBUSY;
> > 
> > +	if (!try_module_get(bridge->module))
> > +		return -ENOENT;
> 
> Isn't this still racy ? What happens if the module is removed right before 
> this call ? Won't the bridge object be freed, and this code then try to call 
> try_module_get() on freed memory ?
> 
> To fix this properly I think we need to make the bridge object refcounted, 
> with a release callback to signal to the bridge driver that memory can be 
> freed. The refcount should be increased in of_drm_find_bridge(), and decreased 
> in a new drm_bridge_put() function (the "fun" part will be to update drivers 
> to call that :-S).
> 
> The module refcount still needs to be increased in drm_bridge_attach() like 
> you do here, but you'll need to protect it with bridge_lock to avoid a race 
> between try_module_get() and drm_bridge_remove().

Hm right, I thought _attach is the function called directly, but we do
lookup+attach. Might be good to have an of helper which does the
lookup+attach and then drops the reference. We can do that as long as
attach/detach holds onto a reference like the one below.

And I don't think we need a new bridge refcount, we can just us
try_module_get/put for that (but wrapped into helpers ofc).
-Daniel

> 
> >  	bridge->dev = dev;
> > 
> >  	if (bridge->funcs->attach)
> > @@ -144,6 +151,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >  	if (bridge->funcs->detach)
> >  		bridge->funcs->detach(bridge);
> > 
> > +	module_put(bridge->module);
> > +
> >  	bridge->dev = NULL;
> >  }
> >  EXPORT_SYMBOL(drm_bridge_detach);
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 530a1d6..d60d5d2 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -25,6 +25,7 @@
> > 
> >  #include <linux/list.h>
> >  #include <linux/ctype.h>
> > +#include <linux/module.h>
> >  #include <drm/drm_mode_object.h>
> >  #include <drm/drm_modes.h>
> > 
> > @@ -192,13 +193,21 @@ struct drm_bridge {
> >  #ifdef CONFIG_OF
> >  	struct device_node *of_node;
> >  #endif
> > +#ifdef MODULE
> > +	struct module *module;
> > +#endif
> >  	struct list_head list;
> > 
> >  	const struct drm_bridge_funcs *funcs;
> >  	void *driver_private;
> >  };
> > 
> > -int drm_bridge_add(struct drm_bridge *bridge);
> > +int __drm_bridge_add(struct drm_bridge *bridge, struct module *module);
> > +#ifdef MODULE
> > +#define drm_bridge_add(bridge) __drm_bridge_add(bridge, THIS_MODULE)
> > +#else
> > +#define drm_bridge_add(bridge) __drm_bridge_add(bridge, NULL)
> > +#endif
> >  void drm_bridge_remove(struct drm_bridge *bridge);
> >  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> >  int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch