| 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
| ||
|
Message-ID: <CAFqH_53CF2rOVt8G=amQXC2_0mNXZw2m8_kvkM2wMjx6GyWSaQ@mail.gmail.com>
Date: Thu, 28 Feb 2019 00:05:35 +0100
From: Enric Balletbo Serra <eballetbo@...il.com>
To: Gwendal Grignou <gwendal@...omium.org>
Cc: Enric Balletbo i Serra <enric.balletbo@...labora.com>,
Jett Rink <jettrink@...omium.org>,
Guenter Roeck <groeck@...gle.com>,
Rushikesh S Kadam <rushikesh.s.kadam@...el.com>,
Lee Jones <lee.jones@...aro.org>,
Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
andriy.shevchenko@...el.com, Aaron Durbin <adurbin@...gle.com>,
Duncan Laurie <dlaurie@...gle.com>
Subject: Re: [PATCH] cros_ec: instantiate properly Intel ISH MCU device
Hi Gwendal,
Missatge de Gwendal Grignou <gwendal@...omium.org> del dia dc., 27 de
febr. 2019 a les 19:37:
>
> On Wed, Feb 27, 2019 at 10:08 AM Enric Balletbo i Serra
> <enric.balletbo@...labora.com> wrote:
> >
> > Hi Jett,
> >
> > Many thanks for the explanation.
> >
> > On 27/2/19 16:13, Jett Rink wrote:
> > > The diagram you provided is correct.
> > >
> > > The ISH device is a MCU that can run general purpose code that is
> > > located on the SoC. Typically the ISH collects sensor data and
> > > processes it before giving it to the AP. The ISH HW can run any RTOS,
> > > and in this scenario we are choosing to run the ECOS RTOS. In doing
> > > so, we have access to the standard cros_ec command interface for
> > > communication between the AP and ISH. This is helpful because we have
> > > sensors drivers (Cros EC IIO Sensors) that depend on the cros_ec
> > > command interface.
> > >
> > > The idea behind using a different sysfs path was to ensure that there
> > > weren't any unintended consequences by adding a cros_ec device when
> > > the ISH doesn't support most of the EC type tasks. Here are a few
> > > examples:
> > > - Mosys tool is specifically trying to talk with an EC:
> > > https://chromium.googlesource.com/chromiumos/platform/mosys/+/152a306a82009cd6ca68760b0902fc03781e0d5d/platform/daisy/ec.c#41
> > > - The ectool is specifically trying to talk with an EC:
> > > https://chromium.googlesource.com/chromiumos/platform/ec/+/a3c27b39fa69f280a3728a5cf24de57a5d3ccb0d/util/ectool.c#8546
> That's the default, we can override the name with --name option.
> > >
> > > At least on active project, there has already been confusion that the
> > > normal ectool program doesn't work because the EC on the system is 3rd
> > > party and does not support the normal cros_ec interface. This
> > > confusion would compound if the ISH started presenting the cros_ec
> > > interface and the ectool started talking to the ISH instead of the 3rd
> > > party EC. Maybe we just need to modify ectool to make that situation
> > > less confusing, but this is an example of the cross over using the
> > > cros_ish device name is trying to avoid.
> > >
> >
> > IMHO the problem with names is that is not really scalable, right now, we have
> >
> > /sys/class/chromeos/cros_ec
> > /dev/cros_ec
> >
> > and
> >
> > /sys/class/chromeos/cros_pd
> > /dev/cros_pd
> >
> > but looks like, apart from this, we will have
> >
> > /sys/class/chromeos/cros_ish
> > /dev/cros_ish
> >
> > And a lot more: cros_fp, cros_tp, cros_scp, etc
> >
> >
> > I was thinking in a solution more scalable where all the cros-ec are enumerated
> > by an index, so is more generic. So in a system with 2 cros-ec you'll have
> >
> > /sys/class/chromeos/cros_ec0
> > /dev/cros_ec0
> >
> > /sys/class/chromeos/cros_ec1
> > /dev/cros_ec1
> >
> > ...
> >
> > In such case, from userspace point of view, when you open the device you can
> > send the EC_CMD_GET_FEATURES and know which EC is under the device.
> >
> > One of the problems I see is that some old ECs (cros_pd I guess) doesn't
> > implement that command, in such case maybe we can continue using the name to
> > differentiate from other ECs.
> >
> > I'm unsure yet of the impact of this change though, so I'd like to listen
> > Guenter and Benson opinions here :)
>
> You're right, the cros_ names are based on what the EC provides.
> cros_ec for generic EC, fp, tp for fingerprint, touch pad
> respectively.
> ish is for standalone sensor hub [it does not have to be Intel Sensor Hub].
>
Thanks for the explanation. I didn't know that and I assumed the 'i'
was for 'intel', maybe would be good call cros_ssh or cros_sh to avoid
confusion?
> ChromeOS user space tool are using the cros_XX names directly, like
> biod is using cros_fp.
>
> I agree the number of standalone EC in the system is increasing, but
> it is still bounded.
Let me ask another question :)
I understand different names based on what the EC provides. Is this
also the case for the cros_scp? In other words, will cros_ec and
cros_scp co-exist and provide different functionalities than ec, fp,
tp or standalone sensor hub?
Thanks,
Enric
> Gwendal.
>
>
>
> >
> > Will this solution work for you? Do you see any problem?
> >
> >
> > > At this point though, we will definitely follow the guidance of people
> > > more experienced in kernel design. If using cros_ish as a device name
> > > instead of cros_ec is actually not a good idea, then we can accept
> > > that and move forward and try to see what the fallout is from
> > > userspace tools.
> > >
> > > -Jett
> > >
> > >
> > > On Wed, Feb 27, 2019 at 7:22 AM Enric Balletbo i Serra
> > > <enric.balletbo@...labora.com> wrote:
> > >>
> > >> Hi,
> > >>
> > >> On 26/2/19 18:21, Jett Rink wrote:
> > >>> We are specifically wanting userspace applications to not worry/confuse cros_ish
> > >>> with a normal cros_ec. Adding an attribute instead of changing the path would
> > >>> make it easy for userspace application to forget to check properly before
> > >>> accessing the ish as an EC when it shouldn't.
> > >>>
> > >>> On Mon, Feb 25, 2019 at 4:37 PM Guenter Roeck <groeck@...gle.com
> > >>> <mailto:groeck@...gle.com>> wrote:
> > >>>
> > >>> On Mon, Feb 25, 2019 at 3:22 PM Jett Rink <jettrink@...omium.org
> > >>> <mailto:jettrink@...omium.org>> wrote:
> > >>>
> > >>> A cros_ec and cros_ish device could both be present on the same system.
> > >>> We want to change the device path to ensure that drivers/code further up
> > >>> the stack does not get confuse the ISH with as an EC.
> > >>>
> > >>> The ISH device can export a similar sysfs interface as they both use the
> > >>> same command interface for communication (although they will use
> > >>> different transport layers). The common cros code will detect certain
> > >>> EC_FEATURES and enable the correct subsystem based on the level of
> > >>> functionality the device supports. In the case of the ISH, the sensor
> > >>> subsystem will be enabled.
> > >>>
> > >>> Seems to me it would make more sense to handle that difference with a sysfs
> > >>> attribute (instead of forcing each userspace application to support multiple
> > >>> sysfs paths).
> > >>>
> > >>
> > >> Is still unclear to me what's that ISH device, so I'd appreciate if you can give
> > >> some more background. Trying to understand the topology, makes sense that block
> > >> diagram to you?
> > >>
> > >>
> > >> ---------------------------
> > >> | User Space Applications |
> > >> ---------------------------
> > >>
> > >> ----------------IIO ABI----------------
> > >>
> > >> -----------------------------
> > >> | CrOS EC IIO Sensor Drivers |
> > >> -----------------------------
> > >>
> > >> --------------------------
> > >> | CrOS EC over ISH Driver |
> > >> --------------------------
> > >>
> > >> ---------------- OS ------------------
> > >>
> > >> --------------------------
> > >> | CrOS EC Firmware |
> > >> --------------------------
> > >>
> > >> --------------------------
> > >> | ISH Hardware/Firmware |
> > >> --------------------------
> > >>
> > >> So I'm right assuming that this CrOS EC will implement only the sensor features?
> > >>
> > >> And then, the system will have another CrOS EC implementing other features like
> > >> RTC, USBPD-charger, etc?
> > >>
> > >> Apart from the sensors features, will the CrOS EC ISH implement other features?
> > >>
> > >> I'm a bit worried about the increasing way to use a particular name for
> > >> different CrOS EC, actually we have only cros_ec and cros_pd. But in the
> > >> chromeos kernels there is /dev/cros_fp, /dev/cros_tp, /dev/cros_ish,
> > >> /dev/cros_scp and who knows how many more in the future. So I'm wondering if
> > >> wouldn't be better use standard names, i.e /dev/cros_ec0, /dev/cros_ec1, etc. as
> > >> userspace, for those cases, should be able to query the EC_FEATURE_ISH/FP/TP/SCP
> > >> and know against which EC the device is attached.
> > >>
> > >> Cheers,
> > >> Enric
> > >>
> > >>> Guenter
> > >>>
> > >>>
> > >>> -Jett
> > >>>
> > >>> On Sun, Feb 24, 2019 at 4:03 PM Guenter Roeck <groeck@...gle.com
> > >>> <mailto:groeck@...gle.com>> wrote:
> > >>>
> > >>> On Sun, Feb 24, 2019 at 1:14 AM Rushikesh S Kadam
> > >>> <rushikesh.s.kadam@...el.com <mailto:rushikesh.s.kadam@...el.com>>
> > >>> wrote:
> > >>>
> > >>> Intel Integrated Sensor Hub (ISH) is also a MCU running EC
> > >>> having feature bit EC_FEATURE_ISH. Instantiate it as a special
> > >>> CrOS EC device with device name 'cros_ish'.
> > >>>
> > >>>
> > >>> The type of MCU doesn't really have to be reflected in the sysfs
> > >>> directory path. cros_ec uses different
> > >>> MCUs over time.
> > >>>
> > >>> Will the new path exist in parallel with cros_ec (in other words,
> > >>> will there also be a stand-alone EC in the same system) ? Does it
> > >>> have different or the same sysfs attributes as cros_ec ?
> > >>>
> > >>> Also,, what is the impact on userspace ?
> > >>>
> > >>> Thanks,
> > >>> Guenter
> > >>>
> > >>> Signed-off-by: Rushikesh S Kadam <rushikesh.s.kadam@...el.com
> > >>> <mailto:rushikesh.s.kadam@...el.com>>
> > >>> ---
> > >>> drivers/mfd/cros_ec_dev.c | 10 ++++++++++
> > >>> include/linux/mfd/cros_ec.h | 1 +
> > >>> include/linux/mfd/cros_ec_commands.h | 2 ++
> > >>> 3 files changed, 13 insertions(+)
> > >>>
> > >>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > >>> index 2d0fee4..be499b8 100644
> > >>> --- a/drivers/mfd/cros_ec_dev.c
> > >>> +++ b/drivers/mfd/cros_ec_dev.c
> > >>> @@ -414,6 +414,16 @@ static int ec_device_probe(struct
> > >>> platform_device *pdev)
> > >>> device_initialize(&ec->class_dev);
> > >>> cdev_init(&ec->cdev, &fops);
> > >>>
> > >>> + /* check whether this is actually a Intel ISH rather
> > >>> than an EC */
> > >>> + if (cros_ec_check_features(ec, EC_FEATURE_ISH)) {
> > >>> + dev_info(dev, "Intel ISH MCU detected.\n");
> > >>> + /*
> > >>> + * Help userspace differentiating ECs from ISH MCU,
> > >>> + * regardless of the probing order.
> > >>> + */
> > >>> + ec_platform->ec_name = CROS_EC_DEV_ISH_NAME;
> > >>> + }
> > >>> +
> > >>> /*
> > >>> * Add the class device
> > >>> * Link to the character device for creating the /dev entry
> > >>> diff --git a/include/linux/mfd/cros_ec.h
> > >>> b/include/linux/mfd/cros_ec.h
> > >>> index de8b588..00c5765 100644
> > >>> --- a/include/linux/mfd/cros_ec.h
> > >>> +++ b/include/linux/mfd/cros_ec.h
> > >>> @@ -24,6 +24,7 @@
> > >>>
> > >>> #define CROS_EC_DEV_NAME "cros_ec"
> > >>> #define CROS_EC_DEV_PD_NAME "cros_pd"
> > >>> +#define CROS_EC_DEV_ISH_NAME "cros_ish"
> > >>>
> > >>> /*
> > >>> * The EC is unresponsive for a time after a reboot command. Add a
> > >>> diff --git a/include/linux/mfd/cros_ec_commands.h
> > >>> b/include/linux/mfd/cros_ec_commands.h
> > >>> index fc91082..9276c3c 100644
> > >>> --- a/include/linux/mfd/cros_ec_commands.h
> > >>> +++ b/include/linux/mfd/cros_ec_commands.h
> > >>> @@ -856,6 +856,8 @@ enum ec_feature_code {
> > >>> EC_FEATURE_RTC = 27,
> > >>> /* EC supports CEC commands */
> > >>> EC_FEATURE_CEC = 35,
> > >>> + /* The MCU is an Intel Integrated Sensor Hub */
> > >>> + EC_FEATURE_ISH = 40,
> > >>> };
> > >>>
> > >>> #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> > >>> --
> > >>> 1.9.1
> > >>>
Powered by blists - more mailing lists