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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Andrzej,

On Wednesday 30 Nov 2016 09:11:56 Andrzej Hajda wrote:
> On 29.11.2016 22:12, 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.
> 
> Module refcounting as a way of preventing driver from unbinding
> is quite popular, but as other developers said already is incorrect
> solution - it does not really prevent from unbinding and is a hack.

Absolutely, module refcounting must not be used as a way to prevent unbinding, 
but it's still necessary to prevent functions from disappearing. The unbinding 
race has to be fixed through reference counting to prevent objects from being 
freed, but if an object contains function pointers that refer to code part of 
a module, object refcounting won't prevent the code from being removed. Only 
module refcounting helps there. The two techniques are thus complementary.

> > Signed-off-by: Jyri Sarha <jsarha@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;
> > +
> 
> module field can be missing, will it compile in such case?
> 
> >  	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
> 
> If I remember correctly THIS_MODULE is NULL if MODULE is undefined.
> So the whole #ifdef here is unnecessary.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel