diff mbox series

[v5,2/2] pinctrl: amd: Fix wakeups when IRQ is shared with SCI

Message ID 20211028144541.12617-2-mario.limonciello@amd.com
State New
Headers show
Series [v5,1/2] ACPI: Add stubs for wakeup handler functions | expand

Commit Message

Mario Limonciello Oct. 28, 2021, 2:45 p.m. UTC 
On some Lenovo AMD Gen2 platforms the IRQ for the SCI and pinctrl drivers
are shared.  Due to how the s2idle loop handling works, this case needs
an extra explicit check whether the interrupt was caused by SCI or by
the GPIO controller.

To fix this rework the existing IRQ handler function to function as a
checker and an IRQ handler depending on the calling arguments.

Cc: stable@kernel.org
BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1738
Reported-by: Joerie de Gram <j.de.gram@gmail.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
Note:
 This is *possibly* a fix from fdde0ff8590b, 56b991849009 or other
 changes in the acpi_s2idle_wake handling, but AMD didn't support
 s2idle across the kernel widely until 5.14 or later. This is the
 reason for lack of a fixes tag.
Changes from v4->v5:
 * Correct debugging statement that was obfuscated by kernel %p behavior.
 * Instead show actual GPIO number, which is much more useful for debugging
 * Target to stable as well
Changes from v3->v4:
 * Adjust to reverse xmas for newly added variable
 * Add a debugging line to show the values in the register that caused system
   wake to allow for debugging of spurious wakeups
Changes from v2->v3:
 * Add new precursor patch for fixing missing ACPI function stubs
 * Add __maybe_unused for unused function when set with COMPILE_TEST
Changes from v1->v2:
 * drop Kconfig changes to drop COMPILE_TEST, instead #ifdef CONFIG_ACPI
 * fix a logic error during wakeup
 * Use IRQ_RETVAL()
 drivers/pinctrl/pinctrl-amd.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

Comments

