[PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

So, after revisiting old mail threads, taking part in a similar discussion on
the USB list, and implementing a not-convincing solution before, here is what I
cooked up to document and ease DMA handling for I2C within Linux. Please have a
look at the documentation introduced in patch 7 for details. And to make it
clear again: The stuff below is opt-in. If host drivers are not updated, they
will continue to work like before.

While previous versions until v3 tried to magically apply bounce buffers when
needed, it became clear that detecting DMA safe buffers is too fragile. This
approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
outcome so far is very convincing IMO. The core additions are simple and easy
to understand. The driver changes for the Renesas IP cores became easy to
understand, too.

Of course, we must now whitelist DMA safe buffers. This series implements it
for core functionality:

a) for the I2C_RDWR when messages come from userspace
b) for i2c_smbus_xfer_emulated(), DMA safe buffers are now allocated for
   block transfers
c) i2c_master_{send|recv} have now a *_dmasafe variant

I am still not sure how we can teach regmap this new flag. regmap is a heavy
user of I2C, so broonie's opinion here would be great to have. The rest should
mostly be updating individual drivers which can be done when needed.

All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W) and
Renesas Lager board (r8a7790/H2). But more testing is really really welcome.

The branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma-v6

It is planned to land upstream in v4.16 and I want to push it to linux-next
early after v4.15 to get lots of testing for the core changes.

Big kudos to Renesas Electronics for funding this work, thank you very much!

Regards,

   Wolfram

Change since V5:
* i2c_master_{send|recv} have now a *_dmasafe variant
* for i2c_smbus_xfer_emulated(), DMA safe buffers are now allocated for
  block transfers
* updated the documentation
* merged some rewording suggestions from Jonathan Cameron (thanks!)
* rebased the patches v4.14-rc6+i2c/for-next, reordered patches


Wolfram Sang (9):
  i2c: add a message flag for DMA safe buffers
  i2c: add helpers to ease DMA handling
  i2c: dev: mark RDWR buffers as DMA_SAFE
  i2c: refactor i2c_master_{send_recv}
  i2c: add i2c_master_{send|recv}_dmasafe
  i2c: smbus: use DMA safe buffers for emulated SMBus transactions
  i2c: add docs to clarify DMA handling
  i2c: sh_mobile: use core helper to decide when to use DMA
  i2c: rcar: skip DMA if buffer is not safe

 Documentation/i2c/DMA-considerations |  67 +++++++++++++++++++++
 drivers/i2c/busses/i2c-rcar.c        |   2 +-
 drivers/i2c/busses/i2c-sh_mobile.c   |   8 ++-
 drivers/i2c/i2c-core-base.c          | 110 ++++++++++++++++++++---------------
 drivers/i2c/i2c-core-smbus.c         |  45 ++++++++++++--
 drivers/i2c/i2c-dev.c                |   2 +
 include/linux/i2c.h                  |  68 ++++++++++++++++++++--
 include/uapi/linux/i2c.h             |   3 +
 8 files changed, 246 insertions(+), 59 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

So, after revisiting old mail threads, taking part in a similar discussion on
the USB list, and implementing a not-convincing solution before, here is what I
cooked up to document and ease DMA handling for I2C within Linux. Please have a
look at the documentation introduced in patch 7 for details. And to make it
clear again: The stuff below is opt-in. If host drivers are not updated, they
will continue to work like before.

While previous versions until v3 tried to magically apply bounce buffers when
needed, it became clear that detecting DMA safe buffers is too fragile. This
approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
outcome so far is very convincing IMO. The core additions are simple and easy
to understand. The driver changes for the Renesas IP cores became easy to
understand, too.

Of course, we must now whitelist DMA safe buffers. This series implements it
for core functionality:

a) for the I2C_RDWR when messages come from userspace
b) for i2c_smbus_xfer_emulated(), DMA safe buffers are now allocated for
   block transfers
c) i2c_master_{send|recv} have now a *_dmasafe variant

I am still not sure how we can teach regmap this new flag. regmap is a heavy
user of I2C, so broonie's opinion here would be great to have. The rest should
mostly be updating individual drivers which can be done when needed.

All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W) and
Renesas Lager board (r8a7790/H2). But more testing is really really welcome.

The branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma-v6

It is planned to land upstream in v4.16 and I want to push it to linux-next
early after v4.15 to get lots of testing for the core changes.

Big kudos to Renesas Electronics for funding this work, thank you very much!

Regards,

   Wolfram

Change since V5:
* i2c_master_{send|recv} have now a *_dmasafe variant
* for i2c_smbus_xfer_emulated(), DMA safe buffers are now allocated for
  block transfers
* updated the documentation
* merged some rewording suggestions from Jonathan Cameron (thanks!)
* rebased the patches v4.14-rc6+i2c/for-next, reordered patches


Wolfram Sang (9):
  i2c: add a message flag for DMA safe buffers
  i2c: add helpers to ease DMA handling
  i2c: dev: mark RDWR buffers as DMA_SAFE
  i2c: refactor i2c_master_{send_recv}
  i2c: add i2c_master_{send|recv}_dmasafe
  i2c: smbus: use DMA safe buffers for emulated SMBus transactions
  i2c: add docs to clarify DMA handling
  i2c: sh_mobile: use core helper to decide when to use DMA
  i2c: rcar: skip DMA if buffer is not safe

 Documentation/i2c/DMA-considerations |  67 +++++++++++++++++++++
 drivers/i2c/busses/i2c-rcar.c        |   2 +-
 drivers/i2c/busses/i2c-sh_mobile.c   |   8 ++-
 drivers/i2c/i2c-core-base.c          | 110 ++++++++++++++++++++---------------
 drivers/i2c/i2c-core-smbus.c         |  45 ++++++++++++--
 drivers/i2c/i2c-dev.c                |   2 +
 include/linux/i2c.h                  |  68 ++++++++++++++++++++--
 include/uapi/linux/i2c.h             |   3 +
 8 files changed, 246 insertions(+), 59 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[PATCH v6 1/9] i2c: add a message flag for DMA safe buffers

[Wolfram Sang]

