diff mbox series

[v3,1/4] remoteproc: core: Move cdev add before device add

Message ID 1623723671-5517-2-git-send-email-sidgup@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series remoteproc: core: Fixes for rproc cdev and add | expand

Commit Message

Siddharth Gupta June 15, 2021, 2:21 a.m. UTC
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.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Greg KH June 15, 2021, 4:54 a.m. UTC | #1
On Mon, Jun 14, 2021 at 07:21:08PM -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.
> 
> 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.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>
Greg KH June 15, 2021, 4:56 a.m. UTC | #2
On Mon, Jun 14, 2021 at 07:21:08PM -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.
> 
> 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.

So this means no one ever ran this code on a system that used devtmpfs?

How was it ever tested?
Siddharth Gupta June 15, 2021, 7:03 p.m. UTC | #3
On 6/14/2021 9:56 PM, Greg KH wrote:
> On Mon, Jun 14, 2021 at 07:21:08PM -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.
>>
>> 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.
> So this means no one ever ran this code on a system that used devtmpfs?
>
> How was it ever tested?
My testing was done with toybox + Android's ueventd ramdisk.
As I mentioned in the discussion, the race became evident
recently. I will make sure to test all such changes without
systemd/ueventd in the future.

Thanks,
Sid
Greg KH June 16, 2021, 5:58 a.m. UTC | #4
On Tue, Jun 15, 2021 at 12:03:26PM -0700, Siddharth Gupta wrote:
> 
> On 6/14/2021 9:56 PM, Greg KH wrote:
> > On Mon, Jun 14, 2021 at 07:21:08PM -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.
> > > 
> > > 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.
> > So this means no one ever ran this code on a system that used devtmpfs?
> > 
> > How was it ever tested?
> My testing was done with toybox + Android's ueventd ramdisk.
> As I mentioned in the discussion, the race became evident
> recently. I will make sure to test all such changes without
> systemd/ueventd in the future.

It isn't an issue of systemd/ueventd, those do not control /dev on a
normal system, that is what devtmpfs is for.

And devtmpfs nodes are only created if you create a struct device
somewhere with a proper major/minor, which you were not doing here, so
you must have had a static /dev on your test systems, right?

thanks,

greg k-h
Siddharth Gupta June 16, 2021, 6:47 p.m. UTC | #5
On 6/15/2021 10:58 PM, Greg KH wrote:
> On Tue, Jun 15, 2021 at 12:03:26PM -0700, Siddharth Gupta wrote:
>> On 6/14/2021 9:56 PM, Greg KH wrote:
>>> On Mon, Jun 14, 2021 at 07:21:08PM -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.
>>>>
>>>> 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.
>>> So this means no one ever ran this code on a system that used devtmpfs?
>>>
>>> How was it ever tested?
>> My testing was done with toybox + Android's ueventd ramdisk.
>> As I mentioned in the discussion, the race became evident
>> recently. I will make sure to test all such changes without
>> systemd/ueventd in the future.
> It isn't an issue of systemd/ueventd, those do not control /dev on a
> normal system, that is what devtmpfs is for.
I am not fully aware of when devtmpfs is enabled or not, but in
case it is not - systemd/ueventd will create these files with
mknod, right? I was even manually able to call mknod from the
terminal when some of the remoteproc character device entries
showed up (using major number from there, and minor number being
the remoteproc id), and that allowed me to boot up the
remoteprocs as well.
>
> And devtmpfs nodes are only created if you create a struct device
> somewhere with a proper major/minor, which you were not doing here, so
> you must have had a static /dev on your test systems, right?
I am not sure of what you mean by a static /dev? Could you
explain? In case you mean the character device would be
non-functional, that is not the case. They have been working
for us since the beginning.

Thanks,
Sid
>
> thanks,
>
> greg k-h
Greg KH June 23, 2021, 7:27 a.m. UTC | #6
On Wed, Jun 16, 2021 at 11:47:01AM -0700, Siddharth Gupta wrote:
> 
> On 6/15/2021 10:58 PM, Greg KH wrote:
> > On Tue, Jun 15, 2021 at 12:03:26PM -0700, Siddharth Gupta wrote:
> > > On 6/14/2021 9:56 PM, Greg KH wrote:
> > > > On Mon, Jun 14, 2021 at 07:21:08PM -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.
> > > > > 
> > > > > 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.
> > > > So this means no one ever ran this code on a system that used devtmpfs?
> > > > 
> > > > How was it ever tested?
> > > My testing was done with toybox + Android's ueventd ramdisk.
> > > As I mentioned in the discussion, the race became evident
> > > recently. I will make sure to test all such changes without
> > > systemd/ueventd in the future.
> > It isn't an issue of systemd/ueventd, those do not control /dev on a
> > normal system, that is what devtmpfs is for.
> I am not fully aware of when devtmpfs is enabled or not, but in
> case it is not - systemd/ueventd will create these files with
> mknod, right?

