[edk2-platforms: PATCH 0/3] Platform/RPi3: Improve timeout handling in DwUsbHostDxe

[edk2-platforms: PATCH 0/3] Platform/RPi3: Improve timeout handling in DwUsbHostDxe

[Pete Batard]

Networking applications (e.g. iPXE) might experience failures when submitting
a bulk IN for the NIC's RX endpoint, because the bulk IN (correctly) times out
when no received packet is waiting, but DwUsbHostDxe.c treats this as a fatal
error.

With these patches, iPXE is able to successfully download a 128MB test file
via HTTP.

Michael Brown (3):
  Platform/RPi3: Use Wait4Bit return value consistently
  Platform/RPi3: Gracefully disable USB channel after a timeout
  Platform/RPi3: Reduce debug noise when using a USB network device

 Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c | 44 +++++++++++---------
 1 file changed, 25 insertions(+), 19 deletions(-)

-- 
2.21.0.windows.1

[edk2-platforms: PATCH 1/3] Platform/RPi3: Use Wait4Bit return value consistently

[Pete Batard]

From: Michael Brown <mbrown@fensystems.co.uk>

There are multiple points in the existing code that take the UINT32
value returned by Wait4Bit and return it directly as an EFI_STATUS
value.  If Wait4Bit fails, this will result in a spurious EFI_STATUS
value of 0x1 (EFI_WARN_UNKNOWN_GLYPH) being returned to the caller.  A
caller treating any non-zero value as an error will see this as a
failure, but a caller using the EFI_ERROR macro will incorrectly see
it as a success.

Fix by defining the return type of Wait4Bit as EFI_STATUS and ensuring
that a valid EFI_STATUS value is always returned.

Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c b/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c
index 59120b9d8564..c1e1229f858e 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c
@@ -53,7 +53,7 @@ DwCoreInit (
   IN EFI_EVENT        Timeout
   );
 
-UINT32
+EFI_STATUS
 Wait4Bit (
   IN  EFI_EVENT Timeout,
   IN  UINT32    Reg,
@@ -70,14 +70,14 @@ Wait4Bit (
     }
 
     if ((Value & Mask) == Mask) {
-      return 0;
+      return EFI_SUCCESS;
     }
   } while (EFI_ERROR (gBS->CheckEvent (Timeout)));
 
   DEBUG ((DEBUG_ERROR, "Wait4Bit: %a timeout (reg:0x%x, value:0x%x, mask:0x%x)\n",
     Set ? "set" : "clear", Reg, Set ? Value : ~Value, Mask));
 
-  return 1;
+  return EFI_TIMEOUT;
 }
 
 CHANNEL_HALT_REASON
@@ -91,13 +91,14 @@ Wait4Chhltd (
   IN  SPLIT_CONTROL   *Split
   )
 {
-  INT32   Ret;
+  EFI_STATUS Status;
   UINT32  Hcint, Hctsiz;
   UINT32  HcintCompHltAck = DWC2_HCINT_XFERCOMP;
 
   MicroSecondDelay (100);
-  Ret = Wait4Bit (Timeout, DwHc->DwUsbBase + HCINT (Channel), DWC2_HCINT_CHHLTD, 1);
-  if (Ret) {
+  Status = Wait4Bit (Timeout, DwHc->DwUsbBase + HCINT (Channel),
+                     DWC2_HCINT_CHHLTD, 1);
+  if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "Channel %u did not halt\n", Channel));
     return XFER_NOT_HALTED;
   }
