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

Groups > linux.kernel > #1652169 > unrolled thread

Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

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

  Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity Wolfram Sang <wsa@the-dreams.de> - 2017-05-28 18:10 +0200
    RE: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity David Laight <David.Laight@ACULAB.COM> - 2017-05-31 13:10 +0200
      Re: RE: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity Vignesh R <vigneshr@ti.com> - 2017-05-31 14:00 +0200
        Re: RE: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity Wolfram Sang <wsa@the-dreams.de> - 2017-05-31 18:00 +0200
          Re: RE: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity Wolfram Sang <wsa@the-dreams.de> - 2017-05-31 21:30 +0200
    Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity Vignesh R <vigneshr@ti.com> - 2017-05-31 14:00 +0200
      Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity Wolfram Sang <wsa@the-dreams.de> - 2017-05-31 17:20 +0200

#1652169 — Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

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

On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
> On 04/25/2017 05:56 PM, Florian Fainelli wrote:
> > Changes in v3:
> > 
> > - added check in usb_gadget_map_request_by_dev (Felipe), new patch
> > - improved commit message description (Clemens)
> > - added additiona checks for urb->setup_packet (Alan)
> > 
> > Changes in v2:
> > 
> > - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
> 
> Is this version looking good now? Thanks!

So, it seems I am in a similar situation with the I2C subsystem right
now. I need to check the message buffers if they are DMA capable.

Because you have basically the same checks in 3 different places, and I
need something similar for I2C, I wondered about a generic place to put
these checks. Especially since we want future improvements to these
checks applied everywhere immediately. Here is a small diff on what I
have now:

===

dma-mapping(?): introduce helper to check for DMA capable addresses

Introduce a helper to check if an address is DMA capable. Such a check
is subtle, so it is good to have a centralized place for it.

Note: I am absolutely not sure if dma-mapping.h is a good place for such
a function. I just couldn't think of a better one for now.

Second note: I am not even sure the checks are complete (kmapped mem?).
I am not an MM expert. But that just strengthens the argument of having
on centralized place IMO.

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


diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 5dea98358c05c4..777a37b395ff19 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1584,7 +1584,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 					ret = -EAGAIN;
 				else
 					urb->transfer_flags |= URB_DMA_MAP_PAGE;
