[PATCH v3 0/4] i2c: document DMA handling and add helpers for it

[PATCH v3 0/4] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

So, after revisiting old mail threads and taking part in a similar discussion
on the USB list, 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 2
for further details.

All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W) and
a Renesas Lager board (r8a7790/H2). A more detailed test description can be
found here: http://elinux.org/Tests:I2C-core-DMA

The branch can be found here:

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

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

Regards,

   Wolfram


Changes since v2:

* rebased to v4.13-rc1
* helper functions are not inlined anymore but moved to i2c core
* __must_check has been added to the buffer check helper
* the release function has been renamed to contain 'dma' as well
* documentation updates. Hopefully better wording now
* removed the doubled Signed-offs
* adding more potentially interested parties to CC


Wolfram Sang (4):
  i2c: add helpers to ease DMA handling
  i2c: add docs to clarify DMA handling
  i2c: sh_mobile: use helper to decide if DMA is useful
  i2c: rcar: check for DMA-capable buffers

 Documentation/i2c/DMA-considerations | 38 ++++++++++++++++++++
 drivers/i2c/busses/i2c-rcar.c        | 18 +++++++---
 drivers/i2c/busses/i2c-sh_mobile.c   |  8 +++--
 drivers/i2c/i2c-core-base.c          | 68 ++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h                  |  5 +++
 5 files changed, 130 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[PATCH v3 1/4] 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.

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
Changes since v2:

* rebased to v4.13-rc1
* helper functions are not inlined anymore but moved to i2c core
* __must_check has been added to the buffer check helper
* the release function has been renamed to contain 'dma' as well

 drivers/i2c/i2c-core-base.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  5 ++++
 2 files changed, 73 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index c89dac7fd2e7b7..7326a9d2e4eb69 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -34,6 +34,7 @@
 #include <linux/irqflags.h>
 #include <linux/jump_label.h>
 #include <linux/kernel.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
@@ -44,6 +45,7 @@
 #include <linux/pm_wakeirq.h>
 #include <linux/property.h>
 #include <linux/rwsem.h>
+#include <linux/sched/task_stack.h>
 #include <linux/slab.h>
 
 #include "i2c-core.h"
@@ -2240,6 +2242,72 @@ void i2c_put_adapter(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL(i2c_put_adapter);
 
+/**
+ * i2c_check_msg_for_dma() - check if a message is suitable for DMA
+ * @msg: the message to be checked
+ * @threshold: the amount of byte from which using DMA makes sense
+ * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this
+ *			ptr, if needed. The bounce buffer must be freed by the
+ *			caller using i2c_release_dma_bounce_buf().
+ *
+ * Return: -ERANGE if message is smaller than threshold
+ *	   -EFAULT if message buffer is not DMA capable and no bounce buffer
+ *		   was requested
+ *	   -ENOMEM if a bounce buffer could not be created
+ *	   0 if message is suitable for DMA
+ *
+ * The return value must be checked.
+ *
+ * Note: This function should only be called from process context! It uses
+ * helper functions which work on the 'current' task.
+ */
+int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
+					u8 **ptr_for_bounce_buf)
+{
+	if (ptr_for_bounce_buf)
+		*ptr_for_bounce_buf = NULL;
+
+	if (msg->len < threshold)
+		return -ERANGE;
+
+	if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
+		pr_debug("msg buffer to 0x%04x is not DMA safe%s\n", msg->addr,
+			 ptr_for_bounce_buf ? ", trying bounce buffer" : "");
+		if (ptr_for_bounce_buf) {
+			if (msg->flags & I2C_M_RD)
+				*ptr_for_bounce_buf = kzalloc(msg->len, GFP_KERNEL);
+			else
+				*ptr_for_bounce_buf = kmemdup(msg->buf, msg->len,
+							      GFP_KERNEL);
+			if (!*ptr_for_bounce_buf)
+				return -ENOMEM;
+		} else {
+			return -EFAULT;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_check_msg_for_dma);
+
+/**
+ * i2c_release_bounce_buf - copy data back from bounce buffer and release it
+ * @msg: the message to be copied back to
+ * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma().
+ *		May be NULL.
+ */
+void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)
+{
+	if (!bounce_buf)
+		return;
+
+	if (msg->flags & I2C_M_RD)
+		memcpy(msg->buf, bounce_buf, msg->len);
+
+	kfree(bounce_buf);
+}
+EXPORT_SYMBOL_GPL(i2c_release_bounce_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 00ca5b86a753f8..ac02287b6c0d8f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -766,6 +766,11 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
 	return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
 }
 
