Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755779AbcCCHYd (ORCPT ); Thu, 3 Mar 2016 02:24:33 -0500 Received: from mx2.suse.de ([195.135.220.15]:36739 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755350AbcCCHYb (ORCPT ); Thu, 3 Mar 2016 02:24:31 -0500 Subject: Re: [PATCH v5 04/15] scsi: ufs: verify hba controller hce reg value To: ygardi@codeaurora.org References: <1456666367-11418-1-git-send-email-ygardi@codeaurora.org> <1456666367-11418-5-git-send-email-ygardi@codeaurora.org> <56D545AB.1030606@suse.de> Cc: james.bottomley@hansenpartnership.com, linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org, santoshsy@gmail.com, linux-scsi-owner@vger.kernel.org, Raviv Shvili , Vinayak Holikatti , "James E.J. Bottomley" , "Martin K. Petersen" From: Hannes Reinecke Message-ID: <56D7E69F.4070200@suse.de> Date: Thu, 3 Mar 2016 15:24:15 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4428 Lines: 125 On 03/01/2016 09:32 PM, ygardi@codeaurora.org wrote: >> On 02/28/2016 09:32 PM, Yaniv Gardi wrote: >>> Sometimes due to hw issues it takes some time to the >>> host controller register to update. In order to verify the register >>> has updated, a polling is done until its value is set. >>> >>> In addition the functions ufshcd_hba_stop() and >>> ufshcd_wait_for_register() was updated with an additional input >>> parameter, indicating the timeout between reads will >>> be done by sleeping or spinning the cpu. >>> >>> Signed-off-by: Raviv Shvili >>> Signed-off-by: Yaniv Gardi >>> >>> --- >>> drivers/scsi/ufs/ufshcd.c | 53 >>> ++++++++++++++++++++++++++++------------------- >>> drivers/scsi/ufs/ufshcd.h | 12 +++-------- >>> 2 files changed, 35 insertions(+), 30 deletions(-) >>> >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>> index 3400ceb..80031e6 100644 >>> --- a/drivers/scsi/ufs/ufshcd.c >>> +++ b/drivers/scsi/ufs/ufshcd.c >>> @@ -240,11 +240,13 @@ static inline void ufshcd_disable_irq(struct >>> ufs_hba *hba) >>> * @val - wait condition >>> * @interval_us - polling interval in microsecs >>> * @timeout_ms - timeout in millisecs >>> + * @can_sleep - perform sleep or just spin >>> * >>> * Returns -ETIMEDOUT on error, zero on success >>> */ >>> -static int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 >>> mask, >>> - u32 val, unsigned long interval_us, unsigned long timeout_ms) >>> +int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask, >>> + u32 val, unsigned long interval_us, >>> + unsigned long timeout_ms, bool can_sleep) >>> { >>> int err = 0; >>> unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms); >>> @@ -253,9 +255,10 @@ static int ufshcd_wait_for_register(struct ufs_hba >>> *hba, u32 reg, u32 mask, >>> val = val & mask; >>> >>> while ((ufshcd_readl(hba, reg) & mask) != val) { >>> - /* wakeup within 50us of expiry */ >>> - usleep_range(interval_us, interval_us + 50); >>> - >>> + if (can_sleep) >>> + usleep_range(interval_us, interval_us + 50); >>> + else >>> + udelay(interval_us); >>> if (time_after(jiffies, timeout)) { >>> if ((ufshcd_readl(hba, reg) & mask) != val) >>> err = -ETIMEDOUT; >>> @@ -1459,7 +1462,7 @@ ufshcd_clear_cmd(struct ufs_hba *hba, int tag) >>> */ >>> err = ufshcd_wait_for_register(hba, >>> REG_UTP_TRANSFER_REQ_DOOR_BELL, >>> - mask, ~mask, 1000, 1000); >>> + mask, ~mask, 1000, 1000, true); >>> >>> return err; >>> } >>> @@ -2815,6 +2818,23 @@ out: >>> } >>> >>> /** >>> + * ufshcd_hba_stop - Send controller to reset state >>> + * @hba: per adapter instance >>> + * @can_sleep: perform sleep or just spin >>> + */ >>> +static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep) >>> +{ >>> + int err; >>> + >>> + ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE); >>> + err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE, >>> + CONTROLLER_ENABLE, CONTROLLER_DISABLE, >>> + 10, 1, can_sleep); >>> + if (err) >>> + dev_err(hba->dev, "%s: Controller disable failed\n", __func__); >>> +} >>> + >> Shouldn't you return an error here? >> If the controller disable failed you probably need a hard reset or >> something, otherwise I would assume that every other command from that >> point on will not work as expected. >> >> Cheers, >> >> Hannes > > > Hello Hannes, > The original routine signature is: > void ufshcd_hba_stop(struct ufs_hba *hba); > > as you can see, no return value, the reason is simple - there is nothing > we can do if writing to the register fails. > > all we wanted to do here, was to add a graceful time to change the > register value. also, we decided to add error msg in case the value is not > change within this timeout. > We can not do anything else, not to say, return error, as there is no > error handling in such case. > > So, as far as i see it, we only improved the already exists logic, by > adding some graceful time to the register change, and also, by adding an > error message that was absent before. > Thanks for the explanation. Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)