[if gte mso 9]>

Re: [edk2] Another bug in NVMe driver

Subject: Re: [edk2] Another bug in NVMe driver

From: "Tian, Feng" <feng.tian@intel.com>

To: "Simon (Xiang) Lian-SSI" <simon.lian@ssi.samsung.com>, "edk2-devel@lists.sourceforge.net" <edk2-devel@lists.sourceforge.net>, "Kinney, Michael D" <michael.d.kinney@intel.com>

Date: 2014-01-07 09:04:56

I would prefer not to add error handling here because ComponentName and ComponentName2 are installed at image handles and the uninstallation operation would not fail.

 

Thanks

Feng

 

From: Simon (Xiang) Lian-SSI [mailto:simon.lian@ssi.samsung.com]
Sent: Tuesday, January 07, 2014 06:45
To: Tian, Feng; edk2-devel@lists.sourceforge.net; Kinney, Michael D
Subject: RE: Another bug in NVMe driver

 

The un-installation of ComponentName and ComponentName2 are assumed to be always successful. But in case they fail, the unload should return failure as well. Others look good to me.

 

Thanks,

Simon

 

From: Tian, Feng [mailto:feng.tian@intel.com]
Sent: Sunday, January 05, 2014 5:30 PM
To: edk2-devel@lists.sourceforge.net; Simon (Xiang) Lian-SSI; Kinney, Michael D
Cc: Tian, Feng
Subject: RE: Another bug in NVMe driver

 

Thanks for reminder, Mike.

 

So looks like only the For loop in unload() is redundant, Please review the proposed fix.

 

Best Regards

Feng

 

From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
Sent: Saturday, January 04, 2014 01:55
To: edk2-devel@lists.sourceforge.net; Simon (Xiang) Lian-SSI; Kinney, Michael D
Subject: Re: [edk2] Another bug in NVMe driver

 

Feng,

 

Many UEFI Drivers use the UefiLib to install the driver related protocols and some of the driver related protocols are optionally installed based on PCD settings.  If the Uninstall attempts to uninstall a protocol that is not present, then the uninstall will fail and unload will fail.  This is likely why the logic is a slightly more complex than expected in the Unload() function.

 

The PCDs used by the UefiLib that apply here are:

  gEfiMdePkgTokenSpaceGuid.PcdDriverDiagnosticsDisable    ## CONSUMES

  gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable        ## CONSUMES

  gEfiMdePkgTokenSpaceGuid.PcdDriverDiagnostics2Disable   ## CONSUMES

  gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable       ## CONSUMES

 

Best regards,

 

Mike

 

From: Tian, Feng [mailto:feng.tian@intel.com]
Sent: Thursday, January 02, 2014 6:33 PM
To: Simon (Xiang) Lian-SSI
Cc: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] Another bug in NVMe driver

 

Agree, the logic could be simplified like your proposal.

 

Thanks

Feng

 

From: Simon (Xiang) Lian-SSI [mailto:simon.lian@ssi.samsung.com]
Sent: Friday, January 03, 2014 09:41
To: Tian, Feng
Cc: edk2-devel@lists.sourceforge.net
Subject: RE: Another bug in NVMe driver

 

Hi Feng,

 

One more thing is that in NvmExpressUnload, the following for loop doesn’t seem to be necessary since we are uninstalling all protocols previously attached to this specific driver image handle, ie. DriverBinding->ImageHandle==ImageHandle, all other handles do not even need to be checked.

 

  for (Index = 0; Index < DeviceHandleCount; Index++) {

    Status = gBS->HandleProtocol (

                    DeviceHandleBuffer[Index],

                    &gEfiDriverBindingProtocolGuid,

                    (VOID **) &DriverBinding

                    );

 

    if (EFI_ERROR (Status)) {

      continue;

    }

 

    if (DriverBinding->ImageHandle != ImageHandle) {

      continue;

    }

 

    gBS->UninstallProtocolInterface (

           ImageHandle,

           &gEfiDriverBindingProtocolGuid,

           DriverBinding

           );

 

    Status = gBS->HandleProtocol (

                    DeviceHandleBuffer[Index],

                    &gEfiComponentNameProtocolGuid,

                    (VOID **) &ComponentName

                    );

    if (!EFI_ERROR (Status)) {

      gBS->UninstallProtocolInterface (

             ImageHandle,

             &gEfiComponentNameProtocolGuid,

             ComponentName

             );

}

… // Query and uninstall all other protocols

 

 

Therefore, can the above code be simply replaced with following?

 

  Status = gBS->UninstallMultipleProtocolInterfaces (

                  ImageHandle,

                  &gEfiDriverBindingProtocolGuid,

                  &gNvmExpressDriverBinding,

                  &gEfiComponentNameProtocolGuid,

                  &gNvmExpressComponentName,

                  &gEfiComponentName2ProtocolGuid,

                 &gNvmExpressComponentName2,

                  &gEfiDriverDiagnosticsProtocolGuid,

                  &gNvmExpressDriverDiagnostics,

                  &gEfiDriverDiagnostics2ProtocolGuid,

                  &gNvmExpressDriverDiagnostics2,

                  &gEfiDriverSupportedEfiVersionProtocolGuid,

                  &gNvmExpressDriverSupportedEfiVersion,

                  NULL

                  );

 

 

Thanks,

Simon

 

 

From: Tian, Feng [mailto:feng.tian@intel.com]
Sent: Thursday, January 02, 2014 5:02 PM
To: Simon (Xiang) Lian-SSI
Cc: edk2-devel@lists.sourceforge.net; Tian, Feng
Subject: RE: Another bug in NVMe driver

 

You are right. I will fix this issue as soon as possible.

 

Thanks

Feng

 

From: Simon (Xiang) Lian-SSI [mailto:simon.lian@ssi.samsung.com]
Sent: Friday, January 03, 2014 06:41
To: Tian, Feng
Cc: edk2-devel@lists.sourceforge.net
Subject: Another bug in NVMe driver

 

Hi Feng,

 

There seems to have a minor bug in current NVMe driver that DriverSupportedEfiVersion protocol hasn’t been uninstalled in NvmExpressUnload, resulting in the driver image handle never gets freed up from the handle database.

 

 

Thanks,

Simon