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

[Sai Prakash Ranjan]

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__,
-D__NO_FORTIFY, -D__DISABLE_EXPORTS, -DDISABLE_BRANCH_PROFILING".

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/".

> 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).

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?

> 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.

> 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.

Thanks,
Sai