[PATCH 0/6] drm: Series with core improvements/refactorings

[PATCH 0/6] drm: Series with core improvements/refactorings

[Heiner Kallweit]

Series with DRM core improvements/refactorings.

Heiner Kallweit (6):
  drm/sysfs: Remove version attribute
  drm/sysfs: Drop unused drm_class_device_(un)register
  drm: Refactor drm_core_init error path
  drm: Change drm_class from pointer to const struct class
  drm: Add __init annotations
  drm/sysfs: Remove device type drm_minor

 drivers/accel/drm_accel.c                |  2 +-
 drivers/gpu/drm/drm_cache.c              |  2 +-
 drivers/gpu/drm/drm_connector.c          |  2 +-
 drivers/gpu/drm/drm_drv.c                | 18 +++--
 drivers/gpu/drm/drm_internal.h           |  2 +-
 drivers/gpu/drm/drm_panic.c              |  4 +-
 drivers/gpu/drm/drm_privacy_screen.c     |  2 +-
 drivers/gpu/drm/drm_privacy_screen_x86.c |  2 +-
 drivers/gpu/drm/drm_sysfs.c              | 89 +++++-------------------
 include/drm/drm_sysfs.h                  |  3 -
 10 files changed, 39 insertions(+), 87 deletions(-)

-- 
2.46.0

[PATCH 1/6] drm/sysfs: Remove version attribute

[Heiner Kallweit]

This undocumented attribute returns a version string which hasn't been
changed for ages. libdrm doesn't use it and I also found no other user.
So I think we can remove it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/gpu/drm/drm_sysfs.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index fb3bbb6ad..49e5faf11 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = {
 	.unbind = typec_connector_unbind,
 };
 
-static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
-
 /**
  * drm_sysfs_init - initialize sysfs helpers
  *
@@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
  */
 int drm_sysfs_init(void)
 {
-	int err;
-
 	drm_class = class_create("drm");
 	if (IS_ERR(drm_class))
 		return PTR_ERR(drm_class);
 
-	err = class_create_file(drm_class, &class_attr_version.attr);
-	if (err) {
-		class_destroy(drm_class);
-		drm_class = NULL;
-		return err;
-	}
-
 	drm_class->devnode = drm_devnode;
 
 	drm_sysfs_acpi_register();
@@ -169,7 +158,6 @@ void drm_sysfs_destroy(void)
 	if (IS_ERR_OR_NULL(drm_class))
 		return;
 	drm_sysfs_acpi_unregister();
-	class_remove_file(drm_class, &class_attr_version.attr);
 	class_destroy(drm_class);
 	drm_class = NULL;
 }
-- 
2.46.0

[PATCH 2/6] drm/sysfs: Drop unused drm_class_device_(un)register

[Heiner Kallweit]

These two functions have no user, so remove them.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/gpu/drm/drm_sysfs.c | 32 --------------------------------
 include/drm/drm_sysfs.h     |  3 ---
 2 files changed, 35 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 49e5faf11..a713f0500 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -536,35 +536,3 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
 	put_device(kdev);
 	return ERR_PTR(r);
 }
-
-/**
- * drm_class_device_register - register new device with the DRM sysfs class
- * @dev: device to register
- *
- * Registers a new &struct device within the DRM sysfs class. Essentially only
- * used by ttm to have a place for its global settings. Drivers should never use
- * this.
- */
-int drm_class_device_register(struct device *dev)
-{
-	if (!drm_class || IS_ERR(drm_class))
-		return -ENOENT;
-
-	dev->class = drm_class;
-	return device_register(dev);
-}
-EXPORT_SYMBOL_GPL(drm_class_device_register);
-
-/**
- * drm_class_device_unregister - unregister device with the DRM sysfs class
- * @dev: device to unregister
- *
- * Unregisters a &struct device from the DRM sysfs class. Essentially only used
- * by ttm to have a place for its global settings. Drivers should never use
- * this.
- */
-void drm_class_device_unregister(struct device *dev)
-{
-	return device_unregister(dev);
-}
-EXPORT_SYMBOL_GPL(drm_class_device_unregister);
diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h
index 96a5d8584..929d957e4 100644
--- a/include/drm/drm_sysfs.h
+++ b/include/drm/drm_sysfs.h
@@ -7,9 +7,6 @@ struct device;
 struct drm_connector;
 struct drm_property;
 
-int drm_class_device_register(struct device *dev);
-void drm_class_device_unregister(struct device *dev);
-
 void drm_sysfs_hotplug_event(struct drm_device *dev);
 void drm_sysfs_connector_hotplug_event(struct drm_connector *connector);
 void drm_sysfs_connector_property_event(struct drm_connector *connector,
-- 
2.46.0

[PATCH 3/6] drm: Refactor drm_core_init error path

[Heiner Kallweit]

These changes make the error path a little more robust, because exit
steps in drm_core_exit() don't have to ensure any longer that they
work properly even if the associated init step wasn't executed.
In addition these changes allow to annotate a few functions as __exit,
saving some memory if drm is built-in.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/accel/drm_accel.c                |  2 +-
 drivers/gpu/drm/drm_drv.c                | 18 ++++++++++++------
 drivers/gpu/drm/drm_panic.c              |  4 ++--
 drivers/gpu/drm/drm_privacy_screen_x86.c |  2 +-
 drivers/gpu/drm/drm_sysfs.c              |  2 --
 5 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
index aa826033b..25fdbea36 100644
--- a/drivers/accel/drm_accel.c
+++ b/drivers/accel/drm_accel.c
@@ -191,7 +191,7 @@ static const struct file_operations accel_stub_fops = {
 	.llseek = noop_llseek,
 };
 
-void accel_core_exit(void)
+void __exit accel_core_exit(void)
 {
 	unregister_chrdev(ACCEL_MAJOR, "accel");
 	debugfs_remove(accel_debugfs_root);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index ac30b0ec9..ea59994e5 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -1062,7 +1062,7 @@ static const struct file_operations drm_stub_fops = {
 	.llseek = noop_llseek,
 };
 
-static void drm_core_exit(void)
+static void __exit drm_core_exit(void)
 {
 	drm_privacy_screen_lookup_exit();
 	drm_panic_exit();
@@ -1084,18 +1084,18 @@ static int __init drm_core_init(void)
 	ret = drm_sysfs_init();
 	if (ret < 0) {
 		DRM_ERROR("Cannot create DRM class: %d\n", ret);
-		goto error;
+		goto err_ida;
 	}
 
 	drm_debugfs_root = debugfs_create_dir("dri", NULL);
 
 	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
 	if (ret < 0)
-		goto error;
+		goto err_debugfs;
 
 	ret = accel_core_init();
 	if (ret < 0)
-		goto error;
+		goto err_chrdev;
 
 	drm_panic_init();
 
@@ -1106,8 +1106,14 @@ static int __init drm_core_init(void)
 	DRM_DEBUG("Initialized\n");
 	return 0;
 
-error:
-	drm_core_exit();
+err_chrdev:
+	unregister_chrdev(DRM_MAJOR, "drm");
+err_debugfs:
+	debugfs_remove(drm_debugfs_root);
+	drm_sysfs_destroy();
+err_ida:
+	WARN_ON(!xa_empty(&drm_minors_xa));
+	drm_connector_ida_destroy();
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 74412b7bf..d00fdb12d 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -679,7 +679,7 @@ static void __init drm_panic_qr_init(void)
 				   GFP_KERNEL);
 }
 
-static void drm_panic_qr_exit(void)
+static void __exit drm_panic_qr_exit(void)
 {
 	kfree(qrbuf1);
 	qrbuf1 = NULL;
@@ -1058,7 +1058,7 @@ void __init drm_panic_init(void)
 /**
  * drm_panic_exit() - Free the resources taken by drm_panic_exit()
  */
-void drm_panic_exit(void)
+void __exit drm_panic_exit(void)
 {
 	drm_panic_qr_exit();
 }
diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
index 72ed40e49..6be96a0cc 100644
--- a/drivers/gpu/drm/drm_privacy_screen_x86.c
+++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
@@ -98,7 +98,7 @@ void __init drm_privacy_screen_lookup_init(void)
 	}
 }
 
