netdev - Re: [PATCH V5 net-next 06/11] net: hibmcge: Implement .ndo_start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <7822930d-1331-4631-9d7e-bd37a40f44bd@lunn.ch>
Date: Thu, 29 Aug 2024 15:16:11 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jijie Shao <shaojijie@...wei.com>
Cc: netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH V5 net-next 06/11] net: hibmcge: Implement
 .ndo_start_xmit function

On Thu, Aug 29, 2024 at 08:55:35PM +0800, Jijie Shao wrote:
> 
> on 2024/8/29 11:02, Andrew Lunn wrote:
> > On Thu, Aug 29, 2024 at 10:32:33AM +0800, Jijie Shao wrote:
> > > on 2024/8/29 9:41, Jakub Kicinski wrote:
> > > > On Tue, 27 Aug 2024 21:14:50 +0800 Jijie Shao wrote:
> > > > > @@ -111,6 +135,14 @@ static int hbg_init(struct hbg_priv *priv)
> > > > >    	if (ret)
> > > > >    		return ret;
> > > > > +	ret = hbg_txrx_init(priv);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > You allocate buffers on the probe path?
> > > > The queues and all the memory will be sitting allocated but unused if
> > > > admin does ip link set dev $eth0 down?
> > > The network port has only one queue and
> > > the TX queue depth is 64, RX queue depth is 127.
> > > so it doesn't take up much memory.
> > > 
> > > Also, I plan to add the self_test feature in a future patch.
> > > If I don't allocate buffers on the probe path,
> > > I won't be able to do a loopback test if the network port goes down unexpectedly.
> > When you come to implement ethtool --set-ring, you will need to
> > allocate and free the ring buffers outside of probe.
> > 
> > 	Andrew
> 
> We discussed this internally, and now we have a problem that we can't solve:
> 
> After allocate ring buffers, the driver will writes the address to the hardware FIFO
> for receiving packets.
> 
> However, FIFO does not have a separate interface to clear ring buffers address.
> 
> If ring buffers can be allocated and released on other paths,
> the driver buffer address is inconsistent with the hardware buffer address.
> As a result, packet receiving is abnormal. The fault is rectified only
> after the buffers of a queue depth are used up and new buffers are allocated for.

If the hardware is designed correctly, there should be away to tell
the hardware to stop receiving packets. Often there is then a way to
know it has actually stopped, and all in flight DMA transfers are
complete. Otherwise, you can make a guess at how long the worse case
DMA transfer takes, maybe a jumbo packet at 10Mbps, and simply sleep
that long. It is then safe to allocate new ring buffers, swap the
pointer, and then free the old.

You probably even have this code already in u-boot. You cannot jump
into the kernel without first ensuring the device has stopped DMAing
packets.

> Actually, the buffer will be released during FLR reset and are allocated again after reset.
> In this case, the FIFO are cleared. Therefore, driver writes the ring buffer address
> to the hardware again to ensure that the driver address is the same as the hardware address.
> 
> If we do an FLR every time we allocate ring buffers on other path, It is expensive.

It does not matter if it is expensive. This is not hot path. ethtool
--set-ring is only going to be called once, maybe twice before the
machine is shut down. So we generally don't care about the cost.

	Andrew