+int __must_check i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
+					u8 **ptr_for_bounce_buf);
+
+void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_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

Re: [PATCH v3 1/4] i2c: add helpers to ease DMA handling

[Niklas Söderlund]

Hi Wolfram,

On 2017-07-18 12:23:36 +0200, Wolfram Sang wrote:
> 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.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
> Changes since v2:
> 
> * rebased to v4.13-rc1
> * helper functions are not inlined anymore but moved to i2c core
> * __must_check has been added to the buffer check helper
> * the release function has been renamed to contain 'dma' as well
> 
>  drivers/i2c/i2c-core-base.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h         |  5 ++++
>  2 files changed, 73 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index c89dac7fd2e7b7..7326a9d2e4eb69 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -34,6 +34,7 @@
>  #include <linux/irqflags.h>
>  #include <linux/jump_label.h>
>  #include <linux/kernel.h>
> +#include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> @@ -44,6 +45,7 @@
>  #include <linux/pm_wakeirq.h>
>  #include <linux/property.h>
>  #include <linux/rwsem.h>
> +#include <linux/sched/task_stack.h>
>  #include <linux/slab.h>
>  
>  #include "i2c-core.h"
> @@ -2240,6 +2242,72 @@ void i2c_put_adapter(struct i2c_adapter *adap)
>  }
>  EXPORT_SYMBOL(i2c_put_adapter);
>  
> +/**
> + * i2c_check_msg_for_dma() - check if a message is suitable for DMA
> + * @msg: the message to be checked
> + * @threshold: the amount of byte from which using DMA makes sense
> + * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this
> + *			ptr, if needed. The bounce buffer must be freed by the
> + *			caller using i2c_release_dma_bounce_buf().
> + *
> + * Return: -ERANGE if message is smaller than threshold
> + *	   -EFAULT if message buffer is not DMA capable and no bounce buffer
> + *		   was requested
> + *	   -ENOMEM if a bounce buffer could not be created
> + *	   0 if message is suitable for DMA
> + *
> + * The return value must be checked.
> + *
> + * Note: This function should only be called from process context! It uses
> + * helper functions which work on the 'current' task.
> + */
> +int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
> +					u8 **ptr_for_bounce_buf)
> +{
> +	if (ptr_for_bounce_buf)
> +		*ptr_for_bounce_buf = NULL;
> +
> +	if (msg->len < threshold)
> +		return -ERANGE;
> +
> +	if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
> +		pr_debug("msg buffer to 0x%04x is not DMA safe%s\n", msg->addr,
> +			 ptr_for_bounce_buf ? ", trying bounce buffer" : "");
> +		if (ptr_for_bounce_buf) {
> +			if (msg->flags & I2C_M_RD)
> +				*ptr_for_bounce_buf = kzalloc(msg->len, GFP_KERNEL);
> +			else
> +				*ptr_for_bounce_buf = kmemdup(msg->buf, msg->len,
> +							      GFP_KERNEL);
> +			if (!*ptr_for_bounce_buf)
> +				return -ENOMEM;
> +		} else {
> +			return -EFAULT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i2c_check_msg_for_dma);
> +
> +/**
> + * i2c_release_bounce_buf - copy data back from bounce buffer and release it
> + * @msg: the message to be copied back to
> + * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma().
> + *		May be NULL.
> + */
> +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)
> +{
> +	if (!bounce_buf)
> +		return;
> +
> +	if (msg->flags & I2C_M_RD)
> +		memcpy(msg->buf, bounce_buf, msg->len);
> +
> +	kfree(bounce_buf);
> +}
> +EXPORT_SYMBOL_GPL(i2c_release_bounce_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 00ca5b86a753f8..ac02287b6c0d8f 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -766,6 +766,11 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
>  	return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
>  }
>  
> +int __must_check i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
> +					u8 **ptr_for_bounce_buf);
> +
> +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_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
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 1/4] i2c: add helpers to ease DMA handling