I2C has no requirement that the buffer of a message needs to be DMA
safe. In case it is, it can now be flagged, so drivers wishing to
do DMA can use the buffer directly.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/uapi/linux/i2c.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 009e27bb9abe19..1c683cb319e4b7 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -71,6 +71,9 @@ struct i2c_msg {
 #define I2C_M_RD		0x0001	/* read data, from slave to master */
 					/* I2C_M_RD is guaranteed to be 0x0001! */
 #define I2C_M_TEN		0x0010	/* this is a ten bit chip address */
+#define I2C_M_DMA_SAFE		0x0200	/* the buffer of this message is DMA safe */
+					/* makes only sense in kernelspace */
+					/* userspace buffers are copied anyway */
 #define I2C_M_RECV_LEN		0x0400	/* length will be first received byte */
 #define I2C_M_NO_RD_ACK		0x0800	/* if I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_IGNORE_NAK	0x1000	/* if I2C_FUNC_PROTOCOL_MANGLING */
-- 
2.11.0

[PATCH v6 2/9] i2c: add helpers to ease DMA handling

[Wolfram Sang]

One helper checks if DMA is suitable and optionally creates a bounce
buffer, if not. The other function returns the bounce buffer and makes
sure the data is properly copied back to the message.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  3 +++
 2 files changed, 49 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 706164b4c5be46..de1850bd440659 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2261,6 +2261,52 @@ void i2c_put_adapter(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL(i2c_put_adapter);
 
+/**
+ * i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg
+ * @msg: the message to be checked
+ * @threshold: the minimum number of bytes for which using DMA makes sense
+ *
+ * Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with PIO.
+ *	   Or a valid pointer to be used with DMA. After use, release it by
+ *	   calling i2c_release_dma_safe_msg_buf().
+ *
+ * This function must only be called from process context!
+ */
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold)
+{
+	if (msg->len < threshold)
+		return NULL;
+
+	if (msg->flags & I2C_M_DMA_SAFE)
+		return msg->buf;
+
+	pr_debug("using bounce buffer for addr=0x%02x, len=%d\n",
+		 msg->addr, msg->len);
+
+	if (msg->flags & I2C_M_RD)
+		return kzalloc(msg->len, GFP_KERNEL);
+	else
+		return kmemdup(msg->buf, msg->len, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(i2c_get_dma_safe_msg_buf);
+
+/**
+ * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync with i2c_msg
+ * @msg: the message to be synced with
+ * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
+ */
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
+{
+	if (!buf || buf == msg->buf)
+		return;
+
+	if (msg->flags & I2C_M_RD)
+		memcpy(msg->buf, buf, msg->len);
+
+	kfree(buf);
+}
+EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
+
 MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>");
 MODULE_DESCRIPTION("I2C-Bus main module");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0f774406fad0e4..a0b57de91e21d3 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -769,6 +769,9 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
 	return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
 }
 
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
+
 int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
 /**
  * module_i2c_driver() - Helper macro for registering a modular I2C driver
-- 
2.11.0

[PATCH v6 2/9] i2c: add helpers to ease DMA handling

[Wolfram Sang]

One helper checks if DMA is suitable and optionally creates a bounce
buffer, if not. The other function returns the bounce buffer and makes
sure the data is properly copied back to the message.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 drivers/i2c/i2c-core-base.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  3 +++
 2 files changed, 49 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 706164b4c5be46..de1850bd440659 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2261,6 +2261,52 @@ void i2c_put_adapter(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL(i2c_put_adapter);
 
+/**
+ * i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg
+ * @msg: the message to be checked
+ * @threshold: the minimum number of bytes for which using DMA makes sense
+ *
+ * Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with PIO.
+ *	   Or a valid pointer to be used with DMA. After use, release it by
+ *	   calling i2c_release_dma_safe_msg_buf().
+ *
+ * This function must only be called from process context!
+ */
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold)
+{
+	if (msg->len < threshold)
+		return NULL;
+
+	if (msg->flags & I2C_M_DMA_SAFE)
+		return msg->buf;
+
+	pr_debug("using bounce buffer for addr=0x%02x, len=%d\n",
+		 msg->addr, msg->len);
+
+	if (msg->flags & I2C_M_RD)
+		return kzalloc(msg->len, GFP_KERNEL);
+	else
+		return kmemdup(msg->buf, msg->len, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(i2c_get_dma_safe_msg_buf);
+
+/**
+ * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync with i2c_msg
+ * @msg: the message to be synced with
+ * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
+ */
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
+{
+	if (!buf || buf == msg->buf)
+		return;
+
+	if (msg->flags & I2C_M_RD)
+		memcpy(msg->buf, buf, msg->len);
+
+	kfree(buf);
+}
+EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
+
 MODULE_AUTHOR("Simon G. Vogl <simon-nD9nYVNVf00W+b/DJNNodF6hYfS7NtTn@public.gmane.org>");
 MODULE_DESCRIPTION("I2C-Bus main module");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0f774406fad0e4..a0b57de91e21d3 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -769,6 +769,9 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
 	return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
 }
 
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
+
 int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
 /**
  * module_i2c_driver() - Helper macro for registering a modular I2C driver
-- 
2.11.0

[PATCH v6 3/9] i2c: dev: mark RDWR buffers as DMA_SAFE

[Wolfram Sang]

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 6f638bbc922db4..bbc7aadb4c899d 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -280,6 +280,8 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 			res = PTR_ERR(rdwr_pa[i].buf);
 			break;
 		}
+		/* memdup_user allocates with GFP_KERNEL, so DMA is ok */
+		rdwr_pa[i].flags |= I2C_M_DMA_SAFE;
 
 		/*
 		 * If the message length is received from the slave (similar
-- 
2.11.0

[PATCH v6 3/9] i2c: dev: mark RDWR buffers as DMA_SAFE

[Wolfram Sang]

Reviewed-by: Jonathan Cameron <Jonathan.Cameron-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 drivers/i2c/i2c-dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 6f638bbc922db4..bbc7aadb4c899d 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -280,6 +280,8 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 			res = PTR_ERR(rdwr_pa[i].buf);
 			break;
 		}
+		/* memdup_user allocates with GFP_KERNEL, so DMA is ok */
+		rdwr_pa[i].flags |= I2C_M_DMA_SAFE;
 
 		/*
 		 * If the message length is received from the slave (similar
-- 
2.11.0

[PATCH v6 4/9] i2c: refactor i2c_master_{send_recv}

[Wolfram Sang]

Those two functions are very similar, the only differences are that one
needs the I2C_M_RD flag for its message while the other one needs the
buffer casted to drop the const. Introduce a generic helper which
allows to specify the flags (also needed later for DMA safe variants of
these calls) and let the casting be done in the inlining fuctions which
are now calling the new helper function.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 64 +++++++++++++--------------------------------
 include/linux/i2c.h         | 34 +++++++++++++++++++++---
 2 files changed, 48 insertions(+), 50 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index de1850bd440659..206c47c85c98c5 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1972,63 +1972,35 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 EXPORT_SYMBOL(i2c_transfer);
 
 /**
- * i2c_master_send - issue a single I2C message in master transmit mode
+ * i2c_transfer_buffer_flags - issue a single I2C message transferring data
+ * 			       to/from a buffer
  * @client: Handle to slave device
- * @buf: Data that will be written to the slave
- * @count: How many bytes to write, must be less than 64k since msg.len is u16
+ * @buf: Where the data is stored
+ * @count: How many bytes to transfer, must be less than 64k since msg.len is u16
+ * @flags: The flags to be used for the message, e.g. I2C_M_RD for reads
  *
- * Returns negative errno, or else the number of bytes written.
+ * Returns negative errno, or else the number of bytes transferred.
  */
-int i2c_master_send(const struct i2c_client *client, const char *buf, int count)
+int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
+			      int count, u16 flags)
 {
 	int ret;
-	struct i2c_adapter *adap = client->adapter;
-	struct i2c_msg msg;
-
-	msg.addr = client->addr;
-	msg.flags = client->flags & I2C_M_TEN;
-	msg.len = count;
-	msg.buf = (char *)buf;
-
-	ret = i2c_transfer(adap, &msg, 1);
-
-	/*
-	 * If everything went ok (i.e. 1 msg transmitted), return #bytes
-	 * transmitted, else error code.
-	 */
-	return (ret == 1) ? count : ret;
-}
-EXPORT_SYMBOL(i2c_master_send);
-
-/**
- * i2c_master_recv - issue a single I2C message in master receive mode
- * @client: Handle to slave device
- * @buf: Where to store data read from slave
- * @count: How many bytes to read, must be less than 64k since msg.len is u16
- *
- * Returns negative errno, or else the number of bytes read.
- */
-int i2c_master_recv(const struct i2c_client *client, char *buf, int count)
-{
-	struct i2c_adapter *adap = client->adapter;
-	struct i2c_msg msg;
-	int ret;
-
-	msg.addr = client->addr;
-	msg.flags = client->flags & I2C_M_TEN;
-	msg.flags |= I2C_M_RD;
-	msg.len = count;
-	msg.buf = buf;
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.flags = flags | (client->flags & I2C_M_TEN),
+		.len = count,
+		.buf = buf,
+	};
 
-	ret = i2c_transfer(adap, &msg, 1);
+	ret = i2c_transfer(client->adapter, &msg, 1);
 
 	/*
-	 * If everything went ok (i.e. 1 msg received), return #bytes received,
-	 * else error code.
+	 * If everything went ok (i.e. 1 msg transferred), return #bytes
+	 * transferred, else error code.
 	 */
 	return (ret == 1) ? count : ret;
 }
-EXPORT_SYMBOL(i2c_master_recv);
+EXPORT_SYMBOL(i2c_transfer_buffer_flags);
 
 /* ----------------------------------------------------
  * the i2c address scanning function
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index a0b57de91e21d3..ef1a8791c1ae24 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -63,10 +63,36 @@ struct property_entry;
  * transmit an arbitrary number of messages without interruption.
  * @count must be be less than 64k since msg.len is u16.
  */
