| Message ID | 20161018084343.680-1-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 18 October 2016 at 16:43, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > mmc_regulator_get_supply might silently fail if the regulators are not > found, which is the right thing to do since both these regulators are > optional. > > However, the drivers then have no way to know whether or not they should > proceed and call mmc_regulator_set_ocr. And since this function doesn't > check for the validity of the regulator pointer, it leads to a null pointer > dereference. Add such a check to make sure everything works fine. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/mmc/core/core.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 2553d903a82b..1d3ea5e1aa37 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1474,6 +1474,12 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc, > int result = 0; > int min_uV, max_uV; > > + if (!supply) > + return -EINVAL; > + > + if (IS_ERR(supply)) > + return PTR_ERR(supply); You can simply these checking with IS_ERR_OR_NULL(). > + > if (vdd_bit) { > mmc_ocrbitnum_to_vdd(vdd_bit, &min_uV, &max_uV); > > -- > 2.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18 October 2016 at 10:43, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > mmc_regulator_get_supply might silently fail if the regulators are not > found, which is the right thing to do since both these regulators are > optional. > > However, the drivers then have no way to know whether or not they should > proceed and call mmc_regulator_set_ocr. And since this function doesn't Host drivers should check "if (!IS_ERR(mmc->supply.vmmc))" before invoking mmc_regulator_set_ocr(). I wasn't aware that some didn't. My point is, that in some cases the regulator is optional, then a host driver need to take other actions to power on/off the card. So, I am wondering whether adding these checks in mmc_regulator_set_ocr() is a bit unnecessary, as the host drivers are already checking the IS_ERR(). > check for the validity of the regulator pointer, it leads to a null pointer > dereference. Add such a check to make sure everything works fine. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/mmc/core/core.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 2553d903a82b..1d3ea5e1aa37 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1474,6 +1474,12 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc, > int result = 0; > int min_uV, max_uV; > > + if (!supply) > + return -EINVAL; > + > + if (IS_ERR(supply)) > + return PTR_ERR(supply); > + > if (vdd_bit) { > mmc_ocrbitnum_to_vdd(vdd_bit, &min_uV, &max_uV); > > -- > 2.9.3 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Oct 18, 2016 at 11:03:02AM +0200, Ulf Hansson wrote: > On 18 October 2016 at 10:43, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > mmc_regulator_get_supply might silently fail if the regulators are not > > found, which is the right thing to do since both these regulators are > > optional. > > > > However, the drivers then have no way to know whether or not they should > > proceed and call mmc_regulator_set_ocr. And since this function doesn't > > Host drivers should check "if (!IS_ERR(mmc->supply.vmmc))" before > invoking mmc_regulator_set_ocr(). I wasn't aware that some didn't. Ok, so the sunxi one definitely doesn't: http://lxr.free-electrons.com/source/drivers/mmc/host/sunxi-mmc.c#L735 > My point is, that in some cases the regulator is optional, then a host > driver need to take other actions to power on/off the card. What are those actions? Just power up the card through some other mean, or is it more tied to the MMC protocol? Also, I'm wondering if VMMC is actually optional at all. In all cases I can think of, it would be represented as a regulator anyway. Thanks, Maxime
On 18 October 2016 at 11:47, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi, > > On Tue, Oct 18, 2016 at 11:03:02AM +0200, Ulf Hansson wrote: >> On 18 October 2016 at 10:43, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >> > mmc_regulator_get_supply might silently fail if the regulators are not >> > found, which is the right thing to do since both these regulators are >> > optional. >> > >> > However, the drivers then have no way to know whether or not they should >> > proceed and call mmc_regulator_set_ocr. And since this function doesn't >> >> Host drivers should check "if (!IS_ERR(mmc->supply.vmmc))" before >> invoking mmc_regulator_set_ocr(). I wasn't aware that some didn't. > > Ok, so the sunxi one definitely doesn't: > http://lxr.free-electrons.com/source/drivers/mmc/host/sunxi-mmc.c#L735 > >> My point is, that in some cases the regulator is optional, then a host >> driver need to take other actions to power on/off the card. > > What are those actions? Just power up the card through some other > mean, or is it more tied to the MMC protocol? Through some other way, which is managed by the internal logic of the mmc controller. > > Also, I'm wondering if VMMC is actually optional at all. In all cases > I can think of, it would be represented as a regulator anyway. Yes, you are right. For most cases, but not all. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 2016/10/18 17:47, Maxime Ripard 写道: > Hi, > > On Tue, Oct 18, 2016 at 11:03:02AM +0200, Ulf Hansson wrote: >> On 18 October 2016 at 10:43, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >>> mmc_regulator_get_supply might silently fail if the regulators are not >>> found, which is the right thing to do since both these regulators are >>> optional. >>> >>> However, the drivers then have no way to know whether or not they should >>> proceed and call mmc_regulator_set_ocr. And since this function doesn't >> >> Host drivers should check "if (!IS_ERR(mmc->supply.vmmc))" before >> invoking mmc_regulator_set_ocr(). I wasn't aware that some didn't. > > Ok, so the sunxi one definitely doesn't: > http://lxr.free-electrons.com/source/drivers/mmc/host/sunxi-mmc.c#L735 > >> My point is, that in some cases the regulator is optional, then a host >> driver need to take other actions to power on/off the card. > > What are those actions? Just power up the card through some other > mean, or is it more tied to the MMC protocol? > > Also, I'm wondering if VMMC is actually optional at all. In all cases > I can think of, it would be represented as a regulator anyway. If I understand your point correctly, for sunxi's mmc controller, the only way to power-on/off the vmmc is to call mmc_regulator_set_ocr? There are some cases for mmc-core/host to control the power: 1) assign a vmmc-supply for no matter of gpio-based regulator or PMIC 2) use mmc_pwrseq which is often used by sdio fucntion or emmc. 3) use a functional port to control it which is similar to gpio-based regulator but the host drivers could control this via internal register, for instance, dw_mmc.. 4) And I even see some boards use fixed power which means we cannot power off vmmc from the hardware part. And also, if I decide to add alwyas-on and boot-on for the vmmc regulator I was using, and also there is no really need for mmc-core/driver to know whether we have vmmc, right!? > > Thanks, > Maxime >
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 2553d903a82b..1d3ea5e1aa37 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1474,6 +1474,12 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc, int result = 0; int min_uV, max_uV; + if (!supply) + return -EINVAL; + + if (IS_ERR(supply)) + return PTR_ERR(supply); + if (vdd_bit) { mmc_ocrbitnum_to_vdd(vdd_bit, &min_uV, &max_uV);
mmc_regulator_get_supply might silently fail if the regulators are not found, which is the right thing to do since both these regulators are optional. However, the drivers then have no way to know whether or not they should proceed and call mmc_regulator_set_ocr. And since this function doesn't check for the validity of the regulator pointer, it leads to a null pointer dereference. Add such a check to make sure everything works fine. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- drivers/mmc/core/core.c | 6 ++++++ 1 file changed, 6 insertions(+)