[RFC PATCH v5 0/6] i2c: document DMA handling and add helpers for it

[RFC PATCH v5 0/6] 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 3 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 the previous versions 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 (makes me even think of inlining them again?). The driver changes
for the Renesas IP cores became easier to understand, too. While only a tad for
the i2c-sh_mobile driver, the situation became a LOT better for the i2c-rcar
driver. No more DMA disabling for the whole transfer in case of unsafe buffers,
we are back to per-msg handling. And the code fix is now an easy to understand
one line change. Yay!

Of course, we must now whitelist DMA safe buffers. An example for I2C_RDWR case
is in this series. It makes the i2ctransfer utility have DMA_SAFE buffers,
which is nice for testing as i2cdump will (currently) not use DMA_SAFE buffers.
My plan is to add two new calls: i2c_master_{send|receive}_dma_safe which can
be used if DMA_SAFE buffers are provided. So, drivers can simply switch to
them. Also, the buffers used within i2c_smbus_xfer_emulated() need to be
converted to be DMA_SAFE which will cover a huge bunch of use cases. The rest
is then updating drivers which can be done when needed.

As these conversions are not done yet, this patch series has RFC status. But I
already would like to get opinions on this approach, so I'll cc mailing lists
of the heavier I2C users. Please let me know what you think.

All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W).

The branch can be found here:

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

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

Regards,

   Wolfram

Changes since v4:
	* rebased to v4.14-rc1
	* (hopefully) improved the docs to be more clear
	* added basic RST syntax to the docs

This is mainly an update to agree on the docs. Missing code is still missing
and will be added in v6.

Changes since v3:
	* completely redesigned


Wolfram Sang (6):
  i2c: add a message flag for DMA safe buffers
  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: skip DMA if buffer is not safe
  i2c: dev: mark RDWR buffers as DMA_SAFE

 Documentation/i2c/DMA-considerations | 58 ++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-rcar.c        |  2 +-
 drivers/i2c/busses/i2c-sh_mobile.c   |  8 +++--
 drivers/i2c/i2c-core-base.c          | 45 ++++++++++++++++++++++++++++
 drivers/i2c/i2c-dev.c                |  2 ++
 include/linux/i2c.h                  |  3 ++
 include/uapi/linux/i2c.h             |  3 ++
 7 files changed, 118 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[RFC PATCH v5 1/6] 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.

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

[RFC PATCH v5 2/6] 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@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  3 +++
 2 files changed, 48 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 56e46581b84bdb..925a22794d3ced 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2241,6 +2241,51 @@ 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 amount of byte from 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. Note that it can either be
+ *	   msg->buf or a bounce buffer. 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;
+
+	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 d501d3956f13f0..1e99342f180f45 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -767,6 +767,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

[RFC PATCH v5 3/6] i2c: add docs to clarify DMA handling

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/DMA-considerations | 58 ++++++++++++++++++++++++++++++++++++
 1 file changed, 58 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..5a63355c6a9b6f
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,58 @@
+=================
+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). 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.
+
+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.
+
+FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers for i2c_smbus_xfer_emulated.
+
+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

[RFC PATCH v5 4/6] i2c: sh_mobile: use helper to decide if DMA is useful

