[edk2-devel] [PATCH] MdeModulePkg/ScsiDisk: Change TPL to NOTIFY
Wu, Hao A
hao.a.wu at intel.com
Thu Jun 23 02:53:59 UTC 2022
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 at edk2.groups.io; jbrasen at nvidia.com
> Cc: Ni, Ray <ray.ni at intel.com>; Kinney, Michael D
> <michael.d.kinney at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>;
> Zeng, Star <star.zeng at 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 at edk2.groups.io <devel at 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 at intel.com>; devel at edk2.groups.io
> > Cc: Ni, Ray <ray.ni at 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 at intel.com>; devel at edk2.groups.io
> > > Cc: Ni, Ray <ray.ni at intel.com>
> > > Subject: RE: [PATCH] MdeModulePkg/ScsiDisk: Change TPL to NOTIFY
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wu, Hao A <hao.a.wu at intel.com>
> > > > Sent: Tuesday, December 14, 2021 8:00 PM
> > > > To: Jeff Brasen <jbrasen at nvidia.com>; devel at edk2.groups.io
> > > > Cc: Ni, Ray <ray.ni at 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 at nvidia.com>
> > > > > Sent: Wednesday, December 15, 2021 1:59 AM
> > > > > To: devel at edk2.groups.io
> > > > > Cc: Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>;
> > > > > Jeff Brasen <jbrasen at 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 at 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
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90708): https://edk2.groups.io/g/devel/message/90708
Mute This Topic: https://groups.io/mt/87726872/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list