Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Oct 2022 11:15:03 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Parav Pandit <parav@nvidia.com>
Cc:     Will Deacon <will@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        "bagasdotme@gmail.com" <bagasdotme@gmail.com>,
        Alan Stern <stern@rowland.harvard.edu>,
        "parri.andrea@gmail.com" <parri.andrea@gmail.com>,
        Peter Zijlstra <peterz@infradead.org>,
        "boqun.feng@gmail.com" <boqun.feng@gmail.com>,
        Nicholas Piggin <npiggin@gmail.com>,
        "dhowells@redhat.com" <dhowells@redhat.com>,
        "j.alglave@ucl.ac.uk" <j.alglave@ucl.ac.uk>,
        "luc.maranget@inria.fr" <luc.maranget@inria.fr>,
        Akira Yokosawa <akiyks@gmail.com>,
        Dan Lustig <dlustig@nvidia.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Jonathan Corbet <corbet@lwn.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Linux-Arch <linux-arch@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>
Subject: Re: [PATCH v4] locking/memory-barriers.txt: Improve documentation
 for writel() example
Message-ID: <20221027181503.GE5600@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20221010101331.29942-1-parav@nvidia.com>
 <d5faaf6f-7de5-49b0-92d6-9989ffbdbf2e@app.fastmail.com>
 <20221018100554.GA3112@willie-the-truck>
 <20221018174907.GT5600@paulmck-ThinkPad-P17-Gen-1>
 <PH0PR12MB54810CA260159E805D448375DC289@PH0PR12MB5481.namprd12.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PH0PR12MB54810CA260159E805D448375DC289@PH0PR12MB5481.namprd12.prod.outlook.com>
Precedence: bulk

On Tue, Oct 18, 2022 at 08:33:08PM +0000, Parav Pandit wrote:
> Hi Paul, Will,
> 
> > From: Paul E. McKenney <paulmck@kernel.org>
> > Sent: Tuesday, October 18, 2022 1:49 PM
> > 
> > On Tue, Oct 18, 2022 at 11:05:55AM +0100, Will Deacon wrote:
> > > On Mon, Oct 17, 2022 at 10:55:00PM +0200, Arnd Bergmann wrote:
> > > > On Mon, Oct 10, 2022, at 12:13 PM, Parav Pandit wrote:
> > > > > The cited commit describes that when using writel(), explcit wmb()
> > > > > is not needed. wmb() is an expensive barrier. writel() uses the
> > > > > needed platform specific barrier instead of expensive wmb().
> > > > >
> > > > > Hence update the example to be more accurate that matches the
> > > > > current implementation.
> > > > >
> > > > > commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA
> > vs.
> > > > > MMIO ordering example")
> > > > >
> > > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > >
> > > > I have no objections, though I still don't see a real need to change
> > > > the wording here.
> > >
> > > FWIW, I also don't think this change is necessary. If anything, I'd
> > > say we'd be better off _removing_ the text about writel from this
> > > section and extending the reference to the "KERNEL I/O BARRIER
> > > EFFECTS" section, as you could make similar comments about e.g.
> > > readb() and subsequent barriers.
> > >
> > > For example, something like the diff below.
> > 
> > I do like this change, but we might be dealing with two different groups of
> > readers.  Will and Arnd implemented significant parts of the current
> > MMIO/DMA ordering infrastructure.  It is thus quite possible that wording
> > which suffices to remind them of how things work might or might not help
> > someone new to Linux who is trying to figure out what is required to make
> > their driver work.
> > 
> > The traditional resolution of this sort of thing is to provide the
> > documentation to a newbie and take any resulting confusion seriously.
> > 
> > Parav, thoughts?
> 
> I am ok with the change from Will that removes the writel() description.
> However, it removes useful short description from the example of "why" writel() is used.
> This is useful for newbie and experienced developers both.
> 
> So how about below additional change on top of Will's change?
> This also aligns to rest of the short C comments in this example pseudo code.
> 
> If ok, I will take Will's and mine below change to v5.
> 
> index 4d24d39f5e42..5939c5e09570 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1919,7 +1919,9 @@ There are some more advanced barrier functions:
>                 /* assign ownership */
>                 desc->status = DEVICE_OWN;
> 
> -               /* notify device of new descriptors */
> +               /* Make descriptor status visible to the device followed by
> +                * notify device of new descriptors
> +                */
>                 writel(DESC_NOTIFY, doorbell);

Hearing no objections, please proceed.

							Thanx, Paul