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

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: dariobin@libero.it
Cc: Jacob Kroon <jacob.kroon@gmail.com>,
	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: Wed, 28 Sep 2022 10:02:43 +0200	[thread overview]
Message-ID: <20220928080243.er7pnbu4mmvyfsbw@pengutronix.de> (raw)
In-Reply-To: <1885528784.804387.1663962304792@mail1.libero.it>

[-- Attachment #1: Type: text/plain, Size: 3483 bytes --]

On 23.09.2022 21:45:04, dariobin@libero.it wrote:
> 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;

This works, but it is frowned upon. You intentionally omit adequate flow
control for the C_CAN core. The overhead for starting the
ndo_start_xmit() callback and aborting it with ETDEV_TX_BUSY is
considered too high.

> +               }
> +       }
> +
>         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.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]