linux-kernel - Re: [PATCH] ARM: dts: allwinner: minimize irq debounce filter per 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: <20230213084329.ulckaigwd7dof37u@houat>
Date:   Mon, 13 Feb 2023 09:43:29 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Andre Przywara <andre.przywara@....com>
Cc:     Andreas Feldner <andreas@...dner-bv.de>,
        Andreas Feldner <pelzi@...ing-snail.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ARM: dts: allwinner: minimize irq debounce filter per
 default

On Fri, Feb 10, 2023 at 10:18:14AM +0000, Andre Przywara wrote:
> > > Not sure if you were actually arguing this, but the change I sketched
> > > above (interpreting 0 as 24MHz/1) is separate though, as the current
> > > default is "no DT property", and not 0. There is no input-debounce
> > > property user in the kernel tree at the moment, so we wouldn't break
> > > anyone. The only thing that would change is if a downstream user was
> > > relying on "0" being interpreted as "skip the setup", which isn't
> > > really documented and could be argued to be an implementation detail.
> > > 
> > > So I'd suggest to implement 0 as "lowest possible", and documenting that
> > > and the 32KHz/1 default if no property is given.  
> > 
> > Ah, my bad.
> > 
> > There's another thing to consider: there's already a generic per-pin
> > input-debounce property in pinctrl.
> > 
> > Since we can't control it per pin but per bank, we moved it to the
> > controller back then, but there's always been this (implicit)
> > expectation that it was behaving the same way.
> > 
> > And the generic, per-pin, input-debounce documentation says:
> > 
> > > Takes the debounce time in usec as argument or 0 to disable debouncing  
> > 
> > I agree that silently ignoring it is not great, but interpreting 0 as
> > the lowest possible is breaking that behaviour which, I believe, is a
> > worse outcome.
> 
> Is it really? If I understand the hardware manuals correctly, we cannot
> really turn that feature off, so isn't the lowest possible time period (24
> MHz/1 at the moment) the closest we can get to "turn it off"? So
> implementing this would bring us actually closer to the documented
> behaviour? Or did I get the meaning of this time period wrong?
> At least that's my understanding of how it fixed Andreas' problem: 1µs
> is still not "off", but much better than the 31µs of the default. The new
> 0 would then be 0.041µs.

My point was that the property we share the name (and should share the
semantics with) documents 0 as disabled. We would have a behavior that
doesn't disable it. It's inconsistent.

The reason doesn't really matter, we would share the same name but have
a completely different behavior, this is super confusing to me.

Maxime