linux-kernel - Re: [PATCH V1 1/2] mfd: tps65910: use regmap for device register access.

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: <20120208114120.GF3120@opensource.wolfsonmicro.com>
Date:	Wed, 8 Feb 2012 11:41:20 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	sameo@...ux.intel.com, lrg@...com, jedu@...mlogic.co.uk,
	gg@...mlogic.co.uk, linux-kernel@...r.kernel.org,
	linux-tegra@...r.kernel.org
Subject: Re: [PATCH V1 1/2] mfd: tps65910: use regmap for device register
 access.

On Wed, Feb 08, 2012 at 04:16:24PM +0530, Laxman Dewangan wrote:

> +static bool is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	struct tps65910 *tps65910 = dev_get_drvdata(dev);
> +	return test_bit(reg, tps65910->cache_reg) ? false : true;
> +}

This is *really* odd.  Why is this not static data (or mostly static
data), why does it vary at runtime?

> +static bool regmap_volatile_range(struct tps65910 *tps65910,
> +			unsigned int reg, unsigned int bytes)
> +{
> +	unsigned int i;
> +	for (i = 0; i < bytes; i++)
> +		if (!is_volatile_reg(tps65910->dev, reg + i))
> +			return false;
> +	return true;
> +}

I don't think this should be here (the naming is a bit of a clue - it's
named like a core function), see below where you're using it...

> +	unsigned char *wbuf = src;
> +	unsigned int ival;
> +
> +	if (regmap_volatile_range(tps65910, reg, bytes))
> +		return regmap_raw_write(tps65910->regmap, reg, src, bytes);
> +
> +	/* If any of register is non-volatile then use byte-wise transfer */
> +	for (i = 0; i < bytes; ++i) {
> +		ival = (unsigned int) (*wbuf++);
> +		ret = regmap_write(tps65910->regmap, reg, ival);
> +		if (ret < 0)
> +			return ret;
> +	}

There's nothing specific to the driver about this, if this is a good
idea add support for it to the core.

> +void tps65910_enable_reg_cache(struct tps65910 *tps65910, int reg)
> +{
> +	set_bit(reg, tps65910->cache_reg);
> +}
> +EXPORT_SYMBOL_GPL(tps65910_enable_reg_cache);

Why are you doing this?  This looks very icky, and if it is needed there
needs to be more code here to make sure the register cache and device
are in sync.

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