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

[Peter Rosin]

On 2018-07-08 13:58, Peter Rosin wrote:
> On 2018-07-07 11:29, Jun Gao wrote:
>> From: Jun Gao <jun.gao@xxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxxxx>");
>>  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);
>>  /**
>>
>