netdev - Re: [PATCH net-next] net: ipa: use dev PM wakeirq handling

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: <bc54d9ea-aaa5-eea6-a954-807b3451d070@linaro.org>
Date:   Tue, 31 Jan 2023 12:40:21 +0000
From:   Caleb Connolly <caleb.connolly@...aro.org>
To:     Alex Elder <elder@...aro.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     elder@...nel.org, netdev@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, phone-devel@...r.kernel.org
Subject: Re: [PATCH net-next] net: ipa: use dev PM wakeirq handling



On 1/28/23 13:47, Alex Elder wrote:
> On 1/27/23 2:27 PM, Caleb Connolly wrote:
>> Replace the enable_irq_wake() call with one to dev_pm_set_wake_irq()
>> instead. This will let the dev PM framework automatically manage the
>> the wakeup capability of the ipa IRQ and ensure that userspace requests
>> to enable/disable wakeup for the IPA via sysfs are respected.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@...aro.org>
> 
> Looks OK to me.  Can you say something about how you
> tested this, and what the result was?  Thanks.

Ah yeah. This was tested on the SDM845 (IPA 3.5.1) based SHIFT6mq in the 
UK with an EE SIM card.

All network connections were disabled except for mobile data which was 
configured using ModemManager. Then I set up a basic TCP server using 
netcat on a public IP address and connected to it from the device.

It is then possible to validate that the wakeirq fires and the interrupt 
is handled correctly by putting the device into s2idle sleep (echo mem > 
/sys/power/state) and typing some data into the server terminal.

Then I disabled the wakeup as follows and repeated the test to ensure 
that the device would no longer wake up on incoming data, and that the 
data was received when the device resumes.

echo disabled > /sys/devices/platform/soc\@0/1e40000.ipa/power/wakeup

> 
>                      -Alex
> 
>> ---
>>   drivers/net/ipa/ipa_interrupt.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ipa/ipa_interrupt.c 
>> b/drivers/net/ipa/ipa_interrupt.c
>> index c19cd27ac852..9a1153e80a3a 100644
>> --- a/drivers/net/ipa/ipa_interrupt.c
>> +++ b/drivers/net/ipa/ipa_interrupt.c
>> @@ -22,6 +22,7 @@
>>   #include <linux/types.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/pm_wakeirq.h>
>>   #include "ipa.h"
>>   #include "ipa_reg.h"
>> @@ -269,9 +270,9 @@ struct ipa_interrupt *ipa_interrupt_config(struct 
>> ipa *ipa)
>>           goto err_kfree;
>>       }
>> -    ret = enable_irq_wake(irq);
>> +    ret = dev_pm_set_wake_irq(dev, irq);
>>       if (ret) {
>> -        dev_err(dev, "error %d enabling wakeup for \"ipa\" IRQ\n", ret);
>> +        dev_err(dev, "error %d registering \"ipa\" IRQ as wakeirq\n", 
>> ret);
>>           goto err_free_irq;
>>       }
>> @@ -289,11 +290,8 @@ struct ipa_interrupt *ipa_interrupt_config(struct 
>> ipa *ipa)
>>   void ipa_interrupt_deconfig(struct ipa_interrupt *interrupt)
>>   {
>>       struct device *dev = &interrupt->ipa->pdev->dev;
>> -    int ret;
>> -    ret = disable_irq_wake(interrupt->irq);
>> -    if (ret)
>> -        dev_err(dev, "error %d disabling \"ipa\" IRQ wakeup\n", ret);
>> +    dev_pm_clear_wake_irq(dev);
>>       free_irq(interrupt->irq, interrupt);
>>       kfree(interrupt);
>>   }
> 

-- 
--
Kind Regards,
Caleb (they/them)