linux-kernel - Re: [PATCH] drm: amdkfd: Replace (un)register_chrdev() by (unregister/alloc)_chrdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a5b1d94e-30ee-411c-88f5-1e340068220c@amd.com>
Date: Wed, 5 Mar 2025 19:18:33 -0500
From: Felix Kuehling <felix.kuehling@....com>
To: Salah Triki <salah.triki@...il.com>,
 Alex Deucher <alexander.deucher@....com>,
 Christian König <christian.koenig@....com>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm: amdkfd: Replace (un)register_chrdev() by
 (unregister/alloc)_chrdev_region()


On 2025-03-05 16:08, Salah Triki wrote:
> Replace (un)register_chrdev() by (unregister/alloc)_chrdev_region() as
> they are deprecated since kernel 2.6.

Where is that information coming from? I see __register_chrdev 
documented in the current kernel documentation. I see no indication that 
it's deprecated: 
https://docs.kernel.org/core-api/kernel-api.html#c.__register_chrdev


>   alloc_chrdev_region() generates a
> dev_t value, so replace the kfd_char_dev_major int variable by the
> kfd_char_dev_id dev_t variable and drop the MKDEV() call. Initialize a
> cdev structure and add it to the device driver model as register_chrdev()
> used to do and since alloc_chrdev_region() does not do it. Drop the
> iminor() call since alloc_chrdev_region() allocates only one minor number.
> On error and in the module exit function, remove the cdev structure from
> the device driver model as unregister_chrdev() used to do.

Sounds complicated. Your patch seems to open-code a bunch of details 
that are neatly hidden inside register_chrdev. Why would I want all that 
detail in my driver? I don't see an obvious advantage.

Regards,
   Felix


> Signed-off-by: Salah Triki <salah.triki@...il.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 35 ++++++++++++++++--------
>   1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 065d87841459..55c74466d2c5 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -37,6 +37,8 @@
>   #include <linux/ptrace.h>
>   #include <linux/dma-buf.h>
>   #include <linux/processor.h>
> +#include <linux/cdev.h>
> +
>   #include "kfd_priv.h"
>   #include "kfd_device_queue_manager.h"
>   #include "kfd_svm.h"
> @@ -61,12 +63,14 @@ static const struct file_operations kfd_fops = {
>   	.mmap = kfd_mmap,
>   };
>   
> -static int kfd_char_dev_major = -1;
> +static dev_t kfd_char_dev_id;
>   struct device *kfd_device;
>   static const struct class kfd_class = {
>   	.name = kfd_dev_name,
>   };
>   
> +static struct cdev kfd_cdev;
> +
>   static inline struct kfd_process_device *kfd_lock_pdd_by_id(struct kfd_process *p, __u32 gpu_id)
>   {
>   	struct kfd_process_device *pdd;
> @@ -90,17 +94,24 @@ int kfd_chardev_init(void)
>   {
>   	int err = 0;
>   
> -	kfd_char_dev_major = register_chrdev(0, kfd_dev_name, &kfd_fops);
> -	err = kfd_char_dev_major;
> +	err = alloc_chrdev_region(&kfd_char_dev_id, 0, 1, kfd_dev_name);
> +
>   	if (err < 0)
> -		goto err_register_chrdev;
> +		goto err_alloc_chrdev_region;
> +
> +	cdev_init(&kfd_cdev, &kfd_fops);
> +	kfd_cdev.owner = THIS_MODULE;
> +
> +	err = cdev_add(&kfd_cdev, kfd_char_dev_id, 1);
> +	if (err)
> +		goto err_cdev_add;
>   
>   	err = class_register(&kfd_class);
>   	if (err)
>   		goto err_class_create;
>   
>   	kfd_device = device_create(&kfd_class, NULL,
> -				   MKDEV(kfd_char_dev_major, 0),
> +				   kfd_char_dev_id,
>   				   NULL, kfd_dev_name);
>   	err = PTR_ERR(kfd_device);
>   	if (IS_ERR(kfd_device))
> @@ -111,16 +122,19 @@ int kfd_chardev_init(void)
>   err_device_create:
>   	class_unregister(&kfd_class);
>   err_class_create:
> -	unregister_chrdev(kfd_char_dev_major, kfd_dev_name);
> -err_register_chrdev:
> +	cdev_del(&kfd_cdev);
> +err_cdev_add:
> +	unregister_chrdev_region(kfd_char_dev_id, 1);
> +err_alloc_chrdev_region:
>   	return err;
>   }
>   
>   void kfd_chardev_exit(void)
>   {
> -	device_destroy(&kfd_class, MKDEV(kfd_char_dev_major, 0));
> +	device_destroy(&kfd_class, kfd_char_dev_id);
>   	class_unregister(&kfd_class);
> -	unregister_chrdev(kfd_char_dev_major, kfd_dev_name);
> +	cdev_del(&kfd_cdev);
> +	unregister_chrdev_region(kfd_char_dev_id, 1);
>   	kfd_device = NULL;
>   }
>   
> @@ -130,9 +144,6 @@ static int kfd_open(struct inode *inode, struct file *filep)
>   	struct kfd_process *process;
>   	bool is_32bit_user_mode;
>   
> -	if (iminor(inode) != 0)
> -		return -ENODEV;
> -
>   	is_32bit_user_mode = in_compat_syscall();
>   
>   	if (is_32bit_user_mode) {