| Message ID | 20231129065748.19871-1-quic_sibis@quicinc.com |
|---|---|
| Headers | show |
| Series | firmware: arm_scmi: Miscellaneous fixes and opp count increase | expand |
On Wed, Nov 29, 2023 at 12:27:47PM +0530, Sibi Sankar wrote: > Fix frequency and power truncation seen in the performance protocol by > casting it with the correct type. > While I always remembered to handle this when reviewing the spec, seem to have forgotten when it came to handling in the implementation :(. Thanks for spotting this. However I don't like the ugly type casting. I think we can do better. Also looking at the code around the recently added level index mode, I think we can simplify things like below patch. Cristian, What do you think ? Regards, Sudeep -->8 drivers/firmware/arm_scmi/perf.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c index a648521e04a3..2e828b29efab 100644 --- a/drivers/firmware/arm_scmi/perf.c +++ b/drivers/firmware/arm_scmi/perf.c @@ -268,13 +268,14 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph, dom_info->sustained_perf_level = le32_to_cpu(attr->sustained_perf_level); if (!dom_info->sustained_freq_khz || - !dom_info->sustained_perf_level) + !dom_info->sustained_perf_level || + dom_info->level_indexing_mode) /* CPUFreq converts to kHz, hence default 1000 */ dom_info->mult_factor = 1000; else dom_info->mult_factor = - (dom_info->sustained_freq_khz * 1000) / - dom_info->sustained_perf_level; + (dom_info->sustained_freq_khz * 1000UL) + / dom_info->sustained_perf_level; strscpy(dom_info->info.name, attr->name, SCMI_SHORT_NAME_MAX_SIZE); } @@ -804,9 +805,10 @@ static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph, for (idx = 0; idx < dom->opp_count; idx++) { if (!dom->level_indexing_mode) - freq = dom->opp[idx].perf * dom->mult_factor; + freq = dom->opp[idx].perf; else - freq = dom->opp[idx].indicative_freq * 1000; + freq = dom->opp[idx].indicative_freq; + freq *= dom->mult_factor; data.level = dom->opp[idx].perf; data.freq = freq; @@ -879,7 +881,7 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain, return ret; if (!dom->level_indexing_mode) { - *freq = level * dom->mult_factor; + *freq = level; } else { struct scmi_opp *opp; @@ -887,8 +889,9 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain, if (!opp) return -EIO; - *freq = opp->indicative_freq * 1000; + *freq = opp->indicative_freq; } + freq *= dom->mult_factor; return ret; } @@ -908,9 +911,10 @@ static int scmi_dvfs_est_power_get(const struct scmi_protocol_handle *ph, for (opp = dom->opp, idx = 0; idx < dom->opp_count; idx++, opp++) { if (!dom->level_indexing_mode) - opp_freq = opp->perf * dom->mult_factor; + opp_freq = opp->perf; else - opp_freq = opp->indicative_freq * 1000; + opp_freq = opp->indicative_freq; + opp_freq *= dom->mult_factor; if (opp_freq < *freq) continue;
On Thu, Nov 30, 2023 at 12:05:06PM +0000, Sudeep Holla wrote: > On Wed, Nov 29, 2023 at 12:27:47PM +0530, Sibi Sankar wrote: > > Fix frequency and power truncation seen in the performance protocol by > > casting it with the correct type. > > > > While I always remembered to handle this when reviewing the spec, seem to > have forgotten when it came to handling in the implementation :(. Thanks > for spotting this. > > However I don't like the ugly type casting. I think we can do better. Also > looking at the code around the recently added level index mode, I think we > can simplify things like below patch. > > Cristian, > What do you think ? > Hi the cleanup seems nice in general to compact the mult_factor multipliers in one place, and regarding addressing the problem of truncation without the need of the explicit casting, should not be enough to change to additionally also change mult_factor to be an u64 ? Not tested so I could miss something... Thanks, Cristian > Regards, > Sudeep > > -->8 > > drivers/firmware/arm_scmi/perf.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > index a648521e04a3..2e828b29efab 100644 > --- a/drivers/firmware/arm_scmi/perf.c > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -268,13 +268,14 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph, > dom_info->sustained_perf_level = > le32_to_cpu(attr->sustained_perf_level); > if (!dom_info->sustained_freq_khz || > - !dom_info->sustained_perf_level) > + !dom_info->sustained_perf_level || > + dom_info->level_indexing_mode) > /* CPUFreq converts to kHz, hence default 1000 */ > dom_info->mult_factor = 1000; > else > dom_info->mult_factor = > - (dom_info->sustained_freq_khz * 1000) / > - dom_info->sustained_perf_level; > + (dom_info->sustained_freq_khz * 1000UL) > + / dom_info->sustained_perf_level; > strscpy(dom_info->info.name, attr->name, > SCMI_SHORT_NAME_MAX_SIZE); > } > @@ -804,9 +805,10 @@ static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph, > > for (idx = 0; idx < dom->opp_count; idx++) { > if (!dom->level_indexing_mode) > - freq = dom->opp[idx].perf * dom->mult_factor; > + freq = dom->opp[idx].perf; > else > - freq = dom->opp[idx].indicative_freq * 1000; > + freq = dom->opp[idx].indicative_freq; > + freq *= dom->mult_factor; > > data.level = dom->opp[idx].perf; > data.freq = freq; > @@ -879,7 +881,7 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain, > return ret; > > if (!dom->level_indexing_mode) { > - *freq = level * dom->mult_factor; > + *freq = level; > } else { > struct scmi_opp *opp; > > @@ -887,8 +889,9 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain, > if (!opp) > return -EIO; > > - *freq = opp->indicative_freq * 1000; > + *freq = opp->indicative_freq; > } > + freq *= dom->mult_factor; > > return ret; > } > @@ -908,9 +911,10 @@ static int scmi_dvfs_est_power_get(const struct scmi_protocol_handle *ph, > > for (opp = dom->opp, idx = 0; idx < dom->opp_count; idx++, opp++) { > if (!dom->level_indexing_mode) > - opp_freq = opp->perf * dom->mult_factor; > + opp_freq = opp->perf; > else > - opp_freq = opp->indicative_freq * 1000; > + opp_freq = opp->indicative_freq; > + opp_freq *= dom->mult_factor; > > if (opp_freq < *freq) > continue; >
On Wed, Nov 29, 2023 at 12:27:48PM +0530, Sibi Sankar wrote: > The number of opps on certain variants of the X1E80100 SoC are greater > than current maximum, so increase the MAX_OPP count to the next log level > to accommodate that. > Hi, > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > drivers/firmware/arm_scmi/perf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > index 3344ce3a2026..edf34a3c4d6a 100644 > --- a/drivers/firmware/arm_scmi/perf.c > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -24,7 +24,7 @@ > #include "protocols.h" > #include "notify.h" > > -#define MAX_OPPS 16 > +#define MAX_OPPS 24 > There is an hashtable, opps_by_freq, sized by an ilog2().... ....so, can we stick to a power-of-2 like 32 instead ? (and be more future proof too...) Other than this, LGTM Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Thanks, Cristian
On Thu, Nov 30, 2023 at 12:49:42PM +0000, Cristian Marussi wrote: > On Thu, Nov 30, 2023 at 12:05:06PM +0000, Sudeep Holla wrote: > > On Wed, Nov 29, 2023 at 12:27:47PM +0530, Sibi Sankar wrote: > > > Fix frequency and power truncation seen in the performance protocol by > > > casting it with the correct type. > > > > > > > While I always remembered to handle this when reviewing the spec, seem to > > have forgotten when it came to handling in the implementation :(. Thanks > > for spotting this. > > > > However I don't like the ugly type casting. I think we can do better. Also > > looking at the code around the recently added level index mode, I think we > > can simplify things like below patch. > > > > Cristian, > > What do you think ? > > > > Hi > > the cleanup seems nice in general to compact the mult_factor multipliers > in one place, and regarding addressing the problem of truncation without > the need of the explicit casting, should not be enough to change to > additionally also change mult_factor to be an u64 ? > I started exactly with that, but when I completed the patch, there was no explicit need for it, so dropped it again. I can bump mult_factor to be u64 but do you see any other place that would need it apart from having single statement that does multiplication and assignment ? I am exploiting the conditional based on level_indexing_mode here but I agree it may help in backporting if I make mult_factor u64.
On Thu, Nov 30, 2023 at 01:56:56PM +0000, Sudeep Holla wrote: > On Thu, Nov 30, 2023 at 12:49:42PM +0000, Cristian Marussi wrote: > > On Thu, Nov 30, 2023 at 12:05:06PM +0000, Sudeep Holla wrote: > > > On Wed, Nov 29, 2023 at 12:27:47PM +0530, Sibi Sankar wrote: > > > > Fix frequency and power truncation seen in the performance protocol by > > > > casting it with the correct type. > > > > > > > > > > While I always remembered to handle this when reviewing the spec, seem to > > > have forgotten when it came to handling in the implementation :(. Thanks > > > for spotting this. > > > > > > However I don't like the ugly type casting. I think we can do better. Also > > > looking at the code around the recently added level index mode, I think we > > > can simplify things like below patch. > > > > > > Cristian, > > > What do you think ? > > > > > > > Hi > > > > the cleanup seems nice in general to compact the mult_factor multipliers > > in one place, and regarding addressing the problem of truncation without > > the need of the explicit casting, should not be enough to change to > > additionally also change mult_factor to be an u64 ? > > > > I started exactly with that, but when I completed the patch, there was no > explicit need for it, so dropped it again. I can bump mult_factor to be > u64 but do you see any other place that would need it apart from having > single statement that does multiplication and assignment ? I am exploiting > the conditional based on level_indexing_mode here but I agree it may help > in backporting if I make mult_factor u64. > Ah right freq *= dom->multi_fact; does the trick..but cannot this by itself (under unplausibl conds) overflow and does not fit into a u32 mult_factor ? dom_info->mult_factor = (dom_info->sustained_freq_khz * 1000UL) / dom_info->sustained_perf_level; Thanks, Cristian
On 11/30/23 18:30, Cristian Marussi wrote: > On Wed, Nov 29, 2023 at 12:27:48PM +0530, Sibi Sankar wrote: >> The number of opps on certain variants of the X1E80100 SoC are greater >> than current maximum, so increase the MAX_OPP count to the next log level >> to accommodate that. >> > > Hi, Hey Cristian, Thanks for taking time to review the series. > > >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> drivers/firmware/arm_scmi/perf.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c >> index 3344ce3a2026..edf34a3c4d6a 100644 >> --- a/drivers/firmware/arm_scmi/perf.c >> +++ b/drivers/firmware/arm_scmi/perf.c >> @@ -24,7 +24,7 @@ >> #include "protocols.h" >> #include "notify.h" >> >> -#define MAX_OPPS 16 >> +#define MAX_OPPS 24 >> > > There is an hashtable, opps_by_freq, sized by an ilog2().... > > ....so, can we stick to a power-of-2 like 32 instead ? > (and be more future proof too...) Thanks, will get this changed in the next re-spin. > > Other than this, LGTM > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > Thanks, > Cristian
On 11/30/23 21:55, Cristian Marussi wrote: > On Thu, Nov 30, 2023 at 01:56:56PM +0000, Sudeep Holla wrote: >> On Thu, Nov 30, 2023 at 12:49:42PM +0000, Cristian Marussi wrote: >>> On Thu, Nov 30, 2023 at 12:05:06PM +0000, Sudeep Holla wrote: >>>> On Wed, Nov 29, 2023 at 12:27:47PM +0530, Sibi Sankar wrote: >>>>> Fix frequency and power truncation seen in the performance protocol by >>>>> casting it with the correct type. >>>>> >>>> >>>> While I always remembered to handle this when reviewing the spec, seem to >>>> have forgotten when it came to handling in the implementation :(. Thanks >>>> for spotting this. >>>> >>>> However I don't like the ugly type casting. I think we can do better. Also >>>> looking at the code around the recently added level index mode, I think we >>>> can simplify things like below patch. >>>> >>>> Cristian, >>>> What do you think ? >>>> >>> >>> Hi >>> >>> the cleanup seems nice in general to compact the mult_factor multipliers >>> in one place, and regarding addressing the problem of truncation without >>> the need of the explicit casting, should not be enough to change to >>> additionally also change mult_factor to be an u64 ? >>> >> >> I started exactly with that, but when I completed the patch, there was no >> explicit need for it, so dropped it again. I can bump mult_factor to be >> u64 but do you see any other place that would need it apart from having >> single statement that does multiplication and assignment ? I am exploiting >> the conditional based on level_indexing_mode here but I agree it may help >> in backporting if I make mult_factor u64. >> > > Ah right > > freq *= dom->multi_fact; > > does the trick..but cannot this by itself (under unplausibl conds) > overflow and does not fit into a u32 mult_factor ? > > dom_info->mult_factor = > (dom_info->sustained_freq_khz * 1000UL) wouldn't having the 1000UL ensure that we don't truncate though? Anyway will drop the patch when I re-spin the series. -Sibi > / dom_info->sustained_perf_level; > > > Thanks, > Cristian >
On Thu, Nov 30, 2023 at 04:25:44PM +0000, Cristian Marussi wrote: > On Thu, Nov 30, 2023 at 01:56:56PM +0000, Sudeep Holla wrote: > > I started exactly with that, but when I completed the patch, there was no > > explicit need for it, so dropped it again. I can bump mult_factor to be > > u64 but do you see any other place that would need it apart from having > > single statement that does multiplication and assignment ? I am exploiting > > the conditional based on level_indexing_mode here but I agree it may help > > in backporting if I make mult_factor u64. > > > > Ah right > > freq *= dom->multi_fact; > > does the trick..but cannot this by itself (under unplausibl conds) > overflow and does not fit into a u32 mult_factor ? > > dom_info->mult_factor = > (dom_info->sustained_freq_khz * 1000UL) > / dom_info->sustained_perf_level; Agreed. Also thinking about backports, I think making it u64 is simple fix. I will also thinking of splitting the changes so that fixes are more appropriate. I will try to post something soonish. -- Regards, Sudeep
On Fri, Dec 01, 2023 at 01:02:25AM +0530, Sibi Sankar wrote: > > On 11/30/23 21:55, Cristian Marussi wrote: > > > > Ah right > > > > freq *= dom->multi_fact; > > > > does the trick..but cannot this by itself (under unplausibl conds) > > overflow and does not fit into a u32 mult_factor ? > > > > dom_info->mult_factor = > > (dom_info->sustained_freq_khz * 1000UL) > > wouldn't having the 1000UL ensure that we don't truncate though? Correct but the point was mult_factor itself can be >= 2^32 > Anyway will drop the patch when I re-spin the series. > Are you re-spining just to change 24 to 32 in PATCH 3/3, if so no need. I have already applied 1 and 3 here[1]. Just waiting for the builder results to confirm it -- Regards, Sudeep [1] https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi/updates
On Wed, 29 Nov 2023 12:27:45 +0530, Sibi Sankar wrote: > The patch series includes bug fixes identified while testing the > performance protocol on the X1E80100 SoC. It also includes an > increase of the maximum opps count to the next log level. > > base tree: next-20231129 > > Sibi Sankar (3): > firmware: arm_scmi: Fix null pointer dereference during fastchannel init > firmware: arm_scmi: Fix freq/power truncation in the perf protocol > firmware: arm_scmi: Increase the maximum opp count > > [...] Applied to sudeep.holla/linux (for-next/scmi/updates), thanks! [1/3] firmware: arm_scmi: Fix null pointer dereference during fastchannel init https://git.kernel.org/sudeep.holla/c/3cc12bb83e67 [3/3] firmware: arm_scmi: Increase the maximum opp count https://git.kernel.org/sudeep.holla/c/c3f17d5f89fc -- Regards, Sudeep
The patch series includes bug fixes identified while testing the performance protocol on the X1E80100 SoC. It also includes an increase of the maximum opps count to the next log level. base tree: next-20231129 Sibi Sankar (3): firmware: arm_scmi: Fix null pointer dereference during fastchannel init firmware: arm_scmi: Fix freq/power truncation in the perf protocol firmware: arm_scmi: Increase the maximum opp count drivers/firmware/arm_scmi/perf.c | 33 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 19 deletions(-)