linux-kernel - Re: [PATCH v2 09/16] mmc: sdhci: Add quirk to disable HW timeout

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:   Mon, 19 Feb 2018 10:51:20 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Kishon Vijay Abraham I <kishon@...com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Tony Lindgren <tony@...mide.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        linux-mmc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 09/16] mmc: sdhci: Add quirk to disable HW timeout

On 05/02/18 14:50, Kishon Vijay Abraham I wrote:
> Add quirk to disable HW timeout if the requested timeout is more than
> the maximum obtainable timeout.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@...com>
> ---
>  drivers/mmc/host/sdhci.c | 12 ++++++++++++
>  drivers/mmc/host/sdhci.h |  7 +++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 070aff9c108f..1aa74b4682f3 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -735,6 +735,12 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>  		DBG("Too large timeout 0x%x requested for CMD%d!\n",
>  		    count, cmd->opcode);
>  		count = 0xE;
> +		if (host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) {
> +			host->ier &= ~SDHCI_INT_DATA_TIMEOUT;
> +			sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
> +			sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
> +			host->hw_timeout_disabled = true;
> +		}

sdhci_calc_timeout() is really for calculations so please do this in
sdhci_set_timeout() instead (i.e. change sdhci_calc_timeout() to return
whether the timeout is too big).  Note that you need to cater for the "/*
Unspecified timeout, assume max */" case and the
SDHCI_QUIRK_BROKEN_TIMEOUT_VAL case.

Also if SDHCI_INT_DATA_TIMEOUT is not being disabled, check here to
re-enable it and leave sdhci_request_done() alone. i.e.

	else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
		host->ier |= SDHCI_INT_DATA_TIMEOUT;
		sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
		sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
	}

Maybe make a separate subroutine for the update:

void sdhci_set_data_timeout_irq(struct sdhci_host *host, bool enable)
{
	if (enable)
		host->ier |= SDHCI_INT_DATA_TIMEOUT;
	else
		host->ier &= ~SDHCI_INT_DATA_TIMEOUT;
	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
}


>  	}
>  
>  	return count;
> @@ -2349,6 +2355,12 @@ static bool sdhci_request_done(struct sdhci_host *host)
>  	}
>  
>  	sdhci_del_timer(host, mrq);
> +	if (sdhci_data_line_cmd(mrq->cmd) && host->hw_timeout_disabled) {
> +		host->ier |= SDHCI_INT_DATA_TIMEOUT;
> +		sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
> +		sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
> +		host->hw_timeout_disabled = false;
> +	}
>  
>  	/*
>  	 * Always unmap the data buffers if they were mapped by
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index afab26fd70e6..3a967a56fcc3 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -437,6 +437,11 @@ struct sdhci_host {
>  #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
>  /* Controller has CRC in 136 bit Command Response */
>  #define SDHCI_QUIRK2_RSP_136_HAS_CRC			(1<<16)
> +/*
> + * Disable HW timeout if the requested timeout is more than the maximum
> + * obtainable timeout
> + */
> +#define SDHCI_QUIRK2_DISABLE_HW_TIMEOUT			(1<<17)
>  
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
> @@ -522,6 +527,8 @@ struct sdhci_host {
>  	unsigned int            ocr_avail_mmc;
>  	u32 ocr_mask;		/* available voltages */
>  
> +	bool			hw_timeout_disabled;
> +
>  	unsigned		timing;		/* Current timing */
>  
>  	u32			thread_isr;
>