linux-kernel - Re: [PATCH v5] 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]

Date:   Tue, 2 Apr 2019 04:46:10 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Gwendal Grignou <gwendal@...omium.org>
Cc:     andy.shevchenko@...il.com, groeck@...omium.org,
        enric.balletbo@...labora.com, linux-kernel@...r.kernel.org,
        kernel@...labora.com
Subject: Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy
 driver as a subdevice

On Wed, 27 Feb 2019, Gwendal Grignou wrote:

> From: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> 
> 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 v5:
> - Remove unnecessary white lines.
> 
> 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 | 66 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index d275deaecb12..64567bd0a081 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -376,6 +376,69 @@ 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 }
> +};

I'm still very uncomfortable with this struct.

Other than these indices, the sensors have no other distinguishing
features, thus there should be no need to identify or distinguish
between them in this way.

> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> +	{
> +		.name = "cros-ec-accel-legacy",
> +		.id = 0,
> +		.platform_data = &sensor_platforms[0],
> +		.pdata_size = sizeof(struct cros_ec_sensor_platform),
> +	},
> +	{
> +		.name = "cros-ec-accel-legacy",
> +		.id = 1,
> +		.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,

Using PLATFORM_DEVID_AUTO whilst providing the MFD cells with IDs
doesn't make any sense.  Please remove the IDs from the cells.

> +			      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_cec_cells[] = {
>  	{ .name = "cros-ec-cec" }
>  };
> @@ -437,6 +500,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 CEC host command support */
>  	if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {

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