Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: Return error when the device is not present

["Wu, Hao A" <hao.a.wu@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4635 bytes --]

Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
And patch pushed via commit 1702e2ce5a5bc2eb4514f6b1c0d68927b920528a.

Best Regards,
Hao Wu

From: Marcin Wojtas [mailto:mw@semihalf.com]
Sent: Friday, August 02, 2019 3:12 PM
To: edk2-devel-groups-io; Wu, Hao A
Cc: Ni, Ray; leif.lindholm@linaro.org; ard.biesheuvel@linaro.org; jsd@semihalf.com; jaz@semihalf.com; Kinney, Michael D; Gao, Liming; lersek@redhat.com
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: Return error when the device is not present

Hi Hao,

pt., 2 sie 2019 o 03:04 Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>> napisał(a):
> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io>] On Behalf Of Ni,
> Ray
> Sent: Thursday, August 01, 2019 12:51 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; mw@semihalf.com<mailto:mw@semihalf.com>
> Cc: leif.lindholm@linaro.org<mailto:leif.lindholm@linaro.org>; ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>; jsd@semihalf.com<mailto:jsd@semihalf.com>;
> jaz@semihalf.com<mailto:jaz@semihalf.com>; Tian, Feng; Kinney, Michael D; Gao, Liming;
> lersek@redhat.com<mailto:lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: Return error
> when the device is not present
>
> Marcin,
> What does the failure look like without your fix?


From my observation of the code:

A. Changes are made in UsbEnumerateNewDev();
B. Caller of UsbEnumerateNewDev(): UsbEnumeratePort();
C. UsbEnumeratePort() directly returns the status from UsbEnumerateNewDev()
   as its own return value;
D. Callers of UsbEnumeratePort(): UsbHubEnumeration() and UsbRootHubEnumeration();
E. Both of them do not care about the return status from UsbEnumeratePort().

I think there is no direct failure without the proposed fix.
However, let us wait the confirmation from Marcin.

Indeed, the code path guarantees that nothing is done with the UsbEnumeratePort() Status. The patch is not a fix for the upstream code, however current behavior was problematic for a solution I was implementing on top of it (i.e. disabling USB hot-plug feature for a certain platform).

Best regards,
Marcin

Also, I think the proposed patch is a good refinement. If future updates to
the driver requires checking the return status during the above code path,
then proper status will be returned.

Best Regards,
Hao Wu


>
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> Marcin Wojtas
> > Sent: Wednesday, July 31, 2019 2:25 PM
> > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > Cc: leif.lindholm@linaro.org<mailto:leif.lindholm@linaro.org>; ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>;
> mw@semihalf.com<mailto:mw@semihalf.com>; jsd@semihalf.com<mailto:jsd@semihalf.com>; jaz@semihalf.com<mailto:jaz@semihalf.com>; Tian, Feng
> > <feng.tian@intel.com<mailto:feng.tian@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>;
> Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>;
> > lersek@redhat.com<mailto:lersek@redhat.com>
> > Subject: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: Return error
> when the device is not present
> >
> > Until now, during the USB device enumeration when its PortState
> > USB_PORT_STAT_CONNECTION bit was not set, the stack was not
> informed
> > that the device is not present. Fix that by returning appropriate
> > error code.
> >
> > Change-Id: I588f82b987993e9755f64ce76cde9eb690ef1d54
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com<mailto:mw@semihalf.com>>
> > ---
> >  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
> > index be9d9bd..ab1db15 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
> > @@ -719,6 +719,7 @@ UsbEnumerateNewDev (
> >
> >    if (!USB_BIT_IS_SET (PortState.PortStatus,
> USB_PORT_STAT_CONNECTION)) {
> >      DEBUG ((EFI_D_ERROR, "UsbEnumerateNewDev: No device present at
> port %d\n", Port));
> > +    Status = EFI_NOT_FOUND;
> >      goto ON_ERROR;
> >    } else if (USB_BIT_IS_SET (PortState.PortStatus,
> USB_PORT_STAT_SUPER_SPEED)){
> >      Child->Speed      = EFI_USB_SPEED_SUPER;
> > --
> > 2.7.4
> >
> >
> >
>
>
>




[-- Attachment #2: Type: text/html, Size: 29562 bytes --]