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 <[email protected]>
> > > > ---
> > > > 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
signature.asc
Description: PGP signature
