linux-kernel - Re: [PATCH v4 5/8] mfd: cros_ec_dev: Register cros_ec_accel

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]
Message-ID: <20180416132055.udgkpge6poaqix3z@dell>
Date:   Mon, 16 Apr 2018 14:20:55 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Enric Balletbo Serra <eballetbo@...il.com>
Cc:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Guenter Roeck <groeck@...omium.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        kernel@...labora.com, Gwendal Grignou <gwendal@...omium.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 5/8] mfd: cros_ec_dev: Register cros_ec_accel_legacy
 driver as a subdevice.

On Wed, 04 Apr 2018, Enric Balletbo Serra wrote:

> Hi again,
> 
> 2018-04-04 10:03 GMT+02:00 Enric Balletbo Serra <eballetbo@...il.com>:
> > Hi Lee,
> >
> > 2018-03-28 13:03 GMT+02:00 Lee Jones <lee.jones@...aro.org>:
> >> On Tue, 20 Mar 2018, Enric Balletbo i Serra wrote:
> >>
> >>> With this patch, the cros_ec_ctl driver will register the legacy
> >>> accelerometer driver (named cros_ec_accel_legacy) if it fails to
> >>> register sensors through the usual path cros_ec_sensors_register().
> >>> This legacy device is present on Chromebook devices with older EC
> >>> firmware only supporting deprecated EC commands (Glimmer based devices).
> >>>
> >>> Tested-by: Gwendal Grignou <gwendal@...omium.org>
> >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> >>> Reviewed-by: Gwendal Grignou <gwendal@...omium.org>
> >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> >>> ---
> >>>
> >>> Changes in v4:
> >>> - [5/8] Nit: EC -> ECs (Lee Jones)
> >>> - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> >>>
> >>> Changes in v3:
> >>> - [5/8] Add the Reviewed-by Andy Shevchenko.
> >>>
> >>> Changes in v2:
> >>> - [5/8] Add the Reviewed-by Gwendal.
> >>>
> >>>  drivers/mfd/cros_ec_dev.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 70 insertions(+)
> >>>
> >>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> >>> index f60a53f11942..0d541d59d6f5 100644
> >>> --- a/drivers/mfd/cros_ec_dev.c
> >>> +++ b/drivers/mfd/cros_ec_dev.c
> >>> @@ -389,6 +389,73 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> >>>       kfree(msg);
> >>>  }
> >>>
> >>> +static struct cros_ec_sensor_platform sensor_platforms[] = {
> >>> +     {
> >>> +             .sensor_num = 0,
> >>> +     },
> >>> +     {
> >>> +             .sensor_num = 1,
> >>> +     }
> >>> +};
> >>
> >> Also no need to be so many  lines.
> >>
> >> Each one of these entries can be placed on a single line.
> >>
> >> And there's no need for a comma if there is nothing to separate.
> >>
> >>         { .sensor_num = 0 },
> >>         { .sensor_num = 1 }
> >>
> >> Also, this seems like a pretty pointless struct.
> >>
> >> What is the sensor_num property used for?
> >>
> >> Why does it care what sensor number it is?
> >>
> >
> > I thought that was used but after look again I didn't see where, so
> > seems that you have reason and this struct is pointless. I'll remove
> > this and the .id in the cells, we can always send a patch later
> > introducing this if I am missing something. I'll send another version.
> >
> 
> Ok, forget what I said.
> 
> Actually, sensor_num is used by the cros_ec_legacy driver to get the
> right sensor properties from the EC and to get the sensor data from
> the EC, sensor_num is the offset passed to the EC read command.

I'm sure that it is being used, but my question is, why?

Passing a 0 and a 1 to a child driver seems like a pointless exercise
to me.  We do not usually enumerate devices like this with platform
data.  Do these values ever change, or are they simply used to
enumerate 2 devices which are always called 0 and 1?

> .id is not used but as there are two accelerometers that use the same
> driver, shouldn't we set the id (or I am missing something)?
> 
> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> +     { .name = "cros-ec-accel-legacy", .id = 0 },
> +     { .name = "cros-ec-accel-legacy", .id = 1 },
> + };

I think you need to understand what MFD/Platform code does with these
IDs.  Take a look at the MFD/Platform core code to enlighten
yourself.  If there's anything you then do not understand, please
ask and I'll try to fill in the gaps.

> >>> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> >>> +     {
> >>> +             .name = "cros-ec-accel-legacy",
> >>> +             .id = 0,
> >>
> >> What are you using this for?
> >>
> >>> +             .platform_data = &sensor_platforms[0],
> >>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> >>> +     },
> >>> +     {
> >>> +             .name = "cros-ec-accel-legacy",
> >>> +             .id = 1,
> >>
> >> And this?
> >>
> >>> +             .platform_data = &sensor_platforms[1],
> >>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> >>> +     }
> >>> +};
> >>> +
> >>> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
> >>> +{
> >>> +     struct cros_ec_device *ec_dev = ec->ec_dev;
> >>> +     u8 status;
> >>> +     int ret;
> >>> +
> >>> +     /*
> >>> +      * ECs that need legacy support are the main EC, directly connected to
> >>> +      * the AP.
> >>> +      */
> >>> +     if (ec->cmd_offset != 0)
> >>> +             return;
> >>> +
> >>> +     /*
> >>> +      * Check if EC supports direct memory reads and if EC has
> >>> +      * accelerometers.
> >>> +      */
> >>> +     if (!ec_dev->cmd_readmem)
> >>> +             return;
> >>> +
> >>> +     ret = ec_dev->cmd_readmem(ec_dev, EC_MEMMAP_ACC_STATUS, 1, &status);
> >>> +     if (ret < 0) {
> >>> +             dev_warn(ec->dev, "EC does not support direct reads.\n");
> >>> +             return;
> >>> +     }
> >>> +
> >>> +     /* Check if EC has accelerometers. */
> >>> +     if (!(status & EC_MEMMAP_ACC_STATUS_PRESENCE_BIT)) {
> >>> +             dev_info(ec->dev, "EC does not have accelerometers.\n");
> >>> +             return;
> >>> +     }
> >>> +
> >>> +     /*
> >>> +      * Register 2 accelerometers
> >>> +      */
> >>> +     ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> >>> +                           cros_ec_accel_legacy_cells,
> >>> +                           ARRAY_SIZE(cros_ec_accel_legacy_cells),
> >>> +                           NULL, 0, NULL);
> >>> +     if (ret)
> >>> +             dev_err(ec_dev->dev, "failed to add EC sensors\n");
> >>> +}
> >>> +
> >>>  static const struct mfd_cell cros_ec_rtc_cells[] = {
> >>>       {
> >>>               .name = "cros-ec-rtc",
> >>> @@ -442,6 +509,9 @@ static int ec_device_probe(struct platform_device *pdev)
> >>>       /* check whether this EC is a sensor hub. */
> >>>       if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
> >>>               cros_ec_sensors_register(ec);
> >>> +     else
> >>> +             /* Workaroud for older EC firmware */
> >>> +             cros_ec_accel_legacy_register(ec);
> >>>
> >>>       /* Check whether this EC instance has RTC host command support */
> >>>       if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
> >>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog