On Tue, 29 Aug 2023, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> 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
