[PATCH v3 0/3] device property: Remove device_add_properties()

[PATCH v3 0/3] device property: Remove device_add_properties()

[Heikki Krogerus]

Hi,

In this third version of this series, the second patch is now split in
two. The device_remove_properties() call is first removed from
device_del() in its own patch, and the
device_add/remove_properties() API is removed separately in the last
patch. I hope the commit messages are clear enough this time.


v2 cover letter:

This is the second version where I only modified the commit message of
the first patch according to comments from Bjorn.


Original cover letter:

There is one user left for the API, so converting that to use software
node API instead, and removing the function.


Heikki Krogerus (3):
  PCI: Convert to device_create_managed_software_node()
  driver core: Don't call device_remove_properties() from device_del()
  device property: Remove device_add_properties() API

 drivers/base/core.c      |  1 -
 drivers/base/property.c  | 48 ----------------------------------------
 drivers/pci/quirks.c     |  2 +-
 include/linux/property.h |  4 ----
 4 files changed, 1 insertion(+), 54 deletions(-)

-- 
2.33.0

[PATCH v3 1/3] PCI: Convert to device_create_managed_software_node()

[Heikki Krogerus]

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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/pci/quirks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b6b4c803bdc94..fe5eedba47908 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1850,7 +1850,7 @@ static void quirk_huawei_pcie_sva(struct pci_dev *pdev)
 	 * can set it directly.
 	 */
 	if (!pdev->dev.of_node &&
-	    device_add_properties(&pdev->dev, properties))
+	    device_create_managed_software_node(&pdev->dev, properties, NULL))
 		pci_warn(pdev, "could not add stall property");
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);
-- 
2.33.0

Re: [PATCH v3 1/3] PCI: Convert to device_create_managed_software_node()

[Bjorn Helgaas]

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.

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/pci/quirks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index b6b4c803bdc94..fe5eedba47908 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -1850,7 +1850,7 @@ static void quirk_huawei_pcie_sva(struct pci_dev *pdev)
>  	 * can set it directly.
>  	 */
>  	if (!pdev->dev.of_node &&
> -	    device_add_properties(&pdev->dev, properties))
> +	    device_create_managed_software_node(&pdev->dev, properties, NULL))
>  		pci_warn(pdev, "could not add stall property");
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);
> -- 
> 2.33.0
> 

Re: [PATCH v3 1/3] PCI: Convert to device_create_managed_software_node()

[Heikki Krogerus]

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().

        SoB...

thanks,

-- 
heikki

Re: [PATCH v3 1/3] PCI: Convert to device_create_managed_software_node()

[Bjorn Helgaas]

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

[PATCH v3 2/3] driver core: Don't call device_remove_properties() from device_del()

[Heikki Krogerus]

All the drivers that relied on device_del() to call
device_remove_properties() have now been converted to either
use device_create_managed_software_node() instead of
device_add_properties(), or to register the software node
completely separately from the device.

This will make it finally possible to share and reuse the
software nodes that hold the additional device properties.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index c4a2c97a21a2b..ea39ffad11179 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3584,7 +3584,6 @@ void device_del(struct device *dev)
 	device_pm_remove(dev);
 	driver_deferred_probe_del(dev);
 	device_platform_notify_remove(dev);
-	device_remove_properties(dev);
 	device_links_purge(dev);
 
 	if (dev->bus)
-- 
2.33.0

[PATCH v3 3/3] device property: Remove device_add_properties() API

[Heikki Krogerus]

There are no more users for it.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/property.c  | 48 ----------------------------------------
 include/linux/property.h |  4 ----
 2 files changed, 52 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 453918eb7390c..1f1eee37817e0 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -508,54 +508,6 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_find_reference);
 
-/**
- * device_remove_properties - Remove properties from a device object.
- * @dev: Device whose properties to remove.
- *
- * The function removes properties previously associated to the device
- * firmware node with device_add_properties(). Memory allocated to the
- * properties will also be released.
- */
-void device_remove_properties(struct device *dev)
-{
-	struct fwnode_handle *fwnode = dev_fwnode(dev);
-
-	if (!fwnode)
-		return;
-
-	if (is_software_node(fwnode->secondary)) {
-		fwnode_remove_software_node(fwnode->secondary);
-		set_secondary_fwnode(dev, NULL);
-	}
-}
-EXPORT_SYMBOL_GPL(device_remove_properties);
-
-/**
- * device_add_properties - Add a collection of properties to a device object.
- * @dev: Device to add properties to.
- * @properties: Collection of properties to add.
- *
- * Associate a collection of device properties represented by @properties with
- * @dev. The function takes a copy of @properties.
- *
- * WARNING: The callers should not use this function if it is known that there
- * is no real firmware node associated with @dev! In that case the callers
- * should create a software node and assign it to @dev directly.
- */
-int device_add_properties(struct device *dev,
-			  const struct property_entry *properties)
-{
-	struct fwnode_handle *fwnode;
-
-	fwnode = fwnode_create_software_node(properties, NULL);
-	if (IS_ERR(fwnode))
-		return PTR_ERR(fwnode);
-
-	set_secondary_fwnode(dev, fwnode);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(device_add_properties);
-
 /**
  * fwnode_get_name - Return the name of a node
  * @fwnode: The firmware node
diff --git a/include/linux/property.h b/include/linux/property.h
index 357513a977e5d..daf0b5841286f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -377,10 +377,6 @@ property_entries_dup(const struct property_entry *properties);
 
 void property_entries_free(const struct property_entry *properties);
 
-int device_add_properties(struct device *dev,
-			  const struct property_entry *properties);
-void device_remove_properties(struct device *dev);
-
 bool device_dma_supported(struct device *dev);
 
 enum dev_dma_attr device_get_dma_attr(struct device *dev);
-- 
2.33.0