[PATCH 0/2] staging: pi433: Use class_create API.

[PATCH 0/2] staging: pi433: Use class_create API.

[Shahar Avidar]

This patchset reduces static allocated memory by using class_create
instead of class_register.

Shahar Avidar (2):
  staging: pi433: Use class_create instead of class_register.
  staging: pi433: Rename goto label.

 drivers/staging/pi433/pi433_if.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)


base-commit: 75ff53c44f5e151d21d949416633b56e56160124
prerequisite-patch-id: 91943193af2fea74182be67fb583235a3fbeb77b
prerequisite-patch-id: 2cad031ba6a0782a67ab1645ff034a8be65c2e76
prerequisite-patch-id: 1a852ed8f9d133aec7c651fd9007e59e39c55fb7
-- 
2.34.1

[PATCH 1/2] staging: pi433: Use class_create instead of class_register.

[Shahar Avidar]

Make use of a higher level API.
Reduce global memory allocation from struct class to pointer size.

Signed-off-by: Shahar Avidar <ikobh7@gmail.com>
---
 drivers/staging/pi433/pi433_if.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 31aeabb1f153..c8c1d296184b 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -59,9 +59,7 @@ static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
 static struct dentry *root_dir;	/* debugfs root directory for the driver */
 
 /* mainly for udev to create /dev/pi433 */
-static const struct class pi433_class = {
-	.name = "pi433",
-};
+static const struct class *pi433_class;
 
 /*
  * tx config is instance specific
@@ -1263,7 +1261,7 @@ static int pi433_probe(struct spi_device *spi)
 
 	/* create device */
 	pi433->devt = MKDEV(MAJOR(pi433_devt), pi433->minor);
