Re: [PATCH v1 1/2] MdeModulePkg: Fix bug in ScsiBusDxe/ScsiBus.c

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

Thanks for the patch, inline comments below:


> -----Original Message-----
> From: Yuan Yu <yuanyu@google.com>
> Sent: Wednesday, January 18, 2023 5:14 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; C, sivaparvathi <sivaparvathic@ami.com>
> Subject: [PATCH v1 1/2] MdeModulePkg: Fix bug in ScsiBusDxe/ScsiBus.c
> 
> A while loop in SCSIBusDriverBindingStart() is supposed to scan all the
> possible Puns in the SCSI channel by calling ScsiScanCreateDevice() for
> each of them. Therefore, we should not abort the loop even when one of
> the Puns is disconnected.
> 
> The following is one of the scenarios.
> 
> SCSIBusDriverBindingStart()
> > ScsiScanCreateDevice()
>   > DiscoverScsiDevice()
>     > ScsiInquiryCommand()
>       > ...
>         > ParseResponse()
> 
> When virtio-scsi returns VIRTIO_SCSI_S_BAD_TARGET, ParseResponse() in
> VirtioScsi.c will return EFI_TIMEOUT:
> 
>     case VIRTIO_SCSI_S_BAD_TARGET:
>       //
>       // This is non-intuitive but explicitly required by the
>       // EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() specification for
>       // disconnected (but otherwise valid) target / LUN addresses.
>       //
>       Packet->HostAdapterStatus =
>         EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT_COMMAND;
>       return EFI_TIMEOUT;
> 
> This will eventually cause DiscoverScsiDevice() to return FALSE, which
> will cause ScsiScanCreateDevice() to return EFI_OUT_OF_RESOURCES:
> 
>   if (!DiscoverScsiDevice (ScsiIoDevice)) {
>     Status = EFI_OUT_OF_RESOURCES;
>     goto ErrorExit;
>   }


I think when DiscoverScsiDevice() returns FALSE, it (the current code implementation) is improper
to simply return EFI_OUT_OF_RESOURCES.

My take is that, under the scope of the SCSI driver, EFI_OUT_OF_RESOURCES should be used as return
status when memory allocation fails (due to insufficient free memory).

But when DiscoverScsiDevice() returns FALSE, there are cases where the cause is SCSI command
execution failure like you mentioned above.

So my recommendation is to refine the interface for DiscoverScsiDevice() to return accurate status
of the execution of the function, like
* EFI_SUCCESS for device successfully discovered;
* EFI_OUT_OF_RESOURCES for memory allocation failure;
* EFI_NOT_FOUND for cases when:
    a) SCSI command execution failure or
    b) Return data of the command indicates no device

Maybe the refined interface can look like:
EFI_STATUS
DiscoverScsiDevice (
  IN OUT  SCSI_IO_DEV  *ScsiIoDevice
  )

The caller of DiscoverScsiDevice() will return proper status rather than simply returning
EFI_OUT_OF_RESOURCES when device discovery fails.


The reason for the below logic in SCSIBusDriverBindingStart():

    Status = ScsiScanCreateDevice (This, Controller, &ScsiTargetId, Lun, ScsiBusDev);
    if (Status == EFI_OUT_OF_RESOURCES) {
      goto ErrorExit;
    }

is that it expects ScsiScanCreateDevice() to return EFI_OUT_OF_RESOURCES as an indication of lack
of memory resource in the system. Under such situation, subsequent memory allocations are very likely
to fail as well, so there is little value in continuing the loop for further device discovery.


Best Regards,
Hao Wu


> 
> In sum, "disconnected (but otherwise valid) target / LUN addresses" can
> result in EFI_OUT_OF_RESOURCES and when this happens the while loop in
> SCSIBusDriverBindingStart() should continue, not abort.
> 
> Without this fix, the loop can terminate prematurely with good devices
> not having a chance to be discovered. If this good device is the boot
> device, boot will fail.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Sivaparvathi chellaiah <sivaparvathic@ami.com>
> 
> Signed-off-by: Yuan Yu <yuanyu@google.com>
> ---
>  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> index fbe14c772496..2ed816da4abe 100644
> --- a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> +++ b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> @@ -533,9 +533,6 @@ SCSIBusDriverBindingStart (
>      // then create handle and install scsi i/o protocol.
>      //
>      Status = ScsiScanCreateDevice (This, Controller, &ScsiTargetId, Lun,
> ScsiBusDev);
> -    if (Status == EFI_OUT_OF_RESOURCES) {
> -      goto ErrorExit;
> -    }
>    }
> 
>    return EFI_SUCCESS;
> --
> 2.39.0.314.g84b9a713c41-goog