linux-kernel - Re: [PATCH 1/7] regulator: act8865: Expose act8600 registers via debugfs

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, 29 Feb 2016 20:28:57 +0900
From:	Mark Brown <broonie@...nel.org>
To:	Maarten ter Huurne <maarten@...ewalker.org>
Cc:	Wenyou Yang <wenyou.yang@...el.com>,
	Zubair Lutfullah Kakakhel <Zubair.Kakakhel@...tec.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] regulator: act8865: Expose act8600 registers via
 debugfs

On Sun, Feb 28, 2016 at 04:53:23PM +0100, Maarten ter Huurne wrote:

> The read/write/volatile configuration is valid also when debugfs is
> not enabled, but it doesn't add any value then.

Please write changelogs that accurately describe what your changes do.
Any debugfs changes are at best a second order side effect of changes
here, what this appears to do is some combination of defining one or
more new registers (which don't seem to ever be referred to...) and
providing some access maps.

> +#ifdef CONFIG_DEBUG_FS
> +

No, this is broken.  The access information for a device is not affected
by debugfs and does have an effect on how we work with the device.
Having access maps that depend on random build settings like this is a
recipie for hard to diagnose bugs.

> +static const struct regmap_range act8600_reg_ranges[] = {
> +	{ 0x00, 0x01 },
> +	{ 0x10, 0x10 }, { 0x12, 0x12 },
> +	{ 0x20, 0x20 }, { 0x22, 0x22 },
> +	{ 0x30, 0x30 }, { 0x32, 0x32 },

Your formatting here is just weird and confusing, randomly grouping
things on lines with no obvious .

> +};
> +static const struct regmap_range act8600_reg_ro_ranges[] = {

Missing blank lines between variables.

> +static const struct regmap_range act8600_reg_volatile_ranges[] = {
> +	{ 0x00, 0x01 },

For ranges you should explicitly use .start and .end otherwise these
look like they're intended to be register default tables.

> @@ -421,6 +470,11 @@ static int act8865_pmic_probe(struct i2c_client *client,
>  	struct device_node **of_node;
>  	int i, ret, num_regulators;
>  	struct act8865 *act8865;
> +	struct regmap_config regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.max_register = 0xFF,
> +	};

Why have you moved this from a global static variable where it normally
is to a local variable?  This is unusual and confusing...

> +#ifdef CONFIG_DEBUG_FS
> +		regmap_config.wr_table = &act8600_write_ranges_table;
> +		regmap_config.rd_table = &act8600_read_ranges_table;
> +		regmap_config.volatile_table = &act8600_volatile_ranges_table;
> +#endif

...and the fact that you're doing things like this ought to be a warning
that there's a problem with the way you're handling the access maps.

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