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

[Laurent Pinchart]

Hi Jyri,

On Wednesday 30 Nov 2016 11:13:41 Jyri Sarha wrote:
> On 11/30/16 11:03, Laurent Pinchart wrote:
> >>> 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 :-)
> 
> Ok :). But first I'll make a pull request of all accumulated tilcdc
> patches, as all problems I have found in them are to be fixed outside
> tilcdc.

Sure.

> >>> 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.
>
> But the devm is not at all as evil when the object is refcounted,
> because every piece of code that wants to keep a reference to the object
> can keep it.

devm_* is fine when allocating a resources that is release at device unbind 
time. I/O regions, IRQ, and most probably bridges (I'd have to sleep over that 
last one) should fall in that category, at least most of the time. 
devm_kzalloc() is evil in the sense that many driver authors use it as a 
solution to world hunger without thinking twice, while memory allocated by 
drivers for objects that are registered and must outlive the lifetime of the 
device binding is not a resource consumed by the driver that can be released 
at unbind time.

-- 
Regards,

Laurent Pinchart