diff mbox series

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

Message ID 20230127202758.2913612-1-caleb.connolly@linaro.org (mailing list archive)
State Accepted
Commit df54fde451db9534f2fd9838d4c7d2a10ccfb6e8
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: ipa: use dev PM wakeirq handling | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Caleb Connolly Jan. 27, 2023, 8:27 p.m. UTC
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@linaro.org>
---
 drivers/net/ipa/ipa_interrupt.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Alex Elder Jan. 28, 2023, 1:47 p.m. UTC | #1
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@linaro.org>

Looks OK to me.  Can you say something about how you
tested this, and what the result was?  Thanks.

					-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);
>   }
Caleb Connolly Jan. 31, 2023, 12:40 p.m. UTC | #2
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@linaro.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);
>>   }
>
Alex Elder Jan. 31, 2023, 1:04 p.m. UTC | #3
On 1/31/23 6:40 AM, Caleb Connolly wrote:
> 
> 
> 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@linaro.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

Great explanation, thank you.  This looks good.

Reviewed-by: Alex Elder <elder@linaro.org>

>>
>>                      -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);
>>>   }
>>
>
patchwork-bot+netdevbpf@kernel.org Jan. 31, 2023, 2:30 p.m. UTC | #4
Hello:

This patch was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 27 Jan 2023 20:27:58 +0000 you 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@linaro.org>
> 
> [...]

Here is the summary with links:
  - [net-next] net: ipa: use dev PM wakeirq handling
    https://git.kernel.org/netdev/net-next/c/df54fde451db

You are awesome, thank you!
diff mbox series

Patch

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);
 }