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;
Date:   Thu, 7 Oct 2021 14:35:36 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Zhangfei Gao <zhangfei.gao@linaro.org>,
        linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH v3 1/3] PCI: Convert to
 device_create_managed_software_node()
Message-ID: <20211007193536.GA1260015@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YV7UFoAXb5MrkaFg@kuha.fi.intel.com>
Precedence: bulk

On Thu, Oct 07, 2021 at 02:03:50PM +0300, Heikki Krogerus wrote:
> On Wed, Oct 06, 2021 at 01:47:54PM -0500, Bjorn Helgaas wrote:
> > On Wed, Oct 06, 2021 at 02:26:41PM +0300, Heikki Krogerus wrote:
> > > In quirk_huawei_pcie_sva(), use device_create_managed_software_node()
> > > instead of device_add_properties() to set the "dma-can-stall"
> > > property.
> > > 
> > > This is the last user of device_add_properties() that relied on
> > > device_del() to take care of also calling device_remove_properties().
> > > After this change we can finally get rid of that
> > > device_remove_properties() call in device_del().
> > > 
> > > After that device_remove_properties() call has been removed from
> > > device_del(), the software nodes that hold the additional device
> > > properties become reusable and shareable as there is no longer a
> > > default assumption that those nodes are lifetime bound the first
> > > device they are associated with.
> > 
> > This does not help me determine whether this patch is safe.
> > device_create_managed_software_node() sets swnode->managed = true,
> > but device_add_properties() did not.  I still don't know what the
> > effect of that is.
> 
> OK. So how about this:
> 
>         PCI: Convert to device_create_managed_software_node()
> 
>         In quirk_huawei_pcie_sva(), device_add_properties() is used to
>         inject additional device properties, but there is no
>         device_remove_properties() call anywhere to remove those
>         properties. The assumption is most likely that the device is
>         never removed, and the properties therefore do not also never
>         need to be removed.
> 
>         Even though it is unlikely that the device is ever removed in
>         this case, it is safer to make sure that the properties are
>         also removed if the device ever does get unregistered.
> 
>         To achieve this, instead of adding a separate quirk for the
>         case of device removal where device_remove_properties() is
>         called, using device_create_managed_software_node() instead of
>         device_add_properties(). Both functions create a software node
>         (a type of fwnode) that holds the device properties, which is
>         then assigned to the device very much the same way.
> 
>         The difference between the two functions is, that
>         device_create_managed_software_node() guarantees that the
>         software node (together with the properties) is removed when
>         the device is removed. The function device_add_property() does
>         _not_ guarantee that, so the properties added with it should
>         always be removed with device_remove_properties().

That makes sense to me, thanks.  And it sounds like it *does* resolve
a lifetime issue, namely, a caller of device_add_properties(dev) is
required to arrange for device_remove_properties(dev) to be called
when "dev" is removed.

The fact that in this particular case, "dev" is a non-removable AMBA
device doesn't mean there was no issue; it only means we should have
had a matching device_remove_properties() call somewhere or at the
very least a comment about why it wasn't needed.  Otherwise people
copy the code to somewhere where it *does* matter.

But removing device_add_properties() altogether will mean this is all
moot anyway.

You can add my:

  Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Bjorn