linux-kernel - Re: [PATCH] platform/chrome: wilco_ec: Add debugfs test

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: <20190918173236.GB24119@chromium.org>
Date:   Wed, 18 Sep 2019 11:32:36 -0600
From:   Daniel Campello <campello@...omium.org>
To:     Nick Crews <ncrews@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Benson Leung <bleung@...omium.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Duncan Laurie <dlaurie@...gle.com>,
        Trent Begin <tbegin@...gle.com>
Subject: Re: [PATCH] platform/chrome: wilco_ec: Add debugfs test_event file

On Fri, Sep 06, 2019 at 06:20:51PM -0400, Nick Crews wrote:
> Thanks for the patch Daniel! A few thoughts that I didn't
> have on the review on Gerrit, sorry :) After those changes,
>
No problem, I will take care of them.

> Reviewed-by: Nick Crews <ncrews@...omium.org>
>
> >         u8 sub_cmd;     /* Always SUB_CMD_H1_GPIO */
>
> This comment should be removed.
>
I overlooked this. Done.

> This comment should be moved to h1_gpio_get().
>
Agree

> > -static int h1_gpio_get(void *arg, u64 *val)
> > +static int write_to_mailbox(struct wilco_ec_device *ec, u8 sub_cmd, u64 *val)
>
> What about send_ec_cmd() or similar? Something that communicates that
> we are sometimes telling the EC to do something, and sometimes reading something
> back. Also, since we are adding in another layer in here, we can fix
> the signature from
> the one required by a debugfs attribute. Use "ret" instead of "val"
> and make it a u8*.
>
send_ec_cmd() sounds good to me but ret is already used. Will change
to out_val as discussed offline.

> A one line comment as to what test_event does?
>
> > +static int test_event_set(void *arg, u64 val)
> > +{
> > +       u64 ret;
> > +
> > +       return write_to_mailbox(arg, SUB_CMD_TEST_EVENT, &ret);
> > +}
> > +

Will do. Thanks again.