linux-kernel - Re: [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support

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: <20160924182249.3wr66ytcxvz7tkgv@sirena.org.uk>
Date:   Sat, 24 Sep 2016 19:22:49 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Eric Jeong <eric.jeong.opensource@...semi.com>
Cc:     LINUXKERNEL <linux-kernel@...r.kernel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Support Opensource <support.opensource@...semi.com>
Subject: Re: [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB
 silicon support

On Wed, Sep 21, 2016 at 01:44:39PM +0900, Eric Jeong wrote:

> +#ifdef CONFIG_OF
> +static const struct of_device_id pv88080_dt_ids[] = {
> +	{ .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> +	{ .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pv88080_dt_ids);
> +#endif

This doesn't list the old compatible string and therefore ends up
breaking since the code defaults to assuming something it hasn't heard
of is BB when it seems more likely that old DTs using the old compatible
string will be for boards that also use the old silicon.  It would be
much better practice to explicitly list the old compatible string and
how we handle it, that won't break in future and avoids this kind of
error.

Is it really not possible to read the chip revision from the device
using ID registers?  That'd be even better.

> +	if (i2c->dev.of_node) {
> +		match = of_match_node(pv88080_dt_ids, i2c->dev.of_node);
> +		if (!match) {
> +			dev_err(chip->dev, "Failed to get of_match_node\n");
> +			return -EINVAL;
> +		}
> +		chip->type = (unsigned long)match->data;

This will generate the warning for all existing DTs as a result of the
above which isn't great.

> +	if (chip->type == TYPE_PV88080_AA)
> +		chip->regmap_config = &pv88080_aa_regs;
> +	else
> +		chip->regmap_config = &pv88080_ba_regs;

Use a switch statement here, it is more extensible if we get future chip
versions and will improve error handling since we won't just silently
treat unrecognized values as BB.

>  static const struct i2c_device_id pv88080_i2c_id[] = {
> -	{"pv88080", 0},
> +	{ "pv88080-aa", TYPE_PV88080_AA },
> +	{ "pv88080-ba", TYPE_PV88080_BA },

Same thing as with the compatible string here.

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)