Re: [PATCH 05/29] drm/atomic_state_helper: Fix bridge state initialization

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3252 bytes --]

On Mon, Sep 15, 2025 at 03:12:11PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 15.09.25 um 13:27 schrieb Maxime Ripard:
> > On Tue, Sep 02, 2025 at 03:18:17PM +0200, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 02.09.25 um 10:32 schrieb Maxime Ripard:
> > > > Bridges implement their state using a drm_private_obj and an
> > > > hand-crafted reset implementation.
> > > > 
> > > > Since drm_private_obj doesn't have a set of reset helper like the other
> > > > states, __drm_atomic_helper_bridge_reset() was initializing both the
> > > > drm_private_state and the drm_bridge_state structures.
> > > > 
> > > > This initialization however was missing the drm_private_state.obj
> > > > pointer to the drm_private_obj the state was allocated for, creating a
> > > > NULL pointer dereference when trying to access it.
> > > > 
> > > > Fixes: 751465913f04 ("drm/bridge: Add a drm_bridge_state object")
> > > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > > ---
> > > >    drivers/gpu/drm/drm_atomic_state_helper.c | 8 ++++++++
> > > >    1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > > index 7142e163e618ea0d7d9d828e1bd9ff2a6ec0dfeb..b962c342b16aabf4e3bea52a914e5deb1c2080ce 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > > @@ -707,10 +707,17 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
> > > >    	__drm_atomic_helper_connector_destroy_state(state);
> > > >    	kfree(state);
> > > >    }
> > > >    EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
> > > > +static void __drm_atomic_helper_private_obj_reset(struct drm_private_obj *obj,
> > > > +						  struct drm_private_state *state)
> > > > +{
> > > > +	memset(state, 0, sizeof(*state));
> > > This argument is guaranteed to be zero'd, I think. No need for a memset.
> > > 
> > > > +	state->obj = obj;
> > > > +}
> > > > +
> > > >    /**
> > > >     * __drm_atomic_helper_private_obj_duplicate_state - copy atomic private state
> > > >     * @obj: CRTC object
> > > >     * @state: new private object state
> > > >     *
> > > > @@ -796,10 +803,11 @@ EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);
> > > >     */
> > > >    void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,
> > > >    				      struct drm_bridge_state *state)
> > > >    {
> > > >    	memset(state, 0, sizeof(*state));
> > > Another unnecessary memset?
> > I guess the two can be seen as redundant, but I'd argue the one in
> > __drm_atomic_helper_private_obj_reset should still be there.
> > 
> > What guarantees that the pointer points to a zero'd structure?
> 
> We only call this helper after allocation AFAICT. And the DRM APIs already
> assume that allocation clears to zero.

Really? Do we have that documented anywhere?

And even then, there's nothing that requires that helper to be called
straight-away after allocation.

More importantly, do we really care about skipping them? Like, all the
other reset helpers are doing it, it's cheap, safe, why should we remove
it?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 269 bytes --]