From: Wolfram Sang <wsa+renesas@sang-engineering.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-renesas-soc@vger.kernel.org,
Dirk Behme <dirk.behme@de.bosch.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jirislaby@kernel.org>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Aleksandar Mitev <amitev@visteon.com>,
linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] serial: sh-sci: always cancel hrtimer when DMA RX is invalidated
Date: Thu, 2 May 2024 20:17:37 +0200 [thread overview]
Message-ID: <20240502181737.b5vvghnwzievvlgj@ninjato> (raw)
In-Reply-To: <CAMuHMdUEvft0B9WdfZ936ccomZW4Qea8MVNSj-Q-Dyn8EKSUdA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]
Hi Geert,
good news, I was able to trigger the DMA rx code path. I dunno what I
did wrong last time. I started from scratch again and it worked easily
by dd-ing random data to the second non-console debug port.
> I do think you need to cancel the timer: even when not restarting
> the timer in sci_dma_rx_complete() due to a DMA failure, the previous
> timer may still be running, and will cause a NULL pointer dereference
> on s->chan_rx on timer expiry.
Taking locking into account, I think this patch is bogus. If we run into
this NULL-pointer, we have a locking problem and cancelling the timer in
sci_dma_rx_chan_invalidate() is not going to fix the underlying locking
problem. sci_dma_rx_chan_invalidate() does not only clear the pointer
but also the cookie_rx-array. sci_dma_rx_timer_fn() bails out via
sci_dma_rx_find_active() if that array is cleared. It does so before
accessing the chan_rx-pointer. So, it looks to me that should work once
all calls to sci_dma_rx_chan_invalidate() are protected. And there is
one path where this is not true, namely via sci_dma_rx_release() during
shutdown. This is why I asked Dirk if the system was about to shutdown.
Currently, I don't see any other problematic code path.
> > -#ifdef CONFIG_SERIAL_SH_SCI_DMA
> > - if (s->chan_rx_saved) {
> > - dev_dbg(port->dev, "%s(%d) deleting rx_timer\n", __func__,
> > - port->line);
> > - hrtimer_cancel(&s->rx_timer);
> > - }
> > -#endif
Also, this chunk needs to stay. I suggested in patch 1 to cancel the
timer on successful dma_rx_complete, so the timer only runs when a DMA
is in progress. Then, of course, we need to cancel it in shutdown.
I hope I am not seeing "no wood for the trees" by now. I am not
convinced that I actually found Dirk's race condition yet...
All the best,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
prev parent reply other threads:[~2024-05-02 18:17 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-16 12:35 [RFC PATCH 0/2] serial: sci: fix OOPS because of wrongly running hrtimer Wolfram Sang
2024-04-16 12:35 ` [RFC PATCH 1/2] serial: sh-sci: start hrtimer after setting up DMA Wolfram Sang
2024-04-24 9:41 ` Geert Uytterhoeven
2024-05-02 18:01 ` Wolfram Sang
2024-04-16 12:35 ` [RFC PATCH 2/2] serial: sh-sci: always cancel hrtimer when DMA RX is invalidated Wolfram Sang
2024-04-24 9:48 ` Geert Uytterhoeven
2024-05-02 18:17 ` Wolfram Sang [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=20240502181737.b5vvghnwzievvlgj@ninjato \
--to=wsa+renesas@sang-engineering.com \
--cc=amitev@visteon.com \
--cc=dirk.behme@de.bosch.com \
--cc=geert+renesas@glider.be \
--cc=geert@linux-m68k.org \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
/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).