-			} else if (is_vmalloc_addr(urb->transfer_buffer)) {
+			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {
 				WARN_ONCE(1, "transfer buffer not dma capable\n");
 				ret = -EAGAIN;
 			} else {
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4f3eecedca2d7c..da8c1230302505 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -10,6 +10,8 @@
 #include <linux/scatterlist.h>
 #include <linux/kmemcheck.h>
 #include <linux/bug.h>
+#include <linux/mm.h>
+#include <linux/sched/task_stack.h>
 
 /**
  * List of possible attributes associated with a DMA mapping. The semantics
@@ -818,4 +820,10 @@ static inline int dma_mmap_wc(struct device *dev,
 #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
 #endif
 
+/* only works in process context because of stack detection */
+static inline bool is_dma_capable_addr(void *addr)
+{
+	return !(is_vmalloc_or_module_addr(addr) ||
+		 object_is_on_stack(addr));
+}
 #endif
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 34a1c3e46ed725..47de0c0a700e7c 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -261,6 +261,7 @@ int is_vmalloc_or_module_addr(const void *x)
 #endif
 	return is_vmalloc_addr(x);
 }
+EXPORT_SYMBOL_GPL(is_vmalloc_or_module_addr);
 
 /*
  * Walk a vmap address to the struct page it maps.

===

The WIP branch containing also the I2C parts can be found here:

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

I think the whole series needs 1 or 2 days more before I send out an
RFC, but I thought I'll let you know about my idea already.

Thanks and kind regards,

   Wolfram

[toc] | [next] | [standalone]

#1654127

From: Wolfram Sang
> Sent: 28 May 2017 17:04
> 
> On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
> > On 04/25/2017 05:56 PM, Florian Fainelli wrote:
> > > Changes in v3:
> > >
> > > - added check in usb_gadget_map_request_by_dev (Felipe), new patch
> > > - improved commit message description (Clemens)
> > > - added additiona checks for urb->setup_packet (Alan)
> > >
> > > Changes in v2:
> > >
> > > - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
> >
> > Is this version looking good now? Thanks!
> 
> So, it seems I am in a similar situation with the I2C subsystem right
> now. I need to check the message buffers if they are DMA capable.
> 
> Because you have basically the same checks in 3 different places, and I
> need something similar for I2C, I wondered about a generic place to put
> these checks. Especially since we want future improvements to these
> checks applied everywhere immediately. Here is a small diff on what I
> have now:
...
> +			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {

For a generic function I'd pass the length as well.
It might be that buffers that don't cross page boundaries might be
deemed 'dma-able'.

Possibly more useful would be a variant of (IIRC) dma_map_for_device()
that will allocate a suitable bounce buffer for non-dma memory.
I think it can already do so for memory that is outside the address
range that the device can address (eg for a 32bit PCIe master in 64bit
system).

	David

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

#1654157

On Wednesday 31 May 2017 04:34 PM, David Laight wrote:
> From: Wolfram Sang
>> Sent: 28 May 2017 17:04
>>
>> On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
>>> On 04/25/2017 05:56 PM, Florian Fainelli wrote:
>>>> Changes in v3:
>>>>
>>>> - added check in usb_gadget_map_request_by_dev (Felipe), new patch
>>>> - improved commit message description (Clemens)
>>>> - added additiona checks for urb->setup_packet (Alan)
>>>>
>>>> Changes in v2:
>>>>
>>>> - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
>>>
>>> Is this version looking good now? Thanks!
>>
>> So, it seems I am in a similar situation with the I2C subsystem right
>> now. I need to check the message buffers if they are DMA capable.
>>
>> Because you have basically the same checks in 3 different places, and I
>> need something similar for I2C, I wondered about a generic place to put
>> these checks. Especially since we want future improvements to these
>> checks applied everywhere immediately. Here is a small diff on what I
>> have now:
> ...
>> +			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {
> 
> For a generic function I'd pass the length as well.
> It might be that buffers that don't cross page boundaries might be
> deemed 'dma-able'.
> 
> Possibly more useful would be a variant of (IIRC) dma_map_for_device()
> that will allocate a suitable bounce buffer for non-dma memory.
> I think it can already do so for memory that is outside the address
> range that the device can address (eg for a 32bit PCIe master in 64bit
> system).
> 

Such generic DMA API would be greatly useful!

I tried adding bounce buffers support to handle vmalloc'd buffers in
MTD/SPI subsystem. But, there was a need felt for generic DMA API that
can allocate bounce buffer for non-dma'able buffers that all drivers can
make use of[1][2]


[1] https://lkml.org/lkml/2017/3/1/488
[2] https://lkml.org/lkml/2017/4/25/278



-- 
Regards
Vignesh

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

#1654379

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

> > Possibly more useful would be a variant of (IIRC) dma_map_for_device()
> > that will allocate a suitable bounce buffer for non-dma memory.
> > I think it can already do so for memory that is outside the address
> > range that the device can address (eg for a 32bit PCIe master in 64bit
> > system).
> > 
> 
> Such generic DMA API would be greatly useful!
> 
> I tried adding bounce buffers support to handle vmalloc'd buffers in
> MTD/SPI subsystem. But, there was a need felt for generic DMA API that
> can allocate bounce buffer for non-dma'able buffers that all drivers can
> make use of[1][2]

Yes, I see this DMA API would make sense for subsystems like SPI, MTD or
USB. For I2C, I don't think it makes a lot of sense because DMA is
rarely used there. Most hardware doesn't even have DMA support and if
so, the drivers apply a threshold (say 8 bytes) because most I2C
transfers are smaller and setting up DMA for that simply doesn't pay
off. And we are still talking of a mostly 100 or 400 kHz bus here.

So, I'd prefer a lightweight helper function telling if DMA is
possible/feasible for a given I2C message. If so, do it. If not, falling
back to PIO might be good enough for now. We can implement bounce buffer
support in the above helper function later. I don't really want to
enforce DMA capable buffers for I2C transactions when DMA is so rarely
needed there.

So, if I2C is a bit different, then it might simply make sense to keep
the function local for I2C now? This seems like a sensible start to me
meanwhile.

Thanks to all for the helpful input here!

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

#1654564

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

> So, if I2C is a bit different, then it might simply make sense to keep
> the function local for I2C now? This seems like a sensible start to me
> meanwhile.

Then again, the DMA API would be for drivers. If the USB core wants to
check for capable buffers, such a helper might be nice nonetheless.
Especially if the USB core needs the check in at least 3 places.

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

#1654158

On Sunday 28 May 2017 09:33 PM, Wolfram Sang wrote:
> On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
>> On 04/25/2017 05:56 PM, Florian Fainelli wrote:
>>> Changes in v3:
>>>
>>> - added check in usb_gadget_map_request_by_dev (Felipe), new patch
>>> - improved commit message description (Clemens)
>>> - added additiona checks for urb->setup_packet (Alan)
>>>
>>> Changes in v2:
>>>
>>> - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
>>
>> Is this version looking good now? Thanks!
> 
> So, it seems I am in a similar situation with the I2C subsystem right
> now. I need to check the message buffers if they are DMA capable.
> 
> Because you have basically the same checks in 3 different places, and I
> need something similar for I2C, I wondered about a generic place to put
> these checks. Especially since we want future improvements to these
> checks applied everywhere immediately. Here is a small diff on what I
> have now:
> 
> ===
> 
> dma-mapping(?): introduce helper to check for DMA capable addresses
> 
> Introduce a helper to check if an address is DMA capable. Such a check
> is subtle, so it is good to have a centralized place for it.
> 
> Note: I am absolutely not sure if dma-mapping.h is a good place for such
> a function. I just couldn't think of a better one for now.
> 
> Second note: I am not even sure the checks are complete (kmapped mem?).
> I am not an MM expert. But that just strengthens the argument of having
> on centralized place IMO.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 5dea98358c05c4..777a37b395ff19 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1584,7 +1584,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  					ret = -EAGAIN;
>  				else
>  					urb->transfer_flags |= URB_DMA_MAP_PAGE;
> -			} else if (is_vmalloc_addr(urb->transfer_buffer)) {
> +			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {
>  				WARN_ONCE(1, "transfer buffer not dma capable\n");
>  				ret = -EAGAIN;
>  			} else {
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4f3eecedca2d7c..da8c1230302505 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -10,6 +10,8 @@
>  #include <linux/scatterlist.h>
>  #include <linux/kmemcheck.h>
>  #include <linux/bug.h>
> +#include <linux/mm.h>
> +#include <linux/sched/task_stack.h>
>  
>  /**
>   * List of possible attributes associated with a DMA mapping. The semantics
> @@ -818,4 +820,10 @@ static inline int dma_mmap_wc(struct device *dev,
>  #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
>  #endif
>  
> +/* only works in process context because of stack detection */
> +static inline bool is_dma_capable_addr(void *addr)
> +{
> +	return !(is_vmalloc_or_module_addr(addr) ||
> +		 object_is_on_stack(addr));

This does not catch kmap'ed buffers which are not directly DMA'able.
I would suggest to use virt_addr_valid() instead. Something like:

	return (virt_addr_valid(addr) && !object_is_on_stack(addr));

> +}
>  #endif
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 34a1c3e46ed725..47de0c0a700e7c 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -261,6 +261,7 @@ int is_vmalloc_or_module_addr(const void *x)
>  #endif
>  	return is_vmalloc_addr(x);
>  }
> +EXPORT_SYMBOL_GPL(is_vmalloc_or_module_addr);
>  
>  /*
>   * Walk a vmap address to the struct page it maps.
> 
> ===
> 
> The WIP branch containing also the I2C parts can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma
> 
> I think the whole series needs 1 or 2 days more before I send out an
> RFC, but I thought I'll let you know about my idea already.
> 
> Thanks and kind regards,
> 
>    Wolfram
> 

-- 
Regards
Vignesh

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

#1654342

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

> > +/* only works in process context because of stack detection */
> > +static inline bool is_dma_capable_addr(void *addr)
> > +{
> > +	return !(is_vmalloc_or_module_addr(addr) ||
> > +		 object_is_on_stack(addr));
> 
> This does not catch kmap'ed buffers which are not directly DMA'able.
> I would suggest to use virt_addr_valid() instead. Something like:
> 
> 	return (virt_addr_valid(addr) && !object_is_on_stack(addr));

Hehe, here is the part of the commit message I have for this code:

===

Second note: I am not even sure the checks complete (kmapped mem?). But
that just strengthens the argument of having on centralized place IMO :)

===

So, thanks for the heads up!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web