* [PATCH net-next v2 1/6] net: dsa: comment hot path requirements
2017-05-30 18:33 [PATCH net-next v2 0/6] net: dsa: tagger simplification Vivien Didelot
@ 2017-05-30 18:33 ` Vivien Didelot
2017-05-30 20:34 ` Florian Fainelli
2017-05-30 18:33 ` [PATCH net-next v2 2/6] net: dsa: do not cast dst Vivien Didelot
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Vivien Didelot @ 2017-05-30 18:33 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
The DSA layer uses inline helpers and copies of the tagging functions
for faster access in hot path. Add comments to detail that.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
include/net/dsa.h | 3 +++
net/dsa/dsa_priv.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index c0e567c0c824..58297e4c6b31 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -126,6 +126,8 @@ struct dsa_switch_tree {
* protocol to use.
*/
struct net_device *master_netdev;
+
+ /* Copy of tag_ops->rcv for faster access in hot path */
struct sk_buff * (*rcv)(struct sk_buff *skb,
struct net_device *dev,
struct packet_type *pt,
@@ -464,6 +466,7 @@ struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
struct net_device *dsa_dev_to_net_device(struct device *dev);
+/* Keep inline for faster access in hot path */
static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
{
return dst->rcv != NULL;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index c1d4180651af..2ce9fec9eec5 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -73,6 +73,7 @@ struct dsa_device_ops {
};
struct dsa_slave_priv {
+ /* Copy of dp->ds->dst->tag_ops->xmit for faster access in hot path */
struct sk_buff * (*xmit)(struct sk_buff *skb,
struct net_device *dev);
--
2.13.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH net-next v2 2/6] net: dsa: do not cast dst
2017-05-30 18:33 [PATCH net-next v2 0/6] net: dsa: tagger simplification Vivien Didelot
2017-05-30 18:33 ` [PATCH net-next v2 1/6] net: dsa: comment hot path requirements Vivien Didelot
@ 2017-05-30 18:33 ` Vivien Didelot
2017-05-30 20:34 ` Florian Fainelli
2017-05-30 18:33 ` [PATCH net-next v2 3/6] net: dsa: remove dsa_uses_tagged_protocol Vivien Didelot
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Vivien Didelot @ 2017-05-30 18:33 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
dsa_ptr is not a void pointer anymore since Nov 2011, as of cf50dcc24f82
("dsa: Change dsa_uses_{dsa, trailer}_tags() into inline functions"),
but an explicit dsa_switch_tree pointer, thus remove the (void *) cast.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
net/dsa/dsa2.c | 2 +-
net/dsa/legacy.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 4301f52e4f5a..c4afeca6f478 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -454,7 +454,7 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst)
* sent to the tag format's receive function.
*/
wmb();
- dst->master_netdev->dsa_ptr = (void *)dst;
+ dst->master_netdev->dsa_ptr = dst;
dst->applied = true;
return 0;
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index ac4379b8d7ac..d70a1a788d17 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -651,7 +651,7 @@ static int dsa_setup_dst(struct dsa_switch_tree *dst, struct net_device *dev,
* sent to the tag format's receive function.
*/
wmb();
- dev->dsa_ptr = (void *)dst;
+ dev->dsa_ptr = dst;
return 0;
}
--
2.13.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v2 2/6] net: dsa: do not cast dst
2017-05-30 18:33 ` [PATCH net-next v2 2/6] net: dsa: do not cast dst Vivien Didelot
@ 2017-05-30 20:34 ` Florian Fainelli
0 siblings, 0 replies; 17+ messages in thread
From: Florian Fainelli @ 2017-05-30 20:34 UTC (permalink / raw)
To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn
On 05/30/2017 11:33 AM, Vivien Didelot wrote:
> dsa_ptr is not a void pointer anymore since Nov 2011, as of cf50dcc24f82
> ("dsa: Change dsa_uses_{dsa, trailer}_tags() into inline functions"),
> but an explicit dsa_switch_tree pointer, thus remove the (void *) cast.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v2 3/6] net: dsa: remove dsa_uses_tagged_protocol
2017-05-30 18:33 [PATCH net-next v2 0/6] net: dsa: tagger simplification Vivien Didelot
2017-05-30 18:33 ` [PATCH net-next v2 1/6] net: dsa: comment hot path requirements Vivien Didelot
2017-05-30 18:33 ` [PATCH net-next v2 2/6] net: dsa: do not cast dst Vivien Didelot
@ 2017-05-30 18:33 ` Vivien Didelot
2017-05-30 20:47 ` Florian Fainelli
2017-05-30 18:33 ` [PATCH net-next v2 4/6] net: dsa: remove unused arguments of tagger rcv Vivien Didelot
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Vivien Didelot @ 2017-05-30 18:33 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
Since dev->dsa_ptr is a pointer to a dsa_switch_tree, there is no need
to have another inline helper just to check rcv.
Remove dsa_uses_tagged_protocol and check dsa_ptr && dsa_ptr->rcv
together at the same time.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
include/net/dsa.h | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 58297e4c6b31..4675b52c964c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -467,16 +467,10 @@ struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
struct net_device *dsa_dev_to_net_device(struct device *dev);
/* Keep inline for faster access in hot path */
-static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
-{
- return dst->rcv != NULL;
-}
-
static inline bool netdev_uses_dsa(struct net_device *dev)
{
#if IS_ENABLED(CONFIG_NET_DSA)
- if (dev->dsa_ptr != NULL)
- return dsa_uses_tagged_protocol(dev->dsa_ptr);
+ return dev->dsa_ptr && dev->dsa_ptr->rcv;
#endif
return false;
}
--
2.13.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v2 3/6] net: dsa: remove dsa_uses_tagged_protocol
2017-05-30 18:33 ` [PATCH net-next v2 3/6] net: dsa: remove dsa_uses_tagged_protocol Vivien Didelot
@ 2017-05-30 20:47 ` Florian Fainelli
0 siblings, 0 replies; 17+ messages in thread
From: Florian Fainelli @ 2017-05-30 20:47 UTC (permalink / raw)
To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn
On 05/30/2017 11:33 AM, Vivien Didelot wrote:
> Since dev->dsa_ptr is a pointer to a dsa_switch_tree, there is no need
> to have another inline helper just to check rcv.
>
> Remove dsa_uses_tagged_protocol and check dsa_ptr && dsa_ptr->rcv
> together at the same time.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
FYI, the part that matters here is that we know whether the master
network device actually uses a tagged DSA protocol, this is important
for e.g: bridge (see 8db0a2ee2c6302a1dcbcdb93cb731dfc6c0cdb5e). Some
drivers like bcmsysport.c also care about that in order to enable
specific switch tagging protocol parsing hints.
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> include/net/dsa.h | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 58297e4c6b31..4675b52c964c 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -467,16 +467,10 @@ struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
> struct net_device *dsa_dev_to_net_device(struct device *dev);
>
> /* Keep inline for faster access in hot path */
> -static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
> -{
> - return dst->rcv != NULL;
> -}
> -
> static inline bool netdev_uses_dsa(struct net_device *dev)
> {
> #if IS_ENABLED(CONFIG_NET_DSA)
> - if (dev->dsa_ptr != NULL)
> - return dsa_uses_tagged_protocol(dev->dsa_ptr);
> + return dev->dsa_ptr && dev->dsa_ptr->rcv;
> #endif
> return false;
> }
>
--
Florian
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v2 4/6] net: dsa: remove unused arguments of tagger rcv
2017-05-30 18:33 [PATCH net-next v2 0/6] net: dsa: tagger simplification Vivien Didelot
` (2 preceding siblings ...)
2017-05-30 18:33 ` [PATCH net-next v2 3/6] net: dsa: remove dsa_uses_tagged_protocol Vivien Didelot
@ 2017-05-30 18:33 ` Vivien Didelot
2017-05-30 19:03 ` Florian Fainelli
2017-05-30 18:33 ` [PATCH net-next v2 5/6] net: dsa: remove out_drop label in taggers rcv Vivien Didelot
2017-05-30 18:33 ` [PATCH net-next v2 6/6] net: dsa: factor skb freeing on xmit Vivien Didelot
5 siblings, 1 reply; 17+ messages in thread
From: Vivien Didelot @ 2017-05-30 18:33 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
The struct dsa_device_ops defines the rcv function with 2 unused
arguments struct packet_type *pt, and struct net_device *orig_dev.
This patch removes them from the definition and implementations.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
include/net/dsa.h | 4 +---
net/dsa/dsa.c | 2 +-
net/dsa/dsa_priv.h | 4 +---
net/dsa/tag_brcm.c | 4 +---
net/dsa/tag_dsa.c | 4 +---
net/dsa/tag_edsa.c | 4 +---
net/dsa/tag_lan9303.c | 3 +--
net/dsa/tag_mtk.c | 4 +---
net/dsa/tag_qca.c | 4 +---
net/dsa/tag_trailer.c | 4 +---
10 files changed, 10 insertions(+), 27 deletions(-)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 4675b52c964c..9e128e447f0b 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -129,9 +129,7 @@ struct dsa_switch_tree {
/* Copy of tag_ops->rcv for faster access in hot path */
struct sk_buff * (*rcv)(struct sk_buff *skb,
- struct net_device *dev,
- struct packet_type *pt,
- struct net_device *orig_dev);
+ struct net_device *dev);
/*
* Original copy of the master netdev ethtool_ops
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 3288a80d4d6c..58594784ab94 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -204,7 +204,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
if (!skb)
return 0;
- nskb = dst->rcv(skb, dev, pt, orig_dev);
+ nskb = dst->rcv(skb, dev);
if (!nskb) {
kfree_skb(skb);
return 0;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 2ce9fec9eec5..1a69d70fcd3a 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -67,9 +67,7 @@ struct dsa_notifier_vlan_info {
struct dsa_device_ops {
struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
- struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt,
- struct net_device *orig_dev);
+ struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev);
};
struct dsa_slave_priv {
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 9f204f18ada3..c8bff4d7543d 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -92,9 +92,7 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
return NULL;
}
-static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt,
- struct net_device *orig_dev)
+static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
{
struct dsa_switch_tree *dst = dev->dsa_ptr;
struct dsa_switch *ds;
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 3b62a57956a3..41f78e42f9ba 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -68,9 +68,7 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
return NULL;
}
-static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt,
- struct net_device *orig_dev)
+static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
{
struct dsa_switch_tree *dst = dev->dsa_ptr;
struct dsa_switch *ds;
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index f95cafd05702..36389fc8fe06 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -81,9 +81,7 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
return NULL;
}
-static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt,
- struct net_device *orig_dev)
+static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
{
struct dsa_switch_tree *dst = dev->dsa_ptr;
struct dsa_switch *ds;
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index afd59330b5f1..82e150486497 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -71,8 +71,7 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
return NULL;
}
-static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt, struct net_device *orig_dev)
+static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev)
{
u16 *lan9303_tag;
struct dsa_switch_tree *dst = dev->dsa_ptr;
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index d1258e84cd71..7c9764c8d61b 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -47,9 +47,7 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
return NULL;
}
-static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt,
- struct net_device *orig_dev)
+static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev)
{
struct dsa_switch_tree *dst = dev->dsa_ptr;
struct dsa_switch *ds;
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 2451007699b7..d38bd5cbfad8 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -66,9 +66,7 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
return NULL;
}
-static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt,
- struct net_device *orig_dev)
+static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
{
struct dsa_switch_tree *dst = dev->dsa_ptr;
struct dsa_switch *ds;
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index 7488ab2932ab..e1fd78197fc8 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -58,9 +58,7 @@ static struct sk_buff *trailer_xmit(struct sk_buff *skb, struct net_device *dev)
return nskb;
}
-static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt,
- struct net_device *orig_dev)
+static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev)
{
struct dsa_switch_tree *dst = dev->dsa_ptr;
struct dsa_switch *ds;
--
2.13.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v2 4/6] net: dsa: remove unused arguments of tagger rcv
2017-05-30 18:33 ` [PATCH net-next v2 4/6] net: dsa: remove unused arguments of tagger rcv Vivien Didelot
@ 2017-05-30 19:03 ` Florian Fainelli
2017-05-30 19:55 ` Vivien Didelot
0 siblings, 1 reply; 17+ messages in thread
From: Florian Fainelli @ 2017-05-30 19:03 UTC (permalink / raw)
To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn
Hey Vivien,
On 05/30/2017 11:33 AM, Vivien Didelot wrote:
> The struct dsa_device_ops defines the rcv function with 2 unused
> arguments struct packet_type *pt, and struct net_device *orig_dev.
>
> This patch removes them from the definition and implementations.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
I actually have a patch pending that adds support for HW
insertion/extraction of switch tags (broadcom HW supports that) which
require the orig_dev to be there so we know what features are supported
by the master network device.
Do you mind dropping this one from your patch series?
Thanks!
--
Florian
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v2 4/6] net: dsa: remove unused arguments of tagger rcv
2017-05-30 19:03 ` Florian Fainelli
@ 2017-05-30 19:55 ` Vivien Didelot
2017-05-30 19:59 ` Florian Fainelli
0 siblings, 1 reply; 17+ messages in thread
From: Vivien Didelot @ 2017-05-30 19:55 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn
Hi Florian,
Florian Fainelli <f.fainelli@gmail.com> writes:
> I actually have a patch pending that adds support for HW
> insertion/extraction of switch tags (broadcom HW supports that) which
> require the orig_dev to be there so we know what features are supported
> by the master network device.
Is orig_dev really needed in the tagging implementation, or only in the
layer above? (dsa_slave_xmit and dsa_switch_rcv.)
> Do you mind dropping this one from your patch series?
I don't mind if they are actually needed in taggers. I'd wait for
reviews of the other patches before respinning though.
Thanks,
Vivien
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v2 5/6] net: dsa: remove out_drop label in taggers rcv
2017-05-30 18:33 [PATCH net-next v2 0/6] net: dsa: tagger simplification Vivien Didelot
` (3 preceding siblings ...)
2017-05-30 18:33 ` [PATCH net-next v2 4/6] net: dsa: remove unused arguments of tagger rcv Vivien Didelot
@ 2017-05-30 18:33 ` Vivien Didelot
2017-05-30 20:48 ` Florian Fainelli
2017-05-30 18:33 ` [PATCH net-next v2 6/6] net: dsa: factor skb freeing on xmit Vivien Didelot
5 siblings, 1 reply; 17+ messages in thread
From: Vivien Didelot @ 2017-05-30 18:33 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
Many rcv functions from net/dsa/tag_*.c have a useless out_drop goto
label which simply returns NULL. Kill it in favor of the obvious.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
net/dsa/tag_brcm.c | 11 ++++-------
net/dsa/tag_dsa.c | 13 +++++--------
net/dsa/tag_edsa.c | 13 +++++--------
net/dsa/tag_mtk.c | 9 +++------
net/dsa/tag_qca.c | 11 ++++-------
net/dsa/tag_trailer.c | 9 +++------
6 files changed, 24 insertions(+), 42 deletions(-)
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index c8bff4d7543d..10fa4c0ca46b 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -102,27 +102,27 @@ static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
ds = dst->cpu_dp->ds;
if (unlikely(!pskb_may_pull(skb, BRCM_TAG_LEN)))
- goto out_drop;
+ return NULL;
/* skb->data points to the EtherType, the tag is right before it */
brcm_tag = skb->data - 2;
/* The opcode should never be different than 0b000 */
if (unlikely((brcm_tag[0] >> BRCM_OPCODE_SHIFT) & BRCM_OPCODE_MASK))
- goto out_drop;
+ return NULL;
/* We should never see a reserved reason code without knowing how to
* handle it
*/
if (unlikely(brcm_tag[2] & BRCM_EG_RC_RSVD))
- goto out_drop;
+ return NULL;
/* Locate which port this is coming from */
source_port = brcm_tag[3] & BRCM_EG_PID_MASK;
/* Validate port against switch setup, either the port is totally */
if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
- goto out_drop;
+ return NULL;
/* Remove Broadcom tag and update checksum */
skb_pull_rcsum(skb, BRCM_TAG_LEN);
@@ -135,9 +135,6 @@ static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
skb->dev = ds->ports[source_port].netdev;
return skb;
-
-out_drop:
- return NULL;
}
const struct dsa_device_ops brcm_netdev_ops = {
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 41f78e42f9ba..9f5fecaa4a93 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -77,7 +77,7 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
int source_port;
if (unlikely(!pskb_may_pull(skb, DSA_HLEN)))
- goto out_drop;
+ return NULL;
/*
* The ethertype field is part of the DSA header.
@@ -88,7 +88,7 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
* Check that frame type is either TO_CPU or FORWARD.
*/
if ((dsa_header[0] & 0xc0) != 0x00 && (dsa_header[0] & 0xc0) != 0xc0)
- goto out_drop;
+ return NULL;
/*
* Determine source device and port.
@@ -101,14 +101,14 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
* port is a registered DSA port.
*/
if (source_device >= DSA_MAX_SWITCHES)
- goto out_drop;
+ return NULL;
ds = dst->ds[source_device];
if (!ds)
- goto out_drop;
+ return NULL;
if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
- goto out_drop;
+ return NULL;
/*
* Convert the DSA header to an 802.1q header if the 'tagged'
@@ -159,9 +159,6 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
skb->dev = ds->ports[source_port].netdev;
return skb;
-
-out_drop:
- return NULL;
}
const struct dsa_device_ops dsa_netdev_ops = {
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 36389fc8fe06..a9a06e19abeb 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -90,7 +90,7 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
int source_port;
if (unlikely(!pskb_may_pull(skb, EDSA_HLEN)))
- goto out_drop;
+ return NULL;
/*
* Skip the two null bytes after the ethertype.
@@ -101,7 +101,7 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
* Check that frame type is either TO_CPU or FORWARD.
*/
if ((edsa_header[0] & 0xc0) != 0x00 && (edsa_header[0] & 0xc0) != 0xc0)
- goto out_drop;
+ return NULL;
/*
* Determine source device and port.
@@ -114,14 +114,14 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
* port is a registered DSA port.
*/
if (source_device >= DSA_MAX_SWITCHES)
- goto out_drop;
+ return NULL;
ds = dst->ds[source_device];
if (!ds)
- goto out_drop;
+ return NULL;
if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
- goto out_drop;
+ return NULL;
/*
* If the 'tagged' bit is set, convert the DSA tag to a 802.1q
@@ -178,9 +178,6 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
skb->dev = ds->ports[source_port].netdev;
return skb;
-
-out_drop:
- return NULL;
}
const struct dsa_device_ops edsa_netdev_ops = {
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 7c9764c8d61b..327dd7b596df 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -55,7 +55,7 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev)
__be16 *phdr, hdr;
if (unlikely(!pskb_may_pull(skb, MTK_HDR_LEN)))
- goto out_drop;
+ return NULL;
/* The MTK header is added by the switch between src addr
* and ethertype at this point, skb->data points to 2 bytes
@@ -77,19 +77,16 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev)
*/
ds = dst->ds[0];
if (!ds)
- goto out_drop;
+ return NULL;
/* Get source port information */
port = (hdr & MTK_HDR_RECV_SOURCE_PORT_MASK);
if (!ds->ports[port].netdev)
- goto out_drop;
+ return NULL;
skb->dev = ds->ports[port].netdev;
return skb;
-
-out_drop:
- return NULL;
}
const struct dsa_device_ops mtk_netdev_ops = {
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index d38bd5cbfad8..8080ad8f2abb 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -75,7 +75,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
__be16 *phdr, hdr;
if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
- goto out_drop;
+ return NULL;
/* The QCA header is added by the switch between src addr and Ethertype
* At this point, skb->data points to ethertype so header should be
@@ -87,7 +87,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
/* Make sure the version is correct */
ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S;
if (unlikely(ver != QCA_HDR_VERSION))
- goto out_drop;
+ return NULL;
/* Remove QCA tag and recalculate checksum */
skb_pull_rcsum(skb, QCA_HDR_LEN);
@@ -99,20 +99,17 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
*/
ds = dst->cpu_dp->ds;
if (!ds)
- goto out_drop;
+ return NULL;
/* Get source port information */
port = (hdr & QCA_HDR_RECV_SOURCE_PORT_MASK);
if (!ds->ports[port].netdev)
- goto out_drop;
+ return NULL;
/* Update skb & forward the frame accordingly */
skb->dev = ds->ports[port].netdev;
return skb;
-
-out_drop:
- return NULL;
}
const struct dsa_device_ops qca_netdev_ops = {
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index e1fd78197fc8..7422b1329712 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -68,25 +68,22 @@ static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev)
ds = dst->cpu_dp->ds;
if (skb_linearize(skb))
- goto out_drop;
+ return NULL;
trailer = skb_tail_pointer(skb) - 4;
if (trailer[0] != 0x80 || (trailer[1] & 0xf8) != 0x00 ||
(trailer[2] & 0xef) != 0x00 || trailer[3] != 0x00)
- goto out_drop;
+ return NULL;
source_port = trailer[1] & 7;
if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
- goto out_drop;
+ return NULL;
pskb_trim_rcsum(skb, skb->len - 4);
skb->dev = ds->ports[source_port].netdev;
return skb;
-
-out_drop:
- return NULL;
}
const struct dsa_device_ops trailer_netdev_ops = {
--
2.13.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH net-next v2 6/6] net: dsa: factor skb freeing on xmit
2017-05-30 18:33 [PATCH net-next v2 0/6] net: dsa: tagger simplification Vivien Didelot
` (4 preceding siblings ...)
2017-05-30 18:33 ` [PATCH net-next v2 5/6] net: dsa: remove out_drop label in taggers rcv Vivien Didelot
@ 2017-05-30 18:33 ` Vivien Didelot
2017-05-30 20:59 ` Florian Fainelli
5 siblings, 1 reply; 17+ messages in thread
From: Vivien Didelot @ 2017-05-30 18:33 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
The taggers are currently responsible to free the original SKB if they
made a copy of it, or in case of error.
This patch simplifies this by freeing the original SKB in the
dsa_slave_xmit caller, but only if an error (NULL) is returned.
It is still the responsibility of the tagger to free the original SKB if
it returned a copy of it.
At the same time, fix the checkpatch NULL comparison check:
CHECK: Comparison to NULL could be written "!nskb"
#208: FILE: net/dsa/tag_trailer.c:35:
+ if (nskb == NULL)
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
net/dsa/slave.c | 8 ++++++--
net/dsa/tag_brcm.c | 6 +-----
net/dsa/tag_dsa.c | 8 ++------
net/dsa/tag_edsa.c | 8 ++------
net/dsa/tag_lan9303.c | 5 +----
net/dsa/tag_mtk.c | 6 +-----
net/dsa/tag_qca.c | 6 +-----
net/dsa/tag_trailer.c | 4 +---
8 files changed, 15 insertions(+), 36 deletions(-)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 887e26695519..4473aec9dcda 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -362,10 +362,14 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
dev->stats.tx_packets++;
dev->stats.tx_bytes += skb->len;
- /* Transmit function may have to reallocate the original SKB */
+ /* Transmit function may have to reallocate the original SKB,
+ * in which case it must have freed it. Only free it here on error.
+ */
nskb = p->xmit(skb, dev);
- if (!nskb)
+ if (!nskb) {
+ kfree_skb(skb);
return NETDEV_TX_OK;
+ }
/* SKB for netpoll still need to be mangled with the protocol-specific
* tag to be successfully transmitted
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 10fa4c0ca46b..5db6bcfde025 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -65,7 +65,7 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
u8 *brcm_tag;
if (skb_cow_head(skb, BRCM_TAG_LEN) < 0)
- goto out_free;
+ return NULL;
skb_push(skb, BRCM_TAG_LEN);
@@ -86,10 +86,6 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
brcm_tag[3] = (1 << p->dp->index) & BRCM_IG_DSTMAP1_MASK;
return skb;
-
-out_free:
- kfree_skb(skb);
- return NULL;
}
static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 9f5fecaa4a93..6837ca9160a8 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -28,7 +28,7 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
*/
if (skb->protocol == htons(ETH_P_8021Q)) {
if (skb_cow_head(skb, 0) < 0)
- goto out_free;
+ return NULL;
/*
* Construct tagged FROM_CPU DSA tag from 802.1q tag.
@@ -46,7 +46,7 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
}
} else {
if (skb_cow_head(skb, DSA_HLEN) < 0)
- goto out_free;
+ return NULL;
skb_push(skb, DSA_HLEN);
memmove(skb->data, skb->data + DSA_HLEN, 2 * ETH_ALEN);
@@ -62,10 +62,6 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
}
return skb;
-
-out_free:
- kfree_skb(skb);
- return NULL;
}
static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index a9a06e19abeb..96ddc90472a2 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -30,7 +30,7 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
*/
if (skb->protocol == htons(ETH_P_8021Q)) {
if (skb_cow_head(skb, DSA_HLEN) < 0)
- goto out_free;
+ return NULL;
skb_push(skb, DSA_HLEN);
memmove(skb->data, skb->data + DSA_HLEN, 2 * ETH_ALEN);
@@ -55,7 +55,7 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
}
} else {
if (skb_cow_head(skb, EDSA_HLEN) < 0)
- goto out_free;
+ return NULL;
skb_push(skb, EDSA_HLEN);
memmove(skb->data, skb->data + EDSA_HLEN, 2 * ETH_ALEN);
@@ -75,10 +75,6 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
}
return skb;
-
-out_free:
- kfree_skb(skb);
- return NULL;
}
static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 82e150486497..e33348101e8f 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -52,7 +52,7 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
if (skb_cow_head(skb, LAN9303_TAG_LEN) < 0) {
dev_dbg(&dev->dev,
"Cannot make room for the special tag. Dropping packet\n");
- goto out_free;
+ return NULL;
}
/* provide 'LAN9303_TAG_LEN' bytes additional space */
@@ -66,9 +66,6 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
lan9303_tag[1] = htons(p->dp->index | BIT(4));
return skb;
-out_free:
- kfree_skb(skb);
- return NULL;
}
static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 327dd7b596df..ee62ae5f03e1 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -27,7 +27,7 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
u8 *mtk_tag;
if (skb_cow_head(skb, MTK_HDR_LEN) < 0)
- goto out_free;
+ return NULL;
skb_push(skb, MTK_HDR_LEN);
@@ -41,10 +41,6 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
mtk_tag[3] = 0;
return skb;
-
-out_free:
- kfree_skb(skb);
- return NULL;
}
static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 8080ad8f2abb..4d3bc2920477 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -45,7 +45,7 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
dev->stats.tx_bytes += skb->len;
if (skb_cow_head(skb, 0) < 0)
- goto out_free;
+ return NULL;
skb_push(skb, QCA_HDR_LEN);
@@ -60,10 +60,6 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
*phdr = htons(hdr);
return skb;
-
-out_free:
- kfree_skb(skb);
- return NULL;
}
static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index 7422b1329712..8c9a7f5ecc66 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -32,10 +32,8 @@ static struct sk_buff *trailer_xmit(struct sk_buff *skb, struct net_device *dev)
padlen = 60 - skb->len;
nskb = alloc_skb(NET_IP_ALIGN + skb->len + padlen + 4, GFP_ATOMIC);
- if (nskb == NULL) {
- kfree_skb(skb);
+ if (!nskb)
return NULL;
- }
skb_reserve(nskb, NET_IP_ALIGN);
skb_reset_mac_header(nskb);
--
2.13.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v2 6/6] net: dsa: factor skb freeing on xmit
2017-05-30 18:33 ` [PATCH net-next v2 6/6] net: dsa: factor skb freeing on xmit Vivien Didelot
@ 2017-05-30 20:59 ` Florian Fainelli
2017-05-31 18:18 ` Vivien Didelot
0 siblings, 1 reply; 17+ messages in thread
From: Florian Fainelli @ 2017-05-30 20:59 UTC (permalink / raw)
To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn
On 05/30/2017 11:33 AM, Vivien Didelot wrote:
> The taggers are currently responsible to free the original SKB if they
> made a copy of it, or in case of error.
>
> This patch simplifies this by freeing the original SKB in the
> dsa_slave_xmit caller, but only if an error (NULL) is returned.
Is not it a clearer contract if the tagging protocol must always free
the original SKB, whether it succeeded in creating a new one or not?
>
> It is still the responsibility of the tagger to free the original SKB if
> it returned a copy of it.
>
> At the same time, fix the checkpatch NULL comparison check:
>
> CHECK: Comparison to NULL could be written "!nskb"
> #208: FILE: net/dsa/tag_trailer.c:35:
> + if (nskb == NULL)
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
> net/dsa/slave.c | 8 ++++++--
> net/dsa/tag_brcm.c | 6 +-----
> net/dsa/tag_dsa.c | 8 ++------
> net/dsa/tag_edsa.c | 8 ++------
> net/dsa/tag_lan9303.c | 5 +----
> net/dsa/tag_mtk.c | 6 +-----
> net/dsa/tag_qca.c | 6 +-----
> net/dsa/tag_trailer.c | 4 +---
> 8 files changed, 15 insertions(+), 36 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 887e26695519..4473aec9dcda 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -362,10 +362,14 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
> dev->stats.tx_packets++;
> dev->stats.tx_bytes += skb->len;
>
> - /* Transmit function may have to reallocate the original SKB */
> + /* Transmit function may have to reallocate the original SKB,
> + * in which case it must have freed it. Only free it here on error.
> + */
> nskb = p->xmit(skb, dev);
> - if (!nskb)
> + if (!nskb) {
> + kfree_skb(skb);
> return NETDEV_TX_OK;
> + }
>
> /* SKB for netpoll still need to be mangled with the protocol-specific
> * tag to be successfully transmitted
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index 10fa4c0ca46b..5db6bcfde025 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -65,7 +65,7 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
> u8 *brcm_tag;
>
> if (skb_cow_head(skb, BRCM_TAG_LEN) < 0)
> - goto out_free;
> + return NULL;
>
> skb_push(skb, BRCM_TAG_LEN);
>
> @@ -86,10 +86,6 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
> brcm_tag[3] = (1 << p->dp->index) & BRCM_IG_DSTMAP1_MASK;
>
> return skb;
> -
> -out_free:
> - kfree_skb(skb);
> - return NULL;
> }
>
> static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index 9f5fecaa4a93..6837ca9160a8 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -28,7 +28,7 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
> */
> if (skb->protocol == htons(ETH_P_8021Q)) {
> if (skb_cow_head(skb, 0) < 0)
> - goto out_free;
> + return NULL;
>
> /*
> * Construct tagged FROM_CPU DSA tag from 802.1q tag.
> @@ -46,7 +46,7 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
> }
> } else {
> if (skb_cow_head(skb, DSA_HLEN) < 0)
> - goto out_free;
> + return NULL;
> skb_push(skb, DSA_HLEN);
>
> memmove(skb->data, skb->data + DSA_HLEN, 2 * ETH_ALEN);
> @@ -62,10 +62,6 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
> }
>
> return skb;
> -
> -out_free:
> - kfree_skb(skb);
> - return NULL;
> }
>
> static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
> diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
> index a9a06e19abeb..96ddc90472a2 100644
> --- a/net/dsa/tag_edsa.c
> +++ b/net/dsa/tag_edsa.c
> @@ -30,7 +30,7 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
> */
> if (skb->protocol == htons(ETH_P_8021Q)) {
> if (skb_cow_head(skb, DSA_HLEN) < 0)
> - goto out_free;
> + return NULL;
> skb_push(skb, DSA_HLEN);
>
> memmove(skb->data, skb->data + DSA_HLEN, 2 * ETH_ALEN);
> @@ -55,7 +55,7 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
> }
> } else {
> if (skb_cow_head(skb, EDSA_HLEN) < 0)
> - goto out_free;
> + return NULL;
> skb_push(skb, EDSA_HLEN);
>
> memmove(skb->data, skb->data + EDSA_HLEN, 2 * ETH_ALEN);
> @@ -75,10 +75,6 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
> }
>
> return skb;
> -
> -out_free:
> - kfree_skb(skb);
> - return NULL;
> }
>
> static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
> diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
> index 82e150486497..e33348101e8f 100644
> --- a/net/dsa/tag_lan9303.c
> +++ b/net/dsa/tag_lan9303.c
> @@ -52,7 +52,7 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
> if (skb_cow_head(skb, LAN9303_TAG_LEN) < 0) {
> dev_dbg(&dev->dev,
> "Cannot make room for the special tag. Dropping packet\n");
> - goto out_free;
> + return NULL;
> }
>
> /* provide 'LAN9303_TAG_LEN' bytes additional space */
> @@ -66,9 +66,6 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
> lan9303_tag[1] = htons(p->dp->index | BIT(4));
>
> return skb;
> -out_free:
> - kfree_skb(skb);
> - return NULL;
> }
>
> static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev)
> diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
> index 327dd7b596df..ee62ae5f03e1 100644
> --- a/net/dsa/tag_mtk.c
> +++ b/net/dsa/tag_mtk.c
> @@ -27,7 +27,7 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
> u8 *mtk_tag;
>
> if (skb_cow_head(skb, MTK_HDR_LEN) < 0)
> - goto out_free;
> + return NULL;
>
> skb_push(skb, MTK_HDR_LEN);
>
> @@ -41,10 +41,6 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
> mtk_tag[3] = 0;
>
> return skb;
> -
> -out_free:
> - kfree_skb(skb);
> - return NULL;
> }
>
> static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> index 8080ad8f2abb..4d3bc2920477 100644
> --- a/net/dsa/tag_qca.c
> +++ b/net/dsa/tag_qca.c
> @@ -45,7 +45,7 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
> dev->stats.tx_bytes += skb->len;
>
> if (skb_cow_head(skb, 0) < 0)
> - goto out_free;
> + return NULL;
>
> skb_push(skb, QCA_HDR_LEN);
>
> @@ -60,10 +60,6 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
> *phdr = htons(hdr);
>
> return skb;
> -
> -out_free:
> - kfree_skb(skb);
> - return NULL;
> }
>
> static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
> index 7422b1329712..8c9a7f5ecc66 100644
> --- a/net/dsa/tag_trailer.c
> +++ b/net/dsa/tag_trailer.c
> @@ -32,10 +32,8 @@ static struct sk_buff *trailer_xmit(struct sk_buff *skb, struct net_device *dev)
> padlen = 60 - skb->len;
>
> nskb = alloc_skb(NET_IP_ALIGN + skb->len + padlen + 4, GFP_ATOMIC);
> - if (nskb == NULL) {
> - kfree_skb(skb);
> + if (!nskb)
> return NULL;
> - }
> skb_reserve(nskb, NET_IP_ALIGN);
>
> skb_reset_mac_header(nskb);
>
--
Florian
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH net-next v2 6/6] net: dsa: factor skb freeing on xmit
2017-05-30 20:59 ` Florian Fainelli
@ 2017-05-31 18:18 ` Vivien Didelot
0 siblings, 0 replies; 17+ messages in thread
From: Vivien Didelot @ 2017-05-31 18:18 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn
Hi Florian,
Florian Fainelli <f.fainelli@gmail.com> writes:
> On 05/30/2017 11:33 AM, Vivien Didelot wrote:
>> The taggers are currently responsible to free the original SKB if they
>> made a copy of it, or in case of error.
>>
>> This patch simplifies this by freeing the original SKB in the
>> dsa_slave_xmit caller, but only if an error (NULL) is returned.
>
> Is not it a clearer contract if the tagging protocol must always free
> the original SKB, whether it succeeded in creating a new one or not?
The rcv caller frees the original SKB on error, as of a86d8becc3f0
("net: dsa: Factor bottom tag receive functions").
It'd be nice if we could make both xmit and rcv symmetric, and limit the
tagging implementations to only do their real job: tag/untag an SKB with
the destination/source port.
Thanks,
Vivien
^ permalink raw reply [flat|nested] 17+ messages in thread