linux-kernel - Re: [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins

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

Message-ID: <20141119175510.GB37989@dtor-ws>
Date:	Wed, 19 Nov 2014 09:55:10 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Doug Anderson <dianders@...omium.org>
Cc:	Heiko Stuebner <heiko@...ech.de>,
	Linus Walleij <linus.walleij@...aro.org>,
	Sonny Rao <sonnyrao@...omium.org>,
	Chris Zhong <zyw@...k-chips.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org, linux-gpio@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins

On Tue, Nov 18, 2014 at 03:49:55PM -0800, Doug Anderson wrote:
> The rockchip pinctrl driver was using irq_gc_set_wake() as its
> implementation of irq_set_wake() but was totally ignoring everything
> that irq_gc_set_wake() did (which is to upkeep gc->wake_active).
> 
> Let's fix that by setting gc->wake_active as GPIO_INTEN at suspend
> time and restoring GPIO_INTEN at resume time.
> 
> NOTE a few quirks when thinking about this patch:
> - Rockchip pinctrl hardware supports both "disable/enable" and
>   "mask/unmask".  Right now we only use "disable/enable" and present
>   those to Linux as "mask/unmask".  This should be OK because
>   enable/disable is optional and Linux will implement it in terms of
>   mask/unmask.  At the moment we always tell hardware all interrupts
>   are unmasked (the boot default).
> - At suspend time Linux tries to call "disable" on all interrupts and
>   also enables wakeup on all wakeup interrupts.  One would think that
>   since "disable" is implemented as "mask" when "disable" isn't
>   provided and that since we were ignoring gc->wake_active that
>   nothing would have woken us up.  That's not the case since Linux
>   "optimizes" things and just leaves interrutps unmasked, assuming it
>   could mask them later when they go off.  That meant that at suspend
>   time all interrupts were actually being left enabled.
> 
> With this patch random non-wakeup interrupts no longer wake the system
> up.  Wakeup interrupts still wake the system up.
> 
> Signed-off-by: Doug Anderson <dianders@...omium.org>

Seems reasonable to me.

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@...il.com>

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index ba74f0a..e91e845 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -89,6 +89,7 @@ struct rockchip_iomux {
>   * @reg_pull: optional separate register for additional pull settings
>   * @clk: clock of the gpio bank
>   * @irq: interrupt of the gpio bank
> + * @saved_enables: Saved content of GPIO_INTEN at suspend time.
>   * @pin_base: first pin number
>   * @nr_pins: number of pins in this bank
>   * @name: name of the bank
> @@ -107,6 +108,7 @@ struct rockchip_pin_bank {
>  	struct regmap			*regmap_pull;
>  	struct clk			*clk;
>  	int				irq;
> +	u32				saved_enables;
>  	u32				pin_base;
>  	u8				nr_pins;
>  	char				*name;
> @@ -1543,6 +1545,23 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
>  	return 0;
>  }
>  
> +static void rockchip_irq_suspend(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct rockchip_pin_bank *bank = gc->private;
> +
> +	bank->saved_enables = irq_reg_readl(gc, GPIO_INTEN);
> +	irq_reg_writel(gc, gc->wake_active, GPIO_INTEN);
> +}
> +
> +static void rockchip_irq_resume(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct rockchip_pin_bank *bank = gc->private;
> +
> +	irq_reg_writel(gc, bank->saved_enables, GPIO_INTEN);
> +}
> +
>  static int rockchip_interrupts_register(struct platform_device *pdev,
>  						struct rockchip_pinctrl *info)
>  {
> @@ -1587,6 +1606,8 @@ static int rockchip_interrupts_register(struct platform_device *pdev,
>  		gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
>  		gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
>  		gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake;
> +		gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend;
> +		gc->chip_types[0].chip.irq_resume = rockchip_irq_resume;
>  		gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type;
>  		gc->wake_enabled = IRQ_MSK(bank->nr_pins);
>  
> -- 
> 2.1.0.rc2.206.gedb03e5
> 

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/