Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1667121 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] i2c: document DMA handling and add helpers for it Wolfram Sang <wsa+renesas@sang-engineering.com> - 2017-06-15 20:40 +0200
    [PATCH 1/4] i2c: add helpers to ease DMA handling Wolfram Sang <wsa+renesas@sang-engineering.com> - 2017-06-15 20:40 +0200
      Re: [PATCH 1/4] i2c: add helpers to ease DMA handling Geert Uytterhoeven <geert@linux-m68k.org> - 2017-06-15 22:40 +0200
    [PATCH 2/4] i2c: add docs to clarify DMA handling Wolfram Sang <wsa+renesas@sang-engineering.com> - 2017-06-15 20:40 +0200
      Re: [PATCH 2/4] i2c: add docs to clarify DMA handling Geert Uytterhoeven <geert@linux-m68k.org> - 2017-06-15 22:40 +0200
    Re: [PATCH 0/4] i2c: document DMA handling and add helpers for it Wolfram Sang <wsa@the-dreams.de> - 2017-06-15 20:50 +0200

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

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.

The branch can be found here:

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

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

Regards,

   Wolfram

Changes since RFC:

* the helper i2c_check_msg_for_dma() can optionally take a pointer and then it
  will attach a bounce buffer to it in case the message buffer is not DMA safe
* a second helper was added. i2c_release_bounce_buf() will copy data back to
  the message buffer and release the bounce buffer
* the documentation has been extended to match the above
* no ifdeffery around CONFIG_DMA_API_DEBUG anymore. The I2C core behaviour
  should not depend on such symbols.
* the i2c-sh_mobile driver has been updated to use bounce buffers


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 | 37 ++++++++++++++++++++
 drivers/i2c/busses/i2c-rcar.c        | 18 +++++++---
 drivers/i2c/busses/i2c-sh_mobile.c   |  8 +++--
 include/linux/i2c.h                  | 65 ++++++++++++++++++++++++++++++++++++
 4 files changed, 121 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[toc] | [next] | [standalone]

#1667124 — [PATCH 1/4] i2c: add helpers to ease DMA handling

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>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 include/linux/i2c.h | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 72d0ece70ed30d..7204d38eaff07c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -33,7 +33,10 @@
 #include <linux/rtmutex.h>
 #include <linux/irqdomain.h>		/* for Host Notify IRQ */
 #include <linux/of.h>		/* for struct device_node */
+#include <linux/slab.h>
 #include <linux/swab.h>		/* for swab16 */
+#include <linux/mm.h>
+#include <linux/sched/task_stack.h>
 #include <uapi/linux/i2c.h>
 
 extern struct bus_type i2c_bus_type;
@@ -764,6 +767,68 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
 	return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
 }
 
+/**
+ * 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_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
+ *
+ * Note: This function should only be called from process context! It uses
+ * helper functions which work on the 'current' task.
+ */
+static inline 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;
+}
+
+/**
+ * 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.
+ */
+static inline void i2c_release_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);
+}
+
 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

[toc] | [prev] | [next] | [standalone]

#1667245 — Re: [PATCH 1/4] i2c: add helpers to ease DMA handling

Hi Wolfram,

On Thu, Jun 15, 2017 at 8:30 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.

> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -764,6 +767,68 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
>         return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
>  }
>
> +/**
> + * 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_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
> + *
> + * Note: This function should only be called from process context! It uses
> + * helper functions which work on the 'current' task.
> + */
> +static inline int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
> +                                       u8 **ptr_for_bounce_buf)

__must_check?

Isn't this function a bit large to be inlined?

> +/**
> + * 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.
> + */
> +static inline void i2c_release_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)

This one is smaller.

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

[toc] | [prev] | [next] | [standalone]

#1667127 — [PATCH 2/4] i2c: add docs to clarify DMA handling

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 Documentation/i2c/DMA-considerations | 37 ++++++++++++++++++++++++++++++++++++
 1 file changed, 37 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..92cdb6835ccf95
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,37 @@
+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 other one releases the bounce buffer ensuring data is copied back to the
+message:
+
+	i2c_release_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.
+
+The bounce buffer handling from the core is generic and simple. It will always
+allocate a new buffer. If you want a more sophisticated buffer handling (e.g.
+reusing pre-allocated buffers), you can skip the generic handling from the core
+and implement your own.
+
+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

[toc] | [prev] | [next] | [standalone]

#1667247 — Re: [PATCH 2/4] i2c: add docs to clarify DMA handling

Hi Wolfram,

On Thu, Jun 15, 2017 at 8:30 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,37 @@
> +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);

Obviously the return value must be checked before proceeding.

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

[toc] | [prev] | [next] | [standalone]

#1667136

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jun 15, 2017 at 08:30:35PM +0200, Wolfram Sang wrote:
> 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.
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma
> 
> And big kudos to Renesas Electronics for funding this work, thank you very much!

I forgot to write that those 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

And sorry for the doubled Signed-offs below the patches. My mistake
here, there should be only the ones from sang-engineering.com.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web