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

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

[Heikki Krogerus]

Hi,

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

thanks,

Heikki Krogerus (2):
  PCI: Use software node API with additional device properties
  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 1/2] PCI: Use software node API with additional device properties

[Heikki Krogerus]

Using device_create_managed_software_node() to inject the
properties in quirk_huawei_pcie_sva() instead of with the
old device_add_properties() API.

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

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

[Heikki Krogerus]

There are no more users for it.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/core.c      |  1 -
 drivers/base/property.c  | 48 ----------------------------------------
 include/linux/property.h |  4 ----
 3 files changed, 53 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7758223f040c8..7935ee642fa3f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3573,7 +3573,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)
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

Re: [PATCH 0/2] device property: Remove device_add_properties()

[Andy Shevchenko]

On Wed, Sep 29, 2021 at 5:18 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi,
>
> There is one user left for the API, so converting that to use software
> node API instead, and removing the function.

All look good to me, thank you for removing the old API!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> thanks,
>
> Heikki Krogerus (2):
>   PCI: Use software node API with additional device properties
>   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(-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] PCI: Use software node API with additional device properties

[Bjorn Helgaas]

[+cc Zhangfei, author of 8304a3a199ee ("PCI: Set dma-can-stall for
HiSilicon chips"), which added this]

On Wed, Sep 29, 2021 at 04:37:28PM +0300, Heikki Krogerus wrote:
> Using device_create_managed_software_node() to inject the
> properties in quirk_huawei_pcie_sva() instead of with the
> old device_add_properties() API.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This is fine with me, but please update the subject line and commit
log something like this:

  PCI: Convert to device_create_managed_software_node()

  In quirk_huawei_pcie_sva(), use device_create_managed_software_node()
  instead of device_add_properties() to set the "dma-can-stall"
  property.

  This resolves a software node lifetime issue (see 151f6ff78cdf
  ("software node: Provide replacement for device_add_properties()"))
  and paves the way for removing device_add_properties() completely.

Actually, 8304a3a199ee was merged during the v5.15 merge window, so if
this does in fact fix a lifetime issue, I can merge this before
v5.15-final.

I know *this* quirk applies to AMBA devices, and I assume they cannot
be removed, so there's no actual lifetime problem in this particular
case, but in general it looks like a problem for PCI devices.

> ---
>  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 1/2] PCI: Use software node API with additional device properties

[Zhangfei Gao]

On 2021/9/30 上午1:08, Bjorn Helgaas wrote:
> [+cc Zhangfei, author of 8304a3a199ee ("PCI: Set dma-can-stall for
> HiSilicon chips"), which added this]
>
> On Wed, Sep 29, 2021 at 04:37:28PM +0300, Heikki Krogerus wrote:
>> Using device_create_managed_software_node() to inject the
>> properties in quirk_huawei_pcie_sva() instead of with the
>> old device_add_properties() API.
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> This is fine with me, but please update the subject line and commit
> log something like this:
>
>    PCI: Convert to device_create_managed_software_node()
>
>    In quirk_huawei_pcie_sva(), use device_create_managed_software_node()
>    instead of device_add_properties() to set the "dma-can-stall"
>    property.
>
>    This resolves a software node lifetime issue (see 151f6ff78cdf
>    ("software node: Provide replacement for device_add_properties()"))
>    and paves the way for removing device_add_properties() completely.
>
> Actually, 8304a3a199ee was merged during the v5.15 merge window, so if
> this does in fact fix a lifetime issue, I can merge this before
> v5.15-final.
>
> I know *this* quirk applies to AMBA devices, and I assume they cannot
> be removed, so there's no actual lifetime problem in this particular
> case, but in general it looks like a problem for PCI devices.
Thanks Bjorn
This patch also works, though the quirk is for platform devices and not 
removed.

Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>
>> ---
>>   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 1/2] PCI: Use software node API with additional device properties

[Heikki Krogerus]

Hi guys,

On Thu, Sep 30, 2021 at 10:33:27AM +0800, Zhangfei Gao wrote:
> On 2021/9/30 上午1:08, Bjorn Helgaas wrote:
> > [+cc Zhangfei, author of 8304a3a199ee ("PCI: Set dma-can-stall for
> > HiSilicon chips"), which added this]
> > 
> > On Wed, Sep 29, 2021 at 04:37:28PM +0300, Heikki Krogerus wrote:
> > > Using device_create_managed_software_node() to inject the
> > > properties in quirk_huawei_pcie_sva() instead of with the
> > > old device_add_properties() API.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > This is fine with me, but please update the subject line and commit
> > log something like this:
> > 
> >    PCI: Convert to device_create_managed_software_node()
> > 
> >    In quirk_huawei_pcie_sva(), use device_create_managed_software_node()
> >    instead of device_add_properties() to set the "dma-can-stall"
> >    property.
> > 
> >    This resolves a software node lifetime issue (see 151f6ff78cdf
> >    ("software node: Provide replacement for device_add_properties()"))
> >    and paves the way for removing device_add_properties() completely.
> > 
> > Actually, 8304a3a199ee was merged during the v5.15 merge window, so if
> > this does in fact fix a lifetime issue, I can merge this before
> > v5.15-final.

It does not fix lifetime issues. This is because device_del() called
device_remove_properties() unconditionally with every device.

There should be no functional impact.

> > I know *this* quirk applies to AMBA devices, and I assume they cannot
> > be removed, so there's no actual lifetime problem in this particular
> > case, but in general it looks like a problem for PCI devices.
> Thanks Bjorn
> This patch also works, though the quirk is for platform devices and not
> removed.

If the device is really never removed, then we could also constify the
node and the properties in it. Then the patch would look like this:

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b6b4c803bdc94..3dc7a1c62bf24 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1833,13 +1833,17 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI
  * even when a "PCI" device turns out to be a regular old SoC device
  * dressed up as a RCiEP and normal rules don't apply.
  */
+static const struct property_entry huawei_pcie_sva_props[] = {
+       PROPERTY_ENTRY_BOOL("dma-can-stall"),
+       { }
+};
+
+static const struct software_node huawei_pcie_sva_swnode = {
+       .properties = huawei_pcie_sva_props,
+};
+
 static void quirk_huawei_pcie_sva(struct pci_dev *pdev)
 {
-       struct property_entry properties[] = {
-               PROPERTY_ENTRY_BOOL("dma-can-stall"),
-               {},
-       };
-
        if (pdev->revision != 0x21 && pdev->revision != 0x30)
                return;
 
@@ -1850,7 +1854,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_add_software_node(&pdev->dev, &huawei_pcie_sva_swnode))
                pci_warn(pdev, "could not add stall property");
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);


Let me know if you prefer it that way.

thanks,

-- 
heikki

Re: [PATCH 1/2] PCI: Use software node API with additional device properties

[Andy Shevchenko]

On Thu, Sep 30, 2021 at 1:06 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Thu, Sep 30, 2021 at 10:33:27AM +0800, Zhangfei Gao wrote:

...

> If the device is really never removed, then we could also constify the
> node and the properties in it. Then the patch would look like this:

I'm not sure the user can't force removal of the device (via PCI
rescan, for example,, or via unbind/bind cycle). I guess this way
should be really taken carefully.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] PCI: Use software node API with additional device properties

[Rafael J. Wysocki]

On Thu, Sep 30, 2021 at 12:20 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Sep 30, 2021 at 1:06 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > On Thu, Sep 30, 2021 at 10:33:27AM +0800, Zhangfei Gao wrote:
>
> ...
>
> > If the device is really never removed, then we could also constify the
> > node and the properties in it. Then the patch would look like this:
>
> I'm not sure the user can't force removal of the device (via PCI
> rescan, for example,, or via unbind/bind cycle).

The sysfs unbind doesn't remove the device, though, AFAICS.  It just
unbinds the driver from it, if any.

> I guess this way should be really taken carefully.

But I agree.

Re: [PATCH 1/2] PCI: Use software node API with additional device properties

[Heikki Krogerus]

On Thu, Sep 30, 2021 at 12:37:27PM +0200, Rafael J. Wysocki wrote:
> On Thu, Sep 30, 2021 at 12:20 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Thu, Sep 30, 2021 at 1:06 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > > On Thu, Sep 30, 2021 at 10:33:27AM +0800, Zhangfei Gao wrote:
> >
> > ...
> >
> > > If the device is really never removed, then we could also constify the
> > > node and the properties in it. Then the patch would look like this:
> >
> > I'm not sure the user can't force removal of the device (via PCI
> > rescan, for example,, or via unbind/bind cycle).
> 
> The sysfs unbind doesn't remove the device, though, AFAICS.  It just
> unbinds the driver from it, if any.
> 
> > I guess this way should be really taken carefully.
> 
> But I agree.

OK. Makes sense.

Thanks guys,

-- 
heikki