linux-kernel - Re: [PATCH v2 1/3] Input: ti_am335x_tsc

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] [day] [month] [year] [list]

Date:   Tue, 17 Apr 2018 13:50:18 +0530
From:   Vignesh R <vigneshr@...com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:     "Strashko, Grygorii" <grygorii.strashko@...com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
        Tony Lindgren <tony@...mide.com>
Subject: Re: [PATCH v2 1/3] Input: ti_am335x_tsc - Mark IRQ as wakeup capable

Hi,

On Monday 16 April 2018 11:15 PM, Dmitry Torokhov wrote:
> On Sat, Apr 14, 2018 at 03:21:51PM +0530, Vignesh R wrote:
>> On AM335x, ti_am335x_tsc can wake up the system from suspend, mark the
>> IRQ as wakeup capable, so that device irq is not disabled during system
>> suspend.
>> 
>> Signed-off-by: Vignesh R <vigneshr@...com>
>> ---
>> 
>> v2: No changes
>> 
>>  drivers/input/touchscreen/ti_am335x_tsc.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>> index f1043ae71dcc..810e05c9c4f5 100644
>> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
>> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>> @@ -27,6 +27,7 @@
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>>  #include <linux/sort.h>
>> +#include <linux/pm_wakeirq.h>
>>  
>>  #include <linux/mfd/ti_am335x_tscadc.h>
>>  
>> @@ -432,6 +433,12 @@ static int titsc_probe(struct platform_device *pdev)
>>                goto err_free_mem;
>>        }
>>  
>> +     if (device_may_wakeup(tscadc_dev->dev)) {
>> +             err = dev_pm_set_wake_irq(tscadc_dev->dev, ts_dev->irq);
> 
> Hmm, most of the drivers simply use enable_irq_wake()/disable_irq_wake()
> in suspend/resume paths 

dev_pm_*_wake_irq() function are alternative to above

[1]:
For most drivers, we should be able to drop the following
boilerplate code from runtime_suspend and runtime_resume
functions:

	...
	device_init_wakeup(dev, true);
	...
	if (device_may_wakeup(dev))
		enable_irq_wake(irq);
	...
	if (device_may_wakeup(dev))
		disable_irq_wake(irq);
	...
	device_init_wakeup(dev, false);
	...

We can replace it with just the following init and exit
time code:

	...
	device_init_wakeup(dev, true);
	dev_pm_set_wake_irq(dev, irq);
	...
	dev_pm_clear_wake_irq(dev);
	device_init_wakeup(dev, false);

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/power/wakeirq.c?id=4990d4fe327b9d9a7a3be7103a82699406fdde69

I see device_may_wakeup() check is not required. Will drop that in next version.

> and use dev_pm_set_wake_irq() only for dedicated
> and distinct wake interrupts. Why do we not follow the same pattern
> here?

Thats dev_pm_*_dedicated_wake_irq()


Thanks for the review!


-- 
Regards
Vignesh