linux-kernel - Re: [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

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:   Tue, 03 Apr 2018 17:55:55 +0000
From:   Evan Green <evgreen@...omium.org>
To:     tdas@...eaurora.org
Cc:     sboyd@...eaurora.org, Michael Turquette <mturquette@...libre.com>,
        Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        okukatla@...eaurora.org, anischal@...eaurora.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        collinsd@...eaurora.org
Subject: Re: [PATCH 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

Hi Taniya,

On Mon, Apr 2, 2018 at 3:33 AM Taniya Das <tdas@...eaurora.org> wrote:

> Hello Evan,

> Thanks for the review comments.

> On 3/30/2018 3:19 AM, Evan Green wrote:
> > Hi Taniya,
> >
> > On Wed, Mar 28, 2018 at 11:19 PM Taniya Das <tdas@...eaurora.org> wrote:
> >
> >> From: Amit Nischal <anischal@...eaurora.org>

> >> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
> >> +{
> >> +       struct tcs_cmd cmd = { 0 };
> >> +       int ret = 0;
> >> +
> >> +       cmd.addr = c->res_addr + c->res_en_offset;
> >> +
> >> +       if (has_state_changed(c, RPMH_SLEEP_STATE)) {
> >> +               cmd.data = (c->aggr_state >> RPMH_SLEEP_STATE) & 1
> >> +                       ? c->res_on_val : c->res_off_val;
> >
> > I found it quite confusing that you're shifting in the opposite
direction
> > than in has_state_changed. How about this:
> >
> > cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE)) ? c->res_on_val :
> > c->res_off_val;
> >

> It is kept keeping in mind the consistency of the command data for all
> the different states.

> Would it be better if I define a new macro ?

> #define RPMH_CMD_DATA(aggr_state, rpmh_state) \
>           ((aggr_state >> rpmh_state) & 1)

> If you agree, I could use the new macro in the next series.

I'm afraid I don't really understand the "consistency of command data for
all the different states". It seems like every other time you refer to a
state mask (eg state, valid_state_mask, and all the *_STATE_MASK defines,
you use BIT(state). The version above seemed like an unnecessarily
confusing variant. In this case, aggr_state is only being used as a
conditional to determine whether to put the res_on or off value, so it's
not like that form is needed because of hardware bitfields or something.
All of the state-like members seem to be purely software masks.

Anyway, I'll leave it to you after this. If you choose to keep the shifts
as written and hide them behind a macro, the name as you've suggested isn't
great, as that macro only gives you the conditional for which data to use,
not the data itself as the macro name might have you believe.

-Evan