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

[Paul Kocialkowski]

Hi,

Le Mon 02 Dec 24, 12:03, Maxime Ripard a écrit :
> On Sat, Nov 30, 2024 at 11:34:08AM +0100, Paul Kocialkowski wrote:
> > Well I'm an electrical engineer and the first thing we were told about
> > buttons and connectors is to include hardware debouncing. The second thing
> > is that it can be done in software (which again is done in a number of drivers)
> > by just disabling the interrupt for a while if it happens too often.
> > 
> > So I'm quite affirmative that taking none of these into account is constitutive
> > of a broken hardware design. No electrical engineer is told that they shouldn't
> > care about this because the SoC will filter interrupts for them.
> 
> The SoC provides the hardware debouncing. There's no reason not to use
> it, or to choose something redundant. Some might, but it's also
> perfectly valid to just rely on the SoC there.
> 
> > Of course it's fine to use this mechanism when it exists, but it's not a
> > reasonable expectation to just assume it will always be there. This is why
> > I think it's not a legitimate reason to make it a default.
> 
> Nobody ever designed a board without considering the SoC features but
> rather by adhering to a dogma. The SoC features, components chosen and
> their price, etc. all play a role.

Okay that's fair enough. I guess what I had in mind was that it would be
unusual and unlikely. I don't think it disqualifies the fact that it would
be sensible to enable this behavior with the property and not the other way
round though.

> > > But let me rephrase if my main objection wasn't clear enough: you want
> > > to introduce an ABI breaking change. With the possibility of breaking
> > > devices that have worked fine so far. That's not ok.
> > 
> > I believe it is highly questionable that this constitutes ABI breakage.
> > To me there was no defined behavior in the first place, since the debouncing
> > configuration is inherited either from the reset value or the boot stage.
> > There is also no formal indication of what the default is, anywhere.
> 
> Depending on the interpretation, it either means that you change the
> default, or add a default, to a device-tree property. That constitutes
> an ABI breakage on its own right. And then we can introduce regressions
> for boards, which is another breakage.

It feels very questionable that adding a default over undefined behavior
constitutes an ABI breakage.

> > Changing the default configuration of hardware is commonplace. One might
> > for example introduce a reset to a driver to get a clean state because it
> > happened that some boot software would mess it up and it went unnoticed for
> > a while. Would you also call that ABI breakage?
> 
> No, because it doesn't require a change in the default state Linux
> expects when it boots, or changing anything in the device tree. It's a
> self-contained change, and thus there's no interface to break.

It could definitely result in changes in behavior though. One could imagine
the case of a sensor that was configured with 4x gain by the boot software,
which wasn't reset by the driver. Now when the driver is updated to include a
reset the sensor will fall back to 1x gain, with a direct result on the values
that are read with default configuration.

This would however be perfectly fine since it wasn't specified anywhere that the
sensor should report values with 4x gain in its default state. It's a behavior
change that has direct effect, but doesn't break any ABI at all.

> > I think there's a number of situations where it's much more sensible to change
> > a default state to avoid very visible and practical issues. And it does happen.
> > 
> > Also my understanding of the "ABI breakage" rule in the kernel is that no
> > userspace program should stop working when it was implemented to (correctly)
> > follow some kernel-related ABI. It doesn't mean that we cannot change any
> > default state
> 
> If applications rely on that default one way or another, it's absolutely
> what it means. The only criteria is whether this will create a
> regression for any application.

For any application *that uses the ABI correctly*. Any example of defaults that
are inherited from the boot phase clearly shows that it is not a reasonable
expectation to rely on them and doing so is not correct use of the ABI.
It is rather very clearly relying on undefined behavior.

Cheers,

Paul

-- 
Paul Kocialkowski,

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

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

Attachment: signature.asc
Description: PGP signature