On Fri, 7 Jan 2022 at 14:15, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote: > > On 31/12/2021 14:33, Ulf Hansson wrote: > > On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote: > >> > >> The dtpm table is used to let the different dtpm backends to register > >> their setup callbacks in a single place and preventing to export > >> multiple functions all around the kernel. That allows the dtpm code to > >> be self-encapsulated. > > > > Well, that's not entirely true. The dtpm code and its backends (or > > ops, whatever we call them) are already maintained from a single > > place, the /drivers/powercap/* directory. I assume we intend to keep > > it like this going forward too, right? > > > > That is also what patch4 with the devfreq backend continues to conform to. > > > >> > >> The dtpm hierarchy will be passed as a parameter by a platform > >> specific code and that will lead to the creation of the different dtpm > >> nodes. > >> > >> The function creating the hierarchy could be called from a module at > >> init time or when it is loaded. However, at this moment the table is > >> already freed as it belongs to the init section and the creation will > >> lead to a invalid memory access. > >> > >> Fix this by moving the table to the data section. > > > > With the above said, I find it a bit odd to put a table in the data > > section like this. Especially, since the only remaining argument for > > why, is to avoid exporting functions, which isn't needed anyway. > > > > I mean, it would be silly if we should continue to put subsystem > > specific tables in here, to just let them contain a set of subsystem > > specific callbacks. > > So I tried to change the approach and right now I was not able to find > an alternative keeping the code self-encapsulate and without introducing > cyclic dependencies. > > I suggest to keep the patch as it is and double check if it makes sense > to change it after adding more dtpm backends > > Alternatively I can copy the table to a dynamically allocated table. I am not sure I understand the problem. You don't need a "table of callbacks" at all, at least to start with. Instead, what you need is to make a call to a function, or actually one call per supported dtpm type from dtpm_setup_dt() (introduced in patch2). For CPUs, you would simply call dtpm_cpu_setup() (introduced in patch3) from dtpm_setup_dt(), rather than walking the dtpm table an invoking the ->setup() callback. Did that make sense to you? Going forward, when we decide to introduce the option to add/remove support for dtpm types dynamically, you can then convert to a dynamically allocated table. [...] Kind regards Uffe
