[PATCH 1/2] debugfs: Allow debugfs_create_dir() to take data

[PATCH 1/2] debugfs: Allow debugfs_create_dir() to take data

[Hiroshi Doyu]

Add __debugfs_create_dir(), which takes data passed from caller.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 fs/debugfs/inode.c      |    7 ++++---
 include/linux/debugfs.h |    9 ++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 4733eab..423df9f 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -387,7 +387,7 @@ struct dentry *debugfs_create_file(const char *name, umode_t mode,
 EXPORT_SYMBOL_GPL(debugfs_create_file);
 
 /**
- * debugfs_create_dir - create a directory in the debugfs filesystem
+ * __debugfs_create_dir - create a directory in the debugfs filesystem
  * @name: a pointer to a string containing the name of the directory to
  *        create.
  * @parent: a pointer to the parent dentry for this file.  This should be a
@@ -404,10 +404,11 @@ EXPORT_SYMBOL_GPL(debugfs_create_file);
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
  * returned.
  */
-struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
+struct dentry *__debugfs_create_dir(const char *name, struct dentry *parent,
+				    void *data)
 {
 	return __create_file(name, S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
-				   parent, NULL, NULL);
+				   parent, data, NULL);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_dir);
 
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 66c434f..4ba9f0a 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -50,7 +50,14 @@ struct dentry *debugfs_create_file(const char *name, umode_t mode,
 				   struct dentry *parent, void *data,
 				   const struct file_operations *fops);
 
-struct dentry *debugfs_create_dir(const char *name, struct dentry *parent);
+struct dentry *__debugfs_create_dir(const char *name, struct dentry *parent,
+				    void *data);
+
+static inline struct dentry *debugfs_create_dir(const char *name,
+						struct dentry *parent)
+{
+	return __debugfs_create_dir(name, parent, NULL);
+}
 
 struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 				      const char *dest);
-- 
1.7.5.4

Re: [PATCH 1/2] debugfs: Allow debugfs_create_dir() to take data

[Greg Kroah-Hartman]

On Wed, Aug 08, 2012 at 09:24:32AM +0300, Hiroshi Doyu wrote:
> Add __debugfs_create_dir(), which takes data passed from caller.

Why?

> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
>  fs/debugfs/inode.c      |    7 ++++---
>  include/linux/debugfs.h |    9 ++++++++-
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 4733eab..423df9f 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -387,7 +387,7 @@ struct dentry *debugfs_create_file(const char *name, umode_t mode,
>  EXPORT_SYMBOL_GPL(debugfs_create_file);
>  
>  /**
> - * debugfs_create_dir - create a directory in the debugfs filesystem
> + * __debugfs_create_dir - create a directory in the debugfs filesystem
>   * @name: a pointer to a string containing the name of the directory to
>   *        create.
>   * @parent: a pointer to the parent dentry for this file.  This should be a
> @@ -404,10 +404,11 @@ EXPORT_SYMBOL_GPL(debugfs_create_file);
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
>   */
> -struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
> +struct dentry *__debugfs_create_dir(const char *name, struct dentry *parent,
> +				    void *data)
>  {
>  	return __create_file(name, S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
> -				   parent, NULL, NULL);
> +				   parent, data, NULL);
>  }
>  EXPORT_SYMBOL_GPL(debugfs_create_dir);

You can't export a symbol that doesn't exist anymore.

What are you trying to do here?  This patch doesn't look right at all.

greg k-h

Re: [PATCH 1/2] debugfs: Allow debugfs_create_dir() to take data

[Hiroshi Doyu]

Hi Greg, Felipe,

On Wed, 8 Aug 2012 15:34:27 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Aug 08, 2012 at 09:24:32AM +0300, Hiroshi Doyu wrote:
> > Add __debugfs_create_dir(), which takes data passed from caller.
> 
> Why?
> 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> >  fs/debugfs/inode.c      |    7 ++++---
> >  include/linux/debugfs.h |    9 ++++++++-
> >  2 files changed, 12 insertions(+), 4 deletions(-)
.....
> What are you trying to do here?  This patch doesn't look right at all.

I missed to send the cover letter of this patch series to LKML, which
explained the background. I attached that cover letter below. Please
read the following explanation too.

The point was that, since directory hierarchy itself already has
the necessary info, like parent <- child relationships, among each
entities(here, smmu?, mc?, cache{tlb,ptc}), I just wanted to avoid to
have those *residual* info embedded in normal data hierarchy, because
cache{tlb,ptc} is used only in debugfs, not in the normal path. With
directory/file hierarchy, we could get necessary data at any time
on-the-fly.

OTOH, if debugfs has the assumption that it has to be always projected
from the existing data hierarchy, I should fix to have the same
hierarchy in entities(here, smmu?, mc?, cache{tlb,ptc}), as Felipe
pointed out in another thread.

