linux-kernel - Re: [PATCH v4 1/3] mfd: tps6586x: add version detection

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: <52A0B2AF.60803@wwwdotorg.org>
Date:	Thu, 05 Dec 2013 10:06:55 -0700
From:	Stephen Warren <swarren@...dotorg.org>
To:	Stefan Agner <stefan@...er.ch>, thierry.reding@...il.com,
	dev@...xeye.de, lee.jones@...aro.org, lgirdwood@...il.com,
	broonie@...nel.org
CC:	kai.poggensee@...onic-design.de, sameo@...ux.intel.com,
	linux-tegra@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] mfd: tps6586x: add version detection

On 12/03/2013 03:18 PM, Stefan Agner wrote:
> Use the VERSIONCRC to determine the exact device version. According to
> the datasheet this register can be used as device identifier. The
> identification is needed since some tps6586x regulators use a different
> voltage table.

> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c

> @@ -493,13 +527,12 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>  		return -EIO;
>  	}
>  
> -	dev_info(&client->dev, "VERSIONCRC is %02x\n", ret);
> -
>  	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
> -	if (tps6586x == NULL) {
> -		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
> +	if (!tps6586x)
>  		return -ENOMEM;
> -	}
> +
> +	tps6586x->version = ret;

I have to say, I dislike this version of the patch. Separating the
reading of the version register from the assignment to tps6586x->version
doesn't make any sense, especially given that the version value is
stored in a variable named "ret"; that name isn't remotely related to
what's stored there. What if someone comes along later and adds more
code that assigns to ret between where it's repurposed for the version
value and where it's assigned to tps6586x->version? It'd be extremely
difficult for a patch reviewer to spot that given the limited context in
a diff, and quite non-obvious to the person changing the code too..
--
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/