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
Subject: Re: [PATCH v5] can: mcp251xfd: fix infinite loop when xmit fails
Date: Wed, 15 May 2024 17:29:38 +0100 [thread overview]
Message-ID: <11e4ff07ad6c0e9077326cf288665922ccfc0afd.camel@gmail.com> (raw)
In-Reply-To: <20240515-athletic-sensible-swine-4e7692-mkl@pengutronix.de>
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)
> > > > +{
> > > > + struct mcp251xfd_priv *priv = container_of(ws, struct
> > > > mcp251xfd_priv,
> > > > + tx_work);
> > > > + struct mcp251xfd_tx_obj *tx_obj = priv->tx_work_obj;
> > > > + struct mcp251xfd_tx_ring *tx_ring = priv->tx;
> > > > + int err;
> > > > +
> > > > + err = spi_sync(priv->spi, &tx_obj->msg);
> > > > + if (err)
> > > > + mcp251xfd_tx_failure_drop(priv, tx_ring, err);
> > > > +
> > > > + priv->tx_work_obj = NULL;
> > >
> > > 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 = 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;
> > > >
> > > > - if (mcp251xfd_tx_busy(priv, tx_ring))
> > > > + if (mcp251xfd_tx_busy(priv, tx_ring) || priv->tx_work_obj)
> > >
> > > This should not happen, but better save than sorry.
> >
> > As there is the race condition you mentioned above, on this condition:
> > priv->tx_work_obj = tx_obj --> xmit will return NETDEV_TX_BUSY
> >
> > or
> >
> > priv->tx_work_obj = 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'll implement it.
> I'm not sure what happens if the xmit is called between the
> "priv->tx_work_obj = 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
- stop netif queue
- fill the tx_work_obj
- start worker
queue_work() doesn't return false even when work_busy() = true.
- xmit handler return, and wait netif_wake_queue()
- the work handler waits until the previous job gets done before starting the
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 = NULL.
Best regards,
Vitor Soares
prev parent reply other threads:[~2024-05-15 16:29 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-14 10:58 [PATCH v5] can: mcp251xfd: fix infinite loop when xmit fails Vitor Soares
2024-05-14 12:23 ` Marc Kleine-Budde
2024-05-14 14:34 ` Vitor Soares
2024-05-15 11:12 ` Marc Kleine-Budde
2024-05-15 16:29 ` Vitor Soares [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=11e4ff07ad6c0e9077326cf288665922ccfc0afd.camel@gmail.com \
--to=ivitro@gmail.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mailhol.vincent@wanadoo.fr \
--cc=manivannan.sadhasivam@linaro.org \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=stable@vger.kernel.org \
--cc=thomas.kopp@microchip.com \
--cc=vitor.soares@toradex.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).