linux-kernel - RE: [PATCH v11 08/11] firmware: xilinx: Add debugfs for query data API

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 Sep 2018 19:19:00 +0000
From:   Jolly Shah <JOLLYS@...inx.com>
To:     Olof Johansson <olof@...om.net>
CC:     "ard.biesheuvel@...aro.org" <ard.biesheuvel@...aro.org>,
        Ingo Molnar <mingo@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "matt@...eblueprint.co.uk" <matt@...eblueprint.co.uk>,
        Sudeep Holla <sudeep.holla@....com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Michal Simek <michals@...inx.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-clk <linux-clk@...r.kernel.org>,
        Rajan Vaja <RAJANV@...inx.com>,
        Linux ARM Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>
Subject: RE: [PATCH v11 08/11] firmware: xilinx: Add debugfs for query data
 API

Hi Olof,

> -----Original Message-----
> From: Olof Johansson [mailto:olof@...om.net]
> Sent: Saturday, September 08, 2018 6:15 PM
> To: Jolly Shah <JOLLYS@...inx.com>
> Cc: ard.biesheuvel@...aro.org; Ingo Molnar <mingo@...nel.org>; Greg Kroah-
> Hartman <gregkh@...uxfoundation.org>; matt@...eblueprint.co.uk; Sudeep
> Holla <sudeep.holla@....com>; hkallweit1@...il.com; Kees Cook
> <keescook@...omium.org>; Dmitry Torokhov <dmitry.torokhov@...il.com>;
> Michael Turquette <mturquette@...libre.com>; Stephen Boyd
> <sboyd@...eaurora.org>; Michal Simek <michals@...inx.com>; Rob Herring
> <robh+dt@...nel.org>; Mark Rutland <mark.rutland@....com>; linux-clk
> <linux-clk@...r.kernel.org>; Rajan Vaja <RAJANV@...inx.com>; Linux ARM
> Mailing List <linux-arm-kernel@...ts.infradead.org>; Linux Kernel Mailing List
> <linux-kernel@...r.kernel.org>; DTML <devicetree@...r.kernel.org>; Jolly
> Shah <JOLLYS@...inx.com>
> Subject: Re: [PATCH v11 08/11] firmware: xilinx: Add debugfs for query data API
> 
> Hi,
> 
> I noticed the below when I was reviewing the code for merge into arm-soc.
> Would you mind following up with an incremental patch? I don't think we need
> to ask Michal to respin for this:
> 

Sure. I will submit incremental patches for suggested changes.

Thanks,
Jolly Shah


> On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah <jolly.shah@...inx.com> wrote:
> > From: Rajan Vaja <rajanv@...inx.com>
> >
> > Add debugfs file to query platform specific data from firmware using
> > debugfs interface.
> >
> > Signed-off-by: Rajan Vaja <rajanv@...inx.com>
> > Signed-off-by: Jolly Shah <jollys@...inx.com>
> > ---
> >  drivers/firmware/xilinx/zynqmp-debug.c | 27
> > +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/firmware/xilinx/zynqmp-debug.c
> > b/drivers/firmware/xilinx/zynqmp-debug.c
> > index fc11db9..4532bd0 100644
> > --- a/drivers/firmware/xilinx/zynqmp-debug.c
> > +++ b/drivers/firmware/xilinx/zynqmp-debug.c
> > @@ -33,6 +33,7 @@ static char debugfs_buf[PAGE_SIZE];  static struct
> > pm_api_info pm_api_list[] = {
> >         PM_API(PM_GET_API_VERSION),
> >         PM_API(PM_IOCTL),
> > +       PM_API(PM_QUERY_DATA),
> >  };
> >
> >  /**
> > @@ -105,6 +106,32 @@ static int process_api_request(u32 pm_id, u64
> *pm_api_arg, u32 *pm_api_ret)
> >                         sprintf(debugfs_buf, "IOCTL return value: %u\n",
> >                                 pm_api_ret[1]);
> >                 break;
> > +       case PM_QUERY_DATA:
> > +       {
> > +               struct zynqmp_pm_query_data qdata = {0};
> > +
> > +               qdata.qid = pm_api_arg[0];
> > +               qdata.arg1 = pm_api_arg[1];
> > +               qdata.arg2 = pm_api_arg[2];
> > +               qdata.arg3 = pm_api_arg[3];
> 
> This is usually a pattern we try to avoid (having full code blocks in a switch
> statement, and local variables).
> 
> Please move the declaration of qdata to the top of the function so you can drop
> the braces.
> 
> > +
> > +               ret = eemi_ops->query_data(qdata, pm_api_ret);
> > +               if (ret)
> > +                       break;
> > +
> > +               if (qdata.qid == PM_QID_CLOCK_GET_NAME)
> > +                       sprintf(debugfs_buf, "Clock name = %s\n",
> > +                               (char *)pm_api_ret);
> > +               else if (qdata.qid == PM_QID_CLOCK_GET_FIXEDFACTOR_PARAMS)
> > +                       sprintf(debugfs_buf, "Multiplier = %d, Divider = %d\n",
> > +                               pm_api_ret[1], pm_api_ret[2]);
> > +               else
> > +                       sprintf(debugfs_buf,
> > +                               "data[0] = 0x%08x\ndata[1] = 0x%08x\n data[2] =
> 0x%08x\ndata[3] = 0x%08x\n",
> > +                               pm_api_ret[0], pm_api_ret[1],
> > +                               pm_api_ret[2], pm_api_ret[3]);
> 
> If you anticipate more qids here later, a switch could be nicer than a sequence of
> if/else.
> 
> 
> 
> -Olof