linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Fixes for KSZ DSA switch
@ 2021-07-21 21:56 Lino Sanfilippo
  2021-07-21 21:56 ` [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers Lino Sanfilippo
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Lino Sanfilippo @ 2021-07-21 21:56 UTC (permalink / raw)
  To: woojung.huh, olteanv
  Cc: UNGLinuxDriver, andrew, vivien.didelot, f.fainelli, davem, kuba,
	netdev, linux-kernel, Lino Sanfilippo

These patches fix issues I encountered while using a KSZ9897 as a DSA
switch with a broadcom GENET network device as the DSA master device.

PATCH 1 fixes an invalid access to an SKB in case it is scattered.
PATCH 2 fixes incorrect hardware checksum calculation caused by the DSA
tag.

Changes in v2:
- instead of linearizing the SKBs only for KSZ switches ensure linearized
  SKBs for all tail taggers by clearing the feature flags NETIF_F_HW_SG and
  NETIF_F_FRAGLIST (suggested by Vladimir Oltean)

The patches have been tested with a KSZ9897 and apply against net-next.

Lino Sanfilippo (2):
  net: dsa: ensure linearized SKBs in case of tail taggers
  net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum

 net/dsa/slave.c   | 14 +++++++++-----
 net/dsa/tag_ksz.c |  9 +++++++++
 2 files changed, 18 insertions(+), 5 deletions(-)


base-commit: 54cb43199e14c1181ddcd4a3782f1f7eb56bdab8
-- 
2.32.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers
  2021-07-21 21:56 [PATCH v2 0/2] Fixes for KSZ DSA switch Lino Sanfilippo
@ 2021-07-21 21:56 ` Lino Sanfilippo
  2021-07-21 23:35   ` Vladimir Oltean
  2021-07-21 21:56 ` [PATCH v2 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum Lino Sanfilippo
  2021-07-22  6:20 ` [PATCH v2 0/2] Fixes for KSZ DSA switch patchwork-bot+netdevbpf
  2 siblings, 1 reply; 13+ messages in thread
From: Lino Sanfilippo @ 2021-07-21 21:56 UTC (permalink / raw)
  To: woojung.huh, olteanv
  Cc: UNGLinuxDriver, andrew, vivien.didelot, f.fainelli, davem, kuba,
	netdev, linux-kernel, Lino Sanfilippo

The function skb_put() that is used by tail taggers to make room for the
DSA tag must only be called for linearized SKBS. However in case that the
slave device inherited features like NETIF_F_HW_SG or NETIF_F_FRAGLIST the
SKB passed to the slaves transmit function may not be linearized.
Avoid those SKBs by clearing the NETIF_F_HW_SG and NETIF_F_FRAGLIST flags
for tail taggers.
Furthermore since the tagging protocol can be changed at runtime move the
code for setting up the slaves features into dsa_slave_setup_tagger().

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 net/dsa/slave.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 22ce11cd770e..ae2a648ed9be 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1808,6 +1808,7 @@ void dsa_slave_setup_tagger(struct net_device *slave)
 	struct dsa_slave_priv *p = netdev_priv(slave);
 	const struct dsa_port *cpu_dp = dp->cpu_dp;
 	struct net_device *master = cpu_dp->master;
+	const struct dsa_switch *ds = dp->ds;
 
 	slave->needed_headroom = cpu_dp->tag_ops->needed_headroom;
 	slave->needed_tailroom = cpu_dp->tag_ops->needed_tailroom;
@@ -1819,6 +1820,14 @@ void dsa_slave_setup_tagger(struct net_device *slave)
 	slave->needed_tailroom += master->needed_tailroom;
 
 	p->xmit = cpu_dp->tag_ops->xmit;
+
+	slave->features = master->vlan_features | NETIF_F_HW_TC;
+	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
+		slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
+	slave->hw_features |= NETIF_F_HW_TC;
+	slave->features |= NETIF_F_LLTX;
+	if (slave->needed_tailroom)
+		slave->features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
 }
 
 static struct lock_class_key dsa_slave_netdev_xmit_lock_key;
@@ -1881,11 +1890,6 @@ int dsa_slave_create(struct dsa_port *port)
 	if (slave_dev == NULL)
 		return -ENOMEM;
 
-	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
-	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
-		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
-	slave_dev->hw_features |= NETIF_F_HW_TC;
-	slave_dev->features |= NETIF_F_LLTX;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
 	if (!is_zero_ether_addr(port->mac))
 		ether_addr_copy(slave_dev->dev_addr, port->mac);
-- 
2.32.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
  2021-07-21 21:56 [PATCH v2 0/2] Fixes for KSZ DSA switch Lino Sanfilippo
  2021-07-21 21:56 ` [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers Lino Sanfilippo
@ 2021-07-21 21:56 ` Lino Sanfilippo
  2021-07-21 23:37   ` Vladimir Oltean
  2021-07-22  3:55   ` Florian Fainelli
  2021-07-22  6:20 ` [PATCH v2 0/2] Fixes for KSZ DSA switch patchwork-bot+netdevbpf
  2 siblings, 2 replies; 13+ messages in thread
From: Lino Sanfilippo @ 2021-07-21 21:56 UTC (permalink / raw)
  To: woojung.huh, olteanv
  Cc: UNGLinuxDriver, andrew, vivien.didelot, f.fainelli, davem, kuba,
	netdev, linux-kernel, Lino Sanfilippo

If the checksum calculation is offloaded to the network device (e.g due to
NETIF_F_HW_CSUM inherited from the DSA master device), the calculated
layer 4 checksum is incorrect. This is since the DSA tag which is placed
after the layer 4 data is considered as being part of the daa and thus
errorneously included into the checksum calculation.
To avoid this, always calculate the layer 4 checksum in software.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 net/dsa/tag_ksz.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 53565f48934c..a201ccf2435d 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -53,6 +53,9 @@ static struct sk_buff *ksz8795_xmit(struct sk_buff *skb, struct net_device *dev)
 	u8 *tag;
 	u8 *addr;
 
+	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
+		return NULL;
+
 	/* Tag encoding */
 	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
 	addr = skb_mac_header(skb);
@@ -114,6 +117,9 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
 	u8 *addr;
 	u16 val;
 
+	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
+		return NULL;
+
 	/* Tag encoding */
 	tag = skb_put(skb, KSZ9477_INGRESS_TAG_LEN);
 	addr = skb_mac_header(skb);
@@ -164,6 +170,9 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
 	u8 *addr;
 	u8 *tag;
 
+	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
+		return NULL;
+
 	/* Tag encoding */
 	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
 	addr = skb_mac_header(skb);
-- 
2.32.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers
  2021-07-21 21:56 ` [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers Lino Sanfilippo
@ 2021-07-21 23:35   ` Vladimir Oltean
  2021-07-22  3:56     ` Florian Fainelli
  0 siblings, 1 reply; 13+ messages in thread
From: Vladimir Oltean @ 2021-07-21 23:35 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: woojung.huh, UNGLinuxDriver, andrew, vivien.didelot, f.fainelli,
	davem, kuba, netdev, linux-kernel

On Wed, Jul 21, 2021 at 11:56:41PM +0200, Lino Sanfilippo wrote:
> The function skb_put() that is used by tail taggers to make room for the
> DSA tag must only be called for linearized SKBS. However in case that the
> slave device inherited features like NETIF_F_HW_SG or NETIF_F_FRAGLIST the
> SKB passed to the slaves transmit function may not be linearized.
> Avoid those SKBs by clearing the NETIF_F_HW_SG and NETIF_F_FRAGLIST flags
> for tail taggers.
> Furthermore since the tagging protocol can be changed at runtime move the
> code for setting up the slaves features into dsa_slave_setup_tagger().
> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>  net/dsa/slave.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 22ce11cd770e..ae2a648ed9be 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1808,6 +1808,7 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>  	struct dsa_slave_priv *p = netdev_priv(slave);
>  	const struct dsa_port *cpu_dp = dp->cpu_dp;
>  	struct net_device *master = cpu_dp->master;
> +	const struct dsa_switch *ds = dp->ds;
>  
>  	slave->needed_headroom = cpu_dp->tag_ops->needed_headroom;
>  	slave->needed_tailroom = cpu_dp->tag_ops->needed_tailroom;
> @@ -1819,6 +1820,14 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>  	slave->needed_tailroom += master->needed_tailroom;
>  
>  	p->xmit = cpu_dp->tag_ops->xmit;
> +
> +	slave->features = master->vlan_features | NETIF_F_HW_TC;
> +	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
> +		slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +	slave->hw_features |= NETIF_F_HW_TC;
> +	slave->features |= NETIF_F_LLTX;
> +	if (slave->needed_tailroom)
> +		slave->features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
>  }
>  
>  static struct lock_class_key dsa_slave_netdev_xmit_lock_key;
> @@ -1881,11 +1890,6 @@ int dsa_slave_create(struct dsa_port *port)
>  	if (slave_dev == NULL)
>  		return -ENOMEM;
>  
> -	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
> -	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
> -		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> -	slave_dev->hw_features |= NETIF_F_HW_TC;
> -	slave_dev->features |= NETIF_F_LLTX;
>  	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>  	if (!is_zero_ether_addr(port->mac))
>  		ether_addr_copy(slave_dev->dev_addr, port->mac);
> -- 
> 2.32.0
> 

I would have probably changed the code in dsa_slave_create just like
this:

-	slave->features = master->vlan_features | NETIF_F_HW_TC;
+	slave->features = NETIF_F_HW_TC;
...
-	slave_dev->vlan_features = master->vlan_features;

and in dsa_slave_setup_tagger:

+	vlan_features = master->vlan_features;
+	slave->features &= ~vlan_features;
+	if (slave->needed_tailroom)
+		vlan_features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
+	slave->features |= vlan_features;
+	slave->vlan_features = vlan_features;

no need to move around NETIF_F_HW_TC and NETIF_F_LLTX. Makes sense?

And I would probably add:

Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol support")

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
  2021-07-21 21:56 ` [PATCH v2 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum Lino Sanfilippo
@ 2021-07-21 23:37   ` Vladimir Oltean
  2021-07-22  3:55   ` Florian Fainelli
  1 sibling, 0 replies; 13+ messages in thread
From: Vladimir Oltean @ 2021-07-21 23:37 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: woojung.huh, UNGLinuxDriver, andrew, vivien.didelot, f.fainelli,
	davem, kuba, netdev, linux-kernel

On Wed, Jul 21, 2021 at 11:56:42PM +0200, Lino Sanfilippo wrote:
> If the checksum calculation is offloaded to the network device (e.g due to
> NETIF_F_HW_CSUM inherited from the DSA master device), the calculated
> layer 4 checksum is incorrect. This is since the DSA tag which is placed
> after the layer 4 data is considered as being part of the daa and thus
> errorneously included into the checksum calculation.
> To avoid this, always calculate the layer 4 checksum in software.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---

Fixes: 8b8010fb7876 ("dsa: add support for Microchip KSZ tail tagging")
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
  2021-07-21 21:56 ` [PATCH v2 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum Lino Sanfilippo
  2021-07-21 23:37   ` Vladimir Oltean
@ 2021-07-22  3:55   ` Florian Fainelli
  1 sibling, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2021-07-22  3:55 UTC (permalink / raw)
  To: Lino Sanfilippo, woojung.huh, olteanv
  Cc: UNGLinuxDriver, andrew, vivien.didelot, davem, kuba, netdev,
	linux-kernel



On 7/21/2021 2:56 PM, Lino Sanfilippo wrote:
> If the checksum calculation is offloaded to the network device (e.g due to
> NETIF_F_HW_CSUM inherited from the DSA master device), the calculated
> layer 4 checksum is incorrect. This is since the DSA tag which is placed
> after the layer 4 data is considered as being part of the daa and thus
> errorneously included into the checksum calculation.
> To avoid this, always calculate the layer 4 checksum in software.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers
  2021-07-21 23:35   ` Vladimir Oltean
@ 2021-07-22  3:56     ` Florian Fainelli
  2021-07-22 14:14       ` Andrew Lunn
  0 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2021-07-22  3:56 UTC (permalink / raw)
  To: Vladimir Oltean, Lino Sanfilippo
  Cc: woojung.huh, UNGLinuxDriver, andrew, vivien.didelot, davem, kuba,
	netdev, linux-kernel



On 7/21/2021 4:35 PM, Vladimir Oltean wrote:
> On Wed, Jul 21, 2021 at 11:56:41PM +0200, Lino Sanfilippo wrote:
>> The function skb_put() that is used by tail taggers to make room for the
>> DSA tag must only be called for linearized SKBS. However in case that the
>> slave device inherited features like NETIF_F_HW_SG or NETIF_F_FRAGLIST the
>> SKB passed to the slaves transmit function may not be linearized.
>> Avoid those SKBs by clearing the NETIF_F_HW_SG and NETIF_F_FRAGLIST flags
>> for tail taggers.
>> Furthermore since the tagging protocol can be changed at runtime move the
>> code for setting up the slaves features into dsa_slave_setup_tagger().
>>
>> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>> ---
>>   net/dsa/slave.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 22ce11cd770e..ae2a648ed9be 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -1808,6 +1808,7 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>>   	struct dsa_slave_priv *p = netdev_priv(slave);
>>   	const struct dsa_port *cpu_dp = dp->cpu_dp;
>>   	struct net_device *master = cpu_dp->master;
>> +	const struct dsa_switch *ds = dp->ds;
>>   
>>   	slave->needed_headroom = cpu_dp->tag_ops->needed_headroom;
>>   	slave->needed_tailroom = cpu_dp->tag_ops->needed_tailroom;
>> @@ -1819,6 +1820,14 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>>   	slave->needed_tailroom += master->needed_tailroom;
>>   
>>   	p->xmit = cpu_dp->tag_ops->xmit;
>> +
>> +	slave->features = master->vlan_features | NETIF_F_HW_TC;
>> +	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
>> +		slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>> +	slave->hw_features |= NETIF_F_HW_TC;
>> +	slave->features |= NETIF_F_LLTX;
>> +	if (slave->needed_tailroom)
>> +		slave->features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
>>   }
>>   
>>   static struct lock_class_key dsa_slave_netdev_xmit_lock_key;
>> @@ -1881,11 +1890,6 @@ int dsa_slave_create(struct dsa_port *port)
>>   	if (slave_dev == NULL)
>>   		return -ENOMEM;
>>   
>> -	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
>> -	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
>> -		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>> -	slave_dev->hw_features |= NETIF_F_HW_TC;
>> -	slave_dev->features |= NETIF_F_LLTX;
>>   	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>>   	if (!is_zero_ether_addr(port->mac))
>>   		ether_addr_copy(slave_dev->dev_addr, port->mac);
>> -- 
>> 2.32.0
>>
> 
> I would have probably changed the code in dsa_slave_create just like
> this:
> 
> -	slave->features = master->vlan_features | NETIF_F_HW_TC;
> +	slave->features = NETIF_F_HW_TC;
> ...
> -	slave_dev->vlan_features = master->vlan_features;
> 
> and in dsa_slave_setup_tagger:
> 
> +	vlan_features = master->vlan_features;
> +	slave->features &= ~vlan_features;
> +	if (slave->needed_tailroom)
> +		vlan_features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
> +	slave->features |= vlan_features;
> +	slave->vlan_features = vlan_features;
> 
> no need to move around NETIF_F_HW_TC and NETIF_F_LLTX. Makes sense?
> 
> And I would probably add:
> 
> Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol support")

Agreed, with those fixed:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/2] Fixes for KSZ DSA switch
  2021-07-21 21:56 [PATCH v2 0/2] Fixes for KSZ DSA switch Lino Sanfilippo
  2021-07-21 21:56 ` [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers Lino Sanfilippo
  2021-07-21 21:56 ` [PATCH v2 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum Lino Sanfilippo
@ 2021-07-22  6:20 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-07-22  6:20 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: woojung.huh, olteanv, UNGLinuxDriver, andrew, vivien.didelot,
	f.fainelli, davem, kuba, netdev, linux-kernel, LinoSanfilippo

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Wed, 21 Jul 2021 23:56:40 +0200 you wrote:
> These patches fix issues I encountered while using a KSZ9897 as a DSA
> switch with a broadcom GENET network device as the DSA master device.
> 
> PATCH 1 fixes an invalid access to an SKB in case it is scattered.
> PATCH 2 fixes incorrect hardware checksum calculation caused by the DSA
> tag.
> 
> [...]

Here is the summary with links:
  - [v2,1/2] net: dsa: ensure linearized SKBs in case of tail taggers
    https://git.kernel.org/netdev/net/c/21cf377a9c40
  - [v2,2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
    https://git.kernel.org/netdev/net/c/37120f23ac89

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers
  2021-07-22  3:56     ` Florian Fainelli
@ 2021-07-22 14:14       ` Andrew Lunn
  2021-07-22 16:05         ` Florian Fainelli
  2021-07-23  7:47         ` Lino Sanfilippo
  0 siblings, 2 replies; 13+ messages in thread
From: Andrew Lunn @ 2021-07-22 14:14 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Vladimir Oltean, Lino Sanfilippo, woojung.huh, UNGLinuxDriver,
	vivien.didelot, davem, kuba, netdev, linux-kernel

> Agreed, with those fixed:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Hi Florian, Vladimir

I would suggest stop adding Reviewed-by: when you actual want changes
made. The bot does not seem to be reading the actual emails, it just
looks for tags. And when there are sufficient tags, it merges,
independent of requests for change, open questions, etc.

	    Andrew

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers
  2021-07-22 14:14       ` Andrew Lunn
@ 2021-07-22 16:05         ` Florian Fainelli
  2021-07-23  7:47         ` Lino Sanfilippo
  1 sibling, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2021-07-22 16:05 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Vladimir Oltean, Lino Sanfilippo, woojung.huh, UNGLinuxDriver,
	vivien.didelot, davem, kuba, netdev, linux-kernel

On 7/22/21 7:14 AM, Andrew Lunn wrote:
>> Agreed, with those fixed:
>>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Hi Florian, Vladimir
> 
> I would suggest stop adding Reviewed-by: when you actual want changes
> made. The bot does not seem to be reading the actual emails, it just
> looks for tags. And when there are sufficient tags, it merges,
> independent of requests for change, open questions, etc.

Yes, I will definitively stop doing that. I did not think that the
merging was handled by a bot, but if it is, that makes me seriously
nervous about the whole process.
-- 
Florian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers
  2021-07-22 14:14       ` Andrew Lunn
  2021-07-22 16:05         ` Florian Fainelli
@ 2021-07-23  7:47         ` Lino Sanfilippo
  2021-07-23 12:22           ` Vladimir Oltean
  1 sibling, 1 reply; 13+ messages in thread
From: Lino Sanfilippo @ 2021-07-23  7:47 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli
  Cc: Vladimir Oltean, woojung.huh, UNGLinuxDriver, vivien.didelot,
	davem, kuba, netdev, linux-kernel

On 22.07.21 at 16:14, Andrew Lunn wrote:
>> Agreed, with those fixed:
>>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>
> Hi Florian, Vladimir
>
> I would suggest stop adding Reviewed-by: when you actual want changes
> made. The bot does not seem to be reading the actual emails, it just
> looks for tags. And when there are sufficient tags, it merges,
> independent of requests for change, open questions, etc.
>
> 	    Andrew
>

Hi,

since I got a message that the patches have already been applied to netdev/net.git.
How should I proceed if I want to send a new version of the series? Just ignore the
merge to netdev and send the patches nevertheless?

Regards,
Lino

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers
  2021-07-23  7:47         ` Lino Sanfilippo
@ 2021-07-23 12:22           ` Vladimir Oltean
  2021-07-23 13:41             ` Lino Sanfilippo
  0 siblings, 1 reply; 13+ messages in thread
From: Vladimir Oltean @ 2021-07-23 12:22 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: Andrew Lunn, Florian Fainelli, woojung.huh, UNGLinuxDriver,
	vivien.didelot, davem, kuba, netdev, linux-kernel

On Fri, Jul 23, 2021 at 09:47:39AM +0200, Lino Sanfilippo wrote:
> since I got a message that the patches have already been applied to netdev/net.git.
> How should I proceed if I want to send a new version of the series? Just ignore the
> merge to netdev and send the patches nevertheless?

Since the git history is immutable you need to work with what is already
in the current net/master branch. What do you want to change, just
address the feedback I gave? If that is all, just don't bother, I intend
to look at adding a framework through which the DSA master can declare
what features it supports in conjunction with specific DSA tagging protocols.
That is material for net-next, and Dave took your patch at the last
minute for the "net" pull request towards Linus' tree. If you send
another patch on "net" in that area now, we'd have to wait for another
week or two until "net" will be merged again into "net-next". Not sure
if it's worth it. The only thing that was of concern to me is that you
assign the DSA interface's slave->vlan_features = master->vlan_features.
So even though you clear the NETIF_F_SG feature for the DSA slave
interface, VLAN uppers on top of DSA interfaces will still have NETIF_F_SG.
However, those skbs will be linearized during the dev_queue_xmit call
done by the 8021q driver towards DSA, so in the end, the way in which
you restructured the code may not be cosmetically ideal, but also
appears to not be functionally problematic.
Anyway, your patch will probably conflict with the stable trees (the
tag_ops->needed_tailroom was introduced very recently), so we will have
another chance to fix it up when Greg sends the email that the patch
failed to apply.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers
  2021-07-23 12:22           ` Vladimir Oltean
@ 2021-07-23 13:41             ` Lino Sanfilippo
  0 siblings, 0 replies; 13+ messages in thread
From: Lino Sanfilippo @ 2021-07-23 13:41 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Andrew Lunn, Florian Fainelli, woojung.huh, UNGLinuxDriver,
	vivien.didelot, davem, kuba, netdev, linux-kernel


Hi Vladimir,

On 23.07.21 at 14:22, Vladimir Oltean wrote:
> On Fri, Jul 23, 2021 at 09:47:39AM +0200, Lino Sanfilippo wrote:
>> since I got a message that the patches have already been applied to netdev/net.git.
>> How should I proceed if I want to send a new version of the series? Just ignore the
>> merge to netdev and send the patches nevertheless?
>
> Since the git history is immutable you need to work with what is already
> in the current net/master branch. What do you want to change, just
> address the feedback I gave? If that is all, just don't bother, I intend
> to look at adding a framework through which the DSA master can declare
> what features it supports in conjunction with specific DSA tagging protocols.
> That is material for net-next, and Dave took your patch at the last
> minute for the "net" pull request towards Linus' tree. If you send
> another patch on "net" in that area now, we'd have to wait for another
> week or two until "net" will be merged again into "net-next". Not sure
> if it's worth it. The only thing that was of concern to me is that you
> assign the DSA interface's slave->vlan_features = master->vlan_features.
> So even though you clear the NETIF_F_SG feature for the DSA slave
> interface, VLAN uppers on top of DSA interfaces will still have NETIF_F_SG.
> However, those skbs will be linearized during the dev_queue_xmit call
> done by the 8021q driver towards DSA, so in the end, the way in which
> you restructured the code may not be cosmetically ideal, but also
> appears to not be functionally problematic.
> Anyway, your patch will probably conflict with the stable trees (the
> tag_ops->needed_tailroom was introduced very recently), so we will have
> another chance to fix it up when Greg sends the email that the patch
> failed to apply.
>

Yes, I just wanted to address your feedback concerning the feature assignment
in a new patch version. But as you explained this is not needed and would make
things just unnecessary complicated. So lets wait and see if there are any
conflicts with Gregs stable tree. Thanks for the explanation.

Best Regards,
Lino

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-07-23 13:41 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 21:56 [PATCH v2 0/2] Fixes for KSZ DSA switch Lino Sanfilippo
2021-07-21 21:56 ` [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers Lino Sanfilippo
2021-07-21 23:35   ` Vladimir Oltean
2021-07-22  3:56     ` Florian Fainelli
2021-07-22 14:14       ` Andrew Lunn
2021-07-22 16:05         ` Florian Fainelli
2021-07-23  7:47         ` Lino Sanfilippo
2021-07-23 12:22           ` Vladimir Oltean
2021-07-23 13:41             ` Lino Sanfilippo
2021-07-21 21:56 ` [PATCH v2 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum Lino Sanfilippo
2021-07-21 23:37   ` Vladimir Oltean
2021-07-22  3:55   ` Florian Fainelli
2021-07-22  6:20 ` [PATCH v2 0/2] Fixes for KSZ DSA switch patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).