Re: [PATCH] firmware: arm_scmi:: Add opinion for init fastchannel

[Cristian Marussi <cristian.marussi@arm.com>]

On Tue, Oct 31, 2023 at 09:36:01AM +0800, xinglong.yang wrote:
> Fastchannel may not be supported by the platform. It is not need to
> init the fastchannel if the fastchannel is not supported.
> 

Hi,

the commit message is misleading because the patch you provided does not
really does that :D

FCs are indeed not necessarily supported by the platform, and this is
reported for each single domain at discovery time AND we indeed check
already that (dom->perf_fastchannels) BEFORE initiliazing the fastchannel
at all for a specific domain.

What you are doing here, instead, is skipping FC initialization on a
fastchannel by checking the dom->set_perf / dom->set_limit, BUT such flags
are really meant to describe if any set_perf/limit ops can be issued on a
domain at all.

Given that each FC is associated to a specific domain_id AND msg_id it
is indeed plausible that a domain supports FastChannels BUT only for
get_* operations.

In such scenario I would expect, though, the platform to return
NOT_SUPPORTED when required to describe the SET_* FCs.

Having said that, it could be anyway useful to refrain from trying to
describe/init the SET operations on a FC for a domain where the set_*
operations are not supported, because: (1.) avoids unneeded SCMI
exchanges, (2.) avoids to trust the FW reply blindly...

...BUT your patch does NOT really do this, because you are skipping the
fastchannel_init by looking at dom->set_perf and dom->set_limit for BOTH
the set and get commands.

You should just skip the FC init on the SET_ when dom->set_ is set if we
want to do this optimization/hardening.

Am I missing something ?

Thanks,
Cristian


> Signed-off-by: xinglong.yang <xinglong.yang@cixtech.com>
> Change-Id: Id73ab1f37d5a3726243f97beb40c5b2239d65727
> ---
>  drivers/firmware/arm_scmi/perf.c | 38 ++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index ecf5c4de851b..26fa71e2aff8 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -526,32 +526,36 @@ static int scmi_perf_level_limits_notify(const struct scmi_protocol_handle *ph,
>  }
>  
>  static void scmi_perf_domain_init_fc(const struct scmi_protocol_handle *ph,
> -				     u32 domain, struct scmi_fc_info **p_fc)
> +				     u32 domain, struct perf_dom_info* dom)
>  {
> +	struct scmi_fc_info **p_fc = &dom->fc_info;
>  	struct scmi_fc_info *fc;
>  
>  	fc = devm_kcalloc(ph->dev, PERF_FC_MAX, sizeof(*fc), GFP_KERNEL);
>  	if (!fc)
>  		return;
>  
> -	ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> -				   PERF_LEVEL_SET, 4, domain,
> -				   &fc[PERF_FC_LEVEL].set_addr,
> -				   &fc[PERF_FC_LEVEL].set_db);
> +	if (dom->set_perf) {
> +		ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> +					   PERF_LEVEL_SET, 4, domain,
> +					   &fc[PERF_FC_LEVEL].set_addr,
> +					   &fc[PERF_FC_LEVEL].set_db);
>  
> -	ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> -				   PERF_LEVEL_GET, 4, domain,
> -				   &fc[PERF_FC_LEVEL].get_addr, NULL);
> -
> -	ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> -				   PERF_LIMITS_SET, 8, domain,
> -				   &fc[PERF_FC_LIMIT].set_addr,
> -				   &fc[PERF_FC_LIMIT].set_db);
> +		ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> +					   PERF_LEVEL_GET, 4, domain,
> +					   &fc[PERF_FC_LEVEL].get_addr, NULL);
> +	}
>  
> -	ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> -				   PERF_LIMITS_GET, 8, domain,
> -				   &fc[PERF_FC_LIMIT].get_addr, NULL);
> +	if (dom->set_limits) {
> +		ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> +					   PERF_LIMITS_SET, 8, domain,
> +					   &fc[PERF_FC_LIMIT].set_addr,
> +					   &fc[PERF_FC_LIMIT].set_db);
>  
> +		ph->hops->fastchannel_init(ph, PERF_DESCRIBE_FASTCHANNEL,
> +					   PERF_LIMITS_GET, 8, domain,
> +					   &fc[PERF_FC_LIMIT].get_addr, NULL);
> +	}
>  	*p_fc = fc;
>  }
>  
> @@ -835,7 +839,7 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
>  		scmi_perf_describe_levels_get(ph, domain, dom);
>  
>  		if (dom->perf_fastchannels)
> -			scmi_perf_domain_init_fc(ph, domain, &dom->fc_info);
> +			scmi_perf_domain_init_fc(ph, domain, dom);
>  	}
>  
>  	pinfo->version = version;
> -- 
> 2.42.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel