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

[Laurent Pinchart]

Hi Jyri,

On Wednesday 30 Nov 2016 10:32:40 Jyri Sarha wrote:
> On 11/30/16 00:06, Laurent Pinchart wrote:
> >> >  /**
> >> >   * 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).
> 
> I think another option would be to find and attach the bridge object
> atomically by holding the bridge_lock until the try_module_get() has
> succeeded.
> 
> AFAIU, if the module unload is triggered while holding the bridge_lock
> but before the try_module_get() call, then try_module_get() would return
> false and the attach call will return failure.

This would require combining lookup and attach in all cases. I'm not sure that 
would be very convenient for drivers. Keeping the two operations separate 
would be more flexible.

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

-- 
Regards,

Laurent Pinchart