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

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

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@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().

>  	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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel