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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux