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

[Greg KH <gregkh@linuxfoundation.org>]

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