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

Laurentiu Tudor
2021-09-09T14:02:33Z

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