Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <618e533c-7155-3a8d-53f1-04c436a21364@intel.com>
Date:   Fri, 27 May 2022 15:44:03 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.9.1
Subject: Re: [PATCH V1] mmc: core: Enable force hw reset
Content-Language: en-US
To:     "Sarthak Garg (QUIC)" <quic_sartgarg@quicinc.com>,
        "Kamasali Satyanarayan (Consultant) (QUIC)" 
        <quic_kamasali@quicinc.com>, quic_spathi <quic_spathi@quicinc.com>,
        "ulf.hansson@linaro.org" <ulf.hansson@linaro.org>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        "avri.altman@wdc.com" <avri.altman@wdc.com>,
        "kch@nvidia.com" <kch@nvidia.com>,
        "CLoehle@hyperstone.com" <CLoehle@hyperstone.com>,
        "swboyd@chromium.org" <swboyd@chromium.org>,
        "digetx@gmail.com" <digetx@gmail.com>,
        "bigeasy@linutronix.de" <bigeasy@linutronix.de>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <1650961818-13452-1-git-send-email-quic_spathi@quicinc.com>
 <7db46c19-a92a-a13a-eb63-38e5ed31580f@intel.com>
 <MWHPR0201MB3466CC9C676988E90D5D7D6FE3D79@MWHPR0201MB3466.namprd02.prod.outlook.com>
 <f21c11f86bf94e9b881761b9176cc45e@quicinc.com>
From:   Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
In-Reply-To: <f21c11f86bf94e9b881761b9176cc45e@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 25/05/22 10:06, Sarthak Garg (QUIC) wrote:
> Hi Adrian,
> 
> Thanks for the review.
> Please find comments inline.
> 
> Thanks,
> Sarthak
> 
>> -----Original Message-----
>> From: Kamasali Satyanarayan (Consultant) (QUIC)
>> <quic_kamasali@quicinc.com>
>> Sent: Tuesday, May 24, 2022 5:33 PM
>> To: 'Adrian Hunter' <adrian.hunter@intel.com>; quic_spathi
>> <quic_spathi@quicinc.com>; ulf.hansson@linaro.org; riteshh@codeaurora.org;
>> asutoshd@codeaurora.org; axboe@kernel.dk; avri.altman@wdc.com;
>> kch@nvidia.com; CLoehle@hyperstone.com; swboyd@chromium.org;
>> digetx@gmail.com; bigeasy@linutronix.de; linux-mmc@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Sarthak Garg (QUIC) <quic_sartgarg@quicinc.com>
>> Cc: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
>> Subject: RE: [PATCH V1] mmc: core: Enable force hw reset
>>
>> Hi,
>> These patches will be further taken by Sarthak.
>>
>> Thanks,
>> Satya
>>
>> -----Original Message-----
>> From: Adrian Hunter <adrian.hunter@intel.com>
>> Sent: Wednesday, April 27, 2022 6:04 PM
>> To: quic_spathi <quic_spathi@quicinc.com>; ulf.hansson@linaro.org;
>> riteshh@codeaurora.org; asutoshd@codeaurora.org; axboe@kernel.dk;
>> avri.altman@wdc.com; kch@nvidia.com; CLoehle@hyperstone.com;
>> swboyd@chromium.org; digetx@gmail.com; bigeasy@linutronix.de; linux-
>> mmc@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: Shaik Sajida Bhanu <sbhanu@codeaurora.org>; Kamasali Satyanarayan
>> (Consultant) (QUIC) <quic_kamasali@quicinc.com>
>> Subject: Re: [PATCH V1] mmc: core: Enable force hw reset
>>
>> On 26/04/22 11:30, Srinivasarao Pathipati wrote:
>>> From: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
>>>
>>> During error recovery set need hw reset to handle ICE error where cqe
>>> reset is must.
>>
>> How do you get ICE errors?  Doesn't it mean either the hardware is broken or
>> the configuration is broken?
> 
> This patch is not intended for ice errors and will update the commit text in V2.
> Long back intermittent recovery failures were observed but after forcing hardware reset during error recovery we have no single instance of recovery failure. This have made recovery more robust for us.
> Any suggestions on how we can take it forward will be highly appreciated.

