Re: [PATCH net 1/2] net: ethernet: oa_tc6: fix infinite loop error when tx credits becomes 0

[<Parthiban.Veerasooran@microchip.com>]

Hi Jacob Keller,

Thanks for the review.

On 21/11/24 1:24 am, Jacob Keller wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 11/20/2024 5:51 AM, Parthiban Veerasooran wrote:
>> SPI thread wakes up to perform SPI transfer whenever there is an TX skb
>> from n/w stack or interrupt from MAC-PHY. Ethernet frame from TX skb is
>> transferred based on the availability tx credits in the MAC-PHY which is
>> reported from the previous SPI transfer. Sometimes there is a possibility
>> that TX skb is available to transmit but there is no tx credits from
>> MAC-PHY. In this case, there will not be any SPI transfer but the thread
>> will be running in an endless loop until tx credits available again.
>>
>> So checking the availability of tx credits along with TX skb will prevent
>> the above infinite loop. When the tx credits available again that will be
>> notified through interrupt which will trigger the SPI transfer to get the
>> available tx credits.
>>
>> Fixes: 53fbde8ab21e ("net: ethernet: oa_tc6: implement transmit path to transfer tx ethernet frames")
>> Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
>> ---
>>   drivers/net/ethernet/oa_tc6.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
>> index f9c0dcd965c2..4c8b0ca922b7 100644
>> --- a/drivers/net/ethernet/oa_tc6.c
>> +++ b/drivers/net/ethernet/oa_tc6.c
>> @@ -1111,8 +1111,9 @@ static int oa_tc6_spi_thread_handler(void *data)
>>                /* This kthread will be waken up if there is a tx skb or mac-phy
>>                 * interrupt to perform spi transfer with tx chunks.
>>                 */
>> -             wait_event_interruptible(tc6->spi_wq, tc6->waiting_tx_skb ||
>> -                                      tc6->int_flag ||
>> +             wait_event_interruptible(tc6->spi_wq, tc6->int_flag ||
>> +                                      (tc6->waiting_tx_skb &&
>> +                                      tc6->tx_credits) ||
>>                                         kthread_should_stop());
>>
> 
> Ok, so previously we check:
> 
> waiting_tx_skb || int_flag
Previously we checked kthread_should_stop also. Previously it was,

waiting_tx_skb || int_flag || kthread_should_stop

Please refer the below link,

https://elixir.bootlin.com/linux/v6.12/source/drivers/net/ethernet/oa_tc6.c#L1114

Now we only added tx_credits with waiting_tx_skb. Hope this clarifies?
> 
> Now we check:
> 
> int_flag || (waiting_tx_skb && tx_credits) || kthread_should_stop.
> 
> We didn't check kthread_should_stop before and this isn't mentioned in
> the commit message, (or at least its not clear to me).
> 
> Whats the purpose behind that? I guess you want to wake up immediately
> when kthread_should_stop() so that we can shutdown the kthread ASAP? Is
> the condition "waiting_tx_skb && tx_credits" such that we might
> otherwise not wake up, but with just "waiting_tx_skb" we definitely wake
> up and stop earlier?
I think there is a misunderstanding here. Hope the above reply clarifies 
this? If not please let me know what do you expect?

Best regards,
Parthiban V
> 
> I think that change makes sense but I don't like that it was not called
> out in the commit message.
> 
> The code seems correct to me otherwise.
> 
>>                if (kthread_should_stop())
>