sizeof(struct usb_ctrlrequest) = 8, which is as small as the *dt pointer
in a 64bit system.
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Wu Fengguang <[email protected]>
---
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);
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 <[email protected]>
> Signed-off-by: Wu Fengguang <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
> > Signed-off-by: Wu Fengguang <[email protected]>
> > ---
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
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: [email protected]
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!
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
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: [email protected]
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!
On Wed, 10 Dec 2008 22:23:01 +0800, Wu Fengguang <[email protected]> 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
Pete Zaitcev wrote:
> On Wed, 10 Dec 2008 22:23:01 +0800, Wu Fengguang <[email protected]> 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.
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.