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

[Heikki Krogerus]

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.

thanks,

-- 
heikki