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

[Sai Prakash Ranjan <quic_saipraka@quicinc.com>]

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