Re: [PATCH v2 01/10] usb: host: ehci-sched: refactor scan_isoc function

2015-12-13 Thread Geyslan G. Bem 
2015-12-13 12:29 GMT-03:00 Alan Stern :
> On Sun, 13 Dec 2015, Geyslan G. Bem wrote:
>
>> This patch removes an infinite 'for' loop and makes use of the already
>> existing 'restart' tag instead, reducing one leading tab.
>>
>> It also puts the easier evaluation (live variable) to be the first in
>> two conditionals.
>>
>>   if (live && frame == now_frame) { ...
>>
>>   if (live && ((frame == now_frame) || ...
>
> You should never do this.  The first test in a conditional should be
> the one that is most likely to cause the expression to fail.  In this
> case, live is almost always true whereas frame == now_frame is true
> only some percentage of the time.  Therefore the frame == now_frame
> test should come first.
I see.
Doing v3 and undoing the reordering.

>
> Alan Stern
>



-- 
Regards,

Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 01/10] usb: host: ehci-sched: refactor scan_isoc function

2015-12-13 Thread Alan Stern 
On Sun, 13 Dec 2015, Geyslan G. Bem wrote:

> This patch removes an infinite 'for' loop and makes use of the already
> existing 'restart' tag instead, reducing one leading tab.
> 
> It also puts the easier evaluation (live variable) to be the first in
> two conditionals.
> 
>   if (live && frame == now_frame) { ...
> 
>   if (live && ((frame == now_frame) || ...

You should never do this.  The first test in a conditional should be 
the one that is most likely to cause the expression to fail.  In this 
case, live is almost always true whereas frame == now_frame is true 
only some percentage of the time.  Therefore the frame == now_frame 
test should come first.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 01/10] usb: host: ehci-sched: refactor scan_isoc function

2015-12-13 Thread Geyslan G. Bem 
2015-12-13 12:29 GMT-03:00 Alan Stern :
> On Sun, 13 Dec 2015, Geyslan G. Bem wrote:
>
>> This patch removes an infinite 'for' loop and makes use of the already
>> existing 'restart' tag instead, reducing one leading tab.
>>
>> It also puts the easier evaluation (live variable) to be the first in
>> two conditionals.
>>
>>   if (live && frame == now_frame) { ...
>>
>>   if (live && ((frame == now_frame) || ...
>
> You should never do this.  The first test in a conditional should be
> the one that is most likely to cause the expression to fail.  In this
> case, live is almost always true whereas frame == now_frame is true
> only some percentage of the time.  Therefore the frame == now_frame
> test should come first.
I see.
Doing v3 and undoing the reordering.

>
> Alan Stern
>



-- 
Regards,

Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 01/10] usb: host: ehci-sched: refactor scan_isoc function

2015-12-13 Thread Alan Stern 
On Sun, 13 Dec 2015, Geyslan G. Bem wrote:

> This patch removes an infinite 'for' loop and makes use of the already
> existing 'restart' tag instead, reducing one leading tab.
> 
> It also puts the easier evaluation (live variable) to be the first in
> two conditionals.
> 
>   if (live && frame == now_frame) { ...
> 
>   if (live && ((frame == now_frame) || ...

You should never do this.  The first test in a conditional should be 
the one that is most likely to cause the expression to fail.  In this 
case, live is almost always true whereas frame == now_frame is true 
only some percentage of the time.  Therefore the frame == now_frame 
test should come first.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 01/10] usb: host: ehci-sched: refactor scan_isoc function

2015-12-12 Thread Geyslan G. Bem 
This patch removes an infinite 'for' loop and makes use of the already
existing 'restart' tag instead, reducing one leading tab.

It also puts the easier evaluation (live variable) to be the first in
two conditionals.

  if (live && frame == now_frame) { ...

  if (live && ((frame == now_frame) || ...

The comments and code were corrected conforming file coding style.

Tested by compilation only.
Caught by checkpatch:
 WARNING: Too many leading tabs - consider code refactoring

Signed-off-by: Geyslan G. Bem 
---
  v2:
Fixes file coding indentation.
Puts 'live' variable as the first evaluation in two ifs.
---
 drivers/usb/host/ehci-sched.c | 201 +-
 1 file changed, 100 insertions(+), 101 deletions(-)

diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f9a3327..eb14143 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -2379,9 +2379,11 @@ static int sitd_submit (struct ehci_hcd *ehci, struct 
urb *urb,
 
 static void scan_isoc(struct ehci_hcd *ehci)
 {
-   unsigneduf, now_frame, frame;
-   unsignedfmask = ehci->periodic_size - 1;
-   boolmodified, live;
+   unsigneduf, now_frame, frame;
+   unsignedfmask = ehci->periodic_size - 1;
+   boolmodified, live;
+   union ehci_shadow   q, *q_p;
+   __hc32  type, *hw_p;
 
/*
 * When running, scan from last scan point up to "now"
@@ -2399,119 +2401,116 @@ static void scan_isoc(struct ehci_hcd *ehci)
ehci->now_frame = now_frame;
 
frame = ehci->last_iso_frame;
-   for (;;) {
-   union ehci_shadow   q, *q_p;
-   __hc32  type, *hw_p;
 
 restart:
-   /* scan each element in frame's queue for completions */
-   q_p = >pshadow [frame];
-   hw_p = >periodic [frame];
-   q.ptr = q_p->ptr;
-   type = Q_NEXT_TYPE(ehci, *hw_p);
-   modified = false;
-
-   while (q.ptr != NULL) {
-   switch (hc32_to_cpu(ehci, type)) {
-   case Q_TYPE_ITD:
-   /* If this ITD is still active, leave it for
-* later processing ... check the next entry.
-* No need to check for activity unless the
-* frame is current.
-*/
-   if (frame == now_frame && live) {
-   rmb();
-   for (uf = 0; uf < 8; uf++) {
-   if (q.itd->hw_transaction[uf] &
-   ITD_ACTIVE(ehci))
-   break;
-   }
-   if (uf < 8) {
-   q_p = >itd_next;
-   hw_p = >hw_next;
-   type = Q_NEXT_TYPE(ehci,
-   q.itd->hw_next);
-   q = *q_p;
+   /* Scan each element in frame's queue for completions */
+   q_p = >pshadow[frame];
+   hw_p = >periodic[frame];
+   q.ptr = q_p->ptr;
+   type = Q_NEXT_TYPE(ehci, *hw_p);
+   modified = false;
+
+   while (q.ptr != NULL) {
+   switch (hc32_to_cpu(ehci, type)) {
+   case Q_TYPE_ITD:
+   /*
+* If this ITD is still active, leave it for
+* later processing ... check the next entry.
+* No need to check for activity unless the
+* frame is current.
+*/
+   if (live && frame == now_frame) {
+   rmb();
+   for (uf = 0; uf < 8; uf++) {
+   if (q.itd->hw_transaction[uf] &
+   ITD_ACTIVE(ehci))
break;
-   }
}
-
-   /* Take finished ITDs out of the schedule
-* and process them:  recycle, maybe report
-* URB completion.  HC won't cache the
-* pointer for much longer, if at all.
-*/
-   *q_p = q.itd->itd_next;
-   if (!ehci->use_dummy_qh ||
-   q.itd->hw_next != EHCI_LIST_END(ehci))
-

[PATCH v2 01/10] usb: host: ehci-sched: refactor scan_isoc function

2015-12-12 Thread Geyslan G. Bem 
This patch removes an infinite 'for' loop and makes use of the already
existing 'restart' tag instead, reducing one leading tab.

It also puts the easier evaluation (live variable) to be the first in
two conditionals.

  if (live && frame == now_frame) { ...

  if (live && ((frame == now_frame) || ...

The comments and code were corrected conforming file coding style.

Tested by compilation only.
Caught by checkpatch:
 WARNING: Too many leading tabs - consider code refactoring

Signed-off-by: Geyslan G. Bem 
---
  v2:
Fixes file coding indentation.
Puts 'live' variable as the first evaluation in two ifs.
---
 drivers/usb/host/ehci-sched.c | 201 +-
 1 file changed, 100 insertions(+), 101 deletions(-)

diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f9a3327..eb14143 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -2379,9 +2379,11 @@ static int sitd_submit (struct ehci_hcd *ehci, struct 
urb *urb,
 
 static void scan_isoc(struct ehci_hcd *ehci)
 {
-   unsigneduf, now_frame, frame;
-   unsignedfmask = ehci->periodic_size - 1;
-   boolmodified, live;
+   unsigneduf, now_frame, frame;
+   unsignedfmask = ehci->periodic_size - 1;
+   boolmodified, live;
+   union ehci_shadow   q, *q_p;
+   __hc32  type, *hw_p;
 
/*
 * When running, scan from last scan point up to "now"
@@ -2399,119 +2401,116 @@ static void scan_isoc(struct ehci_hcd *ehci)
ehci->now_frame = now_frame;
 
frame = ehci->last_iso_frame;
-   for (;;) {
-   union ehci_shadow   q, *q_p;
-   __hc32  type, *hw_p;
 
 restart:
-   /* scan each element in frame's queue for completions */
-   q_p = >pshadow [frame];
-   hw_p = >periodic [frame];
-   q.ptr = q_p->ptr;
-   type = Q_NEXT_TYPE(ehci, *hw_p);
-   modified = false;
-
-   while (q.ptr != NULL) {
-   switch (hc32_to_cpu(ehci, type)) {
-   case Q_TYPE_ITD:
-   /* If this ITD is still active, leave it for
-* later processing ... check the next entry.
-* No need to check for activity unless the
-* frame is current.
-*/
-   if (frame == now_frame && live) {
-   rmb();
-   for (uf = 0; uf < 8; uf++) {
-   if (q.itd->hw_transaction[uf] &
-   ITD_ACTIVE(ehci))
-   break;
-   }
-   if (uf < 8) {
-   q_p = >itd_next;
-   hw_p = >hw_next;
-   type = Q_NEXT_TYPE(ehci,
-   q.itd->hw_next);
-   q = *q_p;
+   /* Scan each element in frame's queue for completions */
+   q_p = >pshadow[frame];
+   hw_p = >periodic[frame];
+   q.ptr = q_p->ptr;
+   type = Q_NEXT_TYPE(ehci, *hw_p);
+   modified = false;
+
+   while (q.ptr != NULL) {
+   switch (hc32_to_cpu(ehci, type)) {
+   case Q_TYPE_ITD:
+   /*
+* If this ITD is still active, leave it for
+* later processing ... check the next entry.
+* No need to check for activity unless the
+* frame is current.
+*/
+   if (live && frame == now_frame) {
+   rmb();
+   for (uf = 0; uf < 8; uf++) {
+   if (q.itd->hw_transaction[uf] &
+   ITD_ACTIVE(ehci))
break;
-   }
}
-
-   /* Take finished ITDs out of the schedule
-* and process them:  recycle, maybe report
-* URB completion.  HC won't cache the
-* pointer for much longer, if at all.
-*/
-   *q_p = q.itd->itd_next;
-   if (!ehci->use_dummy_qh ||
-   q.itd->hw_next !=