Basavaraj Natikar Oct. 28, 2021, 2:58 p.m. UTC | #1 
On 10/28/2021 8:15 PM, Mario Limonciello wrote:
> On some Lenovo AMD Gen2 platforms the IRQ for the SCI and pinctrl drivers
> are shared.  Due to how the s2idle loop handling works, this case needs
> an extra explicit check whether the interrupt was caused by SCI or by
> the GPIO controller.
>
> To fix this rework the existing IRQ handler function to function as a
> checker and an IRQ handler depending on the calling arguments.
>
> Cc: stable@kernel.org
> BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1738
> Reported-by: Joerie de Gram <j.de.gram@gmail.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Note:
>  This is *possibly* a fix from fdde0ff8590b, 56b991849009 or other
>  changes in the acpi_s2idle_wake handling, but AMD didn't support
>  s2idle across the kernel widely until 5.14 or later. This is the
>  reason for lack of a fixes tag.
> Changes from v4->v5:
>  * Correct debugging statement that was obfuscated by kernel %p behavior.
>  * Instead show actual GPIO number, which is much more useful for debugging
>  * Target to stable as well
> Changes from v3->v4:
>  * Adjust to reverse xmas for newly added variable
>  * Add a debugging line to show the values in the register that caused system
>    wake to allow for debugging of spurious wakeups
> Changes from v2->v3:
>  * Add new precursor patch for fixing missing ACPI function stubs
>  * Add __maybe_unused for unused function when set with COMPILE_TEST
> Changes from v1->v2:
>  * drop Kconfig changes to drop COMPILE_TEST, instead #ifdef CONFIG_ACPI
>  * fix a logic error during wakeup
>  * Use IRQ_RETVAL()
>  drivers/pinctrl/pinctrl-amd.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index d19974aceb2e..83fbcdad0904 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -598,14 +598,14 @@ static struct irq_chip amd_gpio_irqchip = {
>  
>  #define PIN_IRQ_PENDING	(BIT(INTERRUPT_STS_OFF) | BIT(WAKE_STS_OFF))
>  
> -static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
> +static bool _amd_gpio_irq_handler(int irq, void *dev_id)
>  {
>  	struct amd_gpio *gpio_dev = dev_id;
>  	struct gpio_chip *gc = &gpio_dev->gc;
> -	irqreturn_t ret = IRQ_NONE;
>  	unsigned int i, irqnr;
>  	unsigned long flags;
>  	u32 __iomem *regs;
> +	bool ret = false;
>  	u32  regval;
>  	u64 status, mask;
>  
> @@ -627,6 +627,16 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
>  		/* Each status bit covers four pins */
>  		for (i = 0; i < 4; i++) {
>  			regval = readl(regs + i);
> +			/* called from resume context on a shared IRQ, just
> +			 * checking wake source.
> +			 */
> +			if (irq < 0 && (regval & BIT(WAKE_STS_OFF))) {
> +				dev_dbg(&gpio_dev->pdev->dev,
> +					"Waking due to GPIO %ld: 0x%x",
> +					regs + i - ((u32 __iomem *)gpio_dev->base), regval);
> +				return true;
> +			}
> +
>  			if (!(regval & PIN_IRQ_PENDING) ||
>  			    !(regval & BIT(INTERRUPT_MASK_OFF)))
>  				continue;
> @@ -652,9 +662,12 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
>  			}
>  			writel(regval, regs + i);
>  			raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> -			ret = IRQ_HANDLED;
> +			ret = true;
>  		}
>  	}
> +	/* called from resume context on shared IRQ but didn't cause wake */
> +	if (irq < 0)
> +		return false;
>  
>  	/* Signal EOI to the GPIO unit */
>  	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> @@ -666,6 +679,16 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
>  	return ret;
>  }
>  
> +static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
> +{
> +	return IRQ_RETVAL(_amd_gpio_irq_handler(irq, dev_id));
> +}
> +
> +static bool __maybe_unused amd_gpio_check_wake(void *dev_id)
> +{
> +	return _amd_gpio_irq_handler(-1, dev_id);
> +}
> +
>  static int amd_get_groups_count(struct pinctrl_dev *pctldev)
>  {
>  	struct amd_gpio *gpio_dev = pinctrl_dev_get_drvdata(pctldev);
> @@ -1004,6 +1027,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
>  		goto out2;
>  
>  	platform_set_drvdata(pdev, gpio_dev);
> +	acpi_register_wakeup_handler(gpio_dev->irq, amd_gpio_check_wake, gpio_dev);
>  
>  	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
>  	return ret;
> @@ -1021,6 +1045,7 @@ static int amd_gpio_remove(struct platform_device *pdev)
>  	gpio_dev = platform_get_drvdata(pdev);
>  
>  	gpiochip_remove(&gpio_dev->gc);
> +	acpi_unregister_wakeup_handler(amd_gpio_check_wake, gpio_dev);
>  
>  	return 0;
>  }

Looks good to me

Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>