It might not be so bad that debugfs has the ability to manage its own
hierarchy with i_private, apart from the original data hierarchy
too. In our case, cache{tlb,ptc} data is used only for debugfs. They
don't have to be in the normal data hierarchy. This depends on the
above assumption, does debugfs have to be always projected from the
normal data hierarchy?

The original tegra smmu debugfs patch is:
  http://lists.linuxfoundation.org/pipermail/iommu/2012-August/004507.html

Here's the original missed cover letter:

Subject:[RFC][PATCH 0/2] debugfs: Allow debugfs_create_dir() to take data from caller
From: Hiroshi Doyu <hdoyu@nvidia.com>
To: Hiroshi Doyu <hdoyu@nvidia.com>
CC: "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>,
	"linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>, Al Viro <viro@zeniv.linux.org.uk>
Date: Wed, 8 Aug 2012 08:24:31 +0200
Thread-Index: Ac11LoTHb+fK/MOMRX2RFTvtP89jeg==
Accept-Language: en-US, en-CA, fi-FI

The commit c3b1a35 "debugfs: make sure that debugfs_create_file() gets
used only for regulars" doesn't allow to use debugfs_create_file() for
creating directory, and the current debugfs_create_dir() cannot take
the private data from caller. There are some cases that we want to pass some
client data to dir, especially when dir is nested deeply. We can work
around to pass all necessary data with some invented data structure to
the end files, but if directory itself had private data, we could
avoid to introduce new structures just to pass data to end files.

For example, tegra iommu(smmu) case, debugfs structure could be as
below.

sys/
└── kernel
    └── debug
        ├── smmu.0
        │   ├── mc
        │   │   ├── ptc
        │   │   └── tlb
        │   └── mc0
        │       ├── ptc
        │       └── tlb
        └── smmu.1
            ├── mc
            │   ├── ptc
            │   └── tlb
            ├── mc0
            │   ├── ptc
            │   └── tlb
            └── mc1
                ├── ptc
                └── tlb

The end files, {ptc,tlb} depend on which mc? to belong to, and
which smmu.? to belong to. The parent data can be accessed from those
end files if necessary.

  dent = d_find_alias(s->private);
  cache = (int)dent->d_inode->i_private;
  mc = (int)dent->d_parent->d_inode->i_private;
  smmu = dent->d_parent->d_parent->d_inode->i_private;

The original tegra smmu debugfs patch is:
  http://lists.linuxfoundation.org/pipermail/iommu/2012-August/004507.html

Hiroshi Doyu (2):
  debugfs: Allow debugfs_create_dir() to take data
  iommu/tegra: smmu: Use __debugfs_create_dir

 drivers/iommu/tegra-smmu.c |    4 +---
 fs/debugfs/inode.c         |    7 ++++---
 include/linux/debugfs.h    |    9 ++++++++-
 3 files changed, 13 insertions(+), 7 deletions(-)

--
1.7.5.4

Re: [PATCH 1/2] debugfs: Allow debugfs_create_dir() to take data

[Hiroshi Doyu]

On Thu, 9 Aug 2012 14:56:24 +0200
Hiroshi Doyu <hdoyu@nvidia.com> wrote:

> Hi Greg, Felipe,
> 
> On Wed, 8 Aug 2012 15:34:27 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Aug 08, 2012 at 09:24:32AM +0300, Hiroshi Doyu wrote:
> > > Add __debugfs_create_dir(), which takes data passed from caller.
> > 
> > Why?
> > 
> > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > ---
> > >  fs/debugfs/inode.c      |    7 ++++---
> > >  include/linux/debugfs.h |    9 ++++++++-
> > >  2 files changed, 12 insertions(+), 4 deletions(-)
> .....
> > What are you trying to do here?  This patch doesn't look right at all.
> 
> I missed to send the cover letter of this patch series to LKML, which
> explained the background. I attached that cover letter below. Please
> read the following explanation too.

Any chance to get some feedback on this?

I'm also sending another version of patch, which just uses
debgufs_create_dir() as Felipe suggested, in-reply-to this mail.

Re: [PATCH 1/2] debugfs: Allow debugfs_create_dir() to take data

[Greg Kroah-Hartman]

On Wed, Aug 15, 2012 at 08:40:08AM +0300, Hiroshi Doyu wrote:
> On Thu, 9 Aug 2012 14:56:24 +0200
> Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> 
> > Hi Greg, Felipe,
> > 
> > On Wed, 8 Aug 2012 15:34:27 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Wed, Aug 08, 2012 at 09:24:32AM +0300, Hiroshi Doyu wrote:
> > > > Add __debugfs_create_dir(), which takes data passed from caller.
> > > 
> > > Why?
> > > 
> > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > > ---
> > > >  fs/debugfs/inode.c      |    7 ++++---
> > > >  include/linux/debugfs.h |    9 ++++++++-
> > > >  2 files changed, 12 insertions(+), 4 deletions(-)
> > .....
> > > What are you trying to do here?  This patch doesn't look right at all.
> > 
> > I missed to send the cover letter of this patch series to LKML, which
> > explained the background. I attached that cover letter below. Please
> > read the following explanation too.
> 
> Any chance to get some feedback on this?

