char: misc: Make the code for allocating minor in misc_register more concise

[PATCH] char: misc: Make the code for allocating minor in misc_register more concise

Posted by zhangjiao2 5 months ago

From: zhang jiao <zhangjiao2@cmss.chinamobile.com>

There is no need to check the registered misc dev in misc_list. 
If misc_minor_alloc failed, it meens the minor is already alloced 
and the misc dev is linked in misc_list.

Signed-off-by: zhang jiao <zhangjiao2@cmss.chinamobile.com>
---
 drivers/char/misc.c | 32 +++++++-------------------------
 1 file changed, 7 insertions(+), 25 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index a0aae0fc7926..fc2f5e8b2f95 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -211,6 +211,7 @@ int misc_register(struct miscdevice *misc)
 	dev_t dev;
 	int err = 0;
 	bool is_dynamic = (misc->minor == MISC_DYNAMIC_MINOR);
+	int minor = 0;
 
 	if (misc->minor > MISC_DYNAMIC_MINOR) {
 		pr_err("Invalid fixed minor %d for miscdevice '%s'\n",
@@ -221,32 +222,13 @@ int misc_register(struct miscdevice *misc)
 	INIT_LIST_HEAD(&misc->list);
 
 	mutex_lock(&misc_mtx);
-
-	if (is_dynamic) {
-		int i = misc_minor_alloc(misc->minor);
-
-		if (i < 0) {
-			err = -EBUSY;
-			goto out;
-		}
-		misc->minor = i;
-	} else {
-		struct miscdevice *c;
-		int i;
-
-		list_for_each_entry(c, &misc_list, list) {
-			if (c->minor == misc->minor) {
-				err = -EBUSY;
-				goto out;
-			}
-		}
-
-		i = misc_minor_alloc(misc->minor);
-		if (i < 0) {
-			err = -EBUSY;
-			goto out;
-		}
+	minor = misc_minor_alloc(misc->minor);
+	if (minor < 0) {
+		err = -EBUSY;
+		goto out;
 	}
+	if (is_dynamic)
+		misc->minor = minor;
 
 	dev = MKDEV(MISC_MAJOR, misc->minor);
 
-- 
2.33.0

Re: [PATCH] char: misc: Make the code for allocating minor in misc_register more concise

Posted by Greg KH 5 months ago

On Tue, Sep 09, 2025 at 04:58:35PM +0800, zhangjiao2 wrote:
> From: zhang jiao <zhangjiao2@cmss.chinamobile.com>
> 
> There is no need to check the registered misc dev in misc_list. 
> If misc_minor_alloc failed, it meens the minor is already alloced 
> and the misc dev is linked in misc_list.
> 
> Signed-off-by: zhang jiao <zhangjiao2@cmss.chinamobile.com>
> ---
>  drivers/char/misc.c | 32 +++++++-------------------------
>  1 file changed, 7 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index a0aae0fc7926..fc2f5e8b2f95 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -211,6 +211,7 @@ int misc_register(struct miscdevice *misc)
>  	dev_t dev;
>  	int err = 0;
>  	bool is_dynamic = (misc->minor == MISC_DYNAMIC_MINOR);
> +	int minor = 0;
>  
>  	if (misc->minor > MISC_DYNAMIC_MINOR) {
>  		pr_err("Invalid fixed minor %d for miscdevice '%s'\n",
> @@ -221,32 +222,13 @@ int misc_register(struct miscdevice *misc)
>  	INIT_LIST_HEAD(&misc->list);
>  
>  	mutex_lock(&misc_mtx);
> -
> -	if (is_dynamic) {
> -		int i = misc_minor_alloc(misc->minor);
> -
> -		if (i < 0) {
> -			err = -EBUSY;
> -			goto out;
> -		}
> -		misc->minor = i;
> -	} else {
> -		struct miscdevice *c;
> -		int i;
> -
> -		list_for_each_entry(c, &misc_list, list) {
> -			if (c->minor == misc->minor) {
> -				err = -EBUSY;
> -				goto out;
> -			}
> -		}
> -
> -		i = misc_minor_alloc(misc->minor);
> -		if (i < 0) {
> -			err = -EBUSY;
> -			goto out;
> -		}
> +	minor = misc_minor_alloc(misc->minor);
> +	if (minor < 0) {
> +		err = -EBUSY;
> +		goto out;
>  	}
> +	if (is_dynamic)
> +		misc->minor = minor;
>  
>  	dev = MKDEV(MISC_MAJOR, misc->minor);
>  

Does this pass the new test suite for the misc code allocation logic
that we now have in the tree?  Or do we need to write a new test-case
for this codepath?

thanks,

greg k-h

Re: [PATCH] char: misc: Make the code for allocating minor in misc_register more concise

Posted by zhangjiao2 5 months ago

>> On Tue, Sep 09, 2025 at 04:58:35PM +0800, zhangjiao2 wrote:
>> From: zhang jiao <zhangjiao2@cmss.chinamobile.com>
>>
>> There is no need to check the registered misc dev in misc_list.
>> If misc_minor_alloc failed, it meens the minor is already alloced
>> and the misc dev is linked in misc_list.
>>
>> Signed-off-by: zhang jiao <zhangjiao2@cmss.chinamobile.com>
>> ---
>>  drivers/char/misc.c | 32 +++++++-------------------------
>>  1 file changed, 7 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
>> index a0aae0fc7926..fc2f5e8b2f95 100644
>> --- a/drivers/char/misc.c
>> +++ b/drivers/char/misc.c
>> @@ -211,6 +211,7 @@ int misc_register(struct miscdevice *misc)
>>  	dev_t dev;
>>  	int err = 0;
>>  	bool is_dynamic = (misc->minor == MISC_DYNAMIC_MINOR);
>> +	int minor = 0;
>>
>>  	if (misc->minor > MISC_DYNAMIC_MINOR) {
>>  		pr_err("Invalid fixed minor %d for miscdevice '%s'\n",
>> @@ -221,32 +222,13 @@ int misc_register(struct miscdevice *misc)
>>  	INIT_LIST_HEAD(&misc->list);
>>
>>  	mutex_lock(&misc_mtx);
>> -
>> -	if (is_dynamic) {
>> -		int i = misc_minor_alloc(misc->minor);
>> -
>> -		if (i < 0) {
>> -			err = -EBUSY;
>> -			goto out;
>> -		}
>> -		misc->minor = i;
>> -	} else {
>> -		struct miscdevice *c;
>> -		int i;
>> -
>> -		list_for_each_entry(c, &misc_list, list) {
>> -			if (c->minor == misc->minor) {
>> -				err = -EBUSY;
>> -				goto out;
>> -			}
>> -		}
>> -
>> -		i = misc_minor_alloc(misc->minor);
>> -		if (i < 0) {
>> -			err = -EBUSY;
>> -			goto out;
>> -		}
>> +	minor = misc_minor_alloc(misc->minor);
>> +	if (minor < 0) {
>> +		err = -EBUSY;
>> +		goto out;
>>  	}
>> +	if (is_dynamic)
>> +		misc->minor = minor;
>>
>>  	dev = MKDEV(MISC_MAJOR, misc->minor);
>>

> Does this pass the new test suite for the misc code allocation logic
> that we now have in the tree?  Or do we need to write a new test-case
> for this codepath?

 I didn't take this new test suite, can you do it for me? I don't think there's 
 a need to write a new test-case for this codepath. 

thanks,

zhang jiao

Re: [PATCH] char: misc: Make the code for allocating minor in misc_register more concise

Posted by Greg KH 2 months, 1 week ago

On Wed, Sep 10, 2025 at 09:44:20AM +0800, zhangjiao2 wrote:
> >> On Tue, Sep 09, 2025 at 04:58:35PM +0800, zhangjiao2 wrote:
> >> From: zhang jiao <zhangjiao2@cmss.chinamobile.com>
> >>
> >> There is no need to check the registered misc dev in misc_list.
> >> If misc_minor_alloc failed, it meens the minor is already alloced
> >> and the misc dev is linked in misc_list.
> >>
> >> Signed-off-by: zhang jiao <zhangjiao2@cmss.chinamobile.com>
> >> ---
> >>  drivers/char/misc.c | 32 +++++++-------------------------
> >>  1 file changed, 7 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> >> index a0aae0fc7926..fc2f5e8b2f95 100644
> >> --- a/drivers/char/misc.c
> >> +++ b/drivers/char/misc.c
> >> @@ -211,6 +211,7 @@ int misc_register(struct miscdevice *misc)
> >>  	dev_t dev;
> >>  	int err = 0;
> >>  	bool is_dynamic = (misc->minor == MISC_DYNAMIC_MINOR);
> >> +	int minor = 0;
> >>
> >>  	if (misc->minor > MISC_DYNAMIC_MINOR) {
> >>  		pr_err("Invalid fixed minor %d for miscdevice '%s'\n",
> >> @@ -221,32 +222,13 @@ int misc_register(struct miscdevice *misc)
> >>  	INIT_LIST_HEAD(&misc->list);
> >>
> >>  	mutex_lock(&misc_mtx);
> >> -
> >> -	if (is_dynamic) {
> >> -		int i = misc_minor_alloc(misc->minor);
> >> -
> >> -		if (i < 0) {
> >> -			err = -EBUSY;
> >> -			goto out;
> >> -		}
> >> -		misc->minor = i;
> >> -	} else {
> >> -		struct miscdevice *c;
> >> -		int i;
> >> -
> >> -		list_for_each_entry(c, &misc_list, list) {
> >> -			if (c->minor == misc->minor) {
> >> -				err = -EBUSY;
> >> -				goto out;
> >> -			}
> >> -		}
> >> -
> >> -		i = misc_minor_alloc(misc->minor);
> >> -		if (i < 0) {
> >> -			err = -EBUSY;
> >> -			goto out;
> >> -		}
> >> +	minor = misc_minor_alloc(misc->minor);
> >> +	if (minor < 0) {
> >> +		err = -EBUSY;
> >> +		goto out;
> >>  	}
> >> +	if (is_dynamic)
> >> +		misc->minor = minor;
> >>
> >>  	dev = MKDEV(MISC_MAJOR, misc->minor);
> >>
> 
> > Does this pass the new test suite for the misc code allocation logic
> > that we now have in the tree?  Or do we need to write a new test-case
> > for this codepath?
> 
>  I didn't take this new test suite, can you do it for me? I don't think there's 
>  a need to write a new test-case for this codepath. 

Please run it to verify it works properly with the new code paths you
have added here.

thanks,

greg k-h

Re: [PATCH] char: misc: Make the code for allocating minor in misc_register more concise

Posted by Greg KH 4 months ago

On Wed, Sep 10, 2025 at 09:44:20AM +0800, zhangjiao2 wrote:
> >> On Tue, Sep 09, 2025 at 04:58:35PM +0800, zhangjiao2 wrote:
> >> From: zhang jiao <zhangjiao2@cmss.chinamobile.com>
> >>
> >> There is no need to check the registered misc dev in misc_list.
> >> If misc_minor_alloc failed, it meens the minor is already alloced
> >> and the misc dev is linked in misc_list.
> >>
> >> Signed-off-by: zhang jiao <zhangjiao2@cmss.chinamobile.com>
> >> ---
> >>  drivers/char/misc.c | 32 +++++++-------------------------
> >>  1 file changed, 7 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> >> index a0aae0fc7926..fc2f5e8b2f95 100644
> >> --- a/drivers/char/misc.c
> >> +++ b/drivers/char/misc.c
> >> @@ -211,6 +211,7 @@ int misc_register(struct miscdevice *misc)
> >>  	dev_t dev;
> >>  	int err = 0;
> >>  	bool is_dynamic = (misc->minor == MISC_DYNAMIC_MINOR);
> >> +	int minor = 0;
> >>
> >>  	if (misc->minor > MISC_DYNAMIC_MINOR) {
> >>  		pr_err("Invalid fixed minor %d for miscdevice '%s'\n",
> >> @@ -221,32 +222,13 @@ int misc_register(struct miscdevice *misc)
> >>  	INIT_LIST_HEAD(&misc->list);
> >>
> >>  	mutex_lock(&misc_mtx);
> >> -
> >> -	if (is_dynamic) {
> >> -		int i = misc_minor_alloc(misc->minor);
> >> -
> >> -		if (i < 0) {
> >> -			err = -EBUSY;
> >> -			goto out;
> >> -		}
> >> -		misc->minor = i;
> >> -	} else {
> >> -		struct miscdevice *c;
> >> -		int i;
> >> -
> >> -		list_for_each_entry(c, &misc_list, list) {
> >> -			if (c->minor == misc->minor) {
> >> -				err = -EBUSY;
> >> -				goto out;
> >> -			}
> >> -		}
> >> -
> >> -		i = misc_minor_alloc(misc->minor);
> >> -		if (i < 0) {
> >> -			err = -EBUSY;
> >> -			goto out;
> >> -		}
> >> +	minor = misc_minor_alloc(misc->minor);
> >> +	if (minor < 0) {
> >> +		err = -EBUSY;
> >> +		goto out;
> >>  	}
> >> +	if (is_dynamic)
> >> +		misc->minor = minor;
> >>
> >>  	dev = MKDEV(MISC_MAJOR, misc->minor);
> >>
> 
> > Does this pass the new test suite for the misc code allocation logic
> > that we now have in the tree?  Or do we need to write a new test-case
> > for this codepath?
> 
>  I didn't take this new test suite, can you do it for me?

Please test your changes first, don't make maintainers do it for you :)

> I don't think there's 
>  a need to write a new test-case for this codepath. 

Why not?  It seems to be a code path that is worth changing, so why not
test to verify you got it correct?

thanks,

greg k-h