linux-kernel - Re: [PATCH 2/5] firmware: arm_scmi: Make use SCMI v2.0 fastchannel for performance protocol

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 5 Aug 2019 14:28:33 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Etienne Carriere <etienne.carriere@...aro.org>
Cc:     linux-arm-kernel@...ts.infradead.org, Peng Fan <peng.fan@....com>,
        linux-kernel@...r.kernel.org,
        Bo Zhang <bozhang.zhang@...adcom.com>,
        Jim Quinlan <james.quinlan@...adcom.com>,
        Volodymyr Babchuk <volodymyr_babchuk@...m.com>,
        Gaku Inami <gaku.inami.xh@...esas.com>,
        aidapala@....qualcomm.com, pajay@....qualcomm.com,
        Souvik Chakravarty <Souvik.Chakravarty@....com>,
        wesleys@...inx.com, Felix Burton <fburton@...inx.com>,
        Saeed Nowshadi <saeed.nowshadi@...inx.com>,
        Ionela Voinescu <Ionela.Voinescu@....com>,
        Chris Redpath <Chris.Redpath@....com>,
        Quentin Perret <Quentin.Perret@....com>,
        Sudeep Holla <sudeep.holla@....com>
Subject: Re: [PATCH 2/5] firmware: arm_scmi: Make use SCMI v2.0 fastchannel
 for performance protocol

On Mon, Aug 05, 2019 at 03:16:41PM +0200, Etienne Carriere wrote:
> On Fri, 26 Jul 2019 at 16:00, Sudeep Holla <sudeep.holla@....com> wrote:
> >
> > SCMI v2.0 adds support for "FastChannel" which do not use a message
> > header as they are specialized for a single message.
> >
> > Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
> > commands are supported over fastchannels. As they are optional, they
> > need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
> > Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.
> >
> > Add support for making use of these fastchannels.
> >
> > Cc: Ionela Voinescu <Ionela.Voinescu@....com>
> > Cc: Chris Redpath <Chris.Redpath@....com>
> > Cc: Quentin Perret <Quentin.Perret@....com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> > ---
> >  drivers/firmware/arm_scmi/perf.c | 104 +++++++++++++++++++++++++++++--
> >  1 file changed, 100 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> > index 6cce3e82e81e..b9144efbd6fb 100644
> > --- a/drivers/firmware/arm_scmi/perf.c
> > +++ b/drivers/firmware/arm_scmi/perf.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/bits.h>
> >  #include <linux/of.h>
> >  #include <linux/io.h>
> > +#include <linux/io-64-nonatomic-hi-lo.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_opp.h>
> >  #include <linux/sort.h>
> > @@ -293,7 +294,42 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
> >         return ret;
> >  }
> >
> > -static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> > +#define SCMI_PERF_FC_RING_DB(doorbell, w)              \
>
> Suggest to reformat into a macro style:
>     #define SCMI_PERF_FC_RING_DB(doorbell, w)              \
>         do { \
>                 (...) \
>         } while (0)
>

Sure I can try that.

> > +{                                                      \
> > +       u##w val = 0;                                   \
> > +       struct scmi_fc_db_info *db = doorbell;          \
> > +                                                       \
> > +       if ((db)->mask)                                 \
>
> remove parentheses. I would say, could simply remove `if (db->mask)` here.

Ah, missed to drop this one. We can avoid reading the value if mask = 0,
so I prefer to keep it.
>
> > +               val = ioread##w(db->addr) & db->mask;   \
> > +       iowrite##w((u##w)db->set | val, db->addr);      \
> > +}
> > +
> > +static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
> > +{
> > +       if (!db || !db->addr)
> > +               return;
> > +
> > +       if (db->width == 1)
> > +               SCMI_PERF_FC_RING_DB(db, 8)
> > +       else if (db->width == 2)
> > +               SCMI_PERF_FC_RING_DB(db, 16)
> > +       else if (db->width == 4)
> > +               SCMI_PERF_FC_RING_DB(db, 32)
> > +       else /* db->width == 8 */
> > +#ifdef CONFIG_64BIT
> > +               SCMI_PERF_FC_RING_DB(db, 64)
> > +#else
> > +       {
> > +               u64 val = 0;
> > +
> > +               if (db->mask)
> > +                       val = ioread64_hi_lo(db->addr) & db->mask;
> > +               iowrite64_hi_lo(db->set, db->addr);
>
> Is `value` missing here?
> Why not using SCMI_PERF_FC_RING_DB(db, 64) here?
>

I am still using it. Just if CONFIG_64BIT is enabled and  io{read,write}64
are defined. ARM32/MIPS and other 32-bit platform build might fail
otherwise. I don't want to restrict SCMI to 64-bit platforms only.

--
Regards,
Sudeep