Re: CM-ITC, pch_can/c_can_pci, sendto() returning ENOBUFS

From: Jacob Kroon <jacob.kroon@gmail.com>
To: dariobin@libero.it, Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>,
	linux-can@vger.kernel.org, wg@grandegger.com
Subject: Re: CM-ITC, pch_can/c_can_pci, sendto() returning ENOBUFS
Date: Fri, 23 Sep 2022 21:21:26 +0200	[thread overview]
Message-ID: <8665ef57-17fb-dfd7-afa2-8e5bebceb617@gmail.com> (raw)
In-Reply-To: <a162f149-58ba-24eb-474f-294b9fe78e51@gmail.com>

On 9/23/22 21:03, Jacob Kroon wrote:
> Hi Dario,
> 
> On 9/23/22 19:55, dariobin@libero.it wrote:
>> Hi Michael,
>>
>>> Il 23/09/2022 13:36 Marc Kleine-Budde <mkl@pengutronix.de> ha scritto:
>>>
>>> On 16.09.2022 06:14:58, Jacob Kroon wrote:
>>>> What I do know is that if I revert commit:
>>>>
>>>> "can: c_can: cache frames to operate as a true FIFO"
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=387da6bc7a826cc6d532b1c0002b7c7513238d5f
>>>>
>>>> then everything looks good. I don't get any BUG messages, and the host
>>>> has been running overnight without problems, so it seems to have fixed
>>>> the network interface lockup as well.
>>>
>>> Jacob, after this mail, I'll send 2 patches. One tries to disable the
>>> cache feature for C_CAN cores, the other shuts a potential race window.
>>
>> About the "can: c_can: don't cache TX messages for C_CAN cores" patch:
>> Could it make sense to change the c_can_start_xmit in this way
>>
>> -       if (idx < c_can_get_tx_tail(tx_ring))
>> -               cmd &= ~IF_COMM_TXRQST; /* Cache the message */
>> +       if (idx < c_can_get_tx_tail(tx_ring)) {
>> +               if (priv->type == BOSCH_D_CAN) {
>> +                       cmd &= ~IF_COMM_TXRQST; /* Cache the message */
>> +               } else {
>> +                       netif_stop_queue(priv->dev);
>> +                       return NETDEV_TX_BUSY;
>> +               }
>> +       }
>>
>> without changing the c_can_get_tx_{free,busy} routines ?
>>
> 
> I can test, so you suggest only doing the above patch, or were there 
> other parts from Marc's patch you wanted in ?
>

Well I tried only the above, and the patch below

diff --git a/drivers/net/can/c_can/c_can_main.c 
b/drivers/net/can/c_can/c_can_main.c
index a7362af0babb..21d0a55ebbb3 100644
--- a/drivers/net/can/c_can/c_can_main.c
+++ b/drivers/net/can/c_can/c_can_main.c
@@ -468,8 +468,14 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff 
*skb,
  	if (c_can_get_tx_free(tx_ring) == 0)
  		netif_stop_queue(dev);

-	if (idx < c_can_get_tx_tail(tx_ring))
-		cmd &= ~IF_COMM_TXRQST; /* Cache the message */
+	if (idx < c_can_get_tx_tail(tx_ring)) {
+		if (priv->type == BOSCH_D_CAN) {
+			cmd &= ~IF_COMM_TXRQST; /* Cache the message */
+		} else {
+			netif_stop_queue(priv->dev);
+			return NETDEV_TX_BUSY;
+		}
+	}

  	/* Store the message in the interface so we can call
  	 * can_put_echo_skb(). We must do this before we enable
@@ -761,7 +767,7 @@ static void c_can_do_tx(struct net_device *dev)

  	tail = c_can_get_tx_tail(tx_ring);

-	if (tail == 0) {
+	if (priv->type == BOSCH_D_CAN && tail == 0) {
  		u8 head = c_can_get_tx_head(tx_ring);

  		/* Start transmission for all cached messages */


but they both seem to lockup the network.

Jacob
