linux-kernel - Re: [PATCH] mmc: dw_mmc: Add a timeout for sending CMD11

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]

Message-ID: <CAD=FV=VcaW3eG49ASWxVV4YgUjiex77Z4ShJV0RJLYh2FEPMsw@mail.gmail.com>
Date:	Fri, 13 Mar 2015 13:38:17 -0700
From:	Doug Anderson <dianders@...omium.org>
To:	Jaehoon Chung <jh80.chung@...sung.com>
Cc:	Seungwon Jeon <tgih.jun@...sung.com>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Addy Ke <addy.ke@...k-chips.com>,
	Heiko Stuebner <heiko@...ech.de>,
	Andrew Bresticker <abrestic@...omium.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	Chris Ball <chris@...ntf.net>,
	"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
	Alim Akhtar <alim.akhtar@...sung.com>,
	Sonny Rao <sonnyrao@...omium.org>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Alexandru Stan <amstan@...omium.org>
Subject: Re: [PATCH] mmc: dw_mmc: Add a timeout for sending CMD11

Jaehoon,

On Fri, Mar 13, 2015 at 4:19 AM, Jaehoon Chung <jh80.chung@...sung.com> wrote:
> Hi Doug.
>
> This patch is a right process. Just i wonder something.
>
> On 03/10/2015 08:18 AM, Doug Anderson wrote:
>> In the Designware databook's description of the "Voltage Switch Normal
>> Scenario" it instructs us to set a timer and fail the voltage change
>> if we don't see the voltage change interrupt within 2ms.  Let's
>> implement that.  Without implementing this I have often been able to
>> reproduce a hang while trying to send CMD11 on an rk3288-based board
>> while constantly ejecting and inserting UHS cards.
>>
>> Signed-off-by: Doug Anderson <dianders@...omium.org>
>> ---
>>  drivers/mmc/host/dw_mmc.c  | 26 ++++++++++++++++++++++++++
>>  include/linux/mmc/dw_mmc.h |  2 ++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 47dfd0e..d259662 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1020,6 +1020,15 @@ static void __dw_mci_start_request(struct dw_mci *host,
>>
>>       dw_mci_start_command(host, cmd, cmdflags);
>>
>> +     if (cmd->opcode == SD_SWITCH_VOLTAGE) {
>> +             /*
>> +              * Databook says to fail after 2ms w/ no response; give an
>> +              * extra jiffy just in case we're about to roll over.
>> +              */
>> +             mod_timer(&host->cmd11_timer,
>> +                       jiffies + msecs_to_jiffies(2) + 1);
>
> What's "plus one"?

I tried to briefly describe it in the comment above with the "in case
we're about to roll over".  ...but more detail...

* I expect HZ to be something like 100.  ...so a jiffy will be 10ms.
2ms will be rounded up to 1 jiffy.

* It's entirely possible that we're about to roll over jiffies.  That
is, we might make the mod_timer call when we're 1 nanosecond away from
moving from 999 to 1000 jiffies.  We'll still read "jiffies" as 999
and add "msecs_to_jiffies(2)" to get 1000 jiffies.  ...but then it
will roll over and we'll make the call mod_timer(1000) when jiffies is
already 1000.  That means that we really got a 1ns delay--not so good.

...if we add the extra 1 jiffy then we'll probably really delay for
10-20ms, but that should be fine in this case.


If I misunderstood the above, please correct me.

>> +     }
>> +
>>       if (mrq->stop)
>>               host->stop_cmdr = dw_mci_prepare_command(slot->mmc, mrq->stop);
>>       else
>> @@ -2158,6 +2167,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>               /* Check volt switch first, since it can look like an error */
>>               if ((host->state == STATE_SENDING_CMD11) &&
>>                   (pending & SDMMC_INT_VOLT_SWITCH)) {
>> +                     del_timer(&host->cmd11_timer);
>> +
>>                       mci_writel(host, RINTSTS, SDMMC_INT_VOLT_SWITCH);
>>                       pending &= ~SDMMC_INT_VOLT_SWITCH;
>>                       dw_mci_cmd_interrupt(host, pending);
>> @@ -2571,6 +2582,18 @@ ciu_out:
>>       return ret;
>>  }
>>
>> +static void dw_mci_cmd11_timer(unsigned long arg)
>> +{
>> +     struct dw_mci *host = (struct dw_mci *)arg;
>> +
>> +     if (host->state != STATE_SENDING_CMD11)
>> +             dev_info(host->dev, "Unexpected CMD11 timeout\n");
>
> If Unexpected CMD11 timeout, can it do  just" return"?
> Well, I think Unexpected CMD11 timeout is an rare case.

Duh, of course.  I'm happy to respin this or I'm happy if you want to
just add a "return;"  Please let me know.

-Doug
--
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/