[PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps

[Maxime Ripard]

On Fri, Nov 29, 2024 at 06:12:01PM +0200, Imre Deak wrote:
> On Fri, Nov 29, 2024 at 03:46:28PM +0100, Maxime Ripard wrote:
> > On Fri, Nov 29, 2024 at 04:26:01PM +0200, Imre Deak wrote:
> > > Adding more people from DRM core domain. Any comment, objection to this
> > > change?
> > > 
> > > On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote:
> > > > Atm when the connector is added to the drm_mode_config::connector_list,
> > > > the connector may not be fully initialized yet. This is not a problem
> > > > for user space, which will see the connector only after it's registered
> > > > later, it could be a problem for in-kernel users looking up connectors
> > > > via the above list.
> > 
> > It could be, or it actually is a problem? If so, in what situation?
> 
> An actual problem is that after an MST connector is created and added to
> the connector list, the connector could be probed without the connector
> being fully initialized during a hotplug event handling along with all
> the other connectors on the list. The connector's helper functions could
> be still unset leading to a NULL deref while trying to call the
> connector's detect/detect_ctx callbacks, or if these callbacks are set
> already, the detect handler could see a connector which is not yet
> initialized fully.

Ack. Like I said to Jani, this needs to be in the commit log then.

> > > > To resolve the above issue, add a way to separately initialize the DRM
> > > > core specific parts of the connector and add it to the above list. This
> > > > will move adding the connector to the list after the properties on the
> > > > connector have been added, this is ok since these steps don't have a
> > > > dependency.
> > > >
> > > > v2: (Jani)
> > > > - Let initing DDC as well via drm_connector_init_core().
> > > > - Rename __drm_connector_init to drm_connector_init_core_and_add().
> > > > 
> > > > Cc: Jani Nikula <jani.nikula at intel.com>
> > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com> (v1)
> > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > 
> > If we do have an actual problem to solve, we'll need kunit tests too.
> 
> I don't have a good idea for this. The problem is not in a parituclar
> function, rather in the order a connector is initialized and added to
> the connector list. The above is an actual problem though, so I don't
> think fixing that should be blocked by this.

It's not about whether we have a problem or not: you introduce new
framework functions, you need to have kunit tests to check their
behaviour.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20241202/3f9487ab/attachment.sig>