linux-kernel - Re: [PATCH V1 3/3] mmc: host: Register changes for sdcc V5

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: <CAE=gft7wmBRA=qy28mSdSCRsVLCkNyMV=uJMk2RpTFHgzS-YTw@mail.gmail.com>
Date:   Fri, 25 May 2018 13:46:42 -0700
From:   Evan Green <evgreen@...omium.org>
To:     vviswana@...eaurora.org
Cc:     adrian.hunter@...el.com, Ulf Hansson <ulf.hansson@...aro.org>,
        robh+dt@...nel.org, mark.rutland@....com,
        linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
        shawn.lin@...k-chips.com, linux-arm-msm@...r.kernel.org,
        georgi.djakov@...aro.org, devicetree@...r.kernel.org,
        asutoshd@...eaurora.org, stummala@...eaurora.org,
        venkatg@...eaurora.org, jeremymc@...hat.com,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        riteshh@...eaurora.org, vbadigan@...eaurora.org,
        Doug Anderson <dianders@...gle.com>, sayalil@...eaurora.org
Subject: Re: [PATCH V1 3/3] mmc: host: Register changes for sdcc V5

On Thu, May 24, 2018 at 6:01 AM Vijay Viswanath <vviswana@...eaurora.org>
wrote:



> On 5/22/2018 11:42 PM, Evan Green wrote:
> > Hi Vijay. Thanks for this patch.
> >
> > On Thu, May 17, 2018 at 3:30 AM Vijay Viswanath <vviswana@...eaurora.org

> > wrote:
> >
> >> From: Sayali Lokhande <sayalil@...eaurora.org>
> >
...
> > Nit: host->ioaddr + msm_offset->core_dll_config might benefit from
having
> > its own local, since you use it so much in this function. Same goes for
> > where I've noted below...
> >

> core_dll_config is very much used. But having a local for it feels like
> a bad idea. As different versions come up, the most used register may
> change. So it would be better to stick to a consistent approach to
> accessing every register.


I generally optimize for readability, rather than find/replace-ability. In
my opinion, it's distracting to see that expression copy/pasted so many
times in the same function. But ultimately this is a style preference, so
if you decide not to do it, I'll live.

> >
> >> +                       msm_offset->core_pwrctl_status),
> >> +               msm_host->var_ops->msm_readl_relaxed(host,
> >> +                       msm_offset->core_pwrctl_mask),
> >> +               msm_host->var_ops->msm_readl_relaxed(host,
> >> +                       msm_offset->core_pwrctl_ctl));
> >
> > I think the idea of function pointers is fine, but overall the use of
them
> > everywhere sure is ugly. It makes it really hard to actually see what's
> > happening. I wonder if things might look a lot cleaner with a helper
> > function here. Then instead of:
> >
> > msm_host->var_ops->msm_readl_relaxed(host, msm_offset->core_pwrctl_ctl);
> >
> > You could have
> >
> > msm_core_read(host, msm_offset->core_pwrctl_ctl);
> >

> if we use a helper function, then we will have to pass msm_host into it
> as well. Otherwise there would be the hassle of deriving msm_host
> address from sdhci_host.

> How about using a MACRO here instead for readability ?


The deriving part in the helper would likely get inlined and shared by the
compiler among all call-sites within a function. But yes, a macro would
work too.

Thanks Vijay,
Evan