I still don't like it, and then there's the basic fact that I'm pretty
sure you broke the build with the patch, but who's noticing little
things like that :)

> I'm also sending another version of patch, which just uses
> debgufs_create_dir() as Felipe suggested, in-reply-to this mail.

I'd prefer not to change debugfs for this, so if you don't do that, I
don't object to your driver-specific patch.

greg k-h

Re: [v2 1/1] iommu/tegra: smmu: Use debugfs_create_dir for directory

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Wed, 15 Aug 2012 18:13:18 +0200:

> On 08/14/2012 11:47 PM, Hiroshi Doyu wrote:
> > The commit c3b1a35 "debugfs: make sure that debugfs_create_file() gets
> > used only for regulars" doesn't allow to use debugfs_create_file() for
> > dir. Keep debugfs data in smmu_device instead of directory's i_private.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> 
> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
> 
> Since this fixes a BUG during kernel boot, it'd be great if this could
> be applied ASAP unless there are significant issues with it.

Joerg, any chance to get this in?

Re: [v2 1/1] iommu/tegra: smmu: Use debugfs_create_dir for directory

[Stephen Warren]

On 09/04/2012 12:51 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren@wwwdotorg.org> wrote @ Wed, 15 Aug 2012 18:13:18 +0200:
> 
>> On 08/14/2012 11:47 PM, Hiroshi Doyu wrote:
>>> The commit c3b1a35 "debugfs: make sure that debugfs_create_file() gets
>>> used only for regulars" doesn't allow to use debugfs_create_file() for
>>> dir. Keep debugfs data in smmu_device instead of directory's i_private.
>>>
>>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
>>
>> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
>>
>> Since this fixes a BUG during kernel boot, it'd be great if this could
>> be applied ASAP unless there are significant issues with it.
> 
> Joerg, any chance to get this in?

Joerg, could you please apply this patch ASAP? Currently, Tegra won't
boot in linux-next without it. Alternatively, would you like me to take
the patch through the Tegra tree? Thanks.

Re: [v2 1/1] iommu/tegra: smmu: Use debugfs_create_dir for directory

[joerg.roedel]

On Mon, Sep 10, 2012 at 12:24:06PM -0600, Stephen Warren wrote:

> Joerg, could you please apply this patch ASAP? Currently, Tegra won't
> boot in linux-next without it. Alternatively, would you like me to take
> the patch through the Tegra tree? Thanks.

Sorry, I had some vacation. But I am back now and will apply the patch
ASAP.


	Joerg

[PATCH 2/2] iommu/tegra: smmu: Use __debugfs_create_dir

[Hiroshi Doyu]

The commit c3b1a35 "debugfs: make sure that debugfs_create_file() gets
used only for regulars" doesn't allow to use debugfs_create_file() for
dir. Use the version with "data", __debugfs_create_dir().

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
Reported-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/iommu/tegra-smmu.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 5e51fb7..41aff7a 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -1035,9 +1035,7 @@ static void smmu_debugfs_create(struct smmu_device *smmu)
 	int i;
 	struct dentry *root;
 
-	root = debugfs_create_file(dev_name(smmu->dev),
-				   S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
-				   NULL, smmu, NULL);
+	root = __debugfs_create_dir(dev_name(smmu->dev), NULL, smmu);
 	if (!root)
 		goto err_out;
 	smmu->debugfs_root = root;
-- 
1.7.5.4

Re: [PATCH 2/2] iommu/tegra: smmu: Use __debugfs_create_dir

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

Hi,

On Wed, Aug 08, 2012 at 09:24:33AM +0300, Hiroshi Doyu wrote:
> The commit c3b1a35 "debugfs: make sure that debugfs_create_file() gets
> used only for regulars" doesn't allow to use debugfs_create_file() for
> dir. Use the version with "data", __debugfs_create_dir().
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> Reported-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/iommu/tegra-smmu.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 5e51fb7..41aff7a 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -1035,9 +1035,7 @@ static void smmu_debugfs_create(struct smmu_device *smmu)
>  	int i;
>  	struct dentry *root;
>  
> -	root = debugfs_create_file(dev_name(smmu->dev),
> -				   S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
> -				   NULL, smmu, NULL);
> +	root = __debugfs_create_dir(dev_name(smmu->dev), NULL, smmu);

why would the directory need extra data ? Looking in mainline,
tegra-smmu.c doesn't have any debugfs support, where can I see the
patches adding debugfs to tegra-smmu ? It doesn't look correct that the
directory will have a data field.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] iommu/tegra: smmu: Use __debugfs_create_dir

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 7359 bytes --]

Hi,