-extern int i2c_master_send(const struct i2c_client *client, const char *buf,
-			   int count);
-extern int i2c_master_recv(const struct i2c_client *client, char *buf,
-			   int count);
+extern int i2c_transfer_buffer_flags(const struct i2c_client *client,
+				     char *buf, int count, u16 flags);
+
+/**
+ * i2c_master_recv - issue a single I2C message in master receive mode
+ * @client: Handle to slave device
+ * @buf: Where to store data read from slave
+ * @count: How many bytes to read, must be less than 64k since msg.len is u16
+ *
+ * Returns negative errno, or else the number of bytes read.
+ */
+static inline int i2c_master_recv(const struct i2c_client *client,
+				  char *buf, int count)
+{
+	return i2c_transfer_buffer_flags(client, buf, count, I2C_M_RD);
+};
+
+/**
+ * i2c_master_send - issue a single I2C message in master transmit mode
+ * @client: Handle to slave device
+ * @buf: Data that will be written to the slave
+ * @count: How many bytes to write, must be less than 64k since msg.len is u16
+ *
+ * Returns negative errno, or else the number of bytes written.
+ */
+static inline int i2c_master_send(const struct i2c_client *client,
+				  const char *buf, int count)
+{
+	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
+};
 
 /* Transfer num messages.
  */
-- 
2.11.0

Re: [PATCH v6 4/9] i2c: refactor i2c_master_{send_recv}

[Jonathan Cameron]

On Sat, 4 Nov 2017 21:20:04 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> Those two functions are very similar, the only differences are that one
> needs the I2C_M_RD flag for its message while the other one needs the
> buffer casted to drop the const. Introduce a generic helper which
> allows to specify the flags (also needed later for DMA safe variants of
> these calls) and let the casting be done in the inlining fuctions which
> are now calling the new helper function.

The casting away of the const is a bit nasty, but short of making this
whole thing a macro, I can't see a better way.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/i2c/i2c-core-base.c | 64 +++++++++++++--------------------------------
>  include/linux/i2c.h         | 34 +++++++++++++++++++++---
>  2 files changed, 48 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index de1850bd440659..206c47c85c98c5 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1972,63 +1972,35 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  EXPORT_SYMBOL(i2c_transfer);
>  
>  /**
> - * i2c_master_send - issue a single I2C message in master transmit mode
> + * i2c_transfer_buffer_flags - issue a single I2C message transferring data
> + * 			       to/from a buffer
>   * @client: Handle to slave device
> - * @buf: Data that will be written to the slave
> - * @count: How many bytes to write, must be less than 64k since msg.len is u16
> + * @buf: Where the data is stored
> + * @count: How many bytes to transfer, must be less than 64k since msg.len is u16
> + * @flags: The flags to be used for the message, e.g. I2C_M_RD for reads
>   *
> - * Returns negative errno, or else the number of bytes written.
> + * Returns negative errno, or else the number of bytes transferred.
>   */
> -int i2c_master_send(const struct i2c_client *client, const char *buf, int count)
> +int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
> +			      int count, u16 flags)
>  {
>  	int ret;
> -	struct i2c_adapter *adap = client->adapter;
> -	struct i2c_msg msg;
> -
> -	msg.addr = client->addr;
> -	msg.flags = client->flags & I2C_M_TEN;
> -	msg.len = count;
> -	msg.buf = (char *)buf;
> -
> -	ret = i2c_transfer(adap, &msg, 1);
> -
> -	/*
> -	 * If everything went ok (i.e. 1 msg transmitted), return #bytes
> -	 * transmitted, else error code.
> -	 */
> -	return (ret == 1) ? count : ret;
> -}
> -EXPORT_SYMBOL(i2c_master_send);
> -
> -/**
> - * i2c_master_recv - issue a single I2C message in master receive mode
> - * @client: Handle to slave device
> - * @buf: Where to store data read from slave
> - * @count: How many bytes to read, must be less than 64k since msg.len is u16
> - *
> - * Returns negative errno, or else the number of bytes read.
> - */
> -int i2c_master_recv(const struct i2c_client *client, char *buf, int count)
> -{
> -	struct i2c_adapter *adap = client->adapter;
> -	struct i2c_msg msg;
> -	int ret;
> -
> -	msg.addr = client->addr;
> -	msg.flags = client->flags & I2C_M_TEN;
> -	msg.flags |= I2C_M_RD;
> -	msg.len = count;
> -	msg.buf = buf;
> +	struct i2c_msg msg = {
> +		.addr = client->addr,
> +		.flags = flags | (client->flags & I2C_M_TEN),
> +		.len = count,
> +		.buf = buf,
> +	};
>  
> -	ret = i2c_transfer(adap, &msg, 1);
> +	ret = i2c_transfer(client->adapter, &msg, 1);
>  
>  	/*
> -	 * If everything went ok (i.e. 1 msg received), return #bytes received,
> -	 * else error code.
> +	 * If everything went ok (i.e. 1 msg transferred), return #bytes
> +	 * transferred, else error code.
>  	 */
>  	return (ret == 1) ? count : ret;
>  }
> -EXPORT_SYMBOL(i2c_master_recv);
> +EXPORT_SYMBOL(i2c_transfer_buffer_flags);
>  
>  /* ----------------------------------------------------
>   * the i2c address scanning function
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index a0b57de91e21d3..ef1a8791c1ae24 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -63,10 +63,36 @@ struct property_entry;
>   * transmit an arbitrary number of messages without interruption.
>   * @count must be be less than 64k since msg.len is u16.
>   */
> -extern int i2c_master_send(const struct i2c_client *client, const char *buf,
> -			   int count);
> -extern int i2c_master_recv(const struct i2c_client *client, char *buf,
> -			   int count);
> +extern int i2c_transfer_buffer_flags(const struct i2c_client *client,
> +				     char *buf, int count, u16 flags);
> +
> +/**
> + * i2c_master_recv - issue a single I2C message in master receive mode
> + * @client: Handle to slave device
> + * @buf: Where to store data read from slave
> + * @count: How many bytes to read, must be less than 64k since msg.len is u16
> + *
> + * Returns negative errno, or else the number of bytes read.
> + */
> +static inline int i2c_master_recv(const struct i2c_client *client,
> +				  char *buf, int count)
> +{
> +	return i2c_transfer_buffer_flags(client, buf, count, I2C_M_RD);
> +};
> +
> +/**
> + * i2c_master_send - issue a single I2C message in master transmit mode
> + * @client: Handle to slave device
> + * @buf: Data that will be written to the slave
> + * @count: How many bytes to write, must be less than 64k since msg.len is u16
> + *
> + * Returns negative errno, or else the number of bytes written.
> + */
> +static inline int i2c_master_send(const struct i2c_client *client,
> +				  const char *buf, int count)
> +{
> +	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
> +};
>  
>  /* Transfer num messages.
>   */

Re: [PATCH v6 4/9] i2c: refactor i2c_master_{send_recv}

[Jonathan Cameron]

On Sat, 4 Nov 2017 21:20:04 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> Those two functions are very similar, the only differences are that one
> needs the I2C_M_RD flag for its message while the other one needs the
> buffer casted to drop the const. Introduce a generic helper which
> allows to specify the flags (also needed later for DMA safe variants of
> these calls) and let the casting be done in the inlining fuctions which
> are now calling the new helper function.

