linux-kernel - Re: [PATCH] pinctrl: sunxi: Use minimal debouncing period as default

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZzyoIABRArkGoZBn@collins>
Date: Tue, 19 Nov 2024 16:00:48 +0100
From: Paul Kocialkowski <paulk@...-base.io>
To: Maxime Ripard <mripard@...nel.org>
Cc: linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	Uwe Kleine-König <ukleinek@...nel.org>,
	Chen-Yu Tsai <wens@...e.org>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Samuel Holland <samuel@...lland.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Paul Kocialkowski <contact@...lk.fr>
Subject: Re: [PATCH] pinctrl: sunxi: Use minimal debouncing period as default

Hi Maxime,

Le Tue 19 Nov 24, 15:43, Maxime Ripard a écrit :
> On Tue, Nov 19, 2024 at 03:08:05PM +0100, Paul Kocialkowski wrote:
> > From: Paul Kocialkowski <contact@...lk.fr>
> > 
> > The sunxi external interrupts (available from GPIO pins) come with a
> > built-in debouncing mechanism that cannot be disabled. It can be
> > configured to use either the low-frequency oscillator (32 KHz) or the
> > high-frequency oscillator (24 MHz), with a pre-scaler.
> > 
> > The pinctrl code supports an input-debounce device-tree property to set
> > a specific debouncing period and choose which clock source is most
> > relevant. However the property is specified in microseconds, which is
> > longer than the minimal period achievable from the high-frequency
> > oscillator without a pre-scaler.
> 
> That can be fixed by introducing a new property with a ns resolution.

Sure but my point here is rather about what should be default behavior.

The issue I had will remain unsolved by default even with a new property,
since people will still need to patch their device-tree to apply it.

> > When the property is missing, the reset configuration is kept, which
> > selects the low-frequency oscillator without pre-scaling. This severely
> > limits the possible interrupt periods that can be detected.
> > 
> > Instead of keeping this default, use the minimal debouncing period from
> > the high-frequency oscillator without a pre-scaler to allow the largest
> > possible range of interrupt periods.
> > 
> > This issue was encountered with a peripheral that generates active-low
> > interrupts for 1 us. No interrupt was detected with the default setup,
> > while it is now correctly detected with this change.
> 
> I don't think it's wise. If the debouncing is kept as is, the worst case
> scenario is the one you had: a device doesn't work, you change it,
> everything works.

I think this worst-case scenario is very bad and not what people will
expect. In addition it is difficult to debug the issue without specific
knowledge about the SoC.

My use-case here was hooking up a sparkfun sensor board by the way,
not some very advanced corner-case.

> If we set it up as fast as it can however, then our risk becomes
> thousands of spurious interrupts, which is much more detrimental to the
> system.

Keep in mind that this only concerns external GPIO-based interrupts,
which have to be explicitely hooked to a device. If a device or circuit
does generate such spurious interrupts, I think it makes sense that it
would be reflected by default.

Also the notion of spurious interrupt is pretty vague. Having lots of
interrupts happening may be the desired behavior in many cases.

In any case I don't think it makes sense for the platform code to impose
what a reasonable period for interrupts is (especially with such a large
period as default). Some drivers also have mechanisms to detect spurious
interrupts based on their specific use case.

> And that's without accounting the fact that devices might have relied on
> that default for years

They definitely shouldn't have. This feels much closer to a bug, and relying
on a bug not being fixed is not a reasonable expectation.

Cheers,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Specialist in multimedia, graphics and embedded hardware support with Linux.

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)