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

[Laurentiu Tudor <laurentiu.tudor@nxp.com>]

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@nxp.com> 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@linux.intel.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, Jul 16, 2021 at 01:16:02PM +0300, laurentiu.tudor@nxp.com wrote:
>>>>>>>>>>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>>>>>>>>>>
>>>>>>>>>>> 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.

[1]
[   11.760346] sysfs: cannot create duplicate filename
'/devices/platform/808622B7:01/xhci-hcd.2.auto/software_node'
[   11.770612] CPU: 9 PID: 1 Comm: swapper/0 Tainted: G        W
 5.14.0-rc1-00214-gbf7f1083ebd3-dirty #62
[   11.780611] Hardware name: NXP NXP LX2160ARDB Platform, BIOS EDK II
Apr 16 2021
[   11.787913] Call trace:
[   11.790351]  dump_backtrace+0x0/0x2a4
[   11.794017]  show_stack+0x1c/0x30
[   11.797331]  dump_stack_lvl+0x68/0x84
[   11.800991]  dump_stack+0x20/0x3c
[   11.804302]  sysfs_warn_dup+0x88/0xac
[   11.807965]  sysfs_do_create_link_sd+0xf8/0x100
[   11.812492]  sysfs_create_link+0x48/0x80
[   11.816411]  software_node_notify+0x1a8/0x35c
[   11.820769]  device_create_managed_software_node+0x158/0x1b0
[   11.826428]  iort_named_component_init+0xe0/0x140
[   11.831131]  iort_iommu_configure_id+0xf4/0x270
[   11.835660]  acpi_dma_configure_id+0x160/0x200
[   11.840101]  platform_dma_configure+0xa0/0xa4
[   11.844457]  really_probe.part.0+0x84/0x480
[   11.848639]  __driver_probe_device+0xd4/0x180
[   11.852994]  driver_probe_device+0xf8/0x1e0
[   11.857174]  __driver_attach+0x108/0x220
[   11.861095]  bus_for_each_dev+0xe4/0x154
[   11.865014]  driver_attach+0x38/0x50
[   11.868587]  bus_add_driver+0x1bc/0x2c4
[   11.872419]  driver_register+0xf0/0x210
[   11.876253]  __platform_driver_register+0x48/0x60
[   11.880956]  xhci_plat_init+0x34/0x40
[   11.884616]  do_one_initcall+0xa8/0x270
[   11.888449]  kernel_init_freeable+0x2c0/0x348
[   11.892806]  kernel_init+0x28/0x140
[   11.896295]  ret_from_fork+0x10/0x18
[   11.900062] xhci-hcd xhci-hcd.2.auto: Adding to iommu group 6
[   11.906044] xhci-hcd xhci-hcd.2.auto: xHCI Host Controller
[   11.911566] xhci-hcd xhci-hcd.2.auto: new USB bus registered,
assigned bus number 3
[   11.919702] xhci-hcd xhci-hcd.2.auto: hcc params 0x0220f66d hci
version 0x100 quirks 0x0000000000010010
[   11.929187] xhci-hcd xhci-hcd.2.auto: irq 106, io mem 0x03110000

---
Best Regards, Laurentiu