-void drm_privacy_screen_lookup_exit(void)
+void __exit drm_privacy_screen_lookup_exit(void)
 {
 	if (arch_lookup.provider)
 		drm_privacy_screen_lookup_remove(&arch_lookup);
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index a713f0500..f8577043e 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -155,8 +155,6 @@ int drm_sysfs_init(void)
  */
 void drm_sysfs_destroy(void)
 {
-	if (IS_ERR_OR_NULL(drm_class))
-		return;
 	drm_sysfs_acpi_unregister();
 	class_destroy(drm_class);
 	drm_class = NULL;
-- 
2.46.0

[PATCH 4/6] drm: Change drm_class from pointer to const struct class

[Heiner Kallweit]

Define class drm statically and constify it. This ensure that no user
of the exported struct class can tamper with it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/gpu/drm/drm_internal.h       |  2 +-
 drivers/gpu/drm/drm_privacy_screen.c |  2 +-
 drivers/gpu/drm/drm_sysfs.c          | 32 ++++++++++++++--------------
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 1705bfc90..6e0df44b6 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -139,7 +139,7 @@ bool drm_master_internal_acquire(struct drm_device *dev);
 void drm_master_internal_release(struct drm_device *dev);
 
 /* drm_sysfs.c */
-extern struct class *drm_class;
+extern const struct class drm_class;
 
 int drm_sysfs_init(void);
 void drm_sysfs_destroy(void);
diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c
index 6cc39e307..2fbd24ba5 100644
--- a/drivers/gpu/drm/drm_privacy_screen.c
+++ b/drivers/gpu/drm/drm_privacy_screen.c
@@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register(
 	mutex_init(&priv->lock);
 	BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head);
 
-	priv->dev.class = drm_class;
+	priv->dev.class = &drm_class;
 	priv->dev.type = &drm_privacy_screen_type;
 	priv->dev.parent = parent;
 	priv->dev.release = drm_privacy_screen_device_release;
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index f8577043e..f443f9a76 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -58,7 +58,15 @@ static struct device_type drm_sysfs_device_connector = {
 	.name = "drm_connector",
 };
 
-struct class *drm_class;
+static char *drm_devnode(const struct device *dev, umode_t *mode)
+{
+	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
+}
+
+const struct class drm_class = {
+	.name = "drm",
+	.devnode = drm_devnode,
+};
 
 #ifdef CONFIG_ACPI
 static bool drm_connector_acpi_bus_match(struct device *dev)
@@ -93,11 +101,6 @@ static void drm_sysfs_acpi_register(void) { }
 static void drm_sysfs_acpi_unregister(void) { }
 #endif
 
-static char *drm_devnode(const struct device *dev, umode_t *mode)
-{
-	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
-}
-
 static int typec_connector_bind(struct device *dev,
 				struct device *typec_connector, void *data)
 {
@@ -138,14 +141,12 @@ static const struct component_ops typec_connector_ops = {
  */
 int drm_sysfs_init(void)
 {
-	drm_class = class_create("drm");
-	if (IS_ERR(drm_class))
-		return PTR_ERR(drm_class);
+	int ret = class_register(&drm_class);
 
-	drm_class->devnode = drm_devnode;
+	if (!ret)
+		drm_sysfs_acpi_register();
 
-	drm_sysfs_acpi_register();
-	return 0;
+	return ret;
 }
 
 /**
@@ -156,8 +157,7 @@ int drm_sysfs_init(void)
 void drm_sysfs_destroy(void)
 {
 	drm_sysfs_acpi_unregister();
-	class_destroy(drm_class);
-	drm_class = NULL;
+	class_unregister(&drm_class);
 }
 
 static void drm_sysfs_release(struct device *dev)
@@ -337,7 +337,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 		return -ENOMEM;
 
 	device_initialize(kdev);
-	kdev->class = drm_class;
+	kdev->class = &drm_class;
 	kdev->type = &drm_sysfs_device_connector;
 	kdev->parent = dev->primary->kdev;
 	kdev->groups = connector_dev_groups;
@@ -516,7 +516,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
 			minor_str = "card%d";
 
 		kdev->devt = MKDEV(DRM_MAJOR, minor->index);
-		kdev->class = drm_class;
+		kdev->class = &drm_class;
 		kdev->type = &drm_sysfs_device_minor;
 	}
 
-- 
2.46.0

[PATCH 5/6] drm: Add __init annotations

[Heiner Kallweit]

Annotate few more functions being called from drm_core_init() only
as __init.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/gpu/drm/drm_cache.c     | 2 +-
 drivers/gpu/drm/drm_connector.c | 2 +-
 drivers/gpu/drm/drm_sysfs.c     | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
index 7051c9c90..dac790108 100644
--- a/drivers/gpu/drm/drm_cache.c
+++ b/drivers/gpu/drm/drm_cache.c
@@ -329,7 +329,7 @@ EXPORT_SYMBOL(drm_memcpy_from_wc);
 /*
  * drm_memcpy_init_early - One time initialization of the WC memcpy code
  */
-void drm_memcpy_init_early(void)
+void __init drm_memcpy_init_early(void)
 {
 	/*
 	 * Some hypervisors (e.g. KVM) don't support VEX-prefix instructions
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index fc35f47e2..96b234af7 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -112,7 +112,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
 	{ DRM_MODE_CONNECTOR_USB, "USB" },
 };
 
-void drm_connector_ida_init(void)
+void __init drm_connector_ida_init(void)
 {
 	int i;
 
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index f443f9a76..b73c589c5 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -87,7 +87,7 @@ static struct acpi_bus_type drm_connector_acpi_bus = {
 	.find_companion = drm_connector_acpi_find_companion,
 };
 
-static void drm_sysfs_acpi_register(void)
+static void __init drm_sysfs_acpi_register(void)
 {
 	register_acpi_bus_type(&drm_connector_acpi_bus);
 }
@@ -139,7 +139,7 @@ static const struct component_ops typec_connector_ops = {
  *
  * Return: 0 on success, negative error code on failure.
  */
-int drm_sysfs_init(void)
+int __init drm_sysfs_init(void)
 {
 	int ret = class_register(&drm_class);
 
-- 
2.46.0

[PATCH 6/6] drm: drm/sysfs: Remove device type drm_minor

[Heiner Kallweit]

This device type is set but not used, so remove it.
Whilst we're at it, constify device type drm_connector.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/gpu/drm/drm_sysfs.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index b73c589c5..45a1c864a 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -50,11 +50,7 @@
  * drm_connector_unregister().
  */
 
-static struct device_type drm_sysfs_device_minor = {
-	.name = "drm_minor"
-};
-
-static struct device_type drm_sysfs_device_connector = {
+static const struct device_type drm_sysfs_device_connector = {
 	.name = "drm_connector",
 };
 
@@ -517,7 +513,6 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
 
 		kdev->devt = MKDEV(DRM_MAJOR, minor->index);
 		kdev->class = &drm_class;
-		kdev->type = &drm_sysfs_device_minor;
 	}
 
 	kdev->parent = minor->dev->dev;
-- 
2.46.0

Re: [PATCH 1/6] drm/sysfs: Remove version attribute

[Dmitry Baryshkov]

On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote:
> This undocumented attribute returns a version string which hasn't been
> changed for ages. libdrm doesn't use it and I also found no other user.
> So I think we can remove it.

This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete
drm_core.h") replaced variable string with the fixed value that we
currently have, but at the same it clearly documented that the file is
being preserved for the sake of binary compatibility.

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/gpu/drm/drm_sysfs.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index fb3bbb6ad..49e5faf11 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = {
>  	.unbind = typec_connector_unbind,
>  };
>  
> -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
> -
>  /**
>   * drm_sysfs_init - initialize sysfs helpers
>   *
> @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>   */
>  int drm_sysfs_init(void)
>  {
> -	int err;
> -
>  	drm_class = class_create("drm");
>  	if (IS_ERR(drm_class))
>  		return PTR_ERR(drm_class);
>  
> -	err = class_create_file(drm_class, &class_attr_version.attr);
> -	if (err) {
> -		class_destroy(drm_class);
> -		drm_class = NULL;
> -		return err;
> -	}
> -
>  	drm_class->devnode = drm_devnode;
>  
>  	drm_sysfs_acpi_register();
> @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void)
>  	if (IS_ERR_OR_NULL(drm_class))
>  		return;
>  	drm_sysfs_acpi_unregister();
> -	class_remove_file(drm_class, &class_attr_version.attr);
>  	class_destroy(drm_class);
>  	drm_class = NULL;
>  }
> -- 
> 2.46.0
> 
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/6] drm/sysfs: Drop unused drm_class_device_(un)register

[Dmitry Baryshkov]

On Sun, Sep 08, 2024 at 02:09:39PM GMT, Heiner Kallweit wrote:
> These two functions have no user, so remove them.

If there is going to be next iteration please document that the only
user was TTM code which dropped sysfs interfaces in the commit
ed89fff97382 ("drm/ttm: drop sysfs directory")

Other than that:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/gpu/drm/drm_sysfs.c | 32 --------------------------------
>  include/drm/drm_sysfs.h     |  3 ---
>  2 files changed, 35 deletions(-)
> 
-- 
With best wishes
Dmitry

Re: [PATCH 3/6] drm: Refactor drm_core_init error path

[Dmitry Baryshkov]

On Sun, Sep 08, 2024 at 02:10:30PM GMT, Heiner Kallweit wrote:
> These changes make the error path a little more robust, because exit
> steps in drm_core_exit() don't have to ensure any longer that they
> work properly even if the associated init step wasn't executed.

Please use imperative style when describing changes. E.g. "Do this and
that because of ABCDEF". See
Documentation/process/submitting-patches.rst

> In addition these changes allow to annotate a few functions as __exit,
> saving some memory if drm is built-in.

This should be a separate commit.

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/accel/drm_accel.c                |  2 +-
>  drivers/gpu/drm/drm_drv.c                | 18 ++++++++++++------
>  drivers/gpu/drm/drm_panic.c              |  4 ++--
>  drivers/gpu/drm/drm_privacy_screen_x86.c |  2 +-
>  drivers/gpu/drm/drm_sysfs.c              |  2 --
>  5 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
> index aa826033b..25fdbea36 100644
> --- a/drivers/accel/drm_accel.c
> +++ b/drivers/accel/drm_accel.c
> @@ -191,7 +191,7 @@ static const struct file_operations accel_stub_fops = {
>  	.llseek = noop_llseek,
>  };
>  
> -void accel_core_exit(void)
> +void __exit accel_core_exit(void)
>  {
>  	unregister_chrdev(ACCEL_MAJOR, "accel");
>  	debugfs_remove(accel_debugfs_root);
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index ac30b0ec9..ea59994e5 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -1062,7 +1062,7 @@ static const struct file_operations drm_stub_fops = {
>  	.llseek = noop_llseek,
>  };
>  
> -static void drm_core_exit(void)
> +static void __exit drm_core_exit(void)
>  {
>  	drm_privacy_screen_lookup_exit();
>  	drm_panic_exit();
> @@ -1084,18 +1084,18 @@ static int __init drm_core_init(void)
>  	ret = drm_sysfs_init();
>  	if (ret < 0) {
>  		DRM_ERROR("Cannot create DRM class: %d\n", ret);
> -		goto error;
> +		goto err_ida;
>  	}
>  
>  	drm_debugfs_root = debugfs_create_dir("dri", NULL);
>  
>  	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
>  	if (ret < 0)
> -		goto error;
> +		goto err_debugfs;
>  
>  	ret = accel_core_init();
>  	if (ret < 0)
> -		goto error;
> +		goto err_chrdev;
>  
>  	drm_panic_init();
>  
> @@ -1106,8 +1106,14 @@ static int __init drm_core_init(void)
>  	DRM_DEBUG("Initialized\n");
>  	return 0;
>  
> -error:
> -	drm_core_exit();
> +err_chrdev:
> +	unregister_chrdev(DRM_MAJOR, "drm");
> +err_debugfs:
> +	debugfs_remove(drm_debugfs_root);
> +	drm_sysfs_destroy();
> +err_ida:
> +	WARN_ON(!xa_empty(&drm_minors_xa));
> +	drm_connector_ida_destroy();
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 74412b7bf..d00fdb12d 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -679,7 +679,7 @@ static void __init drm_panic_qr_init(void)
>  				   GFP_KERNEL);
>  }
>  
> -static void drm_panic_qr_exit(void)
> +static void __exit drm_panic_qr_exit(void)
>  {
>  	kfree(qrbuf1);
>  	qrbuf1 = NULL;
> @@ -1058,7 +1058,7 @@ void __init drm_panic_init(void)
>  /**
>   * drm_panic_exit() - Free the resources taken by drm_panic_exit()
>   */
> -void drm_panic_exit(void)
> +void __exit drm_panic_exit(void)
>  {
>  	drm_panic_qr_exit();
>  }
> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> index 72ed40e49..6be96a0cc 100644
> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> @@ -98,7 +98,7 @@ void __init drm_privacy_screen_lookup_init(void)
>  	}
>  }
>  
> -void drm_privacy_screen_lookup_exit(void)
> +void __exit drm_privacy_screen_lookup_exit(void)
>  {
>  	if (arch_lookup.provider)
>  		drm_privacy_screen_lookup_remove(&arch_lookup);
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index a713f0500..f8577043e 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -155,8 +155,6 @@ int drm_sysfs_init(void)
>   */
>  void drm_sysfs_destroy(void)
>  {
> -	if (IS_ERR_OR_NULL(drm_class))
> -		return;
>  	drm_sysfs_acpi_unregister();
>  	class_destroy(drm_class);
>  	drm_class = NULL;
> -- 
> 2.46.0
> 
> 

-- 
With best wishes
Dmitry

Re: [PATCH 4/6] drm: Change drm_class from pointer to const struct class

[Dmitry Baryshkov]

On Sun, Sep 08, 2024 at 02:11:25PM GMT, Heiner Kallweit wrote:
> Define class drm statically and constify it. This ensure that no user
> of the exported struct class can tamper with it.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/gpu/drm/drm_internal.h       |  2 +-
>  drivers/gpu/drm/drm_privacy_screen.c |  2 +-
>  drivers/gpu/drm/drm_sysfs.c          | 32 ++++++++++++++--------------
>  3 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 1705bfc90..6e0df44b6 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -139,7 +139,7 @@ bool drm_master_internal_acquire(struct drm_device *dev);
>  void drm_master_internal_release(struct drm_device *dev);
>  
>  /* drm_sysfs.c */
> -extern struct class *drm_class;
> +extern const struct class drm_class;
>  
>  int drm_sysfs_init(void);
>  void drm_sysfs_destroy(void);
> diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c
> index 6cc39e307..2fbd24ba5 100644
> --- a/drivers/gpu/drm/drm_privacy_screen.c
> +++ b/drivers/gpu/drm/drm_privacy_screen.c
> @@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register(
>  	mutex_init(&priv->lock);
>  	BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head);
>  
> -	priv->dev.class = drm_class;
> +	priv->dev.class = &drm_class;
>  	priv->dev.type = &drm_privacy_screen_type;
>  	priv->dev.parent = parent;
>  	priv->dev.release = drm_privacy_screen_device_release;
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index f8577043e..f443f9a76 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -58,7 +58,15 @@ static struct device_type drm_sysfs_device_connector = {
>  	.name = "drm_connector",
>  };
>  
> -struct class *drm_class;
> +static char *drm_devnode(const struct device *dev, umode_t *mode)
> +{
> +	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
> +}
> +
> +const struct class drm_class = {
> +	.name = "drm",
> +	.devnode = drm_devnode,
> +};
>  
>  #ifdef CONFIG_ACPI
>  static bool drm_connector_acpi_bus_match(struct device *dev)
> @@ -93,11 +101,6 @@ static void drm_sysfs_acpi_register(void) { }
>  static void drm_sysfs_acpi_unregister(void) { }
>  #endif
>  
> -static char *drm_devnode(const struct device *dev, umode_t *mode)
> -{
> -	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
> -}
> -

Please keep this function in place and move drm_class declarattion next
to it. It simplifies reviewing the code.

>  static int typec_connector_bind(struct device *dev,
>  				struct device *typec_connector, void *data)
>  {
> @@ -138,14 +141,12 @@ static const struct component_ops typec_connector_ops = {
>   */
>  int drm_sysfs_init(void)
>  {
> -	drm_class = class_create("drm");
> -	if (IS_ERR(drm_class))
> -		return PTR_ERR(drm_class);
> +	int ret = class_register(&drm_class);
>  
> -	drm_class->devnode = drm_devnode;
> +	if (!ret)
> +		drm_sysfs_acpi_register();
>  
> -	drm_sysfs_acpi_register();
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -156,8 +157,7 @@ int drm_sysfs_init(void)
>  void drm_sysfs_destroy(void)
>  {
>  	drm_sysfs_acpi_unregister();
> -	class_destroy(drm_class);
> -	drm_class = NULL;
> +	class_unregister(&drm_class);

This code makes me wonder: can we define static classes in unloadable
modules? What happens if userspace holds the reference on the class in
sysfs, while we remove the module ?

>  }
>  
>  static void drm_sysfs_release(struct device *dev)
> @@ -337,7 +337,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>  		return -ENOMEM;
>  
>  	device_initialize(kdev);
> -	kdev->class = drm_class;
> +	kdev->class = &drm_class;
>  	kdev->type = &drm_sysfs_device_connector;
>  	kdev->parent = dev->primary->kdev;
>  	kdev->groups = connector_dev_groups;
> @@ -516,7 +516,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
>  			minor_str = "card%d";
>  
>  		kdev->devt = MKDEV(DRM_MAJOR, minor->index);
> -		kdev->class = drm_class;
> +		kdev->class = &drm_class;
>  		kdev->type = &drm_sysfs_device_minor;
>  	}
>  
> -- 
> 2.46.0
> 
> 

-- 
With best wishes
Dmitry

Re: [PATCH 5/6] drm: Add __init annotations

[Dmitry Baryshkov]

On Sun, Sep 08, 2024 at 02:12:32PM GMT, Heiner Kallweit wrote:
> Annotate few more functions being called from drm_core_init() only
> as __init.

Why? Please describe the reason for the changes, not the changes
themselves.

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/gpu/drm/drm_cache.c     | 2 +-
>  drivers/gpu/drm/drm_connector.c | 2 +-
>  drivers/gpu/drm/drm_sysfs.c     | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] drm: drm/sysfs: Remove device type drm_minor

[Dmitry Baryshkov]

On Sun, Sep 08, 2024 at 02:14:43PM GMT, Heiner Kallweit wrote:
> This device type is set but not used, so remove it.

Please describe how it was used and why you consider it to be unused
now. Hint: d14d2a8453d6 ("drm: Remove dev_pm_ops from drm_class").

> Whilst we're at it, constify device type drm_connector.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Whilst we are at it => separate commits, please.

> ---
>  drivers/gpu/drm/drm_sysfs.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)

-- 
With best wishes
Dmitry

Re: [PATCH 1/6] drm/sysfs: Remove version attribute

[Heiner Kallweit]

On 22.09.2024 16:55, Dmitry Baryshkov wrote:
> On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote:
>> This undocumented attribute returns a version string which hasn't been
>> changed for ages. libdrm doesn't use it and I also found no other user.
>> So I think we can remove it.
> 
> This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete
> drm_core.h") replaced variable string with the fixed value that we
> currently have, but at the same it clearly documented that the file is
> being preserved for the sake of binary compatibility.
> 
The drm version attribute is documented neither under Documentation/gpu
nor under Documentation/ABI. So do we really have to consider it
part of the ABI? And are you aware of any actual user of this attribute?

The author of 82d5e73f6b79 wasn't sure either, and therefore didn't
dare to drop the attribute (8 yrs ago). He didn't make any statement that
the attribute is actually used.

6.12-rc1 is just out, so we could drop the attribute in linux-next and
would have several weeks before the next merge window to find out
whether anybody complains.

>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_sysfs.c | 12 ------------
>>  1 file changed, 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index fb3bbb6ad..49e5faf11 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = {
>>  	.unbind = typec_connector_unbind,
>>  };
>>  
>> -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>> -
>>  /**
>>   * drm_sysfs_init - initialize sysfs helpers
>>   *
>> @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>>   */
>>  int drm_sysfs_init(void)
>>  {
>> -	int err;
>> -
>>  	drm_class = class_create("drm");
>>  	if (IS_ERR(drm_class))
>>  		return PTR_ERR(drm_class);
>>  
>> -	err = class_create_file(drm_class, &class_attr_version.attr);
>> -	if (err) {
>> -		class_destroy(drm_class);
>> -		drm_class = NULL;
>> -		return err;
>> -	}
>> -
>>  	drm_class->devnode = drm_devnode;
>>  
>>  	drm_sysfs_acpi_register();
>> @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void)
>>  	if (IS_ERR_OR_NULL(drm_class))
>>  		return;
>>  	drm_sysfs_acpi_unregister();
>> -	class_remove_file(drm_class, &class_attr_version.attr);
>>  	class_destroy(drm_class);
>>  	drm_class = NULL;
>>  }
>> -- 
>> 2.46.0
>>
>>
> 

Re: [PATCH 1/6] drm/sysfs: Remove version attribute

[Dmitry Baryshkov]

On September 30, 2024 1:49:41 PM GMT+03:00, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>On 22.09.2024 16:55, Dmitry Baryshkov wrote:
>> On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote:
>>> This undocumented attribute returns a version string which hasn't been
>>> changed for ages. libdrm doesn't use it and I also found no other user.
>>> So I think we can remove it.
>> 
>> This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete
>> drm_core.h") replaced variable string with the fixed value that we
>> currently have, but at the same it clearly documented that the file is
>> being preserved for the sake of binary compatibility.
>> 
>The drm version attribute is documented neither under Documentation/gpu
>nor under Documentation/ABI. So do we really have to consider it
>part of the ABI? And are you aware of any actual user of this attribute?
>
>The author of 82d5e73f6b79 wasn't sure either, and therefore didn't
>dare to drop the attribute (8 yrs ago). He didn't make any statement that
>the attribute is actually used.

A very quick search points out that the file is being used: 

<https://codesearch.debian.net/search?q=drm%2Fversion>


>
>6.12-rc1 is just out, so we could drop the attribute in linux-next and
>would have several weeks before the next merge window to find out
>whether anybody complains.

No, this is not the way to treat userspace ABI.

>
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/gpu/drm/drm_sysfs.c | 12 ------------
>>>  1 file changed, 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>>> index fb3bbb6ad..49e5faf11 100644
>>> --- a/drivers/gpu/drm/drm_sysfs.c
>>> +++ b/drivers/gpu/drm/drm_sysfs.c
>>> @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = {
>>>  	.unbind = typec_connector_unbind,
>>>  };
>>>  
>>> -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>>> -
>>>  /**
>>>   * drm_sysfs_init - initialize sysfs helpers
>>>   *
>>> @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>>>   */
>>>  int drm_sysfs_init(void)
>>>  {
>>> -	int err;
>>> -
>>>  	drm_class = class_create("drm");
>>>  	if (IS_ERR(drm_class))
>>>  		return PTR_ERR(drm_class);
>>>  
>>> -	err = class_create_file(drm_class, &class_attr_version.attr);
>>> -	if (err) {
>>> -		class_destroy(drm_class);
>>> -		drm_class = NULL;
>>> -		return err;
>>> -	}
>>> -
>>>  	drm_class->devnode = drm_devnode;
>>>  
>>>  	drm_sysfs_acpi_register();
>>> @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void)
>>>  	if (IS_ERR_OR_NULL(drm_class))
>>>  		return;
>>>  	drm_sysfs_acpi_unregister();
>>> -	class_remove_file(drm_class, &class_attr_version.attr);
>>>  	class_destroy(drm_class);
>>>  	drm_class = NULL;
>>>  }
>>> -- 
>>> 2.46.0
>>>
>>>
>> 
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/6] drm/sysfs: Remove version attribute

[Heiner Kallweit]

On 01.10.2024 12:01, Dmitry Baryshkov wrote:
> On September 30, 2024 1:49:41 PM GMT+03:00, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> On 22.09.2024 16:55, Dmitry Baryshkov wrote:
>>> On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote:
>>>> This undocumented attribute returns a version string which hasn't been
>>>> changed for ages. libdrm doesn't use it and I also found no other user.
>>>> So I think we can remove it.
>>>
>>> This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete
>>> drm_core.h") replaced variable string with the fixed value that we
>>> currently have, but at the same it clearly documented that the file is
>>> being preserved for the sake of binary compatibility.
>>>
>> The drm version attribute is documented neither under Documentation/gpu
>> nor under Documentation/ABI. So do we really have to consider it
>> part of the ABI? And are you aware of any actual user of this attribute?
>>
>> The author of 82d5e73f6b79 wasn't sure either, and therefore didn't
>> dare to drop the attribute (8 yrs ago). He didn't make any statement that
>> the attribute is actually used.
> 
> A very quick search points out that the file is being used: 
> 
> <https://codesearch.debian.net/search?q=drm%2Fversion>
> 

Thanks. However this script doesn't actually use the version value
and would work perfectly fine also w/o this attribute.

> 
>>
>> 6.12-rc1 is just out, so we could drop the attribute in linux-next and
>> would have several weeks before the next merge window to find out
>> whether anybody complains.
> 
> No, this is not the way to treat userspace ABI.
> 

Mileage of subsystem maintainers seems to vary in this regard.
See following example where Greg supported such an approach.
https://www.spinics.net/lists/linux-i2c/msg71821.html
But fine with me, we can also leave the version attribute in.

>>
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_sysfs.c | 12 ------------
>>>>  1 file changed, 12 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>>>> index fb3bbb6ad..49e5faf11 100644
>>>> --- a/drivers/gpu/drm/drm_sysfs.c
>>>> +++ b/drivers/gpu/drm/drm_sysfs.c
>>>> @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = {
>>>>  	.unbind = typec_connector_unbind,
>>>>  };
>>>>  
>>>> -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>>>> -
>>>>  /**
>>>>   * drm_sysfs_init - initialize sysfs helpers
>>>>   *
>>>> @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>>>>   */
>>>>  int drm_sysfs_init(void)
>>>>  {
>>>> -	int err;
>>>> -
>>>>  	drm_class = class_create("drm");
>>>>  	if (IS_ERR(drm_class))
>>>>  		return PTR_ERR(drm_class);
>>>>  
>>>> -	err = class_create_file(drm_class, &class_attr_version.attr);
>>>> -	if (err) {
>>>> -		class_destroy(drm_class);
>>>> -		drm_class = NULL;
>>>> -		return err;
>>>> -	}
>>>> -
>>>>  	drm_class->devnode = drm_devnode;
>>>>  
>>>>  	drm_sysfs_acpi_register();
>>>> @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void)
>>>>  	if (IS_ERR_OR_NULL(drm_class))
>>>>  		return;
>>>>  	drm_sysfs_acpi_unregister();
>>>> -	class_remove_file(drm_class, &class_attr_version.attr);
>>>>  	class_destroy(drm_class);
>>>>  	drm_class = NULL;
>>>>  }
>>>> -- 
>>>> 2.46.0
>>>>
>>>>
>>>
>>
> 
> 

Re: [PATCH 1/6] drm/sysfs: Remove version attribute

[Dmitry Baryshkov]

On October 1, 2024 2:07:28 PM GMT+03:00, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>On 01.10.2024 12:01, Dmitry Baryshkov wrote:
>> On September 30, 2024 1:49:41 PM GMT+03:00, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>> On 22.09.2024 16:55, Dmitry Baryshkov wrote:
>>>> On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote:
>>>>> This undocumented attribute returns a version string which hasn't been
>>>>> changed for ages. libdrm doesn't use it and I also found no other user.
>>>>> So I think we can remove it.
>>>>
>>>> This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete
>>>> drm_core.h") replaced variable string with the fixed value that we
>>>> currently have, but at the same it clearly documented that the file is
>>>> being preserved for the sake of binary compatibility.
>>>>
>>> The drm version attribute is documented neither under Documentation/gpu
>>> nor under Documentation/ABI. So do we really have to consider it
>>> part of the ABI? And are you aware of any actual user of this attribute?
>>>
>>> The author of 82d5e73f6b79 wasn't sure either, and therefore didn't
>>> dare to drop the attribute (8 yrs ago). He didn't make any statement that
>>> the attribute is actually used.
>> 
>> A very quick search points out that the file is being used: 
>> 
>> <https://codesearch.debian.net/search?q=drm%2Fversion>
>> 
>
>Thanks. However this script doesn't actually use the version value
>and would work perfectly fine also w/o this attribute.
>
>> 
>>>
>>> 6.12-rc1 is just out, so we could drop the attribute in linux-next and
>>> would have several weeks before the next merge window to find out
>>> whether anybody complains.
>> 
>> No, this is not the way to treat userspace ABI.
>> 
>
>Mileage of subsystem maintainers seems to vary in this regard.
>See following example where Greg supported such an approach.
>https://www.spinics.net/lists/linux-i2c/msg71821.html
>But fine with me, we can also leave the version attribute in.

Let's leave the final decision to subsystem maintainers.


>
>>>
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_sysfs.c | 12 ------------
>>>>>  1 file changed, 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>>>>> index fb3bbb6ad..49e5faf11 100644
>>>>> --- a/drivers/gpu/drm/drm_sysfs.c
>>>>> +++ b/drivers/gpu/drm/drm_sysfs.c
>>>>> @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = {
>>>>>  	.unbind = typec_connector_unbind,
>>>>>  };
>>>>>  
>>>>> -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>>>>> -
>>>>>  /**
>>>>>   * drm_sysfs_init - initialize sysfs helpers
>>>>>   *
>>>>> @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>>>>>   */
>>>>>  int drm_sysfs_init(void)
>>>>>  {
>>>>> -	int err;
>>>>> -
>>>>>  	drm_class = class_create("drm");
>>>>>  	if (IS_ERR(drm_class))
>>>>>  		return PTR_ERR(drm_class);
>>>>>  
>>>>> -	err = class_create_file(drm_class, &class_attr_version.attr);
>>>>> -	if (err) {
>>>>> -		class_destroy(drm_class);
>>>>> -		drm_class = NULL;
>>>>> -		return err;
>>>>> -	}
>>>>> -
>>>>>  	drm_class->devnode = drm_devnode;
>>>>>  
>>>>>  	drm_sysfs_acpi_register();
>>>>> @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void)
>>>>>  	if (IS_ERR_OR_NULL(drm_class))
>>>>>  		return;
>>>>>  	drm_sysfs_acpi_unregister();
>>>>> -	class_remove_file(drm_class, &class_attr_version.attr);
>>>>>  	class_destroy(drm_class);
>>>>>  	drm_class = NULL;
>>>>>  }
>>>>> -- 
>>>>> 2.46.0
>>>>>
>>>>>
>>>>
>>>
>> 
>> 
>


