Re: [linux-usb-devel] PATCH: Don't allocate transfer buffers on thestack in hub.c
Alan Stern wrote:
Does this need to be cleaned up?
Yes. I thought we had fixed all of this in the past, did we miss
something?
I think some fixes didn't get merged, for various reasons;
discussion about cache-incoherent DMA derailed a few.
This should fix things. I added buffer space (4 bytes) in struct usb_hub
to store the status reports.
Reads good, but some comments on the GET_STATUS requests:
- Those timeouts should be HZ * USB_CTRL_GET_TIMEOUT, these
are excessively short.
- Naming is problematic: usb_*() suggests they're generic
and exported, but they're not. I'd strike the usb_ prefix.
None of those are new issues, but they could be resolved now
while this code is being updated.
- Dave
--- 1.105/drivers/usb/core/hub.c Sat May 24 18:40:11 2003
+++ edited/drivers/usb/core/hub.c Wed Jun 4 11:32:10 2003
@@ -103,21 +103,23 @@
/*
* USB 2.0 spec Section 11.24.2.6
*/
-static int usb_get_hub_status(struct usb_device *dev, void *data)
+static int usb_get_hub_status(struct usb_device *dev,
+ struct usb_hub_status *data)
{
return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_HUB, 0, 0,
- data, sizeof(struct usb_hub_status), HZ);
+ data, sizeof(*data), HZ);
}
/*
* USB 2.0 spec Section 11.24.2.7
*/
-static int usb_get_port_status(struct usb_device *dev, int port, void *data)
+static int usb_get_port_status(struct usb_device *dev, int port,
+ struct usb_port_status *data)
{
return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_PORT, 0, port,
- data, sizeof(struct usb_hub_status), HZ);
+ data, sizeof(*data), HZ);
}
/* completion function, fires on port status changes and various faults */
---
This SF.net email is sponsored by: Etnus, makers of TotalView, The best
thread debugger on the planet. Designed with thread debugging features
you've never dreamed of, try TotalView 6 free at www.etnus.com.
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] PATCH: Don't allocate transfer buffers on thestack in hub.c
On Wed, 4 Jun 2003, David Brownell wrote: Reads good, but some comments on the GET_STATUS requests: - Those timeouts should be HZ * USB_CTRL_GET_TIMEOUT, these are excessively short. - Naming is problematic: usb_*() suggests they're generic and exported, but they're not. I'd strike the usb_ prefix. None of those are new issues, but they could be resolved now while this code is being updated. What about the other static routines in hub.c that use the same prefix or the same short timeout? usb_get_hub_descriptor() usb_clear_hub_feature() usb_clear_port_feature() usb_set_port_feature() hub_clear_tt_buffer() usb_hub_power_on() usb_hub_configure() usb_hub_reset() usb_hub_disconnect() usb_hub_port_status() usb_hub_port_wait_reset() usb_hub_port_reset() usb_hub_port_debounce() usb_hub_port_connect_change() usb_hub_events() usb_hub_thread() Shouldn't they all be changed? Alan Stern --- This SF.net email is sponsored by: Etnus, makers of TotalView, The best thread debugger on the planet. Designed with thread debugging features you've never dreamed of, try TotalView 6 free at www.etnus.com. ___ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] PATCH: Don't allocate transfer buffers on thestack in hub.c
Alan Stern wrote: On Wed, 4 Jun 2003, David Brownell wrote: Reads good, but some comments on the GET_STATUS requests: - Those timeouts should be HZ * USB_CTRL_GET_TIMEOUT, these are excessively short. - Naming is problematic: usb_*() suggests they're generic and exported, but they're not. I'd strike the usb_ prefix. None of those are new issues, but they could be resolved now while this code is being updated. What about the other static routines in hub.c that use the same prefix or the same short timeout? More issues to address, yes ... :) usb_get_hub_descriptor() usb_clear_hub_feature() usb_clear_port_feature() usb_set_port_feature() hub_clear_tt_buffer() usb_hub_power_on() usb_hub_configure() usb_hub_reset() usb_hub_disconnect() usb_hub_port_status() usb_hub_port_wait_reset() usb_hub_port_reset() usb_hub_port_debounce() usb_hub_port_connect_change() usb_hub_events() usb_hub_thread() Shouldn't they all be changed? I'd rather they were. The rename is purely a cosmetic change, but there's no good reason to have the hub driver use timeouts that are so much shorter than the rest of usbcore. - Dave Alan Stern --- This SF.net email is sponsored by: Etnus, makers of TotalView, The best thread debugger on the planet. Designed with thread debugging features you've never dreamed of, try TotalView 6 free at www.etnus.com. ___ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] PATCH: Don't allocate transfer buffers on thestack in hub.c
On Wed, 4 Jun 2003, Oliver Neukum wrote: It seems to me that this union needs a cacheline of its own for noncoherent architectures. The rest looks good. Oliver: I would appreciate it if you (or anyone else) could post or provide a pointer to a good discussion that explains all the important issues involved in DMA coherency and cache interactions. These are tricky topics with a lot of subtleties. So here, for example, to what extent does it matter that the buffer is not allocated separately? Would moving it to the start of the whole kmalloc'ed structure solve the problem? Is this a question of each DMA master having to write in units of entire cachelines, thereby interfering with whatever data the CPU happens to store in the same cachelines? And does this also mean that it's effectively impossible to dynamically allocate any region smaller than a cacheline? Alan Stern --- This SF.net email is sponsored by: Etnus, makers of TotalView, The best thread debugger on the planet. Designed with thread debugging features you've never dreamed of, try TotalView 6 free at www.etnus.com. ___ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] PATCH: Don't allocate transfer buffers on thestack in hub.c
Alan Stern wrote: And does this also mean that it's effectively impossible to dynamically allocate any region smaller than a cacheline? Only on processors with DMA-incoherent caches. Unfortunately the only way to see if you're compiling for such a system is to use architecture-specific #defines. PPC and MIPs use different ones, as I recall... --- This SF.net email is sponsored by: Etnus, makers of TotalView, The best thread debugger on the planet. Designed with thread debugging features you've never dreamed of, try TotalView 6 free at www.etnus.com. ___ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] PATCH: Don't allocate transfer buffers on thestack in hub.c
I notice that the transfer buffer for the status URB is also part of struct usb_hub, hence not cacheline-aligned. I also notice that the contents of the buffer are never used; when a status change event occurs the driver probes all the ports and the hub itself. Would it be safe to eliminate that buffer and have the status IRQ URB request a 0-length transfer? Or would it be better to put that buffer along with the status report buffers in a separate area of memory? Having them all together shouldn't matter, because they would all be written by the same bus master. Alan Stern --- This SF.net email is sponsored by: Etnus, makers of TotalView, The best thread debugger on the planet. Designed with thread debugging features you've never dreamed of, try TotalView 6 free at www.etnus.com. ___ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] PATCH: Don't allocate transfer buffers on thestack in hub.c
Am Mittwoch, 4. Juni 2003 20:58 schrieb Alan Stern: On Wed, 4 Jun 2003, Oliver Neukum wrote: It seems to me that this union needs a cacheline of its own for noncoherent architectures. The rest looks good. Oliver: I would appreciate it if you (or anyone else) could post or provide a pointer to a good discussion that explains all the important issues involved in DMA coherency and cache interactions. These are tricky topics with a lot of subtleties. I wish I had one myself. So here, for example, to what extent does it matter that the buffer is not allocated separately? Would moving it to the start of the whole kmalloc'ed structure solve the problem? No. Is this a question of each DMA master having to write in units of entire cachelines, thereby interfering with whatever data the CPU happens to store in the same cachelines? No, the other way round. The CPU writes whole cachelines. This corrupts parts of a cacheline not up to date in the CPU's cache. And does this also mean that it's effectively impossible to dynamically allocate any region smaller than a cacheline? Yes. Regards Oliver --- This SF.net email is sponsored by: Etnus, makers of TotalView, The best thread debugger on the planet. Designed with thread debugging features you've never dreamed of, try TotalView 6 free at www.etnus.com. ___ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
