RE: [PATCH 1/1] can: m_can: Control tx flow to avoid message stuck

Hi @Markus Schneider-Pargmann,

-----Original Message-----
From: Markus Schneider-Pargmann <mailto:msp@baylibre.com>
Sent: Thursday, January 9, 2025 9:14 PM
To: Mohan, Subramanian <mailto:subramanian.mohan@intel.com>
Cc: mailto:rcsekar@samsung.com; mailto:davem@davemloft.net; mailto:edumazet@google.com;
mailto:kuba@kernel.org; mailto:pabeni@redhat.com; mailto:balbi@kernel.org; Tan, Raymond
<mailto:raymond.tan@intel.com>; mailto:jarkko.nikula@linux.intel.com; linux-
mailto:can@vger.kernel.org; mailto:netdev@vger.kernel.org; mailto:linux-kernel@vger.kernel.org;
mailto:linux@ew.tq-group.com; mailto:lst@pengutronix.de; Hahn, Matthias
<mailto:matthias.hahn@intel.com>; Chinnadurai, Srinivasan
<mailto:srinivasan.chinnadurai@intel.com>
Subject: Re: [PATCH 1/1] can: m_can: Control tx flow to avoid message stuck

Hi,

On Wed, Jan 08, 2025 at 02:31:12PM +0530,
mailto:subramanian.mohan@intel.com wrote:

quoted

From: Subramanian Mohan <mailto:subramanian.mohan@intel.com>

The prolonged testing of passing can messages between two Elkhartlake
platforms resulted in message stuck i.e Message did not receive at
receiver side

Can you please describe the reason for the stuck messages in your commit
message? I am reading this but I don't understand why this happens or why
your proposed solution helps.

Let me describe problem bit more:
We are using 2 different Python Scripts(client and server) on both of the Elkhart lake connected systems. 
The "server" script sends out messages with Arbitration ID's, and then waits for a response. If the Arbitration ID is different than the 
one expected or no message arrives it logs an error.
The "client" script listens for messages, and depending on the Arbitration ID received it sends a message with a specific Arbitration ID back.
We have deployed both the scripts in 2 different systems and triggered the testing
If any message is lost/stuck then the "server" - Script will log an error.
The Message stuck corresponds over here, whenever the server sends out message and waits for reply, we wont me getting the reply message 
On the server side. Even though time slice increase in scripts did not help. On further debugging enabling TX/TEFN impacts the processing load.
To overcome this we disabled the TX/TEFN interrupt once processed and enable it back in the TX start xmit function.

quoted

Contolling TX i.e TEFN bit helped to resolve the message stuck issue.

The current solution is enhanced/optimized from the below patch:
https://lore.kernel.org/lkml/20230623051124.64132-1-kumari.pallavi@int
el.com/T/

Setup used to reproduce the issue:

+---------------------+         +----------------------+
|Intel ElkhartLake    |         |Intel ElkhartLake     |
|       +--------+    |         |       +--------+     |
|       |m_can 0 |    |<=======>|       |m_can 0 |     |
|       +--------+    |         |       +--------+     |
+---------------------+         +----------------------+

Steps to be run on the two Elkhartlake HW:
1)Bus-Rate is 1 MBit/s
2)Busload during the test is about 40% 3)we initialize the CAN with
following commands 4)ip link set can0 txqueuelen 100/1024/2048 5)ip
link set can0 up type can bitrate 1000000

Python scripts are used send and receive the can messages between the
EHL systems.

Signed-off-by: Hahn Matthias <mailto:matthias.hahn@intel.com>
Signed-off-by: Subramanian Mohan <mailto:subramanian.mohan@intel.com>
---
 drivers/net/can/m_can/m_can.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c

b/drivers/net/can/m_can/m_can.c index 97cd8bbf2e32..0a2c9a622842
100644

--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c

@@ -1220,7 +1220,7 @@ static void m_can_coalescing_update(struct

m_can_classdev *cdev, u32 ir)  static int
m_can_interrupt_handler(struct m_can_classdev *cdev)  {
     struct net_device *dev = cdev->net;
-   u32 ir = 0, ir_read;
+   u32 ir = 0, ir_read, new_interrupts;
     int ret;

     if (pm_runtime_suspended(cdev->dev)) @@ -1283,6 +1283,9 @@

static

quoted

int m_can_interrupt_handler(struct m_can_classdev *cdev)
                     ret = m_can_echo_tx_event(dev);
                     if (ret != 0)
                             return ret;
+
+                   new_interrupts = cdev->active_interrupts &

~(IR_TEFN);

quoted

+                   m_can_interrupt_enable(cdev, new_interrupts);

Here is a theoretical situation of two messages being sent. The first is being
sent and handled in this interrupt handler. Then it would disable the TEFN bit
right? If the second message wasn't done sending yet, how would it ever call
the interrupt handler if the interrupt is disabled?

With this patch we are controlling only TEFN/TX interrupt bit, rest of the interrupts remains unaffected. 
Since We are enabling/disabling TEFN bit only, interrupt handler will be called normally with other interrupts.

Also you are disabling this interrupt here regardless of the type of mcan device
and also regardless of the coalescing state. In the transmit part you are only
enabling it for non-peripheral devices. For peripheral mcan devices this would
also introduce an additional two transfers per transmit.

TEFN bit enabling/disabling applies only to non-peripheral device.
While disabling the TEFN bit in interrupt handler, we will add the check for non-peripheral device before disabling it(V2).
On the coalescing state, The snapshot is already taken while entering the interrupt handler.

In which situations is this really necessary? Does it help to implement
coalescing for non-peripheral devices?

This helps in heavy load/traffic conditions 
Not exactly sure on the coalescing part.

Thanks,
Subbu

Best
Markus

quoted

             }
     }

@@ -1989,6 +1992,7 @@ static netdev_tx_t m_can_start_xmit(struct

sk_buff *skb,

quoted

     struct m_can_classdev *cdev = netdev_priv(dev);
     unsigned int frame_len;
     netdev_tx_t ret;
+   u32 new_interrupts;

     if (can_dev_dropped_skb(dev, skb))
             return NETDEV_TX_OK;

@@ -2008,8 +2012,11 @@ static netdev_tx_t m_can_start_xmit(struct

sk_buff *skb,

     if (cdev->is_peripheral)
             ret = m_can_start_peripheral_xmit(cdev, skb);
-   else
+   else {
+           new_interrupts = cdev->active_interrupts | IR_TEFN;
+           m_can_interrupt_enable(cdev, new_interrupts);
             ret = m_can_tx_handler(cdev, skb);
+   }

     if (ret != NETDEV_TX_OK)
             netdev_completed_queue(dev, 1, frame_len);
--
2.35.3