-- 
With best wishes
Dmitry

Re: [PATCH 0/6] drm: Series with core improvements/refactorings

[Dmitry Baryshkov]

On Sun, Sep 08, 2024 at 02:04:45PM +0200, Heiner Kallweit wrote:
> Series with DRM core improvements/refactorings.
> 
> Heiner Kallweit (6):
>   drm/sysfs: Remove version attribute
>   drm/sysfs: Drop unused drm_class_device_(un)register
>   drm: Refactor drm_core_init error path
>   drm: Change drm_class from pointer to const struct class
>   drm: Add __init annotations
>   drm/sysfs: Remove device type drm_minor
> 
>  drivers/accel/drm_accel.c                |  2 +-
>  drivers/gpu/drm/drm_cache.c              |  2 +-
>  drivers/gpu/drm/drm_connector.c          |  2 +-
>  drivers/gpu/drm/drm_drv.c                | 18 +++--
>  drivers/gpu/drm/drm_internal.h           |  2 +-
>  drivers/gpu/drm/drm_panic.c              |  4 +-
>  drivers/gpu/drm/drm_privacy_screen.c     |  2 +-
>  drivers/gpu/drm/drm_privacy_screen_x86.c |  2 +-
>  drivers/gpu/drm/drm_sysfs.c              | 89 +++++-------------------
>  include/drm/drm_sysfs.h                  |  3 -
>  10 files changed, 39 insertions(+), 87 deletions(-)

