linux-kernel - Re: [PATCH] mmc: core: limit probe clock frequency to configured f

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]

Message-ID: <20200117140511.GC26135@qmqm.qmqm.pl>
Date:   Fri, 17 Jan 2020 15:05:11 +0100
From:   Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mmc: core: limit probe clock frequency to configured
 f_max

On Thu, Jan 16, 2020 at 03:07:22PM +0100, Ulf Hansson wrote:
> On Thu, 2 Jan 2020 at 11:54, Michał Mirosław <mirq-linux@...e.qmqm.pl> wrote:
> >
> > Currently MMC core disregards host->f_max during card initialization
> > phase. Obey upper boundary for the clock frequency and skip faster
> > speeds when they are above the limit.
> 
> Is this a hypothetical problem or a real problem?

This is a problem on noisy or broken boards or cards - so needed for
debugging such a combination. I wouldn't expect this is required for
normal devices.

> > Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> > ---
> >  drivers/mmc/core/core.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index abf8f5eb0a1c..aa54d359dab7 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2330,7 +2330,13 @@ void mmc_rescan(struct work_struct *work)
> >         }
> >
> >         for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> > -               if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> > +               unsigned int freq = freqs[i];
> > +               if (freq > host->f_max) {
> > +                       if (i + 1 < ARRAY_SIZE(freqs))
> > +                               continue;
> > +                       freq = host->f_max;
> 
> This looks wrong to me. For example, what if f_max = 250KHz and f_min = 50 KHz.
> 
> Then we should try with 250KHz, then 200KHz and then 100KHz. This
> isn't what the above code does, I think.
> 
> Instead it will try with 200KHz and then 100KHz, thus skip 250KHz.
> 
> Maybe we should figure out what index of freqs[] to start the loop for
> (before actually starting the loop), depending on the value of f_max -
> rather than always start at 0.

Yes, it will skip higher frequencies. I didn't view it a problem,
because the new code guarantees at least one frequency will be tried.
The eMMC standard specifies only max init frequency (400kHz), so all we
should try is something less whatever works.

SD spec specifies minimal frequency (100kHz), but I wouldn't expect
this to be enforced nor required to be anywhere.

Best Regards
Michał Mirosław