linux-kernel - Re: [PATCH 1/2] mfd: dt: tps6586x: Add power off control

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: <20120817104150.GA27633@avionic-0098.mockup.avionic-design.de>
Date:	Fri, 17 Aug 2012 12:41:50 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Bill Huang <bilhuang@...dia.com>
Cc:	sameo@...ux.intel.com, grant.likely@...retlab.ca,
	rob.herring@...xeda.com, rob@...dley.net,
	broonie@...nsource.wolfsonmicro.com, ldewangan@...dia.com,
	swarren@...dotorg.org, xxie@...dia.com, lrg@...mlogic.co.uk,
	jhovold@...il.com, kyle.manna@...l7.com, rklein@...dia.com,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mfd: dt: tps6586x: Add power off control

On Fri, Aug 17, 2012 at 02:16:28AM -0700, Bill Huang wrote:
[...]
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
[...]
> @@ -505,6 +519,11 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
>  		goto err_add_devs;
>  	}
>  
> +	tps6586x_dev = &client->dev;
> +
> +	if (pdata->pm_off && !pm_power_off)
> +		pm_power_off = tps6586x_power_off;
> +

I think the assignment of tps6586x_dev needs to go inside the if block
as well. Otherwise it might be overwritten by another instance for
systems that actually have more than one tps6586x. Since currently the
driver can't be built as a module it probably makes little sense to
clean this up in .remove(), but it might still be worth adding so it
isn't forgotten if and when somebody tries to convert the driver to a
module.

I should note that I don't like very much how the pm_power_off works.
Maybe this should really be changed to allow passing a context for the
function to work from. Something like:

	pm_power_off = tps6586x_power_off;
	pm_power_off_data = &client->dev;

Where pm_power_off() would receive pm_power_off_data as an argument.

Even that's not very pretty. On the other hand this doesn't really buy
us much because only the storage location of the variable would change
and nothing else. But it would still make the association of the data
clearer.

Thierry

Content of type "application/pgp-signature" skipped