diff mbox series

[v2,1/1] gpio: mlxbf3: Support shutdown() function

Message ID 20240611171509.22151-1-asmaa@nvidia.com
State New
Headers show
Series [v2,1/1] gpio: mlxbf3: Support shutdown() function | expand

Commit Message

Asmaa Mnebhi June 11, 2024, 5:15 p.m. UTC
During Linux graceful reboot, the GPIO interrupts are not disabled.
Since the drivers are not removed during graceful reboot,
the logic to call mlxbf3_gpio_irq_disable() is not triggered.
Interrupts that remain enabled can cause issues on subsequent boots.

For example, the mlxbf-gige driver contains PHY logic to bring up the link.
If the gpio-mlxbf3 driver loads first, the mlxbf-gige driver
will use a GPIO interrupt to bring up the link.
Otherwise, it will use polling.
The next time Linux boots and loads the drivers in this order, we encounter the issue:
- mlxbf-gige loads first and uses polling while the GPIO10
  interrupt is still enabled from the previous boot. So if
  the interrupt triggers, there is nothing to clear it.
- gpio-mlxbf3 loads.
- i2c-mlxbf loads. The interrupt doesn't trigger for I2C
  because it is shared with the GPIO interrupt line which
  was not cleared.

The solution is to add a shutdown function to the GPIO driver to clear and disable
all interrupts. Also clear the interrupt after disabling it in mlxbf3_gpio_irq_disable().

Fixes: 38a700efc510 ("gpio: mlxbf3: Add gpio driver support")
Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
Reviewed-by: David Thompson <davthompson@nvidia.com>
---
v1->v2:
- Change 'gpio' to 'GPIO'
- Use GENMASK()
- Use platform_get_drvdata()

 drivers/gpio/gpio-mlxbf3.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Linus Walleij June 17, 2024, 9:05 a.m. UTC | #1
On Tue, Jun 11, 2024 at 7:15 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> During Linux graceful reboot, the GPIO interrupts are not disabled.
> Since the drivers are not removed during graceful reboot,
> the logic to call mlxbf3_gpio_irq_disable() is not triggered.
> Interrupts that remain enabled can cause issues on subsequent boots.
>
> For example, the mlxbf-gige driver contains PHY logic to bring up the link.
> If the gpio-mlxbf3 driver loads first, the mlxbf-gige driver
> will use a GPIO interrupt to bring up the link.
> Otherwise, it will use polling.
> The next time Linux boots and loads the drivers in this order, we encounter the issue:
> - mlxbf-gige loads first and uses polling while the GPIO10
>   interrupt is still enabled from the previous boot. So if
>   the interrupt triggers, there is nothing to clear it.
> - gpio-mlxbf3 loads.
> - i2c-mlxbf loads. The interrupt doesn't trigger for I2C
>   because it is shared with the GPIO interrupt line which
>   was not cleared.
>
> The solution is to add a shutdown function to the GPIO driver to clear and disable
> all interrupts. Also clear the interrupt after disabling it in mlxbf3_gpio_irq_disable().
>
> Fixes: 38a700efc510 ("gpio: mlxbf3: Add gpio driver support")
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> Reviewed-by: David Thompson <davthompson@nvidia.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Asmaa Mnebhi Aug. 9, 2024, 8:24 p.m. UTC | #2
Thank you Andy and Linus for approving my patch! 

Hi Bart, could you please apply this patch?

Thank you,
Asmaa

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Monday, June 17, 2024 5:05 AM
> To: Asmaa Mnebhi <asmaa@nvidia.com>
> Cc: andy.shevchenko@gmail.com; bgolaszewski@baylibre.com; linux-
> gpio@vger.kernel.org; David Thompson <davthompson@nvidia.com>
> Subject: Re: [PATCH v2 1/1] gpio: mlxbf3: Support shutdown() function
> Importance: High
> 
> On Tue, Jun 11, 2024 at 7:15 PM Asmaa Mnebhi <asmaa@nvidia.com>
> wrote:
> 
> > During Linux graceful reboot, the GPIO interrupts are not disabled.
> > Since the drivers are not removed during graceful reboot, the logic to
> > call mlxbf3_gpio_irq_disable() is not triggered.
> > Interrupts that remain enabled can cause issues on subsequent boots.
> >
> > For example, the mlxbf-gige driver contains PHY logic to bring up the link.
> > If the gpio-mlxbf3 driver loads first, the mlxbf-gige driver will use
> > a GPIO interrupt to bring up the link.
> > Otherwise, it will use polling.
> > The next time Linux boots and loads the drivers in this order, we
> encounter the issue:
> > - mlxbf-gige loads first and uses polling while the GPIO10
> >   interrupt is still enabled from the previous boot. So if
> >   the interrupt triggers, there is nothing to clear it.
> > - gpio-mlxbf3 loads.
> > - i2c-mlxbf loads. The interrupt doesn't trigger for I2C
> >   because it is shared with the GPIO interrupt line which
> >   was not cleared.
> >
> > The solution is to add a shutdown function to the GPIO driver to clear
> > and disable all interrupts. Also clear the interrupt after disabling it in
> mlxbf3_gpio_irq_disable().
> >
> > Fixes: 38a700efc510 ("gpio: mlxbf3: Add gpio driver support")
> > Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> > Reviewed-by: David Thompson <davthompson@nvidia.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Yours,
> Linus Walleij
Bartosz Golaszewski Aug. 10, 2024, 7:35 p.m. UTC | #3
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Tue, 11 Jun 2024 13:15:09 -0400, Asmaa Mnebhi wrote:
> During Linux graceful reboot, the GPIO interrupts are not disabled.
> Since the drivers are not removed during graceful reboot,
> the logic to call mlxbf3_gpio_irq_disable() is not triggered.
> Interrupts that remain enabled can cause issues on subsequent boots.
> 
> For example, the mlxbf-gige driver contains PHY logic to bring up the link.
> If the gpio-mlxbf3 driver loads first, the mlxbf-gige driver
> will use a GPIO interrupt to bring up the link.
> Otherwise, it will use polling.
> The next time Linux boots and loads the drivers in this order, we encounter the issue:
> - mlxbf-gige loads first and uses polling while the GPIO10
>   interrupt is still enabled from the previous boot. So if
>   the interrupt triggers, there is nothing to clear it.
> - gpio-mlxbf3 loads.
> - i2c-mlxbf loads. The interrupt doesn't trigger for I2C
>   because it is shared with the GPIO interrupt line which
>   was not cleared.
> 
> [...]