[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.

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 6f2aaeb7c4fa15..b76399d8a3abd3 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

[RFC PATCH v5 5/6] 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.

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

[RFC PATCH v5 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE

[Wolfram Sang]

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

Re: [RFC PATCH v5 3/6] i2c: add docs to clarify DMA handling

[Mauro Carvalho Chehab]

Em Wed, 20 Sep 2017 20:59:53 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> escreveu:

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

Documentation looks OK on my eyes. So:

Reviewed-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

> ---
>  Documentation/i2c/DMA-considerations | 58 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 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..5a63355c6a9b6f
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,58 @@
> +=================
> +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). 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.
> +
> +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.
> +
> +FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers for i2c_smbus_xfer_emulated.
> +
> +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.



Thanks,
Mauro

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

[Jonathan Cameron]

On Wed, 20 Sep 2017 20:59:52 +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>

One minor suggestion for wording. Otherwise looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/i2c/i2c-core-base.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h         |  3 +++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 56e46581b84bdb..925a22794d3ced 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2241,6 +2241,51 @@ 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 amount of byte from which using DMA makes sense

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. Note that it can either be
> + *	   msg->buf or a bounce buffer. 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;
> +
> +	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 d501d3956f13f0..1e99342f180f45 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -767,6 +767,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

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

[Jonathan Cameron]

On Wed, 20 Sep 2017 20:59:52 +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>

One minor suggestion for wording. Otherwise looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/i2c/i2c-core-base.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h         |  3 +++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 56e46581b84bdb..925a22794d3ced 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2241,6 +2241,51 @@ 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 amount of byte from which using DMA makes sense

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. Note that it can either be
> + *	   msg->buf or a bounce buffer. 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;
> +
> +	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 d501d3956f13f0..1e99342f180f45 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -767,6 +767,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

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

[Jonathan Cameron]

On Thu, 21 Sep 2017 14:59:22 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Wed, 20 Sep 2017 20:59:52 +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>  
> 
> One minor suggestion for wording. Otherwise looks good to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>

Need more coffee. See below.
 
> > ---
> >  drivers/i2c/i2c-core-base.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/i2c.h         |  3 +++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 56e46581b84bdb..925a22794d3ced 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -2241,6 +2241,51 @@ 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 amount of byte from which using DMA makes sense  
> 
> 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. Note that it can either be
> > + *	   msg->buf or a bounce buffer. 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;
> > +
> > +	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);

Only free when you actually allocated it.  Seems to me like you need
to check if (!(msg->flags & I2C_M_DMA_SAFE)) before kfree.

Otherwise the logic to do this will be needed in every driver
which will get irritating fast.


> > +}
> > +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 d501d3956f13f0..1e99342f180f45 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -767,6 +767,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  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

[Jonathan Cameron]

On Thu, 21 Sep 2017 14:59:22 +0100
Jonathan Cameron <Jonathan.Cameron-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:

> On Wed, 20 Sep 2017 20:59:52 +0200
> Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org> 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-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>  
> 
> One minor suggestion for wording. Otherwise looks good to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>

Need more coffee. See below.
 
> > ---
> >  drivers/i2c/i2c-core-base.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/i2c.h         |  3 +++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 56e46581b84bdb..925a22794d3ced 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -2241,6 +2241,51 @@ 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 amount of byte from which using DMA makes sense  
> 
> 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. Note that it can either be
> > + *	   msg->buf or a bounce buffer. 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;
> > +
> > +	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);

Only free when you actually allocated it.  Seems to me like you need
to check if (!(msg->flags & I2C_M_DMA_SAFE)) before kfree.

Otherwise the logic to do this will be needed in every driver
which will get irritating fast.


> > +}
> > +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 d501d3956f13f0..1e99342f180f45 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -767,6 +767,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  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v5 3/6] i2c: add docs to clarify DMA handling

[Jonathan Cameron]

On Wed, 20 Sep 2017 16:56:48 -0300
Mauro Carvalho Chehab <mchehab@s-opensource.com> wrote:

> Em Wed, 20 Sep 2017 20:59:53 +0200
> Wolfram Sang <wsa+renesas@sang-engineering.com> escreveu:
> 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>  
> 
> Documentation looks OK on my eyes. So:
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Really minor suggestion inline. I don't really care either way as
what you had is perfectly comprehensible. 

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> > ---
> >  Documentation/i2c/DMA-considerations | 58 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 58 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..5a63355c6a9b6f
> > --- /dev/null
> > +++ b/Documentation/i2c/DMA-considerations
> > @@ -0,0 +1,58 @@
> > +=================
> > +Linux I2C and DMA
> > +=================
> > +
> > +Given that I2C is a low-speed bus where largely small messages are transferred,

Slightly nicer as:

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.
> > +
> > +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.
> > +
> > +FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers for i2c_smbus_xfer_emulated.
> > +
> > +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.  
> 
> 
> 
> Thanks,
> Mauro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v5 3/6] i2c: add docs to clarify DMA handling

[Jonathan Cameron]

On Wed, 20 Sep 2017 16:56:48 -0300
Mauro Carvalho Chehab <mchehab@s-opensource.com> wrote:

