* [PATCH] usb: xhci: Fix TRB transfer length macro used for Event TRB.
@ 2013-03-20 10:18 Vivek Gautam
2013-03-20 17:49 ` Sarah Sharp
0 siblings, 1 reply; 4+ messages in thread
From: Vivek Gautam @ 2013-03-20 10:18 UTC (permalink / raw)
To: linux-usb; +Cc: linux-kernel, gregkh, sarah.a.sharp
Use proper macro while extracting TRB transfer length from
Transfer event TRBs. Adding a macro EVENT_TRB_LEN (bits 0:23)
for the same, and use it instead of TRB_LEN (bits 0:16) in
case of event TRBs.
Signed-off-by: Vivek gautam <gautam.vivek@samsung.com>
---
drivers/usb/host/xhci-ring.c | 45 +++++++++++++++++++++++------------------
drivers/usb/host/xhci.h | 4 +++
2 files changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 8828754..fe59a30 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2027,8 +2027,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
if (event_trb != ep_ring->dequeue &&
event_trb != td->last_trb)
td->urb->actual_length =
- td->urb->transfer_buffer_length
- - TRB_LEN(le32_to_cpu(event->transfer_len));
+ td->urb->transfer_buffer_length -
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
else
td->urb->actual_length = 0;
@@ -2060,7 +2060,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
/* Maybe the event was for the data stage? */
td->urb->actual_length =
td->urb->transfer_buffer_length -
- TRB_LEN(le32_to_cpu(event->transfer_len));
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
xhci_dbg(xhci, "Waiting for status "
"stage event\n");
return 0;
@@ -2096,7 +2096,7 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
/* handle completion code */
switch (trb_comp_code) {
case COMP_SUCCESS:
- if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
+ if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
frame->status = 0;
break;
}
@@ -2137,11 +2137,13 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
cur_seg = ep_ring->deq_seg; cur_trb != event_trb;
next_trb(xhci, ep_ring, &cur_seg, &cur_trb)) {
if (!TRB_TYPE_NOOP_LE32(cur_trb->generic.field[3]) &&
- !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3]))
- len += TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]));
+ !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3])) {
+ len += EVENT_TRB_LEN(le32_to_cpu
+ (cur_trb->generic.field[2]));
+ }
}
- len += TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
- TRB_LEN(le32_to_cpu(event->transfer_len));
+ len += EVENT_TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
if (trb_comp_code != COMP_STOP_INVAL) {
frame->actual_length = len;
@@ -2199,7 +2201,7 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
case COMP_SUCCESS:
/* Double check that the HW transferred everything. */
if (event_trb != td->last_trb ||
- TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
xhci_warn(xhci, "WARN Successful completion "
"on short TX\n");
if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
@@ -2225,20 +2227,21 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
if (trb_comp_code == COMP_SHORT_TX)
xhci_dbg(xhci, "ep %#x - asked for %d bytes, "
"%d bytes untransferred\n",
- td->urb->ep->desc.bEndpointAddress,
- td->urb->transfer_buffer_length,
- TRB_LEN(le32_to_cpu(event->transfer_len)));
+ td->urb->ep->desc.bEndpointAddress,
+ td->urb->transfer_buffer_length,
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)));
/* Fast path - was this the last TRB in the TD for this URB? */
if (event_trb == td->last_trb) {
- if (TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
+ if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
td->urb->actual_length =
td->urb->transfer_buffer_length -
- TRB_LEN(le32_to_cpu(event->transfer_len));
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
if (td->urb->transfer_buffer_length <
td->urb->actual_length) {
xhci_warn(xhci, "HC gave bad length "
"of %d bytes left\n",
- TRB_LEN(le32_to_cpu(event->transfer_len)));
+ EVENT_TRB_LEN(le32_to_cpu
+ (event->transfer_len)));
td->urb->actual_length = 0;
if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
*status = -EREMOTEIO;
@@ -2270,17 +2273,19 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
cur_trb != event_trb;
next_trb(xhci, ep_ring, &cur_seg, &cur_trb)) {
if (!TRB_TYPE_NOOP_LE32(cur_trb->generic.field[3]) &&
- !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3]))
+ !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3])) {
td->urb->actual_length +=
- TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]));
+ EVENT_TRB_LEN(le32_to_cpu
+ (cur_trb->generic.field[2]));
+ }
}
/* If the ring didn't stop on a Link or No-op TRB, add
* in the actual bytes transferred from the Normal TRB
*/
if (trb_comp_code != COMP_STOP_INVAL)
td->urb->actual_length +=
- TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
- TRB_LEN(le32_to_cpu(event->transfer_len));
+ EVENT_TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]))
+ - EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
}
return finish_td(xhci, td, event_trb, event, ep, status, false);
@@ -2368,7 +2373,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
* transfer type
*/
case COMP_SUCCESS:
- if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
+ if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
break;
if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
trb_comp_code = COMP_SHORT_TX;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f791bd0..61f71ed 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -972,6 +972,10 @@ struct xhci_transfer_event {
__le32 flags;
};
+/* Transfer event TRB length bit mask */
+/* bits 0:23 */
+#define EVENT_TRB_LEN(p) ((p) & 0xffffff)
+
/** Transfer Event bit fields **/
#define TRB_TO_EP_ID(p) (((p) >> 16) & 0x1f)
--
1.7.6.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: xhci: Fix TRB transfer length macro used for Event TRB.
2013-03-20 10:18 [PATCH] usb: xhci: Fix TRB transfer length macro used for Event TRB Vivek Gautam
@ 2013-03-20 17:49 ` Sarah Sharp
2013-03-21 5:03 ` Vivek Gautam
0 siblings, 1 reply; 4+ messages in thread
From: Sarah Sharp @ 2013-03-20 17:49 UTC (permalink / raw)
To: Vivek Gautam; +Cc: linux-usb, linux-kernel, gregkh
On Wed, Mar 20, 2013 at 03:48:40PM +0530, Vivek Gautam wrote:
> Use proper macro while extracting TRB transfer length from
> Transfer event TRBs. Adding a macro EVENT_TRB_LEN (bits 0:23)
> for the same, and use it instead of TRB_LEN (bits 0:16) in
> case of event TRBs.
Thanks for the patch! Comments below.
> Signed-off-by: Vivek gautam <gautam.vivek@samsung.com>
> ---
> drivers/usb/host/xhci-ring.c | 45 +++++++++++++++++++++++------------------
> drivers/usb/host/xhci.h | 4 +++
> 2 files changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 8828754..fe59a30 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2027,8 +2027,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> if (event_trb != ep_ring->dequeue &&
> event_trb != td->last_trb)
> td->urb->actual_length =
> - td->urb->transfer_buffer_length
> - - TRB_LEN(le32_to_cpu(event->transfer_len));
> + td->urb->transfer_buffer_length -
> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
This looks fine.
> else
> td->urb->actual_length = 0;
>
> @@ -2060,7 +2060,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> /* Maybe the event was for the data stage? */
> td->urb->actual_length =
> td->urb->transfer_buffer_length -
> - TRB_LEN(le32_to_cpu(event->transfer_len));
> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
Fine
> xhci_dbg(xhci, "Waiting for status "
> "stage event\n");
> return 0;
> @@ -2096,7 +2096,7 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
> /* handle completion code */
> switch (trb_comp_code) {
> case COMP_SUCCESS:
> - if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
> + if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
Fine
> frame->status = 0;
> break;
> }
> @@ -2137,11 +2137,13 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
This bit of code is looping through the endpoint ring, and cur_trb
points to a transfer TRB on the endpoint ring. We're adding up the
transfer buffer lengths, up to the TRB that had a completion event.
> cur_seg = ep_ring->deq_seg; cur_trb != event_trb;
> next_trb(xhci, ep_ring, &cur_seg, &cur_trb)) {
> if (!TRB_TYPE_NOOP_LE32(cur_trb->generic.field[3]) &&
> - !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3]))
> - len += TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]));
> + !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3])) {
> + len += EVENT_TRB_LEN(le32_to_cpu
> + (cur_trb->generic.field[2]));
> + }
In this case, cur_trb points to a transfer TRB, not an event TRB. So
this instance still needs to use the TRB_LEN macro.
Adding the extra braces here makes it hard to review. Please don't add
extra braces, or do so in a second patch if it really bothers you.
> }
> - len += TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
> - TRB_LEN(le32_to_cpu(event->transfer_len));
> + len += EVENT_TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
Here, cur_trb points to a transfer TRB, and event points to an event
TRB. So you need to use the TRB_LEN macro for the first line, and the
EVENT_TRB_LEN macro for the second line.
>
> if (trb_comp_code != COMP_STOP_INVAL) {
> frame->actual_length = len;
> @@ -2199,7 +2201,7 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
> case COMP_SUCCESS:
> /* Double check that the HW transferred everything. */
> if (event_trb != td->last_trb ||
> - TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
Fine.
> xhci_warn(xhci, "WARN Successful completion "
> "on short TX\n");
> if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
> @@ -2225,20 +2227,21 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
> if (trb_comp_code == COMP_SHORT_TX)
> xhci_dbg(xhci, "ep %#x - asked for %d bytes, "
> "%d bytes untransferred\n",
> - td->urb->ep->desc.bEndpointAddress,
> - td->urb->transfer_buffer_length,
> - TRB_LEN(le32_to_cpu(event->transfer_len)));
> + td->urb->ep->desc.bEndpointAddress,
> + td->urb->transfer_buffer_length,
> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)));
Please don't change indentation. I know it's a pain to keep lines
within 80 chars, but I would rather have a longer line than non-standard
indentation. The macro change is fine.
> /* Fast path - was this the last TRB in the TD for this URB? */
> if (event_trb == td->last_trb) {
> - if (TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
> + if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
> td->urb->actual_length =
> td->urb->transfer_buffer_length -
> - TRB_LEN(le32_to_cpu(event->transfer_len));
> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
> if (td->urb->transfer_buffer_length <
> td->urb->actual_length) {
> xhci_warn(xhci, "HC gave bad length "
> "of %d bytes left\n",
> - TRB_LEN(le32_to_cpu(event->transfer_len)));
> + EVENT_TRB_LEN(le32_to_cpu
> + (event->transfer_len)));
This chunk is fine.
> td->urb->actual_length = 0;
> if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
> *status = -EREMOTEIO;
> @@ -2270,17 +2273,19 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
> cur_trb != event_trb;
> next_trb(xhci, ep_ring, &cur_seg, &cur_trb)) {
> if (!TRB_TYPE_NOOP_LE32(cur_trb->generic.field[3]) &&
> - !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3]))
> + !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3])) {
> td->urb->actual_length +=
> - TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]));
> + EVENT_TRB_LEN(le32_to_cpu
> + (cur_trb->generic.field[2]));
> + }
Again, this is walking the endpoint ring's transfer TRBs to add up the
transferred length, and thus those last two lines should still use the
TRB_LEN macro.
Also, don't add extra braces to one-line blocks. And why was the
indentation on the curly braces so odd? Please only use tabs.
> }
> /* If the ring didn't stop on a Link or No-op TRB, add
> * in the actual bytes transferred from the Normal TRB
> */
> if (trb_comp_code != COMP_STOP_INVAL)
> td->urb->actual_length +=
> - TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
> - TRB_LEN(le32_to_cpu(event->transfer_len));
> + EVENT_TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]))
> + - EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
cur_trb needs to use the TRB_LEN, and event needs to use the
EVENT_TRB_LEN macro.
> }
>
> return finish_td(xhci, td, event_trb, event, ep, status, false);
> @@ -2368,7 +2373,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> * transfer type
> */
> case COMP_SUCCESS:
> - if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
> + if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
Fine.
> break;
> if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
> trb_comp_code = COMP_SHORT_TX;
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index f791bd0..61f71ed 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -972,6 +972,10 @@ struct xhci_transfer_event {
> __le32 flags;
> };
>
> +/* Transfer event TRB length bit mask */
> +/* bits 0:23 */
> +#define EVENT_TRB_LEN(p) ((p) & 0xffffff)
> +
> /** Transfer Event bit fields **/
> #define TRB_TO_EP_ID(p) (((p) >> 16) & 0x1f)
>
> --
> 1.7.6.5
>
Can you correct these and resubmit? Thanks!
Sarah Sharp
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: xhci: Fix TRB transfer length macro used for Event TRB.
2013-03-20 17:49 ` Sarah Sharp
@ 2013-03-21 5:03 ` Vivek Gautam
2013-03-21 6:36 ` [PATCH v2] " Vivek Gautam
0 siblings, 1 reply; 4+ messages in thread
From: Vivek Gautam @ 2013-03-21 5:03 UTC (permalink / raw)
To: Sarah Sharp; +Cc: Vivek Gautam, linux-usb, linux-kernel, gregkh
Hi,
On Wed, Mar 20, 2013 at 11:19 PM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> On Wed, Mar 20, 2013 at 03:48:40PM +0530, Vivek Gautam wrote:
>> Use proper macro while extracting TRB transfer length from
>> Transfer event TRBs. Adding a macro EVENT_TRB_LEN (bits 0:23)
>> for the same, and use it instead of TRB_LEN (bits 0:16) in
>> case of event TRBs.
>
> Thanks for the patch! Comments below.
>
>> Signed-off-by: Vivek gautam <gautam.vivek@samsung.com>
>> ---
>> drivers/usb/host/xhci-ring.c | 45 +++++++++++++++++++++++------------------
>> drivers/usb/host/xhci.h | 4 +++
>> 2 files changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 8828754..fe59a30 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -2027,8 +2027,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
>> if (event_trb != ep_ring->dequeue &&
>> event_trb != td->last_trb)
>> td->urb->actual_length =
>> - td->urb->transfer_buffer_length
>> - - TRB_LEN(le32_to_cpu(event->transfer_len));
>> + td->urb->transfer_buffer_length -
>> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
>
> This looks fine.
>
>> else
>> td->urb->actual_length = 0;
>>
>> @@ -2060,7 +2060,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
>> /* Maybe the event was for the data stage? */
>> td->urb->actual_length =
>> td->urb->transfer_buffer_length -
>> - TRB_LEN(le32_to_cpu(event->transfer_len));
>> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
>
> Fine
>
>> xhci_dbg(xhci, "Waiting for status "
>> "stage event\n");
>> return 0;
>> @@ -2096,7 +2096,7 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
>> /* handle completion code */
>> switch (trb_comp_code) {
>> case COMP_SUCCESS:
>> - if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
>> + if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
>
> Fine
>
>> frame->status = 0;
>> break;
>> }
>> @@ -2137,11 +2137,13 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
>
> This bit of code is looping through the endpoint ring, and cur_trb
> points to a transfer TRB on the endpoint ring. We're adding up the
> transfer buffer lengths, up to the TRB that had a completion event.
>
>> cur_seg = ep_ring->deq_seg; cur_trb != event_trb;
>> next_trb(xhci, ep_ring, &cur_seg, &cur_trb)) {
>> if (!TRB_TYPE_NOOP_LE32(cur_trb->generic.field[3]) &&
>> - !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3]))
>> - len += TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]));
>> + !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3])) {
>> + len += EVENT_TRB_LEN(le32_to_cpu
>> + (cur_trb->generic.field[2]));
>> + }
>
> In this case, cur_trb points to a transfer TRB, not an event TRB. So
> this instance still needs to use the TRB_LEN macro.
Right, thanks.
>
> Adding the extra braces here makes it hard to review. Please don't add
> extra braces, or do so in a second patch if it really bothers you.
Sure, will remove these.
>
>> }
>> - len += TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
>> - TRB_LEN(le32_to_cpu(event->transfer_len));
>> + len += EVENT_TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
>> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
>
> Here, cur_trb points to a transfer TRB, and event points to an event
> TRB. So you need to use the TRB_LEN macro for the first line, and the
> EVENT_TRB_LEN macro for the second line.
Ok
>
>>
>> if (trb_comp_code != COMP_STOP_INVAL) {
>> frame->actual_length = len;
>> @@ -2199,7 +2201,7 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
>> case COMP_SUCCESS:
>> /* Double check that the HW transferred everything. */
>> if (event_trb != td->last_trb ||
>> - TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
>> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
>
> Fine.
>
>> xhci_warn(xhci, "WARN Successful completion "
>> "on short TX\n");
>> if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
>> @@ -2225,20 +2227,21 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
>> if (trb_comp_code == COMP_SHORT_TX)
>> xhci_dbg(xhci, "ep %#x - asked for %d bytes, "
>> "%d bytes untransferred\n",
>> - td->urb->ep->desc.bEndpointAddress,
>> - td->urb->transfer_buffer_length,
>> - TRB_LEN(le32_to_cpu(event->transfer_len)));
>> + td->urb->ep->desc.bEndpointAddress,
>> + td->urb->transfer_buffer_length,
>> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)));
>
> Please don't change indentation. I know it's a pain to keep lines
> within 80 chars, but I would rather have a longer line than non-standard
> indentation. The macro change is fine.
Sure, will keep the indentation intact.
>
>> /* Fast path - was this the last TRB in the TD for this URB? */
>> if (event_trb == td->last_trb) {
>> - if (TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
>> + if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
>> td->urb->actual_length =
>> td->urb->transfer_buffer_length -
>> - TRB_LEN(le32_to_cpu(event->transfer_len));
>> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
>> if (td->urb->transfer_buffer_length <
>> td->urb->actual_length) {
>> xhci_warn(xhci, "HC gave bad length "
>> "of %d bytes left\n",
>> - TRB_LEN(le32_to_cpu(event->transfer_len)));
>> + EVENT_TRB_LEN(le32_to_cpu
>> + (event->transfer_len)));
>
> This chunk is fine.
>
>> td->urb->actual_length = 0;
>> if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
>> *status = -EREMOTEIO;
>> @@ -2270,17 +2273,19 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
>> cur_trb != event_trb;
>> next_trb(xhci, ep_ring, &cur_seg, &cur_trb)) {
>> if (!TRB_TYPE_NOOP_LE32(cur_trb->generic.field[3]) &&
>> - !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3]))
>> + !TRB_TYPE_LINK_LE32(cur_trb->generic.field[3])) {
>> td->urb->actual_length +=
>> - TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]));
>> + EVENT_TRB_LEN(le32_to_cpu
>> + (cur_trb->generic.field[2]));
>> + }
>
> Again, this is walking the endpoint ring's transfer TRBs to add up the
> transferred length, and thus those last two lines should still use the
> TRB_LEN macro.
>
> Also, don't add extra braces to one-line blocks. And why was the
> indentation on the curly braces so odd? Please only use tabs.
Sure
>
>> }
>> /* If the ring didn't stop on a Link or No-op TRB, add
>> * in the actual bytes transferred from the Normal TRB
>> */
>> if (trb_comp_code != COMP_STOP_INVAL)
>> td->urb->actual_length +=
>> - TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
>> - TRB_LEN(le32_to_cpu(event->transfer_len));
>> + EVENT_TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]))
>> + - EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
>
> cur_trb needs to use the TRB_LEN, and event needs to use the
> EVENT_TRB_LEN macro.
>
>> }
>>
>> return finish_td(xhci, td, event_trb, event, ep, status, false);
>> @@ -2368,7 +2373,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>> * transfer type
>> */
>> case COMP_SUCCESS:
>> - if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
>> + if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
>
> Fine.
>
>> break;
>> if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
>> trb_comp_code = COMP_SHORT_TX;
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index f791bd0..61f71ed 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -972,6 +972,10 @@ struct xhci_transfer_event {
>> __le32 flags;
>> };
>>
>> +/* Transfer event TRB length bit mask */
>> +/* bits 0:23 */
>> +#define EVENT_TRB_LEN(p) ((p) & 0xffffff)
>> +
>> /** Transfer Event bit fields **/
>> #define TRB_TO_EP_ID(p) (((p) >> 16) & 0x1f)
>>
>> --
>> 1.7.6.5
>>
>
> Can you correct these and resubmit? Thanks!
>
Thanks for the review Sarah. I shall update and resubmit this patch.
> Sarah Sharp
> --
> 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
--
Thanks & Regards
Vivek
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] usb: xhci: Fix TRB transfer length macro used for Event TRB.
2013-03-21 5:03 ` Vivek Gautam
@ 2013-03-21 6:36 ` Vivek Gautam
0 siblings, 0 replies; 4+ messages in thread
From: Vivek Gautam @ 2013-03-21 6:36 UTC (permalink / raw)
To: linux-usb; +Cc: linux-kernel, gregkh, sarah.a.sharp
Use proper macro while extracting TRB transfer length from
Transfer event TRBs. Adding a macro EVENT_TRB_LEN (bits 0:23)
for the same, and use it instead of TRB_LEN (bits 0:16) in
case of event TRBs.
Signed-off-by: Vivek gautam <gautam.vivek@samsung.com>
---
Hi Sarah,
Updated the patch as suggested.
There are two 80 characters warnings although ;-)
Thanks!
drivers/usb/host/xhci-ring.c | 24 ++++++++++++------------
drivers/usb/host/xhci.h | 4 ++++
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 8828754..30a2489 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2027,8 +2027,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
if (event_trb != ep_ring->dequeue &&
event_trb != td->last_trb)
td->urb->actual_length =
- td->urb->transfer_buffer_length
- - TRB_LEN(le32_to_cpu(event->transfer_len));
+ td->urb->transfer_buffer_length -
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
else
td->urb->actual_length = 0;
@@ -2060,7 +2060,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
/* Maybe the event was for the data stage? */
td->urb->actual_length =
td->urb->transfer_buffer_length -
- TRB_LEN(le32_to_cpu(event->transfer_len));
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
xhci_dbg(xhci, "Waiting for status "
"stage event\n");
return 0;
@@ -2096,7 +2096,7 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
/* handle completion code */
switch (trb_comp_code) {
case COMP_SUCCESS:
- if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
+ if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0) {
frame->status = 0;
break;
}
@@ -2141,7 +2141,7 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
len += TRB_LEN(le32_to_cpu(cur_trb->generic.field[2]));
}
len += TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
- TRB_LEN(le32_to_cpu(event->transfer_len));
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
if (trb_comp_code != COMP_STOP_INVAL) {
frame->actual_length = len;
@@ -2199,7 +2199,7 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
case COMP_SUCCESS:
/* Double check that the HW transferred everything. */
if (event_trb != td->last_trb ||
- TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
xhci_warn(xhci, "WARN Successful completion "
"on short TX\n");
if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
@@ -2227,18 +2227,18 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
"%d bytes untransferred\n",
td->urb->ep->desc.bEndpointAddress,
td->urb->transfer_buffer_length,
- TRB_LEN(le32_to_cpu(event->transfer_len)));
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)));
/* Fast path - was this the last TRB in the TD for this URB? */
if (event_trb == td->last_trb) {
- if (TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
+ if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
td->urb->actual_length =
td->urb->transfer_buffer_length -
- TRB_LEN(le32_to_cpu(event->transfer_len));
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
if (td->urb->transfer_buffer_length <
td->urb->actual_length) {
xhci_warn(xhci, "HC gave bad length "
"of %d bytes left\n",
- TRB_LEN(le32_to_cpu(event->transfer_len)));
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)));
td->urb->actual_length = 0;
if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
*status = -EREMOTEIO;
@@ -2280,7 +2280,7 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
if (trb_comp_code != COMP_STOP_INVAL)
td->urb->actual_length +=
TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) -
- TRB_LEN(le32_to_cpu(event->transfer_len));
+ EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
}
return finish_td(xhci, td, event_trb, event, ep, status, false);
@@ -2368,7 +2368,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
* transfer type
*/
case COMP_SUCCESS:
- if (TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
+ if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
break;
if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
trb_comp_code = COMP_SHORT_TX;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f791bd0..61f71ed 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -972,6 +972,10 @@ struct xhci_transfer_event {
__le32 flags;
};
+/* Transfer event TRB length bit mask */
+/* bits 0:23 */
+#define EVENT_TRB_LEN(p) ((p) & 0xffffff)
+
/** Transfer Event bit fields **/
#define TRB_TO_EP_ID(p) (((p) >> 16) & 0x1f)
--
1.7.6.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-03-21 6:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-20 10:18 [PATCH] usb: xhci: Fix TRB transfer length macro used for Event TRB Vivek Gautam
2013-03-20 17:49 ` Sarah Sharp
2013-03-21 5:03 ` Vivek Gautam
2013-03-21 6:36 ` [PATCH v2] " Vivek Gautam
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.