| 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
| ||
|
Date: Mon, 11 Apr 2016 15:04:33 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Sameer Nanda <snanda@...omium.org>,
Javier Martinez Canillas <javier@....samsung.com>,
Benson Leung <bleung@...omium.org>,
Enric Balletbò
<enric.balletbo@...labora.co.uk>,
Vic Yang <victoryang@...omium.org>,
Stephen Boyd <sboyd@...eaurora.org>,
Vincent Palatin <vpalatin@...omium.org>,
Randall Spangler <rspangler@...omium.org>,
Todd Broch <tbroch@...omium.org>,
Gwendal Grignou <gwendal@...omium.org>,
Vic Yang <victoryang@...gle.com>,
Olof Johansson <olof@...om.net>, linux-input@...r.kernel.org,
Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [RESEND PATCH v7 1/6] mfd: cros_ec: Add MKBP event support
On Mon, 11 Apr 2016, Tomeu Vizoso wrote:
> On 7 April 2016 at 17:29, Lee Jones <lee.jones@...aro.org> wrote:
> > On Tue, 05 Apr 2016, Tomeu Vizoso wrote:
> >
> >> From: Vic Yang <victoryang@...gle.com>
> >>
> >> Newer revisions of the ChromeOS EC add more events besides the keyboard
> >> ones. So handle interrupts in the MFD driver and let consumers register
> >> for notifications for the events they might care.
> >>
> >> To keep backward compatibility, if the EC doesn't support MKBP event, we
> >> fall back to the old MKBP key matrix host command.
> >>
> >> Signed-off-by: Vic Yang <victoryang@...omium.org>
> >> [bleung: fixup some context changes going from v3.14 to v3.18]
> >> Signed-off-by: Benson Leung <bleung@...omium.org>
> >> [tomeu: adapted to changes in mainline (in power-supply and platform/chrome)]
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
> >
> > I'm not interested in your BSP submission path. As far as I'm
> > concerned *this* is the first submission. If these guys are happy
> > with the patch, they can either choose to Ack or Review it. Drop the
> > blurb in the middle.
>
> Ok.
>
> >> Cc: Randall Spangler <rspangler@...omium.org>
> >> Cc: Vincent Palatin <vpalatin@...omium.org>
> >> ---
> >>
> >> Changes in v7: None
> >> Changes in v6: None
> >> Changes in v5: None
> >> Changes in v4:
> >> - Calculate correctly the size of the payloads in
> >> cros_ec_get_host_command_version_mask.
> >>
> >> Changes in v3:
> >> - Remove duplicated prototype of cros_ec_get_host_event.
> >>
> >> Changes in v2:
> >> - Allocate enough for the structs in cros_ec_get_host_command_version_mask,
> >> not their pointers.
> >> - Allocate msg in the stack in get_next_event and
> >> get_keyboard_state_event, as suggested by Gwendal.
> >>
> >> drivers/input/keyboard/cros_ec_keyb.c | 135 ++++++++------------------------
> >> drivers/mfd/cros_ec.c | 57 +++++++++++++-
> >> drivers/platform/chrome/cros_ec_proto.c | 92 ++++++++++++++++++++++
> >> include/linux/mfd/cros_ec.h | 34 ++++++++
> >> include/linux/mfd/cros_ec_commands.h | 34 ++++++++
> >
> > What are the *build time* dependencies that warrant all of these
> > changes happening in one patch?
>
> We can split further the changes without breaking the builds, but
> cros-ec functionality will be broken in between if both the mfd driver
> and the input driver handle the same interrupt, which will hurt
> bisectability.
Then you need to mention that in the cover letter *after* you've split
the patches out. That will inform us that the patches need to be
taken together, massively reducing the chances of a fail. The chances
that a) a bisect will fall exactly between the 2 patches and then b)
the drivers try to handle the same interrupt at the same time are
significantly small enough for us to take that risk.
> >> 5 files changed, 245 insertions(+), 107 deletions(-)
> >
> > [...]
> >
> >> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> >> index 0eee63542038..fbe78b819fdd 100644
> >> --- a/drivers/mfd/cros_ec.c
> >> +++ b/drivers/mfd/cros_ec.c
> >> @@ -23,6 +23,7 @@
> >> #include <linux/module.h>
> >> #include <linux/mfd/core.h>
> >> #include <linux/mfd/cros_ec.h>
> >> +#include <asm/unaligned.h>
> >>
> >> #define CROS_EC_DEV_EC_INDEX 0
> >> #define CROS_EC_DEV_PD_INDEX 1
> >> @@ -49,11 +50,28 @@ static const struct mfd_cell ec_pd_cell = {
> >> .pdata_size = sizeof(pd_p),
> >> };
> >>
> >> +static irqreturn_t ec_irq_thread(int irq, void *data)
> >> +{
> >> + struct cros_ec_device *ec_dev = data;
> >> + int ret;
> >> +
> >> + if (device_may_wakeup(ec_dev->dev))
> >> + pm_wakeup_event(ec_dev->dev, 0);
> >> +
> >> + ret = cros_ec_get_next_event(ec_dev);
> >> + if (ret > 0)
> >> + blocking_notifier_call_chain(&ec_dev->event_notifier,
> >> + 0, ec_dev);
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> int cros_ec_register(struct cros_ec_device *ec_dev)
> >> {
> >> struct device *dev = ec_dev->dev;
> >> int err = 0;
> >>
> >> + BLOCKING_INIT_NOTIFIER_HEAD(&ec_dev->event_notifier);
> >> +
> >> ec_dev->max_request = sizeof(struct ec_params_hello);
> >> ec_dev->max_response = sizeof(struct ec_response_get_protocol_info);
> >> ec_dev->max_passthru = 0;
> >> @@ -70,13 +88,24 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >>
> >> cros_ec_query_all(ec_dev);
> >>
> >> + if (ec_dev->irq) {
> >> + err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
> >> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> >> + "chromeos-ec", ec_dev);
> >> + if (err) {
> >> + dev_err(dev, "request irq %d: error %d\n",
> >
> > This is an ugly error message. Write them like you (as I user) would
> > like to see. I suggest using proper English and grammar.
> >
> > "Failed to request IRQ %d: %d", irq, err ?
>
> Sounds good.
>
> >> + ec_dev->irq, err);
> >> + return err;
> >> + }
> >> + }
> >> +
> >> err = mfd_add_devices(ec_dev->dev, PLATFORM_DEVID_AUTO, &ec_cell, 1,
> >> NULL, ec_dev->irq, NULL);
> >> if (err) {
> >> dev_err(dev,
> >> "Failed to register Embedded Controller subdevice %d\n",
> >> err);
> >> - return err;
> >> + goto fail_mfd;
> >> }
> >>
> >> if (ec_dev->max_passthru) {
> >> @@ -94,7 +123,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >> dev_err(dev,
> >> "Failed to register Power Delivery subdevice %d\n",
> >> err);
> >> - return err;
> >> + goto fail_mfd;
> >> }
> >> }
> >>
> >> @@ -103,13 +132,18 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >> if (err) {
> >> mfd_remove_devices(dev);
> >> dev_err(dev, "Failed to register sub-devices\n");
> >> - return err;
> >> + goto fail_mfd;
> >> }
> >> }
> >>
> >> dev_info(dev, "Chrome EC device registered\n");
> >>
> >> return 0;
> >> +
> >> +fail_mfd:
> >> + if (ec_dev->irq)
> >> + free_irq(ec_dev->irq, ec_dev);
> >> + return err;
> >> }
> >> EXPORT_SYMBOL(cros_ec_register);
> >>
> >> @@ -136,13 +170,30 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
> >> }
> >> EXPORT_SYMBOL(cros_ec_suspend);
> >>
> >> +static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
> >> +{
> >> + while (cros_ec_get_next_event(ec_dev) > 0)
> >> + blocking_notifier_call_chain(&ec_dev->event_notifier,
> >> + 1, ec_dev);
> >> +}
> >> +
> >> int cros_ec_resume(struct cros_ec_device *ec_dev)
> >> {
> >> enable_irq(ec_dev->irq);
> >>
> >> + /*
> >> + * In some case, we need to distinguish events that occur during
> >
> > s/case/cases/
> >
> > s/distinguish/distinguish between/
>
> Cool.
>
> >> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> >> index a677c2bd485c..ddc935ef1911 100644
> >> --- a/include/linux/mfd/cros_ec.h
> >> +++ b/include/linux/mfd/cros_ec.h
> >> @@ -72,6 +72,24 @@ struct cros_ec_command {
> >> uint8_t data[0];
> >> };
> >>
> >> +/*
> >> + * event_data is used by keyboard or event notifier:
> >> + * event_data format:
> >> + * If MKBP protocol is supported:
> >> + * 0 1
> >> + * +-----------+--------------------------------
> >> + * | type | payload
> >> + * +-----------+--------------------------------
> >> + * |HOST_EVENT | EVENT (32 bit)
> >> + * |KEY_MATRIX | Keyboard keys pressed.
> >> + * |SENSOR_FIFO| Sensors FIFO information.
> >> + *
> >> + * Otherwise:
> >> + * 0 1
> >> + * +-----------+--------------------------------
> >> + * |Unused | Keyboard keys pressed.
> >> + */
> >
> > Personally, I don't think this documentation is required. But if you
> > insist on supplying it, I think it'll be better placed near the
> > 'struct ec_response_get_next_event' definition.
>
> I'm personally fine with leaving it out.
>
> >> /**
> >> * struct cros_ec_device - Information about a ChromeOS EC device
> >> *
> >> @@ -107,6 +125,9 @@ struct cros_ec_command {
> >> * should check msg.result for the EC's result code.
> >> * @pkt_xfer: send packet to EC and get response
> >> * @lock: one transaction at a time
> >> + * @event_notifier: interrupt event notifier for transport devices.
> >> + * @event_data: raw payload transferred with the MKBP event.
> >> + * @event_size: size in bytes of the event data.
> >> */
> >> struct cros_ec_device {
> >>
> >> @@ -135,6 +156,11 @@ struct cros_ec_device {
> >> int (*pkt_xfer)(struct cros_ec_device *ec,
> >> struct cros_ec_command *msg);
> >> struct mutex lock;
> >> + bool mkbp_event_supported;
> >
> > Did you document this?
>
> Oops.
>
> >> + struct blocking_notifier_head event_notifier;
> >> +
> >> + struct ec_response_get_next_event event_data;
> >> + int event_size;
> >> };
> >>
> >> /* struct cros_ec_platform - ChromeOS EC platform information
> >> @@ -252,6 +278,14 @@ int cros_ec_register(struct cros_ec_device *ec_dev);
> >> */
> >> int cros_ec_query_all(struct cros_ec_device *ec_dev);
> >>
> >> +/**
> >> + * cros_ec_get_next_event - Fetch next event from the ChromeOS EC
> >> + *
> >> + * @ec_dev: Device to fetch event from
> >> + * @return 0 if ok, -ve on error
> >
> > I'd prefer easy to read/descriptive over trying to be smart.
> >
> > The 'return' value doesn' require a @. Instead, the return section
> > should look like "Return: <blah>".
> >
> > I suggest: "Return: 0 on success, Linux error number on failure"
>
> Sounds good to me.
>
> >> + */
> >> +int cros_ec_get_next_event(struct cros_ec_device *ec_dev);
> >> +
> >> /* sysfs stuff */
> >> extern struct attribute_group cros_ec_attr_group;
> >> extern struct attribute_group cros_ec_lightbar_attr_group;
> >> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> >> index 13b630c10d4c..d86526f0bd8e 100644
> >> --- a/include/linux/mfd/cros_ec_commands.h
> >> +++ b/include/linux/mfd/cros_ec_commands.h
> >> @@ -1762,6 +1762,40 @@ struct ec_result_keyscan_seq_ctrl {
> >> };
> >> } __packed;
> >>
> >> +/*
> >> + * Get the next pending MKBP event.
> >> + *
> >> + * Returns EC_RES_UNAVAILABLE if there is no event pending.
> >> + */
> >
> > You're documenting this command as if it's a function. This command
> > does nothing by it's self, rather it is supplied to a function call,
> > which does the work. Similarly this command returns nothing, the
> > device will provide the UNAVAILABLE return value. Please update the
> > comment.
>
> I see. This was following the style of the existing command docs, so
> it may end up being a bit surprising.
>
> Thanks a lot,
>
> Tomeu
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists