Hi,

(sorry for the late reply)

On 09/08/2017 22:11, Ni, Ruiyu wrote:


Regards,
Ray

-----Original Message-----
From: Paulo Alcantara [mailto:pca...@zytor.com]
Sent: Wednesday, August 9, 2017 10:01 PM
To: Ni, Ruiyu <ruiyu...@intel.com>; Zeng, Star <star.z...@intel.com>; 
edk2-devel@lists.01.org
Cc: Dong, Eric <eric.d...@intel.com>; Wu, Hao A <hao.a...@intel.com>; Justen, Jordan 
L <jordan.l.jus...@intel.com>;
Andrew Fish <af...@apple.com>; Gao, Liming <liming....@intel.com>; Kinney, Michael D 
<michael.d.kin...@intel.com>;
Laszlo Ersek <ler...@redhat.com>
Subject: Re: [edk2] [PATCH 0/4] read-only UDF file system support

(heh - forgot to answer your question about the GUID :-) )

On 8/9/2017 10:26 AM, Paulo Alcantara wrote:
Hi Ray,

Thanks for the review. My comments below.

On 8/9/2017 3:05 AM, Ni, Ruiyu wrote:
Paulo,
Thanks for enabling the UDF support into EDKII.
Below are several comments:
1. Could you please separate the patch to modify MdePkg and MdeModulePkg?

Sure.

2. UDF_CDROM_VOLUME_IDENTIFIER is also defined in Eltorito.h as CDVOL_ID.
       Maybe we could redefine CDVOL_ID to UDF_CDROM_VOLUME_IDENTIFIER?

The Volume Identifier structure is defined in ECMA-167 specification
(not UDF specific), so perhaps it would be better if we create a
separate header file (e.g., Ecma-167.h) and define
ECMA167_VOLUME_IDENTIFIER structure. That would be used in both ElTorito
and UDF codes. What do you think?


2. Why do you need a PCD to control the UDF support? I prefer no. More
choices
       is not always good😊

The original idea of including this PCD was to avoid adding unnecessary
overhead in PartitionDxe driver to something (UDF) that wasn't actually
defined in UEFI specification -- leaving it as an optional feature.

But yes, I agree with you that just complicates things and would be
better to remove it.


3.  Is gUdfVolumeSignatureGuid only used in Partition driver to
produce the device
        path? Can we just use HARDDRIVE_DEVICE_PATH? Or at least
gUdfVolumeSignatureGuid
        can be removed, the GUID macro is enough?

The GUID is used in both Partition and UdfDxe drivers. Do you mean that
we should use HARDDRIVE_DEVICE_PATH and then appending the UDF-specific
GUID to it?

I originally thought UdfDxe driver needs to use this GUID to know it's a UDF 
partition.
But later I found UdfDxe contains a function SupportUdfFileSystem() to parse the
partition content to decide whether to manage that partition.
I wasn't able to find the reference of this GUID.

Yes - you're right. I think it should just check for the installed GUID, indeed. So we don't have to parse and look for UDF file systems twice.


I just tried again, still didn't find it. But I did find some improper 
implementation of
driver-model logic.
I saw UdfDxe uses OPEN_PROTOCOL_GET to open the BlockIo and DiskIo.
It should use OPEN_PROTOCOL_BY_DRIVER.
In details, it should firstly try OPEN_BY_DRIVER to open the BlockIo and DiskIo 
in
Supported(), when either one fails, the Supported() returns failure. But please
keep in mind to close the successfully-opened BlockIo/DiskIo. Supported() is a
test function called by DxeCore, and it should not alter the system state.
Start() should then repeat the same open logic as that in Supported(), but it's
find to use ASSERT_EFI_ERROR() to assert the return status of OPEN_BY_DRIVER
is EFI_SUCCESS because per UEFI spec, Start() should only be called when 
Supported()
returns EFI_SUCCESS. Start() will then install the SimpleFileSystem instance on 
the
same Handle that have BlockIo/DiskIo installed. (I noticed that your driver 
creates
a new handle for SimpleFileSystem, which is unnecessary and may break the driver
model chain.)

The OPEN_BY_DRIVER open is necessary because DxeCore driver model core logic
records the special open operation. and when BlockIo or DiskIo is uninstalled,
the Stop() function whose corresponding Start() is opening the same 
BlockIo/DiskIo
OPEN_BY_DRIVER will get called. So you can treat OPEN_BY_DRIVER is a mechanism
to notify the upper layer driver to destroy the newly created 
service(SimpleFileSystem)
when lower layer services (BlockIo/DiskIo) it layers on is destroyed 
(Uninstalled).

Not sure if I explained well. You could refer to
https://github.com/tianocore/tianocore.github.io/wiki/UEFI-Driver-Writer's-Guide
written by Kinney Michael for detailed instructions.

Good catch! You explained it very well. I'll fix that up in the next series.

Thanks!
Paulo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to