[PATCH v2 0/2] Net driver bugs fix

[PATCH v2 0/2] Net driver bugs fix

[Elad Kanfi]

From: Elad Kanfi <eladkan@mellanox.com>

v2:
Remove code style commit for now.
Code style commit will be added after the bugs fix will be approved.

Summary:
 1. Bug description: TX done interrupts that arrives while interrupts
    are masked, during NAPI poll, will not trigger an interrupt handling.
    Since TX interrupt is of level edge we will lose the TX done interrupt.
    As a result all pending tx frames will get no service.

    Solution: Check if there is a pending tx request after unmasking the
    interrupt and if answer is yes then re-add ourselves to
    the NAPI poll list.

 2. Bug description: CPU-A before sending a frame will set a variable
    to true. CPU-B that executes the tx done interrupt service routine
    might read a non valid value of that variable.

    Solution: Add a memory barrier at the tx sending function.

Elad Kanfi (2):
  net: nps_enet: Sync access to packet sent flag
  net: nps_enet: bug fix - handle lost tx interrupts

 drivers/net/ethernet/ezchip/nps_enet.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

[PATCH v2 1/2] net: nps_enet: Sync access to packet sent flag

[Elad Kanfi]

From: Elad Kanfi <eladkan@mellanox.com>

Below is a description of a possible problematic
sequence. CPU-A is sending a frame and CPU-B handles
the interrupt that indicates the frame was sent. CPU-B
reads an invalid value of tx_packet_sent.

	CPU-A				CPU-B
	-----				-----
	nps_enet_send_frame
	.
	.
	tx_packet_sent = true
	order HW to start tx
	.
	.
	HW complete tx
			    ------> 	get tx complete interrupt
					.
					.
					if(tx_packet_sent == true)

	end memory transaction
	(tx_packet_sent actually
	 written)

Problem solution:

Add a memory barrier after setting tx_packet_sent,
in order to make sure that it is written before
the packet is sent.

Signed-off-by: Elad Kanfi <eladkan@mellanox.com>

Acked-by: Noam Camus <noamca@mellanox.com>
---
 drivers/net/ethernet/ezchip/nps_enet.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/ezchip/nps_enet.c b/drivers/net/ethernet/ezchip/nps_enet.c
