Received-SPF: pass (google.com: domain of linux-kernel+bounces-180139-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: <11e4ff07ad6c0e9077326cf288665922ccfc0afd.camel@gmail.com>
Subject: Re: [PATCH v5] can: mcp251xfd: fix infinite loop when xmit fails
From: Vitor Soares <ivitro@gmail.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>
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
Date: Wed, 15 May 2024 17:29:38 +0100
In-Reply-To: <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>
	 <20240515-athletic-sensible-swine-4e7692-mkl@pengutronix.de>
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

On Wed, 2024-05-15 at 13:12 +0200, Marc Kleine-Budde wrote:
> On 14.05.2024 15:34:01, Vitor Soares wrote:
> > > > +void mcp251xfd_tx_obj_write_sync(struct work_struct *ws)
> > > > +{
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct mcp251xfd_priv *priv =
=3D container_of(ws, struct
> > > > mcp251xfd_priv,
> > > > +=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=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=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 tx_work);
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct mcp251xfd_tx_obj *tx_o=
bj =3D priv->tx_work_obj;
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct mcp251xfd_tx_ring *tx_=
ring =3D priv->tx;
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int err;
> > > > +
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 err =3D spi_sync(priv->spi, &=
tx_obj->msg);
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (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 mcp251xfd_tx_failure_drop(priv, tx_ring, err);
> > > > +
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 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
>=20
> You can avoid the race condition by moving "priv->tx_work_obj =3D NULL;"
> in front of the "spi_sync();". Right?
>=20
> > > > +}
> > > > +
> > > > =C2=A0static int mcp251xfd_tx_obj_write(const struct mcp251xfd_priv=
 *priv,
> > > > =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=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct mcp251xfd_=
tx_obj *tx_obj)
> > > > =C2=A0{
> > > > @@ -175,7 +210,7 @@ netdev_tx_t mcp251xfd_start_xmit(struct sk_buff
> > > > *skb,
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (can_dev_dropped_skb(=
ndev, skb))
> > > > =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 return NETDEV_TX_OK;
> > > > =C2=A0
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (mcp251xfd_tx_busy(priv, t=
x_ring))
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (mcp251xfd_tx_busy(priv, t=
x_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?
>=20
> Yes, introduce mcp251xfd_work_busy().
>=20

I'll implement it.

> 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?

From the test I did so far, my understanding is the following:

If mcp251xfd_tx_obj_write doesn't fail, everything is OK.

if mcp251xfd_tx_obj_write fails with EBUSY=20
 - stop netif queue
 - fill the tx_work_obj
 - start worker

queue_work() doesn't return false even when work_busy() =3D true.=20
 - xmit handler return, and wait netif_wake_queue()
 - the work handler waits until the previous job gets done before starting =
the=20
next one.
 - after the TX completes IRQ, the TX queue is restarted

If the TX queue is restarted immediately after queue_work(), tx_work_obj is
filled, making the xmit handler return NETDEV_TX_BUSY.

The tests were done with a delay after priv->tx_work_obj =3D NULL.


Best regards,
Vitor Soares