linux-kernel - Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs

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]

Date:   Tue, 8 Jan 2019 10:12:44 -0800
From:   Moritz Fischer <mdf@...nel.org>
To:     Enric Balletbo i Serra <enric.balletbo@...labora.com>
Cc:     Guenter Roeck <groeck@...gle.com>,
        Moritz Fischer <moritz.fischer@...us.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        Lee Jones <lee.jones@...aro.org>, moritz@...us.com,
        Moritz Fischer <mdf@...nel.org>
Subject: Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for
 I2C/SPI based ECs

Hi,

On Tue, Jan 08, 2019 at 04:00:58PM +0100, Enric Balletbo i Serra wrote:
> Hi,
> 
> On 8/1/19 15:34, Guenter Roeck wrote:
> > On Tue, Jan 8, 2019 at 6:28 AM Enric Balletbo i Serra
> > <enric.balletbo@...labora.com> wrote:
> >>
> >> Hi Moritz,
> >>
> >> Many thanks for the patch,I've one concern on this, and I'd be also interested
> >> on Benson and Guenter opinion ...
> >>
> >> On 8/1/19 4:56, Moritz Fischer wrote:
> >>> From: Moritz Fischer <mdf@...nel.org>
> >>>
> >>> Add cros_ec_readmem() based helpers for I2C/SPI based ECs.

That sentence also should probably get reworked on my end :)
> >>> Unlike the LPC based ECs the I2C/SPI based ones cannot just directly
> >>> read the mapped region.
> >>>
> >>> This is useful for things like accessing fan speeds or temperatures.
> >>>
> >>> Signed-off-by: Moritz Fischer <mdf@...nel.org>
> >>> ---
> >>>
> >>> Hi all,
> >>>
> >>>
> >>> I had submitted this back in May 2017 [1] and then somewhat forgot
> >>> about it. So here's a new version :)
> >>>
> >>> As to why is this required? we're using this in some of our devices
> >>> [2] that run a modified firmware based on Chromium-EC.
> >>> I've been carrying the patch downstream in our tree for a while and
> >>> it would be nice if we can merge this upstream so I can stop rebasing
> >>> this :)
> >>>
> >>
> >> Right, if we merge this you'll probably reduce your downstream patches but
> >> actually, if I am not wrong, there is no user for this. There isn't an upstream
> >> driver that uses the new functions so we will end up having upstream dead code.
> >> IMO if you want to have this upstream you should also upstream the drivers that
> >> use that code. Makes sense?

Sure, see below.
> >>
> > 
> > He has a hwmon driver which I think uses it. 
> 
> Nice, sorry Moritz because the hwmon driver was not on my radar, I missed it.
> The patch needs some rework I guess so would be nice have all together in the
> same patch series.

My fault, sorry, the reason there is that we have a bunch of versions of
this driver internally (some of them thermal drivers, some of the hwmon
drivers). All of them are open source and in our meta-ettus yocto layer,
just need cleanup on our end first.

> 
> > Question would rather be
> > if that is an acceptable use or if it exposes EC internals, ie non-API
> > data. Comments, anyone ?
> > 
> 
> Yes, that's the question. We have similar command for devices that use the lpc
> bus.  On my side I'll take a deeper look. More Comments ?

Yeah basically one comment from my side: The cros_ec_readmem only
exposes a bunch of 'virtual' registers on I2C/SPI based ECs so it's
not like you're leaking non-API data, but merely brings functionality
for I2C/SPI based ECs to parity :)

Thanks for considering, I'll take another look at the Kbuild issue

Moritz