[ipxe-devel] [PATCH 0/2] [vlan] Support 802.1Q VLAN 0 priority tagging

[Ladi Prosek]

Thank you for the quick response.

On Fri, Apr 15, 2016 at 7:03 PM, Michael Brown <mcb30 at ipxe.org> wrote:
> On 15/04/16 17:48, Michael Brown wrote:
>>
>> On 15/04/16 17:19, Ladi Prosek wrote:
>>>
>>> These patches add a small tweak to vlan_rx to make it accept
>>> priority tagged packets. Since this should be supported even
>>> without full VLAN support,
>>
>>
>> Why must this be supported when VLAN is not enabled as a feature?

I wouldn't say it must be supported but one could argue that
conceptually this is not really related to VLANs. It reuses the same
header yes, but networks with this kind of traffic don't necessarily
need to have any real VLANs configured.

Weighing the amount of added code to all binaries built without
VLAN_CMD against the chances that stuff will just work for people out
of the box (as opposed to scratching their head wondering why they
need to enable *_CMD without actually having to use any commands), I'm
leaning towards the it just works option.

> I have a preference for a simpler patch such as:
>
> diff --git a/src/net/vlan.c b/src/net/vlan.c
> index f515c2d..a30a0d7 100644
> --- a/src/net/vlan.c
> +++ b/src/net/vlan.c
> @@ -203,6 +203,11 @@ struct net_device * vlan_find ( struct net_device
> *trunk, unsigned int tag ) {
>         struct net_device *netdev;
>         struct vlan_device *vlan;
>
> +       /* VLAN 0 represents a priority-tagged packet on the trunk device */
> +       if ( ! tag )
> +               return trunk;
> +
> +       /* Find VLAN device */
>         for_each_netdev ( netdev ) {
>                 if ( netdev->op != &vlan_operations )
>                         continue;

Is it desirable to affect other callers of vlan_find? Genuine
question, I'm not suggesting it's not. Should hermon_eth_complete_recv
work for source->vlan == 0?

> @@ -340,6 +346,12 @@ int vlan_create ( struct net_device *trunk, unsigned
> int tag,
>         struct vlan_device *vlan;
>         int rc;
>
> +       /* VLAN 0 is not permitted */
> +       if ( ! tag ) {
> +               DBGC ( trunk, "VLAN %s cannot create VLAN 0\n", trunk->name
> );
> +               return -ENOTTY;
> +       }
> +
>         /* If VLAN already exists, just update the priority */
>         if ( ( netdev = vlan_find ( trunk, tag ) ) != NULL ) {
>                 vlan = netdev->priv;
>
>
> but I'm prepared to be swayed if there are good reasons otherwise.
>
> Michael

Thanks!
Ladi