[Jonathan Cameron]

On Tue, 18 Jul 2017 12:23:36 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> 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.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
My knowledge of the exact requirements for dma on all platforms isn't
all that good.  Mostly it's based around the assumption that if one
forces a heap allocation and makes sure nothing else is in the
cacheline all is good.

I like the basic idea of this patch set a lot btw!

Jonathan
> ---
> Changes since v2:
> 
> * rebased to v4.13-rc1
> * helper functions are not inlined anymore but moved to i2c core
> * __must_check has been added to the buffer check helper
> * the release function has been renamed to contain 'dma' as well
> 
>  drivers/i2c/i2c-core-base.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h         |  5 ++++
>  2 files changed, 73 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index c89dac7fd2e7b7..7326a9d2e4eb69 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -34,6 +34,7 @@
>  #include <linux/irqflags.h>
>  #include <linux/jump_label.h>
>  #include <linux/kernel.h>
> +#include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> @@ -44,6 +45,7 @@
>  #include <linux/pm_wakeirq.h>
>  #include <linux/property.h>
>  #include <linux/rwsem.h>
> +#include <linux/sched/task_stack.h>
>  #include <linux/slab.h>
>  
>  #include "i2c-core.h"
> @@ -2240,6 +2242,72 @@ void i2c_put_adapter(struct i2c_adapter *adap)
>  }
>  EXPORT_SYMBOL(i2c_put_adapter);
>  
> +/**
> + * i2c_check_msg_for_dma() - check if a message is suitable for DMA
> + * @msg: the message to be checked
> + * @threshold: the amount of byte from which using DMA makes sense
> + * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this
> + *			ptr, if needed. The bounce buffer must be freed by the
> + *			caller using i2c_release_dma_bounce_buf().
> + *
> + * Return: -ERANGE if message is smaller than threshold
> + *	   -EFAULT if message buffer is not DMA capable and no bounce buffer
> + *		   was requested
> + *	   -ENOMEM if a bounce buffer could not be created
> + *	   0 if message is suitable for DMA
> + *
> + * The return value must be checked.
> + *
> + * Note: This function should only be called from process context! It uses
> + * helper functions which work on the 'current' task.
> + */
> +int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
> +					u8 **ptr_for_bounce_buf)
> +{
> +	if (ptr_for_bounce_buf)
> +		*ptr_for_bounce_buf = NULL;
> +
> +	if (msg->len < threshold)
> +		return -ERANGE;
> +
> +	if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
> +		pr_debug("msg buffer to 0x%04x is not DMA safe%s\n", msg->addr,
> +			 ptr_for_bounce_buf ? ", trying bounce buffer" : "");
> +		if (ptr_for_bounce_buf) {
> +			if (msg->flags & I2C_M_RD)
> +				*ptr_for_bounce_buf = kzalloc(msg->len, GFP_KERNEL);
> +			else
> +				*ptr_for_bounce_buf = kmemdup(msg->buf, msg->len,
> +							      GFP_KERNEL);
> +			if (!*ptr_for_bounce_buf)
> +				return -ENOMEM;
> +		} else {
> +			return -EFAULT;
> +		}
> +	}
I'm trying to get my head around whether this is a sufficient set of conditions
for a dma safe buffer and I'm not sure it is.

We go to a lot of effort with SPI buffers to ensure they are in their own cache
lines.  Do we not need to do the same here?  The issue would be the
classic case of embedding a buffer inside a larger structure which is on
the stack.  Need the magic of __cacheline_aligned to force it into it's
own line for devices with dma-incoherent caches.
DMA-API-HOWTO.txt (not read that for a while at it is a rather good
at describing this stuff these days!)

