USB: use stack allocation for struct usb_ctrlrequest
PATCH 1/1 USB: use stack allocation for struct usb_ctrlrequest
sizeof(struct usb_ctrlrequest) = 8, which is as small as the *dt pointer in a 64bit system.
--- drivers/usb/core/message.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)
drivers/usb/core/message.c +8 -19
@@ -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,
Quoted 2 lines · Wu Fengguang
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
Quoted 47 lines
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(-)drivers/usb/core/message.c +8
-20@@ -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,
Quoted 9 lines · Laurent Pinchart
Hi Wu,
Quoted 2 lines · Wu Fengguang
sizeof(struct usb_ctrlrequest)= 8, which is as small as the *dt pointer in a 64bit system.The
usb_ctrlrequestpointer is passed down to the hardware and must point to DMA-able memory. For this reason you can't use the stack and mustkmalloc()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
Quoted 48 lines
Quoted 48 lines
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(-)drivers/usb/core/message.c +8
-20@@ -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
Quoted 8 lines · Wu Fengguang
Hi Laurent,
Quoted 4 lines · Laurent Pinchart
Hi Wu,
On Wednesday 10 December 2008, Wu Fengguang wrote:
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
sizeof(struct usb_ctrlrequest)= 8, which is as small as the *dt pointer in a 64bit system.The
usb_ctrlrequestpointer is passed down to the hardware and must point to DMA-able memory. For this reason you can't use the stack and mustkmalloc()the structure.Ah thanks for the background. Does
GFP_NOIOguarantee 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.
Quoted 2 lines
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
Signature · 16 lines
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!
Quoted 20 lines · Gilad Ben-Yossef
Quoted 11 lines · Wu Fengguang
Hi Laurent,
Quoted 7 lines · Laurent Pinchart
Hi Wu,
Quoted 3 lines · Wu Fengguang
sizeof(struct usb_ctrlrequest)= 8, which is as small as the *dt pointer in a 64bit system.Quoted 7 lines
Quoted 4 lines
The
usb_ctrlrequestpointer is passed down to the hardware and must point to DMA-able memory. For this reason you can't use the stack and mustkmalloc()the structure.Ah thanks for the background. Does
GFP_NOIOguarantee that?
Quoted 7 lines
No,
GFP_NOIOmeans - 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.
Quoted 5 lines
Quoted 2 lines
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_DMAwould be the right flag, notGFP_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
Quoted 11 lines · Wu Fengguang
Quoted 6 lines
Quoted 4 lines
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_DMAwould be the right flag, notGFP_NOIO.For 64bit systems, we can easily go beyond 4GB physical memory. So at least we should add
GFP_DMA32in addition toGFP_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
Signature · 16 lines
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!
Quoted 2 lines · Wu Fengguang
For 64bit systems, we can easily go beyond 4GB physical memory. So at least we should add
GFP_DMA32in addition toGFP_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
Quoted 11 lines · Pete Zaitcev
Quoted 2 lines · Wu Fengguang
For 64bit systems, we can easily go beyond 4GB physical memory. So at least we should add
GFP_DMA32in addition toGFP_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 ofdma_map_single()).
Quoted 10 lines
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.
Quoted 4 lines · Wu Fengguang
Quoted 1 line
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_DMAwould be the right flag, notGFP_NOIO.For 64bit systems, we can easily go beyond 4GB physical memory. So at least we should add
GFP_DMA32in addition toGFP_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.