Thanks for taking a look at this. Comments are inline, and a new patch is attached. Note that this new patch now depends on the EDK headers that I submitted to this list previously.
> -----Original Message----- > From: Michael Brown [mailto:[email protected]] > Sent: Friday, May 28, 2010 6:09 AM > To: [email protected] > Cc: Geoff Lywood > Subject: Re: [gPXE-devel] [PATCH] EFI Simple Network Protocol driver > > On Friday 28 May 2010 05:08:42 Geoff Lywood wrote: > > This patch contains a gPXE network driver that can call into the EFI > > firmware's Simple Network Protocol (SNP) as a means to send and receive > > packets. It is essentially the EFI equivalent of the "undionly" driver. > > > > I have tested this patch on an Apple Xserve (64-bit EFI), and a larger > > patch containing this code as well as some other improvements on both > an > > IBM x3550 (64-bit EFI) and an Apple Mac Mini (32-bit EFI). > > > > I know that this one is a little bit bigger than the other patches I've > > sent, and I'm not 100% certain of coding style and commenting > guidelines, > > but I tried my best. Feedback is certainly welcome. > > Very nice work! > > A few minor comments: > > 1. No need for __attribute__((packed)) on struct snp_device; it doesn't > represent a fixed memory layout requirement. Agreed. I don't know why I wrote that in the first place. > > 2. SNP TX completions are potentially unsafe; see the comments in > efi_snp.c > about the interesting design choices in the SNP TX completion reporting > mechanism. As far as I am aware, you are essentially limited to having > only > one outstanding TX buffer at any one time, otherwise you hit undefined > behaviour. All of the EFI specifications that I can find, from 1.02 to 2.3, say that GetStatus returns a single TX buffer each time it is called, and the implementation is free to queue TX buffers if it pleases. I think that the code in efi_snp.c is actually incorrect in this case. > > 3. snpnet_open() may as well attempt to set the MAC address to netdev- > >ll_addr. Sure. Fixed. > > 4. snpbus_remove() should include list_del ( &snponly_dev.dev.siblings ); Fixed. > > 5. Where possible, it may be desirable to use DBGC() rather than plain > DBG(); > this helps to visually separate messages when multiple debug options (or > multiple devices) are present. Typical pattern is something like > > DBGC ( snp, "SNP %p did something ...", snp, ... ); > > i.e. using some available common value ("snp" in this case) as the > colouriser, > and standardising the message format to always begin with "SNP %p". I changed everything in snpnet.c to follow this pattern. I also made these error messages report the efi_strerror when applicable. > > 6. Are the updated EFI headers imported using import.pl? import.pl will > import the required headers from an EDK tree; the idea was that the EFI > includes directory should always represent a subset of a single snapshot > from > the EDK tree, to reduce the maintenance requirements. (If you need a > current > EDK tree, you can git clone git://git.ipxe.org/mirror/efi/edk2/.git; don't > use > the mirror on git.etherboot.org, which is out of date and should be > removed.) Originally I just copied the header directly from the edk2 tree. The updated patch uses the results of import.pl. > > 7. You have a call to OpenProtocol() for EfiLoadedImageProtocol which > isn't > balanced by any call to CloseProtocol. More generally, would it be > possible > to use either only OpenProtocol or only LocateProtocol in order to obtain > all > the required protocols? Since I use EFI_OPEN_PROTOCOL_GET_PROTOCOL, I don't have to match it with a CloseProtocol. It says so in the spec. I think it is safe to assume that the current image will not go away while it is executing. The problem with LocateProtocol is that it will give you any instance of the protocol, not necessarily the one associated with the handle that you care about. In this particular case, I want the loaded image protocol associated with my own image, not just any image. In fact, I'm not convinced that the current uses of LocateProtocol are actually correct. For example, a system with multiple PCI root bridges will have multiple PCI root bridge I/O protocols, and there is nothing in the gPXE code to select the correct one. Thanks again for taking the time to look at this, Geoff VMware, Inc. is providing this patch to you under the terms of the GPL version 2 and any later version. This patch is provided as is, with no warranties or support. VMware disclaims all liability in connection with the use/inability to use this patch. Any use of the attached is considered acceptance of the above.
0001-Add-a-new-network-driver-that-consumes-the-EFI-Simpl.patch
Description: 0001-Add-a-new-network-driver-that-consumes-the-EFI-Simpl.patch
_______________________________________________ gPXE-devel mailing list [email protected] http://etherboot.org/mailman/listinfo/gpxe-devel
