musb_host: fix lockup on rxcsr_h_error

PATCH In review Max Uvarov 6 participants 28 messages Apr 27, 2016 – Aug 6, 2018

Subsystems (2)

USB MUSB multipoint high speed dual-role controller

lore

Share & tools

lore

Max Uvarov

author

2016-04-27 06:52 UTC #0 lore raw

PATCH v2 1/1 musb_host: fix lockup on rxcsr_h_error

Fix soft lockup when resetting remote device attached
to usb host. Configuration:
pppd -> musb hub -> usb-serial -> gsm modem
When gsm modem resets, musb rolls in incoming rx interrupts which does not give any time to other application as result it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code for setting rx stall with MUSB_RXCSR_H_WZC_BITS.

Signed-off-by Max Uvarov <[email protected]>

---
v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).

 drivers/usb/musb/musb_host.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

drivers/usb/musb/musb_host.c +5 -7

index c3d5fc9..2d9aa78 100644
@@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)
 
 		/* stall; record URB status */
 		status = -EPIPE;
+	} else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
 
-	} else if (rx_csr & MUSB_RXCSR_H_ERROR) {
-		dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
-
-		status = -EPROTO;
-		musb_writeb(epio, MUSB_RXINTERVAL, 0);
-
-	} else if (rx_csr & MUSB_RXCSR_DATAERROR) {
+		if (rx_csr & MUSB_RXCSR_H_ERROR) {
+			status = -EPROTO;
+			musb_writeb(epio, MUSB_RXINTERVAL, 0);
+		}
 
 		if (USB_ENDPOINT_XFER_ISOC != qh->type) {
 			dev_dbg(musb->controller, "RX end %d NAK timeout\n", epnum);

Signature · 1 line

1.9.1

Bin Liu

maintainer

2016-04-27 15:46 UTC #1 lore raw

Hi,

Quoted 3 lines · Max Uvarov

Fix soft lockup when resetting remote device attached to usb host. Configuration:
pppd -> musb hub -> usb-serial -> gsm modem

I have heard a few reports similar to this symptom, but never been able to reproduce it on my side.

Quoted 3 lines

When gsm modem resets, musb rolls in incoming rx interrupts which does not give any time to other application as result it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR

Have you looked where exact place in the interrupt routine the execution has stuck in?

Quoted 2 lines

and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code for setting rx stall with MUSB_RXCSR_H_WZC_BITS.

MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in musb_regs.h.

drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum)

Quoted 13 lines

Signed-off-by Max Uvarov <[email protected]>
---
v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).
 drivers/usb/musb/musb_host.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/usb/musb/musb_host.c +0 -0
index c3d5fc9..2d9aa78 100644
@@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)

What kernel do you use? This line # is away off from upstream kernel.

Quoted 6 lines

drivers/usb/musb/musb_host.c +1 -1

 		/* stall; record URB status */
 		status = -EPIPE;
+	} else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
 
-	} else if (rx_csr & MUSB_RXCSR_H_ERROR) {

drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1600

Quoted 10 lines

drivers/usb/musb/musb_host.c +4 -6

-		dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
-
-		status = -EPROTO;
-		musb_writeb(epio, MUSB_RXINTERVAL, 0);
-
-	} else if (rx_csr & MUSB_RXCSR_DATAERROR) {
+		if (rx_csr & MUSB_RXCSR_H_ERROR) {
+			status = -EPROTO;
+			musb_writeb(epio, MUSB_RXINTERVAL, 0);
+		}

Please help me to understand how this change fixes the issue. I see the most effect of the change here is directly 'goto finish' so that 'done' flag is not set, then musb_advance_schedule() is not called. Is this the case or I missed other important pieces?

Thanks,
-Bin.

Quoted 10 lines

drivers/usb/musb/musb_host.c +0 -0

 		if (USB_ENDPOINT_XFER_ISOC != qh->type) {
 			dev_dbg(musb->controller, "RX end %d NAK timeout\n", epnum);

Signature · 6 lines

1.9.1

--
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

Maxim Uvarov

author

2016-04-27 18:26 UTC #2 lore raw

2016-04-27 18:46 GMT+03:00 Bin Liu <[email protected]>:

Quoted 10 lines

Hi,
Quoted 3 lines · Max Uvarov
Fix soft lockup when resetting remote device attached to usb host. Configuration:
pppd -> musb hub -> usb-serial -> gsm modem
I have heard a few reports similar to this symptom, but never been able to reproduce it on my side.

Ok, I can reproduce it almost very easy.

Quoted 7 lines

Quoted 3 lines
When gsm modem resets, musb rolls in incoming rx interrupts which does not give any time to other application as result it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR
Have you looked where exact place in the interrupt routine the execution has stuck in?

It does not stuck. It goes to that line which print proto error over and over again and
nothing stops that. After some time kernel reports lockup. But actually it's not stuck,
all cpu time was eaten by executing that handlers.

Quoted 15 lines

Quoted 2 lines
and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code for setting rx stall with MUSB_RXCSR_H_WZC_BITS.
MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in musb_regs.h.
Quoted 8 lines
Signed-off-by Max Uvarov <[email protected]>
---
v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).
 drivers/usb/musb/musb_host.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Quoted 8 lines

drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum)
Quoted 5 lines
drivers/usb/musb/musb_host.c +0 -0
index c3d5fc9..2d9aa78 100644
@@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)
What kernel do you use? This line # is away off from upstream kernel.

I did this patch for 4.1 but 4.6 has the same problem and patch cleanly applies to the
latest torvalds/linux.git v4.6-rc5. This interrupt handler has the same code. And looks
like on 3.14 everything worked. I don't have a time to diff 2 versions. Might be regression.

Quoted 5 lines

drivers/usb/musb/musb_host.c +1 -0

              /* stall; record URB status */
              status = -EPIPE;
