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, 28 Apr 2022 14:02:41 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Sai Prakash Ranjan <quic_saipraka@quicinc.com>
Cc:     arnd@arndb.de, catalin.marinas@arm.com, rostedt@goodmis.org,
        linux-arm-kernel@lists.infradead.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        maz@kernel.org, quic_psodagud@quicinc.com, quic_tsoni@quicinc.com,
        will@kernel.org
Subject: Re: [PATCHv12 7/9] asm-generic/io: Add logging support for MMIO
 accessors
Message-ID: <YmqCYZwHoRuKtFSN@kroah.com>
References: <cover.1651139070.git.quic_saipraka@quicinc.com>
 <6673a2e73d3dd4c7aa01fee9b26cc4a52176ba7a.1651139070.git.quic_saipraka@quicinc.com>
 <YmpxxW5CZjMVrzF0@kroah.com>
 <6688ffa2-ec14-9126-1296-6180bab3e1d6@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6688ffa2-ec14-9126-1296-6180bab3e1d6@quicinc.com>
Precedence: bulk

On Thu, Apr 28, 2022 at 04:51:49PM +0530, Sai Prakash Ranjan wrote:
> On 4/28/2022 4:21 PM, Greg KH wrote:
> > On Thu, Apr 28, 2022 at 03:25:30PM +0530, Sai Prakash Ranjan wrote:
> > > Add logging support for MMIO high level accessors such as read{b,w,l,q}
> > > and their relaxed versions to aid in debugging unexpected crashes/hangs
> > > caused by the corresponding MMIO operation.
> > > 
> > > Signed-off-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>
> > > ---
> > >   include/asm-generic/io.h | 82 ++++++++++++++++++++++++++++++++++++++--
> > >   1 file changed, 78 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > > index 7ce93aaf69f8..99090722cb4b 100644
> > > --- a/include/asm-generic/io.h
> > > +++ b/include/asm-generic/io.h
> > > @@ -10,6 +10,7 @@
> > >   #include <asm/page.h> /* I/O is all done through memory accesses */
> > >   #include <linux/string.h> /* for memset() and memcpy() */
> > >   #include <linux/types.h>
> > > +#include <linux/instruction_pointer.h>
> > >   #ifdef CONFIG_GENERIC_IOMAP
> > >   #include <asm-generic/iomap.h>
> > > @@ -61,6 +62,35 @@
> > >   #define __io_par(v)     __io_ar(v)
> > >   #endif
> > > +#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) && !(defined(__DISABLE_TRACE_MMIO__))
> > Shouldn't you document __DISABLE_TRACE_MMIO__ somewhere?  It's not even
> > in the changelog.  Put a big comment above this for what is is for and
> > how to use it.  Otherwise you will forget all about this in 6 months :)
> > 
> > thanks,
> > 
> > greg k-h
> 
> Didn't you ask me to split the patch to the one actually adding the flag and the one using it.

Yes, and isn't this the commit that adds the flag?  Or was that on an
earlier one that I missed?

Ah, it's in patch 6/9

But you should also document it here in the .h file, otherwise the only
place it is described is in some random kvm Makefile that no one will
ever notice :)

thanks,

greg k-h