Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 26 Oct 2022 20:35:27 +0200
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Jakub Kicinski <kuba@kernel.org>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <davem@davemloft.net>, <edumazet@google.com>, <pabeni@redhat.com>,
        <UNGLinuxDriver@microchip.com>
Subject: Re: [PATCH net 3/3] net: lan966x: Fix FDMA when MTU is changed
Message-ID: <20221026183527.z4zuihbk275gm4bi@soft-dev3-1>
References: <20221023184838.4128061-1-horatiu.vultur@microchip.com>
 <20221023184838.4128061-4-horatiu.vultur@microchip.com>
 <20221025193405.1c8f6e74@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20221025193405.1c8f6e74@kernel.org>
Precedence: bulk

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