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

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

Signed-off-by: Wolfram Sang wsa+renesas@sang-engineering.com

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@huawei.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.

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

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