LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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()
Date: Thu, 7 Oct 2021 14:35:36 -0500	[thread overview]
Message-ID: <20211007193536.GA1260015@bhelgaas> (raw)
In-Reply-To: <YV7UFoAXb5MrkaFg@kuha.fi.intel.com>

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

  reply	other threads:[~2021-10-07 19:35 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-06 11:26 [PATCH v3 0/3] device property: Remove device_add_properties() Heikki Krogerus
2021-10-06 11:26 ` [PATCH v3 1/3] PCI: Convert to device_create_managed_software_node() Heikki Krogerus
2021-10-06 18:47   ` Bjorn Helgaas
2021-10-07 11:03     ` Heikki Krogerus
2021-10-07 19:35       ` Bjorn Helgaas [this message]
2021-10-06 11:26 ` [PATCH v3 2/3] driver core: Don't call device_remove_properties() from device_del() Heikki Krogerus
2021-10-06 11:26 ` [PATCH v3 3/3] device property: Remove device_add_properties() API Heikki Krogerus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211007193536.GA1260015@bhelgaas \
    --to=helgaas@kernel.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=zhangfei.gao@linaro.org \
    --subject='Re: [PATCH v3 1/3] PCI: Convert to device_create_managed_software_node()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).