1. Patchew
  2. linux
  3. View series

[PATCH net] net: lan966x: Make sure to insert the vlan tags also in host mode

Horatiu Vultur posted 1 patch 6 months, 3 weeks ago
There is a newer version of this series
.../ethernet/microchip/lan966x/lan966x_main.c |  1 +
.../ethernet/microchip/lan966x/lan966x_main.h |  1 +
.../microchip/lan966x/lan966x_switchdev.c     |  1 +
.../ethernet/microchip/lan966x/lan966x_vlan.c | 21 +++++++++++++++++++
4 files changed, 24 insertions(+)
[PATCH net] net: lan966x: Make sure to insert the vlan tags also in host mode
Posted by Horatiu Vultur 6 months, 3 weeks ago 
When running these commands on DUT (and similar at the other end)
ip link set dev eth0 up
ip link add link eth0 name eth0.10 type vlan id 10
ip addr add 10.0.0.1/24 dev eth0.10
ip link set dev eth0.10 up
ping 10.0.0.2/24

The ping will fail.

The reason why is failing is because, the network interfaces for lan966x
have a flag saying that the HW can insert the vlan tags into the
frames(NETIF_F_HW_VLAN_CTAG_TX). Meaning that the frames that are
transmitted don't have the vlan tag inside the skb data, but they have
it inside the skb. We already get that vlan tag and put it in the IFH
but the problem is that we don't configure the HW to rewrite the frame
when the interface is in host mode.
The fix consists in actually configuring the HW to insert the vlan tag
if it is different than 0.

Fixes: 6d2c186afa5d ("net: lan966x: Add vlan support.")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_main.c |  1 +
 .../ethernet/microchip/lan966x/lan966x_main.h |  1 +
 .../microchip/lan966x/lan966x_switchdev.c     |  1 +
 .../ethernet/microchip/lan966x/lan966x_vlan.c | 21 +++++++++++++++++++
 4 files changed, 24 insertions(+)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 427bdc0e4908c..7001584f1b7a6 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -879,6 +879,7 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
 	lan966x_vlan_port_set_vlan_aware(port, 0);
 	lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
 	lan966x_vlan_port_apply(port);
+	lan966x_vlan_port_rew_host(port);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 1f9df67f05044..4f75f06883693 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -497,6 +497,7 @@ void lan966x_vlan_port_apply(struct lan966x_port *port);
 bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
 void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
 				      bool vlan_aware);
