[v2,2/3] i2c: Add helper to ease DMA handling

Message ID 1530955795-17714-3-git-send-email-jun.gao@mediatek.com
State Superseded
Headers show
Series
  • Register i2c adapter driver earlier and use DMA safe buffers
Related show

Commit Message

Jun Gao July 7, 2018, 9:29 a.m.
From: Jun Gao <jun.gao@mediatek.com>

This function is needed by i2c_get_dma_safe_msg_buf() potentially.
It is used to free DMA safe buffer when DMA operation fails.

Signed-off-by: Jun Gao <jun.gao@mediatek.com>
---
 drivers/i2c/i2c-core-base.c | 14 ++++++++++++++
 include/linux/i2c.h         |  1 +
 2 files changed, 15 insertions(+)

Comments

Peter Rosin July 8, 2018, 11:58 a.m. | #1
On 2018-07-07 11:29, Jun Gao wrote:
> From: Jun Gao <jun.gao@mediatek.com>
> 
> This function is needed by i2c_get_dma_safe_msg_buf() potentially.
> It is used to free DMA safe buffer when DMA operation fails.
> 
> Signed-off-by: Jun Gao <jun.gao@mediatek.com>
> ---
>  drivers/i2c/i2c-core-base.c | 14 ++++++++++++++
>  include/linux/i2c.h         |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 31d16ad..2b518ea 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2288,6 +2288,20 @@ void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
>  }
>  EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
>  
> +/**
> + * i2c_free_dma_safe_msg_buf - free DMA safe buffer
> + * @msg: the message related to DMA safe buffer
> + * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
> + */
> +void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
> +{
> +	if (!buf || buf == msg->buf)
> +		return;
> +
> +	kfree(buf);

Considering that the i2c-core-smbus.c file does the following for its
DMA safe buffers...

static void i2c_smbus_try_get_dmabuf(struct i2c_msg *msg, u8 init_val)
{
	bool is_read = msg->flags & I2C_M_RD;
	unsigned char *dma_buf;

	dma_buf = kzalloc(I2C_SMBUS_BLOCK_MAX + (is_read ? 2 : 3), GFP_KERNEL);
	if (!dma_buf)
		return;

	msg->buf = dma_buf;
	msg->flags |= I2C_M_DMA_SAFE;

	if (init_val)
		msg->buf[0] = init_val;
}

...I do not think your variant of i2c_release_dma_safe_msg_buf is
appropriate for the i2c-core-base.c file. It's simply not possible to have
central knowledge of the rules for when to free the buffer, and encoding one
set of rules is only confusing (when there are more than one set of rules).
I suggest that you make your variant driver specific.

Cheers,
Peter

> +}
> +EXPORT_SYMBOL_GPL(i2c_free_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 254cd34..6d62f93 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -860,6 +860,7 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
>  
>  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);
> +void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
>  
>  int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
>  /**
>
Peter Rosin July 8, 2018, 12:46 p.m. | #2
On 2018-07-08 13:58, Peter Rosin wrote:
> On 2018-07-07 11:29, Jun Gao wrote:
>> From: Jun Gao <jun.gao@mediatek.com>
>>
>> This function is needed by i2c_get_dma_safe_msg_buf() potentially.
>> It is used to free DMA safe buffer when DMA operation fails.
>>
>> Signed-off-by: Jun Gao <jun.gao@mediatek.com>
>> ---
>>  drivers/i2c/i2c-core-base.c | 14 ++++++++++++++
>>  include/linux/i2c.h         |  1 +
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 31d16ad..2b518ea 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -2288,6 +2288,20 @@ void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
>>  }
>>  EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
>>  
>> +/**
>> + * i2c_free_dma_safe_msg_buf - free DMA safe buffer
>> + * @msg: the message related to DMA safe buffer
>> + * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
>> + */
>> +void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
>> +{
>> +	if (!buf || buf == msg->buf)
>> +		return;
>> +
>> +	kfree(buf);
> 
> Considering that the i2c-core-smbus.c file does the following for its
> DMA safe buffers...
> 
> static void i2c_smbus_try_get_dmabuf(struct i2c_msg *msg, u8 init_val)
> {
> 	bool is_read = msg->flags & I2C_M_RD;
> 	unsigned char *dma_buf;
> 
> 	dma_buf = kzalloc(I2C_SMBUS_BLOCK_MAX + (is_read ? 2 : 3), GFP_KERNEL);
> 	if (!dma_buf)
> 		return;
> 
> 	msg->buf = dma_buf;
> 	msg->flags |= I2C_M_DMA_SAFE;
> 
> 	if (init_val)
> 		msg->buf[0] = init_val;
> }
> 
> ...I do not think your variant of i2c_release_dma_safe_msg_buf is
> appropriate for the i2c-core-base.c file. It's simply not possible to have
> central knowledge of the rules for when to free the buffer, and encoding one
> set of rules is only confusing (when there are more than one set of rules).
> I suggest that you make your variant driver specific.

Ignore me. Your i2c_free_dma_safe_msg_buf is of course compatible with
the existing i2c_smbus_try_get_dmabuf. Sorry for the noise.

Cheers,
Peter

( However, the naming of these two functions are not really consistent... )

> Cheers,
> Peter
> 
>> +}
>> +EXPORT_SYMBOL_GPL(i2c_free_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 254cd34..6d62f93 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -860,6 +860,7 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
>>  
>>  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);
>> +void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
>>  
>>  int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
>>  /**
>>
>
Wolfram Sang Aug. 8, 2018, 8:57 p.m. | #3
On Sat, Jul 07, 2018 at 05:29:54PM +0800, Jun Gao wrote:
> From: Jun Gao <jun.gao@mediatek.com>
> 
> This function is needed by i2c_get_dma_safe_msg_buf() potentially.
> It is used to free DMA safe buffer when DMA operation fails.
> 
> Signed-off-by: Jun Gao <jun.gao@mediatek.com>

Right, we need something like this. This leaks in the sh_mobile driver,
too :(

I am still thinking if there is a nice way to put this functionality
into i2c_release_dma_safe_msg_buf() itself somehow...

Thanks!
Yingjoe Chen Aug. 9, 2018, 9:13 a.m. | #4
On Wed, 2018-08-08 at 22:57 +0200, Wolfram Sang wrote:
> On Sat, Jul 07, 2018 at 05:29:54PM +0800, Jun Gao wrote:
> > From: Jun Gao <jun.gao@mediatek.com>
> > 
> > This function is needed by i2c_get_dma_safe_msg_buf() potentially.
> > It is used to free DMA safe buffer when DMA operation fails.
> > 
> > Signed-off-by: Jun Gao <jun.gao@mediatek.com>
> 
> Right, we need something like this. This leaks in the sh_mobile driver,
> too :(
> 
> I am still thinking if there is a nice way to put this functionality
> into i2c_release_dma_safe_msg_buf() itself somehow...

Wolfram,

I have second thought on these API now. Recently, we have saw similar
issue for spi device driver.

I believe the reason for these api is because some arch changed to can
not do DMA on stack recently. Maybe we should have dma_mapping to bounce
buffer like it bounce un-dma-able address for those arch? or we should
have a common driver API for this, not just for i2c?

Joe.C
Wolfram Sang Aug. 9, 2018, 9:23 a.m. | #5
Hi,

> I believe the reason for these api is because some arch changed to can
> not do DMA on stack recently. Maybe we should have dma_mapping to bounce

It is not only arch dependent. You can enable virtual stacks in Kconfig,
too.

> buffer like it bounce un-dma-able address for those arch? or we should
> have a common driver API for this, not just for i2c?

I gave a talk about this problem recently and everyone in the room
agreed the best thing would be to have annotated buffers which can be
used kernel-wide. Someone mentioned DMABUF could be a candidate, but I
haven't looked further into that.

So, yes, a bigger solution is needed but I don't see that coming soon,
so I implemented the I2C part for better safety now. I'd be happy to
remove all that again once we have the annotated buffers. But it is an
effort...

Regards,

   Wolfram

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 31d16ad..2b518ea 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2288,6 +2288,20 @@  void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
 }
 EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
 
+/**
+ * i2c_free_dma_safe_msg_buf - free DMA safe buffer
+ * @msg: the message related to DMA safe buffer
+ * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
+ */
+void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
+{
+	if (!buf || buf == msg->buf)
+		return;
+
+	kfree(buf);
+}
+EXPORT_SYMBOL_GPL(i2c_free_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 254cd34..6d62f93 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -860,6 +860,7 @@  static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
 
 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);
+void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
 
 int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
 /**