linux-kernel - Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6

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]

Date:   Tue, 31 Jan 2017 15:57:39 +0530
From:   Ravikumar <rk@...com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
CC:     Peter Ujfalusi <peter.ujfalusi@...com>,
        Sekhar Nori <nsekhar@...com>, Tony Lindgren <tony@...mide.com>,
        Andreas Fenkart <afenkart@...il.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        linux-omap <linux-omap@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kishon <kishon@...com>
Subject: Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to
 program timeout value for CMD6



On Tuesday 31 January 2017 03:42 PM, Ulf Hansson wrote:
> On 30 January 2017 at 11:11, Ravikumar Kattekola <rk@...com> wrote:
>> From: Kishon Vijay Abraham I <kishon@...com>
>>
>> commit e2bf08d643a244ccb ("omap_hsmmc: set a large data timeout for
>> commands with busy signal") sets an arbitrary timeout value (100ms) for
>> commands like CMD6 (MMC SWITCH). However extended CSD register defined
>> in the eMMC standard has a field for GENERIC_CMD6_TIME which indicates
>> the default maximum timeout for a SWITCH command.
>> Use busy_timeout of cmd structure (populated with GENERIC_CMD6_TIME
>> in the case of SWITCH command) to program the data timeout value in
>> omap_hsmmc driver.
>> SWITCH command to turn the cache on took more than 100ms to complete
>> with MICRON eMMC card present in AM572x IDK REV 1.3A resulting in
>> timeout and failed enumeration. It is fixed here by programming the
>> timeout with the value advertised in GENERIC_CMD6_TIME.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@...com>
>> Signed-off-by: Sekhar Nori <nsekhar@...com>
>> Signed-off-by: Ravikumar Kattekola <rk@...com>
>> ---
>>   drivers/mmc/host/omap_hsmmc.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index 0ee5650..51ca3d7 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -1527,16 +1527,24 @@ static void omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host)
>>   omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req)
>>   {
>>          int ret;
>> +       unsigned int timeout;
>> +
>>          host->data = req->data;
>>
>>          if (req->data == NULL) {
>>                  OMAP_HSMMC_WRITE(host->base, BLK, 0);
>> -               /*
>> -                * Set an arbitrary 100ms data timeout for commands with
>> -                * busy signal.
>> -                */
>> -               if (req->cmd->flags & MMC_RSP_BUSY)
>> -                       set_data_timeout(host, 100000000U, 0);
>> +               if (req->cmd->flags & MMC_RSP_BUSY) {
>> +                       timeout = req->cmd->busy_timeout * NSEC_PER_MSEC;
>> +
>> +                       /*
>> +                        * Set an arbitrary 100ms data timeout for commands with
>> +                        * busy signal and no indication of busy_timeout.
>> +                        */
>> +                       if (!timeout)
> This is a bug in the mmc core if this ever happen.
>
> Therefore I am particularly interested to find out if this is really
> needed or it's just playing safe?
You could call it playing safe.
We haven't hit any case where it was set to zero but per mmc_switch() 
description you are allowed to set it to zero to let the host decide 
what it wants to use.

>> +                               timeout = 100000000U;
>> +
>> +                       set_data_timeout(host, timeout, 0);
>> +               }
>>                  return 0;
>>          }
>>
>> --
>> 1.9.1
>>
> Kind regards
> Uffe
Regards,
RK