linux-kernel - Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio

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: <87ty2bu91e.fsf@ti.com>
Date:	Mon, 27 Feb 2012 15:54:37 -0800
From:	Kevin Hilman <khilman@...com>
To:	Tarun Kanti DebBarma <tarun.kanti@...com>
Cc:	<linux-omap@...r.kernel.org>, <grant.likely@...retlab.ca>,
	<tony@...mide.com>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

Tarun Kanti DebBarma <tarun.kanti@...com> writes:

> Since we already have bank->context.wake_en to keep track
> of gpios which are wakeup enabled, there is no need to have
> this field any more.
>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@...com>

I'm not crazy about this change...

> ---
>  drivers/gpio/gpio-omap.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 64f15d5..b62e861 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -53,7 +53,6 @@ struct gpio_bank {
>  	void __iomem *base;
>  	u16 irq;
>  	u16 virtual_irq_start;
> -	u32 suspend_wakeup;
>  	u32 non_wakeup_gpios;
>  	u32 enabled_non_wakeup_gpios;
>  	struct gpio_regs context;
> @@ -497,9 +496,9 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	if (enable)
> -		bank->suspend_wakeup |= gpio_bit;
> +		bank->context.wake_en |= gpio_bit;
>  	else
> -		bank->suspend_wakeup &= ~gpio_bit;
> +		bank->context.wake_en &= ~gpio_bit;

The bank->context values are expected to be copies of the actual
register contents, and here that is clearly not the case.

With this change, you're using the context register to track changes
that you *might* eventually write to the register.

IMO, this is more confusing than having a separate field to track this.

Kevin

>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	bank->context.wake_en = __raw_readl(mask_reg);
> -	__raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
> +	__raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
>  	return 0;
> @@ -1137,12 +1136,12 @@ static int omap_gpio_suspend(struct device *dev)
>  	if (!bank->mod_usage || !bank->loses_context)
>  		return 0;
>  
> -	if (!bank->regs->wkup_en || !bank->suspend_wakeup)
> +	if (!bank->regs->wkup_en || !bank->context.wake_en)
>  		return 0;
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
> -	_gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
> +	_gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
>  	return 0;
--
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/