From patchwork Wed Dec 27 02:21:54 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Boyd X-Patchwork-Id: 10133329 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B1CE66037D for ; Wed, 27 Dec 2017 02:21:57 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9D7782DAD1 for ; Wed, 27 Dec 2017 02:21:57 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8F64E2DB9B; Wed, 27 Dec 2017 02:21:57 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 12F322DAD1 for ; Wed, 27 Dec 2017 02:21:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751211AbdL0CV4 (ORCPT ); Tue, 26 Dec 2017 21:21:56 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:45834 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751163AbdL0CVz (ORCPT ); Tue, 26 Dec 2017 21:21:55 -0500 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 65FB96081B; Wed, 27 Dec 2017 02:21:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1514341315; bh=JYLHFZ8QI+AfQ8BjSeL5sRIvx4i0SDWuQM6ByKuA63o=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CpGlM8oD2DPbC5kHQcf4S0LTG1LsNThXy8eWCLh0BtFktoraYud4bOFkpzHV4suMh UBJYLlpRwQsbRr3yuMhbXKOBFZLgz1QE4yBnwPgw4oUmAkGFKPZCNnWg11XNX2f4Vs rBJyxD88OVUotLCPuKrfE8k7+qcX7Y6FuMCXDEhY= Received: from localhost (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: sboyd@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id D31A46071B; Wed, 27 Dec 2017 02:21:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1514341314; bh=JYLHFZ8QI+AfQ8BjSeL5sRIvx4i0SDWuQM6ByKuA63o=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gm+fmLUpg3u9gj57tiuO/ZrXwiQ/yxrLjYJkbM0SruKAiZPe6Bwm0Nzf+y1pYd0+a RXUrZm7Ksc1WOS/jdtxnjOUt7HWVqU9vrnmP3PCyT4JZuU0tQ5iMYuEVNb7TcPXTc4 S2lz94GYuRXMYax8bLEQQX0JODE3uduANv1wqGxE= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org D31A46071B Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sboyd@codeaurora.org Date: Tue, 26 Dec 2017 18:21:54 -0800 From: Stephen Boyd To: David Lechner Cc: linux-clk@vger.kernel.org, Michael Turquette , linux-kernel@vger.kernel.org Subject: Re: [PATCH] clk: fix reentrancy of clk_enable() on UP systems Message-ID: <20171227022154.GA7997@codeaurora.org> References: <1514309062-1768-1-git-send-email-david@lechnology.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1514309062-1768-1-git-send-email-david@lechnology.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-clk-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-clk@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 12/26, David Lechner wrote: > Reentrant calls to clk_enable() are not working on UP systems. This is > caused by the fact spin_trylock_irqsave() always returns true when > CONFIG_SMP=n (and CONFIG_DEBUG_SPINLOCK=n) which causes the reference > counting to not work correctly when clk_enable_lock() is called twice > before clk_enable_unlock() is called (this happens when clk_enable() > is called from within another clk_enable()). > > This introduces a new set of clk_enable_lock() and clk_enable_unlock() > functions for UP systems that doesn't use spinlocks but effectively does > the same thing as the SMP version of the functions. > > Signed-off-by: David Lechner > --- > > Previous discussion of this issue for reference: > * https://patchwork.kernel.org/patch/10108437/ > * https://patchwork.kernel.org/patch/10115483/ > > > drivers/clk/clk.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index bb1b1f9..259a77f 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -136,6 +136,8 @@ static void clk_prepare_unlock(void) > mutex_unlock(&prepare_lock); > } > > +#ifdef CONFIG_SMP > + > static unsigned long clk_enable_lock(void) > __acquires(enable_lock) > { > @@ -170,6 +172,43 @@ static void clk_enable_unlock(unsigned long flags) > spin_unlock_irqrestore(&enable_lock, flags); > } > > +#else > + > +static unsigned long clk_enable_lock(void) > + __acquires(enable_lock) > +{ > + unsigned long flags; > + > + local_irq_save(flags); > + preempt_disable(); Well we certainly don't want to do both irq save and preemption disabling. Really all we need to do is save away the current flags to restore them them later. On UP systems, we would do that on each call to this function. On SMP systems, we would actually test the enable_owner and up the enable_refcnt outside of the locked section because if spin_trylock_irqsave() returns a 0, we have restored the irq flags to whatever they were before, and then we test enable_owner. So I suppose if it was safe on SMP to test enable_owner and up the refcnt without holding any sort of lock then it's also safe on UP, because current is a "special" variable. Does this ugly hack do the trick? I really dislike the semi-colon barf, but I fail to see another way out of this right now. diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index b56c11f51baf..77b2c5bbba68 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -141,7 +141,8 @@ static unsigned long clk_enable_lock(void) { unsigned long flags; - if (!spin_trylock_irqsave(&enable_lock, flags)) { + if ((!IS_ENABLED(CONFIG_SMP) && ({local_save_flags(flags); 1;})) || + !spin_trylock_irqsave(&enable_lock, flags)) { if (enable_owner == current) { enable_refcnt++; __acquire(enable_lock);