| Message ID | 20221023184838.4128061-4-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: lan966x: Fixes for when MTU is changed | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 31 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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.
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.
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; }
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(-)