Re: [PATCH i2c/slave-mqueue v5] i2c: slave-mqueue: add a slave backend to receive and queue messages

From: Eduardo Valentin
Date: Fri May 24 2019 - 13:37:09 EST

Hey,

On Fri, May 24, 2019 at 10:43:16AM +0800, Wang, Haiyue wrote:
> Thanks for interest, the design idea is from:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/i2c-slave-eeprom.c?h=v5.2-rc1
>
> and
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/i2c/slave-interface
>
> Then you will get the answer. ;-)

Well, maybe :-) see further comments inline..

>
> BR,
>
> Haiyue
>
>
> å 2019-05-24 06:03, Eduardo Valentin åé:
> >Hey Wang,
> >
> >On Tue, Apr 24, 2018 at 01:06:32AM +0800, Haiyue Wang wrote:
> >>Some protocols over I2C are designed for bi-directional transferring
> >>messages by using I2C Master Write protocol. Like the MCTP (Management
> >>Component Transport Protocol) and IPMB (Intelligent Platform Management
> >>Bus), they both require that the userspace can receive messages from
> >>I2C dirvers under slave mode.
> >>
> >>This new slave mqueue backend is used to receive and queue messages, it
> >>will exposes these messages to userspace by sysfs bin file.
> >>
> >>Signed-off-by: Haiyue Wang <haiyue.wang@xxxxxxxxxxxxxxx>
> >>---
> >>v4 -> v5:
> >> - Typo: bellowing -> the below
> >>
> >>v3 -> v4:
> >> - Drop the small message after receiving I2C STOP.
> >>
> >>v2 -> v3:
> >> - Just remove the ';' after the end '}' of i2c_slave_mqueue_probe().
> >>
> >>v1 -> v2:
> >> - Change MQ_MSGBUF_SIZE and MQ_QUEUE_SIZE to be configurable by Kconfig.
> >>---
> >> Documentation/i2c/slave-mqueue-backend.rst | 125 ++++++++++++++++++
> >> drivers/i2c/Kconfig | 25 ++++
> >> drivers/i2c/Makefile | 1 +
> >> drivers/i2c/i2c-slave-mqueue.c | 203 +++++++++++++++++++++++++++++
> >> 4 files changed, 354 insertions(+)
> >> create mode 100644 Documentation/i2c/slave-mqueue-backend.rst
> >> create mode 100644 drivers/i2c/i2c-slave-mqueue.c
> >>
> >>diff --git a/Documentation/i2c/slave-mqueue-backend.rst b/Documentation/i2c/slave-mqueue-backend.rst
> >>new file mode 100644
> >>index 0000000..3966cf0
> >>--- /dev/null
> >>+++ b/Documentation/i2c/slave-mqueue-backend.rst
> >>@@ -0,0 +1,125 @@
> >>+.. SPDX-License-Identifier: GPL-2.0
> >>+
> >>+=====================================
> >>+Linux I2C slave message queue backend
> >>+=====================================
> >>+
> >>+:Author: Haiyue Wang <haiyue.wang@xxxxxxxxxxxxxxx>
> >>+
> >>+Some protocols over I2C/SMBus are designed for bi-directional transferring
> >>+messages by using I2C Master Write protocol. This requires that both sides
> >>+of the communication have slave addresses.
> >>+
> >>+Like MCTP (Management Component Transport Protocol) and IPMB (Intelligent
> >>+Platform Management Bus), they both require that the userspace can receive
> >>+messages from i2c dirvers under slave mode.
> >>+
> >>+This I2C slave mqueue (message queue) backend is used to receive and queue
> >>+messages from the remote i2c intelligent device; and it will add the target
> >>+slave address (with R/W# bit is always 0) into the message at the first byte,
> >>+so that userspace can use this byte to dispatch the messages into different
> >>+handling modules. Also, like IPMB, the address byte is in its message format,
> >>+it needs it to do checksum.
> >>+
> >>+For messages are time related, so this backend will flush the oldest message
> >>+to queue the newest one.
> >>+
> >>+Link
> >>+----
> >>+`Intelligent Platform Management Bus
> >>+Communications Protocol Specification
> >>+<https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/ipmp-spec-v1.0.pdf>`_
> >>+
> >>+`Management Component Transport Protocol (MCTP)
> >>+SMBus/I2C Transport Binding Specification
> >>+<https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf>`_
> >>+
> >>+How to use
> >>+----------
> >>+For example, the I2C5 bus has slave address 0x10, the below command will create
> >>+the related message queue interface:
> >>+
> >>+ echo slave-mqueue 0x1010 > /sys/bus/i2c/devices/i2c-5/new_device
> >>+
> >>+Then you can dump the messages like this:
> >>+
> >>+ hexdump -C /sys/bus/i2c/devices/5-1010/slave-mqueue
> >>+
> >>+Code Example
> >>+------------
> >>+*Note: call 'lseek' before 'read', this is a requirement from kernfs' design.*
> >>+
> >>+::
> >>+
> >>+ #include <sys/types.h>
> >>+ #include <sys/stat.h>
> >>+ #include <unistd.h>
> >>+ #include <poll.h>
> >>+ #include <time.h>
> >>+ #include <fcntl.h>
> >>+ #include <stdio.h>
> >>+
> >>+ int main(int argc, char *argv[])
> >>+ {
> >>+ int i, r;
> >>+ struct pollfd pfd;
> >>+ struct timespec ts;
> >>+ unsigned char data[256];
> >>+
> >>+ pfd.fd = open(argv[1], O_RDONLY | O_NONBLOCK);
> >>+ if (pfd.fd < 0)
> >>+ return -1;
> >>+
> >>+ pfd.events = POLLPRI;
> >>+
> >>+ while (1) {
> >>+ r = poll(&pfd, 1, 5000);
> >>+
> >>+ if (r < 0)
> >>+ break;
> >>+
> >>+ if (r == 0 || !(pfd.revents & POLLPRI))
> >>+ continue;
> >>+
> >>+ lseek(pfd.fd, 0, SEEK_SET);
> >>+ r = read(pfd.fd, data, sizeof(data));
> >>+ if (r <= 0)
> >>+ continue;
> >>+
> >>+ clock_gettime(CLOCK_MONOTONIC, &ts);
> >>+ printf("[%ld.%.9ld] :", ts.tv_sec, ts.tv_nsec);
> >>+ for (i = 0; i < r; i++)
> >>+ printf(" %02x", data[i]);
> >>+ printf("\n");
> >>+ }
> >>+
> >>+ close(pfd.fd);
> >>+
> >>+ return 0;
> >>+ }
> >>+
> >>+Result
> >>+------
> >>+*./a.out "/sys/bus/i2c/devices/5-1010/slave-mqueue"*
> >>+
> >>+::
> >>+
> >>+ [10183.232500449] : 20 18 c8 2c 78 01 5b
> >>+ [10183.479358348] : 20 18 c8 2c 78 01 5b
> >>+ [10183.726556812] : 20 18 c8 2c 78 01 5b
> >>+ [10183.972605863] : 20 18 c8 2c 78 01 5b
> >>+ [10184.220124772] : 20 18 c8 2c 78 01 5b
> >>+ [10184.467764166] : 20 18 c8 2c 78 01 5b
> >>+ [10193.233421784] : 20 18 c8 2c 7c 01 57
> >>+ [10193.480273460] : 20 18 c8 2c 7c 01 57
> >>+ [10193.726788733] : 20 18 c8 2c 7c 01 57
> >>+ [10193.972781945] : 20 18 c8 2c 7c 01 57
> >>+ [10194.220487360] : 20 18 c8 2c 7c 01 57
> >>+ [10194.468089259] : 20 18 c8 2c 7c 01 57
> >>+ [10203.233433099] : 20 18 c8 2c 80 01 53
> >>+ [10203.481058715] : 20 18 c8 2c 80 01 53
> >>+ [10203.727610472] : 20 18 c8 2c 80 01 53
> >>+ [10203.974044856] : 20 18 c8 2c 80 01 53
> >>+ [10204.220734634] : 20 18 c8 2c 80 01 53
> >>+ [10204.468461664] : 20 18 c8 2c 80 01 53
> >>+
> >>diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> >>index efc3354..31e57d2 100644
> >>--- a/drivers/i2c/Kconfig
> >>+++ b/drivers/i2c/Kconfig
> >>@@ -118,6 +118,31 @@ if I2C_SLAVE
> >> config I2C_SLAVE_EEPROM
> >> tristate "I2C eeprom slave driver"
> >>+config I2C_SLAVE_MQUEUE_MESSAGE_SIZE
> >>+ int "The message size of I2C mqueue slave"
> >>+ default 120
> >>+
> >>+config I2C_SLAVE_MQUEUE_QUEUE_SIZE
> >>+ int "The queue size of I2C mqueue slave"
> >>+ default 32
> >>+ help
> >>+ This number MUST be power of 2.
> >>+
> >>+config I2C_SLAVE_MQUEUE
> >>+ tristate "I2C mqueue (message queue) slave driver"
> >>+ select I2C_SLAVE_MQUEUE_MESSAGE_SIZE
> >>+ select I2C_SLAVE_MQUEUE_QUEUE_SIZE
> >>+ help
> >>+ Some protocols over I2C are designed for bi-directional transferring
> >>+ messages by using I2C Master Write protocol. This driver is used to
> >>+ receive and queue messages from the remote I2C device.
> >>+
> >>+ Userspace can get the messages by reading sysfs file that this driver
> >>+ exposes.
> >>+
> >>+ This support is also available as a module. If so, the module will be
> >>+ called i2c-slave-mqueue.
> >>+
> >> endif
> >> config I2C_DEBUG_CORE
> >>diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> >>index 72c94c6..7ec287b 100644
> >>--- a/drivers/i2c/Makefile
> >>+++ b/drivers/i2c/Makefile
> >>@@ -16,6 +16,7 @@ obj-$(CONFIG_I2C_MUX) += i2c-mux.o
> >> obj-y += algos/ busses/ muxes/
> >> obj-$(CONFIG_I2C_STUB) += i2c-stub.o
> >> obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o
> >>+obj-$(CONFIG_I2C_SLAVE_MQUEUE) += i2c-slave-mqueue.o
> >> ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
> >> CFLAGS_i2c-core-base.o := -Wno-deprecated-declarations
> >>diff --git a/drivers/i2c/i2c-slave-mqueue.c b/drivers/i2c/i2c-slave-mqueue.c
> >>new file mode 100644
> >>index 0000000..424f435
> >>--- /dev/null
> >>+++ b/drivers/i2c/i2c-slave-mqueue.c
> >>@@ -0,0 +1,203 @@
> >>+// SPDX-License-Identifier: GPL-2.0
> >>+// Copyright (c) 2017 - 2018, Intel Corporation.
> >>+
> >>+#include <linux/i2c.h>
> >>+#include <linux/kernel.h>
> >>+#include <linux/module.h>
> >>+#include <linux/of.h>
> >>+#include <linux/slab.h>
> >>+#include <linux/spinlock.h>
> >>+#include <linux/sysfs.h>
> >>+
> >>+#define MQ_MSGBUF_SIZE CONFIG_I2C_SLAVE_MQUEUE_MESSAGE_SIZE
> >>+#define MQ_QUEUE_SIZE CONFIG_I2C_SLAVE_MQUEUE_QUEUE_SIZE
> >>+#define MQ_QUEUE_NEXT(x) (((x) + 1) & (MQ_QUEUE_SIZE - 1))
> >>+
> >>+struct mq_msg {
> >>+ int len;
> >>+ u8 *buf;
> >>+};
> >>+
> >>+struct mq_queue {
> >>+ struct bin_attribute bin;
> >>+ struct kernfs_node *kn;
> >>+
> >>+ spinlock_t lock; /* spinlock for queue index handling */
> >I wonder why you decided to lock only in/out accesses and not the mq_queue struct.
> >
> >>+ int in;
> >>+ int out;
> >>+
> >>+ struct mq_msg *curr;
> >>+ int truncated; /* drop current if truncated */
> >>+ struct mq_msg queue[MQ_QUEUE_SIZE];
> >>+};
> >>+
> >>+static int i2c_slave_mqueue_callback(struct i2c_client *client,
> >>+ enum i2c_slave_event event, u8 *val)
> >>+{
> >>+ struct mq_queue *mq = i2c_get_clientdata(client);
> >>+ struct mq_msg *msg = mq->curr;
> >>+ int ret = 0;
> >>+
> >>+ switch (event) {
> >>+ case I2C_SLAVE_WRITE_REQUESTED:
> >>+ mq->truncated = 0;
> >>+
> >>+ msg->len = 1;
> >>+ msg->buf[0] = client->addr << 1;
> >>+ break;
> >>+
> >>+ case I2C_SLAVE_WRITE_RECEIVED:
> >>+ if (msg->len < MQ_MSGBUF_SIZE) {
> >>+ msg->buf[msg->len++] = *val;
> >Do we need to lock the accesses to msg->buf? how about to msg->len?


this code goes access and modify data here, e.g. msg->len and msg->buf.

On this case (I2C_SLAVE_WRITE_RECEIVED), this code wont protect access.

This can cause concurrence issues if you receive an IRQ when the user
is on your bin_read().

> >
> >>+ } else {
> >>+ dev_err(&client->dev, "message is truncated!\n");
> >>+ mq->truncated = 1;
> >>+ ret = -EINVAL;
> >>+ }
> >>+ break;
> >>+
> >>+ case I2C_SLAVE_STOP:
> >>+ if (unlikely(mq->truncated || msg->len < 2))
> >>+ break;
> >>+
> >>+ spin_lock(&mq->lock);
> >>+ mq->in = MQ_QUEUE_NEXT(mq->in);
> >>+ mq->curr = &mq->queue[mq->in];
> >>+ mq->curr->len = 0;
> >>+
> >>+ /* Flush the oldest message */
> >>+ if (mq->out == mq->in)
> >>+ mq->out = MQ_QUEUE_NEXT(mq->out);
> >>+ spin_unlock(&mq->lock);
> >>+

Here you protect most of it, but you still access msg->len for read.. with no protection.

> >>+ kernfs_notify(mq->kn);
> >>+ break;
> >>+
> >>+ default:
> >>+ *val = 0xFF;
> >>+ break;
> >>+ }
> >>+
> >>+ return ret;
> >>+}
> >>+
> >>+static ssize_t i2c_slave_mqueue_bin_read(struct file *filp,
> >>+ struct kobject *kobj,
> >>+ struct bin_attribute *attr,
> >>+ char *buf, loff_t pos, size_t count)
> >>+{
> >>+ struct mq_queue *mq;
> >>+ struct mq_msg *msg;
> >>+ unsigned long flags;
> >>+ bool more = false;
> >>+ ssize_t ret = 0;
> >>+
> >>+ mq = dev_get_drvdata(container_of(kobj, struct device, kobj));
> >>+
> >>+ spin_lock_irqsave(&mq->lock, flags);
> >>+ if (mq->out != mq->in) {
> >>+ msg = &mq->queue[mq->out];
> >>+
> >>+ if (msg->len <= count) {
> >>+ ret = msg->len;
> >>+ memcpy(buf, msg->buf, ret);
> >Is buf a userspace pointer? should it be a copy_to_user() here?
> >
> >>+ } else {
> >>+ ret = -EOVERFLOW; /* Drop this HUGE one. */
> >>+ }
> >>+
> >>+ mq->out = MQ_QUEUE_NEXT(mq->out);
> >>+ if (mq->out != mq->in)
> >>+ more = true;
> >>+ }
> >>+ spin_unlock_irqrestore(&mq->lock, flags);
> >>+
> >>+ if (more)
> >>+ kernfs_notify(mq->kn);
> >>+
> >>+ return ret;
> >>+}
> >>+
> >>+static int i2c_slave_mqueue_probe(struct i2c_client *client,
> >>+ const struct i2c_device_id *id)
> >>+{
> >>+ struct device *dev = &client->dev;
> >>+ struct mq_queue *mq;
> >>+ int ret, i;
> >>+ void *buf;
> >>+
> >>+ mq = devm_kzalloc(dev, sizeof(*mq), GFP_KERNEL);
> >>+ if (!mq)
> >>+ return -ENOMEM;
> >>+
> >>+ BUILD_BUG_ON(!is_power_of_2(MQ_QUEUE_SIZE));
> >>+
> >>+ buf = devm_kmalloc_array(dev, MQ_QUEUE_SIZE, MQ_MSGBUF_SIZE,
> >>+ GFP_KERNEL);
> >>+ if (!buf)
> >>+ return -ENOMEM;
> >>+
> >>+ for (i = 0; i < MQ_QUEUE_SIZE; i++)
> >>+ mq->queue[i].buf = buf + i * MQ_MSGBUF_SIZE;
> >>+
> >>+ i2c_set_clientdata(client, mq);
> >>+
> >>+ spin_lock_init(&mq->lock);
> >>+ mq->curr = &mq->queue[0];
> >>+
> >>+ sysfs_bin_attr_init(&mq->bin);
> >>+ mq->bin.attr.name = "slave-mqueue";
> >>+ mq->bin.attr.mode = 0400;
> >>+ mq->bin.read = i2c_slave_mqueue_bin_read;
> >>+ mq->bin.size = MQ_MSGBUF_SIZE * MQ_QUEUE_SIZE;
> >>+
> >>+ ret = sysfs_create_bin_file(&dev->kobj, &mq->bin);
> >>+ if (ret)
> >>+ return ret;
> >>+
> >>+ mq->kn = kernfs_find_and_get(dev->kobj.sd, mq->bin.attr.name);
> >>+ if (!mq->kn) {
> >>+ sysfs_remove_bin_file(&dev->kobj, &mq->bin);
> >>+ return -EFAULT;
> >>+ }
> >>+
> >>+ ret = i2c_slave_register(client, i2c_slave_mqueue_callback);
> >>+ if (ret) {
> >>+ kernfs_put(mq->kn);
> >>+ sysfs_remove_bin_file(&dev->kobj, &mq->bin);
> >>+ return ret;
> >>+ }
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static int i2c_slave_mqueue_remove(struct i2c_client *client)
> >>+{
> >>+ struct mq_queue *mq = i2c_get_clientdata(client);
> >>+
> >>+ i2c_slave_unregister(client);
> >>+
> >>+ kernfs_put(mq->kn);
> >>+ sysfs_remove_bin_file(&client->dev.kobj, &mq->bin);
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static const struct i2c_device_id i2c_slave_mqueue_id[] = {
> >>+ { "slave-mqueue", 0 },
> >>+ { }
> >>+};
> >>+MODULE_DEVICE_TABLE(i2c, i2c_slave_mqueue_id);
> >>+
> >>+static struct i2c_driver i2c_slave_mqueue_driver = {
> >>+ .driver = {
> >>+ .name = "i2c-slave-mqueue",
> >>+ },
> >>+ .probe = i2c_slave_mqueue_probe,
> >>+ .remove = i2c_slave_mqueue_remove,
> >>+ .id_table = i2c_slave_mqueue_id,
> >>+};
> >>+module_i2c_driver(i2c_slave_mqueue_driver);
> >>+
> >>+MODULE_LICENSE("GPL v2");
> >>+MODULE_AUTHOR("Haiyue Wang <haiyue.wang@xxxxxxxxxxxxxxx>");
> >>+MODULE_DESCRIPTION("I2C slave mode for receiving and queuing messages");
> >>--
> >>2.7.4
> >>

--
All the best,
Eduardo Valentin