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
