linux-kernel - Re: [PATCH 13/26] mfd: ab8500-debugfs: Formated access AB8500 registers from debugfs entry

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: <20130128084702.GE15873@gmail.com>
Date:	Mon, 28 Jan 2013 08:47:02 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Samuel Ortiz <sameo@...ux.intel.com>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	arnd@...db.de, linus.walleij@...ricsson.com,
	carriere etienne <etienne.carriere@...ricsson.com>
Subject: Re: [PATCH 13/26] mfd: ab8500-debugfs: Formated access AB8500
 registers from debugfs entry

On Mon, 28 Jan 2013, Samuel Ortiz wrote:

> Hi Lee,
> 
> On Tue, Jan 15, 2013 at 12:55:53PM +0000, Lee Jones wrote:
> > +static ssize_t hwreg_common_write(char *b, struct hwreg_cfg *cfg,
> > +		struct device *dev)
> > +{
> > +	uint write, val = 0;
> > +	struct hwreg_cfg loc = {
> > +		.bank = 0,          /* default: invalid phys addr */
> > +		.addr = 0,          /* default: invalid phys addr */
> > +		.fmt = 0,           /* default: 32bit access, hex output */
> > +		.mask = 0xFFFFFFFF, /* default: no mask */
> > +		.shift = 0,         /* default: no bit shift */
> > +	};
> > +
> > +	/* read or write ? */
> > +	if (!strncmp(b, "read ", 5)) {
> > +		write = 0;
> > +		b += 5;
> > +	} else if (!strncmp(b, "write ", 6)) {
> > +		write = 1;
> > +		b += 6;
> > +	} else
> > +		return -EINVAL;
> > +
> > +	/* OPTIONS -l|-w|-b -s -m -o */
> > +	while ((*b == ' ') || (*b == '-')) {
> > +		if (*(b-1) != ' ') {
> > +			b++;
> > +			continue;
> > +		}
> > +		if ((!strncmp(b, "-d ", 3)) ||
> > +				(!strncmp(b, "-dec ", 5))) {
> > +			b += (*(b+2) == ' ') ? 3 : 5;
> > +			loc.fmt |= (1<<0);
> > +		} else if ((!strncmp(b, "-h ", 3)) ||
> > +				(!strncmp(b, "-hex ", 5))) {
> > +			b += (*(b+2) == ' ') ? 3 : 5;
> > +			loc.fmt &= ~(1<<0);
> > +		} else if ((!strncmp(b, "-m ", 3)) ||
> > +				(!strncmp(b, "-mask ", 6))) {
> > +			b += (*(b+2) == ' ') ? 3 : 6;
> > +			if (strval_len(b) == 0)
> > +				return -EINVAL;
> > +			loc.mask = simple_strtoul(b, &b, 0);
> > +		} else if ((!strncmp(b, "-s ", 3)) ||
> > +				(!strncmp(b, "-shift ", 7))) {
> > +			b += (*(b+2) == ' ') ? 3 : 7;
> > +			if (strval_len(b) == 0)
> > +				return -EINVAL;
> > +			loc.shift = simple_strtol(b, &b, 0);
> > +		} else {
> > +			return -EINVAL;
> > +		}
> > +	}
> > +	/* get arg BANK and ADDRESS */
> > +	if (strval_len(b) == 0)
> > +		return -EINVAL;
> > +	loc.bank = simple_strtoul(b, &b, 0);
> > +	while (*b == ' ')
> > +		b++;
> > +	if (strval_len(b) == 0)
> > +		return -EINVAL;
> > +	loc.addr = simple_strtoul(b, &b, 0);
> > +
> > +	if (write) {
> > +		while (*b == ' ')
> > +			b++;
> > +		if (strval_len(b) == 0)
> > +			return -EINVAL;
> > +		val = simple_strtoul(b, &b, 0);
> > +	}
> > +
> > +	/* args are ok, update target cfg (mainly for read) */
> > +	*cfg = loc;
> > +
> > +#if ABB_HWREG_DEBUG
> > +	pr_warn("HWREG request: %s, %s, addr=0x%08X, mask=0x%X, shift=%d
> > +			value=0x%X\n", (write) ? "write" : "read",
> > +			REG_FMT_DEC(cfg) ? "decimal" : "hexa",
> > +			cfg->addr, cfg->mask, cfg->shift, val);
> > +#endif
> > +
> > +	if (write) {
> if (!write)
> 	return 0;
> 
> for a more readable code.

I'll fixup.

> > +		u8  regvalue;
> > +		int ret = abx500_get_register_interruptible(dev,
> > +			(u8)cfg->bank, (u8)cfg->addr, &regvalue);
> > +		if (ret < 0) {
> > +			dev_err(dev, "abx500_get_reg fail %d, %d\n",
> > +				ret, __LINE__);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (cfg->shift >= 0) {
> > +			regvalue &= ~(cfg->mask << (cfg->shift));
> > +			val = (val & cfg->mask) << (cfg->shift);
> > +		} else {
> > +			regvalue &= ~(cfg->mask >> (-cfg->shift));
> > +			val = (val & cfg->mask) >> (-cfg->shift);
> > +		}
> > +		val = val | regvalue;
> > +
> > +		ret = abx500_set_register_interruptible(dev,
> > +			(u8)cfg->bank, (u8)cfg->addr, (u8)val);
> > +		if (ret < 0) {
> > +			pr_err("abx500_set_reg failed %d, %d", ret, __LINE__);
> > +			return -EINVAL;
> > +		}
> > +
> > +	}
> > +	return 0;
> > +}
> I think this is a pretty big routine, that could be split into a command
> parsing part and the actual register write one.

I've made a note to address this as the _end_ of the big push. I'd
really like to avoid any unnecessary conflicts.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/