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

["Jeff Brasen" <jbrasen@nvidia.com>]

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