Heiner, any chance of a respin? I think most of the cleanups were good,
needing just minor polishing.

-- 
With best wishes
Dmitry

Re: [PATCH 0/6] drm: Series with core improvements/refactorings

[Heiner Kallweit]

On 19.10.2024 17:18, Dmitry Baryshkov wrote:
> On Sun, Sep 08, 2024 at 02:04:45PM +0200, Heiner Kallweit wrote:
>> Series with DRM core improvements/refactorings.
>>
>> Heiner Kallweit (6):
>>   drm/sysfs: Remove version attribute
>>   drm/sysfs: Drop unused drm_class_device_(un)register
>>   drm: Refactor drm_core_init error path
>>   drm: Change drm_class from pointer to const struct class
>>   drm: Add __init annotations
>>   drm/sysfs: Remove device type drm_minor
>>
>>  drivers/accel/drm_accel.c                |  2 +-
>>  drivers/gpu/drm/drm_cache.c              |  2 +-
>>  drivers/gpu/drm/drm_connector.c          |  2 +-
>>  drivers/gpu/drm/drm_drv.c                | 18 +++--
>>  drivers/gpu/drm/drm_internal.h           |  2 +-
>>  drivers/gpu/drm/drm_panic.c              |  4 +-
>>  drivers/gpu/drm/drm_privacy_screen.c     |  2 +-
>>  drivers/gpu/drm/drm_privacy_screen_x86.c |  2 +-
>>  drivers/gpu/drm/drm_sysfs.c              | 89 +++++-------------------
>>  include/drm/drm_sysfs.h                  |  3 -
>>  10 files changed, 39 insertions(+), 87 deletions(-)
> 
> Heiner, any chance of a respin? I think most of the cleanups were good,
> needing just minor polishing.
> 
In the last weeks I was working on networking, and now started to rework
the series. I think I can submit a v2 the week after.

