Re: [Intel-gfx] [PATCH 02/12] drm/sysfs: Register "ddc" symlink later

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

On Tue, 29 Aug 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently drm_sysfs_connector_add() attempts to register
> the "ddc" symlink (based one connector->ddc) before the
> driver's .early_register() hook has been called. That is
> too early for i915 which only fully registers the aux ch
> and associated i2c bus from said hook (to prevent half
> initialized stuff getting exposed to userspace). This
> causes my attempt at using drm_connector_init_with_ddc()
> to fail, and the entire connector disappears from sysfs
> on account of sysfs_create_link() failing.
>
> To fix that split the sysfs symlink stuff into separate
> functions (drm_sysfs_connector_add_late() and
> drm_sysfs_connector_remove_early()) which are called
> on the opposite side of the .later_register() and
> .early_unregister() hooks.
>
> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Emil Velikov <emil.velikov@collabora.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/drm_connector.c |  9 +++++++++
>  drivers/gpu/drm/drm_internal.h  |  2 ++
>  drivers/gpu/drm/drm_sysfs.c     | 22 +++++++++++++++-------
>  3 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 988996cf6da5..9d4c7b0c5c05 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -631,6 +631,10 @@ int drm_connector_register(struct drm_connector *connector)
>  			goto err_debugfs;
>  	}
>  
> +	ret = drm_sysfs_connector_add_late(connector);
> +	if (ret)
> +		goto err_late_register;
> +
>  	drm_mode_object_register(connector->dev, &connector->base);
>  
>  	connector->registration_state = DRM_CONNECTOR_REGISTERED;
> @@ -647,6 +651,9 @@ int drm_connector_register(struct drm_connector *connector)
>  	mutex_unlock(&connector_list_lock);
>  	goto unlock;
>  
> +err_late_register:
> +	if (connector->funcs->early_unregister)
> +		connector->funcs->early_unregister(connector);
>  err_debugfs:
>  	drm_debugfs_connector_remove(connector);
>  	drm_sysfs_connector_remove(connector);
> @@ -681,6 +688,8 @@ void drm_connector_unregister(struct drm_connector *connector)
>  					connector->privacy_screen,
>  					&connector->privacy_screen_notifier);
>  
> +	drm_sysfs_connector_remove_early(connector);
> +
>  	if (connector->funcs->early_unregister)
>  		connector->funcs->early_unregister(connector);
>  
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index ba12acd55139..4053cf8105ce 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -153,6 +153,8 @@ int drm_sysfs_init(void);
>  void drm_sysfs_destroy(void);
>  struct device *drm_sysfs_minor_alloc(struct drm_minor *minor);
>  int drm_sysfs_connector_add(struct drm_connector *connector);
> +int drm_sysfs_connector_add_late(struct drm_connector *connector);
> +void drm_sysfs_connector_remove_early(struct drm_connector *connector);
>  void drm_sysfs_connector_remove(struct drm_connector *connector);
>  
>  void drm_sysfs_lease_event(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index b169b3e44a92..a953f69a34b6 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -400,10 +400,6 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>  			drm_err(dev, "failed to add component to create link to typec connector\n");
>  	}
>  
> -	if (connector->ddc)
> -		return sysfs_create_link(&connector->kdev->kobj,
> -				 &connector->ddc->dev.kobj, "ddc");
> -
>  	return 0;
>  
>  err_free:
> @@ -411,14 +407,26 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>  	return r;
>  }
>  
> +int drm_sysfs_connector_add_late(struct drm_connector *connector)
> +{
> +	if (connector->ddc)
> +		return sysfs_create_link(&connector->kdev->kobj,
> +					 &connector->ddc->dev.kobj, "ddc");
> +
> +	return 0;
> +}
> +
> +void drm_sysfs_connector_remove_early(struct drm_connector *connector)
> +{
> +	if (connector->ddc)
> +		sysfs_remove_link(&connector->kdev->kobj, "ddc");
> +}
> +
>  void drm_sysfs_connector_remove(struct drm_connector *connector)
>  {
>  	if (!connector->kdev)
>  		return;
>  
> -	if (connector->ddc)
> -		sysfs_remove_link(&connector->kdev->kobj, "ddc");
> -
>  	if (dev_fwnode(connector->kdev))
>  		component_del(connector->kdev, &typec_connector_ops);

-- 
Jani Nikula, Intel Open Source Graphics Center