ARM's implementation was initial done by a summer student (Brendan). I asked him to implement a file system on top of FV to be able to start Android Fastboot UEFI application located in a FV from BDS. He quickly wrote a first implementation. We later refined his implementation to fix some issues and be more conformant with the UEFI spec.
I do not remember the full details of the story but we were aware of Colin's work. Maybe we knew it before Brendan started his work or we discovered it a bit later. I asked him to evaluate Colin's solution. In his own words: "looks like it's probably better than FvSimpleFilesystemDxe". So maybe we kept FvSimpleFilesystemDxe because we wanted it to be part of the EDK2 repository as our reference implementation would rely on it and we did not add a requirement on pulling external repositories. Anyway, just to say the EDK2 repository should be more open to external packages in the sense they should be part of the repository. There are few useful contributions floating around such as file systems support (Ext2, etc), platform support (BeagleBoard black, Pandaboard, etc), driver support, google summer of code projects. These projects would have a better visibility and support if there were part of EDK2 source tree. That would also avoid people to duplicate work. Including these projects would also bring more contributions/fixes to the core EDK2. An argument often used to prefer u-boot to EDK2/UEFI is u-boot has a better open platform/driver support. > -----Original Message----- > From: Jordan Justen [mailto:jordan.l.jus...@intel.com] > Sent: 17 December 2014 19:50 > To: Tian, Feng; Olivier Martin; Jin, Eric > Cc: edk2-devel@lists.sourceforge.net; Colin Drake > Subject: Re: [edk2] patch review feedback about FvSimpleFileSystem > module > > On 2014-12-16 23:29:05, Tian, Feng wrote: > > Martin, > > > > Please review the latest patch again, > > Once again, you did not supply a Contributed-under or Signed-off-by. > > > in which partial read is supported. > > I think Colin's version already supported this: > https://github.com/cfdrake/FileSystemPkg > > struct _FILE_PRIVATE_DATA.Position: > https://github.com/cfdrake/FileSystemPkg/blob/master/FfsDxe/Ffs.h#L118 > > FfsRead: > https://github.com/cfdrake/FileSystemPkg/blob/master/FfsDxe/Ffs.c#L930 > > > Password: efi;0000 > > I don't think a password zip file is a good way to provide patches. > > I think the best response would have been to reply to Olivier's > original patch, and point out changes that should be made. This way > ARM could make the changes, and we wouldn't have a munged up copyright > situation. > > A second best approach would be to apply Olivier's patch, and then > make a new patch of your own based on his patch. (Then we could easily > see what you changed from his.) > > At the least, it seems like you need to add Contributed-under and > Signed-off-by to be appended to Olivier's patch commit message. > > -Jordan > > > -----Original Message----- > > From: Tian, Feng > > Sent: Monday, December 08, 2014 09:28 > > To: Olivier Martin; Justen, Jordan L; Jin, Eric > > Cc: edk2-devel@lists.sourceforge.net; Tian, Feng > > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem > module > > > > Hi, Martin > > > > Recently I found this patch has other serious issues. So the check-in > has to be delayed. > > > > According to UEFI spec, the SimpleFileSystem protocol interface > produced must support partial read. But the patch doesn't support it. > > > > And I ever said I had made the patch pass UEFI SCT, but after digging > into it, I found it's because the UEFI SCT SimpleFileSystem test case > has bug as well. > > > > Now I am working with SCT owner to solve these bugs together. > > > > Thanks > > Feng > > > > -----Original Message----- > > From: Olivier Martin [mailto:olivier.mar...@arm.com] > > Sent: Saturday, December 06, 2014 03:19 > > To: Justen, Jordan L; Tian, Feng > > Cc: edk2-devel@lists.sourceforge.net > > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem > module > > > > Feng, what is the next step for moving forward with this patch. > > As I said in a previous email, I am happy with the changes you added > to the original patch. > > Thanks, > > Olivier > > > > > -----Original Message----- > > > From: Jordan Justen [mailto:jordan.l.jus...@intel.com] > > > Sent: 13 November 2014 02:50 > > > To: Tian, Feng; Olivier Martin > > > Cc: edk2-devel@lists.sourceforge.net > > > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem > > > module > > > > > > On 2014-11-12 18:10:11, Tian, Feng wrote: > > > > Jordan, > > > > > > > > As Contributed-under and Signed-off-by message are mandatory > infos, > > > > I didn't mention it. > > > > > > Yes, it is mandatory that every contribution have this. Therefore, > it > > > should be mentioned with every contribution of code. I usually > don't > > > even look at a contribution that doesn't have these. > > > > > > > Ok, let's mend the commit message: > > > > > > > > MdeModulePkg/FvSimpleFileSystem: Add a new module to provide > access > > > to executable files in FVs. > > > > > > > > This module implements Simple FileSystem protocol over Firmware > > > Volume (FV). > > > > EFI Modules included into a FV can be listed and launched from > the > > > > EFI Shell or any other EFI applications. > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > > Signed-off-by: Brendan Jackman <brendan.jack...@arm.com> > > > > Signed-off-by: Feng Tian <feng.t...@intel.com> > > > > Reviewed-by: Olivier Martin <olivier.mar...@arm.com> > > > > > > > > I would prefer to let package maintainer, that's me:), to commit > > > > code for quality control. Just giving feedback is not enough as > we > > > > have additional works to do. > > > > > > I do agree about testing. I would pull in Olivier's change. Test > it, > > > and only then give him a r-b and tell him it is okay for him to > commit > > > the change when his series is ready. > > > > > > But, you can handle contributions differently for the packages you > > > maintain. > > > > > > Except, one note. If the contribution had several patches to your > > > package. In that case you should preserve them as separate patches. > > > > > > > For these series patches submitted by Martin, only this one gets > > > > approval at this time. Others need more discussion. > > > > > > Yes. Normally I would think Olivier would get his whole series > > > reviewed and ready before committing it. Maybe it is okay to split > it > > > in some cases. > > > > > > -Jordan > > > > > > > -----Original Message----- > > > > From: Justen, Jordan L > > > > Sent: Thursday, November 13, 2014 09:25 > > > > To: Tian, Feng; 'Olivier Martin (olivier.mar...@arm.com)' > > > > Cc: edk2-devel@lists.sourceforge.net > > > > Subject: Re: [edk2] patch review feedback about > FvSimpleFileSystem > > > module > > > > > > > > On 2014-11-11 23:16:21, Tian, Feng wrote: > > > > > Hi, Martin > > > > > > > > > > Here is my review comments on your FvSimpleFileSystem uefi > driver. > > > Sorry for late response. > > > > > > > > > > 1. Fixed system hang due to wrong assertions in > > > FvSimpleFileSystemDriverStart() and FvSimpleFileSystemOpenVolume(). > > > > > a. change ASSERT (NumChars == GUID_STRING_SIZE); to > ASSERT > > > ((NumChars + 1) * sizeof (CHAR16) == GUID_STRING_SIZE); > > > > > b. change ASSERT (NumChars == FVFS_VOLUME_LABEL_SIZE); > to > > > > > ASSERT ((NumChars + 1) * sizeof (CHAR16) == > > > FVFS_VOLUME_LABEL_SIZE); 2. Fixed SimpleFileSystem SCT errors. > > > > > a. EFI_FILE_PROTOCOL.GetInfo() should handle > VolumeLabel > > > info rather than return EFI_UNSUPPORTED. > > > > > b. EFI_FILE_PROTOCOL.GetInfo() should initial Size > field > > > of EFI_FILE_SYSTEM_INFO at run time. > > > > > c. Some interfaces in EFI_FILE_PROTOCOL should return > > > EFI_WRITE_PROTECTED rather than EFI_UNSUPPORTED. > > > > > d. EFI_FILE_PROTOCOL.Delete() should return > > > EFI_WARN_DELETE_FAILURE rather than EFI_UNSUPPORTED. > > > > > 3. Added missing ComponentName protocol interface and modify > code > > > to use APIs defined in UefiLib to install DriverBinding and > > > ComponentName protocol. > > > > > 4. Fixed wrong UnicodeCollation protocol usage. > > > > > 5. Added missing function comments. > > > > > 6. Added missing user extension meta data in INF file. > > > > > 7. Remove BasePathLib dependency and use internal > implementation > > > as the move operation for this library and others in ShellPkg need > > > more discussions. > > > > > > > > > > I attached the modified path for your review, will check it in > to > > > MdeModulePkg/Universal after pass your review. BasePathLib will be > on > > > other thread. > > > > > > > > I wonder what Olivier is supposed to do with this patch. I notice > it > > > lacks a Contributed-under and Signed-off-by for your changes. > > > > > > > > I assume you'll add that, and then Olivier will add it to the > commit > > > messages? > > > > > > > > Another (I think easier) approach would be to provide feedback > and > > > let Olivier modify his code based on your suggestions. > > > > > > > > You might also give your r-b and let Olivier commit the patch > series > > > since he has commit access. (It seems he had a 3 patch series > > > > originally.) > > > > > > > > -Jordan > > > > > > > > > > > > > > --------------------------------------------------------------------- > --------- > > Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server > > from Actuate! Instantly Supercharge Your Business Reports and > Dashboards > > with Interactivity, Sharing, Native Excel Exports, App Integration & > more > > Get technology previously reserved for billion-dollar corporations, > FREE > > > http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.c > lktrk > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > CAUTION: Please confirm that the password protected > > .zip attachment which contains the file(s) of type > > FvSimpleFileSystemDxe.zip > > is legitimate prior to opening. To make sure this > > message is not infected with a virus, it is important to > > verify that you are expecting the message or else > > confirm its legitimacy with the sender. > > ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
