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

[Laurent Pinchart]

Hi Jyri,

On Wednesday 30 Nov 2016 10:55:10 Jyri Sarha wrote:
> On 11/30/16 10:37, Laurent Pinchart wrote:
> >>> 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.
> 
> That could be avoided if of_drm_find_bridge() would copy the content of
> bridge object, in stead of providing a pointer.

/me shivers

> The attach call could then search for the bridge object again before
> continuing. But still in the end the impact to individual drivers would be
> equal to adding a simple drm_bridge_put() call. Probably better to go
> forward with your suggestion.

Please :-)

> Even adding the drm_bridge_put() would not be necessary in most cases if
> we would add a devm version of getting the bridge object. In 99% of
> cases the device probe will fail if the bridge attach fails, and bridge
> object refcount would return to zero automatically.

devm_* is evil in most cases, especially the devm_kzalloc() function as it 
ties object lifetime to devices, releasing memory at unbind time when the 
object can still be referenced elsewhere. Regarding bridges, a 
devm_of_drm_find_bridge() might make sense as the bridge seems at first glance 
to (nearly) always be a resource that can be released at DRM unbind time.

-- 
Regards,

Laurent Pinchart