On 07/31/19 08:25, Marcin Wojtas wrote:
> 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 <m...@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;
>
I think the patch is correct, based on a quite superficial analysis (i.e.
without actual knowledge of USB specifics on my part).
The reason is that Status is EFI_SUCCESS when the "goto" statement is reached,
due to the preceding context
Status = HubApi->GetPortStatus (HubIf, Port, &PortState);
if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_ERROR, "UsbEnumerateNewDev: failed to get speed of port
%d\n", Port));
goto ON_ERROR;
}
And, the ON_ERROR label is documented as:
ON_ERROR:
//
// If reach here, it means the enumeration process on a given port is
interrupted due to error.
// [...]
//
return Status;
We shouldn't report success when there is no device present on the port.
I think EFI_NOT_FOUND is a suitable error code; while it is not listed
explicitly in the leading comment on the function, it does fit under
@retval Others Failed to enumerate the device.
Marcin, can you please remove the "Change-Id" tag from the commit message? (Or
the MdeModulePkg maintainers could do that, just before they push the patch.)
With "Change-Id" removed:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44677): https://edk2.groups.io/g/devel/message/44677
Mute This Topic: https://groups.io/mt/32662408/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
