Skip to content

MdeModulePkg/Bus/Usb/UsbMassStorageDxe: USBMass Storage Enhancements DXE #11202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+27 −0
Open

MdeModulePkg/Bus/Usb/UsbMassStorageDxe: USBMass Storage Enhancements DXE #11202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0cb4f5
MdeModulePkg/Bus/Usb/UsbMassStorageDxe: USBMass Storage Enhancements DXE
Jun 17, 2025
fe2edbb
CI build fails with uncrustify errors.
Jun 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,24 @@ UsbBootExecCmd (
return EFI_SUCCESS;
}

if (*((UINT8 *)Cmd) == USB_BOOT_INQUIRY_OPCODE) {
//
// Only take care the condition for INQUIRY command.
// It would indicate the DEVICE may not ready for other command.
// Return EFI_DEVICE_ERROR and the caller will do the retry.
// Usb Mass Boot spec 3.1
//
if (!EFI_ERROR (Status) && (CmdResult == USB_MASS_CMD_PERSISTENT)) {
Transport->Reset (UsbMass->Context, TRUE);
return EFI_DEVICE_ERROR;
}

if (Status == EFI_DEVICE_ERROR) {
Transport->Reset (UsbMass->Context, TRUE);
return EFI_DEVICE_ERROR;
}
}

//
// If command execution failed, then retrieve error info via sense request.
//
Expand Down
8 changes: 8 additions & 0 deletions MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,14 @@ UsbBotResetDevice (
UsbClearEndpointStall (UsbBot->UsbIo, UsbBot->BulkInEndpoint->EndpointAddress);
UsbClearEndpointStall (UsbBot->UsbIo, UsbBot->BulkOutEndpoint->EndpointAddress);

//
// Device fw issue.
// There are no delay requirement from UsbMassBulk spec and USB spec.
// DW316 need delay for UsbClearEndpointStall and there might be other devices also need this delay,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What means DW316? What problem will be resolved by this Delay?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What means DW316? What problem will be resolved by this Delay?

Thanks for your response.
DW316 is a DELL optical drive and it looks like the clear endpoint stall will need some delay on it before the endpoint is usable.
Otherwise, the next transaction will be still failed to execute .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lgao4 ,Can you please share your feedback if any ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is the right way to add the delay for the specific device in the common UsbMassStorageDxe module.

@leiflindholm , could you share your comment for this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgao4 I'm no USB expert, but I'll try to loop in someone

Reflexively, this doesn't feel like a clean way to deal with a quirk in a single device.

I think ultimately we will need to revisit the conversation from #6526 : how do we integrate quirks in a TianoCore context?

// so adding the delay as common W/A
//
gBS->Stall (USB_BOT_CLR_STALL_EP_STALL);

return Status;
}

Expand Down
1 change: 1 addition & 0 deletions MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ extern USB_MASS_TRANSPORT mUsbBotTransport;
// Usb Bot wait device reset complete, set by experience
//
#define USB_BOT_RESET_DEVICE_STALL (100 * USB_MASS_1_MILLISECOND)
#define USB_BOT_CLR_STALL_EP_STALL (10 * USB_MASS_1_MILLISECOND)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think such small delay works. The minimal delay should be 20 ms。

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think such small delay works. The minimal delay should be 20 ms。

Thanks for your response.
I can NOT find the reference timing for clear endpoint stall.
So that I used the Reset Recovery timing for this.
If there is any specific value for it, please help to share it and then it might refactor with more proper value.
Thanks a lot.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lgao4 ,Can you please share your feedback if any ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we absolutely sure what is needed is a delay and not a barrier/memory fence?


//
// Usb Bot transport timeout, set by experience
Expand Down
Loading