[PATCH] USB: use stack allocation for struct usb_ctrlrequest

[PATCH] USB: use stack allocation for struct usb_ctrlrequest

[Wu Fengguang]

sizeof(struct usb_ctrlrequest) = 8, which is as small as the *dt pointer
in a 64bit system.

Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 drivers/usb/core/message.c |   27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

--- linux-2.6.orig/drivers/usb/core/message.c
+++ linux-2.6/drivers/usb/core/message.c
@@ -130,26 +130,15 @@ int usb_control_msg(struct usb_device *d
 		    __u8 requesttype, __u16 value, __u16 index, void *data,
 		    __u16 size, int timeout)
 {
-	struct usb_ctrlrequest *dr;
-	int ret;
-
-	dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
-	if (!dr)
-		return -ENOMEM;
-
-	dr->bRequestType = requesttype;
-	dr->bRequest = request;
-	dr->wValue = cpu_to_le16p(&value);
-	dr->wIndex = cpu_to_le16p(&index);
-	dr->wLength = cpu_to_le16p(&size);
+	struct usb_ctrlrequest dr = {
+		.bRequestType = requesttype,
+		.bRequest = request,
+		.wValue = cpu_to_le16p(&value),
+		.wIndex = cpu_to_le16p(&index),
+		.wLength = cpu_to_le16p(&size),
+	};
 
-	/* dbg("usb_control_msg"); */
-
-	ret = usb_internal_control_msg(dev, pipe, dr, data, size, timeout);
-
-	kfree(dr);
-
-	return ret;
+	return usb_internal_control_msg(dev, pipe, &dr, data, size, timeout);
 }
 EXPORT_SYMBOL_GPL(usb_control_msg);
 

Re: [PATCH] USB: use stack allocation for struct usb_ctrlrequest

[Laurent Pinchart]

Hi Wu,

On Wednesday 10 December 2008, Wu Fengguang wrote:
> sizeof(struct usb_ctrlrequest) = 8, which is as small as the *dt pointer
> in a 64bit system.

The usb_ctrlrequest pointer is passed down to the hardware and must point to 
DMA-able memory. For this reason you can't use the stack and must kmalloc() 
the structure.

Best regards,

Laurent Pinchart

> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> ---
>  drivers/usb/core/message.c |   27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
>
> --- linux-2.6.orig/drivers/usb/core/message.c
> +++ linux-2.6/drivers/usb/core/message.c
> @@ -130,26 +130,15 @@ int usb_control_msg(struct usb_device *d
>  		    __u8 requesttype, __u16 value, __u16 index, void *data,
>  		    __u16 size, int timeout)
>  {
> -	struct usb_ctrlrequest *dr;
> -	int ret;
> -
> -	dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
> -	if (!dr)
> -		return -ENOMEM;
> -
> -	dr->bRequestType = requesttype;
> -	dr->bRequest = request;
> -	dr->wValue = cpu_to_le16p(&value);
> -	dr->wIndex = cpu_to_le16p(&index);
> -	dr->wLength = cpu_to_le16p(&size);
> +	struct usb_ctrlrequest dr = {
> +		.bRequestType = requesttype,
> +		.bRequest = request,
> +		.wValue = cpu_to_le16p(&value),
> +		.wIndex = cpu_to_le16p(&index),
> +		.wLength = cpu_to_le16p(&size),
> +	};
>
> -	/* dbg("usb_control_msg"); */
> -
> -	ret = usb_internal_control_msg(dev, pipe, dr, data, size, timeout);
> -
> -	kfree(dr);
> -
> -	return ret;
> +	return usb_internal_control_msg(dev, pipe, &dr, data, size, timeout);
>  }
>  EXPORT_SYMBOL_GPL(usb_control_msg);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: use stack allocation for struct usb_ctrlrequest

[Wu Fengguang]

Hi Laurent,

On Wed, Dec 10, 2008 at 11:40:09AM +0200, Laurent Pinchart wrote:
> Hi Wu,
> 
> On Wednesday 10 December 2008, Wu Fengguang wrote:
> > sizeof(struct usb_ctrlrequest) = 8, which is as small as the *dt pointer
> > in a 64bit system.
> 
> The usb_ctrlrequest pointer is passed down to the hardware and must point to 
> DMA-able memory. For this reason you can't use the stack and must kmalloc() 
> the structure.

Ah thanks for the background. Does GFP_NOIO guarantee that?
e.g. what if the memory is allocated from ZONE_HIGHMEM?

Thanks,
Fengguang

