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

[Greg KH <greg@kroah.com>]

On Tue, Jun 05, 2018 at 11:58:13AM -0500, Gary R Hook wrote:
> On 05/29/2018 01:39 PM, Greg KH wrote:
> > On Tue, May 29, 2018 at 01:23:23PM -0500, Gary R Hook wrote:
> > > Implement a skeleton framework for debugfs support in the
> > > AMD IOMMU. Add a hidden boolean to Kconfig that is defined
> > > for the AMD IOMMU when general IOMMY DebugFS support is
> > > enabled.
> > > 
> > > Signed-off-by: Gary R Hook <gary.hook@amd.com>
> > > ---
> > >   drivers/iommu/Kconfig             |    4 ++++
> > >   drivers/iommu/Makefile            |    1 +
> > >   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   |    5 +++++
> > >   6 files changed, 59 insertions(+), 2 deletions(-)
> > >   create mode 100644 drivers/iommu/amd_iommu_debugfs.c
> > > 
> > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > index f9af25ac409f..ec223f6f4ad4 100644
> > > --- a/drivers/iommu/Kconfig
> > > +++ b/drivers/iommu/Kconfig
> > > @@ -137,6 +137,10 @@ config AMD_IOMMU
> > >   	  your BIOS for an option to enable it or if you have an IVRS ACPI
> > >   	  table.
> > > +config AMD_IOMMU_DEBUGFS
> > > +	def_bool y
> > 
> > Why default y?  Can you not boot a box without this?  If not, it should
> > not be Y.
> 
> Again, apologies for not seeing this sooner.
> 
> Yes, the system can boot without this. The idea of a hidden option was
> surfaced by Robin, and after my first approach was shot down, I tried this.
> 
> Logic: If the over-arching IOMMU debugfs option is enabled, then
> AMD_IOMMU_DEBUGFS gets defined, and AMD IOMMU code gets included.
> 
> This issue was discussed a few weeks ago. No single approach appears to
> satisfy everyone. I like this because it depends upon one switch: Do you
> want DebugFS support enabled in the IOMMU driver, period? Vendor-specific
> code can then choose to implement support or not, and a builder doesn't have
> to worry about enabling/disabling multiple Kconfig options.
> 
> At least, that was my line of reasoning.
> 
> I'm not married to any approach, and I don't find clever use of Kconfig
> options too terribly challenging. And I'm not defending, I'm just
> explaining.

The issue is, no one sets Kconfig options except a very tiny subset of
kernel developers.  Distros allways enable everything, as they have to
do that.

If you are creating something here that is so dangerous that you spam
the kernel log with big warning messages, you should not be making it
easy to enable, let alone be enabled by default :)

Just make it an option, have it rely on the kernel debugging option, and
say "DO NOT ENABLE THIS UNLESS YOU REALLY REALLY REALLY KNOW WHAT YOU
ARE DOING!"

thanks,

greg k-h