linux-kernel - Re: [PATCH v4 2/2] mmc: sdhci-omap: Don't finish

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: <05a2f5ef-1fee-15f4-6a9b-e3f3f3ee5a82@ti.com>
Date:   Wed, 27 Mar 2019 17:17:08 +0530
From:   Faiz Abbas <faiz_abbas@...com>
To:     Adrian Hunter <adrian.hunter@...el.com>,
        <linux-kernel@...r.kernel.org>, <linux-mmc@...r.kernel.org>,
        <linux-omap@...r.kernel.org>
CC:     <ulf.hansson@...aro.org>, <kishon@...com>
Subject: Re: [PATCH v4 2/2] mmc: sdhci-omap: Don't finish_mrq() on a command
 error during tuning

Hi Adrian,

On 27/03/19 4:45 PM, Adrian Hunter wrote:
> On 26/03/19 1:00 PM, Faiz Abbas wrote:
>> commit 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
>> callback") skips data resets during tuning operation. Because of this,
>> a data error or data finish interrupt might still arrive after a command
>> error has been handled and the mrq ended. This ends up with a "mmc0: Got
>> data interrupt 0x00000002 even though no data operation was in progress"
>> error message.
>>
>> Fix this by adding a platform specific callback for sdhci_irq. Mark the
>> mrq as a failure but wait for a data interrupt instead of calling
>> finish_mrq().
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@...com>
>> ---
>>  drivers/mmc/host/sdhci-omap.c | 37 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
>> index 5bbed477c9b1..9da2ff9ede8b 100644
>> --- a/drivers/mmc/host/sdhci-omap.c
>> +++ b/drivers/mmc/host/sdhci-omap.c
>> @@ -797,6 +797,42 @@ void sdhci_omap_reset(struct sdhci_host *host, u8 mask)
>>  	sdhci_reset(host, mask);
>>  }
>>  
>> +#define CMD_ERR_MASK (SDHCI_INT_CRC | SDHCI_INT_END_BIT | SDHCI_INT_INDEX |\
>> +		      SDHCI_INT_TIMEOUT)
>> +#define CMD_MASK (CMD_ERR_MASK | SDHCI_INT_RESPONSE)
>> +
>> +static irqreturn_t sdhci_omap_irq(struct sdhci_host *host, u32 intmask)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	if (omap_host->is_tuning && (intmask & CMD_ERR_MASK)) {
>> +
>> +		/*
>> +		 * Since we are not resetting data lines during tuning
>> +		 * operation, data error or data complete interrupts
>> +		 * might still arrive. Mark this request as a failure
>> +		 * but still wait for the data interrupt
>> +		 */
>> +		if (intmask & SDHCI_INT_TIMEOUT)
>> +			host->cmd->error = -ETIMEDOUT;
>> +		else
>> +			host->cmd->error = -EILSEQ;
>> +
>> +		sdhci_finish_command(host);
> 
> Would it be possible to set SDHCI_INT_RESPONSE instead of calling
> sdhci_finish_command() directly?
>

It should be possible. But what we are trying to do won't be very clear
from the code. Ideally an interrupt handler should not set any interrupt
bits, just handle and clear them. Also, setting the RESPONSE bit even
when there has been no response seems wrong and makes the code more
convoluted.

Thanks,
Faiz