+     } else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {

Quoted 17 lines

drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1600
Quoted 11 lines
drivers/usb/musb/musb_host.c +4 -7
-     } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
-             dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
-
-             status = -EPROTO;
-             musb_writeb(epio, MUSB_RXINTERVAL, 0);
-
-     } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
+             if (rx_csr & MUSB_RXCSR_H_ERROR) {
+                     status = -EPROTO;
+                     musb_writeb(epio, MUSB_RXINTERVAL, 0);
+             }
Please help me to understand how this change fixes the issue. I see the most effect of the change here is directly 'goto finish' so that 'done' flag is not set, then musb_advance_schedule() is not called. Is this the case or I missed other important pieces?

Right that is the goal. On this rxcsr_h_error kernel reschedules current interrupt.
And that continues forever. For example adding msleep() can give some time for other
processes. I'm not an expert in this chip but I think that right solution in that case is not
try to reschedule and quick and allow hub to make reset and once again init all devices
(in my case ppp/pppd also shutdowns and then I bring everything up with script.). The
same behavior with dma and pio mode.

Regards,
Max.

Quoted 13 lines

Thanks,
-Bin.
Quoted 10 lines
drivers/usb/musb/musb_host.c +0 -1
              if (USB_ENDPOINT_XFER_ISOC != qh->type) {
                      dev_dbg(musb->controller, "RX end %d NAK timeout\n", epnum);
--
1.9.1
--
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

Signature · 2 lines

Best regards,
Maxim Uvarov

Bin Liu

maintainer

2016-04-27 19:14 UTC #3 lore raw

Hi,

Quoted 53 lines · Maxim Uvarov

2016-04-27 18:46 GMT+03:00 Bin Liu <[email protected]>:
Quoted 10 lines
Hi,
Quoted 3 lines · Max Uvarov
Fix soft lockup when resetting remote device attached to usb host. Configuration:
pppd -> musb hub -> usb-serial -> gsm modem
I have heard a few reports similar to this symptom, but never been able to reproduce it on my side.
Ok, I can reproduce it almost very easy.
Quoted 7 lines
Quoted 3 lines
When gsm modem resets, musb rolls in incoming rx interrupts which does not give any time to other application as result it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR
Have you looked where exact place in the interrupt routine the execution has stuck in?
It does not stuck. It goes to that line which print proto error over and over again and
nothing stops that. After some time kernel reports lockup. But actually it's not stuck,
all cpu time was eaten by executing that handlers.
Quoted 15 lines
Quoted 2 lines
and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code for setting rx stall with MUSB_RXCSR_H_WZC_BITS.
MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in musb_regs.h.
Quoted 8 lines
Signed-off-by Max Uvarov <[email protected]>
---
v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).
 drivers/usb/musb/musb_host.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
Quoted 8 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum)
Quoted 5 lines
drivers/usb/musb/musb_host.c +0 -0
index c3d5fc9..2d9aa78 100644
@@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)
What kernel do you use? This line # is away off from upstream kernel.

Quoted 3 lines

I did this patch for 4.1 but 4.6 has the same problem and patch cleanly applies to the latest torvalds/linux.git v4.6-rc5. This interrupt handler has the same code. And looks like on 3.14

Yeah, this code hasn't been chaned for year. But in general, it is prepfered to create patches on latest kernel to avoid other headache.

Quoted 27 lines

everything worked. I don't have a time to diff 2 versions. Might be regression.
Quoted 5 lines
Quoted 5 lines
drivers/usb/musb/musb_host.c +1 -0
              /* stall; record URB status */
              status = -EPIPE;
+     } else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
 
Quoted 17 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1600
Quoted 11 lines
drivers/usb/musb/musb_host.c +4 -7
-     } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
-             dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
-
-             status = -EPROTO;
-             musb_writeb(epio, MUSB_RXINTERVAL, 0);
-
-     } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
+             if (rx_csr & MUSB_RXCSR_H_ERROR) {
+                     status = -EPROTO;
+                     musb_writeb(epio, MUSB_RXINTERVAL, 0);
+             }
Please help me to understand how this change fixes the issue. I see the most effect of the change here is directly 'goto finish' so that 'done' flag is not set, then musb_advance_schedule() is not called. Is this the case or I missed other important pieces?

Quoted 2 lines

Right that is the goal. On this rxcsr_h_error kernel reschedules current interrupt. And that continues forever. For example adding

The MUSB Programming Guide says CPU should clear this MUSB_RXCSR_H_ERROR bit, but the current driver doesn't. I am wondering if this causes the controller keeps generating the same interrupt. Can you please try the following change instead to see if the lockup goes away?

@@ -1870,6 +1870,9 @@ void musb_host_rx(struct musb *musb, u8 epnum)

                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);

