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]
Date:	Mon, 1 Apr 2013 10:04:37 +0530
From:	Bibek Basu <bbasu@...dia.com>
To:	Stephen Warren <swarren@...dotorg.org>
CC:	Linus Walleij <linus.walleij@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Pritesh Raithatha <praithatha@...dia.com>
Subject: RE: [PATCH] pinctrl: tegra: add suspend-resume support

Hi Stephen,

My response inline.

Regards
Bibek


-----Original Message-----
From: Stephen Warren [mailto:swarren@...dotorg.org] 
Sent: Thursday, March 28, 2013 11:19 PM
To: Bibek Basu
Cc: Linus Walleij; linux-kernel@...r.kernel.org; Pritesh Raithatha
Subject: Re: [PATCH] pinctrl: tegra: add suspend-resume support

On 03/28/2013 11:11 AM, Bibek Basu wrote:
> From: Pritesh Raithatha <praithatha@...dia.com>
> 
> This patch adds suspend and resume callbacks to the pinctrl-tegra driver.
> 
> Based on work by:
> Pritesh Raithatha <praithatha@...dia.com>

Those two lines are somewhat implied by the fact the commit's git author field is Pritesh, and his s-o-b line is included below.
[BB]: Will get rid of the above lines.

> Signed-off-by: Pritesh Raithatha <praithatha@...dia.com>
> Signed-off-by: Bibek Basu <bbasu@...dia.com>

> diff --git a/drivers/pinctrl/pinctrl-tegra.c 
> b/drivers/pinctrl/pinctrl-tegra.c

> +static struct tegra_pmx *pmx;

Isn't there any way to pass data into the suspend/resume functions so that this global isn't needed?

Why can't we just use the device suspend/resume functions rather than global (syscore) suspend/resume functions? Presumably this is to ensure that all other drivers suspend first, then the pinctrl driver does, and the reverse for resume. Can't we rely on deferred probe to ensure that instead?

To make that work, we might need every affected driver to define a dummy pinmux state in DT that references the pinctrl driver, to make sure they all get probed after the pinctrl driver.
[BB]: Before I add dummy pinmux state in DT of affected driver, I would like to know the following:
    1> The usage of syscore api needs  global data. So, are you suggesting that syscore APIs are not appropriate to be used or syscore implementation is not proper?
    2> Adding dummy DT states may give scope for BUGS. Reason being there must be someone checking that every time some new driver refrences pinmux driver, should put a dummy entry in device tree. Isnt it more time costing   then having "static struct tegra_pmx *pmx;"?

> +#ifdef CONFIG_PM_SLEEP

Perhaps we can remove the ifdefs, and always compile this code. The compiler should remove any unused functions from the final linked image.
[BB]: Will do

>  int tegra_pinctrl_probe(struct platform_device *pdev,
>  			const struct tegra_pinctrl_soc_data *soc_data) -{
> -	struct tegra_pmx *pmx;
> +	{

The indentation of the { is wrong there.
[BB]: Will fix

> +#ifdef CONFIG_PM_SLEEP

I think you can use if (IS_ENABLED(CONFIG_PM_SLEEP)) here instead of an ifdef. This will allow the code to be compiled in all cases (to allow better compile coverage), but it will be optimized out if CONFIG_PM_SLEEP isn't enabled. Can you please make that change, and verify that it works?
[BB]: Will do.

The same comment applies to the other ifdefs below.
--
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/

Powered by blists - more mailing lists