Re: [PATCH] remoteproc: core: Move cdev add before device add

[Siddharth Gupta]

On 4/22/2021 8:01 PM, Bjorn Andersson wrote:
> On Thu 22 Apr 21:12 CDT 2021, Siddharth Gupta wrote:
>
> > On 4/22/2021 11:04 AM, Mathieu Poirier wrote:
> > > 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.
> > Okay sure, I will remove it.
> > > > 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.
> > Not sure what you mean here Mathieu? The reason why we can't use
> > it is because cdev is an optional feature. We would either have to move
> > device_add inside rproc_char_dev_add or the other way around and
> > make cdev a regular feature. Since device_add can't be called on the
> > same device struct twice[1], we have to do things this way. Also this
> > way we don't have to rely on the userspace to call mknod as it will
> > be called[2] as a part of the device_add call in devtmpfs_create_node.
> >
> > Now that I think about it, is the above what you want me to put in the
> > commit text? :)
> Your patch is correct, we need to cdev_add() and in particular assign
> dev->devt before calling device_add(). Given how the code is split in
> core and cdev there's no way to use cdev_device_add(), but I don't think
> anyone is suggesting that - except for your commit message.
>
> So while everything you mention in your commit message seems correct,
> you should be able to make it more to the point by distilling it down to
> something like:
>
>    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.
Okay, I will make the appropriate changes. Thanks!
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/core.c#n3105
> > [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/devtmpfs.c#n215
> > > > 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?
> > It is a sporadic issue due to a race between the userspace uevent handler
> > and cdev_add. If the uevent for the device is received/processed after
> > cdev_add the cdev is created.
> >
> > If "add" is written to the uevent file or mknod is manually called for devt
> > the cdev works as expected, just that the sysfs links won't be created.
> So it works for e.g. systemd based systems (most of the time), while in
> a system based on devtmpfs the dev node would never show up.
Yes exactly! We were just lucky till now :)

Thanks,
Sid
> Regards,
> Bjorn
>
> > > >    	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.
> > Sure. I'll make those changes.
> >
> > Thanks,
> > Sid
> > > 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