Received-SPF: pass (google.com: domain of linux-kernel+bounces-95895-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <75fa2711c1aace90a831f95a00b8ba41abf1c883.camel@gmail.com>
Subject: Re: [PATCH] can: mcp251xfd: fix infinite loop when xmit fails
From: vitor <ivitro@gmail.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: manivannan.sadhasivam@linaro.org, thomas.kopp@microchip.com, 
 wg@grandegger.com, linux-can@vger.kernel.org, linux-kernel@vger.kernel.org,
  vitor.soares@toradex.com, stable@vger.kernel.org
Date: Thu, 07 Mar 2024 16:54:13 +0000
In-Reply-To: <xmk5cgskx2ug2psec6qgbxndbuqq5cnin7rd4zt3thnhybxgeo@xudpdob7tcg2>
References: <20240307120442.12262-1-ivitro@gmail.com>
	 <xmk5cgskx2ug2psec6qgbxndbuqq5cnin7rd4zt3thnhybxgeo@xudpdob7tcg2>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
Precedence: bulk
MIME-Version: 1.0

Hi Marc,

On Thu, 2024-03-07 at 13:23 +0100, Marc Kleine-Budde wrote:
> Sorry, resend. I fsck up the linux-can ML's address.

Thanks for that.
>=20
> On 07.03.2024 12:04:42, Vitor Soares wrote:
> > From: Vitor Soares <vitor.soares@toradex.com>
> >=20
> > When the mcp251xfd_start_xmit() function fails, the driver stops
> > processing messages, and the interrupt routine does not return,
> > running indefinitely even after killing the running application.
> >=20
> > Error messages:
> > [=C2=A0 441.298819] mcp251xfd spi2.0 can0: ERROR in
> > mcp251xfd_start_xmit: -16
> > [=C2=A0 441.306498] mcp251xfd spi2.0 can0: Transmit Event FIFO buffer
> > not empty. (seq=3D0x000017c7, tef_tail=3D0x000017cf,
> > tef_head=3D0x000017d0, tx_head=3D0x000017d3).
> > ... and repeat forever.
> >=20
> > The issue can be triggered when multiple devices share the same
> > SPI interface. And there is concurrent access to the bus.
> >=20
> > The problem occurs because tx_ring->head increments even if
> > mcp251xfd_start_xmit() fails. Consequently, the driver skips one
> > TX package while still expecting a response in
> > mcp251xfd_handle_tefif_one().
> >=20
> > This patch resolves the issue by decreasing tx_ring->head if
> > mcp251xfd_start_xmit() fails. With the fix, if we attempt to
> > trigger
> > the issue again, the driver prints an error and discard the
> > message.
>=20
> What about returning NETDEV_TX_BUSY, then the networking stack will
> retry.

Do you mean when err =3D=3D -EBUSY?

>=20
> > Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip
> > MCP25xxFD SPI CAN")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
> > ---
> > =C2=A0drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c | 27 ++++++++++-----=
-
> > ----
> > =C2=A01 file changed, 14 insertions(+), 13 deletions(-)
> >=20
> > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c
> > b/drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c
> > index 160528d3cc26..a8eb941c1b95 100644
> > --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c
> > +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c
> > @@ -181,25 +181,26 @@ netdev_tx_t mcp251xfd_start_xmit(struct
> > sk_buff *skb,
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0tx_obj =3D mcp251xfd_ge=
t_tx_obj_next(tx_ring);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mcp251xfd_tx_obj_from_s=
kb(priv, tx_obj, skb, tx_ring-
> > >head);
> > =C2=A0
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Stop queue if we occupy t=
he complete TX FIFO */
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0tx_head =3D mcp251xfd_g=
et_tx_head(tx_ring);
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0tx_ring->head++;
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (mcp251xfd_get_tx_free(tx=
_ring) =3D=3D 0)
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0netif_stop_queue(ndev);
> > -
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0frame_len =3D can_skb_g=
et_frame_len(skb);
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0err =3D can_put_echo_skb(skb=
, ndev, tx_head, frame_len);
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!err)
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0netdev_sent_queue(priv->ndev, frame_len);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0can_put_echo_skb(skb, ndev, =
tx_head, frame_len);
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0tx_ring->head++;
> > =C2=A0
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0err =3D mcp251xfd_tx_ob=
j_write(priv, tx_obj);
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (err)
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0goto out_err;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (err) {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0can_free_echo_skb(ndev, tx_head, NULL);
> > =C2=A0
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return NETDEV_TX_OK;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0tx_ring->head--;
> > +

I'm testing returning here when err =3D -EBUSY, but can_put_echo_skb()
should be invoked after mcp251xfd_tx_obj_write(). Otherwise, I get a
Kernel NULL pointer dereference error.


>=20
> I'm not sure, if we want an error message for -EBUSY. We could add
> proper ethtool statistics.
>=20
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0netdev_err(priv->ndev, "ERROR in %s: %d\n",
> > __func__, err);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0/* Stop queue if we occupy the complete TX FIFO */
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0if (mcp251xfd_get_tx_free(tx_ring) =3D=3D 0)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0netif_=
stop_queue(ndev);
> > =C2=A0
> > - out_err:
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0netdev_err(priv->ndev, "ERRO=
R in %s: %d\n", __func__, err);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0netdev_sent_queue(priv->ndev, frame_len);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > =C2=A0
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return NETDEV_TX_OK;
> > =C2=A0}
>=20
> regards,
> Marc
>=20

Best regards,
Vitor Soares