linux-kernel - Re: [PATCH 2/3] mmc: dw_mmc: Fix the CTO timeout calculation

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: <baeb929b-a712-0c69-2963-feb00c0165d9@rock-chips.com>
Date:   Mon, 9 Oct 2017 15:03:08 +0800
From:   Shawn Lin <shawn.lin@...k-chips.com>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     jh80.chung@...sung.com, ulf.hansson@...aro.org,
        shawn.lin@...k-chips.com, briannorris@...omium.org,
        amstan@...omium.org, xzy.xu@...k-chips.com,
        linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] mmc: dw_mmc: Fix the CTO timeout calculation

Hi

On 2017/9/28 4:56, Douglas Anderson wrote:
> In the commit 03de19212ea3 ("mmc: dw_mmc: introduce timer for broken
> command transfer over scheme") we tried to calculate the expected
> hardware command timeout value.  Unfortunately that calculation isn't
> quite correct in all cases.  It used "bus_hz" but, as far as I can
> tell, it's supposed to use the card clock.  Let's account for the div
> value, which is documented as 2x the value stored in the register, or
> 1 if the register is 0.

Good catch.
Would you mind appending a new patch to fix the drto case?

Reviewed-by: Shawn Lin <shawn.lin@...k-chips.com>

> 
> NOTE: It's not expected that this will actually fix anything important
> since the 10 ms margin added by the function will pretty much dwarf
> any calculations.  The card clock should be 100 kHz at minimum and:
>    1000 ms/s * (255 * 2) / 100000 Hz.
> Gives us 5.1 ms.
> 
> ...so really the point of this patch is just to make the code more
> "correct" in case anyone ever tries to remove the 10 ms buffer.
> 
> Fixes: 03de19212ea3 ("mmc: dw_mmc: introduce timer for broken command transfer over scheme")
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> 
>   drivers/mmc/host/dw_mmc.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index f5b2bb4b4d98..16516c528a88 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -401,10 +401,14 @@ static u32 dw_mci_prep_stop_abort(struct dw_mci *host, struct mmc_command *cmd)
>   static inline void dw_mci_set_cto(struct dw_mci *host)
>   {
>   	unsigned int cto_clks;
> +	unsigned int cto_div;
>   	unsigned int cto_ms;
>   
>   	cto_clks = mci_readl(host, TMOUT) & 0xff;
> -	cto_ms = DIV_ROUND_UP(cto_clks, host->bus_hz / 1000);
> +	cto_div = (mci_readl(host, CLKDIV) & 0xff) * 2;
> +	if (cto_div == 0)
> +		cto_div = 1;
> +	cto_ms = DIV_ROUND_UP(MSEC_PER_SEC * cto_clks * cto_div, host->bus_hz);
>   
>   	/* add a bit spare time */
>   	cto_ms += 10;
>