linux-kernel - Re: [PATCH] mmc: move regulator handling to core

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:	Sat, 28 Aug 2010 16:48:58 +0200
From:	Linus Walleij <linus.ml.walleij@...il.com>
To:	Chris Ball <cjb@...top.org>
Cc:	Daniel Mack <daniel@...aq.de>, linux-kernel@...r.kernel.org,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Liam Girdwood <lrg@...mlogic.co.uk>,
	Pierre Ossman <pierre@...man.eu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Matt Fleming <matt@...sole-pimps.org>,
	Adrian Hunter <adrian.hunter@...ia.com>,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Eric Miao <eric.y.miao@...il.com>,
	Robert Jarzmik <robert.jarzmik@...e.fr>,
	Cliff Brake <cbrake@...-systems.com>,
	Jarkko Lavinen <jarkko.lavinen@...ia.com>,
	Madhusudhan <madhu.cr@...com>, linux-mmc@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] mmc: move regulator handling to core

2010/8/27 Chris Ball <cjb@...top.org>:

> Looks like this patch got dropped because of the missing modifications
> to arch/arm/mach-omap2/mmc-twl4030.c.  Are we still interested in the
> patch otherwise, and can anyone help with that?

Actually just before the summer I submitted something not quite similar:
I moved all regulator handling *out* of the MMC core because I didn't
trust the way reference counting was being handled.

There is something very disturbing about this code from
regulator/core/core.c mmc_regulator_set_ocr():

enabled = regulator_is_enabled(supply);
if (enabled < 0)
	return enabled;

if (...) {
	(...)
	voltage = regulator_get_voltage(supply);
	if (voltage < 0)
		result = voltage;
	else if (voltage < min_uV || voltage > max_uV)
		result = regulator_set_voltage(supply, min_uV, max_uV);
	else
		result = 0;

	if (result == 0 && !enabled)
		result = regulator_enable(supply);
} else if (enabled) {
	result = regulator_disable(supply);
}

I didn't realize until today where the problem is: if you have
two hosts on the same regulator this does not handle
concurrency correctly. There is no lock taken between reading
the enabled status and acting on it later in the code.

So it looks to me like it is possible for one slot to enable the
regulator and race with another slot which disables it at the
same time and end up with the regulator disabled :-(

My solution would still be to move the enable/disable out
of the core, so I need to follow up on that.

This old patch however, goes in the opposite direction,
moving it all into the core. If you do this, please fix the
concurrency issue also...

Yours,
Linus Walleij
--
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/