Date:   Fri, 23 Apr 2021 14:48:50 -0700
From:   Siddharth Gupta <sidgup@...eaurora.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Mathieu Poirier <mathieu.poirier@...aro.org>, ohad@...ery.com,
        linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        psodagud@...eaurora.org, eberman@...eaurora.org
Subject: Re: [PATCH] remoteproc: core: Move cdev add before device add


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

Powered by blists - more mailing lists