linux-kernel - Re: [PATCH v2] mmc: sunxi-mmc: check ocr

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] [day] [month] [year] [list]

Date:   Wed, 12 Jan 2022 11:37:44 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Maxime Ripard <maxime@...no.tech>,
        Michael Wu <michael@...winnertech.com>
Cc:     wens@...e.org, samuel@...lland.org, andre.przywara@....com,
        jernej.skrabec@...il.com, linux-mmc@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mmc: sunxi-mmc: check ocr_avail on resource request

On Wed, 12 Jan 2022 at 10:34, Maxime Ripard <maxime@...no.tech> wrote:
>
> Hi,
>
> On Tue, Jan 04, 2022 at 02:03:25PM +0800, Michael Wu wrote:
> > Some platforms have no regulator, discrete power devices are used instead.
>
> Is it really the case? vmmc at least should be mandatory so a platform
> not having a regulator would violate the binding itself.
>
> > However, sunxi_mmc_probe does not catch this exception when regulator is
> > absent in DTS. This leads to sd or eMMC init failure.
>
> This will still happen with your patch though?
>
> > To solve this, a fixed vmmc regulator must be hooked up in DTS, like this:
> > reg_dummy_vmmc: dummy_vmmc {
> >       compatible = "regulator-fixed";
> >       regulator-name = "dummy-vmmc";
> >       regulator-min-microvolt = <3300000>;
> >       regulator-max-microvolt = <3300000>;
> > };
> >
> > mmc0:mmc@...0000 {
> >       compatible = "allwinner,sun50i-a100-emmc";
> >       device_type = "mmc0";
> >       vmmc-supply = <&reg_dummy_vmmc>;
> > }
> >
> > In this patch, we print an error message and abort the probe process if
> > the regulator is not specified in DTS.
>
> I'm fine with the patch itself, but it's really not clear to me what
> situation is being fixed or improved here.
>
> You're first mentioning that this is fixing the driver probing even if a
> regulator is absent, but then states (rightfully) that in such a case we
> should use a fixed regulator. So we should always have a regulator then?
>
> I assume that you want the driver to properly error out instead of going
> on if either a regulator is missing or if its voltages are out of range?
>
> If the former, then we should probably check if host->mmc->supply.vmmc
> returned an error. If the latter, then yes, checking ocr_avail is
> probably fine but we should make it clearer in the error message that
> it's what it's about.

Just wanted to mention that I just had a very similar discussion
around another patch [1]. Please have a look at that discussion to get
my opinion on this.

Kind regards
Uffe

[1]
https://patchwork.kernel.org/project/linux-mmc/patch/20211215130711.111186-4-gsomlo@gmail.com/