On Tue, Feb 11, 2025 at 09:48:31AM +0100, Maxime Ripard wrote:
> On Tue, Feb 11, 2025 at 01:14:28AM +0200, Dmitry Baryshkov wrote:
> > On Mon, Feb 10, 2025 at 06:12:44PM +0100, Luca Ceresoli wrote:
> > > Hi Maxime, Dmitry
> > >
> > > On Fri, 7 Feb 2025 21:54:06 +0200
> > > Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > >
> > > > > > +/* Internal function (for refcounted bridges) */
> > > > > > +void __drm_bridge_free(struct kref *kref)
> > > > > > +{
> > > > > > + struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
> > > > > > + void *container = ((void *)bridge) - bridge->container_offset;
> > > > > > +
> > > > > > + DRM_DEBUG("bridge=%p, container=%p FREE\n", bridge, container);
> > > > >
> > > > > Pointers are not really useful to track here, since they are obfuscated
> > > > > most of the time. Using the bridge device name would probably be better
> > > > > (or removing the SHOUTING DEBUG entirely :))
> > > >
> > > > bridge device name or bridge funcs (I opted for the latter for the
> > > > debugfs file)
> > >
> > > These DRM_DEBUG()s proved extremely useful exactly because of the
> > > pointer. This is because when using hotplug one normally has the same
> > > device added and removed multiple times, and so the device name or
> > > bridge funcs is always the same, preventing from understanding which
> > > instance is leaking, or being freed, get, put, etc.
> > >
> > > Do you think this is a sufficient motivation to keep it?
> >
> > Then it should be something like %px. I found that %p is mangled.
> > What about having both device name _and_ a pointer?
>
> No, %px must not be used there. %p is mangled but should be consistent
> across calls. But yeah, it's kind of the reason I suggested to use the
> bridge device name instead.
Then we need to extend struct drm_bridge with struct device *dev (which
I would appreciate, it will solve whole hdmi_audio_dev / CEC device /
etc story).
--
With best wishes
Dmitry
