diff mbox series

[2/3] Input: axp20x-pek - Respect userspace wakeup configuration

Message ID 20200113032032.38709-2-samuel@sholland.org (mailing list archive)
State New, archived
Headers show
Series [1/3] Input: axp20x-pek - Remove unique wakeup event handling | expand

Commit Message

Samuel Holland Jan. 13, 2020, 3:20 a.m. UTC 
Unlike most other power button drivers, this driver unconditionally
enables its wakeup IRQ. It should be using device_may_wakeup() to
respect the userspace configuration of wakeup sources.

Because the AXP20x MFD device uses regmap-irq, the AXP20x PEK IRQs are
nested off of regmap-irq's threaded interrupt handler. The device core
ignores such interrupts, so to actually disable wakeup, we must
explicitly disable all non-wakeup interrupts during suspend.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/input/misc/axp20x-pek.c | 42 ++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

Comments

kernel test robot Jan. 13, 2020, 4:47 a.m. UTC | #1 
Hi Samuel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on input/next]
[also build test WARNING on v5.5-rc5 next-20200110]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Samuel-Holland/Input-axp20x-pek-Remove-unique-wakeup-event-handling/20200113-112123
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/input/misc/axp20x-pek.c:356:5: warning: "CONFIG_PM_SLEEP" is not defined, evaluates to 0 [-Wundef]
    #if CONFIG_PM_SLEEP
        ^~~~~~~~~~~~~~~

vim +/CONFIG_PM_SLEEP +356 drivers/input/misc/axp20x-pek.c

   355	
 > 356	#if CONFIG_PM_SLEEP
   357	static int axp20x_pek_suspend(struct device *dev)
   358	{
   359		struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
   360	
   361		/*
   362		 * Nested threaded interrupts are not automatically
   363		 * disabled, so we must do it explicitly.
   364		 */
   365		if (device_may_wakeup(dev)) {
   366			enable_irq_wake(axp20x_pek->irq_dbf);
   367			enable_irq_wake(axp20x_pek->irq_dbr);
   368		} else {
   369			disable_irq(axp20x_pek->irq_dbf);
   370			disable_irq(axp20x_pek->irq_dbr);
   371		}
   372	
   373		return 0;
   374	}
   375	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
Hans de Goede Jan. 13, 2020, 10:48 a.m. UTC | #2 
Hi,

On 13-01-2020 04:20, Samuel Holland wrote:
> Unlike most other power button drivers, this driver unconditionally
> enables its wakeup IRQ. It should be using device_may_wakeup() to
> respect the userspace configuration of wakeup sources.
> 
> Because the AXP20x MFD device uses regmap-irq, the AXP20x PEK IRQs are
> nested off of regmap-irq's threaded interrupt handler. The device core
> ignores such interrupts, so to actually disable wakeup, we must
> explicitly disable all non-wakeup interrupts during suspend.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>   drivers/input/misc/axp20x-pek.c | 42 ++++++++++++++++++++++++++++++++-
>   1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
> index 7d0ee5bececb..38cd4a4aeb65 100644
> --- a/drivers/input/misc/axp20x-pek.c
> +++ b/drivers/input/misc/axp20x-pek.c
> @@ -280,7 +280,7 @@ static int axp20x_pek_probe_input_device(struct axp20x_pek *axp20x_pek,
>   	}
>   
>   	if (axp20x_pek->axp20x->variant == AXP288_ID)
> -		enable_irq_wake(axp20x_pek->irq_dbr);
> +		device_init_wakeup(&pdev->dev, true);
>   
>   	return 0;
>   }
> @@ -352,6 +352,45 @@ static int axp20x_pek_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +#if CONFIG_PM_SLEEP

As the kbuild test robot pointed out, you need to use #ifdef here.