-	pi433->dev = device_create(&pi433_class,
+	pi433->dev = device_create(pi433_class,
 				   &spi->dev,
 				   pi433->devt,
 				   pi433,
@@ -1319,7 +1317,7 @@ static int pi433_probe(struct spi_device *spi)
 cdev_failed:
 	kthread_stop(pi433->tx_task_struct);
 send_thread_failed:
-	device_destroy(&pi433_class, pi433->devt);
+	device_destroy(pi433_class, pi433->devt);
 device_create_failed:
 	pi433_free_minor(pi433);
 minor_failed:
@@ -1346,7 +1344,7 @@ static void pi433_remove(struct spi_device *spi)
 
 	kthread_stop(pi433->tx_task_struct);
 
-	device_destroy(&pi433_class, pi433->devt);
+	device_destroy(pi433_class, pi433->devt);
 
 	cdev_del(pi433->cdev);
 
@@ -1401,9 +1399,11 @@ static int __init pi433_init(void)
 	if (status < 0)
 		return status;
 
-	status = class_register(&pi433_class);
-	if (status)
+	pi433_class = class_create("pi433");
+	if (IS_ERR(pi433_class)) {
+		status = PTR_ERR(pi433_class);
 		goto unreg_chrdev;
+	}
 
 	root_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
 
@@ -1415,7 +1415,7 @@ static int __init pi433_init(void)
 
 unreg_class_and_remove_dbfs:
 	debugfs_remove(root_dir);
-	class_unregister(&pi433_class);
+	class_destroy(pi433_class);
 unreg_chrdev:
 	unregister_chrdev(MAJOR(pi433_devt), pi433_spi_driver.driver.name);
 	return status;
@@ -1427,7 +1427,7 @@ static void __exit pi433_exit(void)
 {
 	spi_unregister_driver(&pi433_spi_driver);
 	debugfs_remove(root_dir);
-	class_unregister(&pi433_class);
+	class_destroy(pi433_class);
 	unregister_chrdev(MAJOR(pi433_devt), pi433_spi_driver.driver.name);
 }
 module_exit(pi433_exit);

base-commit: 75ff53c44f5e151d21d949416633b56e56160124
prerequisite-patch-id: 91943193af2fea74182be67fb583235a3fbeb77b
prerequisite-patch-id: 2cad031ba6a0782a67ab1645ff034a8be65c2e76
prerequisite-patch-id: 1a852ed8f9d133aec7c651fd9007e59e39c55fb7
-- 
2.34.1

Re: [PATCH 1/2] staging: pi433: Use class_create instead of class_register.

[Dan Carpenter]

On Wed, May 01, 2024 at 08:58:19AM +0300, Shahar Avidar wrote:
> Make use of a higher level API.
> Reduce global memory allocation from struct class to pointer size.

Doesn't this move the memory in the opposite direction from what we
want?  Originally, it's static const.  Isn't that the simplest best
kind of memory?

regards,
dan carpenter

Re: [PATCH 1/2] staging: pi433: Use class_create instead of class_register.

[Greg KH]

On Wed, May 01, 2024 at 05:00:44PM +0300, Dan Carpenter wrote:
> On Wed, May 01, 2024 at 08:58:19AM +0300, Shahar Avidar wrote:
> > Make use of a higher level API.
> > Reduce global memory allocation from struct class to pointer size.
> 
> Doesn't this move the memory in the opposite direction from what we
> want?  Originally, it's static const.  Isn't that the simplest best
> kind of memory?

Our reviews just crossed...  This is just a revert (in 2 steps oddly),
of a previous commit that changed this api call, and for that reason
alone we can't take it :)

thanks,

greg k-h

Re: [PATCH 1/2] staging: pi433: Use class_create instead of class_register.

[Shahar Avidar]

On 01/05/2024 17:14, Greg KH wrote:
> On Wed, May 01, 2024 at 05:00:44PM +0300, Dan Carpenter wrote:
>> On Wed, May 01, 2024 at 08:58:19AM +0300, Shahar Avidar wrote:
>>> Make use of a higher level API.
>>> Reduce global memory allocation from struct class to pointer size.
>>
>> Doesn't this move the memory in the opposite direction from what we
>> want?  Originally, it's static const.  Isn't that the simplest best
>> kind of memory?
> 
> Our reviews just crossed...  This is just a revert (in 2 steps oddly),
> of a previous commit that changed this api call, and for that reason
> alone we can't take it :)
> 
Thank you for your input.
I replied to Greg's review on another thread.
I won't pursue this change.

-- 
Regards,

Shahar

Re: [PATCH 1/2] staging: pi433: Use class_create instead of class_register.

[Greg KH]

On Wed, May 01, 2024 at 08:58:19AM +0300, Shahar Avidar wrote:
> Make use of a higher level API.

What does this mean?

> Reduce global memory allocation from struct class to pointer size.

No, you increased memory allocation here, why do you think you reduced
it?

Also, this looks like a revert of commit f267da65bb6b ("staging: pi433:
make pi433_class constant"), accepted a few months ago, why not just
call it out as an explicit revert if that's what you want to do?

class_create is going away "soon", why add this back when people are
working so hard to remove its usage?  What tutorial did you read that
made you want to make this change?

thanks,

greg k-h

Re: [PATCH 1/2] staging: pi433: Use class_create instead of class_register.

[Shahar Avidar]

On 01/05/2024 17:12, Greg KH wrote:
> On Wed, May 01, 2024 at 08:58:19AM +0300, Shahar Avidar wrote:
>> Make use of a higher level API.
> 
> What does this mean?
> 
By "higher level" I meant a wrapper function that includes the 
"class_register" call.

>> Reduce global memory allocation from struct class to pointer size.
> 
> No, you increased memory allocation here, why do you think you reduced
> it?
> 
Reducing *global* memory allocation.
I understand the tradeoff would be allocating in run time the class 
struct anyway, but than, it could also be freed.

Since the Pi433 is a RasPi expansion board and can be attached\removed 
in an asynchronous matter by the user, and only one can be attached at a 
time, I thought it is best not to statically allocate memory which won't 
be freed even if the hat is removed.

By using the class_create & class_destroy I thought of reducing memory 
allocated by the RasPi if the pi433 is removed.

But following your response I now actually see that the class struct 
will have the same lifespan anyway if allocated statically or 
dynamically if its alive between the init\exit calls.

> Also, this looks like a revert of commit f267da65bb6b ("staging: pi433:
> make pi433_class constant"), accepted a few months ago, why not just
> call it out as an explicit revert if that's what you want to do?
> 
I actually saw this commit, but for some reason did not connect the dots 
when I wrote this patch. My bad.

> class_create is going away "soon", why add this back when people are
> working so hard to remove its usage?  What tutorial did you read that
> made you want to make this change?
> 
It's true, I got it the wrong way I guess. I thought class_create is the 
preferred API (but now that you mentioned commit f267da65bb6b, I see 
it's not). I did notice it in many other drivers though, and took them 
as an example (e.g. gnss).


> thanks,
> 
> greg k-h

I actually initially thought that the pi433 class should be removed 
since it doesn't bring any new attributes with it, and that 
spi_slave_class is more appropriate, but then I saw no other driver 
using it. Any thoughts about that?
-- 
Regards,

Shahar

Re: [PATCH 1/2] staging: pi433: Use class_create instead of class_register.

[Greg KH]

On Thu, May 02, 2024 at 11:40:44AM +0300, Shahar Avidar wrote:
> On 01/05/2024 17:12, Greg KH wrote:
> > On Wed, May 01, 2024 at 08:58:19AM +0300, Shahar Avidar wrote:
> > > Make use of a higher level API.
> > 
> > What does this mean?
> > 
> By "higher level" I meant a wrapper function that includes the
> "class_register" call.
> 
> > > Reduce global memory allocation from struct class to pointer size.
> > 
> > No, you increased memory allocation here, why do you think you reduced
> > it?
> > 
> Reducing *global* memory allocation.

And again, you *increased* memory allocation by making this be
dynamically created instead of the current code which is a static and
can be placed into read-only memory with no padding required unlike a
dynamic memory chunk is.  You also removed the read-only markings of the
structure for no reason, in a way, making the code a tad be more
insecure as well as increasing memory usage.

So be careful please.

> I understand the tradeoff would be allocating in run time the class struct
> anyway, but than, it could also be freed.

When is it freed that the current code is not also freed?

> Since the Pi433 is a RasPi expansion board and can be attached\removed in an
> asynchronous matter by the user, and only one can be attached at a time, I
> thought it is best not to statically allocate memory which won't be freed
> even if the hat is removed.

Is that what happens in the code?

> By using the class_create & class_destroy I thought of reducing memory
> allocated by the RasPi if the pi433 is removed.

Try it and see :)

> But following your response I now actually see that the class struct will
> have the same lifespan anyway if allocated statically or dynamically if its
> alive between the init\exit calls.

Yes.

> > Also, this looks like a revert of commit f267da65bb6b ("staging: pi433:
> > make pi433_class constant"), accepted a few months ago, why not just
> > call it out as an explicit revert if that's what you want to do?
> > 
> I actually saw this commit, but for some reason did not connect the dots
> when I wrote this patch. My bad.
> 
> > class_create is going away "soon", why add this back when people are
> > working so hard to remove its usage?  What tutorial did you read that
> > made you want to make this change?
> > 
> It's true, I got it the wrong way I guess. I thought class_create is the
> preferred API (but now that you mentioned commit f267da65bb6b, I see it's
> not). I did notice it in many other drivers though, and took them as an
> example (e.g. gnss).

There are patches out that replace almost all users of class_create()
such that it should be almost gone from the tree.

> > thanks,
> > 
> > greg k-h
> 
> I actually initially thought that the pi433 class should be removed since it
> doesn't bring any new attributes with it, and that spi_slave_class is more
> appropriate, but then I saw no other driver using it. Any thoughts about
> that?

The whole driver is going to be removed soon, please see the mailing
list archives for the details.

thanks,

greg k-h

[PATCH 2/2] staging: pi433: Rename goto label.

[Shahar Avidar]

Use destroy_class_and_remove_dbfs instead of unreg_class_and_remove_dbfs.

Signed-off-by: Shahar Avidar <ikobh7@gmail.com>
---
 drivers/staging/pi433/pi433_if.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index c8c1d296184b..4fffd7007040 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1409,11 +1409,11 @@ static int __init pi433_init(void)
 
 	status = spi_register_driver(&pi433_spi_driver);
 	if (status < 0)
-		goto unreg_class_and_remove_dbfs;
+		goto destroy_class_and_remove_dbfs;
 
 	return 0;
 
-unreg_class_and_remove_dbfs:
+destroy_class_and_remove_dbfs:
 	debugfs_remove(root_dir);
 	class_destroy(pi433_class);
 unreg_chrdev:
-- 
2.34.1

Re: [PATCH 2/2] staging: pi433: Rename goto label.

[Dan Carpenter]

On Wed, May 01, 2024 at 08:58:20AM +0300, Shahar Avidar wrote:
> Use destroy_class_and_remove_dbfs instead of unreg_class_and_remove_dbfs.
> 
> Signed-off-by: Shahar Avidar <ikobh7@gmail.com>
> ---
>  drivers/staging/pi433/pi433_if.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index c8c1d296184b..4fffd7007040 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -1409,11 +1409,11 @@ static int __init pi433_init(void)
>  
>  	status = spi_register_driver(&pi433_spi_driver);
>  	if (status < 0)
> -		goto unreg_class_and_remove_dbfs;
> +		goto destroy_class_and_remove_dbfs;
>  
>  	return 0;
>  
> -unreg_class_and_remove_dbfs:
> +destroy_class_and_remove_dbfs:
>  	debugfs_remove(root_dir);
>  	class_destroy(pi433_class);

This is cleaning up something which changed in patch 1 so it should have
been done in patch 1.

regards,
dan carpenter

>  unreg_chrdev:
> -- 
> 2.34.1

Re: [PATCH 2/2] staging: pi433: Rename goto label.

[Shahar Avidar]

On 01/05/2024 17:06, Dan Carpenter wrote:
> On Wed, May 01, 2024 at 08:58:20AM +0300, Shahar Avidar wrote:
>> Use destroy_class_and_remove_dbfs instead of unreg_class_and_remove_dbfs.
>>
>> Signed-off-by: Shahar Avidar <ikobh7@gmail.com>
>> ---
>>   drivers/staging/pi433/pi433_if.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
>> index c8c1d296184b..4fffd7007040 100644
>> --- a/drivers/staging/pi433/pi433_if.c
>> +++ b/drivers/staging/pi433/pi433_if.c
>> @@ -1409,11 +1409,11 @@ static int __init pi433_init(void)
>>   
>>   	status = spi_register_driver(&pi433_spi_driver);
>>   	if (status < 0)
>> -		goto unreg_class_and_remove_dbfs;
>> +		goto destroy_class_and_remove_dbfs;
>>   
>>   	return 0;
>>   
>> -unreg_class_and_remove_dbfs:
>> +destroy_class_and_remove_dbfs:
>>   	debugfs_remove(root_dir);
>>   	class_destroy(pi433_class);
> 
> This is cleaning up something which changed in patch 1 so it should have
> been done in patch 1.
> 
Thanks for your input.
I thought of a previous comment you had were you noted Greg preferred 
small patches, so I did my best to keep the first patch the with minimum 
changes without breaking git digest.

This patchset won't be accepted anyway.

> regards,
> dan carpenter
> 
>>   unreg_chrdev:
>> -- 
>> 2.34.1
-- 
Regards,

Shahar

Re: [PATCH 2/2] staging: pi433: Rename goto label.

[Dan Carpenter]

On Thu, May 02, 2024 at 11:44:15AM +0300, Shahar Avidar wrote:
> On 01/05/2024 17:06, Dan Carpenter wrote:
> > On Wed, May 01, 2024 at 08:58:20AM +0300, Shahar Avidar wrote:
> > > Use destroy_class_and_remove_dbfs instead of unreg_class_and_remove_dbfs.
> > > 
> > > Signed-off-by: Shahar Avidar <ikobh7@gmail.com>
> > > ---
> > >   drivers/staging/pi433/pi433_if.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > > index c8c1d296184b..4fffd7007040 100644
> > > --- a/drivers/staging/pi433/pi433_if.c
> > > +++ b/drivers/staging/pi433/pi433_if.c
> > > @@ -1409,11 +1409,11 @@ static int __init pi433_init(void)
> > >   	status = spi_register_driver(&pi433_spi_driver);
> > >   	if (status < 0)
> > > -		goto unreg_class_and_remove_dbfs;
> > > +		goto destroy_class_and_remove_dbfs;
> > >   	return 0;
> > > -unreg_class_and_remove_dbfs:
> > > +destroy_class_and_remove_dbfs:
> > >   	debugfs_remove(root_dir);
> > >   	class_destroy(pi433_class);
> > 
> > This is cleaning up something which changed in patch 1 so it should have
> > been done in patch 1.
> > 
> Thanks for your input.
> I thought of a previous comment you had were you noted Greg preferred small
> patches, so I did my best to keep the first patch the with minimum changes
> without breaking git digest.

The rule is not "as small as possible", it's "one thing per patch".
It's a bit subtle, but you're doing half a thing in this patch.  Not a
huge deal.  It's part of the learning process.

regards,
dan carpenter