[gPXE-devel] [PATCH] 802.1Q support for gpxe (was: Re: 802.1Q support for gpxe)

[Michael Brown]

On Wednesday 10 Mar 2010 14:19:59 michael-dev at fami-braun.de wrote:
> I've reworked the patch to be fully optional and less invasive.
> 
> The receiving path is from the raw device via 802.1Q network protocol
> into net_rx. The sending path is via linklayer of the proxy device
> which adds 802.1Q header and the forwards the iobuf to the link layer
> of the raw device. The transmit as well as all other commands to the
> proxy device (except pull) are forwarded to the raw device.
> 
> Further, there is a vconfig command to add/remove vlan interfaces just
> like linux does. In order to have network device names like net0.VID,
> I increased the len of the netdevice name to 10 characters and let the
> driver decide upon the name.
> In order to achieve autobooting from the vlan device without trying to
> boot from the raw device first, I optionally added a netboot command.
> The code uses netdev_put and netdev_get commands to lock the raw device.
> 
> The most tricky part is the irq enable/disable code as well as the
> open/close code, the patch just forwards these commands which just works
> though there should be open/close counters.

I like the look of this approach much more than the previous patch.

A few (non-exhaustive) comments from a quick scan:

char name[] in struct net_device may as well be 12 rather than 10, since it's 
going to get dword-aligned anyway.

Why is ieee8021q_transmit() creating a new iobuf?

Device open counts should be implemented in net/netdevice.c within 
netdev_open() and netdev_close(); you can use ib_open() and ib_close() in 
net/infiniband.c as a reference.  The NETDEV_OPEN flag must be removed and 
replaced with something like a static inline netdev_is_open() which just 
checks the netdev's open count.  This change should probably be in its own 
separate commit.

I would try to avoid calling fetch_uintz_setting() for each transmitted 
packet.  Better to hold the VLAN tag in a structure member.  For example:

struct vlan_device {
  /* Underlying net device */
  struct net_device *netdev;
  /* VLAN tag */
  uint32_t tag;
};

with the struct vlan_device forming the private data of the net_device you 
create.

Lastly, is 802.1Q Ethernet-specific?  In that case, you may as well just treat 
it as its own static struct ll_protocol, rather than trying to dynamically 
create a ll_protocol based on the underlying net_device.  (gPXE isn't really 
set up to handle a struct ll_protocol that might be freed; they're assumed to 
be static and permanent.)

Michael