On Mon 17 May 15:45 CDT 2021, Siddharth Gupta wrote:

When cdev_add is called after device_add has been called there is no way for the userspace to know about the addition of a cdev as cdev_add itself doesn't trigger a uevent notification, or for the kernel to know about the change to devt. This results in two problems:

• mknod is never called for the cdev and hence no cdev appears on devtmpfs.
• sysfs links to the new cdev are not established.

The cdev needs to be added and devt assigned before device_add() is called in order for the relevant sysfs and devtmpfs entries to be created and the uevent to be properly populated.

Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org

Regards, Bjorn

Signed-off-by: Siddharth Gupta sidgup@codeaurora.org

drivers/remoteproc/remoteproc_core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 626a6b90f..562355a 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2316,6 +2316,11 @@ int rproc_add(struct rproc *rproc) struct device *dev = &rproc->dev; int ret;

• /* add char device for this remoteproc */
• ret = rproc_char_device_add(rproc);
• if (ret < 0)

• return ret;
  

• ret = device_add(dev); if (ret < 0) return ret;

@@ -2329,11 +2334,6 @@ int rproc_add(struct rproc *rproc) /* create debugfs entries */ rproc_create_debug_dir(rproc);

• /* add char device for this remoteproc */
• ret = rproc_char_device_add(rproc);
• if (ret < 0)

• return ret;
  

• /* if rproc is marked always-on, request it to boot */ if (rproc->auto_boot) { ret = rproc_trigger_auto_boot(rproc);

-- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Mon 17 May 15:45 CDT 2021, Siddharth Gupta wrote:

We can validate whether the remoteproc is correctly setup before calling making the cdev_add and device_add calls. This saves us the trouble of cleaning up later on.

Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org

Signed-off-by: Siddharth Gupta sidgup@codeaurora.org

drivers/remoteproc/remoteproc_core.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 562355a..45d09bf 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2316,16 +2316,16 @@ int rproc_add(struct rproc *rproc) struct device *dev = &rproc->dev; int ret;

• /* add char device for this remoteproc */
• ret = rproc_char_device_add(rproc);

• ret = rproc_validate(rproc); if (ret < 0) return ret;

• ret = device_add(dev);

• /* add char device for this remoteproc */
• ret = rproc_char_device_add(rproc); if (ret < 0) return ret;

• ret = rproc_validate(rproc);

• ret = device_add(dev); if (ret < 0) return ret;

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Mon 17 May 15:45 CDT 2021, Siddharth Gupta wrote:

When a failure occurs in rproc_add() it returns an error, but does not cleanup after itself. This change adds the failure path in such cases.

Signed-off-by: Siddharth Gupta sidgup@codeaurora.org

drivers/remoteproc/remoteproc_core.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 45d09bf..6f5fa81 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2326,8 +2326,10 @@ int rproc_add(struct rproc *rproc) return ret; ret = device_add(dev);

• if (ret < 0)

• return ret;
  

• if (ret < 0) {

• put_device(dev);
  

• goto rproc_remove_cdev;
  

• }

dev_info(dev, "%s is available\n", rproc->name); @@ -2338,7 +2340,7 @@ int rproc_add(struct rproc *rproc) if (rproc->auto_boot) { ret = rproc_trigger_auto_boot(rproc); if (ret < 0)

• 	return ret;
  

• 	goto rproc_remove_dev;
  

  }

/* expose to rproc_get_by_phandle users */ @@ -2347,6 +2349,13 @@ int rproc_add(struct rproc *rproc) mutex_unlock(&rproc_list_mutex); return 0;

+rproc_remove_dev:

• rproc_delete_debug_dir(rproc);
• device_del(dev);

+rproc_remove_cdev:

• rproc_char_device_remove(rproc);

I'm confused, shouldn't this function just do cdev_del()? __unregister_chrdev() seems to do more than unroll what cdev_add() did...

Apart from this, I think the patch looks good. Really nice to see you tidy this up!

Regards, Bjorn

• return ret;

} EXPORT_SYMBOL(rproc_add); -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project