Re: [U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue
Hi,
On 09/20/2014 07:42 PM, Michael Trimarchi wrote:
Hi
On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede hdego...@redhat.com wrote:
Preperation patch to use create_int_queue outside of ehci-hcd.c .
Signed-off-by: Hans de Goede hdego...@redhat.com
---
drivers/usb/host/ehci-hcd.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 20830d7..cf3e3c0 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned
long pipe, int queuesize,
struct int_queue *result = NULL;
int i;
+ /*
+* Interrupt transfers requiring several transactions are not
supported
+* because bInterval is ignored.
+*
+* Also, ehci_submit_async() relies on wMaxPacketSize being a power
of 2
+* = PKT_ALIGN if several qTDs are required, while the USB
+* specification does not constrain this for interrupt transfers.
That
+* means that ehci_submit_async() would support interrupt transfers
+* requiring several transactions only as long as the transfer size
does
+* not require more than a single qTD.
+*/
+ if (elementsize usb_maxpacket(dev, pipe)) {
+ printf(%s: xfers requiring several transactions are not
supported.\n,
+ __func__);
+ return NULL;
+ }
+
debug(Enter create_int_queue\n);
if (usb_pipetype(pipe) != PIPE_INTERRUPT) {
debug(non-interrupt pipe (type=%lu), usb_pipetype(pipe));
@@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long
pipe, void *buffer,
debug(dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d,
dev, pipe, buffer, length, interval);
- /*
-* Interrupt transfers requiring several transactions are not
supported
-* because bInterval is ignored.
-*
-* Also, ehci_submit_async() relies on wMaxPacketSize being a power
of 2
-* = PKT_ALIGN if several qTDs are required, while the USB
-* specification does not constrain this for interrupt transfers.
That
-* means that ehci_submit_async() would support interrupt transfers
-* requiring several transactions only as long as the transfer size
does
-* not require more than a single qTD.
-*/
- if (length usb_maxpacket(dev, pipe)) {
- printf(%s: Interrupt transfers requiring several
- transactions are not supported.\n, __func__);
- return -1;
- }
-
queue = create_int_queue(dev, pipe, 1, length, buffer);
+ if (!queue)
+ return -1;
Can you return a more consistent error code?
I'm just moving code around, and returning the same error code as before. Surely
changing the error code belongs in another patch ?
Regards,
Hans
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue
On Sunday, September 21, 2014 at 07:53:35 PM, Hans de Goede wrote:
Hi,
[...]
- if (length usb_maxpacket(dev, pipe)) {
- printf(%s: Interrupt transfers requiring several
- transactions are not supported.\n, __func__);
- return -1;
- }
-
queue = create_int_queue(dev, pipe, 1, length, buffer);
+ if (!queue)
+ return -1;
Can you return a more consistent error code?
I'm just moving code around, and returning the same error code as before.
Surely changing the error code belongs in another patch ?
Yes, full ACK. This is exactly a prime examply where squashing two fixes into
one patch would break bisectability absolutely perfectly.
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue
Hi Marek
On Sun, Sep 21, 2014 at 9:36 PM, Marek Vasut ma...@denx.de wrote:
On Sunday, September 21, 2014 at 07:53:35 PM, Hans de Goede wrote:
Hi,
[...]
- if (length usb_maxpacket(dev, pipe)) {
- printf(%s: Interrupt transfers requiring several
- transactions are not supported.\n, __func__);
- return -1;
- }
-
queue = create_int_queue(dev, pipe, 1, length, buffer);
+ if (!queue)
+ return -1;
Can you return a more consistent error code?
I'm just moving code around, and returning the same error code as before.
Surely changing the error code belongs in another patch ?
Yes, full ACK. This is exactly a prime examply where squashing two fixes into
one patch would break bisectability absolutely perfectly.
Agree on separated patch, I have just ask if Hans can do in the
patches queue. Marek, thanks for the lesson. Anyway seems that in USB
part we have already several -1 return.
Michael
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue
On Sunday, September 21, 2014 at 10:00:24 PM, Michael Trimarchi wrote:
Hi Marek
On Sun, Sep 21, 2014 at 9:36 PM, Marek Vasut ma...@denx.de wrote:
On Sunday, September 21, 2014 at 07:53:35 PM, Hans de Goede wrote:
Hi,
[...]
- if (length usb_maxpacket(dev, pipe)) {
- printf(%s: Interrupt transfers requiring several
- transactions are not supported.\n,
__func__); - return -1;
- }
-
queue = create_int_queue(dev, pipe, 1, length, buffer);
+ if (!queue)
+ return -1;
Can you return a more consistent error code?
I'm just moving code around, and returning the same error code as
before. Surely changing the error code belongs in another patch ?
Yes, full ACK. This is exactly a prime examply where squashing two fixes
into one patch would break bisectability absolutely perfectly.
Agree on separated patch, I have just ask if Hans can do in the
patches queue. Marek, thanks for the lesson. Anyway seems that in USB
part we have already several -1 return.
You know how it goes, patches are welcome ;-)
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue
Preperation patch to use create_int_queue outside of ehci-hcd.c .
Signed-off-by: Hans de Goede hdego...@redhat.com
---
drivers/usb/host/ehci-hcd.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 20830d7..cf3e3c0 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned long
pipe, int queuesize,
struct int_queue *result = NULL;
int i;
+ /*
+* Interrupt transfers requiring several transactions are not supported
+* because bInterval is ignored.
+*
+* Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2
+* = PKT_ALIGN if several qTDs are required, while the USB
+* specification does not constrain this for interrupt transfers. That
+* means that ehci_submit_async() would support interrupt transfers
+* requiring several transactions only as long as the transfer size does
+* not require more than a single qTD.
+*/
+ if (elementsize usb_maxpacket(dev, pipe)) {
+ printf(%s: xfers requiring several transactions are not
supported.\n,
+ __func__);
+ return NULL;
+ }
+
debug(Enter create_int_queue\n);
if (usb_pipetype(pipe) != PIPE_INTERRUPT) {
debug(non-interrupt pipe (type=%lu), usb_pipetype(pipe));
@@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long
pipe, void *buffer,
debug(dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d,
dev, pipe, buffer, length, interval);
- /*
-* Interrupt transfers requiring several transactions are not supported
-* because bInterval is ignored.
-*
-* Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2
-* = PKT_ALIGN if several qTDs are required, while the USB
-* specification does not constrain this for interrupt transfers. That
-* means that ehci_submit_async() would support interrupt transfers
-* requiring several transactions only as long as the transfer size does
-* not require more than a single qTD.
-*/
- if (length usb_maxpacket(dev, pipe)) {
- printf(%s: Interrupt transfers requiring several
- transactions are not supported.\n, __func__);
- return -1;
- }
-
queue = create_int_queue(dev, pipe, 1, length, buffer);
+ if (!queue)
+ return -1;
timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
while ((backbuffer = poll_int_queue(dev, queue)) == NULL)
--
2.1.0
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue
Hi
On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede hdego...@redhat.com wrote:
Preperation patch to use create_int_queue outside of ehci-hcd.c .
Signed-off-by: Hans de Goede hdego...@redhat.com
---
drivers/usb/host/ehci-hcd.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 20830d7..cf3e3c0 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned long
pipe, int queuesize,
struct int_queue *result = NULL;
int i;
+ /*
+* Interrupt transfers requiring several transactions are not
supported
+* because bInterval is ignored.
+*
+* Also, ehci_submit_async() relies on wMaxPacketSize being a power
of 2
+* = PKT_ALIGN if several qTDs are required, while the USB
+* specification does not constrain this for interrupt transfers. That
+* means that ehci_submit_async() would support interrupt transfers
+* requiring several transactions only as long as the transfer size
does
+* not require more than a single qTD.
+*/
+ if (elementsize usb_maxpacket(dev, pipe)) {
+ printf(%s: xfers requiring several transactions are not
supported.\n,
+ __func__);
+ return NULL;
+ }
+
debug(Enter create_int_queue\n);
if (usb_pipetype(pipe) != PIPE_INTERRUPT) {
debug(non-interrupt pipe (type=%lu), usb_pipetype(pipe));
@@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long
pipe, void *buffer,
debug(dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d,
dev, pipe, buffer, length, interval);
- /*
-* Interrupt transfers requiring several transactions are not
supported
-* because bInterval is ignored.
-*
-* Also, ehci_submit_async() relies on wMaxPacketSize being a power
of 2
-* = PKT_ALIGN if several qTDs are required, while the USB
-* specification does not constrain this for interrupt transfers. That
-* means that ehci_submit_async() would support interrupt transfers
-* requiring several transactions only as long as the transfer size
does
-* not require more than a single qTD.
-*/
- if (length usb_maxpacket(dev, pipe)) {
- printf(%s: Interrupt transfers requiring several
- transactions are not supported.\n, __func__);
- return -1;
- }
-
queue = create_int_queue(dev, pipe, 1, length, buffer);
+ if (!queue)
+ return -1;
Can you return a more consistent error code?
Michael
timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
while ((backbuffer = poll_int_queue(dev, queue)) == NULL)
--
2.1.0
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
