i2c: at91: use dma safe buffers

  The supplied buffer might be on the stack and we get the following error
message:
[    3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc memory

Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region if
necessary.

Cc: stable@vger.kernel.org
Signed-off-by: Michael Walle <michael@walle.cc>
---

I'm not sure if or which Fixes: tag I should add to this patch. The issue
seems to be since a very long time, but nobody seem to have triggered it.
FWIW, I'm using the sff,sfp driver, which triggers this.

 drivers/i2c/busses/i2c-at91-master.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
  

Am 03.03.22 um 17:17 schrieb Michael Walle:
> The supplied buffer might be on the stack and we get the following error
> message:
> [    3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc memory
>
> Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region if
> necessary.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>
> I'm not sure if or which Fixes: tag I should add to this patch. The issue
> seems to be since a very long time, but nobody seem to have triggered it.
> FWIW, I'm using the sff,sfp driver, which triggers this.
>
>   drivers/i2c/busses/i2c-at91-master.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index b0eae94909f4..a7a22fedbaba 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -656,6 +656,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
>   	unsigned int_addr_flag = 0;
>   	struct i2c_msg *m_start = msg;
>   	bool is_read;
> +	u8 *dma_buf;

Maybe call your variable differently. DMA-buf is an inter driver buffer 
sharing frame we use for GPU acceleration and V4L.

It doesn't cause any technical issues, but the maintainer regex now 
triggers on that. So you are CCing people not related to this code in 
any way.

Regards,
Christian.

>   
>   	dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
>   
> @@ -703,7 +704,18 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
>   	dev->msg = m_start;
>   	dev->recv_len_abort = false;
>   
> +	if (dev->use_dma) {
> +		dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1);
> +		if (!dma_buf) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		dev->buf = dma_buf;
> +	}
> +
> +
>   	ret = at91_do_twi_transfer(dev);
> +	i2c_put_dma_safe_msg_buf(dma_buf, m_start, !ret);
>   
>   	ret = (ret < 0) ? ret : num;
>   out:
  

Hi Christian,

> Maybe call your variable differently. DMA-buf is an inter driver buffer
> sharing frame we use for GPU acceleration and V4L.
> 
> It doesn't cause any technical issues, but the maintainer regex now triggers
> on that. So you are CCing people not related to this code in any way.

Frankly, I think the 'dma_buf' regex is a bit too generic. 'dma_buf'
seems like a reasonable name to me if some subsystem has to deal with
different buffers which can be DMA or non-DMA, like I2C. If you git-grep
the tree, you will find it in quite some places.

We could now think of renaming the variable to 'dmabuf' but this is
a strange and kind of arbitrary rule to remember IMO.

I wonder if you'd miss a lot of patches if we remove 'dma_buf' from the
regex and keep 'dma_fence' and 'dma_resv'? Or extend it to 'dma_buf_' or
'struct dma_buf'?

All the best,

   Wolfram
  

Am 04.03.22 um 09:04 schrieb Wolfram Sang:
> Hi Christian,
>
>> Maybe call your variable differently. DMA-buf is an inter driver buffer
>> sharing frame we use for GPU acceleration and V4L.
>>
>> It doesn't cause any technical issues, but the maintainer regex now triggers
>> on that. So you are CCing people not related to this code in any way.
> Frankly, I think the 'dma_buf' regex is a bit too generic. 'dma_buf'
> seems like a reasonable name to me if some subsystem has to deal with
> different buffers which can be DMA or non-DMA, like I2C. If you git-grep
> the tree, you will find it in quite some places.
>
> We could now think of renaming the variable to 'dmabuf' but this is
> a strange and kind of arbitrary rule to remember IMO.
>
> I wonder if you'd miss a lot of patches if we remove 'dma_buf' from the
> regex and keep 'dma_fence' and 'dma_resv'? Or extend it to 'dma_buf_' or
> 'struct dma_buf'?

Yeah, I'm already considering something similar for a while.

I'm getting quite a bunch of unrelated mails because the regex is not 
the best.

On the other hand the framework is used in a lot of drivers and I do 
want to be notified when they mess with their interfaces.

Going to take a another look at that when I have time.

Thanks,
Christian.

>
> All the best,
>
>     Wolfram
>
  

> I'm getting quite a bunch of unrelated mails because the regex is not the
> best.

I can imagine!

> On the other hand the framework is used in a lot of drivers and I do want to
> be notified when they mess with their interfaces.

Sure thing. I am convinced the regex can be improved to ensure that to a
high degree. I think it is also less work for you than asking people to
rename their variable all the time :)

> Going to take a another look at that when I have time.

Thank you!
  

Am 04.03.22 um 09:43 schrieb Wolfram Sang:
>> I'm getting quite a bunch of unrelated mails because the regex is not the
>> best.
> I can imagine!
>
>> On the other hand the framework is used in a lot of drivers and I do want to
>> be notified when they mess with their interfaces.
> Sure thing. I am convinced the regex can be improved to ensure that to a
> high degree. I think it is also less work for you than asking people to
> rename their variable all the time :)

