On 4/28/2022 1:05 PM, Greg KH wrote:
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.
Ok :)
-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.
Sure, will make the change as you suggested.
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.
Ah ok, you mean like that. Sure I don't have any strong opinion, so will move it
to the driver file.
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.
Ok, for kernel drivers I will make the define at the top of the .c driver file and include
those 2 driver changes in the series.
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
Thanks,
Sai
