linux-kernel - Re: [PATCH v1 5/6] platform/chrome: Register USB PD charger device

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: <20160210164635.GM3782@x1>
Date:	Wed, 10 Feb 2016 16:46:35 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:	linux-kernel@...r.kernel.org, Sameer Nanda <snanda@...omium.org>,
	Benson Leung <bleung@...omium.org>,
	Enric Balletbò 
	<enric.balletbo@...labora.co.uk>,
	Vic Yang <victoryang@...omium.org>,
	Vincent Palatin <vpalatin@...omium.org>,
	Randall Spangler <rspangler@...omium.org>,
	Gwendal Grignou <gwendal@...omium.org>,
	Olof Johansson <olof@...om.net>
Subject: Re: [PATCH v1 5/6] platform/chrome: Register USB PD charger device

On Fri, 05 Feb 2016, Tomeu Vizoso wrote:

> Check if a EC considers EC_CMD_USB_PD_PORTS a valid command and register
> a USB PD charger device if so. This check is needed for older versions
> of the ChromeOS EC firmware that don't support the EC_CMD_GET_FEATURES
> command.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
> ---
> 
>  drivers/mfd/cros_ec.c                 | 15 +++++++++++++++
>  drivers/platform/chrome/cros_ec_dev.c | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/mfd/cros_ec.h           |  2 ++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index fbe78b819fdd..20ca9794d2f3 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -202,5 +202,20 @@ EXPORT_SYMBOL(cros_ec_resume);
>  
>  #endif
>  
> +static const struct mfd_cell cros_usb_pd_charger_devs[] = {
> +	{
> +		.name = "cros-usb-pd-charger",
> +		.id   = -1,
> +	},
> +};
> +
> +int cros_ec_usb_pd_charger_register(struct device *dev)
> +{
> +	return mfd_add_devices(dev, 0, cros_usb_pd_charger_devs,
> +			       ARRAY_SIZE(cros_usb_pd_charger_devs),
> +			       NULL, 0, NULL);
> +}
> +EXPORT_SYMBOL(cros_ec_usb_pd_charger_register);

I'm not keen on this idea at all.

You're calling cros_ec_usb_pd_charger_register() from a device you
registered from this same source file.  Seems very incestuous and
hacky.

It's bad enough that we're calling into the platform driver from here
already, but seeing as we are, just extend the call to
cros_ec_query_all() to suit your purposes.

>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("ChromeOS EC core driver");
> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
> index d45cd254ed1c..7a97cd313c68 100644
> --- a/drivers/platform/chrome/cros_ec_dev.c
> +++ b/drivers/platform/chrome/cros_ec_dev.c
> @@ -87,6 +87,29 @@ exit:
>  	return ret;
>  }
>  
> +static int cros_ec_has_cmd_usb_pd_ports(struct cros_ec_dev *ec)
> +{
> +	struct cros_ec_command *msg;
> +	int ret;
> +
> +	msg = kmalloc(sizeof(*msg) + sizeof(struct ec_response_usb_pd_ports),
> +		      GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	msg->version = 0;
> +	msg->command = EC_CMD_USB_PD_PORTS + ec->cmd_offset;
> +	msg->insize = sizeof(struct ec_response_usb_pd_ports);
> +	msg->outsize = 0;
> +
> +	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
> +	ret = ret >= 0 && msg->result == EC_RES_SUCCESS;
> +
> +	kfree(msg);
> +
> +	return ret;
> +}
> +
>  /* Device file ops */
>  static int ec_device_open(struct inode *inode, struct file *filp)
>  {
> @@ -269,8 +292,19 @@ static int ec_device_probe(struct platform_device *pdev)
>  		goto dev_reg_failed;
>  	}
>  
> +	/* check whether this EC instance has the PD charge manager */
> +	if (cros_ec_has_cmd_usb_pd_ports(ec)) {
> +		retval = cros_ec_usb_pd_charger_register(dev);
> +		if (retval) {
> +			dev_err(dev, "failed to add usb-pd-charger device\n");
> +			goto pd_reg_failed;
> +		}
> +	}
> +
>  	return 0;
>  
> +pd_reg_failed:
> +	put_device(&ec->class_dev);
>  dev_reg_failed:
>  set_named_failed:
>  	dev_set_drvdata(dev, NULL);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index c21583411ba0..543191f493a9 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -329,4 +329,6 @@ extern struct attribute_group cros_ec_vbc_attr_group;
>   */
>  uint32_t cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>  
> +int cros_ec_usb_pd_charger_register(struct device *dev);
> +
>  #endif /* __LINUX_MFD_CROS_EC_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog