Re: [edk2-devel] [PATCH] MdeModulePkg/ScsiDisk: Change TPL to NOTIFY

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

Fix a typo below:
blocking I/O request will wait for all the (synchronous -> asynchronous) I/O requests to complete first before sending down the synchronous request

Best Regards,
Hao Wu

> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, June 23, 2022 10:47 AM
> To: devel@edk2.groups.io; jbrasen@nvidia.com
> Cc: Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/ScsiDisk: Change TPL to
> NOTIFY
> 
> Hello,
> 
> Several concerns:
> 1. Raising the TPL to NOTIFY level for blocking/synchronous BlockIO service
> will break the using case when the co-existence of synchronous and non-
> blocking/asynchronous IO tasks
> As far as I know, in DiskIoDxe, blocking I/O request will wait for all the
> synchronous I/O requests to complete first before sending down the
> synchronous request.
> Please consider the scenario where a previous asynchronous IO task, it will
> take a long time to complete due to big data transfer size.
> When a subsequent synchronous IO task is submitted before the
> asynchronous task completes, it will wait for the previous asynchronous task
> to complete.
> However, if the TPL level for this synchronous task is NOTIFY, it will get stuck
> since the completion callback for the asynchronous task is also at NOTIFY TPL
> level, which will never be called.
> More details can be referred to commit:
> SHA-1: a717086c5f973821b9b49646a4ec725f6b898bdb
> * MdeModulePkg ScsiDiskDxe: Raise the Tpl of async IO callback to
> TPL_NOTIFY
> 
> 2. My personal take is that BlockIO is not a proper base service for variable
> service
> To my knowledge, the variable service will still exist during runtime, but the
> BlockIO services (as far as I know) do not.
> My understanding is that the main purpose of the BlockIO services are to
> provide BIOS boot codes with the ability to locate partitions and file systems
> on media storage devices and search boot options on them.
> I would recommend to implement a dedicated platform driver to produce
> the FVB services for variable driver consumption.
> 
> If you think using the BlockIO service as the base for variable service is the
> right direction, I have added people in the CC list that might help.
> Could you please reach out to them for feedbacks on the TPL restriction on
> BlockIO services and Variable service?
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeff
> > Brasen via groups.io
> > Sent: Friday, June 17, 2022 11:37 PM
> > To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>
> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/ScsiDisk: Change TPL to
> > NOTIFY
> >
> > Resending this as I replying via edk2.groups.io doesn't seem to copy
> > maintainers.
> >
> > Resuming this patch to see if there are any additional thoughts on this.
> >
> > In response to the query about DXE/BDS services we have some internal
> > connection logic that runs in DXE to connect the devices that are needed
> for
> > arch services that have to be connected prior the end of dxe.
> >
> > Thanks,
> > Jeff
> >
> > > -----Original Message-----
> > > From: Jeff Brasen
> > > Sent: Tuesday, December 14, 2021 9:48 PM
> > > To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> > > Cc: Ni, Ray <ray.ni@intel.com>
> > > Subject: RE: [PATCH] MdeModulePkg/ScsiDisk: Change TPL to NOTIFY
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wu, Hao A <hao.a.wu@intel.com>
> > > > Sent: Tuesday, December 14, 2021 8:00 PM
> > > > To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> > > > Cc: Ni, Ray <ray.ni@intel.com>
> > > > Subject: RE: [PATCH] MdeModulePkg/ScsiDisk: Change TPL to NOTIFY
> > > >
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jeff Brasen <jbrasen@nvidia.com>
> > > > > Sent: Wednesday, December 15, 2021 1:59 AM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > > > > Jeff Brasen <jbrasen@nvidia.com>
> > > > > Subject: [PATCH] MdeModulePkg/ScsiDisk: Change TPL to NOTIFY
> > > > >
> > > > > Increase TPL to TPL_NOTIFY to allow for use if caller is >
> TPL_CALLBACK.
> > > > > This allows services like variable services that run at TPL_NOTIFY
> > > > > to be hosted on ScsiDisks (i.e. UFS)
> > > > >
> > > > > Aligns with the eMMC driver that also uses a higher TPL.
> > > > > This change was made in
> > 3b1d8241d0dac25c5e678c364fa2754ac1731060
> > > >
> > > >
> > > > Sorry, my take is that this change is not equivalent to the one made
> > > > in the SD/MMC stack.
> > > >
> > > > For the SD/MMC change you mentioned (commit
> > > > 3b1d8241d0dac25c5e678c364fa2754ac1731060), the TPL is raised to
> > > > TPL_NOTIFY only when:
> > > >   a) Operation to the linked lists that manage the asynchronous IO tasks
> > > >   b) Callback functions that process the asynchronous IO tasks The
> > > > TPL remains TPL_CALLBACK during the BlockIO services and the
> > > > majority of the
> > > > BlockIO2 services (operations to asynchronous tasks linked list are
> > > > the exceptions).
> > > >
> > > > But the proposed change in ScsiDisk modifies the TPL level of the
> > > > entire
> > > > BlockIO/BlockIO2 (and other protocols) services to TPL_NOTIFY.
> > > > For me, this is not aligned with the "TPL Restrictions" documented
> > > > in the UEFI specification.
> > > >
> > > > Best Regards,
> > > > Hao Wu
> > > >
> > > >
> > >
> > > I had sent out a query on this before and didn't see any response. The
> > > core of the issue I am trying to solve it support variable services on a UFS
> > device.
> > > When the UFS blockIO is invoked from variable services it is not
> > > allowed (which does align from the UEFI spec perspective but does not
> > > allow me to implement variables services on UFS)
> > >
> > >  The other way that worked was lowering the lock TPL level in the PCD
> > > driver and Variable down to callback. The PCD one seems like it should
> > > be done as variable services is supposed to only be called from <=
> > TPL_CALLBACK.
> > > However, I was worried about that having a larger system impact on
> > > that change.
> > >
> > > > >
> > > > > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > > > > ---
> > > > >  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 22
> > > > > ++++++++++----------
> > > > >  1 file changed, 11 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> > > > > b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> > > > > index 98e84b4ea8..b6e5848e77 100644
> > > > > --- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> > > > > +++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> > > > > @@ -514,7 +514,7 @@ ScsiDiskReset (
> > > > >    SCSI_DISK_DEV  *ScsiDiskDevice;
> > > > >    EFI_STATUS     Status;
> > > > >
> > > > > -  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >
> > > > >    ScsiDiskDevice = SCSI_DISK_DEV_FROM_BLKIO (This);
> > > > >
> > > > > @@ -581,7 +581,7 @@ ScsiDiskReadBlocks (
> > > > >    EFI_TPL             OldTpl;
> > > > >
> > > > >    MediaChange    = FALSE;
> > > > > -  OldTpl         = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl         = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >    ScsiDiskDevice = SCSI_DISK_DEV_FROM_BLKIO (This);
> > > > >    Media          = ScsiDiskDevice->BlkIo.Media;
> > > > >
> > > > > @@ -733,7 +733,7 @@ ScsiDiskWriteBlocks (
> > > > >    EFI_TPL             OldTpl;
> > > > >
> > > > >    MediaChange    = FALSE;
> > > > > -  OldTpl         = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl         = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >    ScsiDiskDevice = SCSI_DISK_DEV_FROM_BLKIO (This);
> > > > >    Media          = ScsiDiskDevice->BlkIo.Media;
> > > > >
> > > > > @@ -898,7 +898,7 @@ ScsiDiskResetEx (
> > > > >    SCSI_DISK_DEV  *ScsiDiskDevice;
> > > > >    EFI_STATUS     Status;
> > > > >
> > > > > -  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >
> > > > >    ScsiDiskDevice = SCSI_DISK_DEV_FROM_BLKIO2 (This);
> > > > >
> > > > > @@ -975,7 +975,7 @@ ScsiDiskReadBlocksEx (
> > > > >    EFI_TPL             OldTpl;
> > > > >
> > > > >    MediaChange    = FALSE;
> > > > > -  OldTpl         = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl         = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >    ScsiDiskDevice = SCSI_DISK_DEV_FROM_BLKIO2 (This);
> > > > >    Media          = ScsiDiskDevice->BlkIo.Media;
> > > > >
> > > > > @@ -1154,7 +1154,7 @@ ScsiDiskWriteBlocksEx (
> > > > >    EFI_TPL             OldTpl;
> > > > >
> > > > >    MediaChange    = FALSE;
> > > > > -  OldTpl         = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl         = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >    ScsiDiskDevice = SCSI_DISK_DEV_FROM_BLKIO2 (This);
> > > > >    Media          = ScsiDiskDevice->BlkIo.Media;
> > > > >
> > > > > @@ -1323,7 +1323,7 @@ ScsiDiskFlushBlocksEx (
> > > > >    EFI_TPL             OldTpl;
> > > > >
> > > > >    MediaChange    = FALSE;
> > > > > -  OldTpl         = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl         = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >    ScsiDiskDevice = SCSI_DISK_DEV_FROM_BLKIO2 (This);
> > > > >    Media          = ScsiDiskDevice->BlkIo.Media;
> > > > >
> > > > > @@ -1717,7 +1717,7 @@ ScsiDiskEraseBlocks (
> > > > >    EFI_TPL             OldTpl;
> > > > >
> > > > >    MediaChange    = FALSE;
> > > > > -  OldTpl         = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl         = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >    ScsiDiskDevice = SCSI_DISK_DEV_FROM_ERASEBLK (This);
> > > > >
> > > > >    if (!IS_DEVICE_FIXED (ScsiDiskDevice)) { @@ -1907,7 +1907,7 @@
> > > > > ScsiDiskReceiveData (
> > > > >    AlignedBuffer          = NULL;
> > > > >    MediaChange            = FALSE;
> > > > >    AlignedBufferAllocated = FALSE;
> > > > > -  OldTpl                 = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl                 = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >    ScsiDiskDevice         = SCSI_DISK_DEV_FROM_STORSEC (This);
> > > > >    Media                  = ScsiDiskDevice->BlkIo.Media;
> > > > >
> > > > > @@ -2122,7 +2122,7 @@ ScsiDiskSendData (
> > > > >    AlignedBuffer          = NULL;
> > > > >    MediaChange            = FALSE;
> > > > >    AlignedBufferAllocated = FALSE;
> > > > > -  OldTpl                 = gBS->RaiseTPL (TPL_CALLBACK);
> > > > > +  OldTpl                 = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >    ScsiDiskDevice         = SCSI_DISK_DEV_FROM_STORSEC (This);
> > > > >    Media                  = ScsiDiskDevice->BlkIo.Media;
> > > > >
> > > > > @@ -2294,7 +2294,7 @@ ScsiDiskDetectMedia (
> > > > >
> > > > >    Status = gBS->CreateEvent (
> > > > >                    EVT_TIMER,
> > > > > -                  TPL_CALLBACK,
> > > > > +                  TPL_NOTIFY,
> > > > >                    NULL,
> > > > >                    NULL,
> > > > >                    &TimeoutEvt
> > > > > --
> > > > > 2.17.1
> >
> >
> >
> > 
> >