[RFC PATCH 0/4] i2c: document DMA handling and add helper

[RFC PATCH 0/4] i2c: document DMA handling and add helper

[Wolfram Sang]

From: Wolfram Sang <wsa@the-dreams.de>

So, after revisiting old mail threads and taking part in a similar discussion
on the USB list, here is an RFC on what I cooked together regarding DMA and I2C
in the Linux world.

I documented my reasoning with patch 2 which mentions a helper function which
gets added with patch 1. Patch 3 shows a super simple example with the
i2c-sh_mobile driver. And patch 4 shows a super complicated example with the
i2c-rcar driver. But it is nice to have such a corner case already in the
beginning of development.

Please let me know what you think. What I surely plan to add until next time is
to fix two central places in the I2C core where the data buffer for block
transfers is currently put on the stack.

A 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


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

 Documentation/i2c/DMA-considerations | 28 ++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-rcar.c        | 18 +++++++++++++-----
 drivers/i2c/busses/i2c-sh_mobile.c   |  2 +-
 include/linux/i2c.h                  | 29 +++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[RFC PATCH 1/4] i2c: add helper to determine if DMA is favoured

[Wolfram Sang]

We not only check if the buffer is DMA capable. We also require a
threshold value to see if it makes sense to setup DMA. Since most
I2C transactions are small, the cost for DMA might be too high.

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

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 72d0ece70ed30d..f0b835ba2ecd10 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -34,6 +34,8 @@
 #include <linux/irqdomain.h>		/* for Host Notify IRQ */
 #include <linux/of.h>		/* for struct device_node */
 #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 +766,33 @@ 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
+ * @dma_threshold: the amount of byte from which using DMA makes sense
+ *
+ * Return: -ERANGE if message is smaller than threshold
+ * 	   -EFAULT if message buffer is not DMA capable
+ * 	   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 dma_threshold)
+{
+	if (msg->len < dma_threshold)
+		return -ERANGE;
+
+#if !defined(CONFIG_DMA_API_DEBUG)
+	if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
+		pr_debug("msg buffer to 0x%04x might not be DMA capable\n",
+			 msg->addr);
+		return -EFAULT;
+	}
+#endif
+	return 0;
+}
+
 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

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

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/DMA-considerations | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 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..945ac5cfe55c12
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,28 @@
+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 (FIXME: > 8 byte?).
+
+To support this, drivers wishing to implement DMA can use a helper function
+checking if the size is suitable for DMA or if the buffer is DMA capable:
+
+	int i2c_check_msg_for_dma(msg, dma_threshold);
+
+Please check its in kernel documentation for details.
+
+It should be further noted that bounce buffer handling is left to be handled on
+driver level because details like alignment requirements are best known on that
+level.
+
+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: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Jun 6, 2017 at 11:20 AM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,28 @@
> +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 (FIXME: > 8 byte?).

So you expect drivers to fall back to PIO automatically if the buffer is
not DMA safe.  Sounds good to me.

> +To support this, drivers wishing to implement DMA can use a helper function
> +checking if the size is suitable for DMA or if the buffer is DMA capable:
> +
> +       int i2c_check_msg_for_dma(msg, dma_threshold);
> +
> +Please check its in kernel documentation for details.
> +
> +It should be further noted that bounce buffer handling is left to be handled on
> +driver level because details like alignment requirements are best known on that
> +level.
> +
> +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.

However, your check for a DMA-capable buffer is invoked only if
CONFIG_DMA_API_DEBUG is enabled:

    #if !defined(CONFIG_DMA_API_DEBUG)
           if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
                   pr_debug("msg buffer to 0x%04x might not be DMA capable\n",
                            msg->addr);
                   return -EFAULT;
           }
    #endif

So the system will work fine if CONFIG_DMA_API_DEBUG is enabled, and may fail
miserably in a production kernel?

Furthermore, pr_debug() messages are not printed by default, so the developer
who did enable CONFIG_DMA_API_DEBUG may not have noticed at all?

If you want to use i2c_check_msg_for_dma() as a generic helper to verify DMA
requirements, and decide when to fall back to PIO, I think it should always do
the buffer check.

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: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling

[Wolfram Sang]

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


> > +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 (FIXME: > 8 byte?).
> 
> So you expect drivers to fall back to PIO automatically if the buffer is
> not DMA safe.  Sounds good to me.

Yes, I strongly recommend that. Otherwise, drivers can always deal with
bounce buffers on their own.

> However, your check for a DMA-capable buffer is invoked only if
> CONFIG_DMA_API_DEBUG is enabled:

is *NOT* enabled!

> 
>     #if !defined(CONFIG_DMA_API_DEBUG)
>            if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
>                    pr_debug("msg buffer to 0x%04x might not be DMA capable\n",
>                             msg->addr);
>                    return -EFAULT;
>            }
>     #endif
> 

The #if block is there because DMA_API_DEBUG does a lot more, but if the
check in the helper is enabled, the core will fall back to PIO and you
won't get the additional info from DMA_API_DEBUG.

I think this needs a comment :)

Now OK?

Regards,

   Wolfram

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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Jun 6, 2017 at 12:24 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> > +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 (FIXME: > 8 byte?).
>>
>> So you expect drivers to fall back to PIO automatically if the buffer is
>> not DMA safe.  Sounds good to me.
>
> Yes, I strongly recommend that. Otherwise, drivers can always deal with
> bounce buffers on their own.
>
>> However, your check for a DMA-capable buffer is invoked only if
>> CONFIG_DMA_API_DEBUG is enabled:
>
> is *NOT* enabled!

Oops ;-)

>>     #if !defined(CONFIG_DMA_API_DEBUG)
>>            if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
>>                    pr_debug("msg buffer to 0x%04x might not be DMA capable\n",
>>                             msg->addr);
>>                    return -EFAULT;
>>            }
>>     #endif
>>
>
> The #if block is there because DMA_API_DEBUG does a lot more, but if the
> check in the helper is enabled, the core will fall back to PIO and you
> won't get the additional info from DMA_API_DEBUG.
>
> I think this needs a comment :)
>
> Now OK?

So it won't fall back to PIO if CONFIG_DMA_API_DEBUG is enabled?

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: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling

[Wolfram Sang]

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


> So it won't fall back to PIO if CONFIG_DMA_API_DEBUG is enabled?

No, it will complain loudly with more details about the same things (and
some more). But for that to do, it cannot fall back to PIO.

Otherwise, you might get the idea that all is fine since you enabled
CONFIG_DMA_API_DEBUG but miss that most of you transfers fell back to
PIO. That was my thinking...


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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Jun 6, 2017 at 12:54 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> So it won't fall back to PIO if CONFIG_DMA_API_DEBUG is enabled?
>
> No, it will complain loudly with more details about the same things (and
> some more). But for that to do, it cannot fall back to PIO.
>
> Otherwise, you might get the idea that all is fine since you enabled
> CONFIG_DMA_API_DEBUG but miss that most of you transfers fell back to
> PIO. That was my thinking...

Hmm....

I have it enabled all the time. So my devices may stop working...

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: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling

[Wolfram Sang]

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


> I have it enabled all the time. So my devices may stop working...

If you have enabled it all the time, then there won't be a difference
from now. Or?

That also shows how rarely I2C DMA transfers are triggered from within
the kernel :)


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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Jun 6, 2017 at 1:23 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> I have it enabled all the time. So my devices may stop working...
>
> If you have enabled it all the time, then there won't be a difference
> from now. Or?
>
> That also shows how rarely I2C DMA transfers are triggered from within
> the kernel :)

... how rarely i2c DMA transfers from possibly unsafe buffers...

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

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

[Wolfram Sang]

This ensures also that we fall back to PIO if the buffer is not DMA
capable.

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

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 3d75593487454c..58e2ae3b4c4751 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -666,7 +666,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) == 0)
 		sh_mobile_i2c_xfer_dma(pd);
 
 	/* Enable all interrupts to begin with */
-- 
2.11.0

[RFC PATCH 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 8be3e6cb8fe686..38985f1b45cd08 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) == -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