The casting away of the const is a bit nasty, but short of making this
whole thing a macro, I can't see a better way.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/i2c/i2c-core-base.c | 64 +++++++++++++--------------------------------
>  include/linux/i2c.h         | 34 +++++++++++++++++++++---
>  2 files changed, 48 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index de1850bd440659..206c47c85c98c5 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1972,63 +1972,35 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  EXPORT_SYMBOL(i2c_transfer);
>  
>  /**
> - * i2c_master_send - issue a single I2C message in master transmit mode
> + * i2c_transfer_buffer_flags - issue a single I2C message transferring data
> + * 			       to/from a buffer
>   * @client: Handle to slave device
> - * @buf: Data that will be written to the slave
> - * @count: How many bytes to write, must be less than 64k since msg.len is u16
> + * @buf: Where the data is stored
> + * @count: How many bytes to transfer, must be less than 64k since msg.len is u16
> + * @flags: The flags to be used for the message, e.g. I2C_M_RD for reads
>   *
> - * Returns negative errno, or else the number of bytes written.
> + * Returns negative errno, or else the number of bytes transferred.
>   */
> -int i2c_master_send(const struct i2c_client *client, const char *buf, int count)
> +int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
> +			      int count, u16 flags)
>  {
>  	int ret;
> -	struct i2c_adapter *adap = client->adapter;
> -	struct i2c_msg msg;
> -
> -	msg.addr = client->addr;
> -	msg.flags = client->flags & I2C_M_TEN;
> -	msg.len = count;
> -	msg.buf = (char *)buf;
> -
> -	ret = i2c_transfer(adap, &msg, 1);
> -
> -	/*
> -	 * If everything went ok (i.e. 1 msg transmitted), return #bytes
> -	 * transmitted, else error code.
> -	 */
> -	return (ret == 1) ? count : ret;
> -}
> -EXPORT_SYMBOL(i2c_master_send);
> -
> -/**
> - * i2c_master_recv - issue a single I2C message in master receive mode
> - * @client: Handle to slave device
> - * @buf: Where to store data read from slave
> - * @count: How many bytes to read, must be less than 64k since msg.len is u16
> - *
> - * Returns negative errno, or else the number of bytes read.
> - */
> -int i2c_master_recv(const struct i2c_client *client, char *buf, int count)
> -{
> -	struct i2c_adapter *adap = client->adapter;
> -	struct i2c_msg msg;
> -	int ret;
> -
> -	msg.addr = client->addr;
> -	msg.flags = client->flags & I2C_M_TEN;
> -	msg.flags |= I2C_M_RD;
> -	msg.len = count;
> -	msg.buf = buf;
> +	struct i2c_msg msg = {
> +		.addr = client->addr,
> +		.flags = flags | (client->flags & I2C_M_TEN),
> +		.len = count,
> +		.buf = buf,
> +	};
>  
> -	ret = i2c_transfer(adap, &msg, 1);
> +	ret = i2c_transfer(client->adapter, &msg, 1);
>  
>  	/*
> -	 * If everything went ok (i.e. 1 msg received), return #bytes received,
> -	 * else error code.
> +	 * If everything went ok (i.e. 1 msg transferred), return #bytes
> +	 * transferred, else error code.
>  	 */
>  	return (ret == 1) ? count : ret;
>  }
> -EXPORT_SYMBOL(i2c_master_recv);
> +EXPORT_SYMBOL(i2c_transfer_buffer_flags);
>  
>  /* ----------------------------------------------------
>   * the i2c address scanning function
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index a0b57de91e21d3..ef1a8791c1ae24 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -63,10 +63,36 @@ struct property_entry;
>   * transmit an arbitrary number of messages without interruption.
>   * @count must be be less than 64k since msg.len is u16.
>   */
> -extern int i2c_master_send(const struct i2c_client *client, const char *buf,
> -			   int count);
> -extern int i2c_master_recv(const struct i2c_client *client, char *buf,
> -			   int count);
> +extern int i2c_transfer_buffer_flags(const struct i2c_client *client,
> +				     char *buf, int count, u16 flags);
> +
> +/**
> + * i2c_master_recv - issue a single I2C message in master receive mode
> + * @client: Handle to slave device
> + * @buf: Where to store data read from slave
> + * @count: How many bytes to read, must be less than 64k since msg.len is u16
> + *
> + * Returns negative errno, or else the number of bytes read.
> + */
> +static inline int i2c_master_recv(const struct i2c_client *client,
> +				  char *buf, int count)
> +{
> +	return i2c_transfer_buffer_flags(client, buf, count, I2C_M_RD);
> +};
> +
> +/**
> + * i2c_master_send - issue a single I2C message in master transmit mode
> + * @client: Handle to slave device
> + * @buf: Data that will be written to the slave
> + * @count: How many bytes to write, must be less than 64k since msg.len is u16
> + *
> + * Returns negative errno, or else the number of bytes written.
> + */
> +static inline int i2c_master_send(const struct i2c_client *client,
> +				  const char *buf, int count)
> +{
> +	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
> +};
>  
>  /* Transfer num messages.
>   */

[PATCH v6 5/9] i2c: add i2c_master_{send|recv}_dmasafe

[Wolfram Sang]

Use the new helper to create variants of i2c_master_{send|recv} which
mark their buffers as DMA safe.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/linux/i2c.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index ef1a8791c1ae24..8c144e3cbfb261 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -81,6 +81,22 @@ static inline int i2c_master_recv(const struct i2c_client *client,
 };
 
 /**
+ * i2c_master_recv_dmasafe - issue a single I2C message in master receive mode
+ *			     using a DMA safe buffer
+ * @client: Handle to slave device
+ * @buf: Where to store data read from slave, must be safe to use with DMA
+ * @count: How many bytes to read, must be less than 64k since msg.len is u16
+ *
+ * Returns negative errno, or else the number of bytes read.
+ */
+static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
+					  char *buf, int count)
+{
+	return i2c_transfer_buffer_flags(client, buf, count,
+					 I2C_M_RD | I2C_M_DMA_SAFE);
+};
+
+/**
  * i2c_master_send - issue a single I2C message in master transmit mode
  * @client: Handle to slave device
  * @buf: Data that will be written to the slave
@@ -93,6 +109,21 @@ static inline int i2c_master_send(const struct i2c_client *client,
 {
 	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
 };
+/**
+ * i2c_master_send_dmasafe - issue a single I2C message in master transmit mode
+ *			     using a DMA safe buffer
+ * @client: Handle to slave device
+ * @buf: Data that will be written to the slave, must be safe to use with DMA
+ * @count: How many bytes to write, must be less than 64k since msg.len is u16
+ *
+ * Returns negative errno, or else the number of bytes written.
+ */
+static inline int i2c_master_send_dmasafe(const struct i2c_client *client,
+					  const char *buf, int count)
+{
+	return i2c_transfer_buffer_flags(client, (char *)buf, count,
+					 I2C_M_DMA_SAFE);
+};
 
 /* Transfer num messages.
  */
-- 
2.11.0

Re: [PATCH v6 5/9] i2c: add i2c_master_{send|recv}_dmasafe

[Jonathan Cameron]

On Sat, 4 Nov 2017 21:20:05 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> Use the new helper to create variants of i2c_master_{send|recv} which
> mark their buffers as DMA safe.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Can't really argue with such a simple patch ;)
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  include/linux/i2c.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index ef1a8791c1ae24..8c144e3cbfb261 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -81,6 +81,22 @@ static inline int i2c_master_recv(const struct i2c_client *client,
>  };
>  
>  /**
> + * i2c_master_recv_dmasafe - issue a single I2C message in master receive mode
> + *			     using a DMA safe buffer
> + * @client: Handle to slave device
> + * @buf: Where to store data read from slave, must be safe to use with DMA
> + * @count: How many bytes to read, must be less than 64k since msg.len is u16
> + *
> + * Returns negative errno, or else the number of bytes read.
> + */
> +static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
> +					  char *buf, int count)
> +{
> +	return i2c_transfer_buffer_flags(client, buf, count,
> +					 I2C_M_RD | I2C_M_DMA_SAFE);
> +};
> +
> +/**
>   * i2c_master_send - issue a single I2C message in master transmit mode
>   * @client: Handle to slave device
>   * @buf: Data that will be written to the slave
> @@ -93,6 +109,21 @@ static inline int i2c_master_send(const struct i2c_client *client,
>  {
>  	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
>  };
> +/**
> + * i2c_master_send_dmasafe - issue a single I2C message in master transmit mode
> + *			     using a DMA safe buffer
> + * @client: Handle to slave device
> + * @buf: Data that will be written to the slave, must be safe to use with DMA
> + * @count: How many bytes to write, must be less than 64k since msg.len is u16
> + *
> + * Returns negative errno, or else the number of bytes written.
> + */
> +static inline int i2c_master_send_dmasafe(const struct i2c_client *client,
> +					  const char *buf, int count)
> +{
> +	return i2c_transfer_buffer_flags(client, (char *)buf, count,
> +					 I2C_M_DMA_SAFE);
> +};
>  
>  /* Transfer num messages.
>   */