> Em Wed, 20 Sep 2017 20:59:53 +0200
> Wolfram Sang <wsa+renesas@sang-engineering.com> escreveu:
> 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>  
> 
> Documentation looks OK on my eyes. So:
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Really minor suggestion inline. I don't really care either way as
what you had is perfectly comprehensible. 

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> > ---
> >  Documentation/i2c/DMA-considerations | 58 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 58 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..5a63355c6a9b6f
> > --- /dev/null
> > +++ b/Documentation/i2c/DMA-considerations
> > @@ -0,0 +1,58 @@
> > +=================
> > +Linux I2C and DMA
> > +=================
> > +
> > +Given that I2C is a low-speed bus where largely small messages are transferred,

Slightly nicer as:

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.
> > +
> > +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.
> > +
> > +FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers for i2c_smbus_xfer_emulated.
> > +
> > +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.  
> 
> 
> 
> Thanks,
> Mauro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

[Wolfram Sang]

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


> > > +/**
> > > + * 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);
> 
> Only free when you actually allocated it.  Seems to me like you need
> to check if (!(msg->flags & I2C_M_DMA_SAFE)) before kfree.
> 
> Otherwise the logic to do this will be needed in every driver
> which will get irritating fast.

Well, I return early if (buf == msg->buf) which is only true for
I2C_M_DMA_SAFE. If not, I allocated the buffer. Am I missing something?
It would be very strange to call this function if the caller allocated
the buffer manually.

Thanks for the review!


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

Re: [RFC PATCH v5 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE

[Jonathan Cameron]

On Wed, 20 Sep 2017 20:59:56 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

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

Makes sense as do the other drivers.

Feel free to add

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

to all of them (though they hardly took a lot of reviewing given how simple
the patches were :)

> ---
>  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

Re: [RFC PATCH v5 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE

[Jonathan Cameron]

On Wed, 20 Sep 2017 20:59:56 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

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

Makes sense as do the other drivers.

Feel free to add

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

to all of them (though they hardly took a lot of reviewing given how simple
the patches were :)

> ---
>  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

Re: [RFC PATCH v5 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE

[Wolfram Sang]

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

On Thu, Sep 21, 2017 at 03:17:44PM +0100, Jonathan Cameron wrote:
> On Wed, 20 Sep 2017 20:59:56 +0200
> Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Makes sense as do the other drivers.
> 
> Feel free to add
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> to all of them (though they hardly took a lot of reviewing given how simple
> the patches were :)

Well, bugs can slip in everywhere, so thanks for the review!


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

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

[Jonathan Cameron]

On Thu, 21 Sep 2017 16:15:28 +0200
Wolfram Sang <wsa@the-dreams.de> wrote:

> > > > +/**
> > > > + * 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);  
> > 
> > Only free when you actually allocated it.  Seems to me like you need
> > to check if (!(msg->flags & I2C_M_DMA_SAFE)) before kfree.
> > 
> > Otherwise the logic to do this will be needed in every driver
> > which will get irritating fast.  
> 
> Well, I return early if (buf == msg->buf) which is only true for
> I2C_M_DMA_SAFE. If not, I allocated the buffer. Am I missing something?
> It would be very strange to call this function if the caller allocated
> the buffer manually.
> 
> Thanks for the review!

Doh missed that check and my comment was bonkers even if it hadn't been there.
I come back to the claim of insufficient caffeine.

You are quite correct.  Please ignore previous comment - the code is
fine as is. 

Jonathan
> 
> 

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

[Jonathan Cameron]

On Thu, 21 Sep 2017 16:15:28 +0200
Wolfram Sang <wsa@the-dreams.de> wrote:

> > > > +/**
> > > > + * 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);  
> > 
> > Only free when you actually allocated it.  Seems to me like you need
> > to check if (!(msg->flags & I2C_M_DMA_SAFE)) before kfree.
> > 
> > Otherwise the logic to do this will be needed in every driver
> > which will get irritating fast.  
> 
> Well, I return early if (buf == msg->buf) which is only true for
> I2C_M_DMA_SAFE. If not, I allocated the buffer. Am I missing something?
> It would be very strange to call this function if the caller allocated
> the buffer manually.
> 
> Thanks for the review!

Doh missed that check and my comment was bonkers even if it hadn't been there.
I come back to the claim of insufficient caffeine.

You are quite correct.  Please ignore previous comment - the code is
fine as is. 

Jonathan
> 
>