Linux-remoteproc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] remoteproc: core: Move cdev add before device add
@ 2021-04-20 19:26 Siddharth Gupta
  2021-04-22 18:04 ` Mathieu Poirier
  0 siblings, 1 reply; 5+ messages in thread
From: Siddharth Gupta @ 2021-04-20 19:26 UTC (permalink / raw)
  To: bjorn.andersson, ohad, linux-remoteproc, linux-kernel
  Cc: Siddharth Gupta, psodagud, eberman

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
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. 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@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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] remoteproc: core: Move cdev add before device add
  2021-04-20 19:26 [PATCH] remoteproc: core: Move cdev add before device add Siddharth Gupta
@ 2021-04-22 18:04 ` Mathieu Poirier
  2021-04-23  2:12   ` Siddharth Gupta
  0 siblings, 1 reply; 5+ messages in thread
From: Mathieu Poirier @ 2021-04-22 18:04 UTC (permalink / raw)
  To: Siddharth Gupta
  Cc: bjorn.andersson, ohad, linux-remoteproc, linux-kernel, psodagud, eberman

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@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;
> +

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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] remoteproc: core: Move cdev add before device add
  2021-04-22 18:04 ` Mathieu Poirier
@ 2021-04-23  2:12   ` Siddharth Gupta
  2021-04-23  3:01     ` Bjorn Andersson
  0 siblings, 1 reply; 5+ messages in thread
From: Siddharth Gupta @ 2021-04-23  2:12 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: bjorn.andersson, ohad, linux-remoteproc, linux-kernel, psodagud, eberman


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? :)

[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@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;
>> +
> 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.
>
>>   	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
>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] remoteproc: core: Move cdev add before device add
  2021-04-23  2:12   ` Siddharth Gupta
@ 2021-04-23  3:01     ` Bjorn Andersson
  2021-04-23 21:48       ` Siddharth Gupta
  0 siblings, 1 reply; 5+ messages in thread
From: Bjorn Andersson @ 2021-04-23  3:01 UTC (permalink / raw)
  To: Siddharth Gupta
  Cc: Mathieu Poirier, ohad, linux-remoteproc, linux-kernel, psodagud, eberman

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.

> [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@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;
> > > +
> > 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.

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
> > > 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] remoteproc: core: Move cdev add before device add
  2021-04-23  3:01     ` Bjorn Andersson
@ 2021-04-23 21:48       ` Siddharth Gupta
  0 siblings, 0 replies; 5+ messages in thread
From: Siddharth Gupta @ 2021-04-23 21:48 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Mathieu Poirier, ohad, linux-remoteproc, linux-kernel, psodagud, eberman


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@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;
>>>> +
>>> 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
>>>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-20 19:26 [PATCH] remoteproc: core: Move cdev add before device add Siddharth Gupta
2021-04-22 18:04 ` Mathieu Poirier
2021-04-23  2:12   ` Siddharth Gupta
2021-04-23  3:01     ` Bjorn Andersson
2021-04-23 21:48       ` Siddharth Gupta

Linux-remoteproc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-remoteproc/0 linux-remoteproc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-remoteproc linux-remoteproc/ https://lore.kernel.org/linux-remoteproc \
		linux-remoteproc@vger.kernel.org
	public-inbox-index linux-remoteproc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-remoteproc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git