Applied, thanks!

[1/1] gpio: mlxbf3: Support shutdown() function
      commit: aad41832326723627ad8ac9ee8a543b6dca4454d

Best regards,
Bartosz Golaszewski Aug. 10, 2024, 7:36 p.m. UTC | #4
On Fri, Aug 9, 2024 at 10:25 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> Thank you Andy and Linus for approving my patch!
>
> Hi Bart, could you please apply this patch?
>
> Thank you,
> Asmaa
>

Sorry, it got lost in my inbox.

Bart
Asmaa Mnebhi Aug. 11, 2024, 10:03 p.m. UTC | #5
Thank  you very much!

> -----Original Message-----
> From: Bartosz Golaszewski <brgl@bgdev.pl>
> Sent: Saturday, August 10, 2024 3:36 PM
> To: andy.shevchenko@gmail.com; linus.walleij@linaro.org; linux-
> gpio@vger.kernel.org; Bartosz Golaszewski <brgl@bgdev.pl>; Asmaa Mnebhi
> <asmaa@nvidia.com>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; David Thompson
> <davthompson@nvidia.com>
> Subject: Re: [PATCH v2 1/1] gpio: mlxbf3: Support shutdown() function
> Importance: High
> 
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 
> On Tue, 11 Jun 2024 13:15:09 -0400, Asmaa Mnebhi wrote:
> > During Linux graceful reboot, the GPIO interrupts are not disabled.
> > Since the drivers are not removed during graceful reboot, the logic to
> > call mlxbf3_gpio_irq_disable() is not triggered.
> > Interrupts that remain enabled can cause issues on subsequent boots.
> >
> > For example, the mlxbf-gige driver contains PHY logic to bring up the link.
> > If the gpio-mlxbf3 driver loads first, the mlxbf-gige driver will use
> > a GPIO interrupt to bring up the link.
> > Otherwise, it will use polling.
> > The next time Linux boots and loads the drivers in this order, we
> encounter the issue:
> > - mlxbf-gige loads first and uses polling while the GPIO10
> >   interrupt is still enabled from the previous boot. So if
> >   the interrupt triggers, there is nothing to clear it.
> > - gpio-mlxbf3 loads.
> > - i2c-mlxbf loads. The interrupt doesn't trigger for I2C
> >   because it is shared with the GPIO interrupt line which
> >   was not cleared.
> >
> > [...]
> 
> Applied, thanks!
> 
> [1/1] gpio: mlxbf3: Support shutdown() function
>       commit: aad41832326723627ad8ac9ee8a543b6dca4454d
> 
> Best regards,
> --
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-mlxbf3.c b/drivers/gpio/gpio-mlxbf3.c
index d5906d419b0a..10ea71273c89 100644
--- a/drivers/gpio/gpio-mlxbf3.c
+++ b/drivers/gpio/gpio-mlxbf3.c
@@ -39,6 +39,8 @@ 
 #define MLXBF_GPIO_CAUSE_OR_EVTEN0        0x14
 #define MLXBF_GPIO_CAUSE_OR_CLRCAUSE      0x18
 
+#define MLXBF_GPIO_CLR_ALL_INTS           GENMASK(31, 0)
+
 struct mlxbf3_gpio_context {
 	struct gpio_chip gc;
 
@@ -82,6 +84,8 @@  static void mlxbf3_gpio_irq_disable(struct irq_data *irqd)
 	val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
 	val &= ~BIT(offset);
 	writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+
+	writel(BIT(offset), gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
 	raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
 
 	gpiochip_disable_irq(gc, offset);
@@ -253,6 +257,15 @@  static int mlxbf3_gpio_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static void mlxbf3_gpio_shutdown(struct platform_device *pdev)
+{
+	struct mlxbf3_gpio_context *gs = platform_get_drvdata(pdev);
+
+	/* Disable and clear all interrupts */
+	writel(0, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+	writel(MLXBF_GPIO_CLR_ALL_INTS, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
+}
+
 static const struct acpi_device_id mlxbf3_gpio_acpi_match[] = {
 	{ "MLNXBF33", 0 },
 	{}
@@ -265,6 +278,7 @@  static struct platform_driver mlxbf3_gpio_driver = {
 		.acpi_match_table = mlxbf3_gpio_acpi_match,
 	},
 	.probe    = mlxbf3_gpio_probe,
+	.shutdown = mlxbf3_gpio_shutdown,
 };
 module_platform_driver(mlxbf3_gpio_driver);