Hi Siddharth, On Tue, Apr 20, 2021 at 12:26:45PM -0700, 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. > > Based on how cdev_device_add[1] is written, it appears that the correct Please don't add this kind of reference to the change log as it will become invalid with time. > way to use these APIs is to call cdev_add before device_add is called. > Since the cdev is an optional feature for remoteproc we cannot directly > use the existing API. Please explain why the existing API can't be used directly. > Hence moving rproc_char_device_add() before > device_add() in rproc_add(). > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/char_dev.c#n537 > > Signed-off-by: Siddharth Gupta <sidgup@xxxxxxxxxxxxxx> > --- > 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; > + I have tested this change and it works. So how did it work before? > 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; > - While reviewing this patch I had another look at rproc_add() and noticed it doesn't clean up after itself in case of failure. If any of the conditions aren't met the function returns but rproc_delete_debug_dir(), rproc_char_device_remove() and device_del() aren't called. Please fix that as part of your next revision. Thanks, Mathieu > /* 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 >
