Date:	Wed, 15 Aug 2012 10:07:29 +0300
From:	Felipe Balbi <balbi@...com>
To:	Hiroshi Doyu <hdoyu@...dia.com>
Cc:	"balbi@...com" <balbi@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Joerg Roedel <joerg.roedel@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Chris Wright <chrisw@...s-sol.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] iommu/tegra: smmu: Use __debugfs_create_dir

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

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists