Re: [PATCH] firmware: arm_scmi: Add protocol versioning checks

[Sudeep Holla]

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@xxxxxxx>
> ---
> 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

-- 
Regards,
Sudeep