Re: [PATCH v2 2/7] platform/x86/intel/ifs: Introduce Array Scan test to IFS

[Greg KH]

On Thu, Feb 16, 2023 at 02:57:13PM -0800, Joseph, Jithu wrote:
> On 2/16/2023 4:40 AM, Greg KH wrote:
> >> +		ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(),
> >> +							     sizeof(bool), GFP_KERNEL);
> >> +		if (!ifs_devices[i].data.pkg_auth)
> >> +			continue;
> > 
> > You have a static array of a structure that contains both things that
> > describe the devices being used, as well as dynamic data with no real
> > lifespan rules.  Please don't perputate this common design pattern
> > mistake.
> > 
> > Always try to make static data constant and make dynamic data dynamic
> > with proper reference counted lifetime rules.  People converting this
> > code into rust in the future will thank you :)
> 
> I may not have fully understood your comment. So pardon me if the following description
> on the lifecycle of the dynamic allocated memory is not to the point.
> 
> The lifetime of this allocation matches the load time of the driver (allocated on init, freed on exit).
> There are no further / allocations or freeing anywhere within the driver.
> There is only a single place where this memory is used, whose access is serialized via a semaphore.

But the memory is associated with "something" that has a lifetime,
right?  This is either a misc device, or a cpu, or a platform device, or
something that you have to determine that you need to allocate it.

So use that as the thing you hang your dynamic data off of, don't use a
static array.  Allow that static array to be put into read-only memory
(i.e. it is const and can not be changed by your code accidentally or on
purpose.)

Does that help explain things better?

thanks,

greg k-h