| 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
| ||
|
Message-ID: <20171019090359.GB31645@kessel>
Date: Thu, 19 Oct 2017 02:03:59 -0700
From: Gregory Fong <gregory.0xf0@...il.com>
To: Doug Berger <opendmb@...il.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Brian Norris <computersforpeace@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
bcm-kernel-feedback-list@...adcom.com, linux-gpio@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 7/7] gpio: brcmstb: implement suspend/resume/shutdown
Hi Doug,
Nice description of the problem with dealing with a pending disabled
wake interrupt and the solution. A few remarks:
On Fri, Sep 29, 2017 at 08:40:57PM -0700, Doug Berger wrote:
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index 752a46ce3589..c964ed71a68d 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
> @@ -19,17 +19,29 @@
> #include <linux/irqdomain.h>
> #include <linux/irqchip/chained_irq.h>
> #include <linux/interrupt.h>
> -#include <linux/reboot.h>
> -
> -#define GIO_BANK_SIZE 0x20
> -#define GIO_ODEN(bank) (((bank) * GIO_BANK_SIZE) + 0x00)
> -#define GIO_DATA(bank) (((bank) * GIO_BANK_SIZE) + 0x04)
> -#define GIO_IODIR(bank) (((bank) * GIO_BANK_SIZE) + 0x08)
> -#define GIO_EC(bank) (((bank) * GIO_BANK_SIZE) + 0x0c)
> -#define GIO_EI(bank) (((bank) * GIO_BANK_SIZE) + 0x10)
> -#define GIO_MASK(bank) (((bank) * GIO_BANK_SIZE) + 0x14)
> -#define GIO_LEVEL(bank) (((bank) * GIO_BANK_SIZE) + 0x18)
> -#define GIO_STAT(bank) (((bank) * GIO_BANK_SIZE) + 0x1c)
> +
> +enum gio_reg_index {
> + GIO_REG_ODEN = 0,
> + GIO_REG_DATA,
> + GIO_REG_IODIR,
> + GIO_REG_EC,
> + GIO_REG_EI,
> + GIO_REG_MASK,
> + GIO_REG_LEVEL,
> + GIO_REG_STAT,
> + GIO_REG_COUNT
Please change the name or add a comment to make it clear that this is
not for an actual register.
> +};
> +
> [snip]
> @@ -37,6 +49,8 @@ struct brcmstb_gpio_bank {
> struct gpio_chip gc;
> struct brcmstb_gpio_priv *parent_priv;
> u32 width;
> + u32 wake_active;
> + u32 regs[GIO_REG_STAT]; /* Don't save and restore GIO_REG_STAT */
Consider naming to make it clearer that this is for save/restore, not
some kind of shadow reg implementation or anything like that.
> };
>
> struct brcmstb_gpio_priv {
> @@ -49,7 +63,6 @@ struct brcmstb_gpio_priv {
> int gpio_base;
> int num_gpios;
> int parent_wake_irq;
> - struct notifier_block reboot_notifier;
> };
>
> #define MAX_GPIO_PER_BANK 32
> @@ -65,15 +78,22 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
> }
>
> static unsigned long
> -brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
> +__brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
> {
> void __iomem *reg_base = bank->parent_priv->reg_base;
> +
> + return bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
> + bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
> +}
> +
> +static unsigned long
> +brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
> +{
> unsigned long status;
> unsigned long flags;
>
> spin_lock_irqsave(&bank->gc.bgpio_lock, flags);
> - status = bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
> - bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
> + status = __brcmstb_gpio_get_active_irqs(bank);
> spin_unlock_irqrestore(&bank->gc.bgpio_lock, flags);
>
> return status;
> @@ -209,11 +229,6 @@ static int brcmstb_gpio_priv_set_wake(struct brcmstb_gpio_priv *priv,
> {
> int ret = 0;
>
> - /*
> - * Only enable wake IRQ once for however many hwirqs can wake
> - * since they all use the same wake IRQ. Mask will be set
> - * up appropriately thanks to IRQCHIP_MASK_ON_SUSPEND flag.
> - */
> if (enable)
> ret = enable_irq_wake(priv->parent_wake_irq);
> else
> @@ -227,7 +242,17 @@ static int brcmstb_gpio_priv_set_wake(struct brcmstb_gpio_priv *priv,
> static int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> - struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
> + struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
> + struct brcmstb_gpio_priv *priv = bank->parent_priv;
> + u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
> +
> + /* Do not do anything specific for now, suspend/resume callbacks will
> + * configure the interrupt mask appropriately
> + */
Please fix comment format (this is the network subsystem style).
>
> [snip]
> @@ -508,6 +511,99 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
> [...]
> +static void brcmstb_gpio_shutdown(struct platform_device *pdev)
> +{
> + /* Enable GPIO for S5 cold boot */
> + brcmstb_gpio_quiesce(&pdev->dev, 0);
nit: use false instead of 0 (it's a bool).
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> [snip]
> +static int brcmstb_gpio_resume(struct device *dev)
> +{
> + struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
> + struct brcmstb_gpio_bank *bank;
> + u32 wake_mask = 0;
This isn't really being used as a mask, contrary to appearances. It's
just tracking whether any active IRQs were seen. Please change to use a
bool instead and adjust the name accordingly.
> +
> + list_for_each_entry(bank, &priv->bank_list, node) {
> + wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
> + brcmstb_gpio_bank_restore(priv, bank);
> + }
> +
> + if (priv->parent_wake_irq && wake_mask)
> + pm_wakeup_event(dev, 0);
> +
> + /* enable non-wake interrupt */
> + if (priv->parent_irq >= 0)
> + enable_irq(priv->parent_irq);
> +
> + return 0;
> +}
> +
> +#else
> +#define brcmstb_gpio_suspend NULL
> +#define brcmstb_gpio_resume NULL
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
> + .suspend_noirq = brcmstb_gpio_suspend,
> + .resume_noirq = brcmstb_gpio_resume,
> +};
> +
> static int brcmstb_gpio_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -517,7 +613,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
> struct resource *res;
> struct property *prop;
> const __be32 *p;
> - u32 bank_width;
> + u32 bank_width, wake_mask = 0;
Same comment on wake_mask as for brcmstb_gpio_resume() above.
> int num_banks = 0;
> int err;
> static int gpio_base;
> @@ -617,6 +713,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
> * Mask all interrupts by default, since wakeup interrupts may
> * be retained from S5 cold boot
> */
> + wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
> gc->write_reg(reg_base + GIO_MASK(bank->id), 0);
>
> err = gpiochip_add_data(gc, bank);
> @@ -646,6 +743,9 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
> dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
> num_banks, priv->gpio_base, gpio_base - 1);
>
> + if (priv->parent_wake_irq && wake_mask)
> + pm_wakeup_event(dev, 0);
Why is this called in probe?
Thanks,
Gregory
Powered by blists - more mailing lists