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

[Cristian Marussi]

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

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