Re: [PATCH v2 1/1] MdeModulePkg: ScsiBusDxe: Refactor DiscoverScsiDevice()

["Yuan Yu" <yuanyu@google.com>]

[-- Attachment #1: Type: text/plain, Size: 6802 bytes --]

Sounds good. Thank you!

Best,
Yuan

On Tue, Jan 31, 2023 at 10:48 PM Wu, Hao A <hao.a.wu@intel.com> wrote:

> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> Will update the function comment for ScsiScanCreateDevice() in ScsiBus.c
> to match the change in ScsiBus.h during merge.
>
> Best Regards,
> Hao Wu
>
> > -----Original Message-----
> > From: Yuan Yu <yuanyu@google.com>
> > Sent: Tuesday, January 31, 2023 2:04 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 v2 1/1] MdeModulePkg: ScsiBusDxe: Refactor
> > DiscoverScsiDevice()
> >
> > Currently DiscoverScsiDevice() returns a boolean which cannot
> > distinguish a "not found" situation from a real problem like
> > memory allocation failures.
> >
> > This patch changes the return value to an EFI_STATUS so that when
> > memory allocation fails, it will return EFI_OUT_OF_RESOURCES.
> >
> > Without this change, any FALSE returned by DiscoverScsiDevice()
> > will result in EFI_OUT_OF_RESOURCES being returned by
> > ScsiScanCreateDevice(), which will cause a while loop in
> > SCSIBusDriverBindingStart() to abort before other possible Puns in
> > the SCSI channel are scanned, which means good devices may not have
> > 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.h | 10 ++++---
> >  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c | 30 ++++++++++----------
> >  2 files changed, 21 insertions(+), 19 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.h
> > b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.h
> > index 68c5c02a9161..35a8a46ca7a2 100644
> > --- a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.h
> > +++ b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.h
> > @@ -455,7 +455,8 @@ ScsiExecuteSCSICommand (
> >
> >    @retval EFI_SUCCESS           Successfully to discover the device and
> attach
> >                                  ScsiIoProtocol to it.
> > -  @retval EFI_OUT_OF_RESOURCES  Fail to discover the device.
> > +  @retval EFI_NOT_FOUND         Fail to discover the device.
> > +  @retval EFI_OUT_OF_RESOURCES  Fail to allocate memory resources.
> >
> >  **/
> >  EFI_STATUS
> > @@ -473,11 +474,12 @@ ScsiScanCreateDevice (
> >
> >    @param  ScsiIoDevice    The pointer of SCSI_IO_DEV
> >
> > -  @retval  TRUE   Find SCSI Device and verify it.
> > -  @retval  FALSE  Unable to find SCSI Device.
> > +  @retval EFI_SUCCESS           Find SCSI Device and verify it.
> > +  @retval EFI_NOT_FOUND         Unable to find SCSI Device.
> > +  @retval EFI_OUT_OF_RESOURCES  Fail to allocate memory resources.
> >
> >  **/
> > -BOOLEAN
> > +EFI_STATUS
> >  DiscoverScsiDevice (
> >    IN  OUT  SCSI_IO_DEV  *ScsiIoDevice
> >    );
> > diff --git a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> > b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> > index fbe14c772496..4414a65eb1e6 100644
> > --- a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> > +++ b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> > @@ -1210,8 +1210,8 @@ ScsiScanCreateDevice (
> >      ScsiBusDev->DevicePath
> >      );
> >
> > -  if (!DiscoverScsiDevice (ScsiIoDevice)) {
> > -    Status = EFI_OUT_OF_RESOURCES;
> > +  Status = DiscoverScsiDevice (ScsiIoDevice);
> > +  if (EFI_ERROR (Status)) {
> >      goto ErrorExit;
> >    }
> >
> > @@ -1276,11 +1276,12 @@ ErrorExit:
> >
> >    @param  ScsiIoDevice    The pointer of SCSI_IO_DEV
> >
> > -  @retval  TRUE   Find SCSI Device and verify it.
> > -  @retval  FALSE  Unable to find SCSI Device.
> > +  @retval EFI_SUCCESS           Find SCSI Device and verify it.
> > +  @retval EFI_NOT_FOUND         Unable to find SCSI Device.
> > +  @retval EFI_OUT_OF_RESOURCES  Fail to allocate memory resources.
> >
> >  **/
> > -BOOLEAN
> > +EFI_STATUS
> >  DiscoverScsiDevice (
> >    IN OUT  SCSI_IO_DEV  *ScsiIoDevice
> >    )
> > @@ -1294,7 +1295,6 @@ DiscoverScsiDevice (
> >    EFI_SCSI_SENSE_DATA    *SenseData;
> >    UINT8                  MaxRetry;
> >    UINT8                  Index;
> > -  BOOLEAN                ScsiDeviceFound;
> >
> >    HostAdapterStatus = 0;
> >    TargetStatus      = 0;
> > @@ -1302,7 +1302,7 @@ DiscoverScsiDevice (
> >
> >    InquiryData = AllocateAlignedBuffer (ScsiIoDevice, sizeof
> > (EFI_SCSI_INQUIRY_DATA));
> >    if (InquiryData == NULL) {
> > -    ScsiDeviceFound = FALSE;
> > +    Status = EFI_OUT_OF_RESOURCES;
> >      goto Done;
> >    }
> >
> > @@ -1311,7 +1311,7 @@ DiscoverScsiDevice (
> >                  sizeof (EFI_SCSI_SENSE_DATA)
> >                  );
> >    if (SenseData == NULL) {
> > -    ScsiDeviceFound = FALSE;
> > +    Status = EFI_OUT_OF_RESOURCES;
> >      goto Done;
> >    }
> >
> > @@ -1342,7 +1342,7 @@ DiscoverScsiDevice (
> >            (SenseData->Error_Code == 0x70) &&
> >            (SenseData->Sense_Key == EFI_SCSI_SK_ILLEGAL_REQUEST))
> >        {
> > -        ScsiDeviceFound = FALSE;
> > +        Status = EFI_NOT_FOUND;
> >          goto Done;
> >        }
> >
> > @@ -1353,13 +1353,13 @@ DiscoverScsiDevice (
> >          (Status == EFI_INVALID_PARAMETER) ||
> >          (Status == EFI_UNSUPPORTED))
> >      {
> > -      ScsiDeviceFound = FALSE;
> > +      Status = EFI_NOT_FOUND;
> >        goto Done;
> >      }
> >    }
> >
> >    if (Index == MaxRetry) {
> > -    ScsiDeviceFound = FALSE;
> > +    Status = EFI_NOT_FOUND;
> >      goto Done;
> >    }
> >
> > @@ -1367,14 +1367,14 @@ DiscoverScsiDevice (
> >    // Retrieved inquiry data successfully
> >    //
> >    if (InquiryData->Peripheral_Qualifier != 0) {
> > -    ScsiDeviceFound = FALSE;
> > +    Status = EFI_NOT_FOUND;
> >      goto Done;
> >    }
> >
> >    if ((InquiryData->Peripheral_Type >= EFI_SCSI_TYPE_RESERVED_LOW) &&
> >        (InquiryData->Peripheral_Type <= EFI_SCSI_TYPE_RESERVED_HIGH))
> >    {
> > -    ScsiDeviceFound = FALSE;
> > +    Status = EFI_NOT_FOUND;
> >      goto Done;
> >    }
> >
> > @@ -1392,13 +1392,13 @@ DiscoverScsiDevice (
> >      ScsiIoDevice->ScsiVersion = (UINT8)(InquiryData->Version & 0x07);
> >    }
> >
> > -  ScsiDeviceFound = TRUE;
> > +  Status = EFI_SUCCESS;
> >
> >  Done:
> >    FreeAlignedBuffer (SenseData, sizeof (EFI_SCSI_SENSE_DATA));
> >    FreeAlignedBuffer (InquiryData, sizeof (EFI_SCSI_INQUIRY_DATA));
> >
> > -  return ScsiDeviceFound;
> > +  return Status;
> >  }
> >
> >  /**
> > --
> > 2.39.1.456.gfc5497dd1b-goog
>
>

[-- Attachment #2: Type: text/html, Size: 9581 bytes --]