@@ -204,7 +205,7 @@ DwCoreReset (
   IN  EFI_EVENT Timeout
   )
 {
-  UINT32  Status;
+  EFI_STATUS Status;
 
   Status = Wait4Bit (Timeout, DwHc->DwUsbBase + GRSTCTL, DWC2_GRSTCTL_AHBIDLE, 1);
   if (Status) {
@@ -1261,7 +1262,7 @@ DwFlushTxFifo (
   IN INT32 Num
   )
 {
-  UINT32 Status;
+  EFI_STATUS Status;
 
   MmioWrite32 (DwHc->DwUsbBase + GRSTCTL, DWC2_GRSTCTL_TXFFLSH |
     (Num << DWC2_GRSTCTL_TXFNUM_OFFSET));
@@ -1279,7 +1280,7 @@ DwFlushRxFifo (
   IN  EFI_EVENT Timeout
   )
 {
-  UINT32 Status;
+  EFI_STATUS Status;
 
   MmioWrite32 (DwHc->DwUsbBase + GRSTCTL, DWC2_GRSTCTL_RXFFLSH);
 
-- 
2.21.0.windows.1

[edk2-platforms: PATCH 2/3] Platform/RPi3: Gracefully disable USB channel after a timeout

[Pete Batard]

From: Michael Brown <mbrown@fensystems.co.uk>

When a timeout occurs, attempt to gracefully disable the channel.
Report a final status of EFI_TIMEOUT if the channel was disabled
successfully, or EFI_DEVICE_ERROR if we were unable to disable the
channel.

Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c b/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c
index c1e1229f858e..22d9bd822f96 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c
@@ -318,13 +318,21 @@ DwHcTransfer (
     Ret = Wait4Chhltd (DwHc, Timeout, Channel, &Sub, Pid, IgnoreAck, &Split);
 
     if (Ret == XFER_NOT_HALTED) {
-      /*
-       * FIXME: do proper channel reset.
-       */
-      MmioWrite32 (DwHc->DwUsbBase + HCCHAR (Channel), DWC2_HCCHAR_CHDIS);
-
       *TransferResult = EFI_USB_ERR_TIMEOUT;
-      Status = EFI_DEVICE_ERROR;
+      MmioOr32 (DwHc->DwUsbBase + HCCHAR (Channel), DWC2_HCCHAR_CHDIS);
+      Status = gBS->SetTimer (Timeout, TimerRelative,
+                              EFI_TIMER_PERIOD_MILLISECONDS (1));
+      ASSERT_EFI_ERROR (Status);
+      if (EFI_ERROR (Status)) {
+        break;
+      }
+      Status = Wait4Bit (Timeout, DwHc->DwUsbBase + HCINT (Channel),
+                         DWC2_HCINT_CHHLTD, 1);
+      if (Status == EFI_SUCCESS) {
+        Status = EFI_TIMEOUT;
+      } else {
+        Status = EFI_DEVICE_ERROR;
+      }
       break;
     } else if (Ret == XFER_STALL) {
       *TransferResult = EFI_USB_ERR_STALL;
-- 
2.21.0.windows.1

[edk2-platforms: PATCH 3/3] Platform/RPi3: Reduce debug noise when using a USB network device

[Pete Batard]

From: Michael Brown <mbrown@fensystems.co.uk>

The design of the EFI_USB2_HC_PROTOCOL does not allow for long-lived
bulk IN transactions as used by network devices, but instead requires
the network driver to rely on repeatedly issuing bulk IN transactions
with a very short timeout and in the expectation that most bulk IN
transactions will time out since no packet will have been received.

Timeouts are therefore normal and expected events when using a USB
network device under UEFI.  This currently results in a constant
stream of spurious "Wait4Bit: set timeout" and "Channel %u did not
halt" debug messages whenever the network device is open.

All callers of Wait4Bit already report a meaningful error in the event
of a timeout, so the Wait4Bit message may safely be removed without
impacting the ability to debug the code.

The "Channel %u did not halt" message may be moved to its sole call
site and restricted to the situation in which the subsequent attempt
to gracefully disable the channel did actually fail.

Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c b/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c
index 22d9bd822f96..37ebf503fd60 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c
@@ -74,9 +74,6 @@ Wait4Bit (
     }
   } while (EFI_ERROR (gBS->CheckEvent (Timeout)));
 
-  DEBUG ((DEBUG_ERROR, "Wait4Bit: %a timeout (reg:0x%x, value:0x%x, mask:0x%x)\n",
-    Set ? "set" : "clear", Reg, Set ? Value : ~Value, Mask));
-
   return EFI_TIMEOUT;
 }
 
@@ -99,7 +96,6 @@ Wait4Chhltd (
   Status = Wait4Bit (Timeout, DwHc->DwUsbBase + HCINT (Channel),
                      DWC2_HCINT_CHHLTD, 1);
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "Channel %u did not halt\n", Channel));
     return XFER_NOT_HALTED;
   }
 
@@ -331,6 +327,7 @@ DwHcTransfer (
       if (Status == EFI_SUCCESS) {
         Status = EFI_TIMEOUT;
       } else {
+        DEBUG ((DEBUG_ERROR, "Channel %u did not halt\n", Channel));
         Status = EFI_DEVICE_ERROR;
       }
       break;
-- 
2.21.0.windows.1

Re: [edk2-platforms: PATCH 0/3] Platform/RPi3: Improve timeout handling in DwUsbHostDxe

[Leif Lindholm]

Hi Pete,

On Wed, Jul 17, 2019 at 12:46:42PM +0100, Pete Batard wrote:
> Networking applications (e.g. iPXE) might experience failures when submitting
> a bulk IN for the NIC's RX endpoint, because the bulk IN (correctly) times out
> when no received packet is waiting, but DwUsbHostDxe.c treats this as a fatal
> error.
> 
> With these patches, iPXE is able to successfully download a 128MB test file
> via HTTP.

The patches look good, and I don't mind you upstreaming Michael's
code, *but* I don't want patches submitted with anyone other than the
contributor's Signed-off-by:. (It's the equivalent of saying "Yeah,
Michael says he's OK with https://developercertificate.org/, and of no
actual use.)

(If patches are modified after contribution, but before being pushed,
then then additional contributions can be reflected with additional
Signed-off-bys. Make sense?)

The From: tag ensures he still retains authorship.
Are you OK with me dropping Michael's Signed-off-by before pushing?

Best Regards,

Leif

> Michael Brown (3):
>   Platform/RPi3: Use Wait4Bit return value consistently
>   Platform/RPi3: Gracefully disable USB channel after a timeout
>   Platform/RPi3: Reduce debug noise when using a USB network device
> 
>  Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c | 44 +++++++++++---------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> -- 
> 2.21.0.windows.1
> 

Re: [edk2-devel] [edk2-platforms: PATCH 0/3] Platform/RPi3: Improve timeout handling in DwUsbHostDxe

[Michael Brown]

On 19/07/2019 12:53, Leif Lindholm wrote:
> The patches look good, and I don't mind you upstreaming Michael's
> code, *but* I don't want patches submitted with anyone other than the
> contributor's Signed-off-by:. (It's the equivalent of saying "Yeah,
> Michael says he's OK with https://developercertificate.org/, and of no
> actual use.)

Sorry; this is almost certainly my fault.  My Signed-off-by was present 
in the original patches that I sent to Pete, and Pete kindly offered to 
take care of upstreaming the patches for me.  For the avoidance of any 
doubt: I definitely agree to the statements in 
https://developercertificate.org/.

I am happy for you to either drop or retain my Signed-off-by before 
pushing, as you see fit.

Many thanks,

Michael

Re: [edk2-devel] [edk2-platforms: PATCH 0/3] Platform/RPi3: Improve timeout handling in DwUsbHostDxe

[Leif Lindholm]

On Fri, Jul 19, 2019 at 04:22:10PM +0100, Michael Brown wrote:
> On 19/07/2019 12:53, Leif Lindholm wrote:
> > The patches look good, and I don't mind you upstreaming Michael's
> > code, *but* I don't want patches submitted with anyone other than the
> > contributor's Signed-off-by:. (It's the equivalent of saying "Yeah,
> > Michael says he's OK with https://developercertificate.org/, and of no
> > actual use.)
> 
> Sorry; this is almost certainly my fault.