Thanks,
Basavaraj
kernel test robot Oct. 29, 2021, 3:01 p.m. UTC | #2 
Hi Mario,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linusw-pinctrl/devel]
[also build test ERROR on rafael-pm/linux-next linus/master v5.15-rc7 next-20211028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mario-Limonciello/ACPI-Add-stubs-for-wakeup-handler-functions/20211028-224738
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/aa823b07a228116ebb416d3c49ec526e78c6c138
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mario-Limonciello/ACPI-Add-stubs-for-wakeup-handler-functions/20211028-224738
        git checkout aa823b07a228116ebb416d3c49ec526e78c6c138
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=powerpc 

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

All errors (new ones prefixed by >>):

   In file included from include/linux/printk.h:555,
                    from include/asm-generic/bug.h:22,
                    from arch/powerpc/include/asm/bug.h:149,
                    from include/linux/bug.h:5,
                    from drivers/pinctrl/pinctrl-amd.c:14:
   drivers/pinctrl/pinctrl-amd.c: In function '_amd_gpio_irq_handler':
>> drivers/pinctrl/pinctrl-amd.c:625:41: error: format '%ld' expects argument of type 'long int', but argument 4 has type 'int' [-Werror=format=]
     625 |                                         "Waking due to GPIO %ld: 0x%x",
         |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:29: note: in definition of macro '__dynamic_func_call'
     134 |                 func(&id, ##__VA_ARGS__);               \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:166:9: note: in expansion of macro '_dynamic_func_call'
     166 |         _dynamic_func_call(fmt,__dynamic_dev_dbg,               \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:9: note: in expansion of macro 'dynamic_dev_dbg'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:30: note: in expansion of macro 'dev_fmt'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                              ^~~~~~~
   drivers/pinctrl/pinctrl-amd.c:624:33: note: in expansion of macro 'dev_dbg'
     624 |                                 dev_dbg(&gpio_dev->pdev->dev,
         |                                 ^~~~~~~
   drivers/pinctrl/pinctrl-amd.c:625:63: note: format string is defined here
     625 |                                         "Waking due to GPIO %ld: 0x%x",
         |                                                             ~~^
         |                                                               |
         |                                                               long int
         |                                                             %d
   drivers/pinctrl/pinctrl-amd.c: In function 'amd_gpio_probe':
   drivers/pinctrl/pinctrl-amd.c:1019:46: error: 'struct amd_gpio' has no member named 'irq'
    1019 |         acpi_register_wakeup_handler(gpio_dev->irq, amd_gpio_check_wake, gpio_dev);
         |                                              ^~
   cc1: all warnings being treated as errors


vim +625 drivers/pinctrl/pinctrl-amd.c

   590	
   591	static bool _amd_gpio_irq_handler(int irq, void *dev_id)
   592	{
   593		struct amd_gpio *gpio_dev = dev_id;
   594		struct gpio_chip *gc = &gpio_dev->gc;
   595		unsigned int i, irqnr;
   596		unsigned long flags;
   597		u32 __iomem *regs;
   598		bool ret = false;
   599		u32  regval;
   600		u64 status, mask;
   601	
   602		/* Read the wake status */
   603		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
   604		status = readl(gpio_dev->base + WAKE_INT_STATUS_REG1);
   605		status <<= 32;
   606		status |= readl(gpio_dev->base + WAKE_INT_STATUS_REG0);
   607		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
   608	
   609		/* Bit 0-45 contain the relevant status bits */
   610		status &= (1ULL << 46) - 1;
   611		regs = gpio_dev->base;
   612		for (mask = 1, irqnr = 0; status; mask <<= 1, regs += 4, irqnr += 4) {
   613			if (!(status & mask))
   614				continue;
   615			status &= ~mask;
   616	
   617			/* Each status bit covers four pins */
   618			for (i = 0; i < 4; i++) {
   619				regval = readl(regs + i);
   620				/* called from resume context on a shared IRQ, just
   621				 * checking wake source.
   622				 */
   623				if (irq < 0 && (regval & BIT(WAKE_STS_OFF))) {
   624					dev_dbg(&gpio_dev->pdev->dev,
 > 625						"Waking due to GPIO %ld: 0x%x",
   626						regs + i - ((u32 __iomem *)gpio_dev->base), regval);
   627					return true;
   628				}
   629	
   630				if (!(regval & PIN_IRQ_PENDING) ||
   631				    !(regval & BIT(INTERRUPT_MASK_OFF)))
   632					continue;
   633				generic_handle_domain_irq(gc->irq.domain, irqnr + i);
   634	
   635				/* Clear interrupt.
   636				 * We must read the pin register again, in case the
   637				 * value was changed while executing
   638				 * generic_handle_domain_irq() above.
   639				 * If we didn't find a mapping for the interrupt,
   640				 * disable it in order to avoid a system hang caused
   641				 * by an interrupt storm.
   642				 */
   643				raw_spin_lock_irqsave(&gpio_dev->lock, flags);
   644				regval = readl(regs + i);
   645				if (irq == 0) {
   646					regval &= ~BIT(INTERRUPT_ENABLE_OFF);
   647					dev_dbg(&gpio_dev->pdev->dev,
   648						"Disabling spurious GPIO IRQ %d\n",
   649						irqnr + i);
   650				}
   651				writel(regval, regs + i);
   652				raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
   653				ret = true;
   654			}
   655		}
   656		/* called from resume context on shared IRQ but didn't cause wake */
   657		if (irq < 0)
   658			return false;
   659	
   660		/* Signal EOI to the GPIO unit */
   661		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
   662		regval = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
   663		regval |= EOI_MASK;
   664		writel(regval, gpio_dev->base + WAKE_INT_MASTER_REG);
   665		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
   666	
   667		return ret;
   668	}
   669	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index d19974aceb2e..83fbcdad0904 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -598,14 +598,14 @@  static struct irq_chip amd_gpio_irqchip = {
 
 #define PIN_IRQ_PENDING	(BIT(INTERRUPT_STS_OFF) | BIT(WAKE_STS_OFF))
 
-static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
+static bool _amd_gpio_irq_handler(int irq, void *dev_id)
 {
 	struct amd_gpio *gpio_dev = dev_id;
 	struct gpio_chip *gc = &gpio_dev->gc;
-	irqreturn_t ret = IRQ_NONE;
 	unsigned int i, irqnr;
 	unsigned long flags;
 	u32 __iomem *regs;
+	bool ret = false;
 	u32  regval;
 	u64 status, mask;
 
@@ -627,6 +627,16 @@  static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
 		/* Each status bit covers four pins */
 		for (i = 0; i < 4; i++) {
 			regval = readl(regs + i);
+			/* called from resume context on a shared IRQ, just
+			 * checking wake source.
+			 */
+			if (irq < 0 && (regval & BIT(WAKE_STS_OFF))) {
+				dev_dbg(&gpio_dev->pdev->dev,
+					"Waking due to GPIO %ld: 0x%x",
+					regs + i - ((u32 __iomem *)gpio_dev->base), regval);
+				return true;
+			}
+
 			if (!(regval & PIN_IRQ_PENDING) ||
 			    !(regval & BIT(INTERRUPT_MASK_OFF)))
 				continue;
@@ -652,9 +662,12 @@  static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
 			}
 			writel(regval, regs + i);
 			raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
-			ret = IRQ_HANDLED;
+			ret = true;
 		}
 	}
+	/* called from resume context on shared IRQ but didn't cause wake */
+	if (irq < 0)
+		return false;
 
 	/* Signal EOI to the GPIO unit */
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
@@ -666,6 +679,16 @@  static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
 	return ret;
 }
 
+static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
+{
+	return IRQ_RETVAL(_amd_gpio_irq_handler(irq, dev_id));
+}
+
+static bool __maybe_unused amd_gpio_check_wake(void *dev_id)
+{
+	return _amd_gpio_irq_handler(-1, dev_id);
+}
+
 static int amd_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	struct amd_gpio *gpio_dev = pinctrl_dev_get_drvdata(pctldev);
@@ -1004,6 +1027,7 @@  static int amd_gpio_probe(struct platform_device *pdev)
 		goto out2;
 
 	platform_set_drvdata(pdev, gpio_dev);
+	acpi_register_wakeup_handler(gpio_dev->irq, amd_gpio_check_wake, gpio_dev);
 
 	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
 	return ret;
@@ -1021,6 +1045,7 @@  static int amd_gpio_remove(struct platform_device *pdev)
 	gpio_dev = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&gpio_dev->gc);
+	acpi_unregister_wakeup_handler(amd_gpio_check_wake, gpio_dev);
 
 	return 0;
 }