linux-kernel - Re: [PATCH v7 09/11] input: keyboard: Add support for MAX7360 keypad

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: <7nuovfjsp7dqklz7khk4biabo22d7qpqqajpcte656vinoft6c@ot6epnbgzgp4>
Date: Mon, 5 May 2025 22:11:04 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>, 
	Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Kamel Bouhara <kamel.bouhara@...tlin.com>, Linus Walleij <linus.walleij@...aro.org>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Uwe Kleine-König <ukleinek@...nel.org>, 
	Michael Walle <mwalle@...nel.org>, Mark Brown <broonie@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Danilo Krummrich <dakr@...nel.org>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-input@...r.kernel.org, linux-pwm@...r.kernel.org, 
	Grégory Clement <gregory.clement@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v7 09/11] input: keyboard: Add support for MAX7360 keypad

On Fri, May 02, 2025 at 04:32:33PM +0300, Andy Shevchenko wrote:
> On Fri, May 02, 2025 at 03:15:34PM +0200, Mathieu Dubois-Briand wrote:
> > On Fri May 2, 2025 at 12:46 PM CEST, Andy Shevchenko wrote:
> > > On Mon, Apr 28, 2025 at 01:57:27PM +0200, Mathieu Dubois-Briand wrote:
> 
> ...
> 
> > >> +static irqreturn_t max7360_keypad_irq(int irq, void *data)
> > >> +{
> > >> +	struct max7360_keypad *max7360_keypad = data;
> > >> +	struct device *dev = max7360_keypad->input->dev.parent;
> > >> +	unsigned int val;
> > >> +	unsigned int row, col;
> > >> +	unsigned int release;
> > >> +	unsigned int code;
> > >> +	int error;
> > >> +
> > >> +	do {
> > >> +		error = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO, &val);
> > >> +		if (error) {
> > >> +			dev_err(dev, "Failed to read max7360 FIFO");
> > >> +			return IRQ_NONE;
> > >> +		}
> > >> +
> > >> +		/* FIFO overflow: ignore it and get next event. */
> > >> +		if (val == MAX7360_FIFO_OVERFLOW)
> > >> +			dev_warn(dev, "max7360 FIFO overflow");
> > >
> > > If many events are missing this will flood the logs, perhaps _ratelimited() ?
> > >
> > >> +	} while (val == MAX7360_FIFO_OVERFLOW);
> > >
> > > regmap_read_poll_timeout() ?
> > 
> > OK, I can try something like:
> > 
> > +       error = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO, &val);
> > +
> > +       /* FIFO overflow: ignore it and get next event. */
> > +       if (!error && (val == MAX7360_FIFO_OVERFLOW)) {
> > +               dev_warn(dev, "max7360 FIFO overflow");
> > +               error = regmap_read_poll_timeout(max7360_keypad->regmap, MAX7360_REG_KEYFIFO,
> > +                                                val, val != MAX7360_FIFO_OVERFLOW, 0, 0);
> > +       }
> > +
> > +       if (error) {
> > +               dev_err(dev, "Failed to read max7360 FIFO");
> > +               return IRQ_NONE;
> > +       }
> 
> Maybe something like this (see also below about timeouts)?
> 
>        error = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO, &val);
>        if (error) {
>                dev_err(dev, "Failed to read MAX7360 FIFO");
>                return IRQ_NONE;
>        }
> 
>        /* FIFO overflow: ignore it and get next event. */
>        if (val == MAX7360_FIFO_OVERFLOW) {
>                dev_warn(dev, "max7360 FIFO overflow");
>                error = regmap_read_poll_timeout(max7360_keypad->regmap, MAX7360_REG_KEYFIFO,
>                                                 val, val != MAX7360_FIFO_OVERFLOW, 0, 1000);
>                if (error) {
>                        dev_err(dev, "Failed to empty MAX7360 FIFO");
>                        return IRQ_NONE;
>                }
>        }
> 
> > Sleep_us is 0 as we are in the IRQ handler,
> 
> Isn't it under the mutex, so we are fine to have small delays? But in general
> it seems not okay to sleep here. In any case 0 for sleep_us gives an atomic read.

This is in a threaded interrupt, you can sleep all you want.

Thanks.

-- 
Dmitry