Hi Rishabh,
----- On 28 Mar, 2020, at 01:09, Rishabh Bhatnagar rishabhb@xxxxxxxxxxxxxx wrote:
> On 2020-03-26 10:37, Clément Leger wrote:
>> Hi Rishabh,
>>
>> While being interesting to have a such a userspace interface, I have
>> some remarks.
>>
>> ----- On 26 Mar, 2020, at 17:50, Rishabh Bhatnagar
>> rishabhb@xxxxxxxxxxxxxx wrote:
>>
>>> Add the driver for creating the character device interface for
>>> userspace applications. The character device interface can be used
>>> in order to boot up and shutdown the remote processor.
>>> This might be helpful for remote processors that are booted by
>>> userspace applications and need to shutdown when the application
>>> crahes/shutsdown.
>>>
>>> Change-Id: If23c8986272bb7c943eb76665f127ff706f12394
>>> Signed-off-by: Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
>>> ---
>>> drivers/remoteproc/Makefile | 1 +
>>> drivers/remoteproc/remoteproc_internal.h | 6 +++
>>> drivers/remoteproc/remoteproc_userspace.c | 90
>>> +++++++++++++++++++++++++++++++
>>> include/linux/remoteproc.h | 2 +
>>> 4 files changed, 99 insertions(+)
>>> create mode 100644 drivers/remoteproc/remoteproc_userspace.c
>>>
>>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>>> index e30a1b1..facb3fa 100644
>>> --- a/drivers/remoteproc/Makefile
>>> +++ b/drivers/remoteproc/Makefile
>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_REMOTEPROC) += remoteproc.o
>>> remoteproc-y := remoteproc_core.o
>>> remoteproc-y += remoteproc_debugfs.o
>>> remoteproc-y += remoteproc_sysfs.o
>>> +remoteproc-y += remoteproc_userspace.o
>>> remoteproc-y += remoteproc_virtio.o
>>> remoteproc-y += remoteproc_elf_loader.o
>>> obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>>> b/drivers/remoteproc/remoteproc_internal.h
>>> index 493ef92..97513ba 100644
>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>> @@ -47,6 +47,9 @@ struct dentry *rproc_create_trace_file(const char
>>> *name,
>>> struct rproc *rproc,
>>> int rproc_init_sysfs(void);
>>> void rproc_exit_sysfs(void);
>>>
>>> +void rproc_init_cdev(void);
>>> +void rproc_exit_cdev(void);
>>> +
>>> void rproc_free_vring(struct rproc_vring *rvring);
>>> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>>>
>>> @@ -63,6 +66,9 @@ struct resource_table
>>> *rproc_elf_find_loaded_rsc_table(struct
>>> rproc *rproc,
>>> struct rproc_mem_entry *
>>> rproc_find_carveout_by_name(struct rproc *rproc, const char *name,
>>> ...);
>>>
>>> +/* from remoteproc_userspace.c */
>>> +int rproc_char_device_add(struct rproc *rproc);
>>> +
>>> static inline
>>> int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware
>>> *fw)
>>> {
>>> diff --git a/drivers/remoteproc/remoteproc_userspace.c
>>> b/drivers/remoteproc/remoteproc_userspace.c
>>> new file mode 100644
>>> index 0000000..2ef7679
>>> --- /dev/null
>>> +++ b/drivers/remoteproc/remoteproc_userspace.c
>>> @@ -0,0 +1,90 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Character device interface driver for Remoteproc framework.
>>> + *
>>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/fs.h>
>>> +#include <linux/cdev.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/remoteproc.h>
>>> +
>>> +#include "remoteproc_internal.h"
>>> +
>>> +#define NUM_RPROC_DEVICES 64
>>> +static dev_t rproc_cdev;
>>> +static DEFINE_IDA(cdev_minor_ida);
>>> +
>>> +static int rproc_open(struct inode *inode, struct file *file)
>>> +{
>>> + struct rproc *rproc;
>>> +
>>> + rproc = container_of(inode->i_cdev, struct rproc, char_dev);
>>> + if (!rproc)
>>> + return -EINVAL;
>>> +
>>> + return rproc_boot(rproc);
>>> +}
>>
>> What happens if multiple user open this chardev ? Apparently,
>> rproc_boot returns 0 if already powered_up, so the next user won't know
>> what is the state of the rproc.
>> Exclusive access could probably be a good idea.
> Since it is synchronized inside rproc_boot multiple users simultaneously
> calling open shouldn't be a problem. If it is one after the other then
> second caller will get result as 0 and assume that rproc booted
> successfully.
It will be the same for close, it will assume the rproc has been stopped ?
But in fact it will still be running until the refcount is 0.
> That is the expected flow right?
I would expect only one caller to be successful, others should probably
receive a EBUSY errno IMHO.
>>
>>> +
>>> +static int rproc_release(struct inode *inode, struct file *file)
>>> +{
>>> + struct rproc *rproc;
>>> +
>>> + rproc = container_of(inode->i_cdev, struct rproc, char_dev);
>>> + if (!rproc)
>>> + return -EINVAL;
>>> +
>>> + rproc_shutdown(rproc);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct file_operations rproc_fops = {
>>> + .open = rproc_open,
>>> + .release = rproc_release,
>>> +};
>>> +
>>> +int rproc_char_device_add(struct rproc *rproc)
>>> +{
>>> + int ret, minor;
>>> + dev_t cdevt;
>>> +
>>> + minor = ida_simple_get(&cdev_minor_ida, 0, NUM_RPROC_DEVICES,
>>> + GFP_KERNEL);
>>> + if (minor < 0) {
>>> + pr_err("%s: No more minor numbers left! rc:%d\n", __func__,
>>> + minor);
>>> + return -ENODEV;
>>> + }
>>
>> How can you make the link between the chardev and the device instance ?
> I do this rproc->dev.devt = cdevt. Let me know of there is a better way
> to do this?
If this is sufficient to create a link in the sysfs, then it's ok but I'm
no expert here.
Clément
>> In our case, we have several remoteproc instances and thus we will end
>> up having multiple chardev.
>>
>> Regards,
>>
>> Clément
>>
> rproc_char_device_add will be called for each remoteproc that is
> added. So we will have one char dev per remoteproc.
>>> +
>>> + cdev_init(&rproc->char_dev, &rproc_fops);
>>> + rproc->char_dev.owner = THIS_MODULE;
>>> +
>>> + cdevt = MKDEV(MAJOR(rproc_cdev), minor);
>>> + ret = cdev_add(&rproc->char_dev, cdevt, 1);
>>> + if (ret < 0)
>>> + ida_simple_remove(&cdev_minor_ida, minor);
>>> +
>>> + rproc->dev.devt = cdevt;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +void __init rproc_init_cdev(void)
>>> +{
>>> + int ret;
>>> +
>>> + ret = alloc_chrdev_region(&rproc_cdev, 0, NUM_RPROC_DEVICES,
>>> "rproc");
>>> + if (ret < 0) {
>>> + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
>>> + return;
>>> + }
>>> +}
>>> +
>>> +void __exit rproc_exit_cdev(void)
>>> +{
>>> + unregister_chrdev_region(MKDEV(MAJOR(rproc_cdev), 0),
>>> + NUM_RPROC_DEVICES);
>>> +}
>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>> index 16ad666..c4ca796 100644
>>> --- a/include/linux/remoteproc.h
>>> +++ b/include/linux/remoteproc.h
>>> @@ -37,6 +37,7 @@
>>>
>>> #include <linux/types.h>
>>> #include <linux/mutex.h>
>>> +#include <linux/cdev.h>
>>> #include <linux/virtio.h>
>>> #include <linux/completion.h>
>>> #include <linux/idr.h>
>>> @@ -514,6 +515,7 @@ struct rproc {
>>> bool auto_boot;
>>> struct list_head dump_segments;
>>> int nb_vdev;
>>> + struct cdev char_dev;
>>> };
>>>
>>> /**
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum,
> >> a Linux Foundation Collaborative Project
