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]

Message-ID: <585d09d8-b2ba-97e3-992e-404364fa6bb5@samsung.com>
Date:   Thu, 17 Feb 2022 15:09:49 +0100
From:   Marek Szyprowski <m.szyprowski@...sung.com>
To:     Charles Keepax <ckeepax@...nsource.cirrus.com>,
        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>
Subject: Re: [PATCH v3] regmap-irq: Update interrupt clear register for
 proper reset

On 17.02.2022 14:17, Charles Keepax wrote:
> 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.

Both devices affected by the previous version of the patch work fine 
with this version.

Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>

>> 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
>>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland