* [PATCH] firmware: arm_scmi:: Add opinion for init fastchannel @ 2023-10-31 1:36 xinglong.yang 2023-10-31 8:05 ` Cristian Marussi 2023-10-31 9:27 ` Sudeep Holla 0 siblings, 2 replies; 4+ messages in thread From: xinglong.yang @ 2023-10-31 1:36 UTC (permalink / raw) To: xinglong.yang, sudeep.holla, cristian.marussi; +Cc: linux-arm-kernel Fastchannel may not be supported by the platform. It is not need to init the fastchannel if the fastchannel is not supported. 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] firmware: arm_scmi:: Add opinion for init fastchannel 2023-10-31 1:36 [PATCH] firmware: arm_scmi:: Add opinion for init fastchannel xinglong.yang @ 2023-10-31 8:05 ` Cristian Marussi 2023-11-01 12:23 ` sean yang 2023-10-31 9:27 ` Sudeep Holla 1 sibling, 1 reply; 4+ messages in thread From: Cristian Marussi @ 2023-10-31 8:05 UTC (permalink / raw) To: xinglong.yang; +Cc: xinglong.yang, sudeep.holla, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] firmware: arm_scmi:: Add opinion for init fastchannel 2023-10-31 8:05 ` Cristian Marussi @ 2023-11-01 12:23 ` sean yang 0 siblings, 0 replies; 4+ messages in thread From: sean yang @ 2023-11-01 12:23 UTC (permalink / raw) To: Cristian Marussi; +Cc: xinglong.yang, sudeep.holla, linux-arm-kernel Cristian Marussi <cristian.marussi@arm.com> 于2023年10月31日周二 16:05写道： > > 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 ? You opinion is right and the advice is useful, thanks! > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] firmware: arm_scmi:: Add opinion for init fastchannel 2023-10-31 1:36 [PATCH] firmware: arm_scmi:: Add opinion for init fastchannel xinglong.yang 2023-10-31 8:05 ` Cristian Marussi @ 2023-10-31 9:27 ` Sudeep Holla 1 sibling, 0 replies; 4+ messages in thread From: Sudeep Holla @ 2023-10-31 9:27 UTC (permalink / raw) To: xinglong.yang Cc: xinglong.yang, cristian.marussi, Sudeep Holla, linux-arm-kernel 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. > > 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 > @@ -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) If the platform doesn't support fast channels, this must not be set and any change must not take any effect on your platform. Why are you changing this then ? > - scmi_perf_domain_init_fc(ph, domain, &dom->fc_info); > + scmi_perf_domain_init_fc(ph, domain, dom); > } > -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-01 12:32 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-31 1:36 [PATCH] firmware: arm_scmi:: Add opinion for init fastchannel xinglong.yang 2023-10-31 8:05 ` Cristian Marussi 2023-11-01 12:23 ` sean yang 2023-10-31 9:27 ` Sudeep Holla
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox