Re: [PATCH 2/2] platform/x86/intel/tpmi: Add debugfs interface
From: srinivas pandruvada
Date: Fri Jun 16 2023 - 12:57:25 EST
On Fri, 2023-06-16 at 10:46 +0300, Ilpo Järvinen wrote:
> On Thu, 15 Jun 2023, Srinivas Pandruvada wrote:
>
> >
[...]
> > + seq_puts(s, help);
>
> The appropriate place to this kinda information would seem to be:
>
> Documentation/ABI/testing/debugfs-... file.
I prefer to add to Documentation.
But this is for validation folks, who struggle to get documentation,
will ask you 10 question before using. Hence added here.
But I don't have strong preference here. I can move to doc area.
>
> > +
> > + return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(tpmi_help);
> > +
> > +static int tpmi_pfs_dbg_show(struct seq_file *s, void *unused)
> > +{
> > + struct intel_tpmi_info *tpmi_info = s->private;
> > + int i, ret;
> > +
> > + seq_printf(s, "tpmi PFS start offset 0x:%llx\n", tpmi_info-
> > >pfs_start);
> > + seq_puts(s,
> > "tpmi_id\t\tnum_entries\tentry_size\t\tcap_offset\tattribute\tfull_
> > base_pointer_for_memmap\tlocked\tdisabled\n");
> > + for (i = 0; i < tpmi_info->feature_count; ++i) {
> > + struct intel_tpmi_pm_feature *pfs;
> > + int locked, disabled;
> > +
> > + pfs = &tpmi_info->tpmi_features[i];
> > + ret = tpmi_read_feature_status(tpmi_info, pfs-
> > >pfs_header.tpmi_id, &locked, &disabled);
> > + if (ret) {
> > + locked = 'U';
> > + disabled = 'U';
> > + } else {
> > + disabled = disabled ? 'Y' : 'N';
> > + locked = locked ? 'Y' : 'N';
> > + }
> > + seq_printf(s,
> > "0x%02x\t\t0x%02x\t\t0x%06x\t\t0x%04x\t\t0x%02x\t\t0x%x\t\t\t%c\t%c
> > \n",
>
> The last hex is just %x (not %08x), is it intentional?
Not intentional.
>
> > + pfs->pfs_header.tpmi_id, pfs-
> > >pfs_header.num_entries, pfs->pfs_header.entry_size,
> > + pfs->pfs_header.cap_offset, pfs-
> > >pfs_header.attribute, pfs->vsec_offset, locked, disabled);
>
> Please split parameters to 100 columns (I'm okay with the string
> exceeding it).
>
> It would help here if you add pointer also to pfs_header struct. ;-)
>
Will think about it if useful.
> > + }
> > +
> > + return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(tpmi_pfs_dbg);
> > +
> > +#define MEM_DUMP_COLUMN_COUNT 8
> > +
> > +static int tpmi_mem_dump_show(struct seq_file *s, void *unused)
> > +{
> > + size_t row_size = MEM_DUMP_COLUMN_COUNT * sizeof(u32);
> > + struct intel_tpmi_pm_feature *pfs = s->private;
> > + int count, ret = 0;
> > + void __iomem *mem;
> > + u16 size;
> > + u32 off;
> > +
> > + off = pfs->vsec_offset;
> > +
> > + mutex_lock(&tpmi_dev_lock);
> > +
> > + for (count = 0; count < pfs->pfs_header.num_entries;
> > ++count) {
> > + u8 *buffer;
>
> Why only this is declared here? I see no consistency based on
> variable usage/scope.
I will fix this.
>
> > + size = pfs->pfs_header.entry_size * sizeof(u32);
>
> Can this overflow?
No. Coming from a trusted architectural source. The system will not
pass BIOS if they are wrong.
>
> > + buffer = kmalloc(size, GFP_KERNEL);
> > + if (!buffer) {
> > + ret = -ENOMEM;
> > + goto done_mem_show;
> > + }
> > +
> > + seq_printf(s, "TPMI Instance:%d offset:0x%x\n",
> > count, off);
> > +
> > + mem = ioremap(off, size);
> > + if (!mem) {
> > + ret = -ENOMEM;
> > + kfree(buffer);
> > + goto done_mem_show;
> > + }
> > +
> > + memcpy_fromio(buffer, mem, size);
> > +
> > + seq_hex_dump(s, " ", DUMP_PREFIX_OFFSET, row_size,
> > sizeof(u32), buffer, size, false);
> > +
> > + iounmap(mem);
> > + kfree(buffer);
> > +
> > + off += size;
> > + }
> > +
> > +done_mem_show:
> > + mutex_unlock(&tpmi_dev_lock);
> > +
> > + return ret;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(tpmi_mem_dump);
> > +
> > +static ssize_t mem_write(struct file *file, const char __user
> > *userbuf, size_t len, loff_t *ppos)
> > +{
> > + struct seq_file *m = file->private_data;
> > + struct intel_tpmi_pm_feature *pfs = m->private;
> > + u32 addr, value, punit;
> > + u32 num_elems, *array;
> > + void __iomem *mem;
> > + u16 size;
> > + int ret;
> > +
> > + ret = parse_int_array_user(userbuf, len, (int **)&array);
> > + if (ret < 0)
> > + return ret;
> > +
> > + num_elems = *array;
> > + if (num_elems != 3) {
> > + ret = -EINVAL;
> > + goto exit_write;
> > + }
> > +
> > + punit = array[1];
> > + addr = array[2];
> > + value = array[3];
> > +
> > + if (punit >= pfs->pfs_header.num_entries) {
> > + ret = -EINVAL;
> > + goto exit_write;
> > + }
> > +
> > + size = pfs->pfs_header.entry_size * sizeof(u32);
>
> There's no consistency in the code, some places do: entry_size * 4
> and
> here it's entry_size * sizeof(u32). Please convert all of them to the
> latter one. You need to do one additional patch to convert the
> existing
> users but that's perfectly fine as an additional cleanup patch (don't
> try to put it either of these patches "while at it").
Good idea.
>
> > + if (addr >= size) {
> > + ret = -EINVAL;
> > + goto exit_write;
> > + }
> > +
> > + mutex_lock(&tpmi_dev_lock);
> > +
> > + mem = ioremap(pfs->vsec_offset + (punit * size), size);
>
> Unnecessary parenthesis.
ok
>
> > + if (!mem) {
> > + ret = -ENOMEM;
> > + goto unlock_mem_write;
> > + }
> > +
> > + writel(value, mem + addr);
> > +
> > + iounmap(mem);
> > +
> > + ret = len;
> > +
> > +unlock_mem_write:
> > + mutex_unlock(&tpmi_dev_lock);
> > +
> > +exit_write:
> > + kfree(array);
> > +
> > + return ret;
> > +}
> > +
> > +static int mem_write_show(struct seq_file *s, void *unused)
> > +{
> > + return 0;
> > +}
> > +
> > +static int mem_write_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, mem_write_show, inode->i_private);
> > +}
> > +
> > +static const struct file_operations mem_write_ops = {
> > + .open = mem_write_open,
> > + .read = seq_read,
> > + .write = mem_write,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
> > +
> > +#define tpmi_to_dev(info) (&info->vsec_dev->pcidev->dev)
> > +
> > +static void tpmi_dbgfs_register(struct intel_tpmi_info *tpmi_info)
> > +{
> > + struct dentry *top_dir;
> > + char name[64];
> > + int i;
> > +
> > + snprintf(name, sizeof(name), "tpmi-%s",
> > dev_name(tpmi_to_dev(tpmi_info)));
> > + top_dir = debugfs_create_dir(name, NULL);
> > + if (IS_ERR_OR_NULL(top_dir))
> > + return;
> > +
> > + tpmi_info->dbgfs_dir = top_dir;
> > +
> > + debugfs_create_file("pfs_dump", 0444, top_dir, tpmi_info,
> > + &tpmi_pfs_dbg_fops);
>
> One line.
OK
>
> > + debugfs_create_file("help", 0444, top_dir, NULL,
> > &tpmi_help_fops);
> > + for (i = 0; i < tpmi_info->feature_count; ++i) {
> > + struct intel_tpmi_pm_feature *pfs;
> > + struct dentry *dir;
> > +
> > + pfs = &tpmi_info->tpmi_features[i];
> > + snprintf(name, sizeof(name), "tpmi-id-%02x", pfs-
> > >pfs_header.tpmi_id);
> > + dir = debugfs_create_dir(name, top_dir);
> > +
> > + debugfs_create_file("mem_dump", 0444, dir, pfs,
> > + &tpmi_mem_dump_fops);
> > + debugfs_create_file("mem_write", 0644, dir, pfs,
> > + &mem_write_ops);
>
> These too can be put to one line.
>
OK
Thanks,
Srinivas
>