Messages in this thread | | | Date | Thu, 2 May 2024 10:54:59 +0200 | From | Greg KH <> | Subject | Re: [PATCH 1/2] staging: pi433: Use class_create instead of class_register. |
| |
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
| |