Hi all,
this series introduces a bunch of SCMIv3.1 miscellaneous changes to support
basically all the SCMIv3.1 specification [1] addition with the exclusion of
the Powercap protocol and driver which will be introduced later on in
another series.
Most notably the series adds:
- supports across all protocols for long resources naming using *_NAME_GET
dedicated new commands
- Clock protocol Rate change pre and post notifications
- Voltage protocol asynchronous voltage level set command
(VOLTAGE_LEVEL_SET_COMPLETE delayed response)
- Perf protocol power-cost in micro-watts (only internal support)
- Perf protocol PERFORMANCE_LIMITS_SET new checks
Beside this, the series starts with a few general fixes (01-08/22) and a
couple of refactoring:
- one (09/22) simply to split out of common.h into a new protocols.h all
the structures needed only by protocol code, so that the protocol
implementation can include such reduced header instead of the whole
common.h which was growing insanely
- another around the handling of multi-part commands.
SCMI already allowed to issue some commands using a multi-message scheme
through which a particularly big response, which could possibly not fit
the underlying transport max payload size, could have been split into
multiple chunks.
Such logic, though, was scattered all across various protocols, leading
to a lot of code duplication, so, before adding even more duplication
with SCMIv3.1, I split out the common logic to a couple of helpers to
handle such mechanism in a common way through some abstract iterators
(13/22) and then ported all the protocols users of such mechanism to the
common iterators (14,16-18/22).
SCMIv3.1 new features have been tested with an emulated backend server,
while refactoring has been chekced against a standard SCMI stack running
on a JUNO.
The series is based on sudeep/for-next/scmi/updates [2] on top of
commit 38a0e5b735d6 ("clk: scmi: Support atomic clock enable/disable API")
Any feedback very welcome,
Thanks,
Cristian
[1]: https://developer.arm.com/documentation/den0056/d/?lang=en
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi/updates
---
Cristian Marussi (22):
firmware: arm_scmi: Fix sorting of retrieved clock rates
firmware: arm_scmi: Make protocols init fail on basic errors
firmware: arm_scmi: Fix Base list protocols enumeration
firmware: arm_scmi: Validate BASE_DISCOVER_LIST_PROTOCOLS reply
firmware: arm_scmi: Dynamically allocate protocols array
firmware: arm_scmi: Make name_get operations return a const
firmware: arm_scmi: Check CLOCK_RATE_SET_COMPLETE async reply
firmware: arm_scmi: Remove unneeded NULL termination of clk name
firmware: arm_scmi: Split protocol specific definitions in a dedicated
header
firmware: arm_scmi: Introduce a common SCMIv3.1 .extended_name_get
helper
firmware: arm_scmi: Add SCMIv3.1 extended names protocols support
firmware: arm_scmi: Parse clock_enable_latency conditionally
firmware: arm_scmi: Add iterators for multi-part commands
firmware: arm_scmi: Use common iterators in Sensor protocol
firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support
firmware: arm_scmi: Use common iterators in Clock protocol
firmware: arm_scmi: Use common iterators in Voltage protocol
firmware: arm_scmi: Use common iterators in Perf protocol
firmware: arm_scmi: Add SCMIv3.1 Clock notifications
firmware: arm_scmi: Add SCMIv3.1 VOLTAGE_LEVEL_SET_COMPLETE
firmware: arm_scmi: Add SCMI v3.1 Perf power-cost in microwatts
firmware: arm_scmi: Add SCMIv3.1 PERFORMANCE_LIMITS_SET checks
drivers/firmware/arm_scmi/base.c | 43 +-
drivers/firmware/arm_scmi/clock.c | 337 +++++++++++---
drivers/firmware/arm_scmi/common.h | 225 +--------
drivers/firmware/arm_scmi/driver.c | 165 ++++++-
drivers/firmware/arm_scmi/perf.c | 162 ++++---
drivers/firmware/arm_scmi/power.c | 44 +-
drivers/firmware/arm_scmi/protocols.h | 318 +++++++++++++
drivers/firmware/arm_scmi/reset.c | 40 +-
drivers/firmware/arm_scmi/sensors.c | 645 +++++++++++++++-----------
drivers/firmware/arm_scmi/system.c | 9 +-
drivers/firmware/arm_scmi/voltage.c | 218 ++++++---
include/linux/scmi_protocol.h | 31 +-
12 files changed, 1495 insertions(+), 742 deletions(-)
create mode 100644 drivers/firmware/arm_scmi/protocols.h
--
2.32.0
During SCMI Clock protocol initialization, after having retrieved from the
SCMI platform all the available discrete rates for a specific clock, the
clock rates array is sorted, unfortunately using a pointer to its end as
a base instead of its start, so that sorting does not work.
Fix invocation of sort() passing as base a pointer to the start of the
retrieved clock rates array.
Fixes: dccec73de91 ("firmware: arm_scmi: Keep the discrete clock rates sorted")
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/clock.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index cf6fed6dec77..ef6431c6eb1c 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -210,7 +210,8 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
if (rate_discrete && rate) {
clk->list.num_rates = tot_rate_cnt;
- sort(rate, tot_rate_cnt, sizeof(*rate), rate_cmp_func, NULL);
+ sort(clk->list.rates, tot_rate_cnt, sizeof(*rate),
+ rate_cmp_func, NULL);
}
clk->rate_discrete = rate_discrete;
--
2.32.0
Do not blindly trust SCMI backend server reply about list of implemented
protocols, instead validate the reported length of the list of protocols
against the real payload size of the message reply.
Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index f279146f8110..c1165d1282ef 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
list = t->rx.buf + sizeof(*num_ret);
do {
+ size_t real_list_sz;
+ u32 calc_list_sz;
+
/* Set the number of protocols to be skipped/already read */
*num_skip = cpu_to_le32(tot_num_ret);
@@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
break;
}
+ if (t->rx.len < (sizeof(u32) * 2)) {
+ dev_err(dev, "Truncated reply - rx.len:%zd\n",
+ t->rx.len);
+ ret = -EPROTO;
+ break;
+ }
+
+ real_list_sz = t->rx.len - sizeof(u32);
+ calc_list_sz = ((loop_num_ret / sizeof(u32)) +
+ !!(loop_num_ret % sizeof(u32))) * sizeof(u32);
+ if (calc_list_sz != real_list_sz) {
+ dev_err(dev,
+ "Malformed reply - real_sz:%zd calc_sz:%u\n",
+ real_list_sz, calc_list_sz);
+ ret = -EPROTO;
+ break;
+ }
+
for (loop = 0; loop < loop_num_ret; loop++)
protocols_imp[tot_num_ret + loop] = *(list + loop);
--
2.32.0
When CLOCK_RATE_SET command is issued in asynchronous mode the delayed
response CLOCK_RATE_SET_COMPLETE comes back once the SCMI platform has
effectively operated the requested change: such delayed response carries
also the clock ID and the final clock rate that has been set.
As an aid to debug issues, check that the clock ID in the delayed
response matches the expected one and debug print the rate value.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/clock.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 0ae39ee920e9..32ccac457d20 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -68,6 +68,12 @@ struct scmi_clock_set_rate {
__le32 value_high;
};
+struct scmi_msg_resp_set_rate_complete {
+ __le32 id;
+ __le32 rate_low;
+ __le32 rate_high;
+};
+
struct clock_info {
u32 version;
int num_clocks;
@@ -266,10 +272,23 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
cfg->value_low = cpu_to_le32(rate & 0xffffffff);
cfg->value_high = cpu_to_le32(rate >> 32);
- if (flags & CLOCK_SET_ASYNC)
+ if (flags & CLOCK_SET_ASYNC) {
ret = ph->xops->do_xfer_with_response(ph, t);
- else
+ if (!ret) {
+ struct scmi_msg_resp_set_rate_complete *resp;
+
+ resp = t->rx.buf;
+ if (le32_to_cpu(resp->id) == clk_id)
+ dev_dbg(ph->dev,
+ "Clock ID %d set async to %llu\n",
+ clk_id,
+ get_unaligned_le64(&resp->rate_low));
+ else
+ ret = -EPROTO;
+ }
+ } else {
ret = ph->xops->do_xfer(ph, t);
+ }
if (ci->max_async_req)
atomic_dec(&ci->cur_async_req);
--
2.32.0
Add SCMIv3.1 internal support for parsing message attributes reporting
the capability of a performance domain to report power-cost in microwatts.
Cc: Lukasz Luba <[email protected]>
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/perf.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index e1aa0ed67971..65ffda5495d6 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -45,6 +45,7 @@ struct scmi_msg_resp_perf_attributes {
__le16 num_domains;
__le16 flags;
#define POWER_SCALE_IN_MILLIWATT(x) ((x) & BIT(0))
+#define POWER_SCALE_IN_MICROWATT(x) ((x) & BIT(1))
__le32 stats_addr_low;
__le32 stats_addr_high;
__le32 stats_size;
@@ -170,6 +171,7 @@ struct scmi_perf_info {
u32 version;
int num_domains;
bool power_scale_mw;
+ bool power_scale_uw;
u64 stats_addr;
u32 stats_size;
struct perf_dom_info *dom_info;
@@ -200,6 +202,8 @@ static int scmi_perf_attributes_get(const struct scmi_protocol_handle *ph,
pi->num_domains = le16_to_cpu(attr->num_domains);
pi->power_scale_mw = POWER_SCALE_IN_MILLIWATT(flags);
+ if (PROTOCOL_REV_MAJOR(pi->version) >= 0x3)
+ pi->power_scale_uw = POWER_SCALE_IN_MICROWATT(flags);
pi->stats_addr = le32_to_cpu(attr->stats_addr_low) |
(u64)le32_to_cpu(attr->stats_addr_high) << 32;
pi->stats_size = le32_to_cpu(attr->stats_size);
--
2.32.0
The clock_enable_latency field in CLOCK_ATTRIBUTES response message has
been added only since SCMI v3.1: use the advertised SCMI Clock protocol
version as a proper condition check for parsing it, instead of the bare
message length lookup.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/clock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 6dd4150b761b..b46b43a99871 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -130,8 +130,8 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
if (!ret) {
attributes = le32_to_cpu(attr->attributes);
strlcpy(clk->name, attr->name, SCMI_MAX_STR_SIZE);
- /* Is optional field clock_enable_latency provided ? */
- if (t->rx.len == sizeof(*attr))
+ /* clock_enable_latency field is present only since SCMI v3.1 */
+ if (PROTOCOL_REV_MAJOR(version) >= 0x2)
clk->enable_latency =
le32_to_cpu(attr->clock_enable_latency);
}
--
2.32.0
Make SCMI Clock protocol use the common iterator protocol helpers.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/clock.c | 150 ++++++++++++++++++------------
1 file changed, 90 insertions(+), 60 deletions(-)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index b46b43a99871..48ce7ef9c74b 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -162,81 +162,111 @@ static int rate_cmp_func(const void *_r1, const void *_r2)
return 1;
}
-static int
-scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
- struct scmi_clock_info *clk)
-{
- u64 *rate = NULL;
- int ret, cnt;
- bool rate_discrete = false;
- u32 tot_rate_cnt = 0, rates_flag;
- u16 num_returned, num_remaining;
- struct scmi_xfer *t;
- struct scmi_msg_clock_describe_rates *clk_desc;
- struct scmi_msg_resp_clock_describe_rates *rlist;
+struct scmi_clk_ipriv {
+ u32 clk_id;
+ struct scmi_clock_info *clk;
+};
- ret = ph->xops->xfer_get_init(ph, CLOCK_DESCRIBE_RATES,
- sizeof(*clk_desc), 0, &t);
- if (ret)
- return ret;
+static void iter_clk_describe_prepare_message(void *message,
+ const unsigned int desc_index,
+ const void *priv)
+{
+ struct scmi_msg_clock_describe_rates *msg = message;
+ const struct scmi_clk_ipriv *p = priv;
- clk_desc = t->tx.buf;
- rlist = t->rx.buf;
+ msg->id = cpu_to_le32(p->clk_id);
+ /* Set the number of rates to be skipped/already read */
+ msg->rate_index = cpu_to_le32(desc_index);
+}
- do {
- clk_desc->id = cpu_to_le32(clk_id);
- /* Set the number of rates to be skipped/already read */
- clk_desc->rate_index = cpu_to_le32(tot_rate_cnt);
+static int
+iter_clk_describe_update_state(struct scmi_iterator_state *st,
+ const void *response, void *priv)
+{
+ u32 flags;
+ struct scmi_clk_ipriv *p = priv;
+ const struct scmi_msg_resp_clock_describe_rates *r = response;
- ret = ph->xops->do_xfer(ph, t);
- if (ret)
- goto err;
+ flags = le32_to_cpu(r->num_rates_flags);
+ st->num_remaining = NUM_REMAINING(flags);
+ st->num_returned = NUM_RETURNED(flags);
+ p->clk->rate_discrete = RATE_DISCRETE(flags);
- rates_flag = le32_to_cpu(rlist->num_rates_flags);
- num_remaining = NUM_REMAINING(rates_flag);
- rate_discrete = RATE_DISCRETE(rates_flag);
- num_returned = NUM_RETURNED(rates_flag);
+ return 0;
+}
- if (tot_rate_cnt + num_returned > SCMI_MAX_NUM_RATES) {
- dev_err(ph->dev, "No. of rates > MAX_NUM_RATES");
+static int
+iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
+ const void *response,
+ struct scmi_iterator_state *st, void *priv)
+{
+ int ret = 0;
+ struct scmi_clk_ipriv *p = priv;
+ const struct scmi_msg_resp_clock_describe_rates *r = response;
+
+ if (!p->clk->rate_discrete) {
+ switch (st->desc_index + st->loop_idx) {
+ case 0:
+ p->clk->range.min_rate = RATE_TO_U64(r->rate[0]);
break;
- }
-
- if (!rate_discrete) {
- clk->range.min_rate = RATE_TO_U64(rlist->rate[0]);
- clk->range.max_rate = RATE_TO_U64(rlist->rate[1]);
- clk->range.step_size = RATE_TO_U64(rlist->rate[2]);
- dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
- clk->range.min_rate, clk->range.max_rate,
- clk->range.step_size);
+ case 1:
+ p->clk->range.max_rate = RATE_TO_U64(r->rate[1]);
+ break;
+ case 2:
+ p->clk->range.step_size = RATE_TO_U64(r->rate[2]);
+ break;
+ default:
+ ret = -EINVAL;
break;
}
+ } else {
+ u64 *rate = &p->clk->list.rates[st->desc_index + st->loop_idx];
- rate = &clk->list.rates[tot_rate_cnt];
- for (cnt = 0; cnt < num_returned; cnt++, rate++) {
- *rate = RATE_TO_U64(rlist->rate[cnt]);
- dev_dbg(ph->dev, "Rate %llu Hz\n", *rate);
- }
+ *rate = RATE_TO_U64(r->rate[st->loop_idx]);
+ p->clk->list.num_rates++;
+ //XXX dev_dbg(ph->dev, "Rate %llu Hz\n", *rate);
+ }
+
+ return ret;
+}
- tot_rate_cnt += num_returned;
+static int
+scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
+ struct scmi_clock_info *clk)
+{
+ int ret;
- ph->xops->reset_rx_to_maxsz(ph, t);
- /*
- * check for both returned and remaining to avoid infinite
- * loop due to buggy firmware
- */
- } while (num_returned && num_remaining);
+ void *iter;
+ struct scmi_msg_clock_describe_rates *msg;
+ struct scmi_iterator_ops ops = {
+ .prepare_message = iter_clk_describe_prepare_message,
+ .update_state = iter_clk_describe_update_state,
+ .process_response = iter_clk_describe_process_response,
+ };
+ struct scmi_clk_ipriv cpriv = {
+ .clk_id = clk_id,
+ .clk = clk,
+ };
+
+ iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_RATES,
+ CLOCK_DESCRIBE_RATES,
+ sizeof(*msg), &cpriv);
+ if (IS_ERR(iter))
+ return PTR_ERR(iter);
+
+ ret = ph->hops->iter_response_run(iter);
+ if (ret)
+ return ret;
- if (rate_discrete && rate) {
- clk->list.num_rates = tot_rate_cnt;
- sort(clk->list.rates, tot_rate_cnt, sizeof(*rate),
- rate_cmp_func, NULL);
+ if (!clk->rate_discrete) {
+ dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
+ clk->range.min_rate, clk->range.max_rate,
+ clk->range.step_size);
+ } else if (clk->list.num_rates) {
+ sort(clk->list.rates, clk->list.num_rates,
+ sizeof(clk->list.rates[0]), rate_cmp_func, NULL);
}
- clk->rate_discrete = rate_discrete;
-
-err:
- ph->xops->xfer_put(ph, t);
return ret;
}
--
2.32.0
On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote:
> Do not blindly trust SCMI backend server reply about list of implemented
> protocols, instead validate the reported length of the list of protocols
> against the real payload size of the message reply.
>
> Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> index f279146f8110..c1165d1282ef 100644
> --- a/drivers/firmware/arm_scmi/base.c
> +++ b/drivers/firmware/arm_scmi/base.c
> @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> list = t->rx.buf + sizeof(*num_ret);
>
> do {
> + size_t real_list_sz;
> + u32 calc_list_sz;
> +
> /* Set the number of protocols to be skipped/already read */
> *num_skip = cpu_to_le32(tot_num_ret);
>
> @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> break;
> }
>
> + if (t->rx.len < (sizeof(u32) * 2)) {
> + dev_err(dev, "Truncated reply - rx.len:%zd\n",
> + t->rx.len);
> + ret = -EPROTO;
> + break;
> + }
> +
> + real_list_sz = t->rx.len - sizeof(u32);
> + calc_list_sz = ((loop_num_ret / sizeof(u32)) +
> + !!(loop_num_ret % sizeof(u32))) * sizeof(u32);
Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ?
--
Regards,
Sudeep
On Wed, Mar 30, 2022 at 04:05:29PM +0100, Cristian Marussi wrote:
> Hi all,
>
> this series introduces a bunch of SCMIv3.1 miscellaneous changes to support
> basically all the SCMIv3.1 specification [1] addition with the exclusion of
> the Powercap protocol and driver which will be introduced later on in
> another series.
>
> Most notably the series adds:
>
> - supports across all protocols for long resources naming using *_NAME_GET
> dedicated new commands
> - Clock protocol Rate change pre and post notifications
> - Voltage protocol asynchronous voltage level set command
> (VOLTAGE_LEVEL_SET_COMPLETE delayed response)
> - Perf protocol power-cost in micro-watts (only internal support)
> - Perf protocol PERFORMANCE_LIMITS_SET new checks
>
Apart from minor comments I have, this looks good and I have queued it
provisionally with the changes I have mentioned in the thread.
--
Regards,
Sudeep
On Thu, Apr 28, 2022 at 02:45:07PM +0100, Cristian Marussi wrote:
> On Thu, Apr 28, 2022 at 11:07:29AM +0100, Sudeep Holla wrote:
> > On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote:
> > > Do not blindly trust SCMI backend server reply about list of implemented
> > > protocols, instead validate the reported length of the list of protocols
> > > against the real payload size of the message reply.
> > >
> > > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> > > Signed-off-by: Cristian Marussi <[email protected]>
> > > ---
> > > drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
> > > 1 file changed, 21 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > > index f279146f8110..c1165d1282ef 100644
> > > --- a/drivers/firmware/arm_scmi/base.c
> > > +++ b/drivers/firmware/arm_scmi/base.c
> > > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > > list = t->rx.buf + sizeof(*num_ret);
> > >
> > > do {
> > > + size_t real_list_sz;
> > > + u32 calc_list_sz;
> > > +
> > > /* Set the number of protocols to be skipped/already read */
> > > *num_skip = cpu_to_le32(tot_num_ret);
> > >
> > > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > > break;
> > > }
> > >
> > > + if (t->rx.len < (sizeof(u32) * 2)) {
> > > + dev_err(dev, "Truncated reply - rx.len:%zd\n",
> > > + t->rx.len);
> > > + ret = -EPROTO;
> > > + break;
> > > + }
> > > +
> > > + real_list_sz = t->rx.len - sizeof(u32);
> > > + calc_list_sz = ((loop_num_ret / sizeof(u32)) +
> > > + !!(loop_num_ret % sizeof(u32))) * sizeof(u32);
> >
> > Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ?
> >
>
> At first sight could be fine with your easier version BUT what if loop_num_ret
> is returned as zero ?
>
> real_list_sz should be ZERO length and calc_list_sz
>
> im my version:
>
> calc_list_sz = ((0/4) +!!(0%4)) * 4 ===>> 0
>
> while in the simplified one gets calculated wrong:
>
> calc_list_sz = (0-1)/4 + 1 ====> 1
>
> ...moreover being both loop_num_ret and calc_list_sz unsigned I am even
> not so sure about implicit casting messing things up evenm more :D
>
> So I sticked to the more convoluted approach :D
>
> ....Have I missed something else ?
>
Right, but shouldn't we break if it 0 much earlier. It must not happen with
your new logic and even if it does, wouldn't it be better to break earlier ?
--
Regards,
Sudeep
On Thu, Apr 28, 2022 at 02:55:04PM +0100, Sudeep Holla wrote:
> On Thu, Apr 28, 2022 at 02:45:07PM +0100, Cristian Marussi wrote:
> > On Thu, Apr 28, 2022 at 11:07:29AM +0100, Sudeep Holla wrote:
> > > On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote:
> > > > Do not blindly trust SCMI backend server reply about list of implemented
> > > > protocols, instead validate the reported length of the list of protocols
> > > > against the real payload size of the message reply.
> > > >
> > > > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> > > > Signed-off-by: Cristian Marussi <[email protected]>
> > > > ---
> > > > drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
> > > > 1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > > > index f279146f8110..c1165d1282ef 100644
> > > > --- a/drivers/firmware/arm_scmi/base.c
> > > > +++ b/drivers/firmware/arm_scmi/base.c
> > > > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > > > list = t->rx.buf + sizeof(*num_ret);
> > > >
> > > > do {
> > > > + size_t real_list_sz;
> > > > + u32 calc_list_sz;
> > > > +
> > > > /* Set the number of protocols to be skipped/already read */
> > > > *num_skip = cpu_to_le32(tot_num_ret);
> > > >
> > > > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > > > break;
> > > > }
> > > >
> > > > + if (t->rx.len < (sizeof(u32) * 2)) {
> > > > + dev_err(dev, "Truncated reply - rx.len:%zd\n",
> > > > + t->rx.len);
> > > > + ret = -EPROTO;
> > > > + break;
> > > > + }
> > > > +
> > > > + real_list_sz = t->rx.len - sizeof(u32);
> > > > + calc_list_sz = ((loop_num_ret / sizeof(u32)) +
> > > > + !!(loop_num_ret % sizeof(u32))) * sizeof(u32);
> > >
> > > Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ?
> > >
> >
> > At first sight could be fine with your easier version BUT what if loop_num_ret
> > is returned as zero ?
> >
> > real_list_sz should be ZERO length and calc_list_sz
> >
> > im my version:
> >
> > calc_list_sz = ((0/4) +!!(0%4)) * 4 ===>> 0
> >
> > while in the simplified one gets calculated wrong:
> >
> > calc_list_sz = (0-1)/4 + 1 ====> 1
> >
> > ...moreover being both loop_num_ret and calc_list_sz unsigned I am even
> > not so sure about implicit casting messing things up evenm more :D
> >
> > So I sticked to the more convoluted approach :D
> >
> > ....Have I missed something else ?
> >
>
> Right, but shouldn't we break if it 0 much earlier. It must not happen with
> your new logic and even if it does, wouldn't it be better to break earlier ?
>
Fine for me.
Thanks,
Cristian
On Thu, Apr 28, 2022 at 11:07:29AM +0100, Sudeep Holla wrote:
> On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote:
> > Do not blindly trust SCMI backend server reply about list of implemented
> > protocols, instead validate the reported length of the list of protocols
> > against the real payload size of the message reply.
> >
> > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> > Signed-off-by: Cristian Marussi <[email protected]>
> > ---
> > drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
> > 1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > index f279146f8110..c1165d1282ef 100644
> > --- a/drivers/firmware/arm_scmi/base.c
> > +++ b/drivers/firmware/arm_scmi/base.c
> > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > list = t->rx.buf + sizeof(*num_ret);
> >
> > do {
> > + size_t real_list_sz;
> > + u32 calc_list_sz;
> > +
> > /* Set the number of protocols to be skipped/already read */
> > *num_skip = cpu_to_le32(tot_num_ret);
> >
> > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > break;
> > }
> >
> > + if (t->rx.len < (sizeof(u32) * 2)) {
> > + dev_err(dev, "Truncated reply - rx.len:%zd\n",
> > + t->rx.len);
> > + ret = -EPROTO;
> > + break;
> > + }
> > +
> > + real_list_sz = t->rx.len - sizeof(u32);
> > + calc_list_sz = ((loop_num_ret / sizeof(u32)) +
> > + !!(loop_num_ret % sizeof(u32))) * sizeof(u32);
>
> Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ?
>
At first sight could be fine with your easier version BUT what if loop_num_ret
is returned as zero ?
real_list_sz should be ZERO length and calc_list_sz
im my version:
calc_list_sz = ((0/4) +!!(0%4)) * 4 ===>> 0
while in the simplified one gets calculated wrong:
calc_list_sz = (0-1)/4 + 1 ====> 1
...moreover being both loop_num_ret and calc_list_sz unsigned I am even
not so sure about implicit casting messing things up evenm more :D
So I sticked to the more convoluted approach :D
....Have I missed something else ?
Thanks,
Cristian
On Wed, 30 Mar 2022 16:05:29 +0100, Cristian Marussi wrote:
> this series introduces a bunch of SCMIv3.1 miscellaneous changes to support
> basically all the SCMIv3.1 specification [1] addition with the exclusion of
> the Powercap protocol and driver which will be introduced later on in
> another series.
>
> Most notably the series adds:
>
> [...]
Applied to sudeep.holla/linux (for-next/scmi), thanks!
[02/22] firmware: arm_scmi: Make protocols init fail on basic errors
https://git.kernel.org/sudeep.holla/c/4de1b36fae
[03/22] firmware: arm_scmi: Fix Base list protocols enumeration
https://git.kernel.org/sudeep.holla/c/8009120e03
[04/22] firmware: arm_scmi: Validate BASE_DISCOVER_LIST_PROTOCOLS reply
https://git.kernel.org/sudeep.holla/c/3b0041f6e1
[05/22] firmware: arm_scmi: Dynamically allocate protocols array
https://git.kernel.org/sudeep.holla/c/776b6c8a25
[06/22] firmware: arm_scmi: Make name_get operations return a const
https://git.kernel.org/sudeep.holla/c/992be5d3c8
[07/22] firmware: arm_scmi: Check CLOCK_RATE_SET_COMPLETE async reply
https://git.kernel.org/sudeep.holla/c/c7e223f5c7
[08/22] firmware: arm_scmi: Remove unneeded NULL termination of clk name
https://git.kernel.org/sudeep.holla/c/91ebc56cbc
[09/22] firmware: arm_scmi: Split protocol specific definitions in a dedicated header
https://git.kernel.org/sudeep.holla/c/23136bff80
[10/22] firmware: arm_scmi: Introduce a common SCMIv3.1 .extended_name_get helper
https://git.kernel.org/sudeep.holla/c/5c873d120d
[11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support
https://git.kernel.org/sudeep.holla/c/b260fccaeb
[12/22] firmware: arm_scmi: Parse clock_enable_latency conditionally
https://git.kernel.org/sudeep.holla/c/df3576d14a
[13/22] firmware: arm_scmi: Add iterators for multi-part commands
https://git.kernel.org/sudeep.holla/c/36b6ea0fc6
[14/22] firmware: arm_scmi: Use common iterators in Sensor protocol
https://git.kernel.org/sudeep.holla/c/7cab537704
[15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support
https://git.kernel.org/sudeep.holla/c/802b0bed01
[16/22] firmware: arm_scmi: Use common iterators in Clock protocol
https://git.kernel.org/sudeep.holla/c/7bc7caafe6
[17/22] firmware: arm_scmi: Use common iterators in Voltage protocol
https://git.kernel.org/sudeep.holla/c/d8d7e91316
[18/22] firmware: arm_scmi: Use common iterators in Perf protocol
https://git.kernel.org/sudeep.holla/c/79d2ea9244
[19/22] firmware: arm_scmi: Add SCMIv3.1 Clock notifications
https://git.kernel.org/sudeep.holla/c/7aa75496ea
[20/22] firmware: arm_scmi: Add SCMIv3.1 VOLTAGE_LEVEL_SET_COMPLETE
https://git.kernel.org/sudeep.holla/c/4c74701b1e
[21/22] firmware: arm_scmi: Add SCMI v3.1 Perf power-cost in microwatts
https://git.kernel.org/sudeep.holla/c/3630cd8130
[22/22] firmware: arm_scmi: Add SCMIv3.1 PERFORMANCE_LIMITS_SET checks
https://git.kernel.org/sudeep.holla/c/71bea05797
--
Regards,
Sudeep