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: <20230210094425.474cfba5@donnerap.cambridge.arm.com>
Date:   Fri, 10 Feb 2023 09:44:25 +0000
From:   Andre Przywara <andre.przywara@....com>
To:     Maxime Ripard <maxime@...no.tech>
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, 10 Feb 2023 09:29:36 +0100
Maxime Ripard <maxime@...no.tech> wrote:

Hi Maxime,

thanks for the reply!

> On Thu, Feb 09, 2023 at 08:29:52PM +0000, Andre Przywara wrote:
> > > >>   &pio {
> > > >> +	/* 1ï¿½s debounce filter on both IRQ banks */  
> > > > Is that supposed to be <micro> in UTF-8? It seems to have got lost in
> > > > translation, or is that just me?  
> > > O yes, the Greek character slipped into the comment.  
> > > >> +	input-debounce = <1 1>;  
> > > > As mentioned above, I am not so sure this is generic enough to put it
> > > > here for PA. And what is the significance of "1 us", in particular? Is
> > > > that just the smallest value?    
> > > 
> > > Yes indeed it's a bit more complicated than I feel it needs to be. The
> > > configuration is taken as microseconds and translated into the best
> > > matching clock and divider by the driver. However, 0 is not translated
> > > to the lowest divider of the high speed clock as would be logical if
> > > you ask for zero microseconds, but to "leave at default". The default
> > > of the board is 0 in the register, translating to lowest divider on the
> > > _low_ speed clock.  
> > 
> > I'd say the "if (!debounce) continue;" code is just to defend against
> > the division by zero, which would be the next statement to execute.
> > 
> > We might want to change that to interpret 0 as "lowest possible", which
> > would be 24MHz/1. Please feel free to send a patch in this regard, and
> > CC: Maxime, to get some input on that idea.  
> 
> I never had any complaint on that part either, so the default looks sane
> to me.
> 
> If some board needs a higher debouncing rate, then we should obviously
> set it up in the device tree of that board, but changing it for every
> user also introduces the risk of breaking other boards that actually
> require a lower debouncing frequency.

Yeah, we definitely should keep the default at 32KHz/1, as this is also
the hardware reset value.

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.

> And any default is always going to be debated, there's one, it seems to
> create little controversy, it seems to work in most case, I'd just stick
> with it.

Agreed.

Cheers,
Andre.

> 
> > > To me this is mindboggling.
> > > 
> > > If you want to keep IRQ bank A as it is today and switch off the
> > > definitely unnecessary (and _potentially_ IRQ eating) debounce off
> > > for bank G only, I'd suggest the following setting:
> > > 
> > >      input-debounce = <31 1>;  
> > 
> > It should be documented that the effective default is 31, I guess the
> > binding is the right place.  
> 
> Yeah, if the documentation is lacking, we should definitely improve it.
> 
> Maxime