Re: svn commit: r305177 - head/sys/net

[Sepherosa Ziehau]

Oh, my fault.  I will revert it.

On Thu, May 18, 2017 at 11:11 PM, Hans Petter Selasky  wrote:
> On 05/18/17 17:00, Oleg Bulyzhin wrote:
>>
>> On Thu, May 18, 2017 at 04:25:01PM +0200, Hans Petter Selasky wrote:
>>>
>>> On 05/18/17 16:04, Oleg Bulyzhin wrote:

 On Thu, Sep 01, 2016 at 06:32:35AM +, Sepherosa Ziehau wrote:
>
> Author: sephe
> Date: Thu Sep  1 06:32:35 2016
> New Revision: 305177
> URL: https://svnweb.freebsd.org/changeset/base/305177
>
> Log:
> net/vlan: Shift for pri is 13 (pri mask 0xe000) not 1.
> Reviewed by:araujo, hps
> MFC after:  1 week
> Sponsored by:   Microsoft
> Differential Revision:  https://reviews.freebsd.org/D7710
>
> Modified:
> head/sys/net/ethernet.h
>
> Modified: head/sys/net/ethernet.h
>
> ==
> --- head/sys/net/ethernet.h Thu Sep  1 06:05:08 2016
> (r305176)
> +++ head/sys/net/ethernet.h Thu Sep  1 06:32:35 2016
> (r305177)
> @@ -92,7 +92,7 @@ struct ether_vlan_header {
>#define  EVL_PRIOFTAG(tag)   (((tag) >> 13) & 7)
>#define  EVL_CFIOFTAG(tag)   (((tag) >> 12) & 1)
>#define  EVL_MAKETAG(vlid, pri, cfi)
> \
> -   ((pri) & 7) << 1) | ((cfi) & 1)) << 12) | ((vlid) &
> EVL_VLID_MASK))
> +   ((pri) & 7) << 13) | ((cfi) & 1)) << 12) | ((vlid) &
> EVL_VLID_MASK))
>   /*
> *  NOTE: 0x-0x05DC (0..1500) are generally IEEE 802.3 length
> fields.


 Please revert this one. It's just plain wrong and previous one was ok.

>>>
>>> Hi,
>>>
>>> Can you explain a bit more what is wrong?
>>>
 If you care about readability it should be:
 pri) & 7) << 13) | (((cfi) & 1) << 12) | ((vlid) & EVL_VLID_MASK))
>>>
>>>
>>> Isn't this exactly what the patch is doing? -R ???
>>
>>
>> Current version is shifting pri out of uint16. If you examine parentheses:
>> pri is shifted left 13, then 12.
>> Original version did it right (shift 1, then 12 (total 13)).
>>
>
> Hi Oleg,
>
> I see. The VLAN priority is then always zero after this change.
>
> I'll let Sepherosa handle the revert and/or readability update.
>
> --HPS



-- 
Tomorrow Will Never Die
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r305177 - head/sys/net

[Hans Petter Selasky]

On 05/18/17 17:00, Oleg Bulyzhin wrote:

On Thu, May 18, 2017 at 04:25:01PM +0200, Hans Petter Selasky wrote:

On 05/18/17 16:04, Oleg Bulyzhin wrote:

On Thu, Sep 01, 2016 at 06:32:35AM +, Sepherosa Ziehau wrote:

Author: sephe
Date: Thu Sep  1 06:32:35 2016
New Revision: 305177
URL: https://svnweb.freebsd.org/changeset/base/305177

Log:
net/vlan: Shift for pri is 13 (pri mask 0xe000) not 1.

Reviewed by:	araujo, hps

MFC after:  1 week
Sponsored by:   Microsoft
Differential Revision:  https://reviews.freebsd.org/D7710

Modified:
head/sys/net/ethernet.h

Modified: head/sys/net/ethernet.h
==
--- head/sys/net/ethernet.h Thu Sep  1 06:05:08 2016(r305176)
+++ head/sys/net/ethernet.h Thu Sep  1 06:32:35 2016(r305177)
@@ -92,7 +92,7 @@ struct ether_vlan_header {
   #define  EVL_PRIOFTAG(tag)   (((tag) >> 13) & 7)
   #define  EVL_CFIOFTAG(tag)   (((tag) >> 12) & 1)
   #define  EVL_MAKETAG(vlid, pri, cfi) 
\
-   ((pri) & 7) << 1) | ((cfi) & 1)) << 12) | ((vlid) & EVL_VLID_MASK))
+   ((pri) & 7) << 13) | ((cfi) & 1)) << 12) | ((vlid) & EVL_VLID_MASK))
   
   /*

*  NOTE: 0x-0x05DC (0..1500) are generally IEEE 802.3 length fields.


Please revert this one. It's just plain wrong and previous one was ok.



Hi,

Can you explain a bit more what is wrong?


If you care about readability it should be:
pri) & 7) << 13) | (((cfi) & 1) << 12) | ((vlid) & EVL_VLID_MASK))


Isn't this exactly what the patch is doing? -R ???


Current version is shifting pri out of uint16. If you examine parentheses:
pri is shifted left 13, then 12.
Original version did it right (shift 1, then 12 (total 13)).



Hi Oleg,

I see. The VLAN priority is then always zero after this change.

I'll let Sepherosa handle the revert and/or readability update.

--HPS
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r305177 - head/sys/net

[Benjamin Kaduk]

On Thu, May 18, 2017 at 10:00 AM, Oleg Bulyzhin  wrote:

> On Thu, May 18, 2017 at 04:25:01PM +0200, Hans Petter Selasky wrote:
> >
> > Can you explain a bit more what is wrong?
> >
> > > If you care about readability it should be:
> > > pri) & 7) << 13) | (((cfi) & 1) << 12) | ((vlid) & EVL_VLID_MASK))
> >
> > Isn't this exactly what the patch is doing? -R ???
>
> Current version is shifting pri out of uint16. If you examine parentheses:
> pri is shifted left 13, then 12.
> Original version did it right (shift 1, then 12 (total 13)).
>

Calculations in the C abstract machine would be performed in the 'int' type
to which uint16 operands are promoted, though.

-Ben
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r305177 - head/sys/net

[Oleg Bulyzhin]

On Thu, May 18, 2017 at 04:25:01PM +0200, Hans Petter Selasky wrote:
> On 05/18/17 16:04, Oleg Bulyzhin wrote:
> > On Thu, Sep 01, 2016 at 06:32:35AM +, Sepherosa Ziehau wrote:
> >> Author: sephe
> >> Date: Thu Sep  1 06:32:35 2016
> >> New Revision: 305177
> >> URL: https://svnweb.freebsd.org/changeset/base/305177
> >>
> >> Log:
> >>net/vlan: Shift for pri is 13 (pri mask 0xe000) not 1.
> >>
> >>Reviewed by:araujo, hps
> >>MFC after:  1 week
> >>Sponsored by:   Microsoft
> >>Differential Revision:  https://reviews.freebsd.org/D7710
> >>
> >> Modified:
> >>head/sys/net/ethernet.h
> >>
> >> Modified: head/sys/net/ethernet.h
> >> ==
> >> --- head/sys/net/ethernet.hThu Sep  1 06:05:08 2016
> >> (r305176)
> >> +++ head/sys/net/ethernet.hThu Sep  1 06:32:35 2016
> >> (r305177)
> >> @@ -92,7 +92,7 @@ struct ether_vlan_header {
> >>   #define  EVL_PRIOFTAG(tag)   (((tag) >> 13) & 7)
> >>   #define  EVL_CFIOFTAG(tag)   (((tag) >> 12) & 1)
> >>   #define  EVL_MAKETAG(vlid, pri, cfi) 
> >> \
> >> -  ((pri) & 7) << 1) | ((cfi) & 1)) << 12) | ((vlid) & EVL_VLID_MASK))
> >> +  ((pri) & 7) << 13) | ((cfi) & 1)) << 12) | ((vlid) & EVL_VLID_MASK))
> >>   
> >>   /*
> >>*  NOTE: 0x-0x05DC (0..1500) are generally IEEE 802.3 length fields.
> > 
> > Please revert this one. It's just plain wrong and previous one was ok.
> > 
> 
> Hi,
> 
> Can you explain a bit more what is wrong?
> 
> > If you care about readability it should be:
> > pri) & 7) << 13) | (((cfi) & 1) << 12) | ((vlid) & EVL_VLID_MASK))
> 
> Isn't this exactly what the patch is doing? -R ???

Current version is shifting pri out of uint16. If you examine parentheses:
pri is shifted left 13, then 12.
Original version did it right (shift 1, then 12 (total 13)).


-- 
Oleg.


=== Oleg Bulyzhin -- OBUL-RIPN -- OBUL-RIPE -- o...@rinet.ru ===


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r305177 - head/sys/net

[Hans Petter Selasky]

On 05/18/17 16:04, Oleg Bulyzhin wrote:

On Thu, Sep 01, 2016 at 06:32:35AM +, Sepherosa Ziehau wrote:

Author: sephe
Date: Thu Sep  1 06:32:35 2016
New Revision: 305177
URL: https://svnweb.freebsd.org/changeset/base/305177

Log:
   net/vlan: Shift for pri is 13 (pri mask 0xe000) not 1.
   
   Reviewed by:	araujo, hps

   MFC after:   1 week
   Sponsored by:Microsoft
   Differential Revision:   https://reviews.freebsd.org/D7710

Modified:
   head/sys/net/ethernet.h

Modified: head/sys/net/ethernet.h
==
--- head/sys/net/ethernet.h Thu Sep  1 06:05:08 2016(r305176)
+++ head/sys/net/ethernet.h Thu Sep  1 06:32:35 2016(r305177)
@@ -92,7 +92,7 @@ struct ether_vlan_header {
  #define   EVL_PRIOFTAG(tag)   (((tag) >> 13) & 7)
  #define   EVL_CFIOFTAG(tag)   (((tag) >> 12) & 1)
  #define   EVL_MAKETAG(vlid, pri, cfi) 
\
-   ((pri) & 7) << 1) | ((cfi) & 1)) << 12) | ((vlid) & EVL_VLID_MASK))
+   ((pri) & 7) << 13) | ((cfi) & 1)) << 12) | ((vlid) & EVL_VLID_MASK))
  
  /*

   *  NOTE: 0x-0x05DC (0..1500) are generally IEEE 802.3 length fields.


Please revert this one. It's just plain wrong and previous one was ok.



Hi,

Can you explain a bit more what is wrong?


If you care about readability it should be:
pri) & 7) << 13) | (((cfi) & 1) << 12) | ((vlid) & EVL_VLID_MASK))


Isn't this exactly what the patch is doing? -R ???

--HPS
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r305177 - head/sys/net

[Oleg Bulyzhin]

On Thu, Sep 01, 2016 at 06:32:35AM +, Sepherosa Ziehau wrote:
> Author: sephe
> Date: Thu Sep  1 06:32:35 2016
> New Revision: 305177
> URL: https://svnweb.freebsd.org/changeset/base/305177
> 
> Log:
>   net/vlan: Shift for pri is 13 (pri mask 0xe000) not 1.
>   
>   Reviewed by:araujo, hps
>   MFC after:  1 week
>   Sponsored by:   Microsoft
>   Differential Revision:  https://reviews.freebsd.org/D7710
> 
> Modified:
>   head/sys/net/ethernet.h
> 
> Modified: head/sys/net/ethernet.h
> ==
> --- head/sys/net/ethernet.h   Thu Sep  1 06:05:08 2016(r305176)
> +++ head/sys/net/ethernet.h   Thu Sep  1 06:32:35 2016(r305177)
> @@ -92,7 +92,7 @@ struct ether_vlan_header {
>  #define  EVL_PRIOFTAG(tag)   (((tag) >> 13) & 7)
>  #define  EVL_CFIOFTAG(tag)   (((tag) >> 12) & 1)
>  #define  EVL_MAKETAG(vlid, pri, cfi) 
> \
> - ((pri) & 7) << 1) | ((cfi) & 1)) << 12) | ((vlid) & EVL_VLID_MASK))
> + ((pri) & 7) << 13) | ((cfi) & 1)) << 12) | ((vlid) & EVL_VLID_MASK))
>  
>  /*
>   *  NOTE: 0x-0x05DC (0..1500) are generally IEEE 802.3 length fields.

Please revert this one. It's just plain wrong and previous one was ok.

If you care about readability it should be: 
pri) & 7) << 13) | (((cfi) & 1) << 12) | ((vlid) & EVL_VLID_MASK))

-- 
Oleg.


=== Oleg Bulyzhin -- OBUL-RIPN -- OBUL-RIPE -- o...@rinet.ru ===


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"