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

[PATCH net v2 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.

v1->v2:
- when calculating max frame possible to receive add also the vlan tags
  length

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 |  8 ++++++--
 .../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, 31 insertions(+), 4 deletions(-)

-- 
2.38.0

[PATCH net v2 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 v2 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 v2 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 | 8 ++++++--
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 2 +-
 2 files changed, 7 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..c235edd2b182a 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,8 @@ 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));
+	max_mtu += VLAN_HLEN * 2;
 
 	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 v2 0/3] net: lan966x: Fixes for when MTU is changed

[David Laight]

From: Horatiu Vultur
> Sent: 30 October 2022 21:37
> 
> 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.

IIRC all these lengths are 1514, 1518 and maybe 1522?
How long are the actual receive buffers?
I'd guess they have to be rounded up to a whole number of cache lines
(especially on non-coherent systems) so are probably 1536 bytes.

If driver does support 8k+ jumbo frames just set the hardware
frame length to match the receive buffer size.

There is no real need to exactly police the receive MTU.
There are definitely situations where the transmit MTU has
to be limited (eg to avoid ptmu blackholes) but where some
systems still send 'full sized' packets.

There is also the possibility of receiving PPPoE encapsulated
full sized ethernet packets.
I can remember how big that header is - something like 8 bytes.
There is no real reason to discard them if they'd fit in the buffer.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

['Horatiu Vultur']

The 10/31/2022 10:43, David Laight wrote:
> 
> From: Horatiu Vultur
> > Sent: 30 October 2022 21:37

Hi David,

> >
> > 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.
> 
> IIRC all these lengths are 1514, 1518 and maybe 1522?

And also 1526, if the frame has 2 vlan tags.

> How long are the actual receive buffers?
> I'd guess they have to be rounded up to a whole number of cache lines
> (especially on non-coherent systems) so are probably 1536 bytes.

The receive buffers can be different sizes, it can be up to 65k.
They are currently allign to page size.

> 
> If driver does support 8k+ jumbo frames just set the hardware
> frame length to match the receive buffer size.

In that case I should always allocate maximum frame size(65k) for all
regardless of the MTU?

> 
> There is no real need to exactly police the receive MTU.
> There are definitely situations where the transmit MTU has
> to be limited (eg to avoid ptmu blackholes) but where some
> systems still send 'full sized' packets.
> 
> There is also the possibility of receiving PPPoE encapsulated
> full sized ethernet packets.
> I can remember how big that header is - something like 8 bytes.
> There is no real reason to discard them if they'd fit in the buffer.
> 
>         David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

-- 
/Horatiu

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

[David Laight]

From: 'Horatiu Vultur'
> Sent: 31 October 2022 15:02
> 
> The 10/31/2022 10:43, David Laight wrote:
> >
> > From: Horatiu Vultur
> > > Sent: 30 October 2022 21:37
> 
> Hi David,
> 
> > >
> > > 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.
> >
> > IIRC all these lengths are 1514, 1518 and maybe 1522?
> 
> And also 1526, if the frame has 2 vlan tags.
> 
> > How long are the actual receive buffers?
> > I'd guess they have to be rounded up to a whole number of cache lines
> > (especially on non-coherent systems) so are probably 1536 bytes.
> 
> The receive buffers can be different sizes, it can be up to 65k.
> They are currently allign to page size.

Is that necessary?
I don't know where the buffers are allocated, but even 4k seems
a bit profligate for normal ethernet mtu.
If the page size if larger it is even sillier.