+ rx_csr &= ~MUSB_RXCSR_H_ERROR;
+ musb_writew(epio, MUSB_RXCSR, rx_csr);
+

        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {

Regards,
-Bin.

Quoted 28 lines

msleep() can give some time for other processes. I'm not an expert in this chip but I think that right solution in that case is not try to reschedule and quick and allow hub to make reset and once again init all devices (in my case ppp/pppd also shutdowns and then I bring everything up with script.). The same behavior with dma and pio mode.
Regards,
Max.
Quoted 13 lines
Thanks,
-Bin.
Quoted 10 lines
drivers/usb/musb/musb_host.c +0 -1
              if (USB_ENDPOINT_XFER_ISOC != qh->type) {
                      dev_dbg(musb->controller, "RX end %d NAK timeout\n", epnum);
--
1.9.1
--
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
Signature · 2 lines
Best regards,
Maxim Uvarov

Bin Liu

maintainer

2016-04-27 21:28 UTC #4 lore raw

Hi,

Quoted 99 lines · Bin Liu

Hi,
Quoted 53 lines · Maxim Uvarov
2016-04-27 18:46 GMT+03:00 Bin Liu <[email protected]>:
Quoted 10 lines
Hi,
Quoted 3 lines · Max Uvarov
Fix soft lockup when resetting remote device attached to usb host. Configuration:
pppd -> musb hub -> usb-serial -> gsm modem
I have heard a few reports similar to this symptom, but never been able to reproduce it on my side.
Ok, I can reproduce it almost very easy.
Quoted 7 lines
Quoted 3 lines
When gsm modem resets, musb rolls in incoming rx interrupts which does not give any time to other application as result it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR
Have you looked where exact place in the interrupt routine the execution has stuck in?
It does not stuck. It goes to that line which print proto error over and over again and
nothing stops that. After some time kernel reports lockup. But actually it's not stuck,
all cpu time was eaten by executing that handlers.
Quoted 15 lines
Quoted 2 lines
and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code for setting rx stall with MUSB_RXCSR_H_WZC_BITS.
MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in musb_regs.h.
Quoted 8 lines
Signed-off-by Max Uvarov <[email protected]>
---
v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).
 drivers/usb/musb/musb_host.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
Quoted 8 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum)
Quoted 5 lines
drivers/usb/musb/musb_host.c +0 -0
index c3d5fc9..2d9aa78 100644
@@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)
What kernel do you use? This line # is away off from upstream kernel.
Quoted 3 lines
I did this patch for 4.1 but 4.6 has the same problem and patch cleanly applies to the latest torvalds/linux.git v4.6-rc5. This interrupt handler has the same code. And looks like on 3.14
Yeah, this code hasn't been chaned for year. But in general, it is prepfered to create patches on latest kernel to avoid other headache.
Quoted 27 lines
everything worked. I don't have a time to diff 2 versions. Might be regression.
Quoted 5 lines
Quoted 5 lines
drivers/usb/musb/musb_host.c +1 -0
              /* stall; record URB status */
              status = -EPIPE;
+     } else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
 
Quoted 17 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1600
Quoted 11 lines
drivers/usb/musb/musb_host.c +4 -7
-     } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
-             dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
-
-             status = -EPROTO;
-             musb_writeb(epio, MUSB_RXINTERVAL, 0);
-
-     } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
+             if (rx_csr & MUSB_RXCSR_H_ERROR) {
+                     status = -EPROTO;
+                     musb_writeb(epio, MUSB_RXINTERVAL, 0);
+             }
Please help me to understand how this change fixes the issue. I see the most effect of the change here is directly 'goto finish' so that 'done' flag is not set, then musb_advance_schedule() is not called. Is this the case or I missed other important pieces?
Quoted 2 lines
Right that is the goal. On this rxcsr_h_error kernel reschedules current interrupt. And that continues forever. For example adding
The MUSB Programming Guide says CPU should clear this MUSB_RXCSR_H_ERROR bit, but the current driver doesn't. I am wondering if this causes the controller keeps generating the same interrupt. Can you please try the following change instead to see if the lockup goes away?

Review void musb_host_rx(struct musb *musb, u8 epnum) · line 1874

Quoted 5 lines

(quoted diff) +2 -0

@@ -1870,6 +1870,9 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);

+ goto finish;

Please also add the line above. I will spend more time to understand what is happening...

First of all, I don't like the idea of merging the two branches, it makes the code ugly.

Regards,
-Bin.

Quoted 36 lines

drivers/usb/musb/musb_host.c +1 -0
+
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Regards,
-Bin.
Quoted 28 lines
msleep() can give some time for other processes. I'm not an expert in this chip but I think that right solution in that case is not try to reschedule and quick and allow hub to make reset and once again init all devices (in my case ppp/pppd also shutdowns and then I bring everything up with script.). The same behavior with dma and pio mode.
Regards,
Max.
Quoted 13 lines
Thanks,
-Bin.
Quoted 10 lines
drivers/usb/musb/musb_host.c +0 -1
              if (USB_ENDPOINT_XFER_ISOC != qh->type) {
                      dev_dbg(musb->controller, "RX end %d NAK timeout\n", epnum);
--
1.9.1
--
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
Signature · 2 lines
Best regards,
Maxim Uvarov

Maxim Uvarov

author

2016-04-28 06:51 UTC #5 lore raw

2016-04-28 0:28 GMT+03:00 Bin Liu <[email protected]>:

Quoted 105 lines

Hi,
Quoted 102 lines · Bin Liu
Hi,
Quoted 53 lines · Maxim Uvarov
2016-04-27 18:46 GMT+03:00 Bin Liu <[email protected]>:
Quoted 10 lines
Hi,
Quoted 3 lines · Max Uvarov
Fix soft lockup when resetting remote device attached to usb host. Configuration:
pppd -> musb hub -> usb-serial -> gsm modem
I have heard a few reports similar to this symptom, but never been able to reproduce it on my side.
Ok, I can reproduce it almost very easy.
Quoted 7 lines
Quoted 3 lines
When gsm modem resets, musb rolls in incoming rx interrupts which does not give any time to other application as result it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR
Have you looked where exact place in the interrupt routine the execution has stuck in?
It does not stuck. It goes to that line which print proto error over and over again and
nothing stops that. After some time kernel reports lockup. But actually it's not stuck,
all cpu time was eaten by executing that handlers.
Quoted 15 lines
Quoted 2 lines
and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code for setting rx stall with MUSB_RXCSR_H_WZC_BITS.
MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in musb_regs.h.
Quoted 8 lines
Signed-off-by Max Uvarov <[email protected]>
---
v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).
 drivers/usb/musb/musb_host.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
Quoted 8 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum)
Quoted 5 lines
drivers/usb/musb/musb_host.c +0 -0
index c3d5fc9..2d9aa78 100644
@@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)
What kernel do you use? This line # is away off from upstream kernel.
Quoted 3 lines
I did this patch for 4.1 but 4.6 has the same problem and patch cleanly applies to the latest torvalds/linux.git v4.6-rc5. This interrupt handler has the same code. And looks like on 3.14
Yeah, this code hasn't been chaned for year. But in general, it is prepfered to create patches on latest kernel to avoid other headache.
Quoted 27 lines
everything worked. I don't have a time to diff 2 versions. Might be regression.
Quoted 5 lines
Quoted 5 lines
drivers/usb/musb/musb_host.c +1 -0
              /* stall; record URB status */
              status = -EPIPE;
+     } else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
 
Quoted 17 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1599
Quoted 11 lines
drivers/usb/musb/musb_host.c +4 -7
-     } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
-             dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
-
-             status = -EPROTO;
-             musb_writeb(epio, MUSB_RXINTERVAL, 0);
-
-     } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
+             if (rx_csr & MUSB_RXCSR_H_ERROR) {
+                     status = -EPROTO;
+                     musb_writeb(epio, MUSB_RXINTERVAL, 0);
+             }
Please help me to understand how this change fixes the issue. I see the most effect of the change here is directly 'goto finish' so that 'done' flag is not set, then musb_advance_schedule() is not called. Is this the case or I missed other important pieces?
Quoted 2 lines
Right that is the goal. On this rxcsr_h_error kernel reschedules current interrupt. And that continues forever. For example adding
The MUSB Programming Guide says CPU should clear this MUSB_RXCSR_H_ERROR bit, but the current driver doesn't. I am wondering if this causes the controller keeps generating the same interrupt. Can you please try the following change instead to see if the lockup goes away?
(quoted diff) +0 -0
@@ -1870,6 +1870,9 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 

Quoted 8 lines

Review void musb_host_rx(struct musb *musb, u8 epnum) · line 1874
Quoted 2 lines
(quoted diff) +2 -0
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+ goto finish;
Please also add the line above. I will spend more time to understand what is happening...

Hello Bin,

yes, it also works with that reset and go to finish:

drivers/usb/musb/musb_host.c +4 -0