Re: [PATCH v6 5/9] i2c: add i2c_master_{send|recv}_dmasafe

[Jonathan Cameron]

On Sat, 4 Nov 2017 21:20:05 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> Use the new helper to create variants of i2c_master_{send|recv} which
> mark their buffers as DMA safe.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Can't really argue with such a simple patch ;)
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  include/linux/i2c.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index ef1a8791c1ae24..8c144e3cbfb261 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -81,6 +81,22 @@ static inline int i2c_master_recv(const struct i2c_client *client,
>  };
>  
>  /**
> + * i2c_master_recv_dmasafe - issue a single I2C message in master receive mode
> + *			     using a DMA safe buffer
> + * @client: Handle to slave device
> + * @buf: Where to store data read from slave, must be safe to use with DMA
> + * @count: How many bytes to read, must be less than 64k since msg.len is u16
> + *
> + * Returns negative errno, or else the number of bytes read.
> + */
> +static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
> +					  char *buf, int count)
> +{
> +	return i2c_transfer_buffer_flags(client, buf, count,
> +					 I2C_M_RD | I2C_M_DMA_SAFE);
> +};
> +
> +/**
>   * i2c_master_send - issue a single I2C message in master transmit mode
>   * @client: Handle to slave device
>   * @buf: Data that will be written to the slave
> @@ -93,6 +109,21 @@ static inline int i2c_master_send(const struct i2c_client *client,
>  {
>  	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
>  };
> +/**
> + * i2c_master_send_dmasafe - issue a single I2C message in master transmit mode
> + *			     using a DMA safe buffer
> + * @client: Handle to slave device
> + * @buf: Data that will be written to the slave, must be safe to use with DMA
> + * @count: How many bytes to write, must be less than 64k since msg.len is u16
> + *
> + * Returns negative errno, or else the number of bytes written.
> + */
> +static inline int i2c_master_send_dmasafe(const struct i2c_client *client,
> +					  const char *buf, int count)
> +{
> +	return i2c_transfer_buffer_flags(client, (char *)buf, count,
> +					 I2C_M_DMA_SAFE);
> +};
>  
>  /* Transfer num messages.
>   */

[PATCH v6 6/9] i2c: smbus: use DMA safe buffers for emulated SMBus transactions

[Wolfram Sang]

For all block commands, try to allocate a DMA safe buffer and mark it
accordingly. Only use the stack, if the buffers cannot be allocated.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-smbus.c | 45 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 4bb9927afd0106..931c274fe26809 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -18,6 +18,7 @@
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/i2c-smbus.h>
+#include <linux/slab.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/smbus.h>
@@ -291,6 +292,22 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, u8 command,
 }
 EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
 
+static void i2c_smbus_try_get_dmabuf(struct i2c_msg *msg, u8 init_val)
+{
+	bool is_read = msg->flags & I2C_M_RD;
+	unsigned char *dma_buf;
+
+	dma_buf = kzalloc(I2C_SMBUS_BLOCK_MAX + (is_read ? 2 : 3), GFP_KERNEL);
+	if (!dma_buf)
+		return;
+
+	msg->buf = dma_buf;
+	msg->flags |= I2C_M_DMA_SAFE;
+
+	if (init_val)
+		msg->buf[0] = init_val;
+
+}
 /* Simulate a SMBus command using the i2c protocol
    No checking of parameters is done!  */
 static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
@@ -368,6 +385,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			msg[1].flags |= I2C_M_RECV_LEN;
 			msg[1].len = 1; /* block length will be added by
 					   the underlying bus driver */
+			i2c_smbus_try_get_dmabuf(&msg[1], 0);
 		} else {
 			msg[0].len = data->block[0] + 2;
 			if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 2) {
@@ -376,8 +394,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 					data->block[0]);
 				return -EINVAL;
 			}
+
+			i2c_smbus_try_get_dmabuf(&msg[0], command);
 			for (i = 1; i < msg[0].len; i++)
-				msgbuf0[i] = data->block[i-1];
+				msg[0].buf[i] = data->block[i-1];
 		}
 		break;
 	case I2C_SMBUS_BLOCK_PROC_CALL:
@@ -389,16 +409,21 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 				data->block[0]);
 			return -EINVAL;
 		}
+
 		msg[0].len = data->block[0] + 2;
+		i2c_smbus_try_get_dmabuf(&msg[0], command);
 		for (i = 1; i < msg[0].len; i++)
-			msgbuf0[i] = data->block[i-1];
+			msg[0].buf[i] = data->block[i-1];
+
 		msg[1].flags |= I2C_M_RECV_LEN;
 		msg[1].len = 1; /* block length will be added by
 				   the underlying bus driver */
+		i2c_smbus_try_get_dmabuf(&msg[1], 0);
 		break;
 	case I2C_SMBUS_I2C_BLOCK_DATA:
 		if (read_write == I2C_SMBUS_READ) {
 			msg[1].len = data->block[0];
+			i2c_smbus_try_get_dmabuf(&msg[1], 0);
 		} else {
 			msg[0].len = data->block[0] + 1;
 			if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 1) {
@@ -407,8 +432,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 					data->block[0]);
 				return -EINVAL;
 			}
+
+			i2c_smbus_try_get_dmabuf(&msg[0], command);
 			for (i = 1; i <= data->block[0]; i++)
-				msgbuf0[i] = data->block[i];
+				msg[0].buf[i] = data->block[i];
 		}
 		break;
 	default:
@@ -456,14 +483,20 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			break;
 		case I2C_SMBUS_I2C_BLOCK_DATA:
 			for (i = 0; i < data->block[0]; i++)
-				data->block[i+1] = msgbuf1[i];
+				data->block[i+1] = msg[1].buf[i];
 			break;
 		case I2C_SMBUS_BLOCK_DATA:
 		case I2C_SMBUS_BLOCK_PROC_CALL:
-			for (i = 0; i < msgbuf1[0] + 1; i++)
-				data->block[i] = msgbuf1[i];
+			for (i = 0; i < msg[1].buf[0] + 1; i++)
+				data->block[i] = msg[1].buf[i];
 			break;
 		}
+
+	if (msg[0].flags & I2C_M_DMA_SAFE)
+		kfree(msg[0].buf);
+	if (msg[1].flags & I2C_M_DMA_SAFE)
+		kfree(msg[1].buf);
+
 	return 0;
 }
 
-- 
2.11.0

Re: [PATCH v6 6/9] i2c: smbus: use DMA safe buffers for emulated SMBus transactions

[Jonathan Cameron]

On Sat, 4 Nov 2017 21:20:06 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> For all block commands, try to allocate a DMA safe buffer and mark it
> accordingly. Only use the stack, if the buffers cannot be allocated.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Hmm. Interesting balance here as this adds allocations to paths where the i2c
master can't take advantage.  Not ideal, but perhaps not worth the hassle
of working around it?

