Re: [PATCHv7 2/3] i2c: slave-mqueue: add a slave backend to receive and queue messages

2019-07-01 Thread Wolfram Sang 
Hi Eduardo,

thanks for stepping up and pushing this further!

On Wed, Jun 05, 2019 at 09:46:50AM -0700, Eduardo Valentin wrote:
> From: Haiyue Wang 
> 
> 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.

So, this is a read-only bin file. Sending is done via the standard
i2c-dev driver? Or not needed at all?

Regarding, IPMB, how does this related to the recently merged IPMB slave
driver?

http://patchwork.ozlabs.org/patch/1113278/

> 
> Note: DT interface and a couple of minor fixes here and there
> by Eduardo, so I kept the original authorship here.
> 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> Cc: Wolfram Sang 
> Cc: Andy Shevchenko 
> Cc: linux-...@vger.kernel.org
> Cc: devicet...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Haiyue Wang 
> Signed-off-by: Eduardo Valentin 

> +I2C SLAVE MQUEUE DRIVER
> +M:   Eduardo Valentin 

Cool, thanks!

> +config I2C_SLAVE_MQUEUE_MESSAGE_SIZE
> + int "The message size of I2C mqueue slave"
> + depends on I2C_SLAVE_MQUEUE
> + default 120
> +
> +config I2C_SLAVE_MQUEUE_QUEUE_SIZE
> + int "The queue size of I2C mqueue slave"
> + depends on I2C_SLAVE_MQUEUE
> + default 32
> + help
> +   This number MUST be power of 2.

I am not happy with this being a Kconfig option. Best would be a
per-instance configuration, so we could have differently sized mqueues
at runtime. I could think of another sysfs-file like
"mqueue-slave-config" which would appear after writing to 'new_device'.
And only after writing to 'mqueue-slave-config', the bin file to read
from would show up. But it is just a quick brainstorming, maybe you have
a better idea?

> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017 - 2018, Intel Corporation.

A short description what this driver does would be nice.

Rest looks decent from a glimpse. I haven't looked into the gory details
yet, though, because I want to get the high level things straight first.

Kind regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCHv7 2/3] i2c: slave-mqueue: add a slave backend to receive and queue messages

2019-06-05 Thread Eduardo Valentin 
On Wed, Jun 05, 2019 at 08:02:11PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 5, 2019 at 7:48 PM Eduardo Valentin  wrote:
> >
> > From: Haiyue Wang 
> >
> > 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.
> >
> > Note: DT interface and a couple of minor fixes here and there
> > by Eduardo, so I kept the original authorship here.
> 
> FWIW,
> Reviewed-by: Andy Shevchenko 

Great!

> 
> though I prefer simple dropping of of_match_ptr(), and by the way,
> #include  is superfluous and may be changed to
> mod_devicetable.h.
> 
> Leave above to Wolfram to judge.

Wolfram, any objections on this series?


