Re: [PATCH v3 1/2] iommu - Enable debugfs exposure of the IOMMU

[Robin Murphy]

On 17/04/18 18:46, Hook, Gary wrote:
> On 4/6/2018 9:17 AM, Gary R Hook wrote:
> > Provide base enablement for using debugfs to expose internal data of
> > an IOMMU driver. When called, create the /sys/kernel/debug/iommu
> > directory. Emit a strong warning at boot time to indicate that this
> > feature is enabled.
> >
> > This patch adds a top-level function that will create the (above)
> > directory, under which a driver may create a hw-specific directory for
> > its use. The function
> >
> > ÂÂÂÂiommu_debugfs_setup()
> >
> > returns a pointer to the new dentry structure created for
> > /sys/kernel/debug/iommu, or NULL in the event of a failure. An IOMMU
> > driver should call this function first, and then create a directory
> > beneath it. A driver implementation might look something like:
> >
> > static struct dentry *my_debugfs;
> >
> > ÂÂÂÂstruct dentry *d_top;
> > ÂÂÂÂif (!my_debugfs) {
> > ÂÂÂÂÂÂÂ d_top = iommu_debugfs_setup();
> > ÂÂÂÂÂÂÂ if (d_top)
> > ÂÂÂÂÂÂÂÂÂÂÂ my_debugfs = debugfs_create_dir("vendor", d_top);
> > ÂÂÂÂ}
> >
> > Since the IOMMU driver can not be removed from the running system, this
> > patch only provides an "on" function.
> >
> > Signed-off-by: Gary R Hook <gary.hook@xxxxxxx>
> > ---
> > Â drivers/iommu/KconfigÂÂÂÂÂÂÂÂ |ÂÂ 11 ++++++++
> > Â drivers/iommu/MakefileÂÂÂÂÂÂÂ |ÂÂÂ 1 +
> > Â drivers/iommu/iommu-debugfs.c |ÂÂ 58 
> > +++++++++++++++++++++++++++++++++++++++++
> > Â include/linux/iommu.hÂÂÂÂÂÂÂÂ |ÂÂÂ 4 +++
> > Â 4 files changed, 74 insertions(+)
> > Â create mode 100644 drivers/iommu/iommu-debugfs.c
> >
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index f3a21343e636..c1e39dabfec2 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -60,6 +60,17 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
> > Â endmenu
> > +config IOMMU_DEBUG
> > +ÂÂÂ bool "Enable IOMMU internals in DebugFS"
> > +ÂÂÂ depends on DEBUG_FS
> > +ÂÂÂ default n
> > +ÂÂÂ help
> > +ÂÂÂÂÂ Allows exposure of IOMMU device internals. This option enables
> > +ÂÂÂÂÂ the use of debugfs by IOMMU drivers as required. Devices can,
> > +ÂÂÂÂÂ at initialization time, cause the IOMMU code to create a top-level
> > +ÂÂÂÂÂ debug/iommu directory, and then populate a subdirectory with
> > +ÂÂÂÂÂ entries as required.
>
> I should explicitly ask about this:
>
> Joerg had suggested IOMMU_DEBUGFS, but here I've changed to IOMMU_DEBUG. 
> I'm not seeing a lot of CONFIG options that use DEBUGFS for debugfs 
> options, so I chose to follow an implied convention.
>
> Question: should this indeed be IOMMU_DEBUGFS?
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ^^^^^^^^^^^^^

Personally I'd say yes, since there is at least some precedent for 
*_DEBUGFS already, and it does help make the intent that much clearer 
(*_DEBUG often just means lots of dmesg spam rather than an actual 
debugfs interface).

Robin.