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@xxxxxxxxxxx>
>>>
>>> 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!