index 1f23845..1373236 100644
--- a/drivers/net/ethernet/ezchip/nps_enet.c
+++ b/drivers/net/ethernet/ezchip/nps_enet.c
@@ -389,6 +389,13 @@ static void nps_enet_send_frame(struct net_device *ndev,
 
 	/* Indicate SW is done */
 	priv->tx_packet_sent = true;
+
+	/* before the frame is sent we have to make
+	 * sure that priv->tx_packet_sent will be valid
+	 * for the CPU'S that handles the ISR and NAPI poll
+	 */
+	smp_wmb();
+
 	tx_ctrl_value |= NPS_ENET_ENABLE << TX_CTL_CT_SHIFT;
 	/* Send Frame */
 	nps_enet_reg_set(priv, NPS_ENET_REG_TX_CTL, tx_ctrl_value);
-- 
1.7.1

Re: [PATCH v2 1/2] net: nps_enet: Sync access to packet sent flag

[Lino Sanfilippo]

Hi,

On 27.04.2016 15:18, Elad Kanfi wrote:
> From: Elad Kanfi <eladkan@mellanox.com>
>
> Below is a description of a possible problematic
> sequence. CPU-A is sending a frame and CPU-B handles
> the interrupt that indicates the frame was sent. CPU-B
> reads an invalid value of tx_packet_sent.
>
> 	CPU-A				CPU-B
> 	-----				-----
> 	nps_enet_send_frame
> 	.
> 	.
> 	tx_packet_sent = true
> 	order HW to start tx
> 	.
> 	.
> 	HW complete tx
> 			    ------> 	get tx complete interrupt
> 					.
> 					.
> 					if(tx_packet_sent == true)
>
> 	end memory transaction
> 	(tx_packet_sent actually
> 	 written)
>
> Problem solution:
>
> Add a memory barrier after setting tx_packet_sent,
> in order to make sure that it is written before
> the packet is sent.

Should not those SMP memory barriers be paired? AFAIK you do not only have to make sure
that the value written by CPU-A actually is written to memory but also that CPU-B
reads that value from memory. At least this is what I have understood from memory-barriers.txt...

Regards,
Lino

Re: [PATCH v2 1/2] net: nps_enet: Sync access to packet sent flag

[David Miller]

From: Elad Kanfi <eladkan@mellanox.com>
Date: Wed, 27 Apr 2016 16:18:29 +0300

> From: Elad Kanfi <eladkan@mellanox.com>
> 
> Below is a description of a possible problematic
> sequence. CPU-A is sending a frame and CPU-B handles
> the interrupt that indicates the frame was sent. CPU-B
> reads an invalid value of tx_packet_sent.
> 
> 	CPU-A				CPU-B
> 	-----				-----
> 	nps_enet_send_frame
> 	.
> 	.
> 	tx_packet_sent = true
> 	order HW to start tx
> 	.
> 	.
> 	HW complete tx
> 			    ------> 	get tx complete interrupt
> 					.
> 					.
> 					if(tx_packet_sent == true)
> 
> 	end memory transaction
> 	(tx_packet_sent actually
> 	 written)
> 
> Problem solution:
> 
> Add a memory barrier after setting tx_packet_sent,
> in order to make sure that it is written before
> the packet is sent.
> 
> Signed-off-by: Elad Kanfi <eladkan@mellanox.com>
> 
> Acked-by: Noam Camus <noamca@mellanox.com>

Please address the feedback about memory barrier pairing.

Also, for both patches, do not put empty lines between the various
tags at the end of the commit message.  They should all be together
in one continuous group.

RE: [PATCH v2 1/2] net: nps_enet: Sync access to packet sent flag

[Elad Kanfi]

-----Original Message-----
From: David Miller [mailto:davem@davemloft.net] 
Sent: Friday, April 29, 2016 12:11 AM
To: Elad Kanfi
Cc: Noam Camus; linux-kernel@vger.kernel.org; abrodkin@synopsys.com; Tal Zilcer; netdev@vger.kernel.org
Subject: Re: [PATCH v2 1/2] net: nps_enet: Sync access to packet sent flag

From: Elad Kanfi <eladkan@mellanox.com>
Date: Wed, 27 Apr 2016 16:18:29 +0300

> From: Elad Kanfi <eladkan@mellanox.com>
> 
> Below is a description of a possible problematic sequence. CPU-A is 
> sending a frame and CPU-B handles the interrupt that indicates the 
> frame was sent. CPU-B reads an invalid value of tx_packet_sent.
> 
> 	CPU-A				CPU-B
> 	-----				-----
> 	nps_enet_send_frame
> 	.
> 	.
> 	tx_packet_sent = true
> 	order HW to start tx
> 	.
> 	.
> 	HW complete tx
> 			    ------> 	get tx complete interrupt
> 					.
> 					.
> 					if(tx_packet_sent == true)
> 
> 	end memory transaction
> 	(tx_packet_sent actually
> 	 written)
> 
> Problem solution:
> 
> Add a memory barrier after setting tx_packet_sent, in order to make 
> sure that it is written before the packet is sent.
> 
> Signed-off-by: Elad Kanfi <eladkan@mellanox.com>
> 
> Acked-by: Noam Camus <noamca@mellanox.com>
>
> Please address the feedback about memory barrier pairing.
>
> Also, for both patches, do not put empty lines between the various tags at the end of the commit message.  They should all be together in one continuous group.

Since this driver handles one frame at a time, I couldn't find a case that requires the paired barrier.

The order of reads is not critical once it is assured that the value is valid.

If there is something I'm missing or for the sake of good order, I can add it. 

Please let me know your opinion on this.

I will remove the empty lines in the commit messages at the next patches version.

Elad.
 

Re: [PATCH v2 1/2] net: nps_enet: Sync access to packet sent flag

[Lino Sanfilippo]

Hi Elad,


On 02.05.2016 12:21, Elad Kanfi wrote:

> Since this driver handles one frame at a time, I couldn't find a case that requires the paired barrier.
>
> The order of reads is not critical once it is assured that the value is valid.
>

Please see sections "SMP BARRIER PAIRING" and "EXAMPLES OF MEMORY BARRIER SEQUENCES" in
  memory-barriers.txt for a description why smp barriers have to be paired and a smp write
barrier on CPU A without a read barrier on CPU B is _not_ sufficient.

Furthermore after having a closer look into the problem you want to fix, it seems that
you also have to ensure the correct order of the assignment of "tx_packet_sent" and the
corresponding skb.

On CPU A you do:

1. tx_skb = skb;
2. tx_packet_sent = true;

On CPU B (the irq handler) you do:

1. Check/read tx_packet_sent.
2.If set then handle tx_skb.

So there is a logical dependency between tx_skb and tx_packet_sent (tx_skb should only
be handled if tx_packet_sent is set). However both compiler and CPU are free to reorder
steps 1. and 2. on both CPU A and CPU B and you have to ensure that tx_skb actually contains
a valid pointer for CPU B as soon at it sees tx_packet_sent == true. So what you need here is:

CPU A:
1. tx_skb = skb
2. smp_wmb()
3. tx_packet_sent = true;

and CPU B:

1. read tx_packet_sent
2. smp_rmb()
3. If set then handle tx_skb

So this is to ensure that tx_skb is in sync with tx_packet_sent on both writer and reader side.


Then there is the second problem (the one you tried to address with your patch) that you have to make
sure that by the time you inform the hardware about the new frame and the thus the tx-done irq may occur
both tx_packet_sent and tx_skb are actually set in the memory. Otherwise the irq may occur but CPU B may
not see tx_packet_sent == true and the irq is lost. To achieve this you would have to use a (non-smp)
write barrier before you inform the HW. So for CPU A the sequence would extend to:

1. tx_skb = skb
2. smp_wmb() /* ensure correct order between both assignments */
3. tx_packet_sent = true;
4. wmb() /* make sure both assignments reach RAM before the HW is informed and the IRQ is fired */
5. nps_enet_reg_set(priv, NPS_ENET_REG_TX_CTL, tx_ctrl.value);

The latter barrier does not require pairing and has to be there even on UP systems because you dont want to
sync between CPU A and CPU B in this case but rather between system RAM and IO-MEMORY.

Please note that this is only how I understood things and it may be totally wrong. But I am sure that the initial
solution with only one smp_wmb() is not correct either.

Regards,
Lino

  

RE: [PATCH v2 1/2] net: nps_enet: Sync access to packet sent flag

[Elad Kanfi]

Hi Lino,

> Please see sections "SMP BARRIER PAIRING" and "EXAMPLES OF MEMORY BARRIER SEQUENCES" in
> memory-barriers.txt for a description why smp barriers have to be paired and a smp write barrier on CPU A without a read barrier on CPU B is _not_ sufficient.
>
> Furthermore after having a closer look into the problem you want to fix, it seems that you also have to ensure the correct order of the assignment of "tx_packet_sent" and the corresponding skb.
>
> On CPU A you do:
>
> 1. tx_skb = skb;
> 2. tx_packet_sent = true;
>
> On CPU B (the irq handler) you do:
>
> 1. Check/read tx_packet_sent.
> 2.If set then handle tx_skb.
>
> So there is a logical dependency between tx_skb and tx_packet_sent (tx_skb should only be handled if tx_packet_sent is set). However both compiler and CPU are free to reorder steps 1. and 2. on both CPU A  and CPU B and you have to ensure that tx_skb actually contains a valid pointer for CPU B as soon at it sees tx_packet_sent == true. So what you need here is:
>
> CPU A:
> 1. tx_skb = skb
> 2. smp_wmb()
> 3. tx_packet_sent = true;
>
> and CPU B:
>
> 1. read tx_packet_sent
> 2. smp_rmb()
> 3. If set then handle tx_skb
>
> So this is to ensure that tx_skb is in sync with tx_packet_sent on both writer and reader side.
>
>
> Then there is the second problem (the one you tried to address with your patch) that you have to make sure that by the time you inform the hardware about the new frame and the thus the tx-done irq may 
>
> occur both tx_packet_sent and tx_skb are actually set in the memory. Otherwise the irq may occur but CPU B may not see tx_packet_sent == true and the irq is lost. To achieve this you would have to use a (non-smp) write barrier before you inform the HW. So for CPU A the sequence would extend to:
>
> 1. tx_skb = skb
> 2. smp_wmb() /* ensure correct order between both assignments */ 3. tx_packet_sent = true; 4. wmb() /* make sure both assignments reach RAM before the HW is informed and the IRQ is fired */ 5. nps_enet_reg_set(priv, NPS_ENET_REG_TX_CTL, tx_ctrl.value);
>
> The latter barrier does not require pairing and has to be there even on UP systems because you dont want to sync between CPU A and CPU B in this case but rather between system RAM and IO-MEMORY.
>
> Please note that this is only how I understood things and it may be totally wrong. But I am sure that the initial solution with only one smp_wmb() is not correct either.
>
> Regards,
> Lino
  
After reviewing the code and your suggestion, it seems that we can do without the flag tx_packet_sent and therefor the first issue becomes irrelevant.
The indication that a packet was sent is (tx_skb != NULL) , and the sequence will be:

CPU A:
1. tx_skb = skb
2. wmb() /* make sure tx_skb reaches the RAM before the HW is informed and the IRQ is fired */
3. nps_enet_reg_set(priv, NPS_ENET_REG_TX_CTL, tx_ctrl.value); /* send frame */

CPU B:
1. read tx_skb 
2. if( tx_skb != NULL ) handle tx_skb
3. tx_skb = NULL 


Regards,
Elad.

Re: [PATCH v2 1/2] net: nps_enet: Sync access to packet sent flag

[Lino Sanfilippo]

Hi Elad,

On 08.05.2016 15:44, Elad Kanfi wrote:

>   
> After reviewing the code and your suggestion, it seems that we can do without the flag tx_packet_sent and therefor the first issue becomes irrelevant.
> The indication that a packet was sent is (tx_skb != NULL) , and the sequence will be:
> 
> CPU A:
> 1. tx_skb = skb
> 2. wmb() /* make sure tx_skb reaches the RAM before the HW is informed and the IRQ is fired */
> 3. nps_enet_reg_set(priv, NPS_ENET_REG_TX_CTL, tx_ctrl.value); /* send frame */
> 
> CPU B:
> 1. read tx_skb 
> 2. if( tx_skb != NULL ) handle tx_skb
> 3. tx_skb = NULL 
> 
> 

Ok, without the tx_packet_sent flag the code becomes simpler. But it
does not mean that we can toss the smp_rmb in the irq handler
completely. We still have to use a read barrier there to ensure that we
see the most recent value of tx_skb. E.g like this:

if (priv->tx_skb != NULL ) {
	smp_rmb()
	/ * handle tx_skb */
}

With both barriers in place the code should work as expected.

Regards,
Lino

[PATCH v2 2/2] net: nps_enet: bug fix - handle lost tx interrupts

[Elad Kanfi]

From: Elad Kanfi <eladkan@mellanox.com>

The tx interrupt is of edge type, and in case such interrupt is triggered
while it is masked it will not be handled even after tx interrupts are
re-enabled in the end of NAPI poll.
This will cause tx network to stop in the following scenario:
 * Rx is being handled, hence interrupts are masked.
 * Tx interrupt is triggered after checking if there is some tx to handle
   and before re-enabling the interrupts.
In this situation only rx transaction will release tx requests.

In order to handle the tx that was missed( if there was one ),
a NAPI reschdule was added after enabling the interrupts.

Signed-off-by: Elad Kanfi <eladkan@mellanox.com>

Acked-by: Noam Camus <noamca@mellanox.com>
---
 drivers/net/ethernet/ezchip/nps_enet.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/ezchip/nps_enet.c b/drivers/net/ethernet/ezchip/nps_enet.c
index 1373236..31e27a7 100644
--- a/drivers/net/ethernet/ezchip/nps_enet.c
+++ b/drivers/net/ethernet/ezchip/nps_enet.c
@@ -183,6 +183,8 @@ static int nps_enet_poll(struct napi_struct *napi, int budget)
 	work_done = nps_enet_rx_handler(ndev);
 	if (work_done < budget) {
 		u32 buf_int_enable_value = 0;
+		u32 tx_ctrl_value = nps_enet_reg_get(priv, NPS_ENET_REG_TX_CTL);
+		u32 tx_ctrl_ct = (tx_ctrl_value & TX_CTL_CT_MASK) >> TX_CTL_CT_SHIFT;
 
 		napi_complete(napi);
 
@@ -192,6 +194,18 @@ static int nps_enet_poll(struct napi_struct *napi, int budget)
 
 		nps_enet_reg_set(priv, NPS_ENET_REG_BUF_INT_ENABLE,
 				 buf_int_enable_value);
+
+		/* in case we will get a tx interrupt while interrupts
+		 * are masked, we will lose it since the tx is edge interrupt.
+		 * specifically, while executing the code section above,
+		 * between nps_enet_tx_handler and the interrupts enable, all
+		 * tx requests will be stuck until we will get an rx interrupt.
+		 * the two code lines below will solve this situation by
+		 * re-adding ourselves to the poll list.
+		 */
+
+		if (priv->tx_packet_sent && !tx_ctrl_ct)
+			napi_reschedule(napi);
 	}
 
 	return work_done;
-- 
1.7.1