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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <938d5a51-0aa8-0d28-84e3-ce007e73c173@infradead.org>
Date:   Mon, 14 May 2018 14:00:02 -0700
From:   Randy Dunlap <rdunlap@...radead.org>
To:     Gary R Hook <gary.hook@....com>, iommu@...ts.linux-foundation.org
Cc:     joro@...tes.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/2] iommu/amd: Add basic debugfs infrastructure for
 AMD IOMMU

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@....com>
>>> ---
>>>   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