Received: by 2002:a05:6a10:1a4d:0:0:0:0 with SMTP id nk13csp3604185pxb; Fri, 4 Feb 2022 12:09:27 -0800 (PST) X-Google-Smtp-Source: ABdhPJyI/EXa+txQDuFuzHR3g4jqp86cZgdEvPrR1KEuOpplfoRgdglG6/ck7U3xSQEru4WbTwf7 X-Received: by 2002:a17:907:6d29:: with SMTP id sa41mr498812ejc.272.1644005367083; Fri, 04 Feb 2022 12:09:27 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1644005367; cv=none; d=google.com; s=arc-20160816; b=shUTzWWukB6Zr2NS49Zl7RTXM0i0v9Fn5rlRG2ibf3qxXIcSXwF8bWG7vXhpX18LDh 8m8iIkmo8a/SDjJ9vjHO8kvhmHCdnF2eGwQ4WS4Qnu5X6afV1of9mtJoSn5Z8FCllCAA R2NIQrZdcaT6FrUXfMeCAenvzMlyiQIiaeSIv/l46u5e3omcdWDFUfcVdCpiNHPXIFhG WmXCZJTFmjjuW3LvrdxAJogCrOEId5v6RS+W6X+Quq56FHpJGq+nYwO3gSJOffoUY8AP 8oacFPQZsxDKCZQMd6x3E47jVQg3gnFNRd1iYOMigwwebAhfepowmp954Ltvbmg+UG8v OfRg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:organization :from:references:cc:to:subject:dkim-signature; bh=1ygOFTIYOY/lna7oFx8jV+R9Xs1Jo/haY8D9IBcgRy8=; b=OkKz+9YnjYZ4JdsyO0220FOxDlh9MDcXbID0mCdHW9dRtmQO4VjzndvQIWhHH51NKP TaO20j+hViaGdOEjtnIK1Si4o1zTt1+De3NReALjcNHCSCOmpm0mSk1m9A70cr5ECBzb sT89qNjuXUyiOFAHa7/gu/nAKvWJj1sMu3UfZ2ssKxjqJiiqhEpTAHwUKBQVgK+w2fr4 OEfQlobCTFYNZBMN5tJdEv0sOGQqm48Eqk10HLfFBpdQyh+oN0WTO14Gefz+yvBaiREa nwqtZ8LDWx2/rrKGwuqZnpg2G+8/lLHfLvexBsSNqh4v9id3Zl3Ef97QL3tq06IWkhaT GysA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=dI5mwMdY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id hv8si2126089ejc.268.2022.02.04.12.09.00; Fri, 04 Feb 2022 12:09:27 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=dI5mwMdY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1358346AbiBDL0N (ORCPT + 99 others); Fri, 4 Feb 2022 06:26:13 -0500 Received: from mga14.intel.com ([192.55.52.115]:60004 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235838AbiBDL0M (ORCPT ); Fri, 4 Feb 2022 06:26:12 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643973972; x=1675509972; h=subject:to:cc:references:from:message-id:date: mime-version:in-reply-to:content-transfer-encoding; bh=zOiOeg3TVAJfReAk+ZzPsbdrjm9uWUHKiDyGZw8xRIM=; b=dI5mwMdYKLuzqHLSXWZ3CGsugML1MSENybODW2nLAgb9jG/wtA0rhOLZ G2l+aIDJHvwEa3vDoxAabJn0qvSfXqnEEZlzst1VldjMMhYfTM3H5AntK Uq3HCHynreBxvzyvBRTEcH1fCXBUmzyBorqq2E7KfXeMgh8U6PcPVnHRy EdGJs0SzZMjlmWgechv8sM5dgoYtrT9nKe/L1jEl2B7Yc1c2qFe3jAHaZ B6I0lWoH8CMheONDF72Z+b9nuGasAf/PAs84LVpzc2t90B2l6PdSo6PkI mWyCJs5gq1D9Z/dHtclXggBgLubSZ3tNsdohEWqgMMWVUf0qOX1dshNDy w==; X-IronPort-AV: E=McAfee;i="6200,9189,10247"; a="248565680" X-IronPort-AV: E=Sophos;i="5.88,342,1635231600"; d="scan'208";a="248565680" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Feb 2022 03:26:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.88,342,1635231600"; d="scan'208";a="631673031" Received: from ahunter-desktop.fi.intel.com (HELO [10.237.72.92]) ([10.237.72.92]) by orsmga004.jf.intel.com with ESMTP; 04 Feb 2022 03:26:10 -0800 Subject: Re: [PATCH] mmc: block: fix read single on recovery logic To: Ulf Hansson , =?UTF-8?Q?Christian_L=c3=b6hle?= Cc: "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Avri Altman References: <5e5f2e45d0a14a55a8b7a9357846114b@hyperstone.com> <7c4757cc707740e580c61c39f963a04d@hyperstone.com> From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: <5c66833d-4b35-2c76-db54-0306e08843e5@intel.com> Date: Fri, 4 Feb 2022 13:26:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/02/2022 11:47, Ulf Hansson wrote: > + Adrian > > On Thu, 3 Feb 2022 at 11:09, Christian Löhle wrote: >> >> So could anyone take a long at this so far? >> > > Thanks for pinging. Apologize for the delay, it's on top of my "to-review" list. > > I have added Adrian too, who knows this code very well too. > > Kind regards > Uffe > >> >> >> From: Christian Löhle >> Sent: Wednesday, January 5, 2022 5:43 PM >> To: ulf.hansson@linaro.org; Christian Löhle; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org >> Cc: Avri Altman >> Subject: [PATCH] mmc: block: fix read single on recovery logic >> >> On reads with MMC_READ_MULTIPLE_BLOCK that fail, >> the recovery handler will use MMC_READ_SINGLE_BLOCK for >> each of the blocks, up to MMC_READ_SINGLE_RETRIES times each. >> The logic for this is fixed to never report unsuccessful reads >> as success to the block layer. >> >> On command error with retries remaining, blk_update_request was >> called with whatever value error was set last to. >> In case it was last set to BLK_STS_OK (default), the read will be >> reported as success, even though there was no data read from the device. >> This could happen on a CRC mismatch for the response, >> a card rejecting the command (e.g. again due to a CRC mismatch). >> In case it was last set to BLK_STS_IOERR, the error is reported correctly, >> but no retries will be attempted. >> >> The patch now will count both command and data errors as retries and >> send BLK_STS_IOERR if there are no retries remaining, >> or BLK_STS_OK if the single read was successful in the meantime. >> >> Signed-off-by: Christian Loehle Thanks for the patch. Looks OK, although a couple of comments below, plus it needs a Fixes tag, and Cc for stable. Fixes: 81196976ed946c ("mmc: block: Add blk-mq support") Cc: stable@vger.kernel.org >> --- >> drivers/mmc/core/block.c | 28 ++++++++++++++-------------- >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c >> index 90e1bcd03b46..d7d880ce0f8a 100644 >> --- a/drivers/mmc/core/block.c >> +++ b/drivers/mmc/core/block.c >> @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) >> struct mmc_card *card = mq->card; >> struct mmc_host *host = card->host; >> blk_status_t error = BLK_STS_OK; >> - int retries = 0; >> >> do { >> u32 status; >> int err; >> + int retries = 0; >> >> - mmc_blk_rw_rq_prep(mqrq, card, 1, mq); >> + while (retries++ < MMC_READ_SINGLE_RETRIES) { Because this is now checked at the top of the loop, wouldn't that result in one fewer retries than before? So, maybe: while (retries++ <= MMC_READ_SINGLE_RETRIES) { >> + mmc_blk_rw_rq_prep(mqrq, card, 1, mq); >> >> - mmc_wait_for_req(host, mrq); >> + mmc_wait_for_req(host, mrq); >> >> - err = mmc_send_status(card, &status); >> - if (err) >> - goto error_exit; >> - >> - if (!mmc_host_is_spi(host) && >> - !mmc_ready_for_data(status)) { >> - err = mmc_blk_fix_state(card, req); >> + err = mmc_send_status(card, &status); >> if (err) >> goto error_exit; >> - } >> >> - if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES) >> - continue; >> + if (!mmc_host_is_spi(host) && >> + !mmc_ready_for_data(status)) { >> + err = mmc_blk_fix_state(card, req); >> + if (err) >> + goto error_exit; >> + } >> >> - retries = 0; >> + if (!mrq->cmd->error && !mrq->data->error) We weren't retrying for data errors before, and I don't think we want to because single block read can be very slow. i.e. just if (!mrq->cmd->error) >> + break; >> + } >> >> if (mrq->cmd->error || >> mrq->data->error || >> -- >> 2.34.1 >> = >> Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz >> Managing Director: Dr. Jan Peter Berns. >> Commercial register of local courts: Freiburg HRB381782 >>