> 
> > Cc: Rob Herring 
> > Cc: Mark Rutland 
> > Cc: Wolfram Sang 
> > Cc: Andy Shevchenko 
> > Cc: linux-...@vger.kernel.org
> > Cc: devicet...@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Haiyue Wang 
> > Signed-off-by: Eduardo Valentin 
> > ---
> >
> > From V6 -> V7:
> > - fixed compile warm when CONFIG_OF=n by wrapping of table into ifdef
> > - minor changes: kobj_to_dev(), moved BUILD_BUG_ON() to start of function,
> > and flagged DT table sentinel in a more strict way.
> > - Also added a MODULE_DEVICE_TABLE() for the of table.
> >
> >  Documentation/i2c/slave-mqueue-backend.rst | 124 
> >  MAINTAINERS|   8 +
> >  drivers/i2c/Kconfig|  25 +++
> >  drivers/i2c/Makefile   |   1 +
> >  drivers/i2c/i2c-slave-mqueue.c | 215 +
> >  5 files changed, 373 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 ..376dff998fa3
> > --- /dev/null
> > +++ b/Documentation/i2c/slave-mqueue-backend.rst
> > @@ -0,0 +1,124 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +=
> > +Linux I2C slave message queue backend
> > +=
> > +
> > +:Author: Haiyue Wang 
> > +
> > +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 drivers 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
> > +`_
> > +
> > +`Management Component Transport Protocol (MCTP)
> > +SMBus/I2C Transport Binding Specification
> > +`_
> > +
> > +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 
> > +  #include 
> > +  #include 
> > +  #include 
> > +  #include 
> > +  #include 
> > +  #include 
> > +
> > +  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;
> > +
> > +

Re: [PATCHv7 2/3] i2c: slave-mqueue: add a slave backend to receive and queue messages

2019-06-05 Thread Andy Shevchenko 
On Wed, Jun 5, 2019 at 7:48 PM Eduardo Valentin  wrote:
>
> From: Haiyue Wang 
>
> 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.
>
> Note: DT interface and a couple of minor fixes here and there
> by Eduardo, so I kept the original authorship here.

FWIW,
Reviewed-by: Andy Shevchenko 

though I prefer simple dropping of of_match_ptr(), and by the way,
#include  is superfluous and may be changed to
mod_devicetable.h.

Leave above to Wolfram to judge.

> Cc: Rob Herring 
> Cc: Mark Rutland 
> Cc: Wolfram Sang 
> Cc: Andy Shevchenko 
> Cc: linux-...@vger.kernel.org
> Cc: devicet...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Haiyue Wang 
> Signed-off-by: Eduardo Valentin 
> ---
>
> From V6 -> V7:
> - fixed compile warm when CONFIG_OF=n by wrapping of table into ifdef
> - minor changes: kobj_to_dev(), moved BUILD_BUG_ON() to start of function,
> and flagged DT table sentinel in a more strict way.
> - Also added a MODULE_DEVICE_TABLE() for the of table.
>
>  Documentation/i2c/slave-mqueue-backend.rst | 124 
>  MAINTAINERS|   8 +
>  drivers/i2c/Kconfig|  25 +++
>  drivers/i2c/Makefile   |   1 +
>  drivers/i2c/i2c-slave-mqueue.c | 215 +
>  5 files changed, 373 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 ..376dff998fa3
> --- /dev/null
> +++ b/Documentation/i2c/slave-mqueue-backend.rst
> @@ -0,0 +1,124 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=
> +Linux I2C slave message queue backend
> +=
> +
> +:Author: Haiyue Wang 
> +
> +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 drivers 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
> +`_
> +
> +`Management Component Transport Protocol (MCTP)
> +SMBus/I2C Transport Binding Specification
> +`_
> +
> +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 
> +  #include 
> +  #include 
> +  #include 
> +  #include 
> +  #include 
> +  #include 
> +
> +  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(, 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)
> +

[PATCHv7 2/3] i2c: slave-mqueue: add a slave backend to receive and queue messages

2019-06-05 Thread Eduardo Valentin 
From: Haiyue Wang 

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.

Note: DT interface and a couple of minor fixes here and there
by Eduardo, so I kept the original authorship here.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: Wolfram Sang 
Cc: Andy Shevchenko 
Cc: linux-...@vger.kernel.org
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Haiyue Wang 
Signed-off-by: Eduardo Valentin 
---

>From V6 -> V7:
- fixed compile warm when CONFIG_OF=n by wrapping of table into ifdef
- minor changes: kobj_to_dev(), moved BUILD_BUG_ON() to start of function,
and flagged DT table sentinel in a more strict way.
- Also added a MODULE_DEVICE_TABLE() for the of table.

 Documentation/i2c/slave-mqueue-backend.rst | 124 
 MAINTAINERS|   8 +
 drivers/i2c/Kconfig|  25 +++
 drivers/i2c/Makefile   |   1 +
 drivers/i2c/i2c-slave-mqueue.c | 215 +
 5 files changed, 373 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 ..376dff998fa3
--- /dev/null
+++ b/Documentation/i2c/slave-mqueue-backend.rst
@@ -0,0 +1,124 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=
+Linux I2C slave message queue backend
+=
+
+:Author: Haiyue Wang 
+
+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 drivers 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
+`_
+
+`Management Component Transport Protocol (MCTP)
+SMBus/I2C Transport Binding Specification
+`_
+
+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 
+  #include 
+  #include 
+  #include 
+  #include 
+  #include 
+  #include 
+
+  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(, 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, );
+  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