linux-kernel - Re: [RFC 1/4] sdhci: allow for multiple delays when sending commands

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:	Fri, 18 Mar 2011 19:10:40 -0400
From:	Chris Ball <cjb@...top.org>
To:	"Luis R. Rodriguez" <lrodriguez@...eros.com>
Cc:	<linux-mmc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<vmehta@...eros.com>, <naveen.singh@...eros.com>,
	<kvalo@...rom.com>, <j@...fi>, Naveen Singh <nsingh@...eros.com>,
	Vipin Mehta <Vipin.Mehta@...eros.com>
Subject: Re: [RFC 1/4] sdhci: allow for multiple delays when sending commands

Hi Luis,

On Fri, Mar 18 2011, Luis R. Rodriguez wrote:
> This is at least known to be required for the ENE 714.
>
> Cc: Chris Ball <cjb@...top.org>
> Cc: Kalle Valo <kvalo@...rom.com>
> Cc: Naveen Singh <nsingh@...eros.com>
> Cc: Vipin Mehta <Vipin.Mehta@...eros.com>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@...eros.com>

I think this wins cjb's "I've never been so confused about what a patch
author thought they were doing before" award.

> ---
>  drivers/mmc/host/sdhci.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 9e15f41..c95dfc2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -891,8 +891,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>  
>  	WARN_ON(host->cmd);
>  
> -	/* Wait max 10 ms */
> -	timeout = 10;
> +	/* Wait max this amount of ms */
> +	timeout = (10*256) + 255;
>  
>  	mask = SDHCI_CMD_INHIBIT;
>  	if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))

Okay, so our original plan is to go through the while loop ten times,
which equals ten mdelay(1)s == 10ms, waiting for the inhibit bit to
become unset.

After this hunk of your patch, we're set to go through the loop 2815
times, which would make for 2.8 seconds.  That seems excessive.

> @@ -913,7 +913,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>  			return;
>  		}
>  		timeout--;
> -		mdelay(1);
> +		if (!(timeout & 0xFF))
> +			mdelay(1);
>  	}
>  
>  	mod_timer(&host->timer, jiffies + 10 * HZ);

But wait, here you decide *not* to call mdelay(1) every time through the
loop, and instead call it only on iterations where the bottom eight bits
are unset.  This disqualifies most of the 2815 values that timeout will
be set to, and leaves the following values triggering the mdelay(1):

0x100 0x200 0x300 0x400 0x500 0x600 0x700 0x800 0x900 0xa00
  256   512   768  1024  1280  1536  1792  2048  2304  2560

The astute observer will notice that there are ten such values.
So you're calling mdelay(1) ten times.  But that's what we were
doing before!  The only difference is that now we spin through
the while loop 2815 times instead of 10, and don't perform any
explicit delay on 2805 of them.  Or am I missing something?

I think you should try:

(a) Reverting the patch and checking that it's actually needed
(b) Leaving the while loop body alone, but increasing the max
    timeout until you bisect to the amount of ms that your controller
    actually takes to release the inhibit bit.
(c) Yelling loudly in the direction that this code came from.  :)

- Chris.
-- 
Chris Ball   <cjb@...top.org>   <http://printf.net/>
One Laptop Per Child
--
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/