> 
> > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > ---
> >  drivers/usb/core/message.c |   27 ++++++++-------------------
> >  1 file changed, 8 insertions(+), 19 deletions(-)
> >
> > --- linux-2.6.orig/drivers/usb/core/message.c
> > +++ linux-2.6/drivers/usb/core/message.c
> > @@ -130,26 +130,15 @@ int usb_control_msg(struct usb_device *d
> >  		    __u8 requesttype, __u16 value, __u16 index, void *data,
> >  		    __u16 size, int timeout)
> >  {
> > -	struct usb_ctrlrequest *dr;
> > -	int ret;
> > -
> > -	dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
> > -	if (!dr)
> > -		return -ENOMEM;
> > -
> > -	dr->bRequestType = requesttype;
> > -	dr->bRequest = request;
> > -	dr->wValue = cpu_to_le16p(&value);
> > -	dr->wIndex = cpu_to_le16p(&index);
> > -	dr->wLength = cpu_to_le16p(&size);
> > +	struct usb_ctrlrequest dr = {
> > +		.bRequestType = requesttype,
> > +		.bRequest = request,
> > +		.wValue = cpu_to_le16p(&value),
> > +		.wIndex = cpu_to_le16p(&index),
> > +		.wLength = cpu_to_le16p(&size),
> > +	};
> >
> > -	/* dbg("usb_control_msg"); */
> > -
> > -	ret = usb_internal_control_msg(dev, pipe, dr, data, size, timeout);
> > -
> > -	kfree(dr);
> > -
> > -	return ret;
> > +	return usb_internal_control_msg(dev, pipe, &dr, data, size, timeout);
> >  }
> >  EXPORT_SYMBOL_GPL(usb_control_msg);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: use stack allocation for struct usb_ctrlrequest

[Gilad Ben-Yossef]

Wu Fengguang wrote:

> Hi Laurent,
>
> On Wed, Dec 10, 2008 at 11:40:09AM +0200, Laurent Pinchart wrote:
>   
>> Hi Wu,
>>
>> On Wednesday 10 December 2008, Wu Fengguang wrote:
>>     
>>> sizeof(struct usb_ctrlrequest) = 8, which is as small as the *dt pointer
>>> in a 64bit system.
>>>       
>> The usb_ctrlrequest pointer is passed down to the hardware and must point to 
>> DMA-able memory. For this reason you can't use the stack and must kmalloc() 
>> the structure.
>>     
>
> Ah thanks for the background. Does GFP_NOIO guarantee that?
>   
No, GFP_NOIO means - do not generate block IO operations (e.g. move 
pages to swap, sync dirty buffers to permanent storage etc.) in order to 
fulfill this allocation.

The reason for this flag here is presumably that such block IO 
operations may very cause USB transaction of the very same kind we're 
trying to service now, which can easily get us to a loop.
> e.g. what if the memory is allocated from ZONE_HIGHMEM?
>   
In many cases there is no problem to DMA high memory. If you happen to 
be working with a device that does have problems with full 32 bit 
addresses then GFP_DMA would be the right flag, not GFP_NOIO.

Cheers,
Gilad


-- 
Gilad Ben-Yossef 
Chief Coffee Drinker

Codefidence Ltd.
The code is free, your time isn't.(TM)

Web:    http://codefidence.com
Email:  gilad@codefidence.com
Office: +972-8-9316883 ext. 201
Fax:    +972-8-9316885
Mobile: +972-52-8260388

	The Doctor: Don't worry, Reinette, just a nightmare. 
	Everyone has nightmares. Even monsters from under the 
	bed have nightmares, don't you, monster?
	Reinette: What do monsters have nightmares about?
	The Doctor: Me! 

Re: [PATCH] USB: use stack allocation for struct usb_ctrlrequest

[Wu Fengguang]

On Wed, Dec 10, 2008 at 04:07:14PM +0200, Gilad Ben-Yossef wrote:
> Wu Fengguang wrote:
> 
> > Hi Laurent,
> >
> > On Wed, Dec 10, 2008 at 11:40:09AM +0200, Laurent Pinchart wrote:
> >   
> >> Hi Wu,
> >>
> >> On Wednesday 10 December 2008, Wu Fengguang wrote:
> >>     
> >>> sizeof(struct usb_ctrlrequest) = 8, which is as small as the *dt pointer
> >>> in a 64bit system.
> >>>       
> >> The usb_ctrlrequest pointer is passed down to the hardware and must point to 
> >> DMA-able memory. For this reason you can't use the stack and must kmalloc() 
> >> the structure.
> >>     
> >
> > Ah thanks for the background. Does GFP_NOIO guarantee that?
> >   
> No, GFP_NOIO means - do not generate block IO operations (e.g. move 
> pages to swap, sync dirty buffers to permanent storage etc.) in order to 
> fulfill this allocation.
> 
> The reason for this flag here is presumably that such block IO 
> operations may very cause USB transaction of the very same kind we're 
> trying to service now, which can easily get us to a loop.

Right.

> > e.g. what if the memory is allocated from ZONE_HIGHMEM?
> >   
> In many cases there is no problem to DMA high memory. If you happen to 
> be working with a device that does have problems with full 32 bit 
> addresses then GFP_DMA would be the right flag, not GFP_NOIO.

For 64bit systems, we can easily go beyond 4GB physical memory.
So at least we should add GFP_DMA32 in addition to GFP_NOIO?

Thanks,
Fengguang

Re: [PATCH] USB: use stack allocation for struct usb_ctrlrequest

[Gilad Ben-Yossef]

Wu Fengguang wrote:

