Re: [PATCH] firmware: tegra: add BPMP debugfs support

[Jon Hunter]

On 29/09/17 14:41, Timo Alho wrote:> 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.

OK, yes that would be better.

>>> 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

...

>>> +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).

OK.

>>> +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?

Yes why not just 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.

OK great.

Cheers
Jon

-- 
nvpublic