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

[sean yang]

Cristian Marussi <cristian.marussi at 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 at 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
> >