linux-kernel - Re: [PATCH v3] regmap-irq: Update interrupt clear register for proper reset

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]

Date:   Thu, 17 Feb 2022 13:17:33 +0000
From:   Charles Keepax <ckeepax@...nsource.cirrus.com>
To:     Prasad Kumpatla <quic_pkumpatl@...cinc.com>
CC:     Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        <linux-kernel@...r.kernel.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH v3] regmap-irq: Update interrupt clear register for
 proper reset

On Thu, Feb 17, 2022 at 02:20:07PM +0530, Prasad Kumpatla wrote:

+ Marek Szyprowski, as the old version of the patch was causing
problems on his system.

> With the existing logic where clear_ack is true (HW doesn’t support
> auto clear for ICR), interrupt clear register reset is not handled
> properly. Due to this only the first interrupts get processed properly
> and further interrupts are blocked due to not resetting interrupt
> clear register.
> 
> Example for issue case where Invert_ack is false and clear_ack is true:
> 
>     Say Default ISR=0x00 & ICR=0x00 and ISR is triggered with 2
>     interrupts making ISR = 0x11.
> 
>     Step 1: Say ISR is set 0x11 (store status_buff = ISR). ISR needs to
>             be cleared with the help of ICR once the Interrupt is processed.
> 
>     Step 2: Write ICR = 0x11 (status_buff), this will clear the ISR to 0x00.
> 
>     Step 3: Issue - In the existing code, ICR is written with ICR =
>             ~(status_buff) i.e ICR = 0xEE -> This will block all the interrupts
>             from raising except for interrupts 0 and 4. So expectation here is to
>             reset ICR, which will unblock all the interrupts.
> 
>             if (chip->clear_ack) {
>                  if (chip->ack_invert && !ret)
>                   ........
>                  else if (!ret)
>                      ret = regmap_write(map, reg,
>                             ~data->status_buf[i]);
> 
> So writing 0 and 0xff (when ack_invert is true) should have no effect, other
> than clearing the ACKs just set.
> 
> Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> Signed-off-by: Prasad Kumpatla <quic_pkumpatl@...cinc.com>
> ---

I think this version looks like it should work to me.

Reviewed-by: Charles Keepax <ckeepax@...nsource.cirrus.com>

Thanks,
Charles

>  drivers/base/regmap/regmap-irq.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> index d2656581a608..4a446259a184 100644
> --- a/drivers/base/regmap/regmap-irq.c
> +++ b/drivers/base/regmap/regmap-irq.c
> @@ -189,11 +189,9 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>  				ret = regmap_write(map, reg, d->mask_buf[i]);
>  			if (d->chip->clear_ack) {
>  				if (d->chip->ack_invert && !ret)
> -					ret = regmap_write(map, reg,
> -							   d->mask_buf[i]);
> +					ret = regmap_write(map, reg, UINT_MAX);
>  				else if (!ret)
> -					ret = regmap_write(map, reg,
> -							   ~d->mask_buf[i]);
> +					ret = regmap_write(map, reg, 0);
>  			}
>  			if (ret != 0)
>  				dev_err(d->map->dev, "Failed to ack 0x%x: %d\n",
> @@ -556,11 +554,9 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
>  						data->status_buf[i]);
>  			if (chip->clear_ack) {
>  				if (chip->ack_invert && !ret)
> -					ret = regmap_write(map, reg,
> -							data->status_buf[i]);
> +					ret = regmap_write(map, reg, UINT_MAX);
>  				else if (!ret)
> -					ret = regmap_write(map, reg,
> -							~data->status_buf[i]);
> +					ret = regmap_write(map, reg, 0);
>  			}
>  			if (ret != 0)
>  				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
> @@ -817,13 +813,9 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>  					d->status_buf[i] & d->mask_buf[i]);
>  			if (chip->clear_ack) {
>  				if (chip->ack_invert && !ret)
> -					ret = regmap_write(map, reg,
> -						(d->status_buf[i] &
> -						 d->mask_buf[i]));
> +					ret = regmap_write(map, reg, UINT_MAX);
>  				else if (!ret)
> -					ret = regmap_write(map, reg,
> -						~(d->status_buf[i] &
> -						  d->mask_buf[i]));
> +					ret = regmap_write(map, reg, 0);
>  			}
>  			if (ret != 0) {
>  				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
> -- 
> 2.17.1
>