linux-kernel - RE: [PATCH net 1/2] net: ethernet: oa_tc6: fix infinite loop error when tx credits becomes 0

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: <CO1PR11MB5089671D438D9CE392B1BB3CD6222@CO1PR11MB5089.namprd11.prod.outlook.com>
Date: Thu, 21 Nov 2024 19:13:12 +0000
From: "Keller, Jacob E" <jacob.e.keller@...el.com>
To: "Parthiban.Veerasooran@...rochip.com"
	<Parthiban.Veerasooran@...rochip.com>
CC: "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH net 1/2] net: ethernet: oa_tc6: fix infinite loop error
 when tx credits becomes 0



> -----Original Message-----
> From: Parthiban.Veerasooran@...rochip.com
> <Parthiban.Veerasooran@...rochip.com>
> Sent: Wednesday, November 20, 2024 8:10 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: andrew+netdev@...n.ch; davem@...emloft.net; edumazet@...gle.com;
> kuba@...nel.org; pabeni@...hat.com; Parthiban.Veerasooran@...rochip.com;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> UNGLinuxDriver@...rochip.com
> Subject: Re: [PATCH net 1/2] net: ethernet: oa_tc6: fix infinite loop error when tx
> credits becomes 0
> 
> 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@...rochip.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#L11
> 14
> 
> 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

Yep, my eyes did not catch the lack of a +.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>