From mboxrd@z Thu Jan 1 00:00:00 1970 From: Srinivas Kandagatla Subject: Re: [PATCH v4 1/2] phy: qcom: Add driver for QCOM APQ8064 SATA PHY Date: Wed, 16 Jul 2014 07:28:16 +0100 Message-ID: <53C61B80.5070600@linaro.org> References: <1405336651-32135-1-git-send-email-srinivas.kandagatla@linaro.org> <1405336679-32179-1-git-send-email-srinivas.kandagatla@linaro.org> <12555066.QsMd3v2SOo@amdc1032> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <12555066.QsMd3v2SOo@amdc1032> Sender: linux-ide-owner@vger.kernel.org To: Bartlomiej Zolnierkiewicz Cc: Kishon Vijay Abraham I , Grant Likely , Rob Herring , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-ide@vger.kernel.org, Sujit Reddy Thumma List-Id: linux-arm-msm@vger.kernel.org On 15/07/14 17:56, Bartlomiej Zolnierkiewicz wrote: >> + >> >+/* Helper function to do poll and timeout */ >> >+static int read_poll_timeout(void __iomem *addr, u32 mask) >> >+{ >> >+ unsigned long timeout = jiffies + msecs_to_jiffies(TIMEOUT_MS); >> >+ >> >+ do { >> >+ if (readl_relaxed(addr) & mask) >> >+ return 0; >> >+ >> >+ usleep_range(DELAY_INTERVAL_US, DELAY_INTERVAL_US + 50); >> >+ } while (!time_after(jiffies, timeout)); >> >+ >> >+ return -ETIMEDOUT; >> >+} > Thanks for reworking this code, unfortunately it still has a one > (unlikely but still theoretically possible) problem. If there is > i.e. a big IRQ load between first usleep_range() call and first Very unlikely but as you said it possible in theory :-) > time_after() check the function will timeout without checking > the register. To fix it you needs to add an additonal register > checking before returning -ETIMEDOUT value or replace time_after() > condition with a fixed number of retries (100000 to cover 1sec > timeout). I will send out a fix on top of my previous patches to fix this. thanks, srini