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

[Jani Nikula <jani.nikula@intel.com>]

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()? What if we just
moved it after the connector init? What are the in-kernel users that can
get called in between?

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?


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