Re: [PATCH 05/16] drm/dp_mst: Switch private_obj initialization to reset

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 08, 2025 at 04:53:20PM +0200, Maxime Ripard wrote:
> On Wed, Oct 08, 2025 at 05:06:43PM +0300, Ville Syrjälä wrote:
> > On Wed, Oct 08, 2025 at 02:04:03PM +0200, Maxime Ripard wrote:
> > > The DP MST implementation relies on a drm_private_obj, that is
> > > initialized by allocating and initializing a state, and then passing it
> > > to drm_private_obj_init.
> > > 
> > > Since we're gradually moving away from that pattern to the more
> > > established one relying on a reset implementation, let's migrate this
> > > instance to the new pattern.
> > > 
> > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > ---
> > >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 39 ++++++++++++++++++---------
> > >  1 file changed, 26 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > index 64e5c176d5cce9df9314f77a0b4c97662c30c070..255fbdcea9f0b6376d15439e3da1dc02be472a20 100644
> > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > @@ -5181,10 +5181,34 @@ static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,
> > >  
> > >  	kfree(mst_state->commit_deps);
> > >  	kfree(mst_state);
> > >  }
> > >  
> > > +static void drm_dp_mst_reset(struct drm_private_obj *obj)
> > > +{
> > > +	struct drm_dp_mst_topology_mgr *mgr =
> > > +		to_dp_mst_topology_mgr(obj);
> > > +	struct drm_dp_mst_topology_state *mst_state;
> > > +
> > > +	if (obj->state) {
> > > +		drm_dp_mst_destroy_state(obj, obj->state);
> > > +		obj->state = NULL;
> > 
> > I'm not a big fan of this "free+reallocate without any way to handle
> > failures" pattern.
> > 
> > Currently i915 does not do any of this, and so there are no unhandled
> > points of failure. But the mst and tunneling changes here force it on
> > i915 as well.
> 
> A very theoretical point of failure. If I'm counting right,
> drm_dp_mst_topology_state takes 72 bytes. Any GFP_KERNEL allocation
> of less than 8 pages cannot fail.

Until you start to inject failures, or you blow up the size of the
state. You wouldn't think to check for potential NULL pointer
dereferences before doing that, nor should you have to.

I'd rather not leave potential footguns lying around intentionally.

> So, sure, it's less satisfying to look at, but that's about it. It's
> just as safe and reliable.
> 
> > I think for the common things it would be nicer to just implement
> > the reset as just that "a reset of the current state", and leave
> > the "let's play fast and loose with kmalloc() failures" to the
> > drivers that want it.
> 
> I'm sorry, but I have no idea what you mean by that. It's using the
> exact same pattern we've been using since forever for every driver
> (except, apparently, i915).

The reset() stuff was added specifically to deal with drivers
that didn't have readout. i915 has always had full readout and
never needed it.

> What would you like to change to that pattern to accomodate i915
> requirements?

I think the reset should be ideally done without reallocation,
or perhaps just don't implement reset for the things that don't
need it (mst and tunneling in this case).

-- 
Ville Syrjälä
Intel