[gPXE-devel] [PATCH] EFI Simple Network Protocol driver

[Michael Brown]

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.

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.

3. snpnet_open() may as well attempt to set the MAC address to netdev-
>ll_addr.

4. snpbus_remove() should include list_del ( &snponly_dev.dev.siblings );

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

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

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?

Michael