| 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
| ||
|
Message-ID: <4C284E5B.503@nokia.com>
Date: Mon, 28 Jun 2010 10:25:15 +0300
From: Adrian Hunter <adrian.hunter@...ia.com>
To: Linus Walleij <linus.walleij@...ricsson.com>
CC: "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
Liam Girdwood <lrg@...mlogic.co.uk>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
Tony Lindgren <tony@...mide.com>,
Robert Jarzmik <robert.jarzmik@...e.fr>,
Sundar Iyer <sundar.iyer@...ricsson.com>,
Bengt Jonsson <bengt.jonsson@...ricsson.com>
Subject: Re: [PATCH] MMC: remove regulator refcount fiddling in mmc core
Linus Walleij wrote:
> Currently the mmc_regulator_set_ocr() fiddles with the regulator
> refcount by selectively calling regulator_[enable|disable]
> depending on the state of the regulator. This will confuse the
> reference count if case the regulator is for example shared with
> other MMC slots or user for other stuff than the MMC card.
>
> Push regulator_[enable|disable] out into the MMC host drivers
> and remove this from the MMC core so the reference count can be
> trusted.
That is not a technical reason. It would be better to provide
more regulator support in core so there is less duplication in
the drivers.
At least it should be a separate patch.
>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Liam Girdwood <lrg@...mlogic.co.uk>
> Cc: Mark Brown <broonie@...nsource.wolfsonmicro.com>
> Cc: Tony Lindgren <tony@...mide.com>
> Cc: Adrian Hunter <adrian.hunter@...ia.com>
> Cc: Robert Jarzmik <robert.jarzmik@...e.fr>
> Cc: Sundar Iyer <sundar.iyer@...ricsson.com>
> Cc: Bengt Jonsson <bengt.jonsson@...ricsson.com>
> Signed-off-by: Linus Walleij <linus.walleij@...ricsson.com>
> ---
> This has been regression compiled for U300, OMAP3, PXA3XX
> defconfigs, tested on U300.
>
> We're facing problems with our regulator reference counters if
> this is not fixed.
> ---
> drivers/mmc/core/core.c | 66 ++++++++++++++++++-----------------------
> drivers/mmc/host/mmci.c | 10 ++++--
> drivers/mmc/host/omap_hsmmc.c | 40 ++++++++++++++++++------
> drivers/mmc/host/pxamci.c | 17 ++++++++--
> 4 files changed, 78 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 569e94d..904f245 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -784,47 +784,39 @@ int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
> {
> int result = 0;
> int min_uV, max_uV;
> - int enabled;
> + int tmp;
> + int voltage;
>
> - enabled = regulator_is_enabled(supply);
> - if (enabled < 0)
> - return enabled;
> -
> - if (vdd_bit) {
> - int tmp;
> - int voltage;
> -
> - /* REVISIT mmc_vddrange_to_ocrmask() may have set some
> - * bits this regulator doesn't quite support ... don't
> - * be too picky, most cards and regulators are OK with
> - * a 0.1V range goof (it's a small error percentage).
> - */
> - tmp = vdd_bit - ilog2(MMC_VDD_165_195);
> - if (tmp == 0) {
> - min_uV = 1650 * 1000;
> - max_uV = 1950 * 1000;
> - } else {
> - min_uV = 1900 * 1000 + tmp * 100 * 1000;
> - max_uV = min_uV + 100 * 1000;
> - }
> -
> - /* avoid needless changes to this voltage; the regulator
> - * might not allow this operation
> - */
> - 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 (!vdd_bit)
> + return 0;
>
> - if (result == 0 && !enabled)
> - result = regulator_enable(supply);
> - } else if (enabled) {
> - result = regulator_disable(supply);
> + /*
> + * REVISIT mmc_vddrange_to_ocrmask() may have set some
> + * bits this regulator doesn't quite support ... don't
> + * be too picky, most cards and regulators are OK with
> + * a 0.1V range goof (it's a small error percentage).
> + */
> + tmp = vdd_bit - ilog2(MMC_VDD_165_195);
> + if (tmp == 0) {
> + min_uV = 1650 * 1000;
> + max_uV = 1950 * 1000;
> + } else {
> + min_uV = 1900 * 1000 + tmp * 100 * 1000;
> + max_uV = min_uV + 100 * 1000;
> }
>
> + /*
> + * Avoid needless changes to this voltage; the regulator
> + * might not allow this operation
> + */
> + 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;
> +
> return result;
> }
> EXPORT_SYMBOL(mmc_regulator_set_ocr);
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 4917af9..5f530b1 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -467,15 +467,17 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
> switch (ios->power_mode) {
> case MMC_POWER_OFF:
> - if(host->vcc &&
> - regulator_is_enabled(host->vcc))
> + if (host->vcc)
> regulator_disable(host->vcc);
> break;
> case MMC_POWER_UP:
> #ifdef CONFIG_REGULATOR
> - if (host->vcc)
> - /* This implicitly enables the regulator */
> + if (host->vcc) {
> + if (regulator_enable(host->vcc))
> + dev_err(mmc_dev(mmc),
> + "could not enable regulator\n");
> mmc_regulator_set_ocr(host->vcc, ios->vdd);
> + }
> #endif
> /*
> * The translate_vdd function is not used if you have
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index b032828..d8d07e1 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -247,10 +247,17 @@ static int omap_hsmmc_1_set_power(struct device *dev, int slot, int power_on,
> if (mmc_slot(host).before_set_reg)
> mmc_slot(host).before_set_reg(dev, slot, power_on, vdd);
>
> - if (power_on)
> - ret = mmc_regulator_set_ocr(host->vcc, vdd);
> - else
> - ret = mmc_regulator_set_ocr(host->vcc, 0);
> + if (power_on) {
> + ret = regulator_enable(host->vcc);
> + if (ret)
> + dev_err(dev, "could not enable regulator\n");
> + else
> + ret = mmc_regulator_set_ocr(host->vcc, vdd);
> + } else {
> + ret = regulator_disable(host->vcc);
> + if (ret)
> + dev_err(dev, "could not disable regulator\n");
> + }
Enabling the regulator before setting the voltage seems like it could
create problems, for example, enabling at an invalid voltage before
ramping to the correct voltage. Also, while regulator_enable typically
waits for the regulator to stablize, regulator_set_voltage typically
does not.
The original code was the other way around.
>
> if (mmc_slot(host).after_set_reg)
> mmc_slot(host).after_set_reg(dev, slot, power_on, vdd);
> @@ -289,7 +296,11 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
> * chips/cards need an interface voltage rail too.
> */
> if (power_on) {
> - ret = mmc_regulator_set_ocr(host->vcc, vdd);
> + ret = regulator_enable(host->vcc);
> + if (ret < 0)
> + dev_err(dev, "could not enable regulator\n");
> + else
> + ret = mmc_regulator_set_ocr(host->vcc, vdd);
> /* Enable interface voltage rail, if needed */
> if (ret == 0 && host->vcc_aux) {
> ret = regulator_enable(host->vcc_aux);
> @@ -297,10 +308,15 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
> ret = mmc_regulator_set_ocr(host->vcc, 0);
> }
> } else {
> + /* Shut down the rail */
> if (host->vcc_aux)
> ret = regulator_disable(host->vcc_aux);
> - if (ret == 0)
> - ret = mmc_regulator_set_ocr(host->vcc, 0);
> + if (ret == 0) {
> + /* Then proeeed to shut down the local regulator */
> + ret = regulator_disable(host->vcc);
> + if (ret == 0)
> + ret = mmc_regulator_set_ocr(host->vcc, 0);
> + }
> }
>
> if (mmc_slot(host).after_set_reg)
> @@ -340,10 +356,14 @@ static int omap_hsmmc_23_set_sleep(struct device *dev, int slot, int sleep,
>
> if (cardsleep) {
> /* VCC can be turned off if card is asleep */
> - if (sleep)
> - err = mmc_regulator_set_ocr(host->vcc, 0);
> - else
> + if (sleep) {
> + err = regulator_disable(host->vcc);
> + } else {
> + err = regulator_enable(host->vcc);
> + if (err)
> + return err;
> err = mmc_regulator_set_ocr(host->vcc, vdd);
> + }
> } else
> err = regulator_set_mode(host->vcc, mode);
> if (err)
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 0a4e43f..b7e0216 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -99,13 +99,22 @@ static inline void pxamci_init_ocr(struct pxamci_host *host)
> }
> }
>
> -static inline void pxamci_set_power(struct pxamci_host *host, unsigned int vdd)
> +static inline void pxamci_set_power(struct pxamci_host *host,
> + unsigned char power_mode,
> + unsigned int vdd)
> {
> int on;
>
> #ifdef CONFIG_REGULATOR
> - if (host->vcc)
> - mmc_regulator_set_ocr(host->vcc, vdd);
> + if (host->vcc) {
> + if (power_mode == MMC_POWER_UP) {
> + if (regulator_enable(host->vcc))
> + dev_err(mmc_dev(host->mmc),
> + "could not enable regulator\n");
> + mmc_regulator_set_ocr(host->vcc, vdd);
> + } else if (power_mode == MMC_POWER_OFF)
> + regulator_disable(host->vcc);
> + }
> #endif
> if (!host->vcc && host->pdata &&
> gpio_is_valid(host->pdata->gpio_power)) {
> @@ -492,7 +501,7 @@ static void pxamci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> if (host->power_mode != ios->power_mode) {
> host->power_mode = ios->power_mode;
>
> - pxamci_set_power(host, ios->vdd);
> + pxamci_set_power(host, ios->power_mode, ios->vdd);
>
> if (ios->power_mode == MMC_POWER_ON)
> host->cmdat |= CMDAT_INIT;
--
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/
Powered by blists - more mailing lists