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 <[email protected]> > > > --- > > > 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
