Re: [PATCH v2] platform/chrome: cros_ec_chardev: Add a poll handler to receive MKBP events

From: Enric Balletbo i Serra
Date: Thu Sep 19 2019 - 09:05:48 EST

Next message: Qian Cai: ""arm64: use asm-generic/dma-mapping.h" introduced a kbuild warning"
Previous message: Enric Balletbo i Serra: "Re: [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host command when probe failed."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 5/9/19 23:25, Gwendal Grignou wrote:
> On Mon, Sep 2, 2019 at 9:24 AM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>>
>> On Mon, Sep 02, 2019 at 06:08:48PM +0200, Enric Balletbo i Serra wrote:
>>> Allow to poll on the cros_ec device to receive the MKBP events.
>>>
>>> The /dev/cros_[ec|fp|..] file operations now implements the poll
>>> operation. The userspace can now receive specific MKBP events by doing
>>> the
>>> following:
>>> - Open the /dev/cros_XX file.
>>> - Call the CROS_EC_DEV_IOCEVENTMASK ioctl with the bitmap of the MKBP
>>> events it wishes to receive as argument.
>>> - Poll on the file descriptor.
>>> - When it gets POLLIN, do a read on the file descriptor, the first
>>> queued event will be returned (using the struct
>>> ec_response_get_next_event format: one byte of event type, then
>>> the payload).
>>>
>>> The read() operation returns at most one event even if there are several
>>> queued, and it might be truncated if the buffer is smaller than the
>>> event (but the caller should know the maximum size of the events it is
>>> reading).
>>>
>>> read() used to return the EC version string, it still does it when no
>>> event mask or an empty event is set for backward compatibility (despite
>>> nobody really using this feature).
>>>
>>> This will be used, for example, by the userspace daemon to receive and
>>> treat the EC_MKBP_EVENT_FINGERPRINT sent by the FP MCU.
>>>
>>
>> Acked-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Reviewed-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>

I modified the patch with Gwendal nits and applied for 5.4, the patches went to
linux-next some time ago, so sorry for late notice.

Thanks,
Enric