Re: [PATCH 4/6] drm: Change drm_class from pointer to const struct class

[Heiner Kallweit]

On 22.09.2024 17:11, Dmitry Baryshkov wrote:
> On Sun, Sep 08, 2024 at 02:11:25PM GMT, Heiner Kallweit wrote:
>> Define class drm statically and constify it. This ensure that no user
>> of the exported struct class can tamper with it.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_internal.h       |  2 +-
>>  drivers/gpu/drm/drm_privacy_screen.c |  2 +-
>>  drivers/gpu/drm/drm_sysfs.c          | 32 ++++++++++++++--------------
>>  3 files changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index 1705bfc90..6e0df44b6 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -139,7 +139,7 @@ bool drm_master_internal_acquire(struct drm_device *dev);
>>  void drm_master_internal_release(struct drm_device *dev);
>>  
>>  /* drm_sysfs.c */
>> -extern struct class *drm_class;
>> +extern const struct class drm_class;
>>  
>>  int drm_sysfs_init(void);
>>  void drm_sysfs_destroy(void);
>> diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c
>> index 6cc39e307..2fbd24ba5 100644
>> --- a/drivers/gpu/drm/drm_privacy_screen.c
>> +++ b/drivers/gpu/drm/drm_privacy_screen.c
>> @@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register(
>>  	mutex_init(&priv->lock);
>>  	BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head);
>>  
>> -	priv->dev.class = drm_class;
>> +	priv->dev.class = &drm_class;
>>  	priv->dev.type = &drm_privacy_screen_type;
>>  	priv->dev.parent = parent;
>>  	priv->dev.release = drm_privacy_screen_device_release;
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index f8577043e..f443f9a76 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -58,7 +58,15 @@ static struct device_type drm_sysfs_device_connector = {
>>  	.name = "drm_connector",
>>  };
>>  
>> -struct class *drm_class;
>> +static char *drm_devnode(const struct device *dev, umode_t *mode)
>> +{
>> +	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
>> +}
>> +
>> +const struct class drm_class = {
>> +	.name = "drm",
>> +	.devnode = drm_devnode,
>> +};
>>  
>>  #ifdef CONFIG_ACPI
>>  static bool drm_connector_acpi_bus_match(struct device *dev)
>> @@ -93,11 +101,6 @@ static void drm_sysfs_acpi_register(void) { }
>>  static void drm_sysfs_acpi_unregister(void) { }
>>  #endif
>>  
>> -static char *drm_devnode(const struct device *dev, umode_t *mode)
>> -{
>> -	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
>> -}
>> -
> 
> Please keep this function in place and move drm_class declarattion next
> to it. It simplifies reviewing the code.
> 
>>  static int typec_connector_bind(struct device *dev,
>>  				struct device *typec_connector, void *data)
>>  {
>> @@ -138,14 +141,12 @@ static const struct component_ops typec_connector_ops = {
>>   */
>>  int drm_sysfs_init(void)
>>  {
>> -	drm_class = class_create("drm");
>> -	if (IS_ERR(drm_class))
>> -		return PTR_ERR(drm_class);
>> +	int ret = class_register(&drm_class);
>>  
>> -	drm_class->devnode = drm_devnode;
>> +	if (!ret)
>> +		drm_sysfs_acpi_register();
>>  
>> -	drm_sysfs_acpi_register();
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -156,8 +157,7 @@ int drm_sysfs_init(void)
>>  void drm_sysfs_destroy(void)
>>  {
>>  	drm_sysfs_acpi_unregister();
>> -	class_destroy(drm_class);
>> -	drm_class = NULL;
>> +	class_unregister(&drm_class);
> 
> This code makes me wonder: can we define static classes in unloadable
> modules? What happens if userspace holds the reference on the class in
> sysfs, while we remove the module ?
> 
I'm not sure, just saw that this isn't an unusual scenario.
Let's ask the drivers/base maintainers.
+Greg/Rafael

>>  }
>>  
>>  static void drm_sysfs_release(struct device *dev)
>> @@ -337,7 +337,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>>  		return -ENOMEM;
>>  
>>  	device_initialize(kdev);
>> -	kdev->class = drm_class;
>> +	kdev->class = &drm_class;
>>  	kdev->type = &drm_sysfs_device_connector;
>>  	kdev->parent = dev->primary->kdev;
>>  	kdev->groups = connector_dev_groups;
>> @@ -516,7 +516,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
>>  			minor_str = "card%d";
>>  
>>  		kdev->devt = MKDEV(DRM_MAJOR, minor->index);
>> -		kdev->class = drm_class;
>> +		kdev->class = &drm_class;
>>  		kdev->type = &drm_sysfs_device_minor;
>>  	}
>>  
>> -- 
>> 2.46.0
>>
>>
> 

Re: [PATCH 4/6] drm: Change drm_class from pointer to const struct class

[Greg Kroah-Hartman]

On Sat, Nov 02, 2024 at 10:33:24PM +0100, Heiner Kallweit wrote:
> On 22.09.2024 17:11, Dmitry Baryshkov wrote:
> > On Sun, Sep 08, 2024 at 02:11:25PM GMT, Heiner Kallweit wrote:
> >> Define class drm statically and constify it. This ensure that no user
> >> of the exported struct class can tamper with it.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/gpu/drm/drm_internal.h       |  2 +-
> >>  drivers/gpu/drm/drm_privacy_screen.c |  2 +-
> >>  drivers/gpu/drm/drm_sysfs.c          | 32 ++++++++++++++--------------
> >>  3 files changed, 18 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> >> index 1705bfc90..6e0df44b6 100644
> >> --- a/drivers/gpu/drm/drm_internal.h
> >> +++ b/drivers/gpu/drm/drm_internal.h
> >> @@ -139,7 +139,7 @@ bool drm_master_internal_acquire(struct drm_device *dev);
> >>  void drm_master_internal_release(struct drm_device *dev);
> >>  
> >>  /* drm_sysfs.c */
> >> -extern struct class *drm_class;
> >> +extern const struct class drm_class;
> >>  
> >>  int drm_sysfs_init(void);
> >>  void drm_sysfs_destroy(void);
> >> diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c
> >> index 6cc39e307..2fbd24ba5 100644
> >> --- a/drivers/gpu/drm/drm_privacy_screen.c
> >> +++ b/drivers/gpu/drm/drm_privacy_screen.c
> >> @@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register(
> >>  	mutex_init(&priv->lock);
> >>  	BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head);
> >>  
> >> -	priv->dev.class = drm_class;
> >> +	priv->dev.class = &drm_class;
> >>  	priv->dev.type = &drm_privacy_screen_type;
> >>  	priv->dev.parent = parent;
> >>  	priv->dev.release = drm_privacy_screen_device_release;
> >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> >> index f8577043e..f443f9a76 100644
> >> --- a/drivers/gpu/drm/drm_sysfs.c
> >> +++ b/drivers/gpu/drm/drm_sysfs.c
> >> @@ -58,7 +58,15 @@ static struct device_type drm_sysfs_device_connector = {
> >>  	.name = "drm_connector",
> >>  };
> >>  
> >> -struct class *drm_class;
> >> +static char *drm_devnode(const struct device *dev, umode_t *mode)
> >> +{
> >> +	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
> >> +}
> >> +
> >> +const struct class drm_class = {
> >> +	.name = "drm",
> >> +	.devnode = drm_devnode,
> >> +};
> >>  
> >>  #ifdef CONFIG_ACPI
> >>  static bool drm_connector_acpi_bus_match(struct device *dev)
> >> @@ -93,11 +101,6 @@ static void drm_sysfs_acpi_register(void) { }
> >>  static void drm_sysfs_acpi_unregister(void) { }
> >>  #endif
> >>  
> >> -static char *drm_devnode(const struct device *dev, umode_t *mode)
> >> -{
> >> -	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
> >> -}
> >> -
> > 
> > Please keep this function in place and move drm_class declarattion next
> > to it. It simplifies reviewing the code.
> > 
> >>  static int typec_connector_bind(struct device *dev,
> >>  				struct device *typec_connector, void *data)
> >>  {
> >> @@ -138,14 +141,12 @@ static const struct component_ops typec_connector_ops = {
> >>   */
> >>  int drm_sysfs_init(void)
> >>  {
> >> -	drm_class = class_create("drm");
> >> -	if (IS_ERR(drm_class))
> >> -		return PTR_ERR(drm_class);
> >> +	int ret = class_register(&drm_class);
> >>  
> >> -	drm_class->devnode = drm_devnode;
> >> +	if (!ret)
> >> +		drm_sysfs_acpi_register();
> >>  
> >> -	drm_sysfs_acpi_register();
> >> -	return 0;
> >> +	return ret;
> >>  }
> >>  
> >>  /**
> >> @@ -156,8 +157,7 @@ int drm_sysfs_init(void)
> >>  void drm_sysfs_destroy(void)
> >>  {
> >>  	drm_sysfs_acpi_unregister();
> >> -	class_destroy(drm_class);
> >> -	drm_class = NULL;
> >> +	class_unregister(&drm_class);
> > 
> > This code makes me wonder: can we define static classes in unloadable
> > modules? What happens if userspace holds the reference on the class in
> > sysfs, while we remove the module ?

Bad things happen, don't do that :)

Good news is it's really hard to hold onto a class structure from
userspace, sysfs should not be doing this, so there shouldn't be really
any other code paths that cause this to happen, unless you do something
odd in your driver.

thanks,

greg k-h