netdev - Re: [PATCH] net: ks8851: Fix potential TX stall after interface reopen

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

Message-ID: <f62b1946-3b5e-475f-8f28-8d44dee28bd8@intel.com>
Date: Thu, 11 Jul 2024 09:32:02 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Ronald Wahl <rwahl@....de>
CC: Ronald Wahl <ronald.wahl@...itan.com>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman
	<horms@...nel.org>, <netdev@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [PATCH] net: ks8851: Fix potential TX stall after interface
 reopen



On 7/10/2024 5:20 PM, Ronald Wahl wrote:
> On 11.07.24 01:48, Jacob Keller wrote:
>>
>>
>> On 7/9/2024 12:58 PM, Ronald Wahl wrote:
>>> From: Ronald Wahl <ronald.wahl@...itan.com>
>>>
>>> The amount of TX space in the hardware buffer is tracked in the tx_space
>>> variable. The initial value is currently only set during driver probing.
>>>
>>> After closing the interface and reopening it the tx_space variable has
>>> the last value it had before close. If it is smaller than the size of
>>> the first send packet after reopeing the interface the queue will be
>>> stopped. The queue is woken up after receiving a TX interrupt but this
>>> will never happen since we did not send anything.
>>>
>>> This commit moves the initialization of the tx_space variable to the
>>> ks8851_net_open function right before starting the TX queue. Also query
>>> the value from the hardware instead of using a hard coded value.
>>>
>>> Only the SPI chip variant is affected by this issue because only this
>>> driver variant actually depends on the tx_space variable in the xmit
>>> function.
>>
>> I'm curious if this dependency could be removed?
> 
> I don't think so.
> 
> The driver must ensure not to write too much data to the hardware so we
> need a precise accounting of how much we can write. In the original
> driver code for the SPI variant this was broken and repaired in
> 3dc5d4454545 ("net: ks8851: Fix TX stall caused by TX buffer overrun").
> Unfortunately we required some rounds of bug fixing to get it finally
> working without any issues. Hopefully this was the last change in that
> regard. :-)
> 
> If you ask why only the SPI version is affected then the answer is that
> for the parallel interface chip there is no internal driver queuing,
> i.e. it writes a single packet per xmit call. Not sure if this can also
> overrun the hardware buffer if the receiver throttles via flow control.
> Since I do not own this chip variant I cannot test this. In the end that
> could even mean that we would need the accounting for the parallel chip
> code as well.
> 
> - ron
> 

That explains why only the one variation has this value.

Thanks!