From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.52]) by mx.groups.io with SMTP id smtpd.web11.6460.1675145254621081533 for ; Mon, 30 Jan 2023 22:07:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@google.com header.s=20210112 header.b=rPgO5zDk; spf=pass (domain: google.com, ip: 209.85.218.52, mailfrom: yuanyu@google.com) Received: by mail-ej1-f52.google.com with SMTP id bk15so38491162ejb.9 for ; Mon, 30 Jan 2023 22:07:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=dLo6ye0WK73uUSTYapPZIJ9I2Y9n18qaBGAhs29s47Y=; b=rPgO5zDkNX6zWo9GNdls1b6LaJ2tXzHRsW9cAJWpU0rqN+Rr5YwIyP7cgbC8wHweDp FOWyySRF6HWnwad+T/oIkqbG++FdHkLAkmrn4fVr1yOeUnCSE6Bt/L/tVdFhlkX2decs Ijvk5LJNuOD+MRG4jWJNoLeCi8vCCQHHq/OF5kUOgkd/AaSCBW7i3MUqMn/gSUdeuOvO d3Cs4gdSkoDeYFUixBSA8simflvhYIY2+sQrb+0zzqEgmTya8dBV3KW9SXTxHb0TkfqS IfpeE03MTPu8yiI1fy94FVIQzkofvXkP+Mc45thlKyeMkpuntloWduJf5l5ABRys+wXt EPkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=dLo6ye0WK73uUSTYapPZIJ9I2Y9n18qaBGAhs29s47Y=; b=jX/1J3GD2sSExr0S6GuPv0EwkpjZnPH9bhqnvf5SkSx95nOKyISEXYTwUO7pdlrCy6 5gkFjAfXCYGLPFjy43kRAnngrsEyeeymwstyw5nN/MSHhmCgqxmboBPQz74CG/j2ZHv5 j2rK7Xuk2TrtuiSuBtOq3X7ZWKK3afWk8Z+b5s9Yw8s7ytG6t7izLl+OXzemQQ/5tOVC xgvgJ4FdjRgYNXS6VnfztG8Lp/IPOL6eEulacaIpUlmLhahakP2fcA3MUBps8aioZ2+W l87KuVzACksckVTa/bzSVNcuc5vnPQ/HZ78x8Akg2DEaKZjk5uRaS00f2gys12SjNE+l utgQ== X-Gm-Message-State: AO0yUKXl1/FzoqqM2sNYu6BvGyN0H9CRaJWZJKxmJIrjsXLNJW798zZt n3ihIxBaYuA/r9ZRXhxpmIMHsGVz5cZGS1bJhGMkew== X-Google-Smtp-Source: AK7set+WUfD0csc2ej8saTE4QkVa2/IXDE2Ta9hmahafTblO9gEcdI46+mbFNbkxYFk/+W7ot2cDZ0JvJ+qob03D8X8= X-Received: by 2002:a17:906:d191:b0:88c:6326:54b0 with SMTP id c17-20020a170906d19100b0088c632654b0mr189884ejz.153.1675145252852; Mon, 30 Jan 2023 22:07:32 -0800 (PST) MIME-Version: 1.0 References: <20230118091402.931498-1-yuanyu@google.com> <20230118091402.931498-2-yuanyu@google.com> In-Reply-To: From: "Yuan Yu" Date: Mon, 30 Jan 2023 22:06:56 -0800 Message-ID: Subject: Re: [PATCH v1 1/2] MdeModulePkg: Fix bug in ScsiBusDxe/ScsiBus.c To: "Wu, Hao A" Cc: "devel@edk2.groups.io" , Ard Biesheuvel , "Gao, Liming" , "Ni, Ray" , "C, sivaparvathi" Content-Type: multipart/alternative; boundary="000000000000bf2d1005f38925e2" --000000000000bf2d1005f38925e2 Content-Type: text/plain; charset="UTF-8" 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 wrote: > Thanks for the patch, inline comments below: > > > > -----Original Message----- > > From: Yuan Yu > > Sent: Wednesday, January 18, 2023 5:14 PM > > To: devel@edk2.groups.io > > Cc: Ard Biesheuvel ; Gao, Liming > > ; Wu, Hao A ; Ni, Ray > > ; C, sivaparvathi > > 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 > > Cc: Liming Gao > > Cc: Hao A Wu > > Cc: Ray Ni > > Cc: Sivaparvathi chellaiah > > > > Signed-off-by: Yuan Yu > > --- > > 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 > > --000000000000bf2d1005f38925e2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Hao Wu,

Thank you very mu= ch for the review and the detailed advice, which is much=C2=A0appreciated!<= /div>

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.co= m> 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@ed= k2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Gao, Liming
> <gaol= iming@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<= br> >
> A while loop in SCSIBusDriverBindingStart() is supposed to scan all th= e
> possible Puns in the SCSI channel by calling ScsiScanCreateDevice() fo= r
> 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()
>=C2=A0 =C2=A0> DiscoverScsiDevice()
>=C2=A0 =C2=A0 =C2=A0> ScsiInquiryCommand()
>=C2=A0 =C2=A0 =C2=A0 =C2=A0> ...
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> ParseResponse()
>
> When virtio-scsi returns VIRTIO_SCSI_S_BAD_TARGET, ParseResponse() in<= br> > VirtioScsi.c will return EFI_TIMEOUT:
>
>=C2=A0 =C2=A0 =C2=A0case VIRTIO_SCSI_S_BAD_TARGET:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0//
>=C2=A0 =C2=A0 =C2=A0 =C2=A0// This is non-intuitive but explicitly requ= ired by the
>=C2=A0 =C2=A0 =C2=A0 =C2=A0// EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru(= ) specification for
>=C2=A0 =C2=A0 =C2=A0 =C2=A0// disconnected (but otherwise valid) target= / LUN addresses.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0//
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Packet->HostAdapterStatus =3D
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIME= OUT_COMMAND;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return EFI_TIMEOUT;
>
> This will eventually cause DiscoverScsiDevice() to return FALSE, which=
> will cause ScsiScanCreateDevice() to return EFI_OUT_OF_RESOURCES:
>
>=C2=A0 =C2=A0if (!DiscoverScsiDevice (ScsiIoDevice)) {
>=C2=A0 =C2=A0 =C2=A0Status =3D EFI_OUT_OF_RESOURCES;
>=C2=A0 =C2=A0 =C2=A0goto ErrorExit;
>=C2=A0 =C2=A0}


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

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

But when DiscoverScsiDevice() returns FALSE, there are cases where the caus= e 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:
=C2=A0 =C2=A0 a) SCSI command execution failure or
=C2=A0 =C2=A0 b) Return data of the command indicates no device

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

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


The reason for the below logic in SCSIBusDriverBindingStart():

=C2=A0 =C2=A0 Status =3D ScsiScanCreateDevice (This, Controller, &ScsiT= argetId, Lun, ScsiBusDev);
=C2=A0 =C2=A0 if (Status =3D=3D EFI_OUT_OF_RESOURCES) {
=C2=A0 =C2=A0 =C2=A0 goto ErrorExit;
=C2=A0 =C2=A0 }

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 a= llocations are very likely
to fail as well, so there is little value in continuing the loop for furthe= r device discovery.


Best Regards,
Hao Wu


>
> In sum, "disconnected (but otherwise valid) target / LUN addresse= s" 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<= br> > 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 <r= ay.ni@intel.com>
> Cc: Sivaparvathi chellaiah <sivaparvathic@ami.com>
>
> Signed-off-by: Yuan Yu <yuanyu@google.com>
> ---
>=C2=A0 MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c | 3 ---
>=C2=A0 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 (
>=C2=A0 =C2=A0 =C2=A0 // then create handle and install scsi i/o protoco= l.
>=C2=A0 =C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 =C2=A0 Status =3D ScsiScanCreateDevice (This, Controller,= &ScsiTargetId, Lun,
> ScsiBusDev);
> -=C2=A0 =C2=A0 if (Status =3D=3D EFI_OUT_OF_RESOURCES) {
> -=C2=A0 =C2=A0 =C2=A0 goto ErrorExit;
> -=C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 }
>
>=C2=A0 =C2=A0 return EFI_SUCCESS;
> --
> 2.39.0.314.g84b9a713c41-goog

--000000000000bf2d1005f38925e2--