Received-SPF: pass (google.com: domain of linux-kernel+bounces-166159-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 2 May 2024 10:54:59 +0200
From: Greg KH <gregkh@linuxfoundation.org>
To: Shahar Avidar <ikobh7@gmail.com>
Cc: hverkuil-cisco@xs4all.nl, andriy.shevchenko@linux.intel.com,
	robh@kernel.org, felixkimbu1@gmail.com, dan.carpenter@linaro.org,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] staging: pi433: Use class_create instead of
 class_register.
Message-ID: <2024050257-gong-issue-ec40@gregkh>
References: <20240501055820.603272-1-ikobh7@gmail.com>
 <20240501055820.603272-2-ikobh7@gmail.com>
 <2024050109-reward-vision-58e9@gregkh>
 <fede8589-dd11-4b0c-aa70-7ec23aed64b1@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fede8589-dd11-4b0c-aa70-7ec23aed64b1@gmail.com>

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