Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753543AbbKZVRr (ORCPT ); Thu, 26 Nov 2015 16:17:47 -0500 Received: from mail-yk0-f174.google.com ([209.85.160.174]:36308 "EHLO mail-yk0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753168AbbKZVRm (ORCPT ); Thu, 26 Nov 2015 16:17:42 -0500 MIME-Version: 1.0 In-Reply-To: <1448541369-23086-1-git-send-email-chaotian.jing@mediatek.com> References: <1448541369-23086-1-git-send-email-chaotian.jing@mediatek.com> Date: Thu, 26 Nov 2015 22:17:41 +0100 Message-ID: Subject: Re: [PATCH v2] mmc: core: fix __mmc_switch timeout caused by preempt From: Ulf Hansson To: Chaotian Jing Cc: Matthias Brugger , Stephen Boyd , Adrian Hunter , Minda Chen , linux-mmc , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , linux-mediatek@lists.infradead.org, srv_heupstream Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3828 Lines: 88 On 26 November 2015 at 13:36, Chaotian Jing wrote: > there is a time window between __mmc_send_status() and time_afer(), > on some eMMC chip, the timeout_ms is only 10ms, if this thread was > scheduled out during this period, then, even card has already changes > to transfer state by the result of CMD13, this part of code also treat > it to timeout error. > So, need calculate timeout first, then call __mmc_send_status(), if > already timeout and card still in programing state, then treat it to > the real timeout error. > > Signed-off-by: Chaotian Jing > --- > drivers/mmc/core/mmc_ops.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index 1f44426..eba5295 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -489,6 +489,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > unsigned long timeout; > u32 status = 0; > bool use_r1b_resp = use_busy_signal; > + bool expired; > > mmc_retune_hold(host); > > @@ -545,9 +546,23 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > timeout = jiffies + msecs_to_jiffies(timeout_ms); > do { > if (send_status) { > + /* > + * Timeout if the device never leaves the program state. > + * Due to the possibility of being preempted after > + * sending the status command, check the expiration > + * time first. > + */ > + expired = time_after(jiffies, timeout); > err = __mmc_send_status(card, &status, ignore_crc); > if (err) > goto out; > + if (expired && > + R1_CURRENT_STATE(status) == R1_STATE_PRG) { > + pr_err("%s: Card stuck in programming state! %s\n", > + mmc_hostname(host), __func__); > + err = -ETIMEDOUT; > + goto out; > + } This "if" actually means we will start verifying the card state, even when "MMC_CAP_WAIT_WHILE_BUSY && use_r1b_resp". I don't think we need or should. Instead let's move this code at its original place below. That also means you indeed need to assign an initial value for "expired" to "false" in the beginning of the function. Apologize for not spotting this change in behaviour in v1. > } > if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) > break; > @@ -563,14 +578,6 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > mmc_delay(timeout_ms); > goto out; > } > - > - /* Timeout if the device never leaves the program state. */ > - if (time_after(jiffies, timeout)) { > - pr_err("%s: Card stuck in programming state! %s\n", > - mmc_hostname(host), __func__); > - err = -ETIMEDOUT; > - goto out; > - } > } while (R1_CURRENT_STATE(status) == R1_STATE_PRG); > > err = mmc_switch_status_error(host, status); > -- > 1.8.1.1.dirty > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/