It's only for the block calls I guess so not that major an issue.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/i2c/i2c-core-smbus.c | 45 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 4bb9927afd0106..931c274fe26809 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -18,6 +18,7 @@
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-smbus.h>
> +#include <linux/slab.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/smbus.h>
> @@ -291,6 +292,22 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, u8 command,
>  }
>  EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
>  
> +static void i2c_smbus_try_get_dmabuf(struct i2c_msg *msg, u8 init_val)
> +{
> +	bool is_read = msg->flags & I2C_M_RD;
> +	unsigned char *dma_buf;
> +
> +	dma_buf = kzalloc(I2C_SMBUS_BLOCK_MAX + (is_read ? 2 : 3), GFP_KERNEL);
> +	if (!dma_buf)
> +		return;
> +
> +	msg->buf = dma_buf;
> +	msg->flags |= I2C_M_DMA_SAFE;
> +
> +	if (init_val)
> +		msg->buf[0] = init_val;
> +
> +}
>  /* Simulate a SMBus command using the i2c protocol
>     No checking of parameters is done!  */
>  static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> @@ -368,6 +385,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			msg[1].flags |= I2C_M_RECV_LEN;
>  			msg[1].len = 1; /* block length will be added by
>  					   the underlying bus driver */
> +			i2c_smbus_try_get_dmabuf(&msg[1], 0);
>  		} else {
>  			msg[0].len = data->block[0] + 2;
>  			if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 2) {
> @@ -376,8 +394,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  					data->block[0]);
>  				return -EINVAL;
>  			}
> +
> +			i2c_smbus_try_get_dmabuf(&msg[0], command);
>  			for (i = 1; i < msg[0].len; i++)
> -				msgbuf0[i] = data->block[i-1];
> +				msg[0].buf[i] = data->block[i-1];
>  		}
>  		break;
>  	case I2C_SMBUS_BLOCK_PROC_CALL:
> @@ -389,16 +409,21 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  				data->block[0]);
>  			return -EINVAL;
>  		}
> +
>  		msg[0].len = data->block[0] + 2;
> +		i2c_smbus_try_get_dmabuf(&msg[0], command);
>  		for (i = 1; i < msg[0].len; i++)
> -			msgbuf0[i] = data->block[i-1];
> +			msg[0].buf[i] = data->block[i-1];
> +
>  		msg[1].flags |= I2C_M_RECV_LEN;
>  		msg[1].len = 1; /* block length will be added by
>  				   the underlying bus driver */
> +		i2c_smbus_try_get_dmabuf(&msg[1], 0);
>  		break;
>  	case I2C_SMBUS_I2C_BLOCK_DATA:
>  		if (read_write == I2C_SMBUS_READ) {
>  			msg[1].len = data->block[0];
> +			i2c_smbus_try_get_dmabuf(&msg[1], 0);
>  		} else {
>  			msg[0].len = data->block[0] + 1;
>  			if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 1) {
> @@ -407,8 +432,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  					data->block[0]);
>  				return -EINVAL;
>  			}
> +
> +			i2c_smbus_try_get_dmabuf(&msg[0], command);
>  			for (i = 1; i <= data->block[0]; i++)
> -				msgbuf0[i] = data->block[i];
> +				msg[0].buf[i] = data->block[i];
>  		}
>  		break;
>  	default:
> @@ -456,14 +483,20 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			break;
>  		case I2C_SMBUS_I2C_BLOCK_DATA:
>  			for (i = 0; i < data->block[0]; i++)
> -				data->block[i+1] = msgbuf1[i];
> +				data->block[i+1] = msg[1].buf[i];
>  			break;
>  		case I2C_SMBUS_BLOCK_DATA:
>  		case I2C_SMBUS_BLOCK_PROC_CALL:
> -			for (i = 0; i < msgbuf1[0] + 1; i++)
> -				data->block[i] = msgbuf1[i];
> +			for (i = 0; i < msg[1].buf[0] + 1; i++)
> +				data->block[i] = msg[1].buf[i];
>  			break;
>  		}
> +
> +	if (msg[0].flags & I2C_M_DMA_SAFE)
> +		kfree(msg[0].buf);
> +	if (msg[1].flags & I2C_M_DMA_SAFE)
> +		kfree(msg[1].buf);
> +
>  	return 0;
>  }
>  

Re: [PATCH v6 6/9] i2c: smbus: use DMA safe buffers for emulated SMBus transactions

[Jonathan Cameron]

On Sat, 4 Nov 2017 21:20:06 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> For all block commands, try to allocate a DMA safe buffer and mark it
> accordingly. Only use the stack, if the buffers cannot be allocated.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Hmm. Interesting balance here as this adds allocations to paths where the i2c
master can't take advantage.  Not ideal, but perhaps not worth the hassle
of working around it?

It's only for the block calls I guess so not that major an issue.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/i2c/i2c-core-smbus.c | 45 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 4bb9927afd0106..931c274fe26809 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -18,6 +18,7 @@
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-smbus.h>
> +#include <linux/slab.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/smbus.h>
> @@ -291,6 +292,22 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, u8 command,
>  }
>  EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
>  
> +static void i2c_smbus_try_get_dmabuf(struct i2c_msg *msg, u8 init_val)
> +{
> +	bool is_read = msg->flags & I2C_M_RD;
> +	unsigned char *dma_buf;
> +
> +	dma_buf = kzalloc(I2C_SMBUS_BLOCK_MAX + (is_read ? 2 : 3), GFP_KERNEL);
> +	if (!dma_buf)
> +		return;
> +
> +	msg->buf = dma_buf;
> +	msg->flags |= I2C_M_DMA_SAFE;
> +
> +	if (init_val)
> +		msg->buf[0] = init_val;
> +
> +}
>  /* Simulate a SMBus command using the i2c protocol
>     No checking of parameters is done!  */
>  static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
> @@ -368,6 +385,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			msg[1].flags |= I2C_M_RECV_LEN;
>  			msg[1].len = 1; /* block length will be added by
>  					   the underlying bus driver */
> +			i2c_smbus_try_get_dmabuf(&msg[1], 0);
>  		} else {
>  			msg[0].len = data->block[0] + 2;
>  			if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 2) {
> @@ -376,8 +394,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  					data->block[0]);
>  				return -EINVAL;
>  			}
> +
> +			i2c_smbus_try_get_dmabuf(&msg[0], command);
>  			for (i = 1; i < msg[0].len; i++)
> -				msgbuf0[i] = data->block[i-1];
> +				msg[0].buf[i] = data->block[i-1];
>  		}
>  		break;
>  	case I2C_SMBUS_BLOCK_PROC_CALL:
> @@ -389,16 +409,21 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  				data->block[0]);
>  			return -EINVAL;
>  		}
> +
>  		msg[0].len = data->block[0] + 2;
> +		i2c_smbus_try_get_dmabuf(&msg[0], command);
>  		for (i = 1; i < msg[0].len; i++)
> -			msgbuf0[i] = data->block[i-1];
> +			msg[0].buf[i] = data->block[i-1];
> +
>  		msg[1].flags |= I2C_M_RECV_LEN;
>  		msg[1].len = 1; /* block length will be added by
>  				   the underlying bus driver */
> +		i2c_smbus_try_get_dmabuf(&msg[1], 0);
>  		break;
>  	case I2C_SMBUS_I2C_BLOCK_DATA:
>  		if (read_write == I2C_SMBUS_READ) {
>  			msg[1].len = data->block[0];
> +			i2c_smbus_try_get_dmabuf(&msg[1], 0);
>  		} else {
>  			msg[0].len = data->block[0] + 1;
>  			if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 1) {
> @@ -407,8 +432,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  					data->block[0]);
>  				return -EINVAL;
>  			}
> +
> +			i2c_smbus_try_get_dmabuf(&msg[0], command);
>  			for (i = 1; i <= data->block[0]; i++)
> -				msgbuf0[i] = data->block[i];
> +				msg[0].buf[i] = data->block[i];
>  		}
>  		break;
>  	default:
> @@ -456,14 +483,20 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
>  			break;
>  		case I2C_SMBUS_I2C_BLOCK_DATA:
>  			for (i = 0; i < data->block[0]; i++)
> -				data->block[i+1] = msgbuf1[i];
> +				data->block[i+1] = msg[1].buf[i];
>  			break;
>  		case I2C_SMBUS_BLOCK_DATA:
>  		case I2C_SMBUS_BLOCK_PROC_CALL:
> -			for (i = 0; i < msgbuf1[0] + 1; i++)
> -				data->block[i] = msgbuf1[i];
> +			for (i = 0; i < msg[1].buf[0] + 1; i++)
> +				data->block[i] = msg[1].buf[i];
>  			break;
>  		}
> +
> +	if (msg[0].flags & I2C_M_DMA_SAFE)
> +		kfree(msg[0].buf);
> +	if (msg[1].flags & I2C_M_DMA_SAFE)
> +		kfree(msg[1].buf);
> +
>  	return 0;
>  }
>  

[PATCH v6 7/9] i2c: add docs to clarify DMA handling

[Wolfram Sang]

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/DMA-considerations | 67 ++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/i2c/DMA-considerations