So that one is easy enough to check by checking if it is cache line aligned,
but what do you do to know there is nothing much after it?

Not sure there is a way around this short of auditing i2c drivers to
change any that do this.
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i2c_check_msg_for_dma);
> +
> +/**
> + * i2c_release_bounce_buf - copy data back from bounce buffer and release it
> + * @msg: the message to be copied back to
> + * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma().
> + *		May be NULL.
> + */
> +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)
> +{
> +	if (!bounce_buf)
> +		return;
> +
> +	if (msg->flags & I2C_M_RD)
> +		memcpy(msg->buf, bounce_buf, msg->len);
> +
> +	kfree(bounce_buf);
> +}
> +EXPORT_SYMBOL_GPL(i2c_release_bounce_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 00ca5b86a753f8..ac02287b6c0d8f 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -766,6 +766,11 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
>  	return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
>  }
>  
> +int __must_check i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
> +					u8 **ptr_for_bounce_buf);
> +
> +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_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

Re: [PATCH v3 1/4] i2c: add helpers to ease DMA handling

[Wolfram Sang]

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

Hi Jonathan,

> I like the basic idea of this patch set a lot btw!

Thanks!

> Jonathan

Could you delete irrelevant parts of the message, please? I nearly
missed your other comment which would have been a great loss!

> I'm trying to get my head around whether this is a sufficient set of conditions
> for a dma safe buffer and I'm not sure it is.
> 
> We go to a lot of effort with SPI buffers to ensure they are in their own cache
> lines.  Do we not need to do the same here?  The issue would be the
> classic case of embedding a buffer inside a larger structure which is on
> the stack.  Need the magic of __cacheline_aligned to force it into it's
> own line for devices with dma-incoherent caches.
> DMA-API-HOWTO.txt (not read that for a while at it is a rather good
> at describing this stuff these days!)
> 
> So that one is easy enough to check by checking if it is cache line aligned,
> but what do you do to know there is nothing much after it?

Thank you, really!

This patch series addressed all cases I have created, but I also
wondered if there are cases left which I missed. Now, also because you
mentioned cacheline alignments, the faint idea of "maybe it could be
fragile" became a strong "it is too fragile"! lib/dma-debug.c is >40KB
for a reason. I just rewrote this series...

> Not sure there is a way around this short of auditing i2c drivers to
> change any that do this.

... to do something like this, more precisely: an opt-in approach. I
introduce a new flag for i2c_msg, namely I2C_M_DMA_SAFE which can opt-in
DMA if we know the i2c_msg buffer is DMA safe. I finished a
proof-of-concept this evening and pushed it here:

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

I will test it on HW tomorrow and send it out, then. There are still
some bits missing, but for getting opinions, it should be good enough.

Thanks and kind regards,

   Wolfram


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

Re: [PATCH v3 1/4] i2c: add helpers to ease DMA handling

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Jul 18, 2017 at 12:23 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> 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.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> Changes since v2:
>
> * rebased to v4.13-rc1
> * helper functions are not inlined anymore but moved to i2c core
> * __must_check has been added to the buffer check helper
> * the release function has been renamed to contain 'dma' as well

Right:

drivers/i2c/i2c-core-base.c:2310:15: error: 'i2c_release_bounce_buf'
undeclared here (not in a function)
 EXPORT_SYMBOL_GPL(i2c_release_bounce_buf);

> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c

> +/**
> + * i2c_release_bounce_buf - copy data back from bounce buffer and release it
      ^^^^^^^^^^^^^^^^^^^^^^
> + * @msg: the message to be copied back to
> + * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma().
> + *             May be NULL.
> + */
> +void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)
> +{
> +       if (!bounce_buf)
> +               return;
> +
> +       if (msg->flags & I2C_M_RD)
> +               memcpy(msg->buf, bounce_buf, msg->len);
> +
> +       kfree(bounce_buf);
> +}
> +EXPORT_SYMBOL_GPL(i2c_release_bounce_buf);
                     ^^^^^^^^^^^^^^^^^^^^^^

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/4] i2c: add helpers to ease DMA handling

