[PATCH] mmc: core: Check regulator pointer

[PATCH] mmc: core: Check regulator pointer

[Maxime Ripard]

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);
+
 	if (vdd_bit) {
 		mmc_ocrbitnum_to_vdd(vdd_bit, &min_uV, &max_uV);
 
-- 
2.9.3

Re: [PATCH] mmc: core: Check regulator pointer

[Baolin Wang]

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



-- 
Baolin.wang
Best Regards

Re: [PATCH] mmc: core: Check regulator pointer

[Ulf Hansson]

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

Re: [PATCH] mmc: core: Check regulator pointer

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] mmc: core: Check regulator pointer

[Ulf Hansson]

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

Re: [PATCH] mmc: core: Check regulator pointer

[Shawn Lin]

在 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
>


-- 
Best Regards
Shawn Lin