+void lan966x_vlan_port_rew_host(struct lan966x_port *port);
 int lan966x_vlan_port_set_vid(struct lan966x_port *port,
 			      u16 vid,
 			      bool pvid,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
index 1c88120eb291a..bcb4db76b75cd 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -297,6 +297,7 @@ static void lan966x_port_bridge_leave(struct lan966x_port *port,
 	lan966x_vlan_port_set_vlan_aware(port, false);
 	lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
 	lan966x_vlan_port_apply(port);
+	lan966x_vlan_port_rew_host(port);
 }
 
 int lan966x_port_changeupper(struct net_device *dev,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
index fa34a739c748e..f158ec6ab10cc 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
@@ -149,6 +149,27 @@ void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
 	port->vlan_aware = vlan_aware;
 }
 
+/* When the interface is in host mode, the interface should not be vlan aware
+ * but it should insert all the tags that it gets from the network stack.
+ * The tags are no in the data of the frame but actually in the skb and the ifh
+ * is confiured already to get this tag. So what we need to do is to update the
+ * rewriter to insert the vlan tag for all frames which have a vlan tag
+ * different than 0.
+ */
+void lan966x_vlan_port_rew_host(struct lan966x_port *port)
+{
+	struct lan966x *lan966x = port->lan966x;
+	u32 val;
+
+	/* Tag all frames except when VID == DEFAULT_VLAN */
+	val = REW_TAG_CFG_TAG_CFG_SET(1);
+
+	/* Update only some bits in the register */
+	lan_rmw(val,
+		REW_TAG_CFG_TAG_CFG,
+		lan966x, REW_TAG_CFG(port->chip_port));
+}
+
 void lan966x_vlan_port_apply(struct lan966x_port *port)
 {
 	struct lan966x *lan966x = port->lan966x;
-- 
2.34.1
Re: [PATCH net] net: lan966x: Make sure to insert the vlan tags also in host mode
Posted by Maxime Chevallier 6 months, 3 weeks ago 
Hello Horatiu,

On Tue, 27 May 2025 09:08:50 +0200
Horatiu Vultur <horatiu.vultur@microchip.com> wrote:

> When running these commands on DUT (and similar at the other end)
> ip link set dev eth0 up
> ip link add link eth0 name eth0.10 type vlan id 10
> ip addr add 10.0.0.1/24 dev eth0.10
> ip link set dev eth0.10 up
> ping 10.0.0.2/24
> 
> The ping will fail.
> 
> The reason why is failing is because, the network interfaces for lan966x
> have a flag saying that the HW can insert the vlan tags into the
> frames(NETIF_F_HW_VLAN_CTAG_TX). Meaning that the frames that are
> transmitted don't have the vlan tag inside the skb data, but they have
> it inside the skb. We already get that vlan tag and put it in the IFH
> but the problem is that we don't configure the HW to rewrite the frame
> when the interface is in host mode.
> The fix consists in actually configuring the HW to insert the vlan tag
> if it is different than 0.
> 
> Fixes: 6d2c186afa5d ("net: lan966x: Add vlan support.")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../ethernet/microchip/lan966x/lan966x_main.c |  1 +
>  .../ethernet/microchip/lan966x/lan966x_main.h |  1 +
>  .../microchip/lan966x/lan966x_switchdev.c     |  1 +
>  .../ethernet/microchip/lan966x/lan966x_vlan.c | 21 +++++++++++++++++++
>  4 files changed, 24 insertions(+)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index 427bdc0e4908c..7001584f1b7a6 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -879,6 +879,7 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
>  	lan966x_vlan_port_set_vlan_aware(port, 0);
>  	lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
>  	lan966x_vlan_port_apply(port);
> +	lan966x_vlan_port_rew_host(port);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index 1f9df67f05044..4f75f06883693 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -497,6 +497,7 @@ void lan966x_vlan_port_apply(struct lan966x_port *port);
>  bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
>  void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
>  				      bool vlan_aware);
> +void lan966x_vlan_port_rew_host(struct lan966x_port *port);
>  int lan966x_vlan_port_set_vid(struct lan966x_port *port,
>  			      u16 vid,
>  			      bool pvid,
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> index 1c88120eb291a..bcb4db76b75cd 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> @@ -297,6 +297,7 @@ static void lan966x_port_bridge_leave(struct lan966x_port *port,
>  	lan966x_vlan_port_set_vlan_aware(port, false);
>  	lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
>  	lan966x_vlan_port_apply(port);
> +	lan966x_vlan_port_rew_host(port);
>  }
>  
>  int lan966x_port_changeupper(struct net_device *dev,
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> index fa34a739c748e..f158ec6ab10cc 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> @@ -149,6 +149,27 @@ void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
>  	port->vlan_aware = vlan_aware;
>  }
>  
> +/* When the interface is in host mode, the interface should not be vlan aware
> + * but it should insert all the tags that it gets from the network stack.
> + * The tags are no in the data of the frame but actually in the skb and the ifh
                   not
> + * is confiured already to get this tag. So what we need to do is to update the
         configured
> + * rewriter to insert the vlan tag for all frames which have a vlan tag
> + * different than 0.

Just to be extra clear, the doc seems to say that

	REW_TAG_CFG_TAG_CFG_SET(1);

means "Tag all frames, except when VID=PORT_VLAN_CFG.PORT_VID or
VID=0."

Another setting for these bits are "Tag all frames except when VID=0",
which is what you document in the above comment.

In this case, is there any chance that it would make a difference ?

> + */
> +void lan966x_vlan_port_rew_host(struct lan966x_port *port)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +	u32 val;
> +
> +	/* Tag all frames except when VID == DEFAULT_VLAN */
> +	val = REW_TAG_CFG_TAG_CFG_SET(1);
> +
> +	/* Update only some bits in the register */
> +	lan_rmw(val,
> +		REW_TAG_CFG_TAG_CFG,
> +		lan966x, REW_TAG_CFG(port->chip_port));
> +}
> +
>  void lan966x_vlan_port_apply(struct lan966x_port *port)
>  {
>  	struct lan966x *lan966x = port->lan966x;

Sorry for the typo nitpicking, but with that :

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime
Re: [PATCH net] net: lan966x: Make sure to insert the vlan tags also in host mode
Posted by Horatiu Vultur 6 months, 3 weeks ago 
The 05/27/2025 10:37, Maxime Chevallier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello Horatiu,

Hi Maxime,

> 
> On Tue, 27 May 2025 09:08:50 +0200
> Horatiu Vultur <horatiu.vultur@microchip.com> wrote:
> 
> > When running these commands on DUT (and similar at the other end)
> > ip link set dev eth0 up
> > ip link add link eth0 name eth0.10 type vlan id 10
> > ip addr add 10.0.0.1/24 dev eth0.10
> > ip link set dev eth0.10 up
> > ping 10.0.0.2/24
> >
> > The ping will fail.
> >
> > The reason why is failing is because, the network interfaces for lan966x
> > have a flag saying that the HW can insert the vlan tags into the
> > frames(NETIF_F_HW_VLAN_CTAG_TX). Meaning that the frames that are
> > transmitted don't have the vlan tag inside the skb data, but they have
> > it inside the skb. We already get that vlan tag and put it in the IFH
> > but the problem is that we don't configure the HW to rewrite the frame
> > when the interface is in host mode.
> > The fix consists in actually configuring the HW to insert the vlan tag
> > if it is different than 0.
> >
> > Fixes: 6d2c186afa5d ("net: lan966x: Add vlan support.")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../ethernet/microchip/lan966x/lan966x_main.c |  1 +
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  1 +
> >  .../microchip/lan966x/lan966x_switchdev.c     |  1 +
> >  .../ethernet/microchip/lan966x/lan966x_vlan.c | 21 +++++++++++++++++++
> >  4 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > index 427bdc0e4908c..7001584f1b7a6 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > @@ -879,6 +879,7 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
> >       lan966x_vlan_port_set_vlan_aware(port, 0);
> >       lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
> >       lan966x_vlan_port_apply(port);
> > +     lan966x_vlan_port_rew_host(port);
> >
> >       return 0;
> >  }
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index 1f9df67f05044..4f75f06883693 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > @@ -497,6 +497,7 @@ void lan966x_vlan_port_apply(struct lan966x_port *port);
> >  bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
> >  void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
> >                                     bool vlan_aware);
> > +void lan966x_vlan_port_rew_host(struct lan966x_port *port);
> >  int lan966x_vlan_port_set_vid(struct lan966x_port *port,
> >                             u16 vid,
> >                             bool pvid,
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > index 1c88120eb291a..bcb4db76b75cd 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > @@ -297,6 +297,7 @@ static void lan966x_port_bridge_leave(struct lan966x_port *port,
> >       lan966x_vlan_port_set_vlan_aware(port, false);
> >       lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
> >       lan966x_vlan_port_apply(port);
> > +     lan966x_vlan_port_rew_host(port);
> >  }
> >
> >  int lan966x_port_changeupper(struct net_device *dev,
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> > index fa34a739c748e..f158ec6ab10cc 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> > @@ -149,6 +149,27 @@ void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
> >       port->vlan_aware = vlan_aware;
> >  }
> >
> > +/* When the interface is in host mode, the interface should not be vlan aware
> > + * but it should insert all the tags that it gets from the network stack.
> > + * The tags are no in the data of the frame but actually in the skb and the ifh
>                    not
> > + * is confiured already to get this tag. So what we need to do is to update the
>          configured
> > + * rewriter to insert the vlan tag for all frames which have a vlan tag
> > + * different than 0.

Well spotted!

> 
> Just to be extra clear, the doc seems to say that
> 
>         REW_TAG_CFG_TAG_CFG_SET(1);
> 
> means "Tag all frames, except when VID=PORT_VLAN_CFG.PORT_VID or
> VID=0."
> 
> Another setting for these bits are "Tag all frames except when VID=0",
> which is what you document in the above comment.
> 
> In this case, is there any chance that it would make a difference ?

I don't see how will this make a difference but I will update it to set
the value of 2 which means VID=0 and also update the comment.

> 
> > + */
> > +void lan966x_vlan_port_rew_host(struct lan966x_port *port)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +     u32 val;
> > +
> > +     /* Tag all frames except when VID == DEFAULT_VLAN */
> > +     val = REW_TAG_CFG_TAG_CFG_SET(1);
> > +
> > +     /* Update only some bits in the register */
> > +     lan_rmw(val,
> > +             REW_TAG_CFG_TAG_CFG,
> > +             lan966x, REW_TAG_CFG(port->chip_port));
> > +}
> > +
> >  void lan966x_vlan_port_apply(struct lan966x_port *port)
> >  {
> >       struct lan966x *lan966x = port->lan966x;
> 
> Sorry for the typo nitpicking, but with that :
> 
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> 
> Maxime

-- 
/Horatiu

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.