Re: [PATCHv11 6/6] asm-generic/io: Add logging support for MMIO accessors

[Greg KH]

On Thu, Apr 28, 2022 at 12:59:13PM +0530, Sai Prakash Ranjan wrote:
> On 4/28/2022 11:21 AM, Greg KH wrote:
> > 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.
> 
> Not exactly, there are already many such build flags being used currently across kernel.
> 
> Example: "-D__KVM_NVHE_HYPERVISOR__, D__KVM_VHE_HYPERVISOR__,

That's crazy KVM stuff, don't extrapoloate that to all kernel drivers
please.

> -D__NO_FORTIFY, -D__DISABLE_EXPORTS, -DDISABLE_BRANCH_PROFILING".

Those are compiler flags that affect gcc, not kernel code functionality.

> It gives us even more flexibility to disable feature for multiple files under a directory
> rather than individually cluttering each file, look at "-D__KVM_NVHE_HYPERVISOR__"
> for files under "arch/arm64/kvm/hyp/nvhe/".

Again, crazy KVM stuff, do not want that for all drivers in the kernel.

> > 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.
> 
> How is this cleaner, lets say we have many such drivers like all drivers under drivers/serial,
> so we go and add #define for each of them? That looks more spread out than having all
> such information under one file (Makefile).

If you have to go and add this to each and every driver, that is a HUGE
hint that this feature is not a good one and that no one should be using
it in the first place, right?

Again, this is a new way to modify driver functionality that is outside
of the driver itself, which is not something that I would like to see
added without a whole lot of discussion and planning.  To throw it in as
part of a kvm change is not a nice way to hide such a thing.

> And I didn't understand how is it harder to track changes to makefile? Makefile is  part
> of the driver directory and any changes to makefile is visible to the corresponding maintainers.
> Do you mean something else?

I mean that we review driver changes all the time, and code is
self-contained and the functionality is only affected right now by
Kconfig options, and what is in the .c files itself.  You are adding a
new way to change functionality by adding a Makefile configuration
option as well.  That is a major functional change for how we do our
configuration logic in Linux.

> > 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.
> 
> That can be done later on top of this series right? This series mainly deals with adding
> initial support for such tracing, there could be numerous drivers who might or might
> not want the feature which can be added onto later. We can't actually identify all
> the driver requirements upfront. As an example, we have already used the flag to
> disable tracing for nVHE KVM, so we know how to use the flag.

Again, make it explicit in the driver file itself that it is doing this,
not in the Makefile, and I will not have any objections.

> > Because of that, who _will_ be using this feature?
> > 
> 
> Every driver except those two or few more, and it is not a bug or anything, they just want to disable it
> to limit the logs in case of example UART driver since the reads/writes are very frequent in those cases
> and the logs are not necessarily useful for them.

I have a feeling that lots more drivers will want this disabled due to
the noise it will cause.  The fact that we already have 2 requests to
change this _before_ the code is even merged is proof of that.

thanks,

greg k-h