Re: Request a freeze exception for vlantag feature

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 09.01.2014 17:58, Paulo Flabiano Smorigo wrote:
> Thu, Jan 09, 2014 at 07:05:16AM +0400, Andrey Borzenkov wrote:
>> В Wed, 8 Jan 2014 16:57:28 -0200
>> Paulo Flabiano Smorigo <address@hidden> пишет:
>>
>>> +
>>> +      inter->vlantag.pcp = vlantag >> 12;
>>> +      inter->vlantag.dei = (vlantag >> 11) & 0x1;
>>> +      inter->vlantag.vid = vlantag & 0x1fff;
>>
>> That's 13 bits, not 12, right? And this really looks like
>> overengeneering - do you really want to be able to set static VLAN
>> priority bits? I do not think it belongs to grub.
>>
>>> +
>>> +  if (grub_strcmp (args[3], "vlan") == 0)
>>> +    vlan_pos = 3;
>>> +
>>> +  if (grub_strcmp (args[4], "vlan") == 0)
>>> +    vlan_pos = 4;
>>
> 
> You're right, thanks. I fixed:
> 
> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c 
> b/grub-core/net/drivers/ieee1275/ofnet.c
> index ffcb943..72359c3 100644
> --- a/grub-core/net/drivers/ieee1275/ofnet.c
> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
> @@ -213,13 +213,12 @@ grub_ieee1275_parse_bootargs (const char *devpath, char 
> *bootpath,
>        inter = grub_net_add_addr ((*card)->name, *card, &client_addr, 
> &hw_addr,
>                                   flags);
>  
> -      inter->vlantag.pcp = vlantag >> 12;
> -      inter->vlantag.dei = (vlantag >> 11) & 0x1;
> -      inter->vlantag.vid = vlantag & 0x1fff;
> +      inter->vlantag.pcp = vlantag >> 13;
> +      inter->vlantag.dei = (vlantag >> 12) & 0x1;
> +      inter->vlantag.vid = vlantag & 0xfff;
>  
>        grub_net_add_ipv4_local (inter,
>                            __builtin_ctz (~grub_le_to_cpu32 
> (subnet_mask.ipv4)));
> -
>      }
>  
>    if (gateway_addr.ipv4 != 0)
> diff --git a/grub-core/net/ethernet.c b/grub-core/net/ethernet.c
> index ae195bc..7ca14e9 100644
> --- a/grub-core/net/ethernet.c
> +++ b/grub-core/net/ethernet.c
> @@ -88,8 +88,8 @@ send_ethernet_packet (struct 
> grub_net_network_level_interface *inf,
>    if (inf->vlantag.vid != 0)
>      {
>        grub_uint32_t vlantag;
> -      vlantag = (VLANTAG_IDENTIFIER << 16) + (inf->vlantag.pcp << 12) +
> -                (inf->vlantag.dei << 11) + inf->vlantag.vid;
> +      vlantag = (VLANTAG_IDENTIFIER << 16) | (inf->vlantag.pcp << 13) |
> +                (inf->vlantag.dei << 12) | inf->vlantag.vid;
>  
>        /* Move eth type to the right */
>        grub_memcpy ((char *) nb->data + etherhdr_size - 2,
> 
> 
> Virtual lan priority is an option in PowerPC SMS:
> 
>  PowerPC Firmware
>  Version ZM770_024
>  SMS 1.7 (c) Copyright IBM Corp. 2000,2008 All rights reserved.
> -------------------------------------------------------------------------------
>  Advanced Setup: BOOTP
> Interpartition Logical LAN: U9109.RMD.10F037P-V4-C3-T1
> 
>  1.   Bootp Retries    5
>  2.   Bootp Blocksize  512
>  3.   TFTP  Retries    5
>  4.   VLAN  Priority   0
>  5.   VLAN  ID         0 (default - not configured)
> 
> 
> Maybe we can use the priority and DEI of incoming package as the values for 
> the
> following packages. What do you think?
> 
What's the advantage to setting it at all? We don't do any QOS.
>> May be it should really start using proper options at this point
>> keeping existing three argument form as legacy.
>>
>> net_add_addr --if=... --addr=... --mask=... --vlan=... --hw=... card
> 
> I prefer the current format but we can switch to another if it's more 
> suitable.
> 
We can go eiher with options or syntax like [vlan <num>]. The only realy
requirement is extendability, we should be able to add new options
easily. The big advantage of option is code reuse from extcmd which
ensures that it's similar handling with other options and less risk of bugs.
> Andrey, ping me in IRC so we can talk about it.
> 
>>
>> _______________________________________________
>> Grub-devel mailing list
>> address@hidden
>> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> I ask this patch as a freeze exception. This feature will only add a option 
> for
> vlan tag and will not change the default grub workflow. Can I push the current
> version in master so it can be included in 2.02? Anyone against?
> 
> Thanks!
> 
> --
> Paulo Flabiano Smorigo
> IBM Linux Technology Center
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

signature.asc
Description: OpenPGP digital signature