diff --git a/Documentation/i2c/DMA-considerations b/Documentation/i2c/DMA-considerations
new file mode 100644
index 00000000000000..966610aa4620fa
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,67 @@
+=================
+Linux I2C and DMA
+=================
+
+Given that i2c is a low-speed bus, over which the majority of messages
+transferred are small, it is not considered a prime user of DMA access. At this
+time of writing, only 10% of I2C bus master drivers have DMA support
+implemented. And the vast majority of transactions are so small that setting up
+DMA for it will likely add more overhead than a plain PIO transfer.
+
+Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe.
+It does not seem reasonable to apply additional burdens when the feature is so
+rarely used. However, it is recommended to use a DMA-safe buffer if your
+message size is likely applicable for DMA. Most drivers have this threshold
+around 8 bytes (as of today, this is mostly an educated guess, however). For
+any message of 16 byte or larger, it is probably a really good idea. Please
+note that other subsystems you use might add requirements. E.g., if your
+I2C bus master driver is using USB as a bridge, then you need to have DMA
+safe buffers always, because USB requires it.
+
+Clients
+-------
+
+For clients, if you use a DMA safe buffer in i2c_msg, set the I2C_M_DMA_SAFE
+flag with it. Then, the I2C core and drivers know they can safely operate DMA
+on it. Note that using this flag is optional. I2C host drivers which are not
+updated to use this flag will work like before. And like before, they risk
+using an unsafe DMA buffer. To improve this situation, using I2C_M_DMA_SAFE in
+more and more clients and host drivers is the planned way forward. Note also
+that setting this flag makes only sense in kernel space. User space data is
+copied into kernel space anyhow. The I2C core makes sure the destination
+buffers in kernel space are always DMA capable. Also, when the core emulates
+SMBus transactions via I2C, the buffers for block transfers are DMA safe. Users
+of i2c_master_send() and i2c_master_recv() functions can now use DMA safe
+variants (i2c_master_send_dmasafe() and i2c_master_recv_dmasafe()) once they
+know their buffers are DMA safe. Users of i2c_transfer() must set the
+I2C_M_DMA_SAFE flag manually.
+
+Masters
+-------
+
+Bus master drivers wishing to implement safe DMA can use helper functions from
+the I2C core. One gives you a DMA-safe buffer for a given i2c_msg as long as a
+certain threshold is met::
+
+	dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);
+
+If a buffer is returned, it is either msg->buf for the I2C_M_DMA_SAFE case or a
+bounce buffer. But you don't need to care about that detail, just use the
+returned buffer. If NULL is returned, the threshold was not met or a bounce
+buffer could not be allocated. Fall back to PIO in that case.
+
+In any case, a buffer obtained from above needs to be released. It ensures data
+is copied back to the message and a potentially used bounce buffer is freed::
+
+	i2c_release_dma_safe_msg_buf(msg, dma_buf);
+
+The bounce buffer handling from the core is generic and simple. It will always
+allocate a new bounce buffer. If you want a more sophisticated handling (e.g.
+reusing pre-allocated buffers), you are free to implement your own.
+
+Please also check the in-kernel documentation for details. The i2c-sh_mobile
+driver can be used as a reference example how to use the above helpers.
+
+Final note: If you plan to use DMA with I2C (or with anything else, actually)
+make sure you have CONFIG_DMA_API_DEBUG enabled during development. It can help
+you find various issues which can be complex to debug otherwise.
-- 
2.11.0

[PATCH v6 8/9] i2c: sh_mobile: use core helper to decide when to use DMA

[Wolfram Sang]

This ensures that we fall back to PIO if the message length is too small
for DMA being useful. Otherwise, we use DMA. A bounce buffer might be
applied by the helper if the original message buffer is not DMA safe.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index c03acdf7139726..fbadb861672b84 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -145,6 +145,7 @@ struct sh_mobile_i2c_data {
 	struct dma_chan *dma_rx;
 	struct scatterlist sg;
 	enum dma_data_direction dma_direction;
+	u8 *dma_buf;
 };
 
 struct sh_mobile_dt_config {
@@ -548,6 +549,8 @@ static void sh_mobile_i2c_dma_callback(void *data)
 	pd->pos = pd->msg->len;
 	pd->stop_after_dma = true;
 
+	i2c_release_dma_safe_msg_buf(pd->msg, pd->dma_buf);
+
 	iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
 }
 
@@ -608,7 +611,7 @@ static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
 	if (IS_ERR(chan))
 		return;
 
-	dma_addr = dma_map_single(chan->device->dev, pd->msg->buf, pd->msg->len, dir);
+	dma_addr = dma_map_single(chan->device->dev, pd->dma_buf, pd->msg->len, dir);
 	if (dma_mapping_error(chan->device->dev, dma_addr)) {
 		dev_dbg(pd->dev, "dma map failed, using PIO\n");
 		return;
@@ -665,7 +668,8 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
 	pd->pos = -1;
 	pd->sr = 0;
 
-	if (pd->msg->len > 8)
+	pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8);
+	if (pd->dma_buf)
 		sh_mobile_i2c_xfer_dma(pd);
 
 	/* Enable all interrupts to begin with */
-- 
2.11.0

[PATCH v6 9/9] i2c: rcar: skip DMA if buffer is not safe

[Wolfram Sang]

This HW is prone to races, so it needs to setup new messages in irq
context. That means we can't alloc bounce buffers if a message buffer is
not DMA safe. So, in that case, simply fall back to PIO.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 15d764afec3b29..8a2ae3e6c561c4 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -359,7 +359,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
 	int len;
 
 	/* Do not use DMA if it's not available or for messages < 8 bytes */
-	if (IS_ERR(chan) || msg->len < 8)
+	if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE))
 		return;
 
 	if (read) {
-- 
2.11.0

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

On Sat, Nov 04, 2017 at 09:20:00PM +0100, Wolfram Sang wrote:

> While previous versions until v3 tried to magically apply bounce buffers when
> needed, it became clear that detecting DMA safe buffers is too fragile. This
> approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
> outcome so far is very convincing IMO. The core additions are simple and easy
> to understand. The driver changes for the Renesas IP cores became easy to
> understand, too.

It would really help a lot of things if there were a way to detect if a
given memory block is DMA safe, having to pass the information around
causes so much pain.  There's the fun with vmalloc() mappings too
needing to be handled differently too though that's less likely to bite
I2C.

> I am still not sure how we can teach regmap this new flag. regmap is a heavy
> user of I2C, so broonie's opinion here would be great to have. The rest should
> mostly be updating individual drivers which can be done when needed.

We pretty much assume everything is DMA safe already, the majority of
transfers go to/from kmalloc()ed scratch buffers so actually are DMA
safe but for bulk transfers we use the caller buffer and there might be
some problem users.  I can't really think of a particularly good way of
handling it off the top of my head, obviously not setting the flag is
easy but doesn't get the benefit while always using a bounce buffer
would involve lots of unneeded memcpy().  Doing _dmasafe() isn't
particularly appealing either but might be what we end up with.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1812 bytes --]

On Wed, Nov 08, 2017 at 10:50:37PM +0000, Mark Brown wrote:
> On Sat, Nov 04, 2017 at 09:20:00PM +0100, Wolfram Sang wrote:
> 
> > While previous versions until v3 tried to magically apply bounce buffers when
> > needed, it became clear that detecting DMA safe buffers is too fragile. This
> > approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
> > outcome so far is very convincing IMO. The core additions are simple and easy
> > to understand. The driver changes for the Renesas IP cores became easy to
> > understand, too.
> 
> It would really help a lot of things if there were a way to detect if a
> given memory block is DMA safe, having to pass the information around
> causes so much pain.

I so agree.

> > I am still not sure how we can teach regmap this new flag. regmap is a heavy
> > user of I2C, so broonie's opinion here would be great to have. The rest should
> > mostly be updating individual drivers which can be done when needed.
> 
> We pretty much assume everything is DMA safe already, the majority of
> transfers go to/from kmalloc()ed scratch buffers so actually are DMA
> safe but for bulk transfers we use the caller buffer and there might be
> some problem users.

