From patchwork Tue Dec 3 23:38:23 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Leigh Brown X-Patchwork-Id: 3279381 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 79821C0D4A for ; Tue, 3 Dec 2013 23:32:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9D9CE2046F for ; Tue, 3 Dec 2013 23:32:29 +0000 (UTC) Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id ABCD12021B for ; Tue, 3 Dec 2013 23:32:28 +0000 (UTC) Received: from merlin.infradead.org ([2001:4978:20e::2]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VnzS5-0001rU-AO; Tue, 03 Dec 2013 23:32:25 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1VnzS2-0004rX-Qd; Tue, 03 Dec 2013 23:32:22 +0000 Received: from doppler.thel33t.co.uk ([193.110.88.198]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VnzS0-0004qD-M2 for linux-arm-kernel@lists.infradead.org; Tue, 03 Dec 2013 23:32:21 +0000 Received: from doppler.thel33t.co.uk (localhost [127.0.0.1]) by doppler.thel33t.co.uk (Postfix) with ESMTP id BB6D8840B4; Tue, 3 Dec 2013 23:38:23 +0000 (GMT) MIME-Version: 1.0 Date: Tue, 03 Dec 2013 23:38:23 +0000 From: Leigh Brown To: Sebastian Hesselbarth Subject: Re: Spurious timeouts in mvmdio In-Reply-To: <529E66A4.4050000@gmail.com> References: <529CA42A.3040504@freebox.fr> <20131203122346.GD29282@titan.lakedaemon.net> <20131203124033.GT16735@n2100.arm.linux.org.uk> <529E5F03.8040607@gmail.com> <8d65780eb7bfe49bb0734a09f05f70a6@doppler.thel33t.co.uk> <529E66A4.4050000@gmail.com> Message-ID: X-Sender: leigh@solinno.co.uk User-Agent: Roundcube Webmail/0.6 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20131203_183220_947364_63402C62 X-CRM114-Status: GOOD ( 18.97 ) X-Spam-Score: -1.9 (-) Cc: Russell King - ARM Linux , Jason Cooper , Nicolas Schichan , netdev@vger.kernel.org, LKML , "David S. Miller" , Florian Fainelli , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 2013-12-03 23:17, Sebastian Hesselbarth wrote: > On 12/04/2013 12:20 AM, Leigh Brown wrote: >> On 2013-12-03 22:45, Sebastian Hesselbarth wrote: >>> On 12/03/2013 09:57 PM, Leigh Brown wrote: >> [...] >>>> Nicolas' patch should fix the issue, but I prefer the following as >>>> it is >>>> more >>>> correct, as it only adjusts the timeout when calling >>>> wait_event_timeout(). As >>>> I said above,I believe the polling code is correct. >>>> >>>> diff --git a/drivers/net/ethernet/marvell/mvmdio.c >>>> b/drivers/net/ethernet/marvell/mvmdio.c >>>> index 7354960..b187c08 100644 >>>> --- a/drivers/net/ethernet/marvell/mvmdio.c >>>> +++ b/drivers/net/ethernet/marvell/mvmdio.c >>>> @@ -92,6 +92,14 @@ static int orion_mdio_wait_ready(struct mii_bus >>>> *bus) >>>> if (time_is_before_jiffies(end)) >>>> ++timedout; >>>> } else { >>>> + /* >>>> + * wait_event_timeout does not guarantee a delay of at >>>> + * least one whole jiffie, so timeout must be no less >>>> + * than two. >>>> + */ >>>> + if (timeout < 2) >>>> + timeout = 2; >>> >>> If you always want to wait at least two jiffies, why not just >>> increase >>> TIMEOUT makro to 20ms instead of messing here with it again? >>> As said on IRC log above, originally timeout was 100ms. >> >> You could do that, but would you not feel bad leaving a latent bug in >> the code? >> I know it's unlikely that someone would set HZ to 50, but if they did, >> the same >> bug would appear again. > > If you want to ensure timeout > 2, why not then just use: > > - unsigned long timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT); > + unsigned long timeout = 1 + usecs_to_jiffies(MVMDIO_SMI_TIMEOUT); This will make it correct when using interrupts but it will make the loop wait one jiffie longer than it should when polling. I think I might be being a little to anal about this, but I guess what you could do is make the above change, then replace the call to time_is_before_jiffies() with time_is_before_eq_jiffies() so that the behaviour when polling and when using interrupts is the same. @@ -89,7 +89,7 @@ static int orion_mdio_wait_ready(struct mii_bus *bus) usleep_range(MVMDIO_SMI_POLL_INTERVAL_MIN, MVMDIO_SMI_POLL_INTERVAL_MAX); - if (time_is_before_jiffies(end)) + if (time_is_before_eq_jiffies(end)) ++timedout; } else { wait_event_timeout(dev->smi_busy_wait, Regards, Leigh. diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c index 7354960..48b8ded 100644 --- a/drivers/net/ethernet/marvell/mvmdio.c +++ b/drivers/net/ethernet/marvell/mvmdio.c @@ -75,7 +75,7 @@ static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev) static int orion_mdio_wait_ready(struct mii_bus *bus) { struct orion_mdio_dev *dev = bus->priv; - unsigned long timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT); + unsigned long timeout = 1 + usecs_to_jiffies(MVMDIO_SMI_TIMEOUT); unsigned long end = jiffies + timeout; int timedout = 0;