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

["Yang, Shunyong" <shunyong.yang@hxt-semitech.com>]

Hi, Sohil

On Wed, 2018-04-18 at 07:27 +0000, Mehta, Sohil wrote:
> On Wed, 2018-04-18 at 05:58 +0000, Yang, Shunyong wrote:
> > 
> > Hi, Gary and Sohil,
> > 
> > On Tue, 2018-04-17 at 13:38 -0400, Hook, Gary wrote:
> > > 
> > > On 4/13/2018 8:08 PM, Mehta, Sohil wrote:
> > > > 
> > > >  
> > > > On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote:
> > > > > 
> > > > >  
> > > > >   
> > > > > +
> > > > > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
> > > > > +{
> > > > > + char name[MAX_NAME_LEN + 1];
> > > > > + struct dentry *d_top;
> > > > > +
> > > > > + if (!debugfs_initialized())
> > > > Probably not needed.
> > > Right.
> > When will this check is needed?
> > IMO, this function is to check debugfs ready status before we want
> > to
> > use debugfs. I just want to understand when we should use
> > debugfs_initialized();
> > 
> You are right debugfs_initialized() can be used to check if debugfs
> is
> ready. However in this case we can also rely on debugfs_create_dir()
> which is called in iommu_debufs_setup().
> 
> debugfs_create_dir() says:
> 
>  * If debugfs is not enabled in the kernel, the value -%ENODEV will
> be
>  * returned.

It seems "If debugfs is not enabled in the kernel"
means CONFIG_DEBUG_FS is not configured. Following is the code of no
such config.

  static inline struct dentry *debugfs_create_dir(const char *name,
						struct dentry *parent)
  {
	return ERR_PTR(-ENODEV);
  }

Looking into the code, debugfs_initialized() return the value of
debugfs_registered. debugfs_registered is set to true after
debugfs_init() has been called.
However, debugfs_create_dir() doesn't call debugfs_initialized() or
check debugfs_registered value.
So, there is tiny different of checking status by debugfs_create_dir()
and debugfs_initialized(). Although it can achieve functionality here.

Maybe the original design is to call debugfs_initialized() before
calling debugfs_create_xxx()?

Thanks.
Shunyong.

> 
> Sohil
> 
> > 
> > Thanks.
> > Shunyong.
> > 
> > > 
> > >  
> > > > 
> > > >  
> > > >  
> > > > > 
> > > > >  
> > > > > +         return;
> > > > > +
> > > > > + mutex_lock(&amd_iommu_debugfs_lock);
> > > > > + if (!amd_iommu_debugfs) {
> > > > > +         d_top = iommu_debugfs_setup();
> > > > > +         if (d_top)
> > > > > +                 amd_iommu_debugfs =
> > > > > debugfs_create_dir("amd", d_top);
> > > > > + }
> > > > > + mutex_unlock(&amd_iommu_debugfs_lock);