| Message ID | 1450251988-9246-2-git-send-email-hariprasad@chelsio.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Hariprasad Shenai <hariprasad@chelsio.com> Date: Wed, 16 Dec 2015 13:16:25 +0530 > @@ -66,7 +66,7 @@ struct l2t_data { > > static inline unsigned int vlan_prio(const struct l2t_entry *e) > { > - return e->vlan >> 13; > + return (e->vlan & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT; e->vlan is a u16, the vlan priotity is the top 3 bits of the 16-bit value, and finally the right shift will be non-signed. Therefore this change is absolutely not necessary. Please remove this patch from the series and resend. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Dec 16, 2015, at 4:07 PM, David Miller <davem@davemloft.net> wrote: > > From: Hariprasad Shenai <hariprasad@chelsio.com> > Date: Wed, 16 Dec 2015 13:16:25 +0530 > >> @@ -66,7 +66,7 @@ struct l2t_data { >> >> static inline unsigned int vlan_prio(const struct l2t_entry *e) >> { >> - return e->vlan >> 13; >> + return (e->vlan & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT; > > e->vlan is a u16, the vlan priotity is the top 3 bits of the 16-bit > value, and finally the right shift will be non-signed. > > Therefore this change is absolutely not necessary. > > Please remove this patch from the series and resend. I assume that you only meant that the masking portion is unnecessary. Doing the shift with the symbolic constant VLAN_PRIO_SHIFT instead of the literal constant “13” is still a reasonable change. The masking was almost certainly from me because once one uses the symbolic constants, weren’t not supposed to “know” about the internal structure of the operation. Modern compilers are of course free to optimize away the mask, etc. Casey-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Casey Leedom <leedom@chelsio.com> Date: Wed, 16 Dec 2015 17:40:41 -0800 > >> On Dec 16, 2015, at 4:07 PM, David Miller <davem@davemloft.net> wrote: >> >> From: Hariprasad Shenai <hariprasad@chelsio.com> >> Date: Wed, 16 Dec 2015 13:16:25 +0530 >> >>> @@ -66,7 +66,7 @@ struct l2t_data { >>> >>> static inline unsigned int vlan_prio(const struct l2t_entry *e) >>> { >>> - return e->vlan >> 13; >>> + return (e->vlan & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT; >> >> e->vlan is a u16, the vlan priotity is the top 3 bits of the 16-bit >> value, and finally the right shift will be non-signed. >> >> Therefore this change is absolutely not necessary. >> >> Please remove this patch from the series and resend. > > I assume that you only meant that the masking portion is > unnecessary. Doing the shift with the symbolic constant > VLAN_PRIO_SHIFT instead of the literal constant “13” is still a > reasonable change. The masking was almost certainly from me because > once one uses the symbolic constants, weren’t not supposed to “know” > about the internal structure of the operation. Modern compilers are > of course free to optimize away the mask, etc. Yes I'm only objecting to the unnecessary mask operation. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/chelsio/cxgb4/l2t.c b/drivers/net/ethernet/chelsio/cxgb4/l2t.c index ac27898..aa5fbad 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/l2t.c +++ b/drivers/net/ethernet/chelsio/cxgb4/l2t.c @@ -66,7 +66,7 @@ struct l2t_data { static inline unsigned int vlan_prio(const struct l2t_entry *e) { - return e->vlan >> 13; + return (e->vlan & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT; } static inline void l2t_hold(struct l2t_data *d, struct l2t_entry *e)
Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com> --- drivers/net/ethernet/chelsio/cxgb4/l2t.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)