Hi
just a bunch of small unrelated fixes for v5.20.
Last one is marked as RFC since as explained it could be controversial.
Thanks,
Cristian
Cristian Marussi (6):
firmware: arm_scmi: Fix missing kernel-doc in optee
firmware: arm_scmi: Improve checks on .info_get operations
firmware: arm_scmi: Harden accesses to Sensor domains
firmware: arm_scmi: Harden accesses to Reset domains
firmware: arm_scmi: Fix asynchronous reset requests
[RFC] firmware: arm_scmi: Add SCMI PM driver remove routine
drivers/firmware/arm_scmi/clock.c | 6 +++++-
drivers/firmware/arm_scmi/optee.c | 1 +
drivers/firmware/arm_scmi/reset.c | 10 ++++++---
drivers/firmware/arm_scmi/scmi_pm_domain.c | 20 +++++++++++++++++
drivers/firmware/arm_scmi/sensors.c | 25 ++++++++++++++++++----
include/linux/scmi_protocol.h | 4 ++--
6 files changed, 56 insertions(+), 10 deletions(-)
--
2.32.0
Add a missing structure field description.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/optee.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
index 8abace56b958..f42dad997ac9 100644
--- a/drivers/firmware/arm_scmi/optee.c
+++ b/drivers/firmware/arm_scmi/optee.c
@@ -106,6 +106,7 @@ enum scmi_optee_pta_cmd {
* @channel_id: OP-TEE channel ID used for this transport
* @tee_session: TEE session identifier
* @caps: OP-TEE SCMI channel capabilities
+ * @rx_len: Response size
* @mu: Mutex protection on channel access
* @cinfo: SCMI channel information
* @shmem: Virtual base address of the shared memory
--
2.32.0
SCMI Reset protocol specification allows for asynchronous reset request
only when an Autonomous Reset action is specified: reset requests based on
explicit assert/deassert of signals should not be served asynchronously.
Current implementation will instead issue an asynchronous request in any
case, as long as the reset domain had advertised to support asynchronous
resets.
Avoid requesting asynchronous resets when the reset action is not of the
Autonomous type, even if the target reset-domain does, in general, support
asynchronous requests.
Fixes: 95a15d80aa0d ("firmware: arm_scmi: Add RESET protocol in SCMI v2.0")
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/reset.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index b0494165b1cb..e9afa8cab730 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -172,7 +172,7 @@ static int scmi_domain_reset(const struct scmi_protocol_handle *ph, u32 domain,
return -EINVAL;
rdom = pi->dom_info + domain;
- if (rdom->async_reset)
+ if (rdom->async_reset && flags & AUTONOMOUS_RESET)
flags |= ASYNCHRONOUS_RESET;
ret = ph->xops->xfer_get_init(ph, RESET, sizeof(*dom), 0, &t);
@@ -184,7 +184,7 @@ static int scmi_domain_reset(const struct scmi_protocol_handle *ph, u32 domain,
dom->flags = cpu_to_le32(flags);
dom->reset_state = cpu_to_le32(state);
- if (rdom->async_reset)
+ if (flags & ASYNCHRONOUS_RESET)
ret = ph->xops->do_xfer_with_response(ph, t);
else
ret = ph->xops->do_xfer(ph, t);
--
2.32.0
SCMI protocols abstract and expose a number of protocol specific resources
like clocks, sensors and so on: information about such specific domain
resources are generally exposed via an .info_get protocol operation.
Improve the sanity check on these .info_get operations where needed.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/clock.c | 6 +++++-
drivers/firmware/arm_scmi/sensors.c | 3 +++
include/linux/scmi_protocol.h | 4 ++--
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 3ed7ae0d6781..96060bf90a24 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -450,9 +450,13 @@ static int scmi_clock_count_get(const struct scmi_protocol_handle *ph)
static const struct scmi_clock_info *
scmi_clock_info_get(const struct scmi_protocol_handle *ph, u32 clk_id)
{
+ struct scmi_clock_info *clk;
struct clock_info *ci = ph->get_priv(ph);
- struct scmi_clock_info *clk = ci->clk + clk_id;
+ if (clk_id >= ci->num_clocks)
+ return NULL;
+
+ clk = ci->clk + clk_id;
if (!clk->name[0])
return NULL;
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 7288c6117838..7d0c7476d206 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -948,6 +948,9 @@ scmi_sensor_info_get(const struct scmi_protocol_handle *ph, u32 sensor_id)
{
struct sensors_info *si = ph->get_priv(ph);
+ if (sensor_id >= si->num_sensors)
+ return NULL;
+
return si->sensors + sensor_id;
}
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index a193884ecf2b..4f765bc788ff 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -84,7 +84,7 @@ struct scmi_protocol_handle;
struct scmi_clk_proto_ops {
int (*count_get)(const struct scmi_protocol_handle *ph);
- const struct scmi_clock_info *(*info_get)
+ const struct scmi_clock_info __must_check *(*info_get)
(const struct scmi_protocol_handle *ph, u32 clk_id);
int (*rate_get)(const struct scmi_protocol_handle *ph, u32 clk_id,
u64 *rate);
@@ -466,7 +466,7 @@ enum scmi_sensor_class {
*/
struct scmi_sensor_proto_ops {
int (*count_get)(const struct scmi_protocol_handle *ph);
- const struct scmi_sensor_info *(*info_get)
+ const struct scmi_sensor_info __must_check *(*info_get)
(const struct scmi_protocol_handle *ph, u32 sensor_id);
int (*trip_point_config)(const struct scmi_protocol_handle *ph,
u32 sensor_id, u8 trip_id, u64 trip_value);
--
2.32.0
Accessing Reset domains descriptors by index upon SCMI drivers requests
through the SCMI reset operations interface can potentially lead to
out-of-bound violations if the SCMI driver misbehave.
Add an internal consistency check in front of such domains descriptors
accesses.
Fixes: 95a15d80aa0de ("firmware: arm_scmi: Add RESET protocol in SCMI v2.0")
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/reset.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 673f3eb498f4..b0494165b1cb 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -166,8 +166,12 @@ static int scmi_domain_reset(const struct scmi_protocol_handle *ph, u32 domain,
struct scmi_xfer *t;
struct scmi_msg_reset_domain_reset *dom;
struct scmi_reset_info *pi = ph->get_priv(ph);
- struct reset_dom_info *rdom = pi->dom_info + domain;
+ struct reset_dom_info *rdom;
+ if (domain >= pi->num_domains)
+ return -EINVAL;
+
+ rdom = pi->dom_info + domain;
if (rdom->async_reset)
flags |= ASYNCHRONOUS_RESET;
--
2.32.0
Currently, when removing the SCMI PM driver not all the resources
registered with GenPD subsystem are currently properly de-registered.
As a side effect of this after a driver unload/load cycle you get a splat
with a few warnings like this:
debugfs: Directory 'BIG_CPU0' with parent 'pm_genpd' already present!
debugfs: Directory 'BIG_CPU1' with parent 'pm_genpd' already present!
debugfs: Directory 'LITTLE_CPU0' with parent 'pm_genpd' already present!
debugfs: Directory 'LITTLE_CPU1' with parent 'pm_genpd' already present!
debugfs: Directory 'LITTLE_CPU2' with parent 'pm_genpd' already present!
debugfs: Directory 'LITTLE_CPU3' with parent 'pm_genpd' already present!
debugfs: Directory 'BIG_SSTOP' with parent 'pm_genpd' already present!
debugfs: Directory 'LITTLE_SSTOP' with parent 'pm_genpd' already present!
debugfs: Directory 'DBGSYS' with parent 'pm_genpd' already present!
debugfs: Directory 'GPUTOP' with parent 'pm_genpd' already present!
Add a proper scmi_pm_domain_remove callback to the driver in order to
take care of all the needed cleanups not handled already by devres.
Signed-off-by: Cristian Marussi <[email protected]>
---
The patch is marked as RFC since this solution can be controversial: what
about any 3rd party consumer of the above unregistered genpd domain ?
Should we even ever allow a PM driver like to be unloaded ?
---
drivers/firmware/arm_scmi/scmi_pm_domain.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/firmware/arm_scmi/scmi_pm_domain.c b/drivers/firmware/arm_scmi/scmi_pm_domain.c
index 581d34c95769..4e27c3d66a83 100644
--- a/drivers/firmware/arm_scmi/scmi_pm_domain.c
+++ b/drivers/firmware/arm_scmi/scmi_pm_domain.c
@@ -138,9 +138,28 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
scmi_pd_data->domains = domains;
scmi_pd_data->num_domains = num_domains;
+ dev_set_drvdata(dev, scmi_pd_data);
+
return of_genpd_add_provider_onecell(np, scmi_pd_data);
}
+static void scmi_pm_domain_remove(struct scmi_device *sdev)
+{
+ int i;
+ struct genpd_onecell_data *scmi_pd_data;
+ struct device *dev = &sdev->dev;
+ struct device_node *np = dev->of_node;
+
+ of_genpd_del_provider(np);
+
+ scmi_pd_data = dev_get_drvdata(dev);
+ for (i = 0; i < scmi_pd_data->num_domains; i++) {
+ if (!scmi_pd_data->domains[i])
+ continue;
+ pm_genpd_remove(scmi_pd_data->domains[i]);
+ }
+}
+
static const struct scmi_device_id scmi_id_table[] = {
{ SCMI_PROTOCOL_POWER, "genpd" },
{ },
@@ -150,6 +169,7 @@ MODULE_DEVICE_TABLE(scmi, scmi_id_table);
static struct scmi_driver scmi_power_domain_driver = {
.name = "scmi-power-domain",
.probe = scmi_pm_domain_probe,
+ .remove = scmi_pm_domain_remove,
.id_table = scmi_id_table,
};
module_scmi_driver(scmi_power_domain_driver);
--
2.32.0
Accessing Sensor domains descriptors by index upon SCMI drivers requests
through the SCMI sensor operations interface can potentially lead to
out-of-bound violations if the SCMI driver misbehave.
Add an internal consistency check in front of such domains descriptors
accesses.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/sensors.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 7d0c7476d206..0b5853fa9d87 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -762,6 +762,10 @@ static int scmi_sensor_config_get(const struct scmi_protocol_handle *ph,
{
int ret;
struct scmi_xfer *t;
+ struct sensors_info *si = ph->get_priv(ph);
+
+ if (sensor_id >= si->num_sensors)
+ return -EINVAL;
ret = ph->xops->xfer_get_init(ph, SENSOR_CONFIG_GET,
sizeof(__le32), sizeof(__le32), &t);
@@ -771,7 +775,6 @@ static int scmi_sensor_config_get(const struct scmi_protocol_handle *ph,
put_unaligned_le32(sensor_id, t->tx.buf);
ret = ph->xops->do_xfer(ph, t);
if (!ret) {
- struct sensors_info *si = ph->get_priv(ph);
struct scmi_sensor_info *s = si->sensors + sensor_id;
*sensor_config = get_unaligned_le64(t->rx.buf);
@@ -788,6 +791,10 @@ static int scmi_sensor_config_set(const struct scmi_protocol_handle *ph,
int ret;
struct scmi_xfer *t;
struct scmi_msg_sensor_config_set *msg;
+ struct sensors_info *si = ph->get_priv(ph);
+
+ if (sensor_id >= si->num_sensors)
+ return -EINVAL;
ret = ph->xops->xfer_get_init(ph, SENSOR_CONFIG_SET,
sizeof(*msg), 0, &t);
@@ -800,7 +807,6 @@ static int scmi_sensor_config_set(const struct scmi_protocol_handle *ph,
ret = ph->xops->do_xfer(ph, t);
if (!ret) {
- struct sensors_info *si = ph->get_priv(ph);
struct scmi_sensor_info *s = si->sensors + sensor_id;
s->sensor_config = sensor_config;
@@ -831,8 +837,11 @@ static int scmi_sensor_reading_get(const struct scmi_protocol_handle *ph,
int ret;
struct scmi_xfer *t;
struct scmi_msg_sensor_reading_get *sensor;
+ struct scmi_sensor_info *s;
struct sensors_info *si = ph->get_priv(ph);
- struct scmi_sensor_info *s = si->sensors + sensor_id;
+
+ if (sensor_id >= si->num_sensors)
+ return -EINVAL;
ret = ph->xops->xfer_get_init(ph, SENSOR_READING_GET,
sizeof(*sensor), 0, &t);
@@ -841,6 +850,7 @@ static int scmi_sensor_reading_get(const struct scmi_protocol_handle *ph,
sensor = t->tx.buf;
sensor->id = cpu_to_le32(sensor_id);
+ s = si->sensors + sensor_id;
if (s->async) {
sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
ret = ph->xops->do_xfer_with_response(ph, t);
@@ -895,9 +905,13 @@ scmi_sensor_reading_get_timestamped(const struct scmi_protocol_handle *ph,
int ret;
struct scmi_xfer *t;
struct scmi_msg_sensor_reading_get *sensor;
+ struct scmi_sensor_info *s;
struct sensors_info *si = ph->get_priv(ph);
- struct scmi_sensor_info *s = si->sensors + sensor_id;
+ if (sensor_id >= si->num_sensors)
+ return -EINVAL;
+
+ s = si->sensors + sensor_id;
if (!count || !readings ||
(!s->num_axis && count > 1) || (s->num_axis && count > s->num_axis))
return -EINVAL;
--
2.32.0
On Wed, 17 Aug 2022 18:27:25 +0100, Cristian Marussi wrote:
> just a bunch of small unrelated fixes for v5.20.
> Last one is marked as RFC since as explained it could be controversial.
>
> Thanks,
> Cristian
>
> Cristian Marussi (6):
> firmware: arm_scmi: Fix missing kernel-doc in optee
> firmware: arm_scmi: Improve checks on .info_get operations
> firmware: arm_scmi: Harden accesses to Sensor domains
> firmware: arm_scmi: Harden accesses to Reset domains
> firmware: arm_scmi: Fix asynchronous reset requests
> [RFC] firmware: arm_scmi: Add SCMI PM driver remove routine
>
> [...]
Applied to sudeep.holla/linux (for-next/scmi), thanks!
[1/6] firmware: arm_scmi: Fix missing kernel-doc in optee
https://git.kernel.org/sudeep.holla/c/2e42b1652df0
[2/6] firmware: arm_scmi: Improve checks on .info_get operations
https://git.kernel.org/sudeep.holla/c/1ecb7d27b1af
[3/6] firmware: arm_scmi: Harden accesses to Sensor domains
https://git.kernel.org/sudeep.holla/c/76f89c954788
[4/6] firmware: arm_scmi: Harden accesses to Reset domains
https://git.kernel.org/sudeep.holla/c/e9076ffbcaed
[5/6] firmware: arm_scmi: Fix asynchronous reset requests
https://git.kernel.org/sudeep.holla/c/b75c83d9b961
[6/6] firmware: arm_scmi: Add SCMI PM driver remove routine
https://git.kernel.org/sudeep.holla/c/dea796fcab0a
--
Regards,
Sudeep