| Message ID | 20200926193215.1405730-7-vladimir.oltean@nxp.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | Generic adjustment for flow dissector in DSA | expand |
On Sat, Sep 26, 2020 at 10:32:06PM +0300, Vladimir Oltean wrote: > For all DSA formats that don't use tail tags, it looks like behind the > obscure number crunching they're all doing the same thing: locating the > real EtherType behind the DSA tag. Nonetheless, this is not immediately > obvious, so create a generic helper for those DSA taggers that put the > header before the EtherType. > > Another assumption for the generic function is that the DSA tags are of > equal length on RX and on TX. Prior to the previous patch, this was not > true for ocelot and for gswip. The problem was resolved for ocelot, but > for gswip it still remains, so that can't use this helper yet. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> +static inline void dsa_tag_generic_flow_dissect(const struct sk_buff *skb, > + __be16 *proto, int *offset) > +{ > +#if IS_ENABLED(CONFIG_NET_DSA) > + const struct dsa_device_ops *ops = skb->dev->dsa_ptr->tag_ops; > + int tag_len = ops->overhead; > + > + *offset = tag_len; > + *proto = ((__be16 *)skb->data)[(tag_len / 2) - 1]; > +#endif > +} > + Do you actually need the IS_ENABLED()? There is only one caller of this function, and it is already protected by IS_ENABLED(CONFIG_NET_DSA). So i don't think it adds anything. Andrew
On Sat, Sep 26, 2020 at 10:33:00PM +0200, Andrew Lunn wrote: > > +static inline void dsa_tag_generic_flow_dissect(const struct sk_buff *skb, > > + __be16 *proto, int *offset) > > +{ > > +#if IS_ENABLED(CONFIG_NET_DSA) > > + const struct dsa_device_ops *ops = skb->dev->dsa_ptr->tag_ops; > > + int tag_len = ops->overhead; > > + > > + *offset = tag_len; > > + *proto = ((__be16 *)skb->data)[(tag_len / 2) - 1]; > > +#endif > > +} > > + > > Do you actually need the IS_ENABLED()? There is only one caller of > this function, and it is already protected by > IS_ENABLED(CONFIG_NET_DSA). So i don't think it adds anything. It doesn't matter how many callers it has, it doesn't compile when NET_DSA=n: ./include/net/dsa.h: In function ‘dsa_tag_generic_flow_dissect’: ./include/net/dsa.h:732:47: error: ‘struct net_device’ has no member named ‘dsa_ptr’; did you mean ‘ip_ptr’? const struct dsa_device_ops *ops = skb->dev->dsa_ptr->tag_ops; ^~~~~~~ ip_ptr
diff --git a/include/net/dsa.h b/include/net/dsa.h index 98d339311898..817fab5e2c21 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -711,6 +711,32 @@ static inline bool dsa_can_decode(const struct sk_buff *skb, return false; } +/* All DSA tags that push the EtherType to the right (basically all except tail + * tags, which don't break dissection) can be treated the same from the + * perspective of the flow dissector. + * + * We need to return: + * - offset: the (B - A) difference between: + * A. the position of the real EtherType and + * B. the current skb->data (aka ETH_HLEN bytes into the frame, aka 2 bytes + * after the normal EtherType was supposed to be) + * The offset in bytes is exactly equal to the tagger overhead (and half of + * that, in __be16 shorts). + * + * - proto: the value of the real EtherType. + */ +static inline void dsa_tag_generic_flow_dissect(const struct sk_buff *skb, + __be16 *proto, int *offset) +{ +#if IS_ENABLED(CONFIG_NET_DSA) + const struct dsa_device_ops *ops = skb->dev->dsa_ptr->tag_ops; + int tag_len = ops->overhead; + + *offset = tag_len; + *proto = ((__be16 *)skb->data)[(tag_len / 2) - 1]; +#endif +} + #if IS_ENABLED(CONFIG_NET_DSA) static inline int __dsa_netdevice_ops_check(struct net_device *dev) {
For all DSA formats that don't use tail tags, it looks like behind the obscure number crunching they're all doing the same thing: locating the real EtherType behind the DSA tag. Nonetheless, this is not immediately obvious, so create a generic helper for those DSA taggers that put the header before the EtherType. Another assumption for the generic function is that the DSA tags are of equal length on RX and on TX. Prior to the previous patch, this was not true for ocelot and for gswip. The problem was resolved for ocelot, but for gswip it still remains, so that can't use this helper yet. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- Changes in v3: Made this a static inline function callable from the outside world. include/net/dsa.h | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)