[PATCH net 0/2] Fixes on the OPEN Alliance TC6 10BASE-T1x MAC-PHY support generic lib

[PATCH net 0/2] Fixes on the OPEN Alliance TC6 10BASE-T1x MAC-PHY support generic lib

[Parthiban Veerasooran]

This patch series contain the below fixes.

- Infinite loop error when tx credits becomes 0.
- Race condition between tx skb reference pointers.


Parthiban Veerasooran (2):
  net: ethernet: oa_tc6: fix infinite loop error when tx credits becomes
    0
  net: ethernet: oa_tc6: fix tx skb race condition between reference
    pointers

 drivers/net/ethernet/oa_tc6.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)


base-commit: dd7207838d38780b51e4690ee508ab2d5057e099
-- 
2.34.1

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

[Parthiban Veerasooran]

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());
 
 		if (kthread_should_stop())
-- 
2.34.1

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

[Jacob Keller]

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())

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

[Parthiban.Veerasooran]

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())
> 

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

[Keller, Jacob E]

> -----Original Message-----
> From: Parthiban.Veerasooran@microchip.com
> <Parthiban.Veerasooran@microchip.com>
> Sent: Wednesday, November 20, 2024 8:10 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; Parthiban.Veerasooran@microchip.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> UNGLinuxDriver@microchip.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@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#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@intel.com>

[PATCH net 2/2] net: ethernet: oa_tc6: fix tx skb race condition between reference pointers

[Parthiban Veerasooran]

There are two skb pointers to manage tx skb's enqueued from n/w stack.
waiting_tx_skb pointer points to the tx skb which needs to be processed
and ongoing_tx_skb pointer points to the tx skb which is being processed.

SPI thread prepares the tx data chunks from the tx skb pointed by the
ongoing_tx_skb pointer. When the tx skb pointed by the ongoing_tx_skb is
processed, the tx skb pointed by the waiting_tx_skb is assigned to
ongoing_tx_skb and the waiting_tx_skb pointer is assigned with NULL.
Whenever there is a new tx skb from n/w stack, it will be assigned to
waiting_tx_skb pointer if it is NULL. Enqueuing and processing of a tx skb
handled in two different threads.

Consider a scenario where the SPI thread processed an ongoing_tx_skb and
it assigns next tx skb from waiting_tx_skb pointer to ongoing_tx_skb
pointer without doing any NULL check. At this time, if the waiting_tx_skb
pointer is NULL then ongoing_tx_skb pointer is also assigned with NULL.
After that, if a new tx skb is assigned to waiting_tx_skb pointer by the
n/w stack and there is a chance to overwrite the tx skb pointer with NULL
in the SPI thread. Finally one of the tx skb will be left as unhandled,
resulting packet missing and memory leak.

