On Fri, Dec 01, 2023 at 01:58:58PM +0000, Cristian Marussi wrote:
> Platform and agent supported protocols versions do not necessarily match.
>
> When talking to an older platform SCMI server, supporting only older
> protocol versions, the kernel SCMI agent will downgrade the version of
> the used protocol to match the platform one and avoid compatibility issues.
>
> In the case, instead, in which the agent happens to communicate with a
> newer platform server which can support newer protocol versions unknown to
> the agent, and potentially backward incompatible, the agent currently
> carries on, silently, in a best-effort approach.
>
> Note that the SCMI server, by the specification, has no means to explicitly
> detect the protocol versions used by the agents, neither it is required to
> support multiple, older, protocol versions.
>
> Add an explicit protocol version check to let the agent detect when this
> version mismatch happens and warn the user about this condition.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> Any suggestion for a more meaningful warn message is very much welcome.
Looks good to me. I will apply soon with couple of changes as mentioned
below. Let me know if you agree/disagree.
> Based on sudeep/for-next/scmi/updates
> ---
> drivers/firmware/arm_scmi/base.c | 6 +++++-
> drivers/firmware/arm_scmi/clock.c | 6 +++++-
> drivers/firmware/arm_scmi/driver.c | 11 ++++++++++-
> drivers/firmware/arm_scmi/perf.c | 6 +++++-
> drivers/firmware/arm_scmi/power.c | 6 +++++-
> drivers/firmware/arm_scmi/powercap.c | 6 +++++-
> drivers/firmware/arm_scmi/protocols.h | 8 +++++++-
> drivers/firmware/arm_scmi/reset.c | 6 +++++-
> drivers/firmware/arm_scmi/sensors.c | 6 +++++-
> drivers/firmware/arm_scmi/system.c | 6 +++++-
> drivers/firmware/arm_scmi/voltage.c | 6 +++++-
> 11 files changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> index a52f084a6a87..3f5c89ae5af2 100644
> --- a/drivers/firmware/arm_scmi/base.c
> +++ b/drivers/firmware/arm_scmi/base.c
> @@ -13,6 +13,9 @@
> #include "common.h"
> #include "notify.h"
>
> +/* Must be updated only after ALL new features for that version are merged */
s/new/mandatory/
> +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x20000
> +
> #define SCMI_BASE_NUM_SOURCES 1
> #define SCMI_BASE_MAX_CMD_ERR_COUNT 1024
>
> index 3eb19ed6f148..46320f627066 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -1849,6 +1853,11 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
> devres_close_group(handle->dev, pi->gid);
> dev_dbg(handle->dev, "Initialized protocol: 0x%X\n", pi->proto->id);
>
> + if (pi->version > proto->supported_version)
> + dev_warn(handle->dev,
> + "Detected UNSUPPORTED version 0x%X for protocol 0x%X. Backward compatibility is NOT assured.\n",
s/UNSUPPORTED version/UNSUPPORTED newer version/ or higher instead of newer
On Fri, Dec 01, 2023 at 03:31:45PM +0000, Sudeep Holla wrote:
> On Fri, Dec 01, 2023 at 01:58:58PM +0000, Cristian Marussi wrote:
> > Platform and agent supported protocols versions do not necessarily match.
> >
> > When talking to an older platform SCMI server, supporting only older
> > protocol versions, the kernel SCMI agent will downgrade the version of
> > the used protocol to match the platform one and avoid compatibility issues.
> >
> > In the case, instead, in which the agent happens to communicate with a
> > newer platform server which can support newer protocol versions unknown to
> > the agent, and potentially backward incompatible, the agent currently
> > carries on, silently, in a best-effort approach.
> >
> > Note that the SCMI server, by the specification, has no means to explicitly
> > detect the protocol versions used by the agents, neither it is required to
> > support multiple, older, protocol versions.
> >
> > Add an explicit protocol version check to let the agent detect when this
> > version mismatch happens and warn the user about this condition.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > Any suggestion for a more meaningful warn message is very much welcome.
>
> Looks good to me. I will apply soon with couple of changes as mentioned
> below. Let me know if you agree/disagree.
>
> > Based on sudeep/for-next/scmi/updates
> > ---
> > drivers/firmware/arm_scmi/base.c | 6 +++++-
> > drivers/firmware/arm_scmi/clock.c | 6 +++++-
> > drivers/firmware/arm_scmi/driver.c | 11 ++++++++++-
> > drivers/firmware/arm_scmi/perf.c | 6 +++++-
> > drivers/firmware/arm_scmi/power.c | 6 +++++-
> > drivers/firmware/arm_scmi/powercap.c | 6 +++++-
> > drivers/firmware/arm_scmi/protocols.h | 8 +++++++-
> > drivers/firmware/arm_scmi/reset.c | 6 +++++-
> > drivers/firmware/arm_scmi/sensors.c | 6 +++++-
> > drivers/firmware/arm_scmi/system.c | 6 +++++-
> > drivers/firmware/arm_scmi/voltage.c | 6 +++++-
> > 11 files changed, 62 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > index a52f084a6a87..3f5c89ae5af2 100644
> > --- a/drivers/firmware/arm_scmi/base.c
> > +++ b/drivers/firmware/arm_scmi/base.c
> > @@ -13,6 +13,9 @@
> > #include "common.h"
> > #include "notify.h"
> >
> > +/* Must be updated only after ALL new features for that version are merged */
>
> s/new/mandatory/
>
> > +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x20000
> > +
> > #define SCMI_BASE_NUM_SOURCES 1
> > #define SCMI_BASE_MAX_CMD_ERR_COUNT 1024
> >
> > index 3eb19ed6f148..46320f627066 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -1849,6 +1853,11 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
> > devres_close_group(handle->dev, pi->gid);
> > dev_dbg(handle->dev, "Initialized protocol: 0x%X\n", pi->proto->id);
> >
> > + if (pi->version > proto->supported_version)
> > + dev_warn(handle->dev,
> > + "Detected UNSUPPORTED version 0x%X for protocol 0x%X. Backward compatibility is NOT assured.\n",
>
> s/UNSUPPORTED version/UNSUPPORTED newer version/ or higher instead of newer
>
Much better ... I'm fine with these changes ... I was tempted to use
'higher' too, but not sure what was more effective at communicating
the issue...a panic usually does it better :P (joking ah,...)
Thanks,
Cristian
@@ -13,6 +13,9 @@
#include "common.h"
#include "notify.h"
+/* Must be updated only after ALL new features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x20000
+
#define SCMI_BASE_NUM_SOURCES 1
#define SCMI_BASE_MAX_CMD_ERR_COUNT 1024
@@ -385,7 +388,7 @@ static int scmi_base_protocol_init(const struct scmi_protocol_handle *ph)
rev->major_ver = PROTOCOL_REV_MAJOR(version),
rev->minor_ver = PROTOCOL_REV_MINOR(version);
- ph->set_priv(ph, rev);
+ ph->set_priv(ph, rev, version);
ret = scmi_base_attributes_get(ph);
if (ret)
@@ -423,6 +426,7 @@ static const struct scmi_protocol scmi_base = {
.instance_init = &scmi_base_protocol_init,
.ops = NULL,
.events = &base_protocol_events,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
};
DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(base, scmi_base)
@@ -12,6 +12,9 @@
#include "protocols.h"
#include "notify.h"
+/* Must be updated only after ALL new features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x20001
+
enum scmi_clock_protocol_cmd {
CLOCK_ATTRIBUTES = 0x3,
CLOCK_DESCRIBE_RATES = 0x4,
@@ -961,7 +964,7 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
}
cinfo->version = version;
- return ph->set_priv(ph, cinfo);
+ return ph->set_priv(ph, cinfo, version);
}
static const struct scmi_protocol scmi_clock = {
@@ -970,6 +973,7 @@ static const struct scmi_protocol scmi_clock = {
.instance_init = &scmi_clock_protocol_init,
.ops = &clk_proto_ops,
.events = &clk_protocol_events,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
};
DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(clock, scmi_clock)
@@ -85,6 +85,7 @@ struct scmi_xfers_info {
* @gid: A reference for per-protocol devres management.
* @users: A refcount to track effective users of this protocol.
* @priv: Reference for optional protocol private data.
+ * @version: Protocol version supported by the platform as detected at runtime.
* @ph: An embedded protocol handle that will be passed down to protocol
* initialization code to identify this instance.
*
@@ -97,6 +98,7 @@ struct scmi_protocol_instance {
void *gid;
refcount_t users;
void *priv;
+ unsigned int version;
struct scmi_protocol_handle ph;
};
@@ -1392,15 +1394,17 @@ static int version_get(const struct scmi_protocol_handle *ph, u32 *version)
*
* @ph: A reference to the protocol handle.
* @priv: The private data to set.
+ * @version: The detected protocol version for the core to register.
*
* Return: 0 on Success
*/
static int scmi_set_protocol_priv(const struct scmi_protocol_handle *ph,
- void *priv)
+ void *priv, u32 version)
{
struct scmi_protocol_instance *pi = ph_to_pi(ph);
pi->priv = priv;
+ pi->version = version;
return 0;
}
@@ -1849,6 +1853,11 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
devres_close_group(handle->dev, pi->gid);
dev_dbg(handle->dev, "Initialized protocol: 0x%X\n", pi->proto->id);
+ if (pi->version > proto->supported_version)
+ dev_warn(handle->dev,
+ "Detected UNSUPPORTED version 0x%X for protocol 0x%X. Backward compatibility is NOT assured.\n",
+ pi->version, pi->proto->id);
+
return pi;
clean:
@@ -24,6 +24,9 @@
#include "protocols.h"
#include "notify.h"
+/* Must be updated only after ALL new features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x40000
+
#define MAX_OPPS 32
enum scmi_performance_protocol_cmd {
@@ -1104,7 +1107,7 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
if (ret)
return ret;
- return ph->set_priv(ph, pinfo);
+ return ph->set_priv(ph, pinfo, version);
}
static const struct scmi_protocol scmi_perf = {
@@ -1113,6 +1116,7 @@ static const struct scmi_protocol scmi_perf = {
.instance_init = &scmi_perf_protocol_init,
.ops = &perf_proto_ops,
.events = &perf_protocol_events,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
};
DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(perf, scmi_perf)
@@ -13,6 +13,9 @@
#include "protocols.h"
#include "notify.h"
+/* Must be updated only after ALL new features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x30000
+
enum scmi_power_protocol_cmd {
POWER_DOMAIN_ATTRIBUTES = 0x3,
POWER_STATE_SET = 0x4,
@@ -328,7 +331,7 @@ static int scmi_power_protocol_init(const struct scmi_protocol_handle *ph)
pinfo->version = version;
- return ph->set_priv(ph, pinfo);
+ return ph->set_priv(ph, pinfo, version);
}
static const struct scmi_protocol scmi_power = {
@@ -337,6 +340,7 @@ static const struct scmi_protocol scmi_power = {
.instance_init = &scmi_power_protocol_init,
.ops = &power_proto_ops,
.events = &power_protocol_events,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
};
DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(power, scmi_power)
@@ -17,6 +17,9 @@
#include "protocols.h"
#include "notify.h"
+/* Must be updated only after ALL new features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x20000
+
enum scmi_powercap_protocol_cmd {
POWERCAP_DOMAIN_ATTRIBUTES = 0x3,
POWERCAP_CAP_GET = 0x4,
@@ -975,7 +978,7 @@ scmi_powercap_protocol_init(const struct scmi_protocol_handle *ph)
}
pinfo->version = version;
- return ph->set_priv(ph, pinfo);
+ return ph->set_priv(ph, pinfo, version);
}
static const struct scmi_protocol scmi_powercap = {
@@ -984,6 +987,7 @@ static const struct scmi_protocol scmi_powercap = {
.instance_init = &scmi_powercap_protocol_init,
.ops = &powercap_proto_ops,
.events = &powercap_protocol_events,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
};
DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(powercap, scmi_powercap)
@@ -174,7 +174,8 @@ struct scmi_protocol_handle {
struct device *dev;
const struct scmi_xfer_ops *xops;
const struct scmi_proto_helpers_ops *hops;
- int (*set_priv)(const struct scmi_protocol_handle *ph, void *priv);
+ int (*set_priv)(const struct scmi_protocol_handle *ph, void *priv,
+ u32 version);
void *(*get_priv)(const struct scmi_protocol_handle *ph);
};
@@ -311,6 +312,10 @@ typedef int (*scmi_prot_init_ph_fn_t)(const struct scmi_protocol_handle *);
* @ops: Optional reference to the operations provided by the protocol and
* exposed in scmi_protocol.h.
* @events: An optional reference to the events supported by this protocol.
+ * @supported_version: The highest version currently supported for this
+ * protocol by the agent. Each protocol implementation
+ * in the agent is supposed to downgrade to match the
+ * protocol version supported by the platform.
*/
struct scmi_protocol {
const u8 id;
@@ -319,6 +324,7 @@ struct scmi_protocol {
const scmi_prot_init_ph_fn_t instance_deinit;
const void *ops;
const struct scmi_protocol_events *events;
+ unsigned int supported_version;
};
#define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(name, proto) \
@@ -13,6 +13,9 @@
#include "protocols.h"
#include "notify.h"
+/* Must be updated only after ALL new features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x30000
+
enum scmi_reset_protocol_cmd {
RESET_DOMAIN_ATTRIBUTES = 0x3,
RESET = 0x4,
@@ -343,7 +346,7 @@ static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph)
}
pinfo->version = version;
- return ph->set_priv(ph, pinfo);
+ return ph->set_priv(ph, pinfo, version);
}
static const struct scmi_protocol scmi_reset = {
@@ -352,6 +355,7 @@ static const struct scmi_protocol scmi_reset = {
.instance_init = &scmi_reset_protocol_init,
.ops = &reset_proto_ops,
.events = &reset_protocol_events,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
};
DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(reset, scmi_reset)
@@ -14,6 +14,9 @@
#include "protocols.h"
#include "notify.h"
+/* Must be updated only after ALL new features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x30000
+
#define SCMI_MAX_NUM_SENSOR_AXIS 63
#define SCMIv2_SENSOR_PROTOCOL 0x10000
@@ -1138,7 +1141,7 @@ static int scmi_sensors_protocol_init(const struct scmi_protocol_handle *ph)
if (ret)
return ret;
- return ph->set_priv(ph, sinfo);
+ return ph->set_priv(ph, sinfo, version);
}
static const struct scmi_protocol scmi_sensors = {
@@ -1147,6 +1150,7 @@ static const struct scmi_protocol scmi_sensors = {
.instance_init = &scmi_sensors_protocol_init,
.ops = &sensor_proto_ops,
.events = &sensor_protocol_events,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
};
DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(sensors, scmi_sensors)
@@ -13,6 +13,9 @@
#include "protocols.h"
#include "notify.h"
+/* Must be updated only after ALL new features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x20000
+
#define SCMI_SYSTEM_NUM_SOURCES 1
enum scmi_system_protocol_cmd {
@@ -144,7 +147,7 @@ static int scmi_system_protocol_init(const struct scmi_protocol_handle *ph)
if (PROTOCOL_REV_MAJOR(pinfo->version) >= 0x2)
pinfo->graceful_timeout_supported = true;
- return ph->set_priv(ph, pinfo);
+ return ph->set_priv(ph, pinfo, version);
}
static const struct scmi_protocol scmi_system = {
@@ -153,6 +156,7 @@ static const struct scmi_protocol scmi_system = {
.instance_init = &scmi_system_protocol_init,
.ops = NULL,
.events = &system_protocol_events,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
};
DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(system, scmi_system)
@@ -10,6 +10,9 @@
#include "protocols.h"
+/* Must be updated only after ALL new features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x20000
+
#define VOLTAGE_DOMS_NUM_MASK GENMASK(15, 0)
#define REMAINING_LEVELS_MASK GENMASK(31, 16)
#define RETURNED_LEVELS_MASK GENMASK(11, 0)
@@ -432,7 +435,7 @@ static int scmi_voltage_protocol_init(const struct scmi_protocol_handle *ph)
dev_warn(ph->dev, "No Voltage domains found.\n");
}
- return ph->set_priv(ph, vinfo);
+ return ph->set_priv(ph, vinfo, version);
}
static const struct scmi_protocol scmi_voltage = {
@@ -440,6 +443,7 @@ static const struct scmi_protocol scmi_voltage = {
.owner = THIS_MODULE,
.instance_init = &scmi_voltage_protocol_init,
.ops = &voltage_proto_ops,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
};
DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(voltage, scmi_voltage)