Re: [PATCH] firmware: tegra: add BPMP debugfs support
From: Timo Alho
Date: Fri Sep 29 2017 - 09:42:05 EST
Jon,
Thanks for reviewing this!
On 21.09.2017 14:10, Jonathan Hunter wrote:
--- a/drivers/firmware/tegra/bpmp.c
+++ b/drivers/firmware/tegra/bpmp.c
@@ -824,6 +824,8 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
if (err < 0)
goto free_mrq;
+ (void)tegra_bpmp_init_debugfs(bpmp);
+
This looks a bit odd, why not just return the error code here?
I wanted to avoid failing probe if only debugfs initialization fails.
I'll add a error print in next version here, but let probing to complete.
diff --git a/drivers/firmware/tegra/bpmp_debugfs.c b/drivers/firmware/tegra/bpmp_debugfs.c
new file mode 100644
index 0000000..4d0279d
--- /dev/null
+++ b/drivers/firmware/tegra/bpmp_debugfs.c
...
+/* map filename in Linux debugfs to corresponding entry in BPMP */
+static const char *get_filename(struct tegra_bpmp *bpmp,
+ const struct file *file, char *buf, int size)
+{
+ char root_path_buf[512];
+ const char *root_path;
+ const char *filename;
+ size_t root_len;
+
+ root_path = dentry_path_raw(bpmp->debugfs_mirror, root_path_buf,
+ sizeof(root_path_buf));
+ if (IS_ERR_OR_NULL(root_path))
Looks like IS_ERR() is sufficient here.
Will fix that and all other places where IS_ERR_OR_NULL() is used.
+static int mrq_debugfs_read(struct tegra_bpmp *bpmp,
+ dma_addr_t name, size_t sz_name,
+ dma_addr_t data, size_t sz_data,
+ size_t *nbytes)
+{
+ struct mrq_debugfs_request req = {
+ .cmd = cpu_to_le32(CMD_DEBUGFS_READ),
+ .fop = {
+ .fnameaddr = cpu_to_le32((uint32_t)name),
+ .fnamelen = cpu_to_le32((uint32_t)sz_name),
+ .dataaddr = cpu_to_le32((uint32_t)data),
+ .datalen = cpu_to_le32((uint32_t)sz_data),
+ },
+ };
+ struct mrq_debugfs_response resp;
+ struct tegra_bpmp_message msg = {
+ .mrq = MRQ_DEBUGFS,
+ .tx = {
+ .data = &req,
+ .size = sizeof(req),
+ },
+ .rx = {
+ .data = &resp,
+ .size = sizeof(resp),
+ },
+ };
Not sure if the above would be better in some sub-function to prepare
the message. Looks like such a sub/helper function could be used by some
of the following functions too.
I thought about it but gave up as it would be just generic extra wrapper
for existing API (that is not specific to this driver).
+static int debugfs_show(struct seq_file *m, void *p)
+{
+ struct file *file = m->private;
+ struct inode *inode = file_inode(file);
+ struct tegra_bpmp *bpmp = inode->i_private;
+ const size_t datasize = m->size;
+ const size_t namesize = SZ_256;
+ void *datavirt, *namevirt;
+ dma_addr_t dataphys, namephys;
+ char buf[256];
+ const char *filename;
+ size_t len, nbytes;
+ int ret;
+
+ filename = get_filename(bpmp, file, buf, sizeof(buf));
+ if (!filename)
+ return -ENOENT;
Is it guaranteed that filename is null terminated here? If not then ...
As far as I can tell by looking at get_filename() and the functions it
calls, filename should be null terminated.
+
+ namevirt = dma_alloc_coherent(bpmp->dev, namesize, &namephys,
+ GFP_KERNEL | GFP_DMA32);
+ if (!namevirt)
+ return -ENOMEM;
+
+ datavirt = dma_alloc_coherent(bpmp->dev, datasize, &dataphys,
+ GFP_KERNEL | GFP_DMA32);
+ if (!datavirt) {
+ ret = -ENOMEM;
+ goto free_namebuf;
+ }
+
+ len = strlen(filename);
+ strlcpy(namevirt, filename, namesize);
Although very unlikely a name would be 256 characters long, but if it
was 256 chars then the last character would be truncated.
Yes, will replace this with strncpy(). BPMP does not require this string
to be NULL terminated anyway.
+static int bpmp_populate_dir(struct tegra_bpmp *bpmp, struct seqbuf *seqbuf,
+ struct dentry *parent, uint32_t depth)
Do we need to use uint32_t here?
+{
+ int err;
+ uint32_t d, t;
And here?
It's part of the BPMP ABI that the data passed is 32 bit unsigned
integer. I wanted to emphasise that with the choice of integer type. Or
did you mean why I did not use u32?
+ const char *name;
+ struct dentry *dentry;
+
+ while (!seqbuf_eof(seqbuf)) {
+ if (seqbuf_read_u32(seqbuf, &d) < 0)
+ return -EIO;
Why not return the actual error code?
Will fix in next patch
+ return dentry ? PTR_ERR(dentry) : -ENOMEM;
+ err = bpmp_populate_dir(bpmp, seqbuf, dentry, depth+1);
+ if (err < 0)
+ return err;
Do we need to remove the directory created here? Or is that handled by
the recursive removal below?
Recursive removal will take care of it.
+static int mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq)
+{
+ struct mrq_query_abi_request req = { .mrq = cpu_to_le32(mrq) };
+ struct mrq_query_abi_response resp;
+ struct tegra_bpmp_message msg = {
+ .mrq = MRQ_QUERY_ABI,
+ .tx = {
+ .data = &req,
+ .size = sizeof(req),
+ },
+ .rx = {
+ .data = &resp,
+ .size = sizeof(resp),
+ },
+ };
+ int ret;
+
+ ret = tegra_bpmp_transfer(bpmp, &msg);
+ /* something went wrong; assume not supported */
+ if (ret < 0)
dev_warn?
Will add.
+
+ virt = dma_alloc_coherent(bpmp->dev, sz, &phys,
+ GFP_KERNEL | GFP_DMA32);
+ if (!virt) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = mrq_debugfs_dumpdir(bpmp, phys, sz, &nbytes);
+ if (ret < 0)
+ goto free;
+
+ ret = create_debugfs_mirror(bpmp, virt, nbytes, root);
+ if (ret < 0)
+ dev_err(bpmp->dev, "create_debugfs_mirror failed (%d)\n", ret);
+
+free:
+ dma_free_coherent(bpmp->dev, sz, virt, phys);
+out:
+ if (ret < 0)
+ debugfs_remove(root);
Should we have a generic warning message here? Looks like it could fail
silently.
I'll add a warning to call site (bpmp.c) if this fails.
-Timo