Hi Hao Wu,

Thank you very much for the review and the detailed advice, which is
much appreciated!

I have made the changes accordingly and sent patch v2. Please take another
look.

Thank you!

Yuan


On Wed, Jan 18, 2023 at 10:33 PM Wu, Hao A <hao.a.wu@intel.com> wrote:

> 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
>
>