Received-SPF: pass (google.com: domain of linux-kernel+bounces-179788-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 15 May 2024 13:12:37 +0200
From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vitor Soares <ivitro@gmail.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>, 
	Thomas Kopp <thomas.kopp@microchip.com>, Vincent Mailhol <mailhol.vincent@wanadoo.fr>, 
	"David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, 
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, 
	Vitor Soares <vitor.soares@toradex.com>, linux-can@vger.kernel.org, netdev@vger.kernel.org, 
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH v5] can: mcp251xfd: fix infinite loop when xmit fails
Message-ID: <20240515-athletic-sensible-swine-4e7692-mkl@pengutronix.de>
References: <20240514105822.99986-1-ivitro@gmail.com>
 <20240514-corgi-of-marvelous-peace-968f5c-mkl@pengutronix.de>
 <465a2ddb222beed7c90b36c523633fc5648715bb.camel@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="p2gqh7q5mtz2efsu"
Content-Disposition: inline
In-Reply-To: <465a2ddb222beed7c90b36c523633fc5648715bb.camel@gmail.com>


--p2gqh7q5mtz2efsu
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On 14.05.2024 15:34:01, Vitor Soares wrote:
> > > +void mcp251xfd_tx_obj_write_sync(struct work_struct *ws)
> > > +{
> > > +       struct mcp251xfd_priv *priv =3D container_of(ws, struct
> > > mcp251xfd_priv,
> > > +                                                  tx_work);
> > > +       struct mcp251xfd_tx_obj *tx_obj =3D priv->tx_work_obj;
> > > +       struct mcp251xfd_tx_ring *tx_ring =3D priv->tx;
> > > +       int err;
> > > +
> > > +       err =3D spi_sync(priv->spi, &tx_obj->msg);
> > > +       if (err)
> > > +               mcp251xfd_tx_failure_drop(priv, tx_ring, err);
> > > +
> > > +       priv->tx_work_obj =3D NULL;
> >=20
> > Race condition:
> > - after spi_sync() the CAN frame is send
> > - after the TX complete IRQ the TX queue is restarted
> > - the xmit handler might get BUSY
> > - fill the tx_work_obj again

You can avoid the race condition by moving "priv->tx_work_obj =3D NULL;"
in front of the "spi_sync();". Right?

> > > +}
> > > +
> > >  static int mcp251xfd_tx_obj_write(const struct mcp251xfd_priv *priv,
> > >                                   struct mcp251xfd_tx_obj *tx_obj)
> > >  {
> > > @@ -175,7 +210,7 @@ netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *=
skb,
> > >         if (can_dev_dropped_skb(ndev, skb))
> > >                 return NETDEV_TX_OK;
> > > =20
> > > -       if (mcp251xfd_tx_busy(priv, tx_ring))
> > > +       if (mcp251xfd_tx_busy(priv, tx_ring) || priv->tx_work_obj)
> >=20
> > This should not happen, but better save than sorry.
>=20
> As there is the race condition you mentioned above, on this condition:
> priv->tx_work_obj =3D tx_obj --> xmit will return NETDEV_TX_BUSY
>=20
> or
>=20
> priv->tx_work_obj =3D NULL --> It goes through the rest of the code or
> the workqueue may sleep after setting tx_work_obj to NULL. Should I
> use work_busy() here instead or do you have another suggestion?

Yes, introduce mcp251xfd_work_busy().

I'm not sure what happens if the xmit is called between the
"priv->tx_work_obj =3D NULL" and the end of the work. Will queue_work()
return false, as the queue is still running?

> Everything else is clear to me and I'm addressing it for the v6.

Marc

--=20
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung N=C3=BCrnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

--p2gqh7q5mtz2efsu
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEUEC6huC2BN0pvD5fKDiiPnotvG8FAmZEmKAACgkQKDiiPnot
vG8QuAf/Sdxq15eqo964upghsJzAYE2c+AqyhOJ41rndONHflyczJems3Jbk1t+3
BiFwD7+PFKsXec+6qguGK2vnmu74D5+PYi2zYI2Cyz5eBJhAj8M/fgpfzroPD9Oe
6tFYeTC6lRi3YW0pHxefQxP44OpTnac3CLYhPREMM0OpJJ8VLljSzEUdx2xWgFmk
M4foDpuran5XVYH/cSh9ecEXkNiJjZ7XZf3f4hXM/K0BdN2og/CyT7xGQG/Yl3lL
ZRZZDV8O6sC/tQhx4UukoaUP7bwR4z6oho8SyYU+wUsiKO6zn9xXZyEFbMmD3iJ0
lym7DsDGUyRI2gCPRjFhnJbBaIWDAQ==
=56G6
-----END PGP SIGNATURE-----

--p2gqh7q5mtz2efsu--