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@xxxxxx> > --- > 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(). > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel