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

[Sai Prakash Ranjan]

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