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(-)
--
2.17.1
Fix frequency and power truncation seen in the performance protocol by
casting it with the correct type.
Fixes: a9e3fbfaa0ff ("firmware: arm_scmi: add initial support for performance protocol")
Signed-off-by: Sibi Sankar <[email protected]>
---
drivers/firmware/arm_scmi/perf.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index a648521e04a3..3344ce3a2026 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -804,9 +804,9 @@ 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 = (unsigned long)dom->opp[idx].perf * dom->mult_factor;
else
- freq = dom->opp[idx].indicative_freq * 1000;
+ freq = (unsigned long)dom->opp[idx].indicative_freq * 1000;
data.level = dom->opp[idx].perf;
data.freq = freq;
@@ -879,7 +879,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 = (unsigned long)level * dom->mult_factor;
} else {
struct scmi_opp *opp;
@@ -887,7 +887,7 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain,
if (!opp)
return -EIO;
- *freq = opp->indicative_freq * 1000;
+ *freq = (unsigned long)opp->indicative_freq * 1000;
}
return ret;
@@ -908,9 +908,9 @@ 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 = (unsigned long)opp->perf * dom->mult_factor;
else
- opp_freq = opp->indicative_freq * 1000;
+ opp_freq = (unsigned long)opp->indicative_freq * 1000;
if (opp_freq < *freq)
continue;
--
2.17.1
The scmi_perf_domain_lookup requires the protocol handle to have the
private data set, which is yet to happen during the fastchannel init
scenario. This results in a null pointer dereference. Fix this by using
the pre-populated perf_dom_info to pass on the required information
instead.
Call trace:
scmi_perf_protocol_init+0x434/0x678
scmi_get_protocol_instance+0x168/0x29c
scmi_devres_protocol_instance_get+0x50/0xa0
scmi_devm_protocol_get+0x20/0x50
scmi_cpufreq_probe+0x34/0xd4
scmi_dev_probe+0x28/0x3c
really_probe+0x148/0x2ac
__driver_probe_device+0x78/0x12c
driver_probe_device+0x40/0x160
__device_attach_driver+0xb8/0x134
bus_for_each_drv+0x80/0xdc
__device_attach+0xa8/0x1b0
device_initial_probe+0x14/0x20
bus_probe_device+0xa8/0xac
device_add+0x5cc/0x778
device_register+0x20/0x30
__scmi_device_create.part.0+0xec/0x1cc
scmi_device_create+0x180/0x1c4
scmi_create_protocol_devices+0x4c/0xb0
scmi_probe+0x660/0x738
platform_probe+0x68/0xdc
really_probe+0x148/0x2ac
__driver_probe_device+0x78/0x12c
driver_probe_device+0x40/0x160
__device_attach_driver+0xb8/0x134
bus_for_each_drv+0x80/0xdc
__device_attach+0xa8/0x1b0
device_initial_probe+0x14/0x20
bus_probe_device+0xa8/0xac
deferred_probe_work_func+0x88/0xc0
process_one_work+0x13c/0x264
worker_thread+0x32c/0x438
kthread+0x118/0x11c
ret_from_fork+0x10/0x20
Fixes: 619bc6e034f3 ("firmware: arm_scmi: Populate fastchannel info only if set operations are allowed")
Signed-off-by: Sibi Sankar <[email protected]>
---
drivers/firmware/arm_scmi/perf.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index d1323c5d9c27..a648521e04a3 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -759,40 +759,35 @@ static int scmi_perf_level_limits_notify(const struct scmi_protocol_handle *ph,
}
static void scmi_perf_domain_init_fc(const struct scmi_protocol_handle *ph,
- u32 domain, struct scmi_fc_info **p_fc)
+ struct perf_dom_info *dom)
{
struct scmi_fc_info *fc;
- struct perf_dom_info *dom;
-
- dom = scmi_perf_domain_lookup(ph, domain);
- if (IS_ERR(dom))
- return;
fc = devm_kcalloc(ph->dev, PERF_FC_MAX, sizeof(*fc), GFP_KERNEL);
if (!fc)
return;
ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
- PERF_LEVEL_GET, 4, domain,
+ PERF_LEVEL_GET, 4, dom->id,
&fc[PERF_FC_LEVEL].get_addr, NULL);
ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
- PERF_LIMITS_GET, 8, domain,
+ PERF_LIMITS_GET, 8, dom->id,
&fc[PERF_FC_LIMIT].get_addr, NULL);
if (dom->info.set_perf)
ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
- PERF_LEVEL_SET, 4, domain,
+ PERF_LEVEL_SET, 4, dom->id,
&fc[PERF_FC_LEVEL].set_addr,
&fc[PERF_FC_LEVEL].set_db);
if (dom->set_limits)
ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
- PERF_LIMITS_SET, 8, domain,
+ PERF_LIMITS_SET, 8, dom->id,
&fc[PERF_FC_LIMIT].set_addr,
&fc[PERF_FC_LIMIT].set_db);
- *p_fc = fc;
+ dom->fc_info = fc;
}
static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
@@ -1102,7 +1097,7 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
scmi_perf_describe_levels_get(ph, dom, version);
if (dom->perf_fastchannels)
- scmi_perf_domain_init_fc(ph, dom->id, &dom->fc_info);
+ scmi_perf_domain_init_fc(ph, dom);
}
ret = devm_add_action_or_reset(ph->dev, scmi_perf_xa_destroy, pinfo);
--
2.17.1
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.
Signed-off-by: Sibi Sankar <[email protected]>
---
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
enum scmi_performance_protocol_cmd {
PERF_DOMAIN_ATTRIBUTES = 0x3,
--
2.17.1
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 Wed, Nov 29, 2023 at 12:27:46PM +0530, Sibi Sankar wrote:
> The scmi_perf_domain_lookup requires the protocol handle to have the
> private data set, which is yet to happen during the fastchannel init
> scenario. This results in a null pointer dereference. Fix this by using
> the pre-populated perf_dom_info to pass on the required information
> instead.
>
> Call trace:
> scmi_perf_protocol_init+0x434/0x678
> scmi_get_protocol_instance+0x168/0x29c
> scmi_devres_protocol_instance_get+0x50/0xa0
> scmi_devm_protocol_get+0x20/0x50
> scmi_cpufreq_probe+0x34/0xd4
> scmi_dev_probe+0x28/0x3c
> really_probe+0x148/0x2ac
> __driver_probe_device+0x78/0x12c
> driver_probe_device+0x40/0x160
> __device_attach_driver+0xb8/0x134
> bus_for_each_drv+0x80/0xdc
> __device_attach+0xa8/0x1b0
> device_initial_probe+0x14/0x20
> bus_probe_device+0xa8/0xac
> device_add+0x5cc/0x778
> device_register+0x20/0x30
> __scmi_device_create.part.0+0xec/0x1cc
> scmi_device_create+0x180/0x1c4
> scmi_create_protocol_devices+0x4c/0xb0
> scmi_probe+0x660/0x738
> platform_probe+0x68/0xdc
> really_probe+0x148/0x2ac
> __driver_probe_device+0x78/0x12c
> driver_probe_device+0x40/0x160
> __device_attach_driver+0xb8/0x134
> bus_for_each_drv+0x80/0xdc
> __device_attach+0xa8/0x1b0
> device_initial_probe+0x14/0x20
> bus_probe_device+0xa8/0xac
> deferred_probe_work_func+0x88/0xc0
> process_one_work+0x13c/0x264
> worker_thread+0x32c/0x438
> kthread+0x118/0x11c
> ret_from_fork+0x10/0x20
>
> Fixes: 619bc6e034f3 ("firmware: arm_scmi: Populate fastchannel info only if set operations are allowed")
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
Yes indeed, I made this same error in the past and then I missed it when
reviewing the offending patch :< ...
Reviewed-by: Cristian Marussi <[email protected]>
Thanks,
Cristian
> drivers/firmware/arm_scmi/perf.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index d1323c5d9c27..a648521e04a3 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -759,40 +759,35 @@ static int scmi_perf_level_limits_notify(const struct scmi_protocol_handle *ph,
> }
>
> static void scmi_perf_domain_init_fc(const struct scmi_protocol_handle *ph,
> - u32 domain, struct scmi_fc_info **p_fc)
> + struct perf_dom_info *dom)
> {
> struct scmi_fc_info *fc;
> - struct perf_dom_info *dom;
> -
> - dom = scmi_perf_domain_lookup(ph, domain);
> - if (IS_ERR(dom))
> - return;
>
> fc = devm_kcalloc(ph->dev, PERF_FC_MAX, sizeof(*fc), GFP_KERNEL);
> if (!fc)
> return;
>
> ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> - PERF_LEVEL_GET, 4, domain,
> + PERF_LEVEL_GET, 4, dom->id,
> &fc[PERF_FC_LEVEL].get_addr, NULL);
>
> ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> - PERF_LIMITS_GET, 8, domain,
> + PERF_LIMITS_GET, 8, dom->id,
> &fc[PERF_FC_LIMIT].get_addr, NULL);
>
> if (dom->info.set_perf)
> ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> - PERF_LEVEL_SET, 4, domain,
> + PERF_LEVEL_SET, 4, dom->id,
> &fc[PERF_FC_LEVEL].set_addr,
> &fc[PERF_FC_LEVEL].set_db);
>
> if (dom->set_limits)
> ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> - PERF_LIMITS_SET, 8, domain,
> + PERF_LIMITS_SET, 8, dom->id,
> &fc[PERF_FC_LIMIT].set_addr,
> &fc[PERF_FC_LIMIT].set_db);
>
> - *p_fc = fc;
> + dom->fc_info = fc;
> }
>
> static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
> @@ -1102,7 +1097,7 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
> scmi_perf_describe_levels_get(ph, dom, version);
>
> if (dom->perf_fastchannels)
> - scmi_perf_domain_init_fc(ph, dom->id, &dom->fc_info);
> + scmi_perf_domain_init_fc(ph, dom);
> }
>
> ret = devm_add_action_or_reset(ph->dev, scmi_perf_xa_destroy, pinfo);
> --
> 2.17.1
>
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 <[email protected]>
> ---
> 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 <[email protected]>
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.
--
Regards,
Sudeep
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> 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