[Wolfram Sang]

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


> Right:
> 
> drivers/i2c/i2c-core-base.c:2310:15: error: 'i2c_release_bounce_buf'
> undeclared here (not in a function)
>  EXPORT_SYMBOL_GPL(i2c_release_bounce_buf);

Thanks. I am just now working on V4 currently which is a redesign.
I'll write more in an hour or so.


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

[PATCH v3 2/4] i2c: add docs to clarify DMA handling

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since v2:

* documentation updates. Hopefully better wording now

 Documentation/i2c/DMA-considerations | 38 ++++++++++++++++++++++++++++++++++++
 1 file changed, 38 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..e46c24d65c8556
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,38 @@
+Linux I2C and DMA
+-----------------
+
+Given that I2C is a low-speed bus where largely small messages are transferred,
+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.
+
+To support this scenario, drivers wishing to implement DMA can use helper
+functions from the I2C core. One checks if a message is DMA capable in terms of
+size and memory type. It can optionally also create a bounce buffer:
+
+	i2c_check_msg_for_dma(msg, threshold, &bounce_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 can leave the pointer to the bounce buffer
+empty and implement your own handling based on the return value of the above
+function.
+
+The other helper function releases the bounce buffer. It ensures data is copied
+back to the message:
+
+	i2c_release_dma_bounce_buf(msg, bounce_buf);
+
+Please check the in-kernel documentation for details. The i2c-sh_mobile driver
+can be used as a reference example.
+
+If you plan to use DMA with I2C (or with any other bus, 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

Re: [PATCH v3 2/4] i2c: add docs to clarify DMA handling

[Niklas Söderlund]

Hi Wolfram,

On 2017-07-18 12:23:37 +0200, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
> Changes since v2:
> 
> * documentation updates. Hopefully better wording now
> 
>  Documentation/i2c/DMA-considerations | 38 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 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..e46c24d65c8556
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,38 @@
> +Linux I2C and DMA
> +-----------------
> +
> +Given that I2C is a low-speed bus where largely small messages are transferred,
> +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.
> +
> +To support this scenario, drivers wishing to implement DMA can use helper
> +functions from the I2C core. One checks if a message is DMA capable in terms of
> +size and memory type. It can optionally also create a bounce buffer:
> +
> +	i2c_check_msg_for_dma(msg, threshold, &bounce_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 can leave the pointer to the bounce buffer
> +empty and implement your own handling based on the return value of the above
> +function.
> +
> +The other helper function releases the bounce buffer. It ensures data is copied
> +back to the message:
> +
> +	i2c_release_dma_bounce_buf(msg, bounce_buf);
> +
> +Please check the in-kernel documentation for details. The i2c-sh_mobile driver
> +can be used as a reference example.
> +
> +If you plan to use DMA with I2C (or with any other bus, 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
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 2/4] i2c: add docs to clarify DMA handling

[Jonathan Cameron]

On Tue, 18 Jul 2017 12:23:37 +0200
Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org> wrote:

> Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
Is this material not perhaps better placed in the sphinx docs?
Up to you of course as your subsystem ;)

Text is good though.

Acked-by: Jonathan Cameron <Jonathan.Cameron-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> Changes since v2:
> 
> * documentation updates. Hopefully better wording now
> 
>  Documentation/i2c/DMA-considerations | 38 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 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..e46c24d65c8556
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,38 @@
> +Linux I2C and DMA
> +-----------------
> +
> +Given that I2C is a low-speed bus where largely small messages are transferred,
> +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.
> +
> +To support this scenario, drivers wishing to implement DMA can use helper
> +functions from the I2C core. One checks if a message is DMA capable in terms of
> +size and memory type. It can optionally also create a bounce buffer:
> +
> +	i2c_check_msg_for_dma(msg, threshold, &bounce_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 can leave the pointer to the bounce buffer
> +empty and implement your own handling based on the return value of the above
> +function.
> +
> +The other helper function releases the bounce buffer. It ensures data is copied
> +back to the message:
> +
> +	i2c_release_dma_bounce_buf(msg, bounce_buf);
> +
> +Please check the in-kernel documentation for details. The i2c-sh_mobile driver
> +can be used as a reference example.
> +
> +If you plan to use DMA with I2C (or with any other bus, 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.

[PATCH v3 3/4] i2c: sh_mobile: use helper to decide if DMA is useful

[Wolfram Sang]

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

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 2e097d97d258bc..19f45bcd9b35ca 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 *bounce_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_bounce_buf(pd->msg, pd->bounce_buf);
+
 	iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
 }
 
@@ -595,6 +598,7 @@ static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
 	struct dma_async_tx_descriptor *txdesc;
 	dma_addr_t dma_addr;
 	dma_cookie_t cookie;
+	u8 *dma_buf = pd->bounce_buf ?: pd->msg->buf;
 
 	if (PTR_ERR(chan) == -EPROBE_DEFER) {
 		if (read)
@@ -608,7 +612,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, 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 +669,7 @@ 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)
+	if (i2c_check_msg_for_dma(pd->msg, 8, &pd->bounce_buf) == 0)
 		sh_mobile_i2c_xfer_dma(pd);
 
 	/* Enable all interrupts to begin with */
-- 
2.11.0

Re: [PATCH v3 3/4] i2c: sh_mobile: use helper to decide if DMA is useful

[Niklas Söderlund]

Hi Wolfram,

On 2017-07-18 12:23:38 +0200, Wolfram Sang wrote:
> This ensures that we fall back to PIO if the buffer is too small for DMA
> being useful. Otherwise, we use DMA. A bounce buffer might be applied if
> the original message buffer is not DMA safe
> 
> 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 2e097d97d258bc..19f45bcd9b35ca 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 *bounce_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_bounce_buf(pd->msg, pd->bounce_buf);
> +
>  	iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
>  }
>  
> @@ -595,6 +598,7 @@ static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
>  	struct dma_async_tx_descriptor *txdesc;
>  	dma_addr_t dma_addr;
>  	dma_cookie_t cookie;
> +	u8 *dma_buf = pd->bounce_buf ?: pd->msg->buf;

This looked funny and I had to look it up, I learnt something new today 
:-)

>  
>  	if (PTR_ERR(chan) == -EPROBE_DEFER) {
>  		if (read)
> @@ -608,7 +612,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, 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 +669,7 @@ 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)
> +	if (i2c_check_msg_for_dma(pd->msg, 8, &pd->bounce_buf) == 0)

Maybe the 8 should be declared in a define to explain the value, like 
you do in patch 4/4?

This nitpick aside:

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

>  		sh_mobile_i2c_xfer_dma(pd);
>  
>  	/* Enable all interrupts to begin with */
> -- 
> 2.11.0
> 

-- 
Regards,
Niklas Söderlund

[PATCH v3 4/4] i2c: rcar: check for DMA-capable buffers

[Wolfram Sang]

Handling this is special for this driver. Because the hardware needs to
initialize the next message in interrupt context, we cannot use the
i2c_check_msg_for_dma() directly. This helper only works reliably in
process context. So, we need to check during initial preparation of the
whole transfer and need to disable DMA completely for the whole transfer
once a message with a not-DMA-capable buffer is found.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 93c1a54981df08..5d0e820d708853 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -111,8 +111,11 @@
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
 /* persistent flags */
+#define ID_P_NODMA	(1 << 30)
 #define ID_P_PM_BLOCKED	(1 << 31)
-#define ID_P_MASK	ID_P_PM_BLOCKED
+#define ID_P_MASK	(ID_P_PM_BLOCKED | ID_P_NODMA)
+
+#define RCAR_DMA_THRESHOLD 8
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
@@ -358,8 +361,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
 	unsigned char *buf;
 	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 < RCAR_DMA_THRESHOLD || priv->flags & ID_P_NODMA)
 		return;
 
 	if (read) {
@@ -657,11 +659,15 @@ static void rcar_i2c_request_dma(struct rcar_i2c_priv *priv,
 				 struct i2c_msg *msg)
 {
 	struct device *dev = rcar_i2c_priv_to_dev(priv);
-	bool read;
+	bool read = msg->flags & I2C_M_RD;
 	struct dma_chan *chan;
 	enum dma_transfer_direction dir;
 
-	read = msg->flags & I2C_M_RD;
+	/* we need to check here because we need the 'current' context */
+	if (i2c_check_msg_for_dma(msg, RCAR_DMA_THRESHOLD, NULL) == -EFAULT) {
+		dev_dbg(dev, "skipping DMA for this whole transfer\n");
+		priv->flags |= ID_P_NODMA;
+	}
 
 	chan = read ? priv->dma_rx : priv->dma_tx;
 	if (PTR_ERR(chan) != -EPROBE_DEFER)
@@ -740,6 +746,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 	if (ret < 0 && ret != -ENXIO)
 		dev_err(dev, "error %d : %x\n", ret, priv->flags);
 
+	priv->flags &= ~ID_P_NODMA;
+
 	return ret;
 }
 
-- 
2.11.0

Re: [PATCH v3 4/4] i2c: rcar: check for DMA-capable buffers

[Niklas Söderlund]

Hi Wolfram,

On 2017-07-18 12:23:39 +0200, Wolfram Sang wrote:
> Handling this is special for this driver. Because the hardware needs to
> initialize the next message in interrupt context, we cannot use the
> i2c_check_msg_for_dma() directly. This helper only works reliably in
> process context. So, we need to check during initial preparation of the
> whole transfer and need to disable DMA completely for the whole transfer
> once a message with a not-DMA-capable buffer is found.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 93c1a54981df08..5d0e820d708853 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -111,8 +111,11 @@
>  #define ID_ARBLOST	(1 << 3)
>  #define ID_NACK		(1 << 4)
>  /* persistent flags */
> +#define ID_P_NODMA	(1 << 30)
>  #define ID_P_PM_BLOCKED	(1 << 31)
> -#define ID_P_MASK	ID_P_PM_BLOCKED
> +#define ID_P_MASK	(ID_P_PM_BLOCKED | ID_P_NODMA)
> +
> +#define RCAR_DMA_THRESHOLD 8
>  
>  enum rcar_i2c_type {
>  	I2C_RCAR_GEN1,
> @@ -358,8 +361,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
>  	unsigned char *buf;
>  	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 < RCAR_DMA_THRESHOLD || priv->flags & ID_P_NODMA)
>  		return;
>  
>  	if (read) {
> @@ -657,11 +659,15 @@ static void rcar_i2c_request_dma(struct rcar_i2c_priv *priv,
>  				 struct i2c_msg *msg)
>  {
>  	struct device *dev = rcar_i2c_priv_to_dev(priv);
> -	bool read;
> +	bool read = msg->flags & I2C_M_RD;
>  	struct dma_chan *chan;
>  	enum dma_transfer_direction dir;
>  
> -	read = msg->flags & I2C_M_RD;
> +	/* we need to check here because we need the 'current' context */

Maybe extend the comment here explaining that the check is primary here 
to make sure the msg->buf is valid for DMA and that the bounce buffer 
can't be used due to the special hardware feature? Else someone might be 
tempted to try and enable the bounce buffer feature in the future?

Nitpicking aside:

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> +	if (i2c_check_msg_for_dma(msg, RCAR_DMA_THRESHOLD, NULL) == -EFAULT) {
> +		dev_dbg(dev, "skipping DMA for this whole transfer\n");
> +		priv->flags |= ID_P_NODMA;
> +	}
>  
>  	chan = read ? priv->dma_rx : priv->dma_tx;
>  	if (PTR_ERR(chan) != -EPROBE_DEFER)
> @@ -740,6 +746,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>  	if (ret < 0 && ret != -ENXIO)
>  		dev_err(dev, "error %d : %x\n", ret, priv->flags);
>  
> +	priv->flags &= ~ID_P_NODMA;
> +
>  	return ret;
>  }
>  
> -- 
> 2.11.0
> 

-- 
Regards,
Niklas Söderlund