Well not all the time. It's just that you absolutely hit the nail on the 
head with the name.

The local variable for the DMA-buf handle is just nearly always named 
dma_buf or dmabuf in the drivers.

Christian.

>
>> Going to take a another look at that when I have time.
> Thank you!
>
  

Hi all,

Am 2022-03-03 17:17, schrieb Michael Walle:
> The supplied buffer might be on the stack and we get the following 
> error
> message:
> [    3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc 
> memory
> 
> Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region if
> necessary.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Walle <michael@walle.cc>

Any news here?

> ---
> 
> I'm not sure if or which Fixes: tag I should add to this patch. The 
> issue
> seems to be since a very long time, but nobody seem to have triggered 
> it.
> FWIW, I'm using the sff,sfp driver, which triggers this.

-michael
  

On 03.03.2022 18:17, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The supplied buffer might be on the stack and we get the following error
> message:
> [    3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc memory
> 
> Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region if
> necessary.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

> ---
> 
> I'm not sure if or which Fixes: tag I should add to this patch. The issue
> seems to be since a very long time, but nobody seem to have triggered it.
> FWIW, I'm using the sff,sfp driver, which triggers this.

I think it should be:
Fixes: 60937b2cdbf9 ("i2c: at91: add dma support")

> +       if (dev->use_dma) {
> +               dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1);

If you want, you could just dev->buf = i2c_get_dma_safe...

> +               if (!dma_buf) {
> +                       ret = -ENOMEM;
> +                       goto out;
> +               }
> +               dev->buf = dma_buf;

Thanks!
  

Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com:
> On 03.03.2022 18:17, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> The supplied buffer might be on the stack and we get the following 
>> error
>> message:
>> [    3.312058] at91_i2c e0070600.i2c: rejecting DMA map of vmalloc 
>> memory
>> 
>> Use i2c_{get,put}_dma_safe_msg_buf() to get a DMA-able memory region 
>> if
>> necessary.
>> 
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Thanks!

>> I'm not sure if or which Fixes: tag I should add to this patch. The 
>> issue
>> seems to be since a very long time, but nobody seem to have triggered 
>> it.
>> FWIW, I'm using the sff,sfp driver, which triggers this.
> 
> I think it should be:
> Fixes: 60937b2cdbf9 ("i2c: at91: add dma support")
> 
>> +       if (dev->use_dma) {
>> +               dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1);
> 
> If you want, you could just dev->buf = i2c_get_dma_safe...

But where is the error handling in that case? dev->buf will
be NULL, which is eventually passed to dma_map_single().

Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf()
call anyway, because dev->buf will be modified during
processing.

-michael
  

On 05.04.2022 12:38, Michael Walle wrote:
> Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com:
>>> +       if (dev->use_dma) {
>>> +               dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1);
>>
>> If you want, you could just dev->buf = i2c_get_dma_safe...
> 
> But where is the error handling in that case? dev->buf will
> be NULL, which is eventually passed to dma_map_single().
> 
> Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf()
> call anyway, because dev->buf will be modified during
> processing.

You still:
	if (!dev->buf) {
		ret = -ENOMEM;
		goto out;
	}

So, at91_do_twi_transfer()/dma_map_single() will not be called.

Best regards,
Codrin
  

Am 2022-04-05 12:02, schrieb Codrin.Ciubotariu@microchip.com:
> On 05.04.2022 12:38, Michael Walle wrote:
>> Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com:
>>>> +       if (dev->use_dma) {
>>>> +               dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1);
>>> 
>>> If you want, you could just dev->buf = i2c_get_dma_safe...
>> 
>> But where is the error handling in that case? dev->buf will
>> be NULL, which is eventually passed to dma_map_single().
>> 
>> Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf()
>> call anyway, because dev->buf will be modified during
>> processing.
> 
> You still:
> 	if (!dev->buf) {
> 		ret = -ENOMEM;
> 		goto out;
> 	}
> 
> So, at91_do_twi_transfer()/dma_map_single() will not be called.

Ahh, I misunderstood you. Yes, but as I said, I need the dma_buf
temporary variable anyway, because dev->buf is modified, eg. see
at91_twi_read_data_dma_callback().

-michael
  

On 05.04.2022 14:09, Michael Walle wrote:
> Am 2022-04-05 12:02, schrieb Codrin.Ciubotariu@microchip.com:
>> On 05.04.2022 12:38, Michael Walle wrote:
>>> Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com:
>>>>> +       if (dev->use_dma) {
>>>>> +               dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1);
>>>>
>>>> If you want, you could just dev->buf = i2c_get_dma_safe...
>>>
>>> But where is the error handling in that case? dev->buf will
>>> be NULL, which is eventually passed to dma_map_single().
>>>
>>> Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf()
>>> call anyway, because dev->buf will be modified during
>>> processing.
>>
>> You still:
>>       if (!dev->buf) {
>>               ret = -ENOMEM;
>>               goto out;
>>       }
>>
>> So, at91_do_twi_transfer()/dma_map_single() will not be called.
> 
> Ahh, I misunderstood you. Yes, but as I said, I need the dma_buf
> temporary variable anyway, because dev->buf is modified, eg. see
> at91_twi_read_data_dma_callback().
at91_twi_read_data_dma_callback() is called as callback if 
dma_async_issue_pending(dma->chan_rx) is called. 
dma_async_issue_pending(dma->chan_rx) is called on 
at91_twi_read_data_dma(), which is called in at91_do_twi_transfer(), 
which we decided above to skip in case of error.
  

Am 2022-04-05 15:58, schrieb Codrin.Ciubotariu@microchip.com:
> On 05.04.2022 14:09, Michael Walle wrote:
>> Am 2022-04-05 12:02, schrieb Codrin.Ciubotariu@microchip.com:
>>> On 05.04.2022 12:38, Michael Walle wrote:
>>>> Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com:
>>>>>> +       if (dev->use_dma) {
>>>>>> +               dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1);
>>>>> 
>>>>> If you want, you could just dev->buf = i2c_get_dma_safe...
>>>> 
>>>> But where is the error handling in that case? dev->buf will
>>>> be NULL, which is eventually passed to dma_map_single().
>>>> 
>>>> Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf()
>>>> call anyway, because dev->buf will be modified during
>>>> processing.
>>> 
>>> You still:
>>>       if (!dev->buf) {
>>>               ret = -ENOMEM;
>>>               goto out;
>>>       }
>>> 
>>> So, at91_do_twi_transfer()/dma_map_single() will not be called.
>> 
>> Ahh, I misunderstood you. Yes, but as I said, I need the dma_buf
>> temporary variable anyway, because dev->buf is modified, eg. see
>> at91_twi_read_data_dma_callback().
> at91_twi_read_data_dma_callback() is called as callback if
> dma_async_issue_pending(dma->chan_rx) is called.
> dma_async_issue_pending(dma->chan_rx) is called on
> at91_twi_read_data_dma(), which is called in at91_do_twi_transfer(),
> which we decided above to skip in case of error.

It is not about errors, you need the exact same pointer you
got from i2c_get_dma_safe_msg_buf() to be passed to
i2c_put_dma_safe_msg_buf(). And because (in some cases, it
isn't really obvious) the dev->buf will be advanced a few
bytes, I cannot pass dev->buf to i2c_put_dma_safe_msg_buf().

-michael
  

On 05.04.2022 17:08, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> Am 2022-04-05 15:58, schrieb Codrin.Ciubotariu@microchip.com:
>> On 05.04.2022 14:09, Michael Walle wrote:
>>> Am 2022-04-05 12:02, schrieb Codrin.Ciubotariu@microchip.com:
>>>> On 05.04.2022 12:38, Michael Walle wrote:
>>>>> Am 2022-04-05 11:23, schrieb Codrin.Ciubotariu@microchip.com:
>>>>>>> +       if (dev->use_dma) {
>>>>>>> +               dma_buf = i2c_get_dma_safe_msg_buf(m_start, 1);
>>>>>>
>>>>>> If you want, you could just dev->buf = i2c_get_dma_safe...
>>>>>
>>>>> But where is the error handling in that case? dev->buf will
>>>>> be NULL, which is eventually passed to dma_map_single().
>>>>>
>>>>> Also, I need the dma_buf for the i2c_put_dma_safe_msg_buf()
>>>>> call anyway, because dev->buf will be modified during
>>>>> processing.
>>>>
>>>> You still:
>>>>       if (!dev->buf) {
>>>>               ret = -ENOMEM;
>>>>               goto out;
>>>>       }
>>>>
>>>> So, at91_do_twi_transfer()/dma_map_single() will not be called.
>>>
>>> Ahh, I misunderstood you. Yes, but as I said, I need the dma_buf
>>> temporary variable anyway, because dev->buf is modified, eg. see
>>> at91_twi_read_data_dma_callback().
>> at91_twi_read_data_dma_callback() is called as callback if
>> dma_async_issue_pending(dma->chan_rx) is called.
>> dma_async_issue_pending(dma->chan_rx) is called on
>> at91_twi_read_data_dma(), which is called in at91_do_twi_transfer(),
>> which we decided above to skip in case of error.
> 
> It is not about errors, you need the exact same pointer you
> got from i2c_get_dma_safe_msg_buf() to be passed to
> i2c_put_dma_safe_msg_buf(). And because (in some cases, it
> isn't really obvious) the dev->buf will be advanced a few
> bytes, I cannot pass dev->buf to i2c_put_dma_safe_msg_buf().

You are right, when dev->use_dma && (dev->buf_len <= AT91_I2C_DMA_THRESHOLD)

got it. Thanks!

Best regards,
Codrin