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-prev] [day] [month] [year] [list]

Message-ID: <00c291f9-4d41-464a-a583-a24aeabfef85@huawei.com>
Date: Fri, 30 Aug 2024 20:27:39 +0800
From: Jijie Shao <shaojijie@...wei.com>
To: Andrew Lunn <andrew@...n.ch>
CC: <shaojijie@...wei.com>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH V5 net-next 06/11] net: hibmcge: Implement .ndo_start_xmit
 function


on 2024/8/29 21:16, Andrew Lunn wrote:
> 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

I have removed the alloc ring buffer from the probe path in v6.
After the test, the network port can work normally after being repeatedly down and up.

	Thanks，
	Jijie Shao