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:
signature.asc
Description: Digital signature