>>
>>> Signed-off-by: Vincent Palatin <vpalatin@xxxxxxxxxxxx>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>>> ---
>>> Note that this patch depends on the immutable branch [1] to apply
>>> cleanly.
>>>
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/log/?h=ib-mfd-extcon-hid-i2c-iio-input-media-chrome-power-pwm-rtc-sound-5.4
>>>
>>>
>>> Changes in v2:
>>> - Moved the code to the cros_ec_chardev driver.
>>>
>>> drivers/platform/chrome/cros_ec_chardev.c | 177 +++++++++++++++++-
>>> include/linux/platform_data/cros_ec_chardev.h | 1 +
>>> 2 files changed, 173 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
>>> index 08abd7e5c7bf..eabb1fe85688 100644
>>> --- a/drivers/platform/chrome/cros_ec_chardev.c
>>> +++ b/drivers/platform/chrome/cros_ec_chardev.c
>>> @@ -16,21 +16,42 @@
>>> #include <linux/mfd/cros_ec.h>
>>> #include <linux/miscdevice.h>
>>> #include <linux/module.h>
>>> +#include <linux/notifier.h>
>>> #include <linux/platform_data/cros_ec_chardev.h>
>>> #include <linux/platform_data/cros_ec_commands.h>
>>> #include <linux/platform_data/cros_ec_proto.h>
>>> #include <linux/platform_device.h>
>>> +#include <linux/poll.h>
>>> #include <linux/slab.h>
>>> #include <linux/types.h>
>>> #include <linux/uaccess.h>
>>>
>>> #define DRV_NAME "cros-ec-chardev"
>>>
>>> +/* Arbitrary bounded size for the event queue */
>>> +#define CROS_MAX_EVENT_LEN PAGE_SIZE
>>> +
>>> struct chardev_data {
>>> struct cros_ec_dev *ec_dev;
>>> struct miscdevice misc;
>>> };
>>>
>>> +struct chardev_priv {
>>> + struct cros_ec_dev *ec_dev;
>>> + struct notifier_block notifier;
>>> + wait_queue_head_t wait_event;
>>> + unsigned long event_mask;
>>> + struct list_head events;
>>> + size_t event_len;
>>> +};
>>> +
>>> +struct ec_event {
>>> + struct list_head node;
>>> + size_t size;
>>> + u8 event_type;
>>> + u8 data[0];
>>> +};
>>> +
>>> static int ec_get_version(struct cros_ec_dev *ec, char *str, int maxlen)
>>> {
>>> static const char * const current_image_name[] = {
>>> @@ -69,6 +90,71 @@ static int ec_get_version(struct cros_ec_dev *ec, char *str, int maxlen)
>>> return ret;
>>> }
>>>
>>> +static int ec_mkbp_event(struct notifier_block *nb,
>>> + unsigned long queued_during_suspend,
>>> + void *_notify)
> nit: call it cros_ec_chardev_mkbp_event() to fit other functions
> naming convention. Change only if a v3 is needed.
>>> +{
>>> + struct chardev_priv *priv = container_of(nb, struct chardev_priv,
>>> + notifier);
>>> + struct cros_ec_device *ec_dev = priv->ec_dev->ec_dev;
>>> + struct ec_event *event;
>>> + unsigned long event_bit = 1 << ec_dev->event_data.event_type;
>>> + int total_size = sizeof(*event) + ec_dev->event_size;
>>> +
>>> + if (!(event_bit & priv->event_mask) ||
>>> + (priv->event_len + total_size) > CROS_MAX_EVENT_LEN)
>>> + return NOTIFY_DONE;
>>> +
>>> + event = kzalloc(total_size, GFP_KERNEL);
>>> + if (!event)
>>> + return NOTIFY_DONE;
>>> +
>>> + event->size = ec_dev->event_size;
>>> + event->event_type = ec_dev->event_data.event_type;
>>> + memcpy(event->data, &ec_dev->event_data.data, ec_dev->event_size);
>>> +
>>> + spin_lock(&priv->wait_event.lock);
>>> + list_add_tail(&event->node, &priv->events);
>>> + priv->event_len += total_size;
>>> + wake_up_locked(&priv->wait_event);
>>> + spin_unlock(&priv->wait_event.lock);
>>> +
>>> + return NOTIFY_OK;
>>> +}
>>> +
>>> +static struct ec_event *ec_fetch_event(struct chardev_priv *priv, bool fetch,
>>> + bool block)
> nit: cros_ec_chardev_fetch_event()
>>> +{
>>> + struct ec_event *event;
>>> + int err;
>>> +
>>> + spin_lock(&priv->wait_event.lock);
>>> + if (!block && list_empty(&priv->events)) {
>>> + event = ERR_PTR(-EWOULDBLOCK);
>>> + goto out;
>>> + }
>>> +
>>> + if (!fetch) {
>>> + event = NULL;
>>> + goto out;
>>> + }
>>> +
>>> + err = wait_event_interruptible_locked(priv->wait_event,
>>> + !list_empty(&priv->events));
>>> + if (err) {
>>> + event = ERR_PTR(err);
>>> + goto out;
>>> + }
>>> +
>>> + event = list_first_entry(&priv->events, struct ec_event, node);
>>> + list_del(&event->node);
>>> + priv->event_len -= sizeof(*event) + event->size;
>>> +
>>> +out:
>>> + spin_unlock(&priv->wait_event.lock);
>>> + return event;
>>> +}
>>> +
>>> /*
>>> * Device file ops
>>> */
>>> @@ -76,11 +162,40 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
>>> {
>>> struct miscdevice *mdev = filp->private_data;
>>> struct cros_ec_dev *ec_dev = dev_get_drvdata(mdev->parent);
>>> + struct chardev_priv *priv;
>>> + int ret;
>>> +
>>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>> + if (!priv)
>>> + return -ENOMEM;
>>>
>>> - filp->private_data = ec_dev;
>>> + priv->ec_dev = ec_dev;
>>> + filp->private_data = priv;
>>> + INIT_LIST_HEAD(&priv->events);
>>> + init_waitqueue_head(&priv->wait_event);
>>> nonseekable_open(inode, filp);
>>>
>>> - return 0;
>>> + priv->notifier.notifier_call = ec_mkbp_event;
>>> + ret = blocking_notifier_chain_register(&ec_dev->ec_dev->event_notifier,
>>> + &priv->notifier);
>>> + if (ret) {
>>> + dev_err(ec_dev->dev, "failed to register event notifier\n");
>>> + kfree(priv);
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static __poll_t cros_ec_chardev_poll(struct file *filp, poll_table *wait)
>>> +{
>>> + struct chardev_priv *priv = filp->private_data;
>>> +
>>> + poll_wait(filp, &priv->wait_event, wait);
>>> +
>>> + if (list_empty(&priv->events))
>>> + return 0;
>>> +
>>> + return EPOLLIN | EPOLLRDNORM;
>>> }
>>>
>>> static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer,
>>> @@ -88,14 +203,42 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer,
>>> {
>>> char msg[sizeof(struct ec_response_get_version) +
>>> sizeof(CROS_EC_DEV_VERSION)];
>>> - struct cros_ec_dev *ec = filp->private_data;
>>> + struct chardev_priv *priv = filp->private_data;
>>> + struct cros_ec_dev *ec_dev = priv->ec_dev;
>>> size_t count;
>>> int ret;
>>>
>>> + if (priv->event_mask) { /* queued MKBP event */
>>> + struct ec_event *event;
>>> +
>>> + event = ec_fetch_event(priv, length != 0,
>>> + !(filp->f_flags & O_NONBLOCK));
>>> + if (IS_ERR(event))
>>> + return PTR_ERR(event);
>>> + /*
>>> + * length == 0 is special - no IO is done but we check
>>> + * for error conditions.
>>> + */
>>> + if (length == 0)
>>> + return 0;
>>> +
>>> + /* The event is 1 byte of type plus the payload */
>>> + count = min(length, event->size + 1);
>>> + ret = copy_to_user(buffer, &event->event_type, count);
>>> + kfree(event);
>>> + if (ret) /* the copy failed */
>>> + return -EFAULT;
>>> + *offset = count;
>>> + return count;
>>> + }
>>> +
>>> + /*
>>> + * Legacy behavior if no event mask is defined
>>> + */
>>> if (*offset != 0)
>>> return 0;
>>>
>>> - ret = ec_get_version(ec, msg, sizeof(msg));
>>> + ret = ec_get_version(ec_dev, msg, sizeof(msg));
>>> if (ret)
>>> return ret;
>>>
>>> @@ -108,6 +251,24 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer,
>>> return count;
>>> }
>>>
>>> +static int cros_ec_chardev_release(struct inode *inode, struct file *filp)
>>> +{
>>> + struct chardev_priv *priv = filp->private_data;
>>> + struct cros_ec_dev *ec_dev = priv->ec_dev;
>>> + struct ec_event *event, *e;
>>> +
>>> + blocking_notifier_chain_unregister(&ec_dev->ec_dev->event_notifier,
>>> + &priv->notifier);
>>> +
>>> + list_for_each_entry_safe(event, e, &priv->events, node) {
>>> + list_del(&event->node);
>>> + kfree(event);
>>> + }
>>> + kfree(priv);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * Ioctls
>>> */
>>> @@ -181,7 +342,8 @@ static long cros_ec_chardev_ioctl_readmem(struct cros_ec_dev *ec,
>>> static long cros_ec_chardev_ioctl(struct file *filp, unsigned int cmd,
>>> unsigned long arg)
>>> {
>>> - struct cros_ec_dev *ec = filp->private_data;
>>> + struct chardev_priv *priv = filp->private_data;
>>> + struct cros_ec_dev *ec = priv->ec_dev;
>>>
>>> if (_IOC_TYPE(cmd) != CROS_EC_DEV_IOC)
>>> return -ENOTTY;
>>> @@ -191,6 +353,9 @@ static long cros_ec_chardev_ioctl(struct file *filp, unsigned int cmd,
>>> return cros_ec_chardev_ioctl_xcmd(ec, (void __user *)arg);
>>> case CROS_EC_DEV_IOCRDMEM:
>>> return cros_ec_chardev_ioctl_readmem(ec, (void __user *)arg);
>>> + case CROS_EC_DEV_IOCEVENTMASK:
>>> + priv->event_mask = arg;
>>> + return 0;
>>> }
>>>
>>> return -ENOTTY;
>>> @@ -198,7 +363,9 @@ static long cros_ec_chardev_ioctl(struct file *filp, unsigned int cmd,
>>>
>>> static const struct file_operations chardev_fops = {
>>> .open = cros_ec_chardev_open,
>>> + .poll = cros_ec_chardev_poll,
>>> .read = cros_ec_chardev_read,
>>> + .release = cros_ec_chardev_release,
>>> .unlocked_ioctl = cros_ec_chardev_ioctl,
>>> #ifdef CONFIG_COMPAT
>>> .compat_ioctl = cros_ec_chardev_ioctl,
>>> diff --git a/include/linux/platform_data/cros_ec_chardev.h b/include/linux/platform_data/cros_ec_chardev.h
>>> index 973b2615aa02..7de8faaf77df 100644
>>> --- a/include/linux/platform_data/cros_ec_chardev.h
>>> +++ b/include/linux/platform_data/cros_ec_chardev.h
>>> @@ -33,5 +33,6 @@ struct cros_ec_readmem {
>>> #define CROS_EC_DEV_IOC 0xEC
>>> #define CROS_EC_DEV_IOCXCMD _IOWR(CROS_EC_DEV_IOC, 0, struct cros_ec_command)
>>> #define CROS_EC_DEV_IOCRDMEM _IOWR(CROS_EC_DEV_IOC, 1, struct cros_ec_readmem)
>>> +#define CROS_EC_DEV_IOCEVENTMASK _IO(CROS_EC_DEV_IOC, 2)
>>>
>>> #endif /* _CROS_EC_DEV_H_ */
>>> --
>>> 2.20.1
>>>
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
>>
>

Next message: Qian Cai: ""arm64: use asm-generic/dma-mapping.h" introduced a kbuild warning"
Previous message: Enric Balletbo i Serra: "Re: [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host command when probe failed."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]