Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

[Randy Dunlap]

On 05/14/2018 01:00 PM, Gary R Hook wrote:
> On 05/14/2018 12:50 PM, Randy Dunlap wrote:
>> On 05/14/2018 10:20 AM, Gary R Hook wrote:
>>> Implement a skeleton framework for debugfs support in the
>>> AMD IOMMU.
>>>
>>> Signed-off-by: Gary R Hook <gary.hook@xxxxxxx>
>>> ---
>>> Â drivers/iommu/MakefileÂÂÂÂÂÂÂÂÂÂÂ |ÂÂÂ 5 +++++
>>> Â drivers/iommu/amd_iommu_debugfs.c |ÂÂ 39 +++++++++++++++++++++++++++++++++++++
>>> Â drivers/iommu/amd_iommu_init.cÂÂÂ |ÂÂÂ 6 ++++--
>>> Â drivers/iommu/amd_iommu_proto.hÂÂ |ÂÂÂ 6 ++++++
>>> Â drivers/iommu/amd_iommu_types.hÂÂ |ÂÂÂ 3 +++
>>> Â 5 files changed, 57 insertions(+), 2 deletions(-)
>>> Â create mode 100644 drivers/iommu/amd_iommu_debugfs.c
>>>
>>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>>> index 74cfbc392862..dd980f7dd8b6 100644
>>> --- a/drivers/iommu/Makefile
>>> +++ b/drivers/iommu/Makefile
>>> @@ -30,3 +30,8 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
>>> Â obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
>>> Â obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
>>> Â obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
>>> +
>>> +# This ensures that only the required files are compiled
>>> +ifeq ($(CONFIG_IOMMU_DEBUGFS), y)
>>
>> Most Makefiles don't use a space before the 'y', but since you tested it,
>> I guess either way works.
> 
> Pretty sure whitespace isn't used as a delimiter in this construct. I could be mistaken. But yes, it's perfectly serviceable.
> 
>> But why do this in the Makefile at all? Why not just add another Kconfig
>> symbol and simplify the Makefile?
>>
>>> +obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o
>>> +endif
> 
> 
> This was brought up a few weeks ago in, I believe, version 3 of this patch. That question was discussed (because that's what I did the first time out), and _someone_ _else_ asked about why I didn't just do it the way I've done it here.

Sorry I missed it.  I would have been glad to chime in at that time.

> Everyone has a preference.
> 
> I chose to simplify the choices and avoid multiple symbols, instead opting for two switches: choose your device, and decide on Debug FS enablement for it. IMO Very simple.
> 
> I can't fathom a scenario where this wouldn't work. Is there one?

Probably not.

But we used to have ugly, messy, convoluted makefiles with very little config help.
Then we got better kconfig tools (well, arguably :) and then the makefiles were
cleaned up and simplified a lot (or A LOT!).  We shouldn't want to go back there.

-- 
~Randy