* [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags
@ 2021-08-28 23:56 Linus Walleij
2021-08-30 7:29 ` Vladimir Oltean
0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2021-08-28 23:56 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski
Cc: netdev, Linus Walleij, DENG Qingfang, Mauri Sandberg
I noticed that only port 0 worked on the RTL8366RB since we
started to use custom tags.
It turns out that the format of egress custom tags is actually
different from ingress custom tags. While the lower bits just
contain the port number in ingress tags, egress tags need to
indicate destination port by setting the bit for the
corresponding port.
It was working on port 0 because port 0 added 0x00 as port
number in the lower bits, and if you do this the packet gets
broadcasted to all ports, including the intended port.
Ooops.
Fix this and all ports work again.
Tested on the D-Link DIR-685 by sending traffic to each of
the ports in turn. It works.
Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
net/dsa/tag_rtl4_a.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
index 57c46b4ab2b3..042a6cb7704a 100644
--- a/net/dsa/tag_rtl4_a.c
+++ b/net/dsa/tag_rtl4_a.c
@@ -54,9 +54,10 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
p = (__be16 *)tag;
*p = htons(RTL4_A_ETHERTYPE);
- out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
- /* The lower bits is the port number */
- out |= (u8)dp->index;
+ out = (RTL4_A_PROTOCOL_RTL8366RB << RTL4_A_PROTOCOL_SHIFT);
+ /* The lower bits indicate the port number */
+ out |= BIT(dp->index);
+
p = (__be16 *)(tag + 2);
*p = htons(out);
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags
2021-08-28 23:56 [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags Linus Walleij
@ 2021-08-30 7:29 ` Vladimir Oltean
2021-08-30 21:56 ` Linus Walleij
0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Oltean @ 2021-08-30 7:29 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, DENG Qingfang, Mauri Sandberg
On Sun, Aug 29, 2021 at 01:56:19AM +0200, Linus Walleij wrote:
> I noticed that only port 0 worked on the RTL8366RB since we
> started to use custom tags.
>
> It turns out that the format of egress custom tags is actually
> different from ingress custom tags. While the lower bits just
> contain the port number in ingress tags, egress tags need to
> indicate destination port by setting the bit for the
> corresponding port.
>
> It was working on port 0 because port 0 added 0x00 as port
> number in the lower bits, and if you do this the packet gets
> broadcasted to all ports, including the intended port.
> Ooops.
Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
for a regular data packet?
>
> Fix this and all ports work again.
>
> Tested on the D-Link DIR-685 by sending traffic to each of
> the ports in turn. It works.
>
> Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
> Cc: DENG Qingfang <dqfext@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> net/dsa/tag_rtl4_a.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
> index 57c46b4ab2b3..042a6cb7704a 100644
> --- a/net/dsa/tag_rtl4_a.c
> +++ b/net/dsa/tag_rtl4_a.c
> @@ -54,9 +54,10 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
> p = (__be16 *)tag;
> *p = htons(RTL4_A_ETHERTYPE);
>
> - out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
What was 2 << 8? This patch changes that part.
> - /* The lower bits is the port number */
> - out |= (u8)dp->index;
> + out = (RTL4_A_PROTOCOL_RTL8366RB << RTL4_A_PROTOCOL_SHIFT);
> + /* The lower bits indicate the port number */
> + out |= BIT(dp->index);
> +
> p = (__be16 *)(tag + 2);
> *p = htons(out);
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags
2021-08-30 7:29 ` Vladimir Oltean
@ 2021-08-30 21:56 ` Linus Walleij
2021-08-30 22:20 ` Vladimir Oltean
0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2021-08-30 21:56 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, DENG Qingfang, Mauri Sandberg
On Mon, Aug 30, 2021 at 9:29 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Sun, Aug 29, 2021 at 01:56:19AM +0200, Linus Walleij wrote:
> > I noticed that only port 0 worked on the RTL8366RB since we
> > started to use custom tags.
> >
> > It turns out that the format of egress custom tags is actually
> > different from ingress custom tags. While the lower bits just
> > contain the port number in ingress tags, egress tags need to
> > indicate destination port by setting the bit for the
> > corresponding port.
> >
> > It was working on port 0 because port 0 added 0x00 as port
> > number in the lower bits, and if you do this the packet gets
> > broadcasted to all ports, including the intended port.
> > Ooops.
>
> Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
> for a regular data packet?
It gets broadcast :/
> > - out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
>
> What was 2 << 8? This patch changes that part.
It was a bit set in the ingress packets, we don't really know
what egress tag bits there are so first I just copied this
and since it turns out the bits in the lower order are not
correct I dropped this too and it works fine.
Do you want me to clarify in the commit message and
resend?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags
2021-08-30 21:56 ` Linus Walleij
@ 2021-08-30 22:20 ` Vladimir Oltean
2021-08-31 18:35 ` Linus Walleij
0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Oltean @ 2021-08-30 22:20 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, DENG Qingfang, Mauri Sandberg
On Mon, Aug 30, 2021 at 11:56:22PM +0200, Linus Walleij wrote:
> On Mon, Aug 30, 2021 at 9:29 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Sun, Aug 29, 2021 at 01:56:19AM +0200, Linus Walleij wrote:
> > > I noticed that only port 0 worked on the RTL8366RB since we
> > > started to use custom tags.
> > >
> > > It turns out that the format of egress custom tags is actually
> > > different from ingress custom tags. While the lower bits just
> > > contain the port number in ingress tags, egress tags need to
> > > indicate destination port by setting the bit for the
> > > corresponding port.
> > >
> > > It was working on port 0 because port 0 added 0x00 as port
> > > number in the lower bits, and if you do this the packet gets
> > > broadcasted to all ports, including the intended port.
> > > Ooops.
> >
> > Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
> > for a regular data packet?
>
> It gets broadcast :/
Okay, so a packet sent to a port mask of zero behaves just the same as a
packet sent to a port mask of all ones is what you're saying?
Sounds a bit... implausible?
When I phrased the question whether it gets "forwarded by MAC DA/VLAN ID",
obviously this includes the possibility of _flooding_, if the MAC
DA/VLAN ID is unknown to the FDB. The behavior of flooding a packet due
to unknown destination can be practically indistinguishable from a
"broadcast" (the latter having the sense that "you've told the switch to
broadcast this packet to all ports", at least this is what is implied by
the context of your commit message).
The point is that if the destination is not unknown, the packet is not
flooded (or "broadcast" as you say). So "broadcast" would be effectively
a mischaracterization of the behavior.
Just want to make sure that the switch does indeed "broadcast" packets
with a destination port mask of zero. Also curious if by "all ports",
the CPU port itself is also included (effectively looping back the packet)?
> > > - out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
> >
> > What was 2 << 8? This patch changes that part.
>
> It was a bit set in the ingress packets, we don't really know
> what egress tag bits there are so first I just copied this
> and since it turns out the bits in the lower order are not
> correct I dropped this too and it works fine.
>
> Do you want me to clarify in the commit message and
> resend?
Well, it is definitely not a logical part of the change. Also, a bug fix
patch that goes to stable kernels seems like the last place to me where
you'd want to change something that you don't really know what it does...
In net-next, this extra change is more than welcome. Possibly has
something to do with hardware address learning on the CPU port, but this
is just a very wild guess based on some other Realtek tagging protocol
drivers I've looked at recently. Anyway, more than likely not just a
random number with no effect.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags
2021-08-30 22:20 ` Vladimir Oltean
@ 2021-08-31 18:35 ` Linus Walleij
2021-08-31 19:05 ` Vladimir Oltean
0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2021-08-31 18:35 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, DENG Qingfang, Mauri Sandberg
On Tue, Aug 31, 2021 at 12:20 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
> > > for a regular data packet?
> >
> > It gets broadcast :/
>
> Okay, so a packet sent to a port mask of zero behaves just the same as a
> packet sent to a port mask of all ones is what you're saying?
> Sounds a bit... implausible?
>
> When I phrased the question whether it gets "forwarded by MAC DA/VLAN ID",
> obviously this includes the possibility of _flooding_, if the MAC
> DA/VLAN ID is unknown to the FDB. The behavior of flooding a packet due
> to unknown destination can be practically indistinguishable from a
> "broadcast" (the latter having the sense that "you've told the switch to
> broadcast this packet to all ports", at least this is what is implied by
> the context of your commit message).
>
> The point is that if the destination is not unknown, the packet is not
> flooded (or "broadcast" as you say). So "broadcast" would be effectively
> a mischaracterization of the behavior.
Oh OK sorry what I mean is that the packet appears on all ports of
the switch. Not sent to the broadcast address.
> Just want to make sure that the switch does indeed "broadcast" packets
> with a destination port mask of zero. Also curious if by "all ports",
> the CPU port itself is also included (effectively looping back the packet)?
It does not seem to appear at the CPU port. It appear on ports
0..4.
> > > > - out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
> > >
> > > What was 2 << 8? This patch changes that part.
> >
> > It was a bit set in the ingress packets, we don't really know
> > what egress tag bits there are so first I just copied this
> > and since it turns out the bits in the lower order are not
> > correct I dropped this too and it works fine.
> >
> > Do you want me to clarify in the commit message and
> > resend?
>
> Well, it is definitely not a logical part of the change. Also, a bug fix
> patch that goes to stable kernels seems like the last place to me where
> you'd want to change something that you don't really know what it does...
> In net-next, this extra change is more than welcome. Possibly has
> something to do with hardware address learning on the CPU port, but this
> is just a very wild guess based on some other Realtek tagging protocol
> drivers I've looked at recently. Anyway, more than likely not just a
> random number with no effect.
Yeah but I don't know anything else about it than that it appear
in the ingress packets which are known to have a different
format than the egress packets, the assumption that they were
the same format was wrong ... so it seems best to drop it as well.
But if you insist I can defer that to a separate patch for next.
I just can't see that it has any effect at all.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags
2021-08-31 18:35 ` Linus Walleij
@ 2021-08-31 19:05 ` Vladimir Oltean
0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2021-08-31 19:05 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, DENG Qingfang, Mauri Sandberg
On Tue, Aug 31, 2021 at 08:35:05PM +0200, Linus Walleij wrote:
> On Tue, Aug 31, 2021 at 12:20 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> > > > Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
> > > > for a regular data packet?
> > >
> > > It gets broadcast :/
> >
> > Okay, so a packet sent to a port mask of zero behaves just the same as a
> > packet sent to a port mask of all ones is what you're saying?
> > Sounds a bit... implausible?
> >
> > When I phrased the question whether it gets "forwarded by MAC DA/VLAN ID",
> > obviously this includes the possibility of _flooding_, if the MAC
> > DA/VLAN ID is unknown to the FDB. The behavior of flooding a packet due
> > to unknown destination can be practically indistinguishable from a
> > "broadcast" (the latter having the sense that "you've told the switch to
> > broadcast this packet to all ports", at least this is what is implied by
> > the context of your commit message).
> >
> > The point is that if the destination is not unknown, the packet is not
> > flooded (or "broadcast" as you say). So "broadcast" would be effectively
> > a mischaracterization of the behavior.
>
> Oh OK sorry what I mean is that the packet appears on all ports of
> the switch. Not sent to the broadcast address.
Yes, but why (due to which hardware decision does this behavior take place)?
I was not hung up on the "broadcast" word. That was used a bit imprecisely,
but I got over that. I was curious as to _why_ would the packets be
delivered to all ports of the switch. After all, you told the switch to
send the packet to _no_ port :-/
The reason why I'm so interested about this is because other switches
(mt7530) treat a destination port mask of 0x0 as "look up the FDB"
(reported by Qingfang here):
https://patchwork.kernel.org/project/netdevbpf/patch/20210825083832.2425886-3-dqfext@gmail.com/#24407683
This means it would be possible to implement the bridge TX forwarding
offload feature:
https://patchwork.kernel.org/project/netdevbpf/cover/20210722155542.2897921-1-vladimir.oltean@nxp.com/
I just wanted to know what type of packets were you testing with. If you
were testing with a unidirectional stream (where the switch has no
opportunity to learn the destination MAC on a particular port), then it
is much more likely that what's happening in your case is that the
packets were flooded, and not simply "broadcast". Pick a different MAC
DA, which _is_ learned in the FDB, and the packets would not be
"broadcast" (actually flooded) at all.
This is still my hypothesis about what was going on.
> > Just want to make sure that the switch does indeed "broadcast" packets
> > with a destination port mask of zero. Also curious if by "all ports",
> > the CPU port itself is also included (effectively looping back the packet)?
>
> It does not seem to appear at the CPU port. It appear on ports
> 0..4.
Which again would be consistent with my theory.
> > > > > - out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
> > > >
> > > > What was 2 << 8? This patch changes that part.
> > >
> > > It was a bit set in the ingress packets, we don't really know
> > > what egress tag bits there are so first I just copied this
> > > and since it turns out the bits in the lower order are not
> > > correct I dropped this too and it works fine.
> > >
> > > Do you want me to clarify in the commit message and
> > > resend?
> >
> > Well, it is definitely not a logical part of the change. Also, a bug fix
> > patch that goes to stable kernels seems like the last place to me where
> > you'd want to change something that you don't really know what it does...
> > In net-next, this extra change is more than welcome. Possibly has
> > something to do with hardware address learning on the CPU port, but this
> > is just a very wild guess based on some other Realtek tagging protocol
> > drivers I've looked at recently. Anyway, more than likely not just a
> > random number with no effect.
>
> Yeah but I don't know anything else about it than that it appear
> in the ingress packets which are known to have a different
> format than the egress packets, the assumption that they were
> the same format was wrong ... so it seems best to drop it as well.
> But if you insist I can defer that to a separate patch for next.
> I just can't see that it has any effect at all.
I was about to say that you can do as you wish, but I see you've resent
already. In general it is good to keep a change to the point, and
documented well and clear.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net] net: dsa: tag_rtl4_a: fix egress tags
@ 2021-02-28 17:08 DENG Qingfang
2021-02-28 17:54 ` Florian Fainelli
2021-03-01 13:58 ` Linus Walleij
0 siblings, 2 replies; 10+ messages in thread
From: DENG Qingfang @ 2021-02-28 17:08 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S. Miller, Jakub Kicinski, netdev, linux-kernel
Cc: Linus Walleij
Commit 86dd9868b878 has several issues, but was accepted too soon
before anyone could take a look.
- Double free. dsa_slave_xmit() will free the skb if the xmit function
returns NULL, but the skb is already freed by eth_skb_pad(). Use
__skb_put_padto() to avoid that.
- Unnecessary allocation. It has been done by DSA core since commit
a3b0b6479700.
- A u16 pointer points to skb data. It should be __be16 for network
byte order.
- Typo in comments. "numer" -> "number".
Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
net/dsa/tag_rtl4_a.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
index c17d39b4a1a0..e9176475bac8 100644
--- a/net/dsa/tag_rtl4_a.c
+++ b/net/dsa/tag_rtl4_a.c
@@ -35,14 +35,12 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
struct net_device *dev)
{
struct dsa_port *dp = dsa_slave_to_port(dev);
+ __be16 *p;
u8 *tag;
- u16 *p;
u16 out;
/* Pad out to at least 60 bytes */
- if (unlikely(eth_skb_pad(skb)))
- return NULL;
- if (skb_cow_head(skb, RTL4_A_HDR_LEN) < 0)
+ if (unlikely(__skb_put_padto(skb, ETH_ZLEN, false)))
return NULL;
netdev_dbg(dev, "add realtek tag to package to port %d\n",
@@ -53,13 +51,13 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
tag = skb->data + 2 * ETH_ALEN;
/* Set Ethertype */
- p = (u16 *)tag;
+ p = (__be16 *)tag;
*p = htons(RTL4_A_ETHERTYPE);
out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
- /* The lower bits is the port numer */
+ /* The lower bits is the port number */
out |= (u8)dp->index;
- p = (u16 *)(tag + 2);
+ p = (__be16 *)(tag + 2);
*p = htons(out);
return skb;
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net] net: dsa: tag_rtl4_a: fix egress tags
2021-02-28 17:08 [PATCH net] net: dsa: tag_rtl4_a: fix " DENG Qingfang
@ 2021-02-28 17:54 ` Florian Fainelli
2021-03-01 13:58 ` Linus Walleij
1 sibling, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2021-02-28 17:54 UTC (permalink / raw)
To: DENG Qingfang, Andrew Lunn, Vivien Didelot, Vladimir Oltean,
David S. Miller, Jakub Kicinski, netdev, linux-kernel
Cc: Linus Walleij
On 2/28/2021 9:08 AM, DENG Qingfang wrote:
> Commit 86dd9868b878 has several issues, but was accepted too soon
> before anyone could take a look.
>
> - Double free. dsa_slave_xmit() will free the skb if the xmit function
> returns NULL, but the skb is already freed by eth_skb_pad(). Use
> __skb_put_padto() to avoid that.
> - Unnecessary allocation. It has been done by DSA core since commit
> a3b0b6479700.
> - A u16 pointer points to skb data. It should be __be16 for network
> byte order.
> - Typo in comments. "numer" -> "number".
>
> Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] net: dsa: tag_rtl4_a: fix egress tags
2021-02-28 17:08 [PATCH net] net: dsa: tag_rtl4_a: fix " DENG Qingfang
2021-02-28 17:54 ` Florian Fainelli
@ 2021-03-01 13:58 ` Linus Walleij
2021-03-01 14:01 ` Vladimir Oltean
1 sibling, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2021-03-01 13:58 UTC (permalink / raw)
To: DENG Qingfang
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S. Miller, Jakub Kicinski, netdev, linux-kernel
On Sun, Feb 28, 2021 at 6:08 PM DENG Qingfang <dqfext@gmail.com> wrote:
> Commit 86dd9868b878 has several issues, but was accepted too soon
> before anyone could take a look.
>
> - Double free. dsa_slave_xmit() will free the skb if the xmit function
> returns NULL, but the skb is already freed by eth_skb_pad(). Use
> __skb_put_padto() to avoid that.
> - Unnecessary allocation. It has been done by DSA core since commit
> a3b0b6479700.
> - A u16 pointer points to skb data. It should be __be16 for network
> byte order.
> - Typo in comments. "numer" -> "number".
>
> Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
Ooops I send patches before properly going through the mailbox.
Oh well things like that happen.
David: ignore my patches to the same tagger and apply this instead!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] net: dsa: tag_rtl4_a: fix egress tags
2021-03-01 13:58 ` Linus Walleij
@ 2021-03-01 14:01 ` Vladimir Oltean
0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2021-03-01 14:01 UTC (permalink / raw)
To: Linus Walleij
Cc: DENG Qingfang, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S. Miller, Jakub Kicinski, netdev, linux-kernel
On Mon, Mar 01, 2021 at 02:58:59PM +0100, Linus Walleij wrote:
> On Sun, Feb 28, 2021 at 6:08 PM DENG Qingfang <dqfext@gmail.com> wrote:
>
> > Commit 86dd9868b878 has several issues, but was accepted too soon
> > before anyone could take a look.
> >
> > - Double free. dsa_slave_xmit() will free the skb if the xmit function
> > returns NULL, but the skb is already freed by eth_skb_pad(). Use
> > __skb_put_padto() to avoid that.
> > - Unnecessary allocation. It has been done by DSA core since commit
> > a3b0b6479700.
> > - A u16 pointer points to skb data. It should be __be16 for network
> > byte order.
> > - Typo in comments. "numer" -> "number".
> >
> > Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
> > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
>
> Ooops I send patches before properly going through the mailbox.
> Oh well things like that happen.
>
> David: ignore my patches to the same tagger and apply this instead!
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij
Last time I checked, performance/timing sensitive code is impacted by
netdev_dbg calls even if dynamic debugging isn't turned on. However,
neither your patches nor Qingfang's have removed that netdev_dbg line.
Is there any good reason to keep it?
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-08-31 19:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-28 23:56 [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags Linus Walleij
2021-08-30 7:29 ` Vladimir Oltean
2021-08-30 21:56 ` Linus Walleij
2021-08-30 22:20 ` Vladimir Oltean
2021-08-31 18:35 ` Linus Walleij
2021-08-31 19:05 ` Vladimir Oltean
-- strict thread matches above, loose matches on Subject: below --
2021-02-28 17:08 [PATCH net] net: dsa: tag_rtl4_a: fix " DENG Qingfang
2021-02-28 17:54 ` Florian Fainelli
2021-03-01 13:58 ` Linus Walleij
2021-03-01 14:01 ` Vladimir Oltean
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.