We can definitely go forward, but with hopefully a little more
explanation first.

It is preferable to be able to explain why changes are being made.

Do you have any logs or other information on the recovery failures?
Are you able to reproduce the problem?

I notice you always do mmc_blk_reset_success().  Does that mean you
sometimes need several resets in a row?

A potential issue that I notice, is that the recovery does not
explicitly deal with the case that the card's command queue has
been disabled e.g. due to RPMB access or IOCTL commands. Are you
using either of those?

>>
>>>
>>> Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
>>> Signed-off-by: kamasali <quic_kamasali@quicinc.com>
>>> Signed-off-by: Srinivasarao Pathipati <quic_spathi@quicinc.com>
>>> ---
>>>  drivers/mmc/core/block.c      | 8 +++++---
>>>  drivers/mmc/host/cqhci-core.c | 7 +++++--
>>>  include/linux/mmc/host.h      | 1 +
>>>  3 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
>>> b35e7a9..f63bf33 100644
>>> --- a/drivers/mmc/core/block.c
>>> +++ b/drivers/mmc/core/block.c
>>> @@ -1482,10 +1482,12 @@ void mmc_blk_cqe_recovery(struct mmc_queue
>> *mq)
>>>  	pr_debug("%s: CQE recovery start\n", mmc_hostname(host));
>>>
>>>  	err = mmc_cqe_recovery(host);
>>> -	if (err)
>>> +	if (err || host->need_hw_reset) {
>>>  		mmc_blk_reset(mq->blkdata, host,
>> MMC_BLK_CQE_RECOVERY);
>>> -	else
>>> -		mmc_blk_reset_success(mq->blkdata,
>> MMC_BLK_CQE_RECOVERY);
>>> +		if (host->need_hw_reset)
>>> +			host->need_hw_reset = false;
>>> +	}
>>> +	mmc_blk_reset_success(mq->blkdata, MMC_BLK_CQE_RECOVERY);
>>>
>>>  	pr_debug("%s: CQE recovery done\n", mmc_hostname(host));  } diff
>>> --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
>>> index b0d30c3..311b510 100644
>>> --- a/drivers/mmc/host/cqhci-core.c
>>> +++ b/drivers/mmc/host/cqhci-core.c
>>> @@ -812,18 +812,21 @@ static void cqhci_finish_mrq(struct mmc_host
>>> *mmc, unsigned int tag)  irqreturn_t cqhci_irq(struct mmc_host *mmc, u32
>> intmask, int cmd_error,
>>>  		      int data_error)
>>>  {
>>> -	u32 status;
>>> +	u32 status, ice_err;
>>>  	unsigned long tag = 0, comp_status;
>>>  	struct cqhci_host *cq_host = mmc->cqe_private;
>>>
>>>  	status = cqhci_readl(cq_host, CQHCI_IS);
>>>  	cqhci_writel(cq_host, status, CQHCI_IS);
>>> +	ice_err = status & (CQHCI_IS_GCE | CQHCI_IS_ICCE);
>>>
>>>  	pr_debug("%s: cqhci: IRQ status: 0x%08x\n", mmc_hostname(mmc),
>>> status);
>>>
>>>  	if ((status & (CQHCI_IS_RED | CQHCI_IS_GCE | CQHCI_IS_ICCE)) ||
>>> -	    cmd_error || data_error)
>>> +	    cmd_error || data_error || ice_err){
>>> +		mmc->need_hw_reset = true;
>>>  		cqhci_error_irq(mmc, status, cmd_error, data_error);
>>> +	}
>>>
>>>  	if (status & CQHCI_IS_TCC) {
>>>  		/* read TCN and complete the request */ diff --git
>>> a/include/linux/mmc/host.h b/include/linux/mmc/host.h index
>>> c193c50..3d00bcf 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -492,6 +492,7 @@ struct mmc_host {
>>>  	int			cqe_qdepth;
>>>  	bool			cqe_enabled;
>>>  	bool			cqe_on;
>>> +	bool                    need_hw_reset;
>>>
>>>  	/* Inline encryption support */
>>>  #ifdef CONFIG_MMC_CRYPTO
>>
>