>
>>> e.g. what if the memory is allocated from ZONE_HIGHMEM?
>>>   
>>>       
>> In many cases there is no problem to DMA high memory. If you happen to 
>> be working with a device that does have problems with full 32 bit 
>> addresses then GFP_DMA would be the right flag, not GFP_NOIO.
>>     
>
> For 64bit systems, we can easily go beyond 4GB physical memory.
> So at least we should add GFP_DMA32 in addition to GFP_NOIO?
>
>   
I don't think so. 64bit systems don't have ZONE_HIMEM - this is why I 
was referring to 32 bit.

More to the point, many devices don't have any issues accessing full 64 
bit addressing mode and many 64bit machines have IOMMU that will take 
care of those devices that DO have a problem. Adding this flag will just 
penalize the MM sub-system for no reason.  For the rare case where there 
is a device that cannot do 64 bit addressing in a 64 bit machine with no 
IOMMU, AFAIK bounce buffers are used to overcome the issue.

Cheers,
Gilad



-- 
Gilad Ben-Yossef 
Chief Coffee Drinker

Codefidence Ltd.
The code is free, your time isn't.(TM)

Web:    http://codefidence.com
Email:  gilad@codefidence.com
Office: +972-8-9316883 ext. 201
Fax:    +972-8-9316885
Mobile: +972-52-8260388

	The Doctor: Don't worry, Reinette, just a nightmare. 
	Everyone has nightmares. Even monsters from under the 
	bed have nightmares, don't you, monster?
	Reinette: What do monsters have nightmares about?
	The Doctor: Me! 

Re: [PATCH] USB: use stack allocation for struct usb_ctrlrequest

[Pete Zaitcev]

On Wed, 10 Dec 2008 22:23:01 +0800, Wu Fengguang <fengguang.wu@intel.com> wrote:

> For 64bit systems, we can easily go beyond 4GB physical memory.
> So at least we should add GFP_DMA32 in addition to GFP_NOIO?

I am afraid the situation is that we really screwed the pooch while
creating the USB API. I may be wrong about this, but my understanding
is that if we get an address above 4GB from kmalloc and then send
it down to usb_submit_urb(), a random memory corruption is likely
(this is because we forget to check the result of dma_map_single()).

The code worked until now because most systems out in the field
either a) had IOMMU, or b) had 4GB or RAM or less, but not both.
The case (a) includes all AMD CPUs, all Itanium CPUs, and the
Intel-based enterprise systems from big vendors, e.g. IBM Calgary,
HP ZX-1, etc. Also, (a) covers Intel P4 class systems with swiotlb.
So, we only blow up if a kernel with swiotlb disabled boots on an
Intel box with more than 4GB of RAM. This is still far from ideal,
but we kinda pretend not to notice. I heard that Intel has seen
the error in their ways and is going to come out with IOMMU for
all their chipsets, so in a few years this is going to be moot.

-- Pete

Re: [PATCH] USB: use stack allocation for struct usb_ctrlrequest

[Robert Hancock]

Pete Zaitcev wrote:
> On Wed, 10 Dec 2008 22:23:01 +0800, Wu Fengguang <fengguang.wu@intel.com> wrote:
> 
>> For 64bit systems, we can easily go beyond 4GB physical memory.
>> So at least we should add GFP_DMA32 in addition to GFP_NOIO?
> 
> I am afraid the situation is that we really screwed the pooch while
> creating the USB API. I may be wrong about this, but my understanding
> is that if we get an address above 4GB from kmalloc and then send
> it down to usb_submit_urb(), a random memory corruption is likely
> (this is because we forget to check the result of dma_map_single()).
> 
> The code worked until now because most systems out in the field
> either a) had IOMMU, or b) had 4GB or RAM or less, but not both.
> The case (a) includes all AMD CPUs, all Itanium CPUs, and the
> Intel-based enterprise systems from big vendors, e.g. IBM Calgary,
> HP ZX-1, etc. Also, (a) covers Intel P4 class systems with swiotlb.
> So, we only blow up if a kernel with swiotlb disabled boots on an
> Intel box with more than 4GB of RAM. This is still far from ideal,
> but we kinda pretend not to notice. I heard that Intel has seen
> the error in their ways and is going to come out with IOMMU for
> all their chipsets, so in a few years this is going to be moot.

If you have memory located above 4GB you essentially need either swiotlb 
or one of the other IOMMUs enabled or the system won't work if any of 
your devices have DMA limits. There is no other way that DMA could occur 
to memory above 4GB for those devices.

DMA mapping could still fail if the IOMMU space overflowed, though.

Re: [PATCH] USB: use stack allocation for struct usb_ctrlrequest

[Robert Hancock]

Wu Fengguang wrote:
>> In many cases there is no problem to DMA high memory. If you happen to 
>> be working with a device that does have problems with full 32 bit 
>> addresses then GFP_DMA would be the right flag, not GFP_NOIO.
> 
> For 64bit systems, we can easily go beyond 4GB physical memory.
> So at least we should add GFP_DMA32 in addition to GFP_NOIO?

No. The DMA mapping API handles either setting up the IOMMU or doing 
software bounce buffering to handle the DMA transfer regardless of where 
the memory is located, so drivers don't need to mess with GFP_DMA32.