lkml.org 
[lkml]   [2017]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] firmware: tegra: add BPMP debugfs support
From
Date

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

\
 
 \ /
  Last update: 2017-09-29 16:59    [W:0.079 / U:1.712 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site