Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1658526 > unrolled thread

[RFC PATCH 2/4] i2c: add docs to clarify DMA handling

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH 2/4] i2c: add docs to clarify DMA handling Wolfram Sang <wsa+renesas@sang-engineering.com> - 2017-06-06 11:30 +0200
    Re: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling Geert Uytterhoeven <geert@linux-m68k.org> - 2017-06-06 12:10 +0200
      Re: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling Wolfram Sang <wsa@the-dreams.de> - 2017-06-06 12:30 +0200
        Re: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling Geert Uytterhoeven <geert@linux-m68k.org> - 2017-06-06 12:40 +0200
          Re: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling Wolfram Sang <wsa@the-dreams.de> - 2017-06-06 13:00 +0200
            Re: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling Geert Uytterhoeven <geert@linux-m68k.org> - 2017-06-06 13:20 +0200
              Re: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling Wolfram Sang <wsa@the-dreams.de> - 2017-06-06 13:30 +0200
                Re: [RFC PATCH 2/4] i2c: add docs to clarify DMA handling Geert Uytterhoeven <geert@linux-m68k.org> - 2017-06-06 17:30 +0200

#1658526 — [RFC PATCH 2/4] i2c: add docs to clarify DMA handling

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/DMA-considerations | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 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..945ac5cfe55c12
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,28 @@
+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 (FIXME: > 8 byte?).
+
+To support this, drivers wishing to implement DMA can use a helper function
+checking if the size is suitable for DMA or if the buffer is DMA capable:
+
+	int i2c_check_msg_for_dma(msg, dma_threshold);
+
+Please check its in kernel documentation for details.
+
+It should be further noted that bounce buffer handling is left to be handled on
+driver level because details like alignment requirements are best known on that
+level.
+
+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.
-- 
2.11.0

[toc] | [next] | [standalone]

#1658569

Hi Wolfram,

On Tue, Jun 6, 2017 at 11:20 AM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,28 @@
> +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 (FIXME: > 8 byte?).

So you expect drivers to fall back to PIO automatically if the buffer is
not DMA safe.  Sounds good to me.

> +To support this, drivers wishing to implement DMA can use a helper function
> +checking if the size is suitable for DMA or if the buffer is DMA capable:
> +
> +       int i2c_check_msg_for_dma(msg, dma_threshold);
> +
> +Please check its in kernel documentation for details.
> +
> +It should be further noted that bounce buffer handling is left to be handled on
> +driver level because details like alignment requirements are best known on that
> +level.
> +
> +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.

However, your check for a DMA-capable buffer is invoked only if
CONFIG_DMA_API_DEBUG is enabled:

    #if !defined(CONFIG_DMA_API_DEBUG)
           if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
                   pr_debug("msg buffer to 0x%04x might not be DMA capable\n",
                            msg->addr);
                   return -EFAULT;
           }
    #endif

So the system will work fine if CONFIG_DMA_API_DEBUG is enabled, and may fail
miserably in a production kernel?

Furthermore, pr_debug() messages are not printed by default, so the developer
who did enable CONFIG_DMA_API_DEBUG may not have noticed at all?

If you want to use i2c_check_msg_for_dma() as a generic helper to verify DMA
requirements, and decide when to fall back to PIO, I think it should always do
the buffer check.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1658592

[Multipart message — attachments visible in raw view] — view raw

> > +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 (FIXME: > 8 byte?).
> 
> So you expect drivers to fall back to PIO automatically if the buffer is
> not DMA safe.  Sounds good to me.

Yes, I strongly recommend that. Otherwise, drivers can always deal with
bounce buffers on their own.

> However, your check for a DMA-capable buffer is invoked only if
> CONFIG_DMA_API_DEBUG is enabled:

is *NOT* enabled!

> 
>     #if !defined(CONFIG_DMA_API_DEBUG)
>            if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
>                    pr_debug("msg buffer to 0x%04x might not be DMA capable\n",
>                             msg->addr);
>                    return -EFAULT;
>            }
>     #endif
> 

The #if block is there because DMA_API_DEBUG does a lot more, but if the
check in the helper is enabled, the core will fall back to PIO and you
won't get the additional info from DMA_API_DEBUG.

I think this needs a comment :)

Now OK?

Regards,

   Wolfram

[toc] | [prev] | [next] | [standalone]

#1658595

Hi Wolfram,

On Tue, Jun 6, 2017 at 12:24 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> > +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 (FIXME: > 8 byte?).
>>
>> So you expect drivers to fall back to PIO automatically if the buffer is
>> not DMA safe.  Sounds good to me.
>
> Yes, I strongly recommend that. Otherwise, drivers can always deal with
> bounce buffers on their own.
>
>> However, your check for a DMA-capable buffer is invoked only if
>> CONFIG_DMA_API_DEBUG is enabled:
>
> is *NOT* enabled!

Oops ;-)

>>     #if !defined(CONFIG_DMA_API_DEBUG)
>>            if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
>>                    pr_debug("msg buffer to 0x%04x might not be DMA capable\n",
>>                             msg->addr);
>>                    return -EFAULT;
>>            }
>>     #endif
>>
>
> The #if block is there because DMA_API_DEBUG does a lot more, but if the
> check in the helper is enabled, the core will fall back to PIO and you
> won't get the additional info from DMA_API_DEBUG.
>
> I think this needs a comment :)
>
> Now OK?

So it won't fall back to PIO if CONFIG_DMA_API_DEBUG is enabled?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1658622

[Multipart message — attachments visible in raw view] — view raw

> So it won't fall back to PIO if CONFIG_DMA_API_DEBUG is enabled?

No, it will complain loudly with more details about the same things (and
some more). But for that to do, it cannot fall back to PIO.

Otherwise, you might get the idea that all is fine since you enabled
CONFIG_DMA_API_DEBUG but miss that most of you transfers fell back to
PIO. That was my thinking...

[toc] | [prev] | [next] | [standalone]

#1658645

Hi Wolfram,

On Tue, Jun 6, 2017 at 12:54 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> So it won't fall back to PIO if CONFIG_DMA_API_DEBUG is enabled?
>
> No, it will complain loudly with more details about the same things (and
> some more). But for that to do, it cannot fall back to PIO.
>
> Otherwise, you might get the idea that all is fine since you enabled
> CONFIG_DMA_API_DEBUG but miss that most of you transfers fell back to
> PIO. That was my thinking...

Hmm....

I have it enabled all the time. So my devices may stop working...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1658649

[Multipart message — attachments visible in raw view] — view raw

> I have it enabled all the time. So my devices may stop working...

If you have enabled it all the time, then there won't be a difference
from now. Or?

That also shows how rarely I2C DMA transfers are triggered from within
the kernel :)

[toc] | [prev] | [next] | [standalone]

#1658861

Hi Wolfram,

On Tue, Jun 6, 2017 at 1:23 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> I have it enabled all the time. So my devices may stop working...
>
> If you have enabled it all the time, then there won't be a difference
> from now. Or?
>
> That also shows how rarely I2C DMA transfers are triggered from within
> the kernel :)

... how rarely i2c DMA transfers from possibly unsafe buffers...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web