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 07:51:39 +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: [PATCHv11 6/6] asm-generic/io: Add logging support for MMIO
 accessors
Message-ID: <YmorayBozWWRlTpP@kroah.com>
References: <cover.1645772606.git.quic_saipraka@quicinc.com>
 <3de35c9f4a3a070d197bab499acefc709a6f5336.1645772606.git.quic_saipraka@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3de35c9f4a3a070d197bab499acefc709a6f5336.1645772606.git.quic_saipraka@quicinc.com>
Precedence: bulk

On Thu, Apr 28, 2022 at 09:00:13AM +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. Also add a generic flag
> (__DISABLE_TRACE_MMIO__) which is used to disable MMIO tracing in nVHE KVM
> and if required can be used to disable MMIO tracing for specific drivers.

This "add a build flag to a Makefile to change how a driver operates"
feels very wrong to me given that this is now a totally new way to
control how a driver works at build time.  That's not anything we have
done before for drivers and if added, is only going to create much added
complexity.

How about requiring that the #define be in the .c files, and not in the
Makefile, as Makefile changes are much much harder to notice and review
over time.

Also, I see that this "disable the trace" feature has already been asked
for for 2 other drivers in the Android kernel tree, why not include
those changes here as well?  That kind of shows that this new feature is
limited in that driver authors are already wanting it disabled, even
before it is accepted.

Because of that, who _will_ be using this feature?

thanks,

greg k-h