On Wed, Aug 08, 2012 at 06:11:29PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Aug 08, 2012 at 09:24:33AM +0300, Hiroshi Doyu wrote:
> > The commit c3b1a35 "debugfs: make sure that debugfs_create_file() gets
> > used only for regulars" doesn't allow to use debugfs_create_file() for
> > dir. Use the version with "data", __debugfs_create_dir().
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > Reported-by: Laxman Dewangan <ldewangan@nvidia.com>
> > ---
> >  drivers/iommu/tegra-smmu.c |    4 +---
> >  1 files changed, 1 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index 5e51fb7..41aff7a 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -1035,9 +1035,7 @@ static void smmu_debugfs_create(struct smmu_device *smmu)
> >  	int i;
> >  	struct dentry *root;
> >  
> > -	root = debugfs_create_file(dev_name(smmu->dev),
> > -				   S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
> > -				   NULL, smmu, NULL);
> > +	root = __debugfs_create_dir(dev_name(smmu->dev), NULL, smmu);
> 
> why would the directory need extra data ? Looking in mainline,
> tegra-smmu.c doesn't have any debugfs support, where can I see the
> patches adding debugfs to tegra-smmu ? It doesn't look correct that the
> directory will have a data field.

Looking at linux-next I found the commit which added it:

> @@ -892,6 +909,164 @@ static struct iommu_ops smmu_iommu_ops = {
>  	.pgsize_bitmap	= SMMU_IOMMU_PGSIZES,
>  };
>  
> +/* Should be in the order of enum */
> +static const char * const smmu_debugfs_mc[] = { "mc", };
> +static const char * const smmu_debugfs_cache[] = {  "tlb", "ptc", };
> +
> +static ssize_t smmu_debugfs_stats_write(struct file *file,
> +					const char __user *buffer,
> +					size_t count, loff_t *pos)
> +{
> +	struct smmu_device *smmu;
> +	struct dentry *dent;
> +	int i, cache, mc;
> +	enum {
> +		_OFF = 0,
> +		_ON,
> +		_RESET,
> +	};
> +	const char * const command[] = {
> +		[_OFF]		= "off",
> +		[_ON]		= "on",
> +		[_RESET]	= "reset",
> +	};
> +	char str[] = "reset";
> +	u32 val;
> +	size_t offs;
> +
> +	count = min_t(size_t, count, sizeof(str));
> +	if (copy_from_user(str, buffer, count))
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(command); i++)
> +		if (strncmp(str, command[i],
> +			    strlen(command[i])) == 0)
> +			break;
> +
> +	if (i == ARRAY_SIZE(command))
> +		return -EINVAL;
> +
> +	dent = file->f_dentry;
> +	cache = (int)dent->d_inode->i_private;

cache you can figure out by the filename. In fact, it would be much
better to have tlc and ptc directories with a "status" filename which
you write ON/OFF/RESET or enable/disable/reset to trigger what you need.

For that to work, you should probably hold tlb and ptc on an array of
some sort, and pass those as data to their respective "status" files as
the data field. If tlb and ptc are properly defined structures which can
point back to smmu, then you have everything you need.

I mean something like:

struct smmu;

struct mc {
	const char *name;
	void __iomem *regs;

	struct smmu *smmu;
};

struct smmu {
	struct mc mc[2]; /*what does mc stand for ? memory controller ? */

	...
};

debugfs_create_dir(smmu);
debugfs_create_dir(mc);
debugfs_create_dir(smmu->mc[1].name);
debugfs_create_dir(smmu->mc[2].name);
debugfs_create_file(&smmu->mc[1], status);
debugfs_create_file(&smmu->mc[2], status);

or something similar. You will avoid all the trickery you did here to
achieve what you need.