If the buffer is embedded in an skb you really want the skb
to be under 4k (I don't think a 1500 byte mtu can fit in 2k).

But you might as well tell the hardware the actual buffer length
(remember to allow for the crc and any alignment header).

> >
> > If driver does support 8k+ jumbo frames just set the hardware
> > frame length to match the receive buffer size.
> 
> In that case I should always allocate maximum frame size(65k) for all
> regardless of the MTU?

That would be very wasteful. I'd set the buffer large enough for
the mtu but let the hardware fill the entire buffer.

Allocating 64k buffers for big jumbo frames doesn't seem right.
If the mtu is 64k then kmalloc() will allocate 128k.
This is going to cause 'oddities' with small packets where
the 'true_size' is massively more than the data size.

Isn't there a scheme where you can create an skb from a page
list that contains fragments of the ethernet frame?
In which case I'd have thought you'd want to fill the ring
with page size buffers and then handle the hardware writing
a long frame to multiple buffers/descriptors.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

['Horatiu Vultur']

The 10/31/2022 15:27, David Laight wrote:
> 
> From: 'Horatiu Vultur'
> > Sent: 31 October 2022 15:02
> >
> > The 10/31/2022 10:43, David Laight wrote:
> > >
> > > From: Horatiu Vultur
> > > > Sent: 30 October 2022 21:37
> >
> > Hi David,
> >
> > > >
> > > > 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.
> > >
> > > IIRC all these lengths are 1514, 1518 and maybe 1522?
> >
> > And also 1526, if the frame has 2 vlan tags.
> >
> > > How long are the actual receive buffers?
> > > I'd guess they have to be rounded up to a whole number of cache lines
> > > (especially on non-coherent systems) so are probably 1536 bytes.
> >
> > The receive buffers can be different sizes, it can be up to 65k.
> > They are currently allign to page size.
> 
> Is that necessary?

HW requires to have the start of frame alligned to 128 bytes.

> I don't know where the buffers are allocated, but even 4k seems
> a bit profligate for normal ethernet mtu.
> If the page size if larger it is even sillier.

For lan966x the pages are allocated here [1]

> 
> If the buffer is embedded in an skb you really want the skb
> to be under 4k (I don't think a 1500 byte mtu can fit in 2k).
> 
> But you might as well tell the hardware the actual buffer length
> (remember to allow for the crc and any alignment header).

I am already doing that here [2]
And I need to do it for each frame it can received.

> 
> > >
> > > If driver does support 8k+ jumbo frames just set the hardware
> > > frame length to match the receive buffer size.
> >
> > In that case I should always allocate maximum frame size(65k) for all
> > regardless of the MTU?
> 
> That would be very wasteful.

Yes, I agree.

> I'd set the buffer large enough for the mtu but let the hardware fill
> the entire buffer.

I am not 100% sure I follow it. Can you expend on this a little bit?

> 
> Allocating 64k buffers for big jumbo frames doesn't seem right.
> If the mtu is 64k then kmalloc() will allocate 128k.
> This is going to cause 'oddities' with small packets where
> the 'true_size' is massively more than the data size.
> 
> Isn't there a scheme where you can create an skb from a page
> list that contains fragments of the ethernet frame?

Can I use '__skb_fill_page_desc'?

> In which case I'd have thought you'd want to fill the ring
> with page size buffers and then handle the hardware writing
> a long frame to multiple buffers/descriptors.

It might be a good idea. I need to look in more details about this.
Because it would change a little bit the logic on how the frames are
received and see how this will impact for frames under a page.
Also I was thinking next to use page_pool API, for which I send a patch
[3] but is deffered by this patch series.
But all these possible changes will need to go through net-next.

> 
>         David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

[1] https://elixir.bootlin.com/linux/v6.1-rc3/source/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c#L17
[2] https://elixir.bootlin.com/linux/v6.1-rc3/source/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c#L70
[3] https://lore.kernel.org/bpf/20221019135008.3281743-6-horatiu.vultur@microchip.com/

-- 
/Horatiu

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

[David Laight]

From: 'Horatiu Vultur'
> Sent: 01 November 2022 07:59
> 
> The 10/31/2022 15:27, David Laight wrote:
> >
> > From: 'Horatiu Vultur'
> > > Sent: 31 October 2022 15:02
> > >
> > > The 10/31/2022 10:43, David Laight wrote:
> > > >
> > > > From: Horatiu Vultur
> > > > > Sent: 30 October 2022 21:37
> > >
> > > Hi David,
> > >
> > > > >
> > > > > 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.
> > > >
> > > > IIRC all these lengths are 1514, 1518 and maybe 1522?
> > >
> > > And also 1526, if the frame has 2 vlan tags.
> > >
> > > > How long are the actual receive buffers?
> > > > I'd guess they have to be rounded up to a whole number of cache lines
> > > > (especially on non-coherent systems) so are probably 1536 bytes.
> > >
> > > The receive buffers can be different sizes, it can be up to 65k.
> > > They are currently allign to page size.
> >
> > Is that necessary?
> 
> HW requires to have the start of frame alligned to 128 bytes.

Not a real problem.
Even dma_alloc_coherent() guarantees alignment.

I'm not 100% sure of all the options of the Linux stack.
But for ~1500 byte mtu I'd have thought that receiving
directly into an skb would be best (1 page allocated for an skb).
For large mtu (and hardware receive coalescing) receiving
into pages is probably better - but not high order allocations.

...
> > If the buffer is embedded in an skb you really want the skb
> > to be under 4k (I don't think a 1500 byte mtu can fit in 2k).
> >
> > But you might as well tell the hardware the actual buffer length
> > (remember to allow for the crc and any alignment header).
> 
> I am already doing that here [2]
> And I need to do it for each frame it can received.

That is the length of the buffer.
Not the maximum frame length - which seems to be elsewhere.
I suspect that having the maximum frame length less than the
buffer size stops the driver having to handle long frames
that span multiple buffers.
(and very long frames that are longer than all the buffers!)

...
> > I'd set the buffer large enough for the mtu but let the hardware fill
> > the entire buffer.
> 
> I am not 100% sure I follow it. Can you expend on this a little bit?

At the moment I think the receive buffer descriptors have a length
of 4k. But you are setting another 'maximum frame length' register
to (eg) 1518 so that the hardware rejects long frames.
But you can set the 'maximum frame length' register to (just under)
4k so that longer frames are received ok but without the driver
having to worry about frames spanning multiple buffer fragments.

The network stack might choose to discard frames with an overlong mtu.
But that can be done after all the headers have been removed.

...
> But all these possible changes will need to go through net-next.

Indeed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

['Horatiu Vultur']

The 11/01/2022 09:03, David Laight wrote:
> > HW requires to have the start of frame alligned to 128 bytes.
> 
> Not a real problem.
> Even dma_alloc_coherent() guarantees alignment.
> 
> I'm not 100% sure of all the options of the Linux stack.
> But for ~1500 byte mtu I'd have thought that receiving
> directly into an skb would be best (1 page allocated for an skb).
> For large mtu (and hardware receive coalescing) receiving
> into pages is probably better - but not high order allocations.

But am I not doing this already? I allocate the page here [1] and then create
the skb here[2].

> 
> ...
> > > If the buffer is embedded in an skb you really want the skb
> > > to be under 4k (I don't think a 1500 byte mtu can fit in 2k).
> > >
> > > But you might as well tell the hardware the actual buffer length
> > > (remember to allow for the crc and any alignment header).
> >
> > I am already doing that here [2]
> > And I need to do it for each frame it can received.
> 
> That is the length of the buffer.
> Not the maximum frame length - which seems to be elsewhere.
> I suspect that having the maximum frame length less than the
> buffer size stops the driver having to handle long frames
> that span multiple buffers.
> (and very long frames that are longer than all the buffers!)
> 
> ...
> > > I'd set the buffer large enough for the mtu but let the hardware fill
> > > the entire buffer.
> >
> > I am not 100% sure I follow it. Can you expend on this a little bit?
> 
> At the moment I think the receive buffer descriptors have a length
> of 4k. But you are setting another 'maximum frame length' register
> to (eg) 1518 so that the hardware rejects long frames.

That is correct.

> But you can set the 'maximum frame length' register to (just under)
> 4k so that longer frames are received ok but without the driver
> having to worry about frames spanning multiple buffer fragments.

But this will not just put more load on CPU? In the way that I accept
long frames but then they will be discard by the CPU.
And I can do this in HW, because I know what is the maximum frame length
accepted on that interface.

> 
> The network stack might choose to discard frames with an overlong mtu.
> But that can be done after all the headers have been removed.
> 
> ...
> > But all these possible changes will need to go through net-next.
> 
> Indeed.
> 
>         David

[1] https://elixir.bootlin.com/linux/v6.1-rc3/source/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c#L17
[2] https://elixir.bootlin.com/linux/v6.1-rc3/source/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c#L417

> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

-- 
/Horatiu

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Sun, 30 Oct 2022 22:36:33 +0100 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v2,1/3] net: lan966x: Fix the MTU calculation
    https://git.kernel.org/netdev/net/c/486c29223016
  - [net,v2,2/3] net: lan966x: Adjust maximum frame size when vlan is enabled/disabled
    https://git.kernel.org/netdev/net/c/25f28bb1b4a7
  - [net,v2,3/3] net: lan966x: Fix FDMA when MTU is changed
    https://git.kernel.org/netdev/net/c/872ad758f9b7

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