No, systemd does not create device nodes, and neither does udev.  Hasn't
done so for well over 10 years now.

> I was even manually able to call mknod from the
> terminal when some of the remoteproc character device entries
> showed up (using major number from there, and minor number being
> the remoteproc id), and that allowed me to boot up the
> remoteprocs as well.

Yes, that is fine, but that also means that this was not working from
the very beginning :(

> > And devtmpfs nodes are only created if you create a struct device
> > somewhere with a proper major/minor, which you were not doing here, so
> > you must have had a static /dev on your test systems, right?
> I am not sure of what you mean by a static /dev? Could you
> explain? In case you mean the character device would be
> non-functional, that is not the case. They have been working
> for us since the beginning.

/dev on modern systems is managed by devtmpfs, which knows to create the
device nodes when you properly register the device with the driver core.
A "static" /dev is managed by mknod from userspace, like you did "by
hand", and that is usually only done by older systems.

thanks,

greg k-h
Siddharth Gupta June 23, 2021, 6:56 p.m. UTC | #7
On 6/23/2021 12:27 AM, Greg KH wrote:
> On Wed, Jun 16, 2021 at 11:47:01AM -0700, Siddharth Gupta wrote:
>> On 6/15/2021 10:58 PM, Greg KH wrote:
>>> On Tue, Jun 15, 2021 at 12:03:26PM -0700, Siddharth Gupta wrote:
>>>> On 6/14/2021 9:56 PM, Greg KH wrote:
>>>>> On Mon, Jun 14, 2021 at 07:21:08PM -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.
>>>>>>
>>>>>> 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.
>>>>> So this means no one ever ran this code on a system that used devtmpfs?
>>>>>
>>>>> How was it ever tested?
>>>> My testing was done with toybox + Android's ueventd ramdisk.
>>>> As I mentioned in the discussion, the race became evident
>>>> recently. I will make sure to test all such changes without
>>>> systemd/ueventd in the future.
>>> It isn't an issue of systemd/ueventd, those do not control /dev on a
>>> normal system, that is what devtmpfs is for.
>> I am not fully aware of when devtmpfs is enabled or not, but in
>> case it is not - systemd/ueventd will create these files with
>> mknod, right?
> No, systemd does not create device nodes, and neither does udev.  Hasn't
> done so for well over 10 years now.
Oh okay. I thought ueventd does it because it allows setting
the node permissions through ueventd.rc:
https://android.googlesource.com/platform/system/core/+/master/rootdir/ueventd.rc
>
>> I was even manually able to call mknod from the
>> terminal when some of the remoteproc character device entries
>> showed up (using major number from there, and minor number being
>> the remoteproc id), and that allowed me to boot up the
>> remoteprocs as well.
> Yes, that is fine, but that also means that this was not working from
> the very beginning :(
Right. To clarify, I did this after we started seeing the problem
on our devices, which led me to believe there was a race between
ueventd and cdev_add(). Not sure anymore if that is not the case.
>
>>> And devtmpfs nodes are only created if you create a struct device
>>> somewhere with a proper major/minor, which you were not doing here, so
>>> you must have had a static /dev on your test systems, right?
>> I am not sure of what you mean by a static /dev? Could you
>> explain? In case you mean the character device would be
>> non-functional, that is not the case. They have been working
>> for us since the beginning.
> /dev on modern systems is managed by devtmpfs, which knows to create the
> device nodes when you properly register the device with the driver core.
> A "static" /dev is managed by mknod from userspace, like you did "by
> hand", and that is usually only done by older systems.
Thanks for the explanation! As I mentioned earlier - I was under
the impression that ueventd does it. I will go through our older
builds where this was working (without this patch) and try to see
how the dev nodes were being populated.

Thanks,
Sid
> thanks,
>
> greg k-h
diff mbox series

Patch

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6348aaa..9ad8c5f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2333,6 +2333,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;
@@ -2346,11 +2351,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);