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]

Date:	Fri, 12 Feb 2016 12:06:22 +0100
From:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	"linux-kernel@...r.kernel.org" <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 10 February 2016 at 17:46, Lee Jones <lee.jones@...aro.org> wrote:
> 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.

But cros_ec_query_all() is querying the EC that is directly accessible
from the AP, while the EC that takes care of PD is the one registered
from ec_pd_cell (it sits behind the other EC).

Maybe it's just that cros_ec_usb_pd_charger_register() belongs to cros_ec_dev.c?

Thanks,

Tomeu