Date:   Fri, 17 Feb 2023 10:25:10 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     "Joseph, Jithu" <jithu.joseph@intel.com>
Cc:     hdegoede@redhat.com, markgross@kernel.org, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        x86@kernel.org, hpa@zytor.com, rostedt@goodmis.org,
        ashok.raj@intel.com, tony.luck@intel.com,
        linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        patches@lists.linux.dev, ravi.v.shankar@intel.com,
        thiago.macieira@intel.com, athenas.jimenez.gonzalez@intel.com,
        sohil.mehta@intel.com
Subject: Re: [PATCH v2 2/7] platform/x86/intel/ifs: Introduce Array Scan test
 to IFS
Message-ID: <Y+9H9otxLYPqMkUh@kroah.com>
References: <20230131234302.3997223-1-jithu.joseph@intel.com>
 <20230214234426.344960-1-jithu.joseph@intel.com>
 <20230214234426.344960-3-jithu.joseph@intel.com>
 <Y+4kQOtrHt5pdsSO@kroah.com>
 <4e5cebd0-815e-e3ca-7068-4d665d337bf2@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4e5cebd0-815e-e3ca-7068-4d665d337bf2@intel.com>
Precedence: bulk

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