Re: [PATCH] software node: balance refcount for managed sw nodes

[Laurentiu Tudor]

On 9/9/2021 5:01 PM, Laurentiu Tudor wrote:
> 
> 
> On 9/9/2021 3:16 PM, Heikki Krogerus wrote:
>> On Thu, Sep 09, 2021 at 03:13:47PM +0300, Heikki Krogerus wrote:
>>> On Tue, Sep 07, 2021 at 06:59:18PM +0300, Laurentiu Tudor wrote:
>>>>
>>>>
>>>> On 7/26/2021 10:59 AM, Laurentiu Tudor wrote:
>>>>>
>>>>>
>>>>> On 7/20/2021 1:27 PM, Andy Shevchenko wrote:
>>>>>> On Tue, Jul 20, 2021 at 12:22 PM Laurentiu Tudor
>>>>>> <laurentiu.tudor@xxxxxxx> wrote:
>>>>>>> On 7/19/2021 3:22 PM, Andy Shevchenko wrote:
>>>>>>>> On Mon, Jul 19, 2021 at 03:00:17PM +0300, Laurentiu Tudor wrote:
>>>>>>>>> On 7/16/2021 8:21 PM, Jon Nettleton wrote:
>>>>>>>>>> On Fri, Jul 16, 2021 at 2:17 PM Andy Shevchenko
>>>>>>>>>> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Jul 16, 2021 at 01:16:02PM +0300, laurentiu.tudor@xxxxxxx wrote:
>>>>>>>>>>>> From: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
>>>>>>>>>>>>
>>>>>>>>>>>> software_node_notify(), on KOBJ_REMOVE drops the refcount twice on managed
>>>>>>>>>>>> software nodes, thus leading to underflow errors. Balance the refcount by
>>>>>>>>>>>> bumping it in the device_create_managed_software_node() function.
>>>>>>>>>>>>
>>>>>>>>>>>> The error [1] was encountered after adding a .shutdown() op to our
>>>>>>>>>>>> fsl-mc-bus driver.
>>>>>>>>>>>
>>>>>>>>>>> Looking into the history of adding ->shutdown() to dwc3 driver (it got reverted
>>>>>>>>>>> later on), I can tell that probably something is wrong in the ->shutdown()
>>>>>>>>>>> method itself.
>>>>>>>>>>
>>>>>>>>>> Isn't the other alternative to just remove the second kobject_put from
>>>>>>>>>> KOBJ_REMOVE ?
>>>>>>>>>
>>>>>>>>> Or maybe on top of Heikki's suggestion, replace the calls to
>>>>>>>>> sysfs_create_link() from KOBJ_ADD with sysfs_create_link_nowarn()?
>>>>>>>>
>>>>>>>> _noearn will hide the problem. It was there, it was removed from there.
>>>>>>>> Perhaps we have to understand the root cause better (some specific flow?).
>>>>>>>>
>>>>>>>> Any insight from you on the flow when the issue appears? I.o.w. what happened
>>>>>>>> on the big picture that we got into the warning you see?
>>>>>>>
>>>>>>> I encountered the initial issue when trying to shut down a system booted
>>>>>>> with ACPI but only after adding a .shutdown() callback to our bus driver
>>>>>>> so that the devices are properly taken down. The problem was that
>>>>>>> software_node_notify(), on KOBJ_REMOVE was dropping the reference count
>>>>>>> twice leading to an underflow error. My initial proposal was to just
>>>>>>> bump the refcount in device_create_managed_software_node(). The device
>>>>>>> properties that triggered the problem are created here [1].
>>>>>>>
>>>>>>> Heikko suggested that instead of manually incrementing the refcount to
>>>>>>> use software_node_notify(KOBJ_ADD). This triggered the second issue, a
>>>>>>> duplicated sysfs entry warning originating in the usb subsystem:
>>>>>>> device_create_managed_software_node() ends up being called twice, once
>>>>>>> here [2] and secondly, the place I previous mentioned [1].
>>>>>>
>>>>>> This [3] is what I have reported against DWC3 when ->shutdown() has
>>>>>> been added there. And here [4] is another thread about the issue with
>>>>>> that callback. The ->release() callback is called at put_device() [5]
>>>>>> and ->shutdown() is called before that [6]. That said, can you inspect
>>>>>> your ->shutdown() implementation once more time and perhaps see if
>>>>>> there is anything that can be amended?
>>>>>>
>>>>>
>>>>> Will do, thanks for the pointers. It could be that we mess something out
>>>>> in how we use the driver model.
>>>>>
>>>>
>>>> Quick (and late, sorry) update from my side. I've spent time on
>>>> debugging our bus, did found some issues but, at least for now, none are
>>>> related to sw node.
>>>> In the mean time, I noticed in the swnode code that
>>>> device_add_software_node() calls software_node_notify(KOBJ_ADD) while
>>>> device_create_managed_software_node() doesn't. Updating [1] the later
>>>> with the call to software_node_notify(KOBJ_ADD) does seem to fix the
>>>> issue I'm seeing.
>>>>
>>>> Could this be a problem? Any comments appreciated.
>>>>
>>>> One more thing perhaps worth mentioning is that, at least for now, there
>>>> are few uses for this device_create_managed_software_node() api,
>>>> mentioning here a couple of them:
>>>>  - arm64 iort code - this seems to be triggering the issue i'm getting
>>>>  - dwc3 usb - Andy reported similar issues here, maybe the issue is common?
>>>>
>>>> [1]
>>>> @@ -1113,6 +1125,15 @@ int device_create_managed_software_node(struct
>>>> device *dev,
>>>>         to_swnode(fwnode)->managed = true;
>>>>         set_secondary_fwnode(dev, fwnode);
>>>>
>>>> +       /*
>>>> +        * If the device has been fully registered by the time this
>>>> function is
>>>> +        * called, software_node_notify() must be called separately so
>>>> that the
>>>> +        * symlinks get created and the reference count of the node is
>>>> kept in
>>>> +        * balance.
>>>> +        */
>>>> +       if (device_is_registered(dev))
>>>> +               software_node_notify(dev, KOBJ_ADD);
>>>> +
>>>>         return 0;
>>>>  }
>>>
>>> That should be fixed indeed. Please send that after -rc1 is out.
>>
>> I mean, resend :-)
>>
> 
> Right, actually I just noticed that this is the fix you suggested last
> time we discussed. :-) I forgot about it, sorry.
> There's still the WARN_ON() [1] triggered by the usb subsys, apparently
> happening only (!) in ACPI boot scenario, so +Lorenzo.
> I'll delay the sending a bit to try to understand what's going on.

I've spent some time looking into this and it  turns out that in the
ACPI case, device_create_managed_software_node() ends up being called
twice, first here [1] and after that, in the IORT code here [2]. With
the proposed patch this causes software_node_notify(KOBJ_ADD) being
called twice thus triggering the dup sysfs entry warning.
Any comments / ideas welcomed.

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/host.c#L111
[2]
https://elixir.bootlin.com/linux/latest/source/drivers/acpi/arm64/iort.c#L952

---
Best Regards, Laurentiu