To overcome the above issue, check waiting_tx_skb pointer is not NULL
along with ongoing_tx_skb pointer's NULL check before proceeding to assign
the tx skb from waiting_tx_skb pointer to ongoing_tx_skb pointer.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 4c8b0ca922b7..e1e7c6e07966 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -1003,7 +1003,7 @@ static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6)
 	 */
 	for (used_tx_credits = 0; used_tx_credits < tc6->tx_credits;
 	     used_tx_credits++) {
-		if (!tc6->ongoing_tx_skb) {
+		if (!tc6->ongoing_tx_skb && tc6->waiting_tx_skb) {
 			tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
 			tc6->waiting_tx_skb = NULL;
 		}
-- 
2.34.1

Re: [PATCH net 2/2] net: ethernet: oa_tc6: fix tx skb race condition between reference pointers

[Jacob Keller]

On 11/20/2024 5:51 AM, Parthiban Veerasooran wrote:
> There are two skb pointers to manage tx skb's enqueued from n/w stack.
> waiting_tx_skb pointer points to the tx skb which needs to be processed
> and ongoing_tx_skb pointer points to the tx skb which is being processed.
> 
> SPI thread prepares the tx data chunks from the tx skb pointed by the
> ongoing_tx_skb pointer. When the tx skb pointed by the ongoing_tx_skb is
> processed, the tx skb pointed by the waiting_tx_skb is assigned to
> ongoing_tx_skb and the waiting_tx_skb pointer is assigned with NULL.
> Whenever there is a new tx skb from n/w stack, it will be assigned to
> waiting_tx_skb pointer if it is NULL. Enqueuing and processing of a tx skb
> handled in two different threads.
> 
> Consider a scenario where the SPI thread processed an ongoing_tx_skb and
> it assigns next tx skb from waiting_tx_skb pointer to ongoing_tx_skb
> pointer without doing any NULL check. At this time, if the waiting_tx_skb
> pointer is NULL then ongoing_tx_skb pointer is also assigned with NULL.
> After that, if a new tx skb is assigned to waiting_tx_skb pointer by the
> n/w stack and there is a chance to overwrite the tx skb pointer with NULL
> in the SPI thread. Finally one of the tx skb will be left as unhandled,
> resulting packet missing and memory leak.
> 
> To overcome the above issue, check waiting_tx_skb pointer is not NULL
> along with ongoing_tx_skb pointer's NULL check before proceeding to assign
> the tx skb from waiting_tx_skb pointer to ongoing_tx_skb pointer.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 4c8b0ca922b7..e1e7c6e07966 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -1003,7 +1003,7 @@ static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6)
>  	 */
>  	for (used_tx_credits = 0; used_tx_credits < tc6->tx_credits;
>  	     used_tx_credits++) {
> -		if (!tc6->ongoing_tx_skb) {
> +		if (!tc6->ongoing_tx_skb && tc6->waiting_tx_skb) {
>  			tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
>  			tc6->waiting_tx_skb = NULL;

It is unclear to me how this additional check completely resolves race
conditions? Is there some other locking or synchronization such the
second thread cannot have updated waiting_tx_skb either prior or after
this check?

This feels like you want some sort of atomic exchange operation...

>  		}

Re: [PATCH net 2/2] net: ethernet: oa_tc6: fix tx skb race condition between reference pointers

[Parthiban.Veerasooran]

Hi Jacob Keller,

On 21/11/24 1:27 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:
>> There are two skb pointers to manage tx skb's enqueued from n/w stack.
>> waiting_tx_skb pointer points to the tx skb which needs to be processed
>> and ongoing_tx_skb pointer points to the tx skb which is being processed.
>>
>> SPI thread prepares the tx data chunks from the tx skb pointed by the
>> ongoing_tx_skb pointer. When the tx skb pointed by the ongoing_tx_skb is
>> processed, the tx skb pointed by the waiting_tx_skb is assigned to
>> ongoing_tx_skb and the waiting_tx_skb pointer is assigned with NULL.
>> Whenever there is a new tx skb from n/w stack, it will be assigned to
>> waiting_tx_skb pointer if it is NULL. Enqueuing and processing of a tx skb
>> handled in two different threads.
>>
>> Consider a scenario where the SPI thread processed an ongoing_tx_skb and
>> it assigns next tx skb from waiting_tx_skb pointer to ongoing_tx_skb
>> pointer without doing any NULL check. At this time, if the waiting_tx_skb
>> pointer is NULL then ongoing_tx_skb pointer is also assigned with NULL.
>> After that, if a new tx skb is assigned to waiting_tx_skb pointer by the
>> n/w stack and there is a chance to overwrite the tx skb pointer with NULL
>> in the SPI thread. Finally one of the tx skb will be left as unhandled,
>> resulting packet missing and memory leak.
>>
>> To overcome the above issue, check waiting_tx_skb pointer is not NULL
>> along with ongoing_tx_skb pointer's NULL check before proceeding to assign
>> the tx skb from waiting_tx_skb pointer to ongoing_tx_skb pointer.
>>
>> 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 | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
>> index 4c8b0ca922b7..e1e7c6e07966 100644
>> --- a/drivers/net/ethernet/oa_tc6.c
>> +++ b/drivers/net/ethernet/oa_tc6.c
>> @@ -1003,7 +1003,7 @@ static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6)
>>         */
>>        for (used_tx_credits = 0; used_tx_credits < tc6->tx_credits;
>>             used_tx_credits++) {
>> -             if (!tc6->ongoing_tx_skb) {
>> +             if (!tc6->ongoing_tx_skb && tc6->waiting_tx_skb) {
>>                        tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
>>                        tc6->waiting_tx_skb = NULL;
> 
> It is unclear to me how this additional check completely resolves race
> conditions? Is there some other locking or synchronization such the
> second thread cannot have updated waiting_tx_skb either prior or after
> this check?
> 
> This feels like you want some sort of atomic exchange operation...
Ok, thanks for your input. I think protecting this code section with a 
mutex lock will prevent this race condition. So the code will become 
like below,

if (!tc6->ongoing_tx_skb) {
	mutex_lock(&tx_skb_lock);
	tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
	tc6->waiting_tx_skb = NULL;
	mutex_unlock(&tx_skb_lock);
}

Hope this is what you expected right?

Best regards,
Parthiban V
> 
>>                }
>