linux-kernel - Re: [PATCH v2 2/2] soc: qcom: cmd-db: Stop memcpy()ing in cmd_db_read_aux

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]

Message-ID: <153724073745.119890.15134402813455486753@swboyd.mtv.corp.google.com>
Date:   Mon, 17 Sep 2018 20:18:57 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Jordan Crouse <jcrouse@...eaurora.org>
Cc:     Andy Gross <andy.gross@...aro.org>, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        Mahesh Sivasubramanian <msivasub@...eaurora.org>,
        Lina Iyer <ilina@...eaurora.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Evan Green <evgreen@...omium.org>,
        Rob Clark <robdclark@...il.com>
Subject: Re: [PATCH v2 2/2] soc: qcom: cmd-db: Stop memcpy()ing in
 cmd_db_read_aux_data()

Quoting Jordan Crouse (2018-09-17 14:14:14)
> On Mon, Sep 17, 2018 at 01:40:07PM -0700, Stephen Boyd wrote:
> > +     vals = cmd_db_read_aux_data(id, &len);
> >       /*
> >        * The data comes back as an array of unsigned shorts so adjust the
> >        * count accordingly
> >        */
> > -     return len >> 1;
> > +     len >>= 1;
> > +
> > +     if (!len || WARN_ON(len > 16))
> > +             len = 0;
> 
> I don't think we care any more about the size of the array since there isn't any
> data being copied.
> 

Ok, sounds good.

> > +
> > +     *size = len;
> > +     return vals;
> >  }
> 
> I'm not sure this function has any value any more. We could just call
> cmd_db_read_aux_data() directly for each id and adjust the size on the fly
> when we call a6xx_gmu_rpmh_arc_votes_init.

Ok. Let me attempt to do more code reduction!

> 
> >  /* Return the 'arc-level' for the given frequency */
> > @@ -907,8 +907,8 @@ static u32 a6xx_gmu_get_arc_level(struct device *dev, unsigned long freq)
> >  
> >  static int a6xx_gmu_rpmh_arc_votes_init(struct device *dev, u32 *votes,
> >               unsigned long *freqs, int freqs_count,
> > -             u16 *pri, int pri_count,
> > -             u16 *sec, int sec_count)
> > +             const u16 *pri, int pri_count,
> > +             const u16 *sec, int sec_count)
> >  {
> >       int i, j;
> >  
> > @@ -970,14 +970,14 @@ static int a6xx_gmu_rpmh_votes_init(struct a6xx_gmu *gmu)
> >       struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
> >       struct msm_gpu *gpu = &adreno_gpu->base;
> >  
> > -     u16 gx[16], cx[16], mx[16];
> > +     const u16 *gx, *cx, *mx;
> >       u32 gxcount, cxcount, mxcount;
> >       int ret;
> >  
> >       /* Get the list of available voltage levels for each component */
> > -     gxcount = a6xx_gmu_rpmh_arc_cmds("gfx.lvl", gx, sizeof(gx));
> > -     cxcount = a6xx_gmu_rpmh_arc_cmds("cx.lvl", cx, sizeof(cx));
> > -     mxcount = a6xx_gmu_rpmh_arc_cmds("mx.lvl", mx, sizeof(mx));
> > +     gx = a6xx_gmu_rpmh_arc_cmds("gfx.lvl", &gxcount);
> > +     cx = a6xx_gmu_rpmh_arc_cmds("cx.lvl", &cxcount);
> > +     mx = a6xx_gmu_rpmh_arc_cmds("mx.lvl", &mxcount);
> >  
> >       /* Build the GX votes */
> >       ret = a6xx_gmu_rpmh_arc_votes_init(&gpu->pdev->dev, gmu->gx_arc_votes,
> > diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
> > index 5c9cc6824891..c701b3b010f1 100644
> > --- a/drivers/soc/qcom/cmd-db.c
> > +++ b/drivers/soc/qcom/cmd-db.c
> > @@ -192,55 +192,28 @@ EXPORT_SYMBOL(cmd_db_read_addr);
> >  /**
> >   * cmd_db_read_aux_data() - Query command db for aux data.
> >   *
> > - *  @id: Resource to retrieve AUX Data on.
> > - *  @data: Data buffer to copy returned aux data to. Returns size on NULL
> > - *  @len: Caller provides size of data buffer passed in.
> > + *  @id: Resource to retrieve AUX Data on
> > + *  @len: size of data buffer returned
> 
> Is the GPU the only consumer of this function?  Would it make more sense to
> return the number of entries rather than the size? I would suppose that would
> imply that the caller knows the size of the entries but we have to know that
> anyway if we're going to walk the list.

I think the bus scaling driver also calls this function. I don't see
much advantage to doing that here when we can just consider the aux_data
as a big blob of data that is interpreted however the consumer cares.