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

[dariobin@libero.it]

Hi Jacob,

> Il 23/09/2022 21:21 Jacob Kroon <jacob.kroon@gmail.com> ha scritto:
> 
>  
> 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.
> 

I didn't understand if you applied two patches separately or not.
This was the only patch I had thought of. Which was similar to Marc's 
for the interrupt part but differed in the c_can_start_xmit part.

--- a/drivers/net/can/c_can/c_can_main.c
+++ b/drivers/net/can/c_can/c_can_main.c
@@ -464,13 +464,19 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
                return NETDEV_TX_BUSY;
 
        idx = c_can_get_tx_head(tx_ring);
+       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;
+               }
+       }
+
        tx_ring->head++;
        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 */
-
        /* Store the message in the interface so we can call
         * can_put_echo_skb(). We must do this before we enable
         * transmit as we might race against do_tx().
@@ -723,7 +729,6 @@ static void c_can_do_tx(struct net_device *dev)
        struct c_can_tx_ring *tx_ring = &priv->tx;
        struct net_device_stats *stats = &dev->stats;
        u32 idx, obj, pkts = 0, bytes = 0, pend;
-       u8 tail;
 
        if (priv->msg_obj_tx_last > 32)
                pend = priv->read_reg32(priv, C_CAN_INTPND3_REG);
@@ -759,15 +764,20 @@ static void c_can_do_tx(struct net_device *dev)
        stats->tx_bytes += bytes;
        stats->tx_packets += pkts;
 
-       tail = c_can_get_tx_tail(tx_ring);
+       if (priv->type == BOSCH_D_CAN) {
+               u8 tail;
+
+               tail = c_can_get_tx_tail(tx_ring);
 
-       if (tail == 0) {
-               u8 head = c_can_get_tx_head(tx_ring);
+               if (tail == 0) {
+                       u8 head = c_can_get_tx_head(tx_ring);
 
-               /* Start transmission for all cached messages */
-               for (idx = tail; idx < head; idx++) {
-                       obj = idx + priv->msg_obj_tx_first;
-                       c_can_object_put(dev, IF_NAPI, obj, IF_COMM_TXRQST);
+                       /* Start transmission for all cached messages */
+                       for (idx = tail; idx < head; idx++) {
+                               obj = idx + priv->msg_obj_tx_first;
+                               c_can_object_put(dev, IF_NAPI, obj,
+                                                IF_COMM_TXRQST);
+                       }
                }
        }
 }

I changed it a little from the previous email because I noticed a problem.

Thanks and regards,
Dario

> Jacob