[PATCH 0/5] Fix UsbMassStorage driver for device compatibility

[PATCH 0/5] Fix UsbMassStorage driver for device compatibility

[Ruiyu Ni]

#1 and #2 are to revert the two commits made by me several weeks ago.
#3 and #4 doesn't impact functionality.
#5 is the real fix.

I have verified the fix by trying different USB keys and one USB
DVD ROM.

Ruiyu Ni (5):
  MdeModulePkg/UsbMass: Revert POWER_ON(29h) ASC handling logic
  MdeModulePkg/UsbMass: Revert "map -r" media change detection fix
  MdeModulePkg/UsbMass: Add more debug message
  MdeModulePkg/UsbMass: remove unnecessary RestoreTPL() call
  MdeModulePkg/UsbMass: Retry CMD for MediaChanged sense key

 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c        | 94 +++++++++++-----------
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h        |  3 +-
 .../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c        | 10 ++-
 3 files changed, 58 insertions(+), 49 deletions(-)

-- 
2.16.1.windows.1

[PATCH 1/5] MdeModulePkg/UsbMass: Revert POWER_ON(29h) ASC handling logic

[Ruiyu Ni]

This reverts commit 6461344c318cd43a5494c0302e142a0cbe386d52.
* MdeModulePkg/UsbMass: Fix hot-plug USB CDROM can't be recognized

UsbBootExecCmd() only calls UsbBootRequestSense() to get sense key
when CMD fails.
When POWER ON (29h) ASC returns from REQUEST SENSE, implementation
should retry the CMD, instead of treating this as a SUCCESS.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 2 --
 MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h | 3 +--
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index 12e68d2149..613008bde7 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -124,8 +124,6 @@ UsbBootRequestSense (
       Status = EFI_NOT_READY;
     } else if (SenseData.Asc == USB_BOOT_ASC_NO_MEDIA) {
       Status = EFI_NOT_READY;
-    } else if (SenseData.Asc == USB_BOOT_ASC_POWER_ON) {
-      Status = EFI_SUCCESS;
     }
     break;
 
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
index 5ee50ac52a..13a926035c 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
@@ -2,7 +2,7 @@
   Definition of the command set of USB Mass Storage Specification
   for Bootability, Revision 1.0.
 
-Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -55,7 +55,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define USB_BOOT_ASC_NOT_READY                        0x04
 #define USB_BOOT_ASC_NO_MEDIA                         0x3A
 #define USB_BOOT_ASC_MEDIA_CHANGE                     0x28
-#define USB_BOOT_ASC_POWER_ON                         0x29
 
 //
 // Supported PDT codes, or Peripheral Device Type
-- 
2.16.1.windows.1

[PATCH 2/5] MdeModulePkg/UsbMass: Revert "map -r" media change detection fix

[Ruiyu Ni]

This reverts commit a662afb5b023a187ef638d3cb0e0c313ad39a7fc.
* MdeModulePkg/UsbStorage: Fix "map -r" cannot detect media change

The above commit fixed the following issue:
When system boots to Shell without CDROM inside USB CDROM drive,
and then user inserts the CDROM with Eltorito file system,
"map -r" cannot show the new ELtorito file system.
The commit caused EFI_MEDIA_CHANGED status returned from
UsbBootDetectMedia().

But that fix exposes another issue:
When issuing ReadCapacity command to certain USB key
(Kingston DataTraveler G3 8GB) after it's hot-plugged, USB device
returns STALL error and RequestSense command returns media changed
sense data. (Most of the USB keys return SUCCESS for ReadCapacity
command after hot-plug.)

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index 613008bde7..a8b6a1c5f1 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -2,7 +2,7 @@
   Implementation of the command set of USB Mass Storage Specification
   for Bootability, Revision 1.0.
 
-Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -630,10 +630,8 @@ UsbBootGetParams (
 {
   EFI_BLOCK_IO_MEDIA          *Media;
   EFI_STATUS                  Status;
-  UINT8                       CmdSet;
 
   Media  = &(UsbMass->BlockIoMedia);
-  CmdSet = ((EFI_USB_INTERFACE_DESCRIPTOR *) (UsbMass->Context))->InterfaceSubClass;
 
   Status = UsbBootInquiry (UsbMass);
   if (EFI_ERROR (Status)) {
@@ -668,18 +666,9 @@ UsbBootGetParams (
     Media->BlockSize        = 0x0800;
   }
 
-  if ((UsbMass->Pdt != USB_PDT_CDROM) && (CmdSet == USB_MASS_STORE_SCSI)) {
-    //
-    // ModeSense is required for the device with PDT of 0x00/0x07/0x0E,
-    // which is from [MassStorageBootabilitySpec-Page7].
-    // ModeSense(10) is useless here, while ModeSense(6) defined in SCSI
-    // could get the information of WriteProtected.
-    // Since not all device support this command, so skip if fail.
-    //
-    UsbScsiModeSense (UsbMass);
-  }
+  Status = UsbBootDetectMedia (UsbMass);
 
-  return UsbBootReadCapacity (UsbMass);
+  return Status;
 }
 
 
@@ -710,7 +699,7 @@ UsbBootDetectMedia (
   CmdSet = ((EFI_USB_INTERFACE_DESCRIPTOR *) (UsbMass->Context))->InterfaceSubClass;
 
   Status = UsbBootIsUnitReady (UsbMass);
-  if (EFI_ERROR (Status)) {
+  if (EFI_ERROR (Status) && (Status != EFI_MEDIA_CHANGED)) {
     goto ON_ERROR;
   }
 
-- 
2.16.1.windows.1

[PATCH 3/5] MdeModulePkg/UsbMass: Add more debug message

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index a8b6a1c5f1..d212960bbc 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -2,7 +2,7 @@
   Implementation of the command set of USB Mass Storage Specification
   for Bootability, Revision 1.0.
 
-Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -137,8 +137,9 @@ UsbBootRequestSense (
     break;
   }
 
-  DEBUG ((EFI_D_INFO, "UsbBootRequestSense: (%r) with sense key %x/%x/%x\n",
+  DEBUG ((EFI_D_INFO, "UsbBootRequestSense: (%r) with error code (%x) sense key %x/%x/%x\n",
           Status,
+          SenseData.ErrorCode,
           USB_BOOT_SENSE_KEY (SenseData.SenseKey),
           SenseData.Asc,
           SenseData.Ascq
@@ -196,7 +197,7 @@ UsbBootExecCmd (
                            );
 
   if (Status == EFI_TIMEOUT) {
-    DEBUG ((EFI_D_ERROR, "UsbBootExecCmd: Timeout to Exec 0x%x Cmd\n", *(UINT8 *)Cmd));
+    DEBUG ((EFI_D_ERROR, "UsbBootExecCmd: %r to Exec 0x%x Cmd\n", Status, *(UINT8 *)Cmd));
     return EFI_TIMEOUT;
   }
 
@@ -211,6 +212,7 @@ UsbBootExecCmd (
   //
   // If command execution failed, then retrieve error info via sense request.
   //
+  DEBUG ((EFI_D_ERROR, "UsbBootExecCmd: %r to Exec 0x%x Cmd (Result = %x)\n", Status, *(UINT8 *)Cmd, CmdResult));
   return UsbBootRequestSense (UsbMass);
 }
 
-- 
2.16.1.windows.1

[PATCH 4/5] MdeModulePkg/UsbMass: remove unnecessary RestoreTPL() call

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index d212960bbc..dd4b3a5f0a 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -691,7 +691,6 @@ UsbBootDetectMedia (
   EFI_BLOCK_IO_MEDIA        OldMedia;
   EFI_BLOCK_IO_MEDIA        *Media;
   UINT8                     CmdSet;
-  EFI_TPL                   OldTpl;
   EFI_STATUS                Status;
 
   Media    = &UsbMass->BlockIoMedia;
@@ -740,11 +739,10 @@ ON_ERROR:
       (Media->LastBlock != OldMedia.LastBlock)) {
 
     //
-    // This function is called by Block I/O Protocol APIs, which run at TPL_NOTIFY.
-    // Here we temporarily restore TPL to TPL_CALLBACK to invoke ReinstallProtocolInterface().
-    //
-    OldTpl = EfiGetCurrentTpl ();
-    gBS->RestoreTPL (TPL_CALLBACK);
+    // This function is called from:
+    //   Block I/O Protocol APIs, which run at TPL_CALLBACK.
+    //   DriverBindingStart(), which raises to TPL_CALLBACK.
+    ASSERT (EfiGetCurrentTpl () == TPL_CALLBACK);
 
     gBS->ReinstallProtocolInterface (
            UsbMass->Controller,
@@ -753,9 +751,6 @@ ON_ERROR:
            &UsbMass->BlockIo
            );
 
-    ASSERT (EfiGetCurrentTpl () == TPL_CALLBACK);
-    gBS->RaiseTPL (OldTpl);
-
     //
     // Update MediaId after reinstalling Block I/O Protocol.
     //
-- 
2.16.1.windows.1

[PATCH 5/5] MdeModulePkg/UsbMass: Retry CMD for MediaChanged sense key

[Ruiyu Ni]

When a USB device reports failure for a CMD and REQUEST SENSE returns
Media Changed key, UsbBootExecCmdWithRetry() stops to retry CMD and
returns EFI_MEDIA_CHANGED to caller.

For this case, the CMD should be retried until success, getting
NoMedia sense key or timeout.
The patch updates UsbBootExecCmdWithRetry() to follow the above
rule so EFI_MEDIA_CHANGED is no longer returned.

UsbBootDetectMedia() is updated accordingly.

Because UsbBootGetParams() is called for new plugged USB storage,
and some USB storage devices may report Media Changed key,
UsbBootGetParams() is updated to treat it as a Success.
This change could fix the issue that some USB storage devices
cannot be detected.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c        | 60 ++++++++++++++--------
 .../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c        | 10 +++-
 2 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index dd4b3a5f0a..b84bfd2d72 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -234,7 +234,7 @@ UsbBootExecCmd (
   @param  Timeout                The timeout used to transfer
 
   @retval EFI_SUCCESS            The command is executed successfully.
-  @retval EFI_MEDIA_CHANGED      The device media has been changed.
+  @retval EFI_NO_MEDIA           The device media is removed.
   @retval Others                 Command execution failed after retrial.
 
 **/
@@ -284,7 +284,7 @@ UsbBootExecCmdWithRetry (
                DataLen,
                Timeout
                );
-    if (Status == EFI_SUCCESS || Status == EFI_MEDIA_CHANGED || Status == EFI_NO_MEDIA) {
+    if (Status == EFI_SUCCESS || Status == EFI_NO_MEDIA) {
       break;
     }
     //
@@ -700,30 +700,42 @@ UsbBootDetectMedia (
   CmdSet = ((EFI_USB_INTERFACE_DESCRIPTOR *) (UsbMass->Context))->InterfaceSubClass;
 
   Status = UsbBootIsUnitReady (UsbMass);
-  if (EFI_ERROR (Status) && (Status != EFI_MEDIA_CHANGED)) {
-    goto ON_ERROR;
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "UsbBootDetectMedia: UsbBootIsUnitReady (%r)\n", Status));
   }
 
-  if ((UsbMass->Pdt != USB_PDT_CDROM) && (CmdSet == USB_MASS_STORE_SCSI)) {
-    //
-    // MODE SENSE is required for the device with PDT of 0x00/0x07/0x0E,
-    // according to Section 4 of USB Mass Storage Specification for Bootability.
-    // MODE SENSE(10) is useless here, while MODE SENSE(6) defined in SCSI
-    // could get the information of Write Protected.
-    // Since not all device support this command, skip if fail.
-    //
-    UsbScsiModeSense (UsbMass);
-  }
+  //
+  // Status could be:
+  //   EFI_SUCCESS: all good.
+  //   EFI_NO_MEDIA: media is not present.
+  //   others: HW error.
+  // For either EFI_NO_MEDIA, or HW error, skip to get WriteProtected and capacity information.
+  //
+  if (!EFI_ERROR (Status)) {
+    if ((UsbMass->Pdt != USB_PDT_CDROM) && (CmdSet == USB_MASS_STORE_SCSI)) {
+      //
+      // MODE SENSE is required for the device with PDT of 0x00/0x07/0x0E,
+      // according to Section 4 of USB Mass Storage Specification for Bootability.
+      // MODE SENSE(10) is useless here, while MODE SENSE(6) defined in SCSI
+      // could get the information of Write Protected.
+      // Since not all device support this command, skip if fail.
+      //
+      UsbScsiModeSense (UsbMass);
+    }
 
-  Status = UsbBootReadCapacity (UsbMass);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((EFI_D_ERROR, "UsbBootDetectMedia: UsbBootReadCapacity (%r)\n", Status));
-    goto ON_ERROR;
+    Status = UsbBootReadCapacity (UsbMass);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "UsbBootDetectMedia: UsbBootReadCapacity (%r)\n", Status));
+    }
   }
 
-  return EFI_SUCCESS;
+  if (EFI_ERROR (Status) && Status != EFI_NO_MEDIA) {
+    //
+    // For NoMedia, BlockIo is still needed.
+    //
+    return Status;
+  }
 
-ON_ERROR:
   //
   // Detect whether it is necessary to reinstall the Block I/O Protocol.
   //
@@ -744,6 +756,10 @@ ON_ERROR:
     //   DriverBindingStart(), which raises to TPL_CALLBACK.
     ASSERT (EfiGetCurrentTpl () == TPL_CALLBACK);
 
+    //
+    // When it is called from DriverBindingStart(), below reinstall fails.
+    // So ignore the return status check.
+    //
     gBS->ReinstallProtocolInterface (
            UsbMass->Controller,
            &gEfiBlockIoProtocolGuid,
@@ -752,7 +768,7 @@ ON_ERROR:
            );
 
     //
-    // Update MediaId after reinstalling Block I/O Protocol.
+    // Reset MediaId after reinstalling Block I/O Protocol.
     //
     if (Media->MediaPresent != OldMedia.MediaPresent) {
       if (Media->MediaPresent) {
@@ -767,6 +783,8 @@ ON_ERROR:
         (Media->LastBlock != OldMedia.LastBlock)) {
       Media->MediaId++;
     }
+
+    Status = Media->MediaPresent ? EFI_MEDIA_CHANGED : EFI_NO_MEDIA;
   }
 
   return Status;
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c
index 9d1bb25fb3..bb292ed3eb 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c
@@ -1,7 +1,7 @@
 /** @file
   USB Mass Storage Driver that manages USB Mass Storage Device and produces Block I/O Protocol.
 
-Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -361,6 +361,14 @@ UsbMassInitMedia (
   Media->MediaId          = 1;
 
   Status = UsbBootGetParams (UsbMass);
+  DEBUG ((DEBUG_INFO, "UsbMassInitMedia: UsbBootGetParams (%r)\n", Status));
+  if (Status == EFI_MEDIA_CHANGED) {
+    //
+    // Some USB storage devices may report MEDIA_CHANGED sense key when hot-plugged.
+    // Treat it as SUCCESS
+    //
+    Status = EFI_SUCCESS;
+  }
   return Status;
 }
 
-- 
2.16.1.windows.1

Re: [PATCH 0/5] Fix UsbMassStorage driver for device compatibility

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Friday, March 2, 2018 7:55 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH 0/5] Fix UsbMassStorage driver for device compatibility

#1 and #2 are to revert the two commits made by me several weeks ago.
#3 and #4 doesn't impact functionality.
#5 is the real fix.

I have verified the fix by trying different USB keys and one USB DVD ROM.

Ruiyu Ni (5):
  MdeModulePkg/UsbMass: Revert POWER_ON(29h) ASC handling logic
  MdeModulePkg/UsbMass: Revert "map -r" media change detection fix
  MdeModulePkg/UsbMass: Add more debug message
  MdeModulePkg/UsbMass: remove unnecessary RestoreTPL() call
  MdeModulePkg/UsbMass: Retry CMD for MediaChanged sense key

 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c        | 94 +++++++++++-----------
 .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h        |  3 +-
 .../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c        | 10 ++-
 3 files changed, 58 insertions(+), 49 deletions(-)

--
2.16.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel