[PATCH net 0/3] net: lan966x: Fixes for when MTU is changed

[PATCH net 0/3] net: lan966x: Fixes for when MTU is changed

[Horatiu Vultur]

There were multiple problems in different parts of the driver when
the MTU was changed.
The first problem was that the HW was missing to configure the correct
value, it was missing ETH_HLEN and ETH_FCS_LEN. The second problem was
when vlan filtering was enabled/disabled, the MRU was not adjusted
corretly. While the last issue was that the FDMA was calculated wrongly
the correct maximum MTU.

Horatiu Vultur (3):
  net: lan966x: Fix the MTU calculation
  net: lan966x: Adjust maximum frame size when vlan is enabled/disabled
  net: lan966x: Fix FDMA when MTU is changed

 .../net/ethernet/microchip/lan966x/lan966x_fdma.c |  7 +++++--
 .../net/ethernet/microchip/lan966x/lan966x_main.c |  4 ++--
 .../net/ethernet/microchip/lan966x/lan966x_main.h |  2 ++
 .../net/ethernet/microchip/lan966x/lan966x_regs.h | 15 +++++++++++++++
 .../net/ethernet/microchip/lan966x/lan966x_vlan.c |  6 ++++++
 5 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.38.0

[PATCH net 1/3] net: lan966x: Fix the MTU calculation

[Horatiu Vultur]

When the MTU was changed, the lan966x didn't take in consideration
the L2 header and the FCS. So the HW was configured with a smaller
value than what was desired. Therefore the correct value to configure
the HW would be new_mtu + ETH_HLEN + ETH_FCS_LEN.
The vlan tag is not considered here, because at the time when the
blamed commit was added, there was no vlan filtering support. The
vlan fix will be part of the next patch.

Fixes: d28d6d2e37d1 ("net: lan966x: add port module support")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 2 +-
 drivers/net/ethernet/microchip/lan966x/lan966x_main.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index be2fd030cccbe..b3070c3fcad0a 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -386,7 +386,7 @@ static int lan966x_port_change_mtu(struct net_device *dev, int new_mtu)
 	int old_mtu = dev->mtu;
 	int err;
 
-	lan_wr(DEV_MAC_MAXLEN_CFG_MAX_LEN_SET(new_mtu),
+	lan_wr(DEV_MAC_MAXLEN_CFG_MAX_LEN_SET(LAN966X_HW_MTU(new_mtu)),
 	       lan966x, DEV_MAC_MAXLEN_CFG(port->chip_port));
 	dev->mtu = new_mtu;
 
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 9656071b8289e..4ec33999e4df6 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -26,6 +26,8 @@
 #define LAN966X_BUFFER_MEMORY		(160 * 1024)
 #define LAN966X_BUFFER_MIN_SZ		60
 
+#define LAN966X_HW_MTU(mtu)		((mtu) + ETH_HLEN + ETH_FCS_LEN)
+
 #define PGID_AGGR			64
 #define PGID_SRC			80
 #define PGID_ENTRIES			89
-- 
2.38.0

[PATCH net 2/3] net: lan966x: Adjust maximum frame size when vlan is enabled/disabled

[Horatiu Vultur]

When vlan filtering is enabled/disabled, it is required to adjust the
maximum received frame size that it can received. When vlan filtering is
enabled, it would all to receive extra 4 bytes, that are the vlan tag.
So the maximum frame size would be 1522 with a vlan tag. If vlan
filtering is disabled then the maximum frame size would be 1518
regardless if there is or not a vlan tag.

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

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
index 1d90b93dd417a..fb5087fef22e1 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
@@ -585,6 +585,21 @@ enum lan966x_target {
 #define DEV_MAC_MAXLEN_CFG_MAX_LEN_GET(x)\
 	FIELD_GET(DEV_MAC_MAXLEN_CFG_MAX_LEN, x)
 
+/*      DEV:MAC_CFG_STATUS:MAC_TAGS_CFG */
+#define DEV_MAC_TAGS_CFG(t)       __REG(TARGET_DEV, t, 8, 28, 0, 1, 44, 12, 0, 1, 4)
+
+#define DEV_MAC_TAGS_CFG_VLAN_DBL_AWR_ENA        BIT(1)
+#define DEV_MAC_TAGS_CFG_VLAN_DBL_AWR_ENA_SET(x)\
+	FIELD_PREP(DEV_MAC_TAGS_CFG_VLAN_DBL_AWR_ENA, x)
+#define DEV_MAC_TAGS_CFG_VLAN_DBL_AWR_ENA_GET(x)\
+	FIELD_GET(DEV_MAC_TAGS_CFG_VLAN_DBL_AWR_ENA, x)
+
+#define DEV_MAC_TAGS_CFG_VLAN_AWR_ENA            BIT(0)
+#define DEV_MAC_TAGS_CFG_VLAN_AWR_ENA_SET(x)\
+	FIELD_PREP(DEV_MAC_TAGS_CFG_VLAN_AWR_ENA, x)
+#define DEV_MAC_TAGS_CFG_VLAN_AWR_ENA_GET(x)\
+	FIELD_GET(DEV_MAC_TAGS_CFG_VLAN_AWR_ENA, x)
+
 /*      DEV:MAC_CFG_STATUS:MAC_IFG_CFG */
 #define DEV_MAC_IFG_CFG(t)        __REG(TARGET_DEV, t, 8, 28, 0, 1, 44, 20, 0, 1, 4)
 
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
index 8d7260cd7da9c..3c44660128dae 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
@@ -169,6 +169,12 @@ void lan966x_vlan_port_apply(struct lan966x_port *port)
 		ANA_VLAN_CFG_VLAN_POP_CNT,
 		lan966x, ANA_VLAN_CFG(port->chip_port));
 
+	lan_rmw(DEV_MAC_TAGS_CFG_VLAN_AWR_ENA_SET(port->vlan_aware) |
+		DEV_MAC_TAGS_CFG_VLAN_DBL_AWR_ENA_SET(port->vlan_aware),
+		DEV_MAC_TAGS_CFG_VLAN_AWR_ENA |
+		DEV_MAC_TAGS_CFG_VLAN_DBL_AWR_ENA,
+		lan966x, DEV_MAC_TAGS_CFG(port->chip_port));
+
 	/* Drop frames with multicast source address */
 	val = ANA_DROP_CFG_DROP_MC_SMAC_ENA_SET(1);
 	if (port->vlan_aware && !pvid)
-- 
2.38.0

[PATCH net 3/3] net: lan966x: Fix FDMA when MTU is changed

[Horatiu Vultur]

When MTU is changed, FDMA is required to calculate what is the maximum
size of the frame that it can received. So it can calculate what is the
page order needed to allocate for the received frames.
The first problem was that, when the max MTU was calculated it was
reading the value from dev and not from HW, so in this way it was
missing L2 header + the FCS.
The other problem was that once the skb is created using
__build_skb_around, it would reserve some space for skb_shared_info.
So if we received a frame which size is at the limit of the page order
then the creating will failed because it would not have space to put all
the data.

Fixes: 2ea1cbac267e ("net: lan966x: Update FDMA to change MTU.")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 7 +++++--
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index a42035cec611c..5a5603f9e9fd3 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -668,12 +668,14 @@ static int lan966x_fdma_get_max_mtu(struct lan966x *lan966x)
 	int i;
 
 	for (i = 0; i < lan966x->num_phys_ports; ++i) {
+		struct lan966x_port *port;
 		int mtu;
 
-		if (!lan966x->ports[i])
+		port = lan966x->ports[i];
+		if (!port)
 			continue;
 
-		mtu = lan966x->ports[i]->dev->mtu;
+		mtu = lan_rd(lan966x, DEV_MAC_MAXLEN_CFG(port->chip_port));
 		if (mtu > max_mtu)
 			max_mtu = mtu;
 	}
@@ -733,6 +735,7 @@ int lan966x_fdma_change_mtu(struct lan966x *lan966x)
 
 	max_mtu = lan966x_fdma_get_max_mtu(lan966x);
 	max_mtu += IFH_LEN * sizeof(u32);
+	max_mtu += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
 	if (round_up(max_mtu, PAGE_SIZE) / PAGE_SIZE - 1 ==
 	    lan966x->rx.page_order)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index b3070c3fcad0a..20ee5b28f70a5 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -395,7 +395,7 @@ static int lan966x_port_change_mtu(struct net_device *dev, int new_mtu)
 
 	err = lan966x_fdma_change_mtu(lan966x);
 	if (err) {
-		lan_wr(DEV_MAC_MAXLEN_CFG_MAX_LEN_SET(old_mtu),
+		lan_wr(DEV_MAC_MAXLEN_CFG_MAX_LEN_SET(LAN966X_HW_MTU(old_mtu)),
 		       lan966x, DEV_MAC_MAXLEN_CFG(port->chip_port));
 		dev->mtu = old_mtu;
 	}
-- 
2.38.0

Re: [PATCH net 3/3] net: lan966x: Fix FDMA when MTU is changed

[Jakub Kicinski]

On Sun, 23 Oct 2022 20:48:38 +0200 Horatiu Vultur wrote:
> +		mtu = lan_rd(lan966x, DEV_MAC_MAXLEN_CFG(port->chip_port));

I'm slightly confused about the vlan situation, does this return the
vlan hdr length when enabled? or vlans are always communicated
out-of-band so don't need to count here?

Unrelated potential issue I spotted:

        skb = build_skb(page_address(page), PAGE_SIZE << rx->page_order);       
        if (unlikely(!skb))                                                     
                goto unmap_page;                                                
                                                                                
        dma_unmap_single(lan966x->dev, (dma_addr_t)db->dataptr,                 
                         FDMA_DCB_STATUS_BLOCKL(db->status),                    
                         DMA_FROM_DEVICE);  

Are you sure it's legal to unmap with a different length than you
mapped? Seems questionable - you can unmap & ask the unmap to skip
the sync, then sync manually only the part that you care about - if you
want to avoid full sync.

What made me pause here is that you build_skb() and then unmap which is
also odd because normally (if unmap was passed full len) unmap could
wipe what build_skb() initialized.

Re: [PATCH net 3/3] net: lan966x: Fix FDMA when MTU is changed

[Horatiu Vultur]

The 10/25/2022 19:34, Jakub Kicinski wrote:
> 
> On Sun, 23 Oct 2022 20:48:38 +0200 Horatiu Vultur wrote:
> > +             mtu = lan_rd(lan966x, DEV_MAC_MAXLEN_CFG(port->chip_port));
> 
> I'm slightly confused about the vlan situation, does this return the
> vlan hdr length when enabled?

No it doesn't.

> or vlans are always communicated out-of-band so don't need to count here?

Actually, the vlan hdr is actually in bound.
So there is a mistake in the code. The function lan966x_fdma_change_mtu
needs to take in consideration also the vlan header. It can have up to
two tags so it is needed twice.
Because if the vlan header is not taken in consideration, then there can
be a skb_panic when the frame is created in case there is no space
enough to put also the vlan header.

This can be reproduced with the following commands:
ip link set dev eth0 up
ip link set dev eth0 mtu 3794
ip link add name br0 type bridge vlan filtering
ip link set dev eth0 master br0
ip link set dev br0 up

Now send a frame with a the total size of 3816(contains ETH_HLEN + FCS +
VLAN_HELN).

> 
> Unrelated potential issue I spotted:
> 
>         skb = build_skb(page_address(page), PAGE_SIZE << rx->page_order);
>         if (unlikely(!skb))
>                 goto unmap_page;
> 
>         dma_unmap_single(lan966x->dev, (dma_addr_t)db->dataptr,
>                          FDMA_DCB_STATUS_BLOCKL(db->status),
>                          DMA_FROM_DEVICE);
> 
> Are you sure it's legal to unmap with a different length than you
> mapped? Seems questionable - you can unmap & ask the unmap to skip
> the sync, then sync manually only the part that you care about - if you
> want to avoid full sync.

That is really good observation. I have looked at some other drivers and
all of them actually unmap with the same length that they used when they
mapped the page.

But the order of the operations should be like this:

---
dma_sync_single_for_cpu(lan966x->dev, (dma_addr_t)db->dataptr,
			FDMA_DCB_STATUS_BLOCKL(db->status),
			DMA_FROM_DEVICE);
skb = build_skb(page_address(page), PAGE_SIZE << rx->page_order);
if (unlikely(!skb))
	goto unmap_page;

dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
		       PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
		       DMA_ATTR_SKIP_CPU_SYNC);
---

Because in this way, I get the data from HW, I build the skb with all
the initialization and then I unmapped without CPU sync because I have
already done that.

> 
> What made me pause here is that you build_skb() and then unmap which is
> also odd because normally (if unmap was passed full len) unmap could
> wipe what build_skb() initialized.

-- 
/Horatiu