[PATCH] drm: amdkfd: Replace (un)register_chrdev() by (unregister/alloc)_chrdev_region()

[PATCH] drm: amdkfd: Replace (un)register_chrdev() by (unregister/alloc)_chrdev_region()

[Salah Triki]

Replace (un)register_chrdev() by (unregister/alloc)_chrdev_region() as
they are deprecated since kernel 2.6. 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.

Signed-off-by: Salah Triki <salah.triki@gmail.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) {
-- 
2.34.1

Re: [PATCH] drm: amdkfd: Replace (un)register_chrdev() by (unregister/alloc)_chrdev_region()

[Felix Kuehling]

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@gmail.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) {

Re: [PATCH] drm: amdkfd: Replace (un)register_chrdev() by (unregister/alloc)_chrdev_region()

[Salah Triki]

On Wed, Mar 05, 2025 at 07:18:33PM -0500, Felix Kuehling wrote:
> 
> 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
> 
In the book "Linux Device Drivers" 3ed of J. Corbet and al. (2005), it
is indicated that using register_chrdev() is the old way to register
char drivers, the new code should not use this interface, it should
instead use the cdev interface. 
> 
> >   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.
> 
register_chrdev() registers 256 minor numbers, calling it will result in
calling kmalloc_array(256, sizeof(struct probe), GFP_KERNEL) whereas
calling alloc_chrdev_region() with count parameter equals to 1, which is
the number of minor numbers requested, will result in calling
kmalloc_array(1, sizeof(stuct probe), GFP_KERNEL). 

Best Regards,
Salah Triki 

Re: [PATCH] drm: amdkfd: Replace (un)register_chrdev() by (unregister/alloc)_chrdev_region()

[Christian König]

Am 07.03.25 um 20:10 schrieb Salah Triki:
> On Wed, Mar 05, 2025 at 07:18:33PM -0500, Felix Kuehling wrote:
>> 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
>>
> In the book "Linux Device Drivers" 3ed of J. Corbet and al. (2005), it
> is indicated that using register_chrdev() is the old way to register
> char drivers, the new code should not use this interface, it should
> instead use the cdev interface. 

Yeah, but that doesn't count. Only in kernel documentation is relevant.

Regards,
Christian.

>>>   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.
>>
> register_chrdev() registers 256 minor numbers, calling it will result in
> calling kmalloc_array(256, sizeof(struct probe), GFP_KERNEL) whereas
> calling alloc_chrdev_region() with count parameter equals to 1, which is
> the number of minor numbers requested, will result in calling
> kmalloc_array(1, sizeof(stuct probe), GFP_KERNEL). 
>
> Best Regards,
> Salah Triki 

Re: [PATCH] drm: amdkfd: Replace (un)register_chrdev() by (unregister/alloc)_chrdev_region()

[Salah Triki]

> > register_chrdev() registers 256 minor numbers, calling it will result in
> > calling kmalloc_array(256, sizeof(struct probe), GFP_KERNEL) whereas
> > calling alloc_chrdev_region() with count parameter equals to 1, which is
> > the number of minor numbers requested, will result in calling
> > kmalloc_array(1, sizeof(stuct probe), GFP_KERNEL). 
> >

Is it worth replacing register_chrdev() by alloc_chrdev_region() for
this ? If so I will change the patch description.

Best Regards,
Salah Triki