| Message ID | 20231129110853.94344-8-lukasz.luba@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce runtime modifiable Energy Model | expand |
On 29/11/2023 12:08, Lukasz Luba wrote: > Split the process of allocation and data initialization for the EM table. > The upcoming changes for modifiable EM will use it. > > This change is not expected to alter the general functionality. NIT: IMHO, I guess you wanted to say: "No functional changes introduced"? I.e. all not only general functionality ... [...] > static int em_create_pd(struct device *dev, int nr_states, > @@ -234,11 +234,15 @@ static int em_create_pd(struct device *dev, int nr_states, > return -ENOMEM; > } > > - ret = em_create_perf_table(dev, pd, nr_states, cb, flags); > - if (ret) { > - kfree(pd); > - return ret; > - } > + pd->nr_perf_states = nr_states; Why does `pd->nr_perf_states = nr_states;` have to move from em_create_perf_table() to em_create_pd()? > + > + ret = em_allocate_perf_table(pd, nr_states); > + if (ret) > + goto free_pd; > + > + ret = em_create_perf_table(dev, pd, pd->table, nr_states, cb, flags); If you set it in em_create_pd() then you can use 'pd->nr_perf_states' in em_create_perf_table() and doesn't have to pass `nr_states`. [...]
On 11/29/23 11:08, Lukasz Luba wrote: > Split the process of allocation and data initialization for the EM table. > The upcoming changes for modifiable EM will use it. > > This change is not expected to alter the general functionality. > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > kernel/power/energy_model.c | 52 ++++++++++++++++++++++--------------- > 1 file changed, 31 insertions(+), 21 deletions(-) > > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c > index 3c8542443dd4..99426b5eedb6 100644 > --- a/kernel/power/energy_model.c > +++ b/kernel/power/energy_model.c > @@ -142,18 +142,25 @@ static int em_compute_costs(struct device *dev, struct em_perf_state *table, > return 0; > } > > +static int em_allocate_perf_table(struct em_perf_domain *pd, > + int nr_states) > +{ > + pd->table = kcalloc(nr_states, sizeof(struct em_perf_state), > + GFP_KERNEL); > + if (!pd->table) > + return -ENOMEM; > + > + return 0; > +} > + > static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, > + struct em_perf_state *table, > int nr_states, struct em_data_callback *cb, > unsigned long flags) > { > unsigned long power, freq, prev_freq = 0; > - struct em_perf_state *table; > int i, ret; > > - table = kcalloc(nr_states, sizeof(*table), GFP_KERNEL); > - if (!table) > - return -ENOMEM; > - > /* Build the list of performance states for this performance domain */ > for (i = 0, freq = 0; i < nr_states; i++, freq++) { > /* > @@ -165,7 +172,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, > if (ret) { > dev_err(dev, "EM: invalid perf. state: %d\n", > ret); > - goto free_ps_table; > + return -EINVAL; > } > > /* > @@ -175,7 +182,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, > if (freq <= prev_freq) { > dev_err(dev, "EM: non-increasing freq: %lu\n", > freq); > - goto free_ps_table; > + return -EINVAL; > } > > /* > @@ -185,7 +192,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, > if (!power || power > EM_MAX_POWER) { > dev_err(dev, "EM: invalid power: %lu\n", > power); > - goto free_ps_table; > + return -EINVAL; > } > > table[i].power = power; > @@ -194,16 +201,9 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, > > ret = em_compute_costs(dev, table, cb, nr_states, flags); > if (ret) > - goto free_ps_table; We don't care about propagating the error number here stored in ret? > - > - pd->table = table; > - pd->nr_perf_states = nr_states; > + return -EINVAL; > > return 0; > - > -free_ps_table: > - kfree(table); > - return -EINVAL; > } > > static int em_create_pd(struct device *dev, int nr_states, > @@ -234,11 +234,15 @@ static int em_create_pd(struct device *dev, int nr_states, > return -ENOMEM; > } > > - ret = em_create_perf_table(dev, pd, nr_states, cb, flags); > - if (ret) { > - kfree(pd); > - return ret; > - } > + pd->nr_perf_states = nr_states; > + > + ret = em_allocate_perf_table(pd, nr_states); > + if (ret) > + goto free_pd; > + > + ret = em_create_perf_table(dev, pd, pd->table, nr_states, cb, flags); > + if (ret) > + goto free_pd_table; Ditto for all the above Cheers -- Qais Yousef > > if (_is_cpu_device(dev)) > for_each_cpu(cpu, cpus) { > @@ -249,6 +253,12 @@ static int em_create_pd(struct device *dev, int nr_states, > dev->em_pd = pd; > > return 0; > + > +free_pd_table: > + kfree(pd->table); > +free_pd: > + kfree(pd); > + return -EINVAL; > } > > static void > -- > 2.25.1 >
On 12/12/23 18:50, Dietmar Eggemann wrote: > On 29/11/2023 12:08, Lukasz Luba wrote: >> Split the process of allocation and data initialization for the EM table. >> The upcoming changes for modifiable EM will use it. >> >> This change is not expected to alter the general functionality. > > NIT: IMHO, I guess you wanted to say: "No functional changes > introduced"? I.e. all not only general functionality ... > Yes 'no functional changes'. Rafael gave me that sense once - and I use in such cases. > [...] > >> static int em_create_pd(struct device *dev, int nr_states, >> @@ -234,11 +234,15 @@ static int em_create_pd(struct device *dev, int nr_states, >> return -ENOMEM; >> } >> >> - ret = em_create_perf_table(dev, pd, nr_states, cb, flags); >> - if (ret) { >> - kfree(pd); >> - return ret; >> - } >> + pd->nr_perf_states = nr_states; > > Why does `pd->nr_perf_states = nr_states;` have to move from > em_create_perf_table() to em_create_pd()? Because I have split the old code which did allocation and initialization w/ data the in em_create_perf_table(). Now we are going to have separate: 1. allocation of a new table (which can be re-used later) 2. initialization of the data (power, freq, etc) in registration code path It will allow to also allow to introduce update data function, and simply use the same allocation function for both cases: - EM registration code path - update EM code path > >> + >> + ret = em_allocate_perf_table(pd, nr_states); >> + if (ret) >> + goto free_pd; >> + >> + ret = em_create_perf_table(dev, pd, pd->table, nr_states, cb, flags); > > If you set it in em_create_pd() then you can use 'pd->nr_perf_states' in > em_create_perf_table() and doesn't have to pass `nr_states`. > > [...] That's true. I could further refactor that function and remove that 'nr_states' argument. I'll do this in v6. Thanks!
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c index 3c8542443dd4..99426b5eedb6 100644 --- a/kernel/power/energy_model.c +++ b/kernel/power/energy_model.c @@ -142,18 +142,25 @@ static int em_compute_costs(struct device *dev, struct em_perf_state *table, return 0; } +static int em_allocate_perf_table(struct em_perf_domain *pd, + int nr_states) +{ + pd->table = kcalloc(nr_states, sizeof(struct em_perf_state), + GFP_KERNEL); + if (!pd->table) + return -ENOMEM; + + return 0; +} + static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, + struct em_perf_state *table, int nr_states, struct em_data_callback *cb, unsigned long flags) { unsigned long power, freq, prev_freq = 0; - struct em_perf_state *table; int i, ret; - table = kcalloc(nr_states, sizeof(*table), GFP_KERNEL); - if (!table) - return -ENOMEM; - /* Build the list of performance states for this performance domain */ for (i = 0, freq = 0; i < nr_states; i++, freq++) { /* @@ -165,7 +172,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, if (ret) { dev_err(dev, "EM: invalid perf. state: %d\n", ret); - goto free_ps_table; + return -EINVAL; } /* @@ -175,7 +182,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, if (freq <= prev_freq) { dev_err(dev, "EM: non-increasing freq: %lu\n", freq); - goto free_ps_table; + return -EINVAL; } /* @@ -185,7 +192,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, if (!power || power > EM_MAX_POWER) { dev_err(dev, "EM: invalid power: %lu\n", power); - goto free_ps_table; + return -EINVAL; } table[i].power = power; @@ -194,16 +201,9 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, ret = em_compute_costs(dev, table, cb, nr_states, flags); if (ret) - goto free_ps_table; - - pd->table = table; - pd->nr_perf_states = nr_states; + return -EINVAL; return 0; - -free_ps_table: - kfree(table); - return -EINVAL; } static int em_create_pd(struct device *dev, int nr_states, @@ -234,11 +234,15 @@ static int em_create_pd(struct device *dev, int nr_states, return -ENOMEM; } - ret = em_create_perf_table(dev, pd, nr_states, cb, flags); - if (ret) { - kfree(pd); - return ret; - } + pd->nr_perf_states = nr_states; + + ret = em_allocate_perf_table(pd, nr_states); + if (ret) + goto free_pd; + + ret = em_create_perf_table(dev, pd, pd->table, nr_states, cb, flags); + if (ret) + goto free_pd_table; if (_is_cpu_device(dev)) for_each_cpu(cpu, cpus) { @@ -249,6 +253,12 @@ static int em_create_pd(struct device *dev, int nr_states, dev->em_pd = pd; return 0; + +free_pd_table: + kfree(pd->table); +free_pd: + kfree(pd); + return -EINVAL; } static void
Split the process of allocation and data initialization for the EM table. The upcoming changes for modifiable EM will use it. This change is not expected to alter the general functionality. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- kernel/power/energy_model.c | 52 ++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 21 deletions(-)