linux-kernel - Re: [PATCH v4 8/8] serial: qcom-geni: Rework TX in FIFO mode to fix hangs/lockups

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 26 Jun 2024 10:20:06 +0200
From: Johan Hovold <johan@...nel.org>
To: Doug Anderson <dianders@...omium.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Yicong Yang <yangyicong@...ilicon.com>,
	Tony Lindgren <tony@...mide.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Johan Hovold <johan+linaro@...nel.org>,
	John Ogness <john.ogness@...utronix.de>,
	linux-arm-msm@...r.kernel.org,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konrad.dybcio@...aro.org>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Stephen Boyd <swboyd@...omium.org>, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v4 8/8] serial: qcom-geni: Rework TX in FIFO mode to fix
 hangs/lockups

On Tue, Jun 25, 2024 at 07:29:38AM -0700, Doug Anderson wrote:
> On Tue, Jun 25, 2024 at 4:21 AM Johan Hovold <johan@...nel.org> wrote:

> > Right. But with a 16 1-byte word FIFO, we may be able to kick of a
> > really long transfer and just keep it running until it needs to be
> > kicked again (cf. enabling TX). The console code can easily insert
> > characters in the FIFO while the transfer is running (and would only
> > have to wait for 16 characters to drain in the worst case).
> >
> > Effectively, most of the identified issues would just go away, as
> > there's basically never any need to cancel anything except at port
> > shutdown.
> 
> Yeah, though you'd still have to make sure that the corner cases
> worked OK. You'll have to pick _some_ sort of fixed transfer size and
> make sure that all the special cases / console / kdb work if they show
> up right at the end of the transfer.

Yes, there are some details like that would need to be worked out.

> I was also a bit curious if there could be power implications with
> leaving an active TX command always in place. Perhaps geni wouldn't be
> able to drop some resources? Do you happen to know?

Hmm, good point. I'll see if I can ask someone with access to docs.

But I guess we can still continue to stop the command on stop_tx() (as
we are considering anyway) to avoid that.

> > I didn't do an in-depth analysis of the slowdown, but I did rerun the
> > tests now and I'm still seeing a 22-24% slowdown on x1e80100 with rc5.
> > This is a new platform so I compared with sc8280xp, which shows similar
> > numbers even if it's slightly faster to begin with:
> >
> >                                         sc8280xp        x1e80100
> >
> >         rc5 full series                 61 s            67 s
> >         rc5 last patch reverted         50 s            54 s
> >
> > I have a getty running and cat a 10x dmesg file of 543950 bytes to
> > /dev/ttyMSM0 from an ssh session (just catting in a serial console gives
> > similar numbers).
> 
> That's really weird / unexpected. Your hardware should be fancier than
> mine so, if anything, I'd expect it to be faster. Is there something
> causing you really bad interrupt latency or something? ...or is some
> clock misconfigured and "geni" is behaving sub-optimally?

That may be the case. I'm not seeing more interrupts with the last patch
applied, and not more time spent servicing interrupts (based on a quick
look at top), so it may just be geni taking a lot of time to start or
stop commands.

> ...although it wouldn't explain the slowness, I'd at least be a little
> curious if you've confirmed that you're running with a 16-word FIFO
> depth. See the function geni_se_get_tx_fifo_depth() where newer
> hardware can actually have larger FIFO depths.

No, I had confirmed that it is using 16 words (64 bytes).
 
> Just in case it matters, I'd be curious if you have
> `CONFIG_IRQ_TIME_ACCOUNTING=y`

I do, yes.

> Oh: one last thing to confirm: do you have kernel console output
> disabled for your tests? I've been doing tests with the kernel console
> _not_ enabled over the serial port and just an agetty there. I could
> believe things might be different if the kernel console was sending
> messages over the same port.

Yes, there has been no console output during my tests, and I get similar
results with the console disabled.

Johan