Otherwise this patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> +static int axp20x_pek_suspend(struct device *dev)
> +{
> +	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
> +
> +	/*
> +	 * Nested threaded interrupts are not automatically
> +	 * disabled, so we must do it explicitly.
> +	 */
> +	if (device_may_wakeup(dev)) {
> +		enable_irq_wake(axp20x_pek->irq_dbf);
> +		enable_irq_wake(axp20x_pek->irq_dbr);
> +	} else {
> +		disable_irq(axp20x_pek->irq_dbf);
> +		disable_irq(axp20x_pek->irq_dbr);
> +	}
> +
> +	return 0;
> +}
> +
> +static int axp20x_pek_resume(struct device *dev)
> +{
> +	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev)) {
> +		disable_irq_wake(axp20x_pek->irq_dbf);
> +		disable_irq_wake(axp20x_pek->irq_dbr);
> +	} else {
> +		enable_irq(axp20x_pek->irq_dbf);
> +		enable_irq(axp20x_pek->irq_dbr);
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(axp20x_pek_pm_ops, axp20x_pek_suspend,
> +					    axp20x_pek_resume);
> +
>   static const struct platform_device_id axp_pek_id_match[] = {
>   	{
>   		.name = "axp20x-pek",
> @@ -371,6 +410,7 @@ static struct platform_driver axp20x_pek_driver = {
>   	.driver		= {
>   		.name		= "axp20x-pek",
>   		.dev_groups	= axp20x_groups,
> +		.pm		= &axp20x_pek_pm_ops,
>   	},
>   };
>   module_platform_driver(axp20x_pek_driver);
>
Dmitry Torokhov Jan. 13, 2020, 9:38 p.m. UTC | #3 
On Mon, Jan 13, 2020 at 11:48:35AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 13-01-2020 04:20, Samuel Holland wrote:
> > Unlike most other power button drivers, this driver unconditionally
> > enables its wakeup IRQ. It should be using device_may_wakeup() to
> > respect the userspace configuration of wakeup sources.
> > 
> > Because the AXP20x MFD device uses regmap-irq, the AXP20x PEK IRQs are
> > nested off of regmap-irq's threaded interrupt handler. The device core
> > ignores such interrupts, so to actually disable wakeup, we must
> > explicitly disable all non-wakeup interrupts during suspend.
> > 
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
> > ---
> >   drivers/input/misc/axp20x-pek.c | 42 ++++++++++++++++++++++++++++++++-
> >   1 file changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
> > index 7d0ee5bececb..38cd4a4aeb65 100644
> > --- a/drivers/input/misc/axp20x-pek.c
> > +++ b/drivers/input/misc/axp20x-pek.c
> > @@ -280,7 +280,7 @@ static int axp20x_pek_probe_input_device(struct axp20x_pek *axp20x_pek,
> >   	}
> >   	if (axp20x_pek->axp20x->variant == AXP288_ID)
> > -		enable_irq_wake(axp20x_pek->irq_dbr);
> > +		device_init_wakeup(&pdev->dev, true);
> >   	return 0;
> >   }
> > @@ -352,6 +352,45 @@ static int axp20x_pek_probe(struct platform_device *pdev)
> >   	return 0;
> >   }
> > +#if CONFIG_PM_SLEEP
> 
> As the kbuild test robot pointed out, you need to use #ifdef here.

I prefer __maybe_unused as this gives more compile coverage.

Thanks.
kernel test robot Jan. 14, 2020, 4:50 a.m. UTC | #4 
Hi Samuel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on input/next]
[also build test WARNING on v5.5-rc6 next-20200110]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Samuel-Holland/Input-axp20x-pek-Remove-unique-wakeup-event-handling/20200113-112123
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: x86_64-randconfig-a001-20200112 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/input/misc/axp20x-pek.c:356:5: warning: "CONFIG_PM_SLEEP" is not defined [-Wundef]
    #if CONFIG_PM_SLEEP
        ^
   Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:ffs
   Cyclomatic Complexity 1 include/linux/pm_wakeup.h:device_set_wakeup_capable
   Cyclomatic Complexity 1 include/linux/pm_wakeup.h:device_set_wakeup_enable
   Cyclomatic Complexity 1 include/linux/pm_wakeup.h:device_init_wakeup
   Cyclomatic Complexity 1 include/linux/device.h:dev_get_drvdata
   Cyclomatic Complexity 1 include/linux/device.h:dev_set_drvdata
   Cyclomatic Complexity 1 include/linux/input.h:input_get_drvdata
   Cyclomatic Complexity 1 include/linux/input.h:input_set_drvdata
   Cyclomatic Complexity 1 include/linux/platform_device.h:platform_set_drvdata
   Cyclomatic Complexity 1 drivers/input/misc/axp20x-pek.c:axp20x_pek_should_register_input
   Cyclomatic Complexity 1 drivers/input/misc/axp20x-pek.c:axp20x_pek_driver_init
   Cyclomatic Complexity 22 drivers/input/misc/axp20x-pek.c:axp20x_store_attr
   Cyclomatic Complexity 1 drivers/input/misc/axp20x-pek.c:axp20x_store_attr_shutdown
   Cyclomatic Complexity 1 drivers/input/misc/axp20x-pek.c:axp20x_store_attr_startup
   Cyclomatic Complexity 8 drivers/input/misc/axp20x-pek.c:axp20x_show_attr
   Cyclomatic Complexity 1 drivers/input/misc/axp20x-pek.c:axp20x_show_attr_shutdown
   Cyclomatic Complexity 1 drivers/input/misc/axp20x-pek.c:axp20x_show_attr_startup
   Cyclomatic Complexity 1 include/linux/device.h:devm_kzalloc
   Cyclomatic Complexity 22 drivers/input/misc/axp20x-pek.c:axp20x_pek_probe_input_device
   Cyclomatic Complexity 12 drivers/input/misc/axp20x-pek.c:axp20x_pek_probe
   Cyclomatic Complexity 1 include/linux/input.h:input_report_key
   Cyclomatic Complexity 1 include/linux/input.h:input_sync
   Cyclomatic Complexity 7 drivers/input/misc/axp20x-pek.c:axp20x_pek_irq
   Cyclomatic Complexity 1 drivers/input/misc/axp20x-pek.c:axp20x_pek_driver_exit