Well, there is nothing *wrong* with that - only there is no value in
second-hand signed-off-bys.

> My Signed-off-by was present in
> the original patches that I sent to Pete, and Pete kindly offered to take
> care of upstreaming the patches for me.  For the avoidance of any doubt: I
> definitely agree to the statements in https://developercertificate.org/.

Glad to hear it :)
This means I *could* have included them, but...

> I am happy for you to either drop or retain my Signed-off-by before pushing,
> as you see fit.

...to reduce confusion, I dropped them before pushing - but you're
still set as the author.

If in future you contribute directly yourself, please keep the
Signed-off-by in.

Best Regards,

Leif

Re: [edk2-platforms: PATCH 0/3] Platform/RPi3: Improve timeout handling in DwUsbHostDxe

[Pete Batard]

Hi Leif,

On 2019.07.19 12:53, Leif Lindholm wrote:
> Hi Pete,
> 
> On Wed, Jul 17, 2019 at 12:46:42PM +0100, Pete Batard wrote:
>> Networking applications (e.g. iPXE) might experience failures when submitting
>> a bulk IN for the NIC's RX endpoint, because the bulk IN (correctly) times out
>> when no received packet is waiting, but DwUsbHostDxe.c treats this as a fatal
>> error.
>>
>> With these patches, iPXE is able to successfully download a 128MB test file
>> via HTTP.
> 
> The patches look good, and I don't mind you upstreaming Michael's
> code, *but* I don't want patches submitted with anyone other than the
> contributor's Signed-off-by:. (It's the equivalent of saying "Yeah,
> Michael says he's OK with https://developercertificate.org/, and of no
> actual use.)

I see. It does make sense, now that you explain it.

> (If patches are modified after contribution, but before being pushed,
> then then additional contributions can be reflected with additional
> Signed-off-bys. Make sense?)
> 
> The From: tag ensures he still retains authorship.
> Are you OK with me dropping Michael's Signed-off-by before pushing?

That's fine with me. I reviewed these patches and can therefore vouch 
for them.

Regards,

/Pete

PS: Thanks for pushing the previous series.

> 
> Best Regards,
> 
> Leif
> 
>> Michael Brown (3):
>>    Platform/RPi3: Use Wait4Bit return value consistently
>>    Platform/RPi3: Gracefully disable USB channel after a timeout
>>    Platform/RPi3: Reduce debug noise when using a USB network device
>>
>>   Platform/RaspberryPi/RPi3/Drivers/DwUsbHostDxe/DwUsbHostDxe.c | 44 +++++++++++---------
>>   1 file changed, 25 insertions(+), 19 deletions(-)
>>
>> -- 
>> 2.21.0.windows.1
>>

Re: [edk2-platforms: PATCH 0/3] Platform/RPi3: Improve timeout handling in DwUsbHostDxe

[Leif Lindholm]

On Fri, Jul 19, 2019 at 04:40:48PM +0100, Pete Batard wrote:
> > (If patches are modified after contribution, but before being pushed,
> > then then additional contributions can be reflected with additional
> > Signed-off-bys. Make sense?)
> > 
> > The From: tag ensures he still retains authorship.
> > Are you OK with me dropping Michael's Signed-off-by before pushing?
> 
> That's fine with me. I reviewed these patches and can therefore vouch for
> them.

Cool, thanks.

For the series:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Pushed as 3542dfdecab1..77dc30a13b13.