So, pretty much the situation I2C was in before this patch set: we
pretty much assume DMA safety but there might be problem users.

> I can't really think of a particularly good way of
> handling it off the top of my head, obviously not setting the flag is
> easy but doesn't get the benefit while always using a bounce buffer
> would involve lots of unneeded memcpy().  Doing _dmasafe() isn't
> particularly appealing either but might be what we end up with.

Okay. That sounds you are fine with the approach taken here, in general?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

On Mon, Nov 27, 2017 at 07:51:16PM +0100, Wolfram Sang wrote:
> On Wed, Nov 08, 2017 at 10:50:37PM +0000, Mark Brown wrote:

> > We pretty much assume everything is DMA safe already, the majority of
> > transfers go to/from kmalloc()ed scratch buffers so actually are DMA
> > safe but for bulk transfers we use the caller buffer and there might be
> > some problem users.

> So, pretty much the situation I2C was in before this patch set: we
> pretty much assume DMA safety but there might be problem users.

It's a bit different in that it's much more likely that a SPI controller
will actually do DMA than an I2C one since the speeds are higher and
there's frequent applications that do large transfers so it's more
likely that people will do the right thing as issues would tend to come
up if they don't.

> > I can't really think of a particularly good way of
> > handling it off the top of my head, obviously not setting the flag is
> > easy but doesn't get the benefit while always using a bounce buffer
> > would involve lots of unneeded memcpy().  Doing _dmasafe() isn't
> > particularly appealing either but might be what we end up with.

> Okay. That sounds you are fine with the approach taken here, in general?

It's hard to summon enthusiasm but yes, without changes to the DMA stuff
it's probably as good as we can do.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]


> > > We pretty much assume everything is DMA safe already, the majority of
> > > transfers go to/from kmalloc()ed scratch buffers so actually are DMA
> > > safe but for bulk transfers we use the caller buffer and there might be
> > > some problem users.
> 
> > So, pretty much the situation I2C was in before this patch set: we
> > pretty much assume DMA safety but there might be problem users.
> 
> It's a bit different in that it's much more likely that a SPI controller
> will actually do DMA than an I2C one since the speeds are higher and
> there's frequent applications that do large transfers so it's more
> likely that people will do the right thing as issues would tend to come
> up if they don't.

Yes, for SPI this is true. I was thinking more of regmap with its
usage of different transport mechanisms. But I guess they should all be
DMA safe because some of them need to be DMA safe?

> > > I can't really think of a particularly good way of
> > > handling it off the top of my head, obviously not setting the flag is
> > > easy but doesn't get the benefit while always using a bounce buffer
> > > would involve lots of unneeded memcpy().  Doing _dmasafe() isn't
> > > particularly appealing either but might be what we end up with.
> 
> > Okay. That sounds you are fine with the approach taken here, in general?
> 
> It's hard to summon enthusiasm but yes, without changes to the DMA stuff
> it's probably as good as we can do.

That sums it up pretty nicely. However, despite even my limited
enthusiasm for this extra flag, I think this set of rules has value. For
some platforms, DMA works more or less by coincidence and can lead to
surprises with e.g. virtual stacks. This is not good engineering
practice, I'd say.

I am going to apply this series now. Thanks for your input!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

On Sun, Dec 03, 2017 at 08:43:47PM +0100, Wolfram Sang wrote:

> > It's a bit different in that it's much more likely that a SPI controller
> > will actually do DMA than an I2C one since the speeds are higher and
> > there's frequent applications that do large transfers so it's more
> > likely that people will do the right thing as issues would tend to come
> > up if they don't.

> Yes, for SPI this is true. I was thinking more of regmap with its
> usage of different transport mechanisms. But I guess they should all be
> DMA safe because some of them need to be DMA safe?

Possibly.  Hopefully.  I guess we'll find out.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

On Sun, Dec 03, 2017 at 08:43:47PM +0100, Wolfram Sang wrote:

> > It's a bit different in that it's much more likely that a SPI controller
> > will actually do DMA than an I2C one since the speeds are higher and
> > there's frequent applications that do large transfers so it's more
> > likely that people will do the right thing as issues would tend to come
> > up if they don't.

> Yes, for SPI this is true. I was thinking more of regmap with its
> usage of different transport mechanisms. But I guess they should all be
> DMA safe because some of them need to be DMA safe?

Possibly.  Hopefully.  I guess we'll find out.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Jonathan Cameron]

On Mon, 4 Dec 2017 22:05:41 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Sun, Dec 03, 2017 at 08:43:47PM +0100, Wolfram Sang wrote:
> 
> > > It's a bit different in that it's much more likely that a SPI controller
> > > will actually do DMA than an I2C one since the speeds are higher and
> > > there's frequent applications that do large transfers so it's more
> > > likely that people will do the right thing as issues would tend to come
> > > up if they don't.  
> 
> > Yes, for SPI this is true. I was thinking more of regmap with its
> > usage of different transport mechanisms. But I guess they should all be
> > DMA safe because some of them need to be DMA safe?  
> 
> Possibly.  Hopefully.  I guess we'll find out.
> 

Yeah, optimistic assumption. Plenty of drivers use regmap for the
convenience of it's caching and field access etc rather than
because they support multiple buses.

I'll audit the IIO drivers and see where we have issues if we
start assuming DMA safe for regmap (which makes sense to me).

Probably worth fixing them all up anyway and tends to be straightforward.

Jonathan

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Jonathan Cameron]

On Mon, 4 Dec 2017 22:05:41 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Sun, Dec 03, 2017 at 08:43:47PM +0100, Wolfram Sang wrote:
> 
> > > It's a bit different in that it's much more likely that a SPI controller
> > > will actually do DMA than an I2C one since the speeds are higher and
> > > there's frequent applications that do large transfers so it's more
> > > likely that people will do the right thing as issues would tend to come
> > > up if they don't.  
> 
> > Yes, for SPI this is true. I was thinking more of regmap with its
> > usage of different transport mechanisms. But I guess they should all be
> > DMA safe because some of them need to be DMA safe?  
> 
> Possibly.  Hopefully.  I guess we'll find out.
> 

Yeah, optimistic assumption. Plenty of drivers use regmap for the
convenience of it's caching and field access etc rather than
because they support multiple buses.

I'll audit the IIO drivers and see where we have issues if we
start assuming DMA safe for regmap (which makes sense to me).

Probably worth fixing them all up anyway and tends to be straightforward.

Jonathan

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]

On Sat, Nov 04, 2017 at 09:20:00PM +0100, Wolfram Sang wrote:
> So, after revisiting old mail threads, taking part in a similar discussion on
> the USB list, and implementing a not-convincing solution before, here is what I
> cooked up to document and ease DMA handling for I2C within Linux. Please have a
> look at the documentation introduced in patch 7 for details. And to make it
> clear again: The stuff below is opt-in. If host drivers are not updated, they
> will continue to work like before.
> 
> While previous versions until v3 tried to magically apply bounce buffers when
> needed, it became clear that detecting DMA safe buffers is too fragile. This
> approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
> outcome so far is very convincing IMO. The core additions are simple and easy
> to understand. The driver changes for the Renesas IP cores became easy to
> understand, too.
> 
> Of course, we must now whitelist DMA safe buffers. This series implements it
> for core functionality:
> 
> a) for the I2C_RDWR when messages come from userspace
> b) for i2c_smbus_xfer_emulated(), DMA safe buffers are now allocated for
>    block transfers
> c) i2c_master_{send|recv} have now a *_dmasafe variant
> 
> I am still not sure how we can teach regmap this new flag. regmap is a heavy
> user of I2C, so broonie's opinion here would be great to have. The rest should
> mostly be updating individual drivers which can be done when needed.
> 
> All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W) and
> Renesas Lager board (r8a7790/H2). But more testing is really really welcome.
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma-v6
> 
> It is planned to land upstream in v4.16 and I want to push it to linux-next
> early after v4.15 to get lots of testing for the core changes.
> 
> Big kudos to Renesas Electronics for funding this work, thank you very much!
> 
> Regards,
> 
>    Wolfram

Applied to for-next after fixing some cosmetic checkpatch issues!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]