Re: [PATCH 1/4] drm/i915/dp_mst: Fix connector initialization in intel_dp_add_mst_connector()

[Imre Deak <imre.deak@intel.com>]

On Mon, Nov 18, 2024 at 11:10:18AM +0200, Jani Nikula wrote:
> On Fri, 15 Nov 2024, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, Nov 15, 2024 at 03:20:58PM -0500, Rodrigo Vivi wrote:
> >> On Fri, Nov 15, 2024 at 06:41:56PM +0200, Imre Deak wrote:
> >> > The connector initialization in intel_dp_add_mst_connector() depends on
> >> > the device pointer in connector to be valid, at least by connector
> >> > debug printing. The device pointer is initialized by drm_connector_init(),
> >> > however that function also exposes the connector to in-kernel users,
> >> > which can't be done before the connector is fully initialized. For now
> >> > make sure the device pointer is valid before it's used, until a
> >> > follow-up change moving this to DRM core.
> >> > 
> >> > This issue was revealed by the commit in the Fixes: line below, before
> >> > which the above debug printing checked and handled a NULL device pointer
> >> > gracefully in DRM core.
> >> > 
> >> > Cc: Jani Nikula <jani.nikula@intel.com>
> >> > Fixes: 529798bd786a ("drm/i915/mst: convert to struct intel_display")
> >> 
> >> This is awkward. This patch actually removes callers of base.dev.
> >> I don't see how that it could be causing this new null dereference.
> >
> > It adds
> >
> > struct intel_display *display = to_intel_display(connector);
> >
> > which will be NULL since connector->base.dev is NULL and later display
> > is dereferenced.
> 
> So this happens in detect_dsc_hblank_expansion_quirk()?
> 
> The changes were:
> 
> -       struct drm_i915_private *i915 = to_i915(connector->base.dev);
> +       struct intel_display *display = to_intel_display(connector);
> 
> -       drm_dbg_kms(&i915->drm,
> +       drm_dbg_kms(display->drm,
> 
> And apparently i915 and &i915->drm were both NULL before, but the change
> turned it into a NULL pointer dereference.
> 
> Why do we have to do this before drm_connector_init()?

drm_connector_init() adds the connector to the connector list, which
makes it visible to everything else that looks up the connector through
this list. Those users should see the driver specific parts of connector
already inited.

> What if we just moved it after the connector init? What are the
> in-kernel users that can get called in between?

Detection on this connector could happen in between for instance.

> Or if it's absolutely required to do all that before init, then pass the
> things to it instead of assuming the connector is ready?

Besides DSC all the other initializing steps in
intel_dp_add_mst_connector() should happen before adding it to the
connector list and those need a pointer to drm_connector.

> BR,
> Jani.
> 
> 
> >
> >> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12799
> >> 
> >> But well, trusting more the tests then my eyes, let's move forward.
> >> 
> >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> 
> >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++++
> >> >  1 file changed, 10 insertions(+)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >> > index df7edcfe885b6..f058360a26413 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >> > @@ -1727,6 +1727,16 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
> >> >  
> >> >  	intel_dp_init_modeset_retry_work(intel_connector);
> >> >  
> >> > +	/*
> >> > +	 * TODO: The following drm_connector specific initialization belongs
> >> > +	 * to DRM core, however it happens atm too late in
> >> > +	 * drm_connector_init(). That function will also expose the connector
> >> > +	 * to in-kernel users, so it can't be called until the connector is
> >> > +	 * sufficiently initialized; init the device pointer used by the
> >> > +	 * following DSC setup, until a fix moving this to DRM core.
> >> > +	 */
> >> > +	intel_connector->base.dev = mgr->dev;
> >> > +
> >> >  	intel_connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port);
> >> >  	intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, intel_connector);
> >> >  	intel_connector->dp.dsc_hblank_expansion_quirk =
> >> > -- 
> >> > 2.44.2
> >> > 
> 
> -- 
> Jani Nikula, Intel