> +	mc = (int)dent->d_parent->d_inode->i_private;
> +	smmu = dent->d_parent->d_parent->d_inode->i_private;
> +
> +	offs = SMMU_CACHE_CONFIG(cache);
> +	val = smmu_read(smmu, offs);
> +	switch (i) {
> +	case _OFF:
> +		val &= ~SMMU_CACHE_CONFIG_STATS_ENABLE;
> +		val &= ~SMMU_CACHE_CONFIG_STATS_TEST;
> +		smmu_write(smmu, val, offs);
> +		break;
> +	case _ON:
> +		val |= SMMU_CACHE_CONFIG_STATS_ENABLE;
> +		val &= ~SMMU_CACHE_CONFIG_STATS_TEST;
> +		smmu_write(smmu, val, offs);
> +		break;
> +	case _RESET:
> +		val |= SMMU_CACHE_CONFIG_STATS_TEST;
> +		smmu_write(smmu, val, offs);
> +		val &= ~SMMU_CACHE_CONFIG_STATS_TEST;
> +		smmu_write(smmu, val, offs);
> +		break;
> +	default:
> +		BUG();
> +		break;
> +	}
> +
> +	dev_dbg(smmu->dev, "%s() %08x, %08x @%08x\n", __func__,
> +		val, smmu_read(smmu, offs), offs);
> +
> +	return count;
> +}
> +
> +static int smmu_debugfs_stats_show(struct seq_file *s, void *v)
> +{
> +	struct smmu_device *smmu;
> +	struct dentry *dent;
> +	int i, cache, mc;
> +	const char * const stats[] = { "hit", "miss", };
> +
> +	dent = d_find_alias(s->private);
> +	cache = (int)dent->d_inode->i_private;
> +	mc = (int)dent->d_parent->d_inode->i_private;
> +	smmu = dent->d_parent->d_parent->d_inode->i_private;
> +
> +	for (i = 0; i < ARRAY_SIZE(stats); i++) {
> +		u32 val;
> +		size_t offs;
> +
> +		offs = SMMU_STATS_CACHE_COUNT(mc, cache, i);
> +		val = smmu_read(smmu, offs);
> +		seq_printf(s, "%s:%08x ", stats[i], val);
> +
> +		dev_dbg(smmu->dev, "%s() %s %08x @%08x\n", __func__,
> +			stats[i], val, offs);
> +	}
> +	seq_printf(s, "\n");
> +
> +	return 0;
> +}
> +
> +static int smmu_debugfs_stats_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, smmu_debugfs_stats_show, inode);
> +}
> +
> +static const struct file_operations smmu_debugfs_stats_fops = {
> +	.open		= smmu_debugfs_stats_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +	.write		= smmu_debugfs_stats_write,
> +};
> +
> +static void smmu_debugfs_delete(struct smmu_device *smmu)
> +{
> +	debugfs_remove_recursive(smmu->debugfs_root);
> +}
> +
> +static void smmu_debugfs_create(struct smmu_device *smmu)
> +{
> +	int i;
> +	struct dentry *root;
> +
> +	root = debugfs_create_file(dev_name(smmu->dev),
> +				   S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
> +				   NULL, smmu, NULL);

directories don't need data. You don't even have a file_operations
structure so when exactly are you going to access the data ? What you
did above is actually wrong. You need to either patch this ASAP or drop
the patch you wrote and rewrite the whole debugfs support.

> +	if (!root)
> +		goto err_out;
> +	smmu->debugfs_root = root;
> +
> +	for (i = 0; i < ARRAY_SIZE(smmu_debugfs_mc); i++) {
> +		int j;
> +		struct dentry *mc;
> +
> +		mc = debugfs_create_file(smmu_debugfs_mc[i],
> +					 S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
> +					 root, (void *)i, NULL);

likewise here. What would you use that index for ? Even if you were to
access it, how would you use it ? Not to mention that you never, ever
access the private_data of the directory :-)

Just convert these two to the proper debugfs_create_dir()

> +		if (!mc)
> +			goto err_out;
> +
> +		for (j = 0; j < ARRAY_SIZE(smmu_debugfs_cache); j++) {
> +			struct dentry *cache;
> +
> +			cache = debugfs_create_file(smmu_debugfs_cache[j],
> +						    S_IWUGO | S_IRUGO, mc,
> +						    (void *)j,
> +						    &smmu_debugfs_stats_fops);

it would be far better to pass a structure which you actually need. In
this case 'smmu'. That will be a lot more useful for your code.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] iommu/tegra: smmu: Use __debugfs_create_dir

[Hiroshi Doyu]

Hi,

Thank you for review. Already sent the another version of patch(v2:
*1), but I tried to answer the remaining questions inlined.....

On Wed, 8 Aug 2012 17:31:02 +0200
Felipe Balbi <balbi@ti.com> wrote:

> * PGP Signed by an unknown key
> 
> Hi,
> 
> On Wed, Aug 08, 2012 at 06:11:29PM +0300, Felipe Balbi wrote:
> > Hi,
> >
> > On Wed, Aug 08, 2012 at 09:24:33AM +0300, Hiroshi Doyu wrote:
> > > The commit c3b1a35 "debugfs: make sure that debugfs_create_file() gets
> > > used only for regulars" doesn't allow to use debugfs_create_file() for
> > > dir. Use the version with "data", __debugfs_create_dir().
> > >
> > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > Reported-by: Laxman Dewangan <ldewangan@nvidia.com>
> > > ---
> > >  drivers/iommu/tegra-smmu.c |    4 +---
> > >  1 files changed, 1 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > > index 5e51fb7..41aff7a 100644
> > > --- a/drivers/iommu/tegra-smmu.c
> > > +++ b/drivers/iommu/tegra-smmu.c
> > > @@ -1035,9 +1035,7 @@ static void smmu_debugfs_create(struct smmu_device *smmu)
> > >     int i;
> > >     struct dentry *root;
> > >
> > > -   root = debugfs_create_file(dev_name(smmu->dev),
> > > -                              S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
> > > -                              NULL, smmu, NULL);
> > > +   root = __debugfs_create_dir(dev_name(smmu->dev), NULL, smmu);
> >
> > why would the directory need extra data ? Looking in mainline,
> > tegra-smmu.c doesn't have any debugfs support, where can I see the
> > patches adding debugfs to tegra-smmu ? It doesn't look correct that the
> > directory will have a data field.
> 
> Looking at linux-next I found the commit which added it:

FYI: The original tegra smmu debugfs patch is:
  http://lists.linuxfoundation.org/pipermail/iommu/2012-August/004507.html

> > @@ -892,6 +909,164 @@ static struct iommu_ops smmu_iommu_ops = {
> >       .pgsize_bitmap  = SMMU_IOMMU_PGSIZES,
> >  };
> >
> > +/* Should be in the order of enum */
> > +static const char * const smmu_debugfs_mc[] = { "mc", };
> > +static const char * const smmu_debugfs_cache[] = {  "tlb", "ptc", };
> > +
> > +static ssize_t smmu_debugfs_stats_write(struct file *file,
> > +                                     const char __user *buffer,
> > +                                     size_t count, loff_t *pos)
> > +{
> > +     struct smmu_device *smmu;
> > +     struct dentry *dent;
> > +     int i, cache, mc;
> > +     enum {
> > +             _OFF = 0,
> > +             _ON,
> > +             _RESET,
> > +     };
> > +     const char * const command[] = {
> > +             [_OFF]          = "off",
> > +             [_ON]           = "on",
> > +             [_RESET]        = "reset",
> > +     };
> > +     char str[] = "reset";
> > +     u32 val;
> > +     size_t offs;
> > +
> > +     count = min_t(size_t, count, sizeof(str));
> > +     if (copy_from_user(str, buffer, count))
> > +             return -EINVAL;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(command); i++)
> > +             if (strncmp(str, command[i],
> > +                         strlen(command[i])) == 0)
> > +                     break;
> > +
> > +     if (i == ARRAY_SIZE(command))
> > +             return -EINVAL;
> > +
> > +     dent = file->f_dentry;
> > +     cache = (int)dent->d_inode->i_private;
> 
> cache you can figure out by the filename. In fact, it would be much
> better to have tlc and ptc directories with a "status" filename which
> you write ON/OFF/RESET or enable/disable/reset to trigger what you need.

Actually I also considered {ptc,tlb} directories, but I thought that
those might be residual, or nested one more unnecessarily.

The current usage is:

  $ echo "reset" > /sys/kernel/debug/smmu/mc/{tlb,ptc}
  $ echo "on" > /sys/kernel/debug/smmu/mc/{tlb,ptc}
  $ echo "off" > /sys/kernel/debug/smmu/mc/{tlb,ptc}
  $ cat /sys/kernel/debug/smmu/mc/{tlb,ptc}
  hit:0014910c miss:00014d22

  The above format is:
  hit:<HIT count><SPC>miss:<MISS count><SPC><CR+LF>

  fscanf(fp, "hit:%lx miss:%lx", &hit, &miss);


If {ptc,tlb} was dir, the usage would be:

  $ echo "reset" > /sys/kernel/debug/smmu/mc/{tlb,ptc}/status
  $ echo "on" > /sys/kernel/debug/smmu/mc/{tlb,ptc}/status
  $ echo "off" > /sys/kernel/debug/smmu/mc/{tlb,ptc}/status
  $ cat /sys/kernel/debug/smmu/mc/{tlb,ptc}/data
  hit:0014910c miss:00014d22

One advantage of dirs could be, you may be able to check the current
status by reading "status". It might be less likely read back
practically because if writing succeeded, the state should be what was
written.

> For that to work, you should probably hold tlb and ptc on an array of
> some sort, and pass those as data to their respective "status" files as
> the data field. If tlb and ptc are properly defined structures which can
> point back to smmu, then you have everything you need.

I also considered to introduce new structure like what you suggested
below, but I felt that the parent<-chile relationships are already in
directory hierarchy, and I wanted to avoid the residual data with
introducing new structures. Instead of introducing new structure,
those parent<-child relationships are always gotton from debugfs
directory hierarchy on demand. That was why I wanted to put data in
debugfs dir. With debugfs directories having private data, the
connections between entities would be kept in filesystem.

I've already sent another version of patch(v2, *1), which passes all
necessary data to a file, put in a structure. This v2 patch may be a
little bit simliear to what Felipe suggested below.

*1: http://lists.linuxfoundation.org/pipermail/iommu/2012-August/004535.html

> I mean something like:
> 
> struct smmu;
> 
> struct mc {
>         const char *name;
>         void __iomem *regs;
> 
>         struct smmu *smmu;
> };
> 
> struct smmu {
>         struct mc mc[2]; /*what does mc stand for ? memory controller ? */
> 
>         ...
> };
> 
> debugfs_create_dir(smmu);
> debugfs_create_dir(mc);
> debugfs_create_dir(smmu->mc[1].name);
> debugfs_create_dir(smmu->mc[2].name);
> debugfs_create_file(&smmu->mc[1], status);
> debugfs_create_file(&smmu->mc[2], status);
> 
> or something similar. You will avoid all the trickery you did here to
> achieve what you need.
> 
> > +     mc = (int)dent->d_parent->d_inode->i_private;
> > +     smmu = dent->d_parent->d_parent->d_inode->i_private;
> > +
> > +     offs = SMMU_CACHE_CONFIG(cache);
> > +     val = smmu_read(smmu, offs);
> > +     switch (i) {
> > +     case _OFF:
> > +             val &= ~SMMU_CACHE_CONFIG_STATS_ENABLE;
> > +             val &= ~SMMU_CACHE_CONFIG_STATS_TEST;
> > +             smmu_write(smmu, val, offs);
> > +             break;
> > +     case _ON:
> > +             val |= SMMU_CACHE_CONFIG_STATS_ENABLE;
> > +             val &= ~SMMU_CACHE_CONFIG_STATS_TEST;
> > +             smmu_write(smmu, val, offs);
> > +             break;
> > +     case _RESET:
> > +             val |= SMMU_CACHE_CONFIG_STATS_TEST;
> > +             smmu_write(smmu, val, offs);
> > +             val &= ~SMMU_CACHE_CONFIG_STATS_TEST;
> > +             smmu_write(smmu, val, offs);
> > +             break;
> > +     default:
> > +             BUG();
> > +             break;
> > +     }
> > +
> > +     dev_dbg(smmu->dev, "%s() %08x, %08x @%08x\n", __func__,
> > +             val, smmu_read(smmu, offs), offs);
> > +
> > +     return count;
> > +}
> > +
> > +static int smmu_debugfs_stats_show(struct seq_file *s, void *v)
> > +{
> > +     struct smmu_device *smmu;
> > +     struct dentry *dent;
> > +     int i, cache, mc;
> > +     const char * const stats[] = { "hit", "miss", };
> > +
> > +     dent = d_find_alias(s->private);
> > +     cache = (int)dent->d_inode->i_private;
> > +     mc = (int)dent->d_parent->d_inode->i_private;
> > +     smmu = dent->d_parent->d_parent->d_inode->i_private;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(stats); i++) {
> > +             u32 val;
> > +             size_t offs;
> > +
> > +             offs = SMMU_STATS_CACHE_COUNT(mc, cache, i);
> > +             val = smmu_read(smmu, offs);
> > +             seq_printf(s, "%s:%08x ", stats[i], val);
> > +
> > +             dev_dbg(smmu->dev, "%s() %s %08x @%08x\n", __func__,
> > +                     stats[i], val, offs);
> > +     }
> > +     seq_printf(s, "\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static int smmu_debugfs_stats_open(struct inode *inode, struct file *file)
> > +{
> > +     return single_open(file, smmu_debugfs_stats_show, inode);
> > +}
> > +
> > +static const struct file_operations smmu_debugfs_stats_fops = {
> > +     .open           = smmu_debugfs_stats_open,
> > +     .read           = seq_read,
> > +     .llseek         = seq_lseek,
> > +     .release        = single_release,
> > +     .write          = smmu_debugfs_stats_write,
> > +};
> > +
> > +static void smmu_debugfs_delete(struct smmu_device *smmu)
> > +{
> > +     debugfs_remove_recursive(smmu->debugfs_root);
> > +}
> > +
> > +static void smmu_debugfs_create(struct smmu_device *smmu)
> > +{
> > +     int i;
> > +     struct dentry *root;
> > +
> > +     root = debugfs_create_file(dev_name(smmu->dev),
> > +                                S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
> > +                                NULL, smmu, NULL);
> 
> directories don't need data. You don't even have a file_operations
> structure so when exactly are you going to access the data ? What you
> did above is actually wrong. You need to either patch this ASAP or drop
> the patch you wrote and rewrite the whole debugfs support.
> 
> > +     if (!root)
> > +             goto err_out;
> > +     smmu->debugfs_root = root;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(smmu_debugfs_mc); i++) {
> > +             int j;
> > +             struct dentry *mc;
> > +
> > +             mc = debugfs_create_file(smmu_debugfs_mc[i],
> > +                                      S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
> > +                                      root, (void *)i, NULL);
> 
> likewise here. What would you use that index for ? Even if you were to
> access it, how would you use it ? Not to mention that you never, ever
> access the private_data of the directory :-)
> 
> Just convert these two to the proper debugfs_create_dir()
> 
> > +             if (!mc)
> > +                     goto err_out;
> > +
> > +             for (j = 0; j < ARRAY_SIZE(smmu_debugfs_cache); j++) {
> > +                     struct dentry *cache;
> > +
> > +                     cache = debugfs_create_file(smmu_debugfs_cache[j],
> > +                                                 S_IWUGO | S_IRUGO, mc,
> > +                                                 (void *)j,
> > +                                                 &smmu_debugfs_stats_fops);
> 
> it would be far better to pass a structure which you actually need. In
> this case 'smmu'. That will be a lot more useful for your code.

Re: [PATCH 2/2] iommu/tegra: smmu: Use __debugfs_create_dir

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5131 bytes --]

Hi,

On Wed, Aug 15, 2012 at 09:34:21AM +0300, Hiroshi Doyu wrote:
> > > @@ -892,6 +909,164 @@ static struct iommu_ops smmu_iommu_ops = {
> > >       .pgsize_bitmap  = SMMU_IOMMU_PGSIZES,
> > >  };
> > >
> > > +/* Should be in the order of enum */
> > > +static const char * const smmu_debugfs_mc[] = { "mc", };
> > > +static const char * const smmu_debugfs_cache[] = {  "tlb", "ptc", };
> > > +
> > > +static ssize_t smmu_debugfs_stats_write(struct file *file,
> > > +                                     const char __user *buffer,
> > > +                                     size_t count, loff_t *pos)
> > > +{
> > > +     struct smmu_device *smmu;
> > > +     struct dentry *dent;
> > > +     int i, cache, mc;
> > > +     enum {
> > > +             _OFF = 0,
> > > +             _ON,
> > > +             _RESET,
> > > +     };
> > > +     const char * const command[] = {
> > > +             [_OFF]          = "off",
> > > +             [_ON]           = "on",
> > > +             [_RESET]        = "reset",
> > > +     };
> > > +     char str[] = "reset";
> > > +     u32 val;
> > > +     size_t offs;
> > > +
> > > +     count = min_t(size_t, count, sizeof(str));
> > > +     if (copy_from_user(str, buffer, count))
> > > +             return -EINVAL;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(command); i++)
> > > +             if (strncmp(str, command[i],
> > > +                         strlen(command[i])) == 0)
> > > +                     break;
> > > +
> > > +     if (i == ARRAY_SIZE(command))
> > > +             return -EINVAL;
> > > +
> > > +     dent = file->f_dentry;
> > > +     cache = (int)dent->d_inode->i_private;
> > 
> > cache you can figure out by the filename. In fact, it would be much
> > better to have tlc and ptc directories with a "status" filename which
> > you write ON/OFF/RESET or enable/disable/reset to trigger what you need.
> 
> Actually I also considered {ptc,tlb} directories, but I thought that
> those might be residual, or nested one more unnecessarily.
> 
> The current usage is:
> 
>   $ echo "reset" > /sys/kernel/debug/smmu/mc/{tlb,ptc}
>   $ echo "on" > /sys/kernel/debug/smmu/mc/{tlb,ptc}
>   $ echo "off" > /sys/kernel/debug/smmu/mc/{tlb,ptc}
>   $ cat /sys/kernel/debug/smmu/mc/{tlb,ptc}
>   hit:0014910c miss:00014d22
> 
>   The above format is:
>   hit:<HIT count><SPC>miss:<MISS count><SPC><CR+LF>

if you're just printing hit and miss count, wouldn't it be a bit more
human-friendly to print it in decimal rather than hex ? no strong
feelings against either way, just thought I'd mention it.

>   fscanf(fp, "hit:%lx miss:%lx", &hit, &miss);
> 
> 
> If {ptc,tlb} was dir, the usage would be:
> 
>   $ echo "reset" > /sys/kernel/debug/smmu/mc/{tlb,ptc}/status
>   $ echo "on" > /sys/kernel/debug/smmu/mc/{tlb,ptc}/status
>   $ echo "off" > /sys/kernel/debug/smmu/mc/{tlb,ptc}/status
>   $ cat /sys/kernel/debug/smmu/mc/{tlb,ptc}/data
>   hit:0014910c miss:00014d22
> 
> One advantage of dirs could be, you may be able to check the current
> status by reading "status". It might be less likely read back
> practically because if writing succeeded, the state should be what was
> written.

sure.

> > For that to work, you should probably hold tlb and ptc on an array of
> > some sort, and pass those as data to their respective "status" files as
> > the data field. If tlb and ptc are properly defined structures which can
> > point back to smmu, then you have everything you need.
> 
> I also considered to introduce new structure like what you suggested
> below, but I felt that the parent<-chile relationships are already in
> directory hierarchy, and I wanted to avoid the residual data with
> introducing new structures. Instead of introducing new structure,
> those parent<-child relationships are always gotton from debugfs
> directory hierarchy on demand. That was why I wanted to put data in
> debugfs dir. With debugfs directories having private data, the
> connections between entities would be kept in filesystem.

fair enough.

> I've already sent another version of patch(v2, *1), which passes all
> necessary data to a file, put in a structure. This v2 patch may be a
> little bit simliear to what Felipe suggested below.

I looked over that, but I'm not sure you should introduce that
smmu_debugfs_info structure. Look at what we do on
drivers/usb/dwc3/debugfs.c, we don't add any extra structures for
debugfs, we use what the driver already has (struct dwc3-only,
currently).

If we were to add debgufs support for each USB endpoint, I would pass
struct dwc3_ep as data for the files. See that I would still be able to
access struct dwc3, should I need it, because struct dwc3_ep knows which
struct dwc3 it belongs to.

That's what I meant when I suggested adding more structures, not
something for debugfs-only, but something for the whole driver to use.
Just re-design the driver a little bit and you won't need to allocate
memory when creating debugfs directories, because the data you need is
already available.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]