Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH 0/3] Fix errors on DT overlay removal with devlinks
To:     Michael Auchter <michael.auchter@ni.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        saravanak@google.com
Cc:     robh+dt@kernel.org, gregkh@linuxfoundation.org, rafael@kernel.org
References: <20201014193615.1045792-1-michael.auchter@ni.com>
From:   Frank Rowand <frowand.list@gmail.com>
Message-ID: <d5f8a417-8db9-9385-dfea-9512b4680124@gmail.com>
Date:   Wed, 21 Oct 2020 16:02:41 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20201014193615.1045792-1-michael.auchter@ni.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Saravana,

Michael found an issue related to the removal of a devicetree node
which involves devlinks:

On 10/14/20 2:36 PM, Michael Auchter wrote:
> After updating to v5.9, I've started seeing errors in the kernel log
> when using device tree overlays. Specifically, the problem seems to
> happen when removing a device tree overlay that contains two devices
> with some dependency between them (e.g., a device that provides a clock
> and a device that consumes that clock). Removing such an overlay results
> in:
> 
>   OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy
>   OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy
> 
> followed by hitting some REFCOUNT_WARNs in refcount.c
> 
> In the first patch, I've included a unittest that can be used to
> reproduce this when built with CONFIG_OF_UNITTEST [1].
> 
> I believe the issue is caused by the cleanup performed when releasing
> the devlink device that's created to represent the dependency between
> devices. The devlink device has references to the consumer and supplier
> devices, which it drops in device_link_free; the devlink device's
> release callback calls device_link_free via call_srcu.
> 
> When the overlay is being removed, all devices are removed, and
> eventually the release callback for the devlink device run, and
> schedules cleanup using call_srcu. Before device_link_free can and call
> put_device on the consumer/supplier, the rest of the overlay removal
> process runs, resulting in the error traces above.

When a devicetree node in an overlay is removed, the remove code expects
all previous users of the related device to have done the appropriate put
of the device and to have no later references.

As Michael described above, the devlink release callback defers the
put_device().  The cleanup via srcu was implemented in commit
843e600b8a2b01463c4d873a90b2c2ea8033f1f6 "driver core: Fix sleeping
in invalid context during device link deletion" to solve yet another
issue.


> 
> Patches 2 and 3 are an attempt at fixing this: call srcu_barrier to wait
> for any pending device_link_free's to execute before continuing on with
> the removal process.
> 
> These patches resolve the issue, but probably not in the best way. In
> particular, it seems strange to need to leak details of devlinks into
> the device tree overlay code. So, I'd be curious to get some feedback or
> hear any other ideas for how to resolve this issue.

I agree with Michael that adding an indirect call of srcu_barrier(&device_links_srcu)
into the devicetree overlay code is not an appropriate solution.

Is there some other way to fix the problem that 843e600b8a2b solves without
deferring the put_device() done by the devlink release callback?

-Frank

> 
> Thanks,
>  Michael
> 
> 1. Note that this isn't a very good unit test: it will report a "pass"
>    even if it fails with the aforementioned errors, as these errors
>    aren't propogated.
> 
> Michael Auchter (3):
>   of: unittest: add test of overlay with devlinks
>   driver core: add device_links_barrier
>   of: dynamic: add device links barrier before detach
> 
>  drivers/base/core.c                     | 10 ++++++++++
>  drivers/of/dynamic.c                    |  3 +++
>  drivers/of/unittest-data/Makefile       |  1 +
>  drivers/of/unittest-data/overlay_16.dts | 26 +++++++++++++++++++++++++
>  drivers/of/unittest.c                   | 16 +++++++++++++++
>  include/linux/device.h                  |  1 +
>  6 files changed, 57 insertions(+)
>  create mode 100644 drivers/of/unittest-data/overlay_16.dts
>