index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {

That I think a key thing, which is done in other error. If that change is good for you than I'm also happy with it.

I also not sure if musb_writeb(epio, MUSB_RXINTERVAL, 0); is needed. In my case it's the same result with it and without it. In other scenarios might be reasonable...

Quoted 2 lines

First of all, I don't like the idea of merging the two branches, it makes the code ugly.

Yes, I don't like that function at all, it's too long and difficult to read if you first look on it first time. It will be good to split it on 3 small functions for each big if.

Maxim.

Quoted 40 lines

Regards,
-Bin.
Quoted 36 lines
(quoted diff) +1 -0
+
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Regards,
-Bin.
Quoted 28 lines
msleep() can give some time for other processes. I'm not an expert in this chip but I think that right solution in that case is not try to reschedule and quick and allow hub to make reset and once again init all devices (in my case ppp/pppd also shutdowns and then I bring everything up with script.). The same behavior with dma and pio mode.
Regards,
Max.
Quoted 13 lines
Thanks,
-Bin.
Quoted 10 lines
drivers/usb/musb/musb_host.c +0 -1
              if (USB_ENDPOINT_XFER_ISOC != qh->type) {
                      dev_dbg(musb->controller, "RX end %d NAK timeout\n", epnum);
--
1.9.1
--
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
--
Best regards,
Maxim Uvarov

Signature · 2 lines

Best regards,
Maxim Uvarov

Bin Liu

maintainer

2016-04-28 14:37 UTC #6 lore raw

Hi,

On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:

[snip]

Quoted 10 lines

Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +0 -0
index c3d5fc9..8cd98e7 100644

drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608

Quoted 10 lines

drivers/usb/musb/musb_host.c +4 -0

@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {

Thanks for testing it.

Quoted 3 lines

That I think a key thing, which is done in other error. If that change is good for you than I'm also happy with it.

We need to understand why the controller keeps generating the same interrupt to come out a proper fix.

I will take a look. But I can only use my spare time on this, so be patient.

Quoted 4 lines

I also not sure if musb_writeb(epio, MUSB_RXINTERVAL, 0); is needed. In my case it's the same result with it and without it. In other scenarios might be reasonable...

It disables NAK timeout.

Quoted 8 lines

Quoted 4 lines
First of all, I don't like the idea of merging the two branches, it makes the code ugly.
Yes, I don't like that function at all, it's too long and difficult to read if you first look on it first time. It will be good to split it on 3 small functions for each big if.

This particular function is not that hard to understand, but the driver in general is messy. But I am not sure if anyone in the community can refactory this driver. The community had some effort in the past to clean up this driver, but it always broke usecases on different platforms.

Regards,
-Bin.

Yegor Yefremov

Tested-by

2016-05-03 10:04 UTC #7 lore raw

Quoted 24 lines · Bin Liu

Hi,

On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:

[snip]

Quoted 18 lines

Hello Bin,

yes, it also works with that reset and go to finish:

drivers/usb/musb/musb_host.c +4 -0

index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {

Quoted 4 lines

drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.

Have tested your patch and now both FT4232 and Huawei don't freeze on removal.

Bin, Max thanks for fixing this issue.

Tested-by Yegor Yefremov <[email protected]>

Quoted 33 lines

Quoted 2 lines
That I think a key thing, which is done in other error. If that change is good for you than I'm also happy with it.
We need to understand why the controller keeps generating the same interrupt to come out a proper fix.
I will take a look. But I can only use my spare time on this, so be patient.
Quoted 4 lines
I also not sure if musb_writeb(epio, MUSB_RXINTERVAL, 0); is needed. In my case it's the same result with it and without it. In other scenarios might be reasonable...
It disables NAK timeout.
Quoted 8 lines
Quoted 4 lines
First of all, I don't like the idea of merging the two branches, it makes the code ugly.
Yes, I don't like that function at all, it's too long and difficult to read if you first look on it first time. It will be good to split it on 3 small functions for each big if.
This particular function is not that hard to understand, but the driver in general is messy. But I am not sure if anyone in the community can refactory this driver. The community had some effort in the past to clean up this driver, but it always broke usecases on different platforms.
Regards,
-Bin.

Bin Liu

maintainer

2016-05-03 13:48 UTC #8 lore raw

Hi,

Quoted 30 lines · Yegor Yefremov

Quoted 24 lines · Bin Liu
Hi,
On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:
[snip]
Quoted 18 lines
Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.

Quoted 5 lines

Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>

Thanks for testing.

Can you please test the patch [1] instead? I'd like to use it as the fix.

Regards,
-Bin.

[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2

Yegor Yefremov

2016-05-03 14:26 UTC #9 lore raw

Quoted 37 lines · Bin Liu

Hi,
Quoted 34 lines · Yegor Yefremov
Quoted 24 lines · Bin Liu
Hi,
On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:
[snip]
Quoted 18 lines
Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.

Quoted 8 lines

Quoted 1 line
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2

The patch behaves the same as the previous one.

Kernel: 4.6-rc6

Yegor

Bin Liu

maintainer

2016-05-03 14:35 UTC #10 lore raw

Hi,

Quoted 42 lines · Yegor Yefremov

Quoted 41 lines · Bin Liu
Hi,
Quoted 30 lines · Yegor Yefremov
Quoted 24 lines · Bin Liu
Hi,
On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:
[snip]
Quoted 18 lines
Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.

Quoted 8 lines

Quoted 4 lines
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Kernel: 4.6-rc6

Thanks for testing. I will add your Tested-by.

Regards,
-Bin.

Yegor Yefremov

2016-05-04 14:50 UTC #11 lore raw

Quoted 48 lines · Bin Liu

Hi,
Quoted 45 lines · Yegor Yefremov
Quoted 44 lines · Bin Liu
Hi,
Quoted 30 lines · Yegor Yefremov
Quoted 24 lines · Bin Liu
Hi,
On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:
[snip]
Quoted 18 lines
Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.

If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.

Yegor

Bin Liu

maintainer

2016-05-04 18:56 UTC #12 lore raw

Hi,

Quoted 51 lines · Yegor Yefremov

Quoted 50 lines · Bin Liu
Hi,
Quoted 47 lines · Yegor Yefremov
Quoted 45 lines · Bin Liu
Hi,
Quoted 30 lines · Yegor Yefremov
Quoted 24 lines · Bin Liu
Hi,
On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:
[snip]
Quoted 18 lines
Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.
If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.

Thanks for testing.

My plan is to not rush it into stable, but let it sit in v4.7 for a while first.

Regards,
-Bin.

Sergei Shtylyov

2016-05-04 19:02 UTC #13 lore raw

Hello.

Quoted 40 lines · Bin Liu

Quoted 40 lines
Quoted 39 lines
Quoted 29 lines
Quoted 29 lines
Quoted 21 lines
Quoted 16 lines
Quoted 16 lines
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                 status = -EPROTO;
                 musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
         } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                 if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Quoted 8 lines
Quoted 4 lines
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.

Quoted 7 lines

Quoted 2 lines
If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.
Thanks for testing.
My plan is to not rush it into stable, but let it sit in v4.7 for a while first.

Are you serious? Fixing interrupt storm due to not cleared interrupt bit
will only be done in 4.7?

Quoted 2 lines

Regards,
-Bin.

MBR, Sergei

Bin Liu

maintainer

2016-05-04 19:17 UTC #14 lore raw

Hi,

Quoted 47 lines · Sergei Shtylyov

Hello.
Quoted 42 lines · Bin Liu
Quoted 42 lines
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 21 lines
Quoted 16 lines
Quoted 16 lines
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.
If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.

Quoted 7 lines

Quoted 4 lines
Thanks for testing.
My plan is to not rush it into stable, but let it sit in v4.7 for a while first.
Are you serious? Fixing interrupt storm due to not cleared
interrupt bit will only be done in 4.7?

Well, I am new to maintianer's role, and thought there is only one week away to v4.7 merge window, there is no big difference to let this patch get into v4.7-rc1. If getting the fix into upstream as soon as possible is important, I will send it for 4.6-rc7.

BTY, the issue is not because of not clearing interrupt bit, but the hub has no chance to report the disconnect event, which causes the controller keeps generating the interrupt for every new rx urb.

Regards,
-Bin.

Quoted 6 lines

Quoted 3 lines
Regards,
-Bin.
MBR, Sergei

Greg KH

maintainer

2016-05-04 20:19 UTC #15 lore raw

Quoted 55 lines · Bin Liu

Hi,
Quoted 52 lines · Sergei Shtylyov
Hello.
Quoted 47 lines · Bin Liu
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 33 lines
Quoted 21 lines
Quoted 16 lines
Quoted 16 lines
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.
If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.
Thanks for testing.
My plan is to not rush it into stable, but let it sit in v4.7 for a while first.

Quoted 7 lines

Quoted 2 lines
Are you serious? Fixing interrupt storm due to not cleared
interrupt bit will only be done in 4.7?
Well, I am new to maintianer's role, and thought there is only one week away to v4.7 merge window, there is no big difference to let this patch get into v4.7-rc1. If getting the fix into upstream as soon as possible is important, I will send it for 4.6-rc7.

No, your original idea of getting it into 4.7-rc1 first is the best, I will not take this for 4.6-final right now, it's not a regression.

thanks,

greg k-h

Bin Liu

maintainer

2016-05-04 20:47 UTC #16 lore raw

Hi,

Quoted 59 lines · Greg KH

Quoted 58 lines · Bin Liu
Hi,
Quoted 54 lines · Sergei Shtylyov
Hello.
Quoted 40 lines · Bin Liu
Quoted 40 lines
Quoted 39 lines
Quoted 29 lines
Quoted 29 lines
Quoted 21 lines
Quoted 16 lines
Quoted 16 lines
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Quoted 8 lines
Quoted 4 lines
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.
Quoted 7 lines
Quoted 2 lines
If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.
Thanks for testing.
My plan is to not rush it into stable, but let it sit in v4.7 for a while first.
Are you serious? Fixing interrupt storm due to not cleared
interrupt bit will only be done in 4.7?

Quoted 7 lines

Quoted 4 lines
Well, I am new to maintianer's role, and thought there is only one week away to v4.7 merge window, there is no big difference to let this patch get into v4.7-rc1. If getting the fix into upstream as soon as possible is important, I will send it for 4.6-rc7.
No, your original idea of getting it into 4.7-rc1 first is the best, I will not take this for 4.6-final right now, it's not a regression.

Ok, noted.

Quoted 4 lines

thanks,
greg k-h

Regards,
-Bin.

Sergei Shtylyov

2016-05-05 13:21 UTC #17 lore raw

In reply to Bin Liu

Hello.

Quoted 51 lines · Bin Liu

Quoted 43 lines
Quoted 42 lines
Quoted 42 lines
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 21 lines
Quoted 16 lines
Quoted 16 lines
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.
If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.
Quoted 7 lines
Quoted 4 lines
Thanks for testing.
My plan is to not rush it into stable, but let it sit in v4.7 for a while first.
Are you serious? Fixing interrupt storm due to not cleared
interrupt bit will only be done in 4.7?

Quoted 8 lines

Well, I am new to maintianer's role, and thought there is only one week away to v4.7 merge window, there is no big difference to let this patch get into v4.7-rc1. If getting the fix into upstream as soon as possible is important, I will send it for 4.6-rc7.
BTY, the issue is not because of not clearing interrupt bit, but the hub has no chance to report the disconnect event, which causes the controller keeps generating the interrupt for every new rx urb.

Sorry, looking at the Mentor manuals, I got the impression that whenever
the RXCSR.Error is set, there's interrupt. Probably they meant that the interrupt is generated only on transition from 0 to 1....

Quoted 2 lines

Regards,
-Bin.

MBR, Sergei

Bin Liu

maintainer

2016-05-05 13:31 UTC #18 lore raw

Hi,

Quoted 60 lines · Sergei Shtylyov

Hello.
Quoted 56 lines · Bin Liu
Quoted 43 lines
Quoted 42 lines
Quoted 42 lines
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 21 lines
Quoted 16 lines
Quoted 16 lines
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
               status = -EPROTO;
               musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
       } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
               if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.
If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.
Quoted 7 lines
Quoted 4 lines
Thanks for testing.
My plan is to not rush it into stable, but let it sit in v4.7 for a while first.
Are you serious? Fixing interrupt storm due to not cleared
interrupt bit will only be done in 4.7?
Well, I am new to maintianer's role, and thought there is only one week away to v4.7 merge window, there is no big difference to let this patch get into v4.7-rc1. If getting the fix into upstream as soon as possible is important, I will send it for 4.6-rc7.

Quoted 6 lines

Quoted 3 lines
BTY, the issue is not because of not clearing interrupt bit, but the hub has no chance to report the disconnect event, which causes the controller keeps generating the interrupt for every new rx urb.
Sorry, looking at the Mentor manuals, I got the impression that
whenever the RXCSR.Error is set, there's interrupt. Probably they

This is my understanding of the manual too.

Quoted 2 lines

meant that the interrupt is generated only on transition from 0 to 1....

What transition? the RXCSR bit? 'set' means from 0 to 1, 'clear' means 1 -> 0, right? I don't see you have any misunderstanding.

Quoted 6 lines

Quoted 3 lines
Regards,
-Bin.
MBR, Sergei

Sergei Shtylyov

2016-05-05 13:39 UTC #19 lore raw

Quoted 63 lines · Bin Liu

Quoted 56 lines
Quoted 56 lines
Quoted 43 lines
Quoted 42 lines
Quoted 42 lines
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 21 lines
Quoted 16 lines
Quoted 16 lines
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
               status = -EPROTO;
               musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
       } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
               if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.
If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.
Quoted 7 lines
Quoted 4 lines
Thanks for testing.
My plan is to not rush it into stable, but let it sit in v4.7 for a while first.
Are you serious? Fixing interrupt storm due to not cleared
interrupt bit will only be done in 4.7?
Well, I am new to maintianer's role, and thought there is only one week away to v4.7 merge window, there is no big difference to let this patch get into v4.7-rc1. If getting the fix into upstream as soon as possible is important, I will send it for 4.6-rc7.
Quoted 6 lines
Quoted 3 lines
BTY, the issue is not because of not clearing interrupt bit, but the hub has no chance to report the disconnect event, which causes the controller keeps generating the interrupt for every new rx urb.
Sorry, looking at the Mentor manuals, I got the impression that
whenever the RXCSR.Error is set, there's interrupt. Probably they

Quoted 6 lines

This is my understanding of the manual too.
Quoted 2 lines
meant that the interrupt is generated only on transition from 0 to 1....
What transition? the RXCSR bit?

Of course.

Quoted 1 line

'set' means from 0 to 1, 'clear' means 1 -> 0, right?

Well, in my understanding "set" means 1 and "clear" means 0.

Quoted 4 lines

I don't see you have any misunderstanding.
Quoted 2 lines
Quoted 2 lines
Regards,
-Bin.

MBR, Sergei

Bin Liu

maintainer

2016-05-05 13:45 UTC #20 lore raw

Quoted 70 lines · Sergei Shtylyov

Quoted 68 lines · Bin Liu
Quoted 56 lines
Quoted 56 lines
Quoted 43 lines
Quoted 42 lines
Quoted 42 lines
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 21 lines
Quoted 16 lines
Quoted 16 lines
yes, it also works with that reset and go to finish:
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index c3d5fc9..8cd98e7 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
status = -EPROTO;
musb_writeb(epio, MUSB_RXINTERVAL, 0);
+ rx_csr &= ~MUSB_RXCSR_H_ERROR;
+ musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+ goto finish;
} else if (rx_csr & MUSB_RXCSR_DATAERROR) {
Quoted 4 lines
Quoted 2 lines
if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Quoted 5 lines
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Kernel: 4.6-rc6
Thanks for testing. I will add your Tested-by.
If you'll resend this patch, it would be good to add it to stable kernels. I've tested 3.18.32 and it fixes the error too.
Quoted 7 lines
Quoted 4 lines
Thanks for testing.
My plan is to not rush it into stable, but let it sit in v4.7 for a while first.
Are you serious? Fixing interrupt storm due to not cleared
interrupt bit will only be done in 4.7?
Well, I am new to maintianer's role, and thought there is only one week away to v4.7 merge window, there is no big difference to let this patch get into v4.7-rc1. If getting the fix into upstream as soon as possible is important, I will send it for 4.6-rc7.
Quoted 6 lines
Quoted 3 lines
BTY, the issue is not because of not clearing interrupt bit, but the hub has no chance to report the disconnect event, which causes the controller keeps generating the interrupt for every new rx urb.
Sorry, looking at the Mentor manuals, I got the impression that
whenever the RXCSR.Error is set, there's interrupt. Probably they
This is my understanding of the manual too.
Quoted 2 lines
meant that the interrupt is generated only on transition from 0 to 1....

Quoted 7 lines

Quoted 1 line
What transition? the RXCSR bit?
Of course.
Quoted 1 line
'set' means from 0 to 1, 'clear' means 1 -> 0, right?
Well, in my understanding "set" means 1 and "clear" means 0.

This is right. I didn't mean raising or falling edge when mentioning 0->1 or 1->0. We don't know when exactly the controller generates the interrupt when CSR bit changes, and we/driver don't care.

Quoted 8 lines

Quoted 5 lines
I don't see you have any misunderstanding.
Quoted 2 lines
Quoted 2 lines
Regards,
-Bin.
MBR, Sergei

Bin Liu

maintainer

2018-01-25 15:45 UTC #21 lore raw

In reply to Yegor Yefremov

Hi Yegor and Max,

Quoted 40 lines · Yegor Yefremov

Quoted 39 lines · Bin Liu
Hi,
Quoted 35 lines · Yegor Yefremov
Quoted 24 lines · Bin Liu
Hi,
On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:
[snip]
Quoted 18 lines
Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>

Quoted 8 lines

Quoted 6 lines
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.

Sorry for bringing up this old thread, but it seems to be too aggressive to stop scheduling further urbs on errors [1]. So is it possible for you to re-test your usecase by reverting commit

dbac5d07d13e ("usb: musb: host: don't start next rx urb if current one failed")

to see if only commit

    b5801212229f ("usb: musb: host: clear rxcsr error bit if set")

itself solves your issue?

I know you have tested the patch in [2], which is similar to commit b5801212229f, but tha latter doesn't have 'goto finish' which does dma cleanup on errors, it makes more sense to me. But I'd like to have you tested with reverting dbac5d07d13e to be sure.

[1] https://marc.info/?l=linux-usb&m=151689238420622&w=2
[2] https://marc.info/?l=linux-kernel&m=146185425805967&w=2

thanks,
-Bin.

Maxim Uvarov

author

2018-01-25 16:24 UTC #22 lore raw

[1] says that issue is with back ported driver to 3.12.10. Can the latest kernel be tested on the same hw?

Maxim.

2018-01-25 18:45 GMT+03:00 Bin Liu <[email protected]>:

Quoted 75 lines

Hi Yegor and Max,
Quoted 40 lines · Yegor Yefremov
Quoted 39 lines · Bin Liu
Hi,
Quoted 35 lines · Yegor Yefremov
Quoted 24 lines · Bin Liu
Hi,
On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:
[snip]
Quoted 18 lines
Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                status = -EPROTO;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
        } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 4 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Tested-by Yegor Yefremov <[email protected]>
Quoted 8 lines
Quoted 6 lines
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Sorry for bringing up this old thread, but it seems to be too aggressive to stop scheduling further urbs on errors [1]. So is it possible for you to re-test your usecase by reverting commit
dbac5d07d13e ("usb: musb: host: don't start next rx urb if current one failed")
to see if only commit
    b5801212229f ("usb: musb: host: clear rxcsr error bit if set")
itself solves your issue?
I know you have tested the patch in [2], which is similar to commit b5801212229f, but tha latter doesn't have 'goto finish' which does dma cleanup on errors, it makes more sense to me. But I'd like to have you tested with reverting dbac5d07d13e to be sure.
[1] https://marc.info/?l=linux-usb&m=151689238420622&w=2
[2] https://marc.info/?l=linux-kernel&m=146185425805967&w=2
thanks,
-Bin.

Signature · 2 lines

Best regards,
Maxim Uvarov

Bin Liu

maintainer

2018-01-25 16:31 UTC #23 lore raw

Maxim,

Quoted 2 lines · Maxim Uvarov

[1] says that issue is with back ported driver to 3.12.10. Can the latest kernel be tested on the same hw?

Agreed that it should be tested with the latest kernel. But my concern now is if stopping scheduling urbs on errors is a right thing to do, that is why I asked if you can re-test your usecase with reverting the commit. I am unable to reproduce the original issue you had.

Thanks,
-Bin.

Maxim Uvarov

author

2018-01-26 09:24 UTC #24 lore raw

Bin,

I looked to my local git and code does not have this latest line "goto finish". It was tested without it and everything worked. Right now I can not get access to that hardware to check with and without. But only can confirm that without "goto finish" function works with bunch of drivers (usb ethernet, hids, hdd).

Best regards,
Maxim.

2018-01-25 19:31 GMT+03:00 Bin Liu <[email protected]>:

Quoted 13 lines

Maxim,
Quoted 2 lines · Maxim Uvarov
[1] says that issue is with back ported driver to 3.12.10. Can the latest kernel be tested on the same hw?
Agreed that it should be tested with the latest kernel. But my concern now is if stopping scheduling urbs on errors is a right thing to do, that is why I asked if you can re-test your usecase with reverting the commit. I am unable to reproduce the original issue you had.
Thanks,
-Bin.

Signature · 2 lines

Best regards,
Maxim Uvarov

Bin Liu

maintainer

2018-02-08 02:38 UTC #25 lore raw

Maxim,

Quoted 7 lines · Maxim Uvarov

Bin,
I looked to my local git and code does not have this latest line "goto finish". It was tested without it and everything worked. Right now I can not get access to that hardware to check with and without. But only can confirm that without "goto finish" function works with bunch of drivers (usb ethernet, hids, hdd).

Thanks for the confirmation. The revert patch has been sent out.

Regards,
-Bin.

Quoted 24 lines

Best regards,
Maxim.
2018-01-25 19:31 GMT+03:00 Bin Liu <[email protected]>:
Quoted 13 lines
Maxim,
Quoted 2 lines · Maxim Uvarov
[1] says that issue is with back ported driver to 3.12.10. Can the latest kernel be tested on the same hw?
Agreed that it should be tested with the latest kernel. But my concern now is if stopping scheduling urbs on errors is a right thing to do, that is why I asked if you can re-test your usecase with reverting the commit. I am unable to reproduce the original issue you had.
Thanks,
-Bin.
Signature · 2 lines
Best regards,
Maxim Uvarov

Tomas Paukrt

2018-01-26 10:42 UTC #26 lore raw

In reply to Maxim Uvarov

Hi Maxim,

unfortunately we cannot test the latest kernel right now, because we have custom drivers and additional changes that need to be ported, but the MUSB driver in our kernel should contain all fixes from https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/drivers/usb/musb

Best regards

Tomas

Dne 25.1.2018 v 17:24 Maxim Uvarov napsal(a):

Quoted 78 lines

[1] says that issue is with back ported driver to 3.12.10. Can the latest kernel be tested on the same hw?
Maxim.
2018-01-25 18:45 GMT+03:00 Bin Liu <[email protected]>:
Quoted 70 lines
Hi Yegor and Max,
Quoted 36 lines · Yegor Yefremov
Quoted 35 lines · Bin Liu
Hi,
Quoted 32 lines · Yegor Yefremov
Quoted 24 lines · Bin Liu
Hi,
On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:
[snip]
Quoted 18 lines
Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
                 status = -EPROTO;
                 musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
         } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 3 lines
drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1608
Quoted 2 lines
drivers/usb/musb/musb_host.c +0 -0
                 if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Quoted 8 lines
Quoted 7 lines
Quoted 1 line
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Sorry for bringing up this old thread, but it seems to be too aggressive to stop scheduling further urbs on errors [1]. So is it possible for you to re-test your usecase by reverting commit
dbac5d07d13e ("usb: musb: host: don't start next rx urb if current one failed")
to see if only commit
     b5801212229f ("usb: musb: host: clear rxcsr error bit if set")
itself solves your issue?
I know you have tested the patch in [2], which is similar to commit b5801212229f, but tha latter doesn't have 'goto finish' which does dma cleanup on errors, it makes more sense to me. But I'd like to have you tested with reverting dbac5d07d13e to be sure.
[1] https://marc.info/?l=linux-usb&m=151689238420622&w=2
[2] https://marc.info/?l=linux-kernel&m=146185425805967&w=2
thanks,
-Bin.

Tomas Paukrt

2018-08-06 13:20 UTC #27 lore raw

Hi Maxim and Bin,

I would like to reopen this issue, because we have moved to 4.12.24 and restarting two cellular modules using AT+CFUN=1,1 command at the same time leads to system freeze, because RX ISR is repeatedly invoked. Maybe *musb_start_urb* should be always called from *musb_advance_schedule* after a random communication error, but should not be called repeatedly if the device has been disconnected.

Best regards

Tomas

Dne 26.1.2018 v 11:42 Tomas Paukrt napsal(a):

Quoted 90 lines

Hi Maxim,
unfortunately we cannot test the latest kernel right now, because we have custom drivers and additional changes that need to be ported, but the MUSB driver in our kernel should contain all fixes from https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/drivers/usb/musb
Best regards
Tomas
Dne 25.1.2018 v 17:24 Maxim Uvarov napsal(a):
Quoted 79 lines
[1] says that issue is with back ported driver to 3.12.10. Can the latest kernel be tested on the same hw?
Maxim.
2018-01-25 18:45 GMT+03:00 Bin Liu <[email protected]>:
Quoted 71 lines
Hi Yegor and Max,
Quoted 37 lines · Yegor Yefremov
Quoted 36 lines · Bin Liu
Hi,
Quoted 33 lines · Yegor Yefremov
Quoted 25 lines · Bin Liu
Hi,
On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:
[snip]
Quoted 19 lines
Hello Bin,
yes, it also works with that reset and go to finish:
drivers/usb/musb/musb_host.c +4 -0
index c3d5fc9..8cd98e7 100644
@@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 
 epnum)
                  status = -EPROTO;
                  musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+               rx_csr &= ~MUSB_RXCSR_H_ERROR;
+               musb_writew(epio, MUSB_RXCSR, rx_csr);
+
+               goto finish;
          } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
Quoted 3 lines
Quoted 2 lines
if (USB_ENDPOINT_XFER_ISOC != qh->type) {
Thanks for testing it.
Have tested your patch and now both FT4232 and Huawei don't freeze on removal.
Bin, Max thanks for fixing this issue.
Quoted 8 lines
Quoted 7 lines
Quoted 1 line
Tested-by Yegor Yefremov <[email protected]>
Thanks for testing.
Can you please test the patch [1] instead? I'd like to use it as the fix.
[1] http://marc.info/?l=linux-usb&m=146222355213935&w=2
The patch behaves the same as the previous one.
Sorry for bringing up this old thread, but it seems to be too aggressive to stop scheduling further urbs on errors [1]. So is it possible for you to re-test your usecase by reverting commit
dbac5d07d13e ("usb: musb: host: don't start next rx urb if current one failed")
to see if only commit
b5801212229f ("usb: musb: host: clear rxcsr error bit if set")
itself solves your issue?
I know you have tested the patch in [2], which is similar to commit b5801212229f, but tha latter doesn't have 'goto finish' which does dma cleanup on errors, it makes more sense to me. But I'd like to have you tested with reverting dbac5d07d13e to be sure.
[1] https://marc.info/?l=linux-usb&m=151689238420622&w=2
[2] https://marc.info/?l=linux-kernel&m=146185425805967&w=2
thanks,
-Bin.