| Message ID | 20241120135142.586845-2-parthiban.veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes on the OPEN Alliance TC6 10BASE-T1x MAC-PHY support generic lib | expand |
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 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 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())
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()) >
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()); if (kthread_should_stop())
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(-)