MdeModulePkg/UsbMassStorageDxe: Remove Port Reset #10882
Conversation
|
The DCO (Signed-off-by:) in patches you submit should only be yours. You cannot make legal statements on behalf of others. The commit authorship is already carried in the commit metadata, so Pohan still gets the credit. |
Ah, ok. I'm used to a certain other OS project where S-o-b is required of the author in addition to that of any the submitters/maintainers that pass it along. |
If you pick up someone else's previously submitted patch as part of your set, and they submitted it with their own DCO, then yes, you should keep theirs. But when posting something publicly for the first time, whatever happened internally to your organisation before then has no relevance. |
|
We also meet with the similar issue for USB CDROM that the old blockio pointer is still used. But, I want to confirm whether this change causes the negative impact. For example, some device may work after port reset, but now it doesn't work any more. |
Hi @lgao4 are you saying you are also trying other scenarios on your end or do you need us to confirm particular corner cases? In general we feel that forcing port reset might be too big of a "hammer" to use in case of misbehaving devices, or at least does not seem proper to for a class driver to have the ability to invoke it directly, as it is akin to a layering violation. |
|
@lgao4 are you hoping to do some testing? If not, we could merge this and revert if someone screams. |
I just want to know what impact with this change. If you are sure its impact is small, I will be OK to merge it. |
|
Hi Liming,
The impact is that USB Port Reset would not be called at all anymore any time there is any kind of error encountered during Mass Storage command transactions. Then again, if certain devices actually did rely on that behavior for whatever reason for recovery to get back into a working state, they certainly are not working today since the current behavior results in crash due to the dangling pointer access as described in this PR.
I would say *if* we do find such devices that stopped working after this change because they really do need a full port reset, that we revisit it later as that will require more thorough fixing of the UsbMassStorage driver to not assume that certain structures are intact after the reset.
|
During USB mass storage enumeration, if a USB transfer fails due to any other reason, UsbMassStorageDxe will attempt to reset the device. With the commit ed07a2b ("MdeModulePkg/UsbBusDxe: USB issue fix when the port reset"), UsbIoPortReset now tears down the USB device context and reinstalls it (via DisconnectController & ConnectController). This process is not handled by the UsbMassDriver, causing the upper layer to access an old pointer that has been freed during the teardown, leading to a crash. Example: UsbMassReadBlocks (Failed) -> UsbMassReset -> UsbBotResetDevice -> UsbIoPortReset (teardown + reinstall and return) Now the UsbBot context pointer is invalidated and pointing to freed memory. -> UsbBot->UsbIo->UsbControlTransfer() therefore accesses a invalid pointer and crashes. The fix is to ignore the ExtendedVerification, which is supposed to perform a more exhaustive verification operation during the reset. In MassStorageDxe, ExtendedVerification perform the parent port reset (UsbIoPortReset). Ultimately, the MassStorage device should not reset the parent port due to a transfer error. By not performing any extended verification, the teardown is prevented, thereby avoiding the crash. Signed-off-by: Jack Pham <jackp@qti.qualcomm.com>
When a USB mass storage device is not ready (e.g., still powering up or the hard disk has not reached the desired RPM), the ExecCommand function fails.This failure is not a true error. Logging it as DEBUG_ERROR will generate logs for properly functioning devices as well, potentially flooding logs for older devices. As mentioned in the command, proper error information retrieval should occur in the sense request. The solution is to downgrade the log level from DEBUG_ERROR to DEBUG_INFO. Signed-off-by: Jack Pham <jackp@qti.qualcomm.com>
3 participants
Description
During USB mass storage enumeration, if a USB transfer fails due to any other reason, UsbMassStorageDxe will attempt to reset the device. With the commit ed07a2b ("MdeModulePkg/UsbBusDxe: USB issue fix when the port reset"), UsbIoPortReset now tears down the USB device context and reinstalls it (via DisconnectController & ConnectController).
This process is not handled by the UsbMassDriver, causing the upper layer to access an old pointer that has been freed during the teardown, leading to a crash.
Example:
UsbMassReadBlocks (Failed)
-> UsbMassReset
-> UsbBotResetDevice
-> UsbIoPortReset (teardown + reinstall and return)
Now the UsbBot context pointer is invalidated and pointing to freed memory.
-> UsbBot->UsbIo->UsbControlTransfer() therefore accesses a invalid pointer and crashes.
The fix is to ignore the ExtendedVerification, which is supposed to perform a more exhaustive verification operation during the reset. In MassStorageDxe, ExtendedVerification perform the parent port reset (UsbIoPortReset). Ultimately, the MassStorage device should not reset the parent port due to a transfer error. By not performing any extended verification, the teardown is prevented, thereby avoiding the crash.
A second minor fix is included to reduce the log level from DEBUG to INFO for commands that result in responses containing a non-zero status code. When encountering a device that constantly has transfer errors this would lead to excessive log spam.
How This Was Tested
Tested against several USB mass storage devices (SSD, spinning HDDs) which for some reason seemed to encounter transfer errors that were easily triggering a crash before this change. Note this PR is not aimed at fixing the underlying transfer errors but rather to at least avoid crashing due to invalid pointer access.
Integration Instructions
N/A