* [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 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
* 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
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