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