vim +/CONFIG_PM_SLEEP +356 drivers/input/misc/axp20x-pek.c

   355	
 > 356	#if CONFIG_PM_SLEEP
   357	static int axp20x_pek_suspend(struct device *dev)
   358	{
   359		struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
   360	
   361		/*
   362		 * Nested threaded interrupts are not automatically
   363		 * disabled, so we must do it explicitly.
   364		 */
   365		if (device_may_wakeup(dev)) {
   366			enable_irq_wake(axp20x_pek->irq_dbf);
   367			enable_irq_wake(axp20x_pek->irq_dbr);
   368		} else {
   369			disable_irq(axp20x_pek->irq_dbf);
   370			disable_irq(axp20x_pek->irq_dbr);
   371		}
   372	
   373		return 0;
   374	}
   375	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
diff mbox series

Patch

diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
index 7d0ee5bececb..38cd4a4aeb65 100644
--- a/drivers/input/misc/axp20x-pek.c
+++ b/drivers/input/misc/axp20x-pek.c
@@ -280,7 +280,7 @@  static int axp20x_pek_probe_input_device(struct axp20x_pek *axp20x_pek,
 	}
 
 	if (axp20x_pek->axp20x->variant == AXP288_ID)
-		enable_irq_wake(axp20x_pek->irq_dbr);
+		device_init_wakeup(&pdev->dev, true);
 
 	return 0;
 }
@@ -352,6 +352,45 @@  static int axp20x_pek_probe(struct platform_device *pdev)
 	return 0;
 }
 
+#if CONFIG_PM_SLEEP
+static int axp20x_pek_suspend(struct device *dev)
+{
+	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
+
+	/*
+	 * Nested threaded interrupts are not automatically
+	 * disabled, so we must do it explicitly.
+	 */
+	if (device_may_wakeup(dev)) {
+		enable_irq_wake(axp20x_pek->irq_dbf);
+		enable_irq_wake(axp20x_pek->irq_dbr);
+	} else {
+		disable_irq(axp20x_pek->irq_dbf);
+		disable_irq(axp20x_pek->irq_dbr);
+	}
+
+	return 0;
+}
+
+static int axp20x_pek_resume(struct device *dev)
+{
+	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev)) {
+		disable_irq_wake(axp20x_pek->irq_dbf);
+		disable_irq_wake(axp20x_pek->irq_dbr);
+	} else {
+		enable_irq(axp20x_pek->irq_dbf);
+		enable_irq(axp20x_pek->irq_dbr);
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(axp20x_pek_pm_ops, axp20x_pek_suspend,
+					    axp20x_pek_resume);
+
 static const struct platform_device_id axp_pek_id_match[] = {
 	{
 		.name = "axp20x-pek",
@@ -371,6 +410,7 @@  static struct platform_driver axp20x_pek_driver = {
 	.driver		= {
 		.name		= "axp20x-pek",
 		.dev_groups	= axp20x_groups,
+		.pm		= &axp20x_pek_pm_ops,
 	},
 };
 module_platform_driver(axp20x_pek_driver);