On Wed, Oct 08, 2025 at 07:12:28PM +0300, Ville Syrjälä wrote:
> 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 <mrip...@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.I'm not sure it's reasonable to expect a structure to grow by three orders of magnitude, but we can add static build checks on the size of the structure if it makes you more comfortable. > > 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). I'll be leaving aside i915 because it's quite far from actually using all that infrastructure. From what I understand, your main concern is that the drm_private_obj would be reset during a suspend, and the DP objs should actually persist. Thomas has been floating the idea recently to create a new helper to supersede reset which would only reset the state itself, and not the hardware. If we're doing that split, I guess it would mean that we would need a new callback to allocate the initial state, since reset does both. Would you feel better about creating an atomic_create_state hook or something? Maxime
signature.asc
Description: PGP signature
