[PATCH 0/6] usb: Handle device properties with software node API

[PATCH 0/6] usb: Handle device properties with software node API

[Heikki Krogerus]

Hi,

Currently it is not possible to take full advantage of software
fwnodes in the drivers because device_del() is calling
device_remove_properties() (that removes the software node attached to
the device) unconditionally which prevents the software nodes from
being reused or shared, and because subsystems are dealing with device
properties instead of software nodes which in many cases prevents the
drivers from using software nodes at all.

To fix the situation, the device_remove_properties() call in
device_del() has to be removed, and later the subsystems need to be
converted so that they deal with software nodes instead of just device
properties. But before that can be done, the drivers must be prepared
for those changes. These patches do that for the USB drivers.

The first patch introduces device_create_managed_software_node()
function that can be used as a drop-in replacement for
device_add_properties(). The rest of the patches simply use that
function, or convert the drivers in some other way to use software
nodes instead of just the device properties in them.

thanks,

Heikki Krogerus (6):
  software node: Provide replacement for device_add_properties()
  usb: dwc2: pci: Drop the empty quirk function
  usb: dwc3: haps: Constify the software node
  usb: dwc3: qcom: Constify the software node
  usb: dwc3: host: Use software node API with the properties
  xhci: ext-caps: Use software node API with the properties

 drivers/base/swnode.c            | 43 ++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/pci.c           | 18 -------------
 drivers/usb/dwc3/dwc3-haps.c     |  8 +++++-
 drivers/usb/dwc3/dwc3-qcom.c     | 12 ++++++---
 drivers/usb/dwc3/host.c          |  2 +-
 drivers/usb/host/xhci-ext-caps.c |  3 ++-
 include/linux/property.h         |  4 +++
 7 files changed, 66 insertions(+), 24 deletions(-)

-- 
2.30.0

[PATCH 1/6] software node: Provide replacement for device_add_properties()

[Heikki Krogerus]

Adding function device_create_managed_software_node() that
is designed to work as a drop-in replacement for
device_add_properties(). The function has one additional
feature compared to device_add_properties(). It takes also
an optional parent node as parameter, and that way allow the
nodes created with it to be part of a node hierarchy.

The lifetime of the software nodes created with this
function will be tied to the device they are assigned to.
The function will therefore behave exactly the same way as
device_add_properties() is expected to behave, except that
it does not simply assume that the nodes attached to the
device are always destroyed in device_del() unconditionally.

The nodes created with this function are guaranteed to be
removed when the device is removed even after device_del()
stops calling device_remove_properties() unconditionally.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c    | 43 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  4 ++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 20faa9f4f5ed2..37179a8b1ceba 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -24,6 +24,7 @@ struct swnode {
 	struct swnode *parent;
 
 	unsigned int allocated:1;
+	unsigned int managed:1;
 };
 
 static DEFINE_IDA(swnode_root_ids);
@@ -1045,6 +1046,43 @@ void device_remove_software_node(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_remove_software_node);
 
+/**
+ * device_create_managed_software_node - Create a software node for a device
+ * @dev: The device the software node is assigned to.
+ * @properties: Device properties for the software node.
+ * @parent: Parent of the software node.
+ *
+ * Creates a software node as a managed resource for @dev, which means the
+ * lifetime of the newly created software node is tied to the lifetime of @dev.
+ * Software nodes created with this function should not be reused or shared
+ * because of that. The function takes a deep copy of @properties for the
+ * software node.
+ *
+ * Since the new software node is assigned directly to @dev, and since it should
+ * not be shared, it is not returned to the caller. The function returns 0 on
+ * success, and errno in case of an error.
+ */
+int device_create_managed_software_node(struct device *dev,
+					const struct property_entry *properties,
+					const struct software_node *parent)
+{
+	struct fwnode_handle *p = software_node_fwnode(parent);
+	struct fwnode_handle *fwnode;
+
+	if (parent && !p)
+		return -EINVAL;
+
+	fwnode = fwnode_create_software_node(properties, p);
+	if (IS_ERR(fwnode))
+		return PTR_ERR(fwnode);
+
+	to_swnode(fwnode)->managed = true;
+	set_secondary_fwnode(dev, fwnode);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_create_managed_software_node);
+
 int software_node_notify(struct device *dev, unsigned long action)
 {
 	struct swnode *swnode;
@@ -1073,6 +1111,11 @@ int software_node_notify(struct device *dev, unsigned long action)
 		sysfs_remove_link(&swnode->kobj, dev_name(dev));
 		sysfs_remove_link(&dev->kobj, "software_node");
 		kobject_put(&swnode->kobj);
+
+		if (swnode->managed) {
+			set_secondary_fwnode(dev, NULL);
+			kobject_put(&swnode->kobj);
+		}
 		break;
 	default:
 		break;
diff --git a/include/linux/property.h b/include/linux/property.h
index b0e413dc59271..dafccfce02624 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -491,4 +491,8 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode);
 int device_add_software_node(struct device *dev, const struct software_node *swnode);
 void device_remove_software_node(struct device *dev);
 
+int device_create_managed_software_node(struct device *dev,
+					const struct property_entry *properties,
+					const struct software_node *parent);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.30.0

[PATCH 2/6] usb: dwc2: pci: Drop the empty quirk function

[Heikki Krogerus]

The function dwc2_pci_quirks() does nothing. Removing.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Minas Harutyunyan <hminas@synopsys.com>
---
 drivers/usb/dwc2/pci.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
index 7afc10872f1f0..0000151e3ca96 100644
--- a/drivers/usb/dwc2/pci.c
+++ b/drivers/usb/dwc2/pci.c
@@ -63,20 +63,6 @@ struct dwc2_pci_glue {
 	struct platform_device *phy;
 };
 
-static int dwc2_pci_quirks(struct pci_dev *pdev, struct platform_device *dwc2)
-{
-	if (pdev->vendor == PCI_VENDOR_ID_SYNOPSYS &&
-	    pdev->device == PCI_PRODUCT_ID_HAPS_HSOTG) {
-		struct property_entry properties[] = {
-			{ },
-		};
-
-		return platform_device_add_properties(dwc2, properties);
-	}
-
-	return 0;
-}
-
 /**
  * dwc2_pci_probe() - Provides the cleanup entry points for the DWC_otg PCI
  * driver
@@ -143,10 +129,6 @@ static int dwc2_pci_probe(struct pci_dev *pci,
 
 	dwc2->dev.parent = dev;
 
-	ret = dwc2_pci_quirks(pci, dwc2);
-	if (ret)
-		goto err;
-
 	glue = devm_kzalloc(dev, sizeof(*glue), GFP_KERNEL);
 	if (!glue) {
 		ret = -ENOMEM;
-- 
2.30.0

[PATCH 3/6] usb: dwc3: haps: Constify the software node

[Heikki Krogerus]

What platform_device_add_properties() does is it allocates
dynamically a software node that will contain the device
properties supplied to it, and then couples that node with
the device. Since that node is always created, it might as
well be constant.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/dwc3-haps.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-haps.c b/drivers/usb/dwc3/dwc3-haps.c
index 55b4a901168e8..a0f046855a233 100644
--- a/drivers/usb/dwc3/dwc3-haps.c
+++ b/drivers/usb/dwc3/dwc3-haps.c
@@ -33,6 +33,10 @@ static const struct property_entry initial_properties[] = {
 	{ },
 };
 
+static const struct software_node dwc3_haps_swnode = {
+	.properties = initial_properties,
+};
+
 static int dwc3_haps_probe(struct pci_dev *pci,
 			   const struct pci_device_id *id)
 {
@@ -77,7 +81,7 @@ static int dwc3_haps_probe(struct pci_dev *pci,
 	dwc->pci = pci;
 	dwc->dwc3->dev.parent = dev;
 
-	ret = platform_device_add_properties(dwc->dwc3, initial_properties);
+	ret = device_add_software_node(&dwc->dwc3->dev, dwc3_haps_swnode);
 	if (ret)
 		goto err;
 
@@ -91,6 +95,7 @@ static int dwc3_haps_probe(struct pci_dev *pci,
 
 	return 0;
 err:
+	device_remove_software_node(&dwc->dwc3->dev);
 	platform_device_put(dwc->dwc3);
 	return ret;
 }
@@ -99,6 +104,7 @@ static void dwc3_haps_remove(struct pci_dev *pci)
 {
 	struct dwc3_haps *dwc = pci_get_drvdata(pci);
 
+	device_remove_software_node(&dwc->dwc3->dev);
 	platform_device_unregister(dwc->dwc3);
 }
 
-- 
2.30.0

[PATCH 4/6] usb: dwc3: qcom: Constify the software node

[Heikki Krogerus]

What platform_device_add_properties() does is it allocates
dynamically a software node that will contain the device
properties supplied to it, and then couples that node with
the device. If the properties are constant, the node can be
constant as well.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index d803ee98c628e..d857d6c160a66 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -567,6 +567,10 @@ static const struct property_entry dwc3_qcom_acpi_properties[] = {
 	{}
 };
 
+static const struct software_node dwc3_qcom_swnode = {
+	.properties = dwc3_qcom_acpi_properties,
+};
+
 static int dwc3_qcom_acpi_register_core(struct platform_device *pdev)
 {
 	struct dwc3_qcom	*qcom = platform_get_drvdata(pdev);
@@ -613,16 +617,17 @@ static int dwc3_qcom_acpi_register_core(struct platform_device *pdev)
 		goto out;
 	}
 
-	ret = platform_device_add_properties(qcom->dwc3,
-					     dwc3_qcom_acpi_properties);
+	ret = device_add_software_node(&qcom->dwc3->dev, dwc3_qcom_swnode);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to add properties\n");
 		goto out;
 	}
 
 	ret = platform_device_add(qcom->dwc3);
-	if (ret)
+	if (ret) {
 		dev_err(&pdev->dev, "failed to add device\n");
+		device_remove_software_node(&qcom->dwc3->dev);
+	}
 
 out:
 	kfree(child_res);
@@ -837,6 +842,7 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int i;
 
+	device_remove_software_node(&qcom->dwc3->dev);
 	of_platform_depopulate(dev);
 
 	for (i = qcom->num_clocks - 1; i >= 0; i--) {
-- 
2.30.0

[PATCH 5/6] usb: dwc3: host: Use software node API with the properties

[Heikki Krogerus]

This replaces the platform_device_add_properties() call with
the safer device_create_managed_software_node() that does
exactly the same, but can also guarantee that the lifetime
of the node that is created for the device is tied to the
lifetime of device itself.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index e195176580de1..f29a264635aa1 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -108,7 +108,7 @@ int dwc3_host_init(struct dwc3 *dwc)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped");
 
 	if (prop_idx) {
-		ret = platform_device_add_properties(xhci, props);
+		ret = device_create_managed_software_node(&xhci->dev, props, NULL);
 		if (ret) {
 			dev_err(dwc->dev, "failed to add properties to xHCI\n");
 			goto err;
-- 
2.30.0

[PATCH 6/6] xhci: ext-caps: Use software node API with the properties

[Heikki Krogerus]

This replaces the platform_device_add_properties() call with
the safer device_create_managed_software_node() that does
exactly the same, but can also guarantee that the lifetime
of the node that is created for the device is tied to the
lifetime of device itself.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/xhci-ext-caps.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ext-caps.c b/drivers/usb/host/xhci-ext-caps.c
index 3351d07c431f1..7a4c2c4ad50e8 100644
--- a/drivers/usb/host/xhci-ext-caps.c
+++ b/drivers/usb/host/xhci-ext-caps.c
@@ -54,7 +54,8 @@ static int xhci_create_intel_xhci_sw_pdev(struct xhci_hcd *xhci, u32 cap_offset)
 	}
 
 	if (pci->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
-		ret = platform_device_add_properties(pdev, role_switch_props);
+		ret = device_create_managed_software_node(&pdev->dev, role_switch_props,
+							  NULL);
 		if (ret) {
 			dev_err(dev, "failed to register device properties\n");
 			platform_device_put(pdev);
-- 
2.30.0

Re: [PATCH 6/6] xhci: ext-caps: Use software node API with the properties

[Hans de Goede]

Hi,

On 2/2/21 1:50 PM, Heikki Krogerus wrote:
> This replaces the platform_device_add_properties() call with
> the safer device_create_managed_software_node() that does
> exactly the same, but can also guarantee that the lifetime
> of the node that is created for the device is tied to the
> lifetime of device itself.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/usb/host/xhci-ext-caps.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-ext-caps.c b/drivers/usb/host/xhci-ext-caps.c
> index 3351d07c431f1..7a4c2c4ad50e8 100644
> --- a/drivers/usb/host/xhci-ext-caps.c
> +++ b/drivers/usb/host/xhci-ext-caps.c
> @@ -54,7 +54,8 @@ static int xhci_create_intel_xhci_sw_pdev(struct xhci_hcd *xhci, u32 cap_offset)
>  	}
>  
>  	if (pci->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
> -		ret = platform_device_add_properties(pdev, role_switch_props);
> +		ret = device_create_managed_software_node(&pdev->dev, role_switch_props,
> +							  NULL);
>  		if (ret) {
>  			dev_err(dev, "failed to register device properties\n");
>  			platform_device_put(pdev);
> 

Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Rafael J. Wysocki]

On Tue, Feb 2, 2021 at 1:50 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Adding function device_create_managed_software_node() that
> is designed to work as a drop-in replacement for
> device_add_properties(). The function has one additional
> feature compared to device_add_properties(). It takes also
> an optional parent node as parameter, and that way allow the
> nodes created with it to be part of a node hierarchy.
>
> The lifetime of the software nodes created with this
> function will be tied to the device they are assigned to.
> The function will therefore behave exactly the same way as
> device_add_properties() is expected to behave, except that
> it does not simply assume that the nodes attached to the
> device are always destroyed in device_del() unconditionally.
>
> The nodes created with this function are guaranteed to be
> removed when the device is removed even after device_del()
> stops calling device_remove_properties() unconditionally.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/base/swnode.c    | 43 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/property.h |  4 ++++
>  2 files changed, 47 insertions(+)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 20faa9f4f5ed2..37179a8b1ceba 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -24,6 +24,7 @@ struct swnode {
>         struct swnode *parent;
>
>         unsigned int allocated:1;
> +       unsigned int managed:1;
>  };
>
>  static DEFINE_IDA(swnode_root_ids);
> @@ -1045,6 +1046,43 @@ void device_remove_software_node(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(device_remove_software_node);
>
> +/**
> + * device_create_managed_software_node - Create a software node for a device
> + * @dev: The device the software node is assigned to.
> + * @properties: Device properties for the software node.
> + * @parent: Parent of the software node.
> + *
> + * Creates a software node as a managed resource for @dev, which means the
> + * lifetime of the newly created software node is tied to the lifetime of @dev.
> + * Software nodes created with this function should not be reused or shared
> + * because of that. The function takes a deep copy of @properties for the
> + * software node.
> + *
> + * Since the new software node is assigned directly to @dev, and since it should
> + * not be shared, it is not returned to the caller. The function returns 0 on
> + * success, and errno in case of an error.
> + */
> +int device_create_managed_software_node(struct device *dev,
> +                                       const struct property_entry *properties,
> +                                       const struct software_node *parent)
> +{
> +       struct fwnode_handle *p = software_node_fwnode(parent);
> +       struct fwnode_handle *fwnode;
> +
> +       if (parent && !p)
> +               return -EINVAL;
> +
> +       fwnode = fwnode_create_software_node(properties, p);
> +       if (IS_ERR(fwnode))
> +               return PTR_ERR(fwnode);
> +
> +       to_swnode(fwnode)->managed = true;
> +       set_secondary_fwnode(dev, fwnode);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(device_create_managed_software_node);
> +
>  int software_node_notify(struct device *dev, unsigned long action)
>  {
>         struct swnode *swnode;
> @@ -1073,6 +1111,11 @@ int software_node_notify(struct device *dev, unsigned long action)
>                 sysfs_remove_link(&swnode->kobj, dev_name(dev));
>                 sysfs_remove_link(&dev->kobj, "software_node");
>                 kobject_put(&swnode->kobj);
> +
> +               if (swnode->managed) {
> +                       set_secondary_fwnode(dev, NULL);
> +                       kobject_put(&swnode->kobj);

Where does the corresponding kobject_get() get called?

> +               }
>                 break;
>         default:
>                 break;
> diff --git a/include/linux/property.h b/include/linux/property.h
> index b0e413dc59271..dafccfce02624 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -491,4 +491,8 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode);
>  int device_add_software_node(struct device *dev, const struct software_node *swnode);
>  void device_remove_software_node(struct device *dev);
>
> +int device_create_managed_software_node(struct device *dev,
> +                                       const struct property_entry *properties,
> +                                       const struct software_node *parent);
> +
>  #endif /* _LINUX_PROPERTY_H_ */
> --
> 2.30.0
>

Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Heikki Krogerus]

Hi Rafael,

On Tue, Feb 02, 2021 at 03:44:05PM +0100, Rafael J. Wysocki wrote:
> > +/**
> > + * device_create_managed_software_node - Create a software node for a device
> > + * @dev: The device the software node is assigned to.
> > + * @properties: Device properties for the software node.
> > + * @parent: Parent of the software node.
> > + *
> > + * Creates a software node as a managed resource for @dev, which means the
> > + * lifetime of the newly created software node is tied to the lifetime of @dev.
> > + * Software nodes created with this function should not be reused or shared
> > + * because of that. The function takes a deep copy of @properties for the
> > + * software node.
> > + *
> > + * Since the new software node is assigned directly to @dev, and since it should
> > + * not be shared, it is not returned to the caller. The function returns 0 on
> > + * success, and errno in case of an error.
> > + */
> > +int device_create_managed_software_node(struct device *dev,
> > +                                       const struct property_entry *properties,
> > +                                       const struct software_node *parent)
> > +{
> > +       struct fwnode_handle *p = software_node_fwnode(parent);
> > +       struct fwnode_handle *fwnode;
> > +
> > +       if (parent && !p)
> > +               return -EINVAL;
> > +
> > +       fwnode = fwnode_create_software_node(properties, p);
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
To answer your question below: here.

> > +       if (IS_ERR(fwnode))
> > +               return PTR_ERR(fwnode);
> > +
> > +       to_swnode(fwnode)->managed = true;
> > +       set_secondary_fwnode(dev, fwnode);
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(device_create_managed_software_node);
> > +
> >  int software_node_notify(struct device *dev, unsigned long action)
> >  {
> >         struct swnode *swnode;
> > @@ -1073,6 +1111,11 @@ int software_node_notify(struct device *dev, unsigned long action)
> >                 sysfs_remove_link(&swnode->kobj, dev_name(dev));
> >                 sysfs_remove_link(&dev->kobj, "software_node");
> >                 kobject_put(&swnode->kobj);
> > +
> > +               if (swnode->managed) {
> > +                       set_secondary_fwnode(dev, NULL);
> > +                       kobject_put(&swnode->kobj);
> 
> Where does the corresponding kobject_get() get called?

So in function fwnode_create_software_node() we use
kobject_init_and_add().


thanks,

-- 
heikki

Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Rafael J. Wysocki]

On Tue, Feb 2, 2021 at 4:01 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Tue, Feb 02, 2021 at 03:44:05PM +0100, Rafael J. Wysocki wrote:
> > > +/**
> > > + * device_create_managed_software_node - Create a software node for a device
> > > + * @dev: The device the software node is assigned to.
> > > + * @properties: Device properties for the software node.
> > > + * @parent: Parent of the software node.
> > > + *
> > > + * Creates a software node as a managed resource for @dev, which means the
> > > + * lifetime of the newly created software node is tied to the lifetime of @dev.
> > > + * Software nodes created with this function should not be reused or shared
> > > + * because of that. The function takes a deep copy of @properties for the
> > > + * software node.
> > > + *
> > > + * Since the new software node is assigned directly to @dev, and since it should
> > > + * not be shared, it is not returned to the caller. The function returns 0 on
> > > + * success, and errno in case of an error.
> > > + */
> > > +int device_create_managed_software_node(struct device *dev,
> > > +                                       const struct property_entry *properties,
> > > +                                       const struct software_node *parent)
> > > +{
> > > +       struct fwnode_handle *p = software_node_fwnode(parent);
> > > +       struct fwnode_handle *fwnode;
> > > +
> > > +       if (parent && !p)
> > > +               return -EINVAL;
> > > +
> > > +       fwnode = fwnode_create_software_node(properties, p);
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> To answer your question below: here.
>
> > > +       if (IS_ERR(fwnode))
> > > +               return PTR_ERR(fwnode);
> > > +
> > > +       to_swnode(fwnode)->managed = true;
> > > +       set_secondary_fwnode(dev, fwnode);
> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_create_managed_software_node);
> > > +
> > >  int software_node_notify(struct device *dev, unsigned long action)
> > >  {
> > >         struct swnode *swnode;
> > > @@ -1073,6 +1111,11 @@ int software_node_notify(struct device *dev, unsigned long action)
> > >                 sysfs_remove_link(&swnode->kobj, dev_name(dev));
> > >                 sysfs_remove_link(&dev->kobj, "software_node");
> > >                 kobject_put(&swnode->kobj);
> > > +
> > > +               if (swnode->managed) {
> > > +                       set_secondary_fwnode(dev, NULL);
> > > +                       kobject_put(&swnode->kobj);
> >
> > Where does the corresponding kobject_get() get called?
>
> So in function fwnode_create_software_node() we use
> kobject_init_and_add().

OK

It looks like there is a use case that cannot be addressed by using
device_add_properties() and that's why you need this new function.

Can you describe that use case, please, and explain what the problem
with using device_add_properties() in it is?

Re: [PATCH 4/6] usb: dwc3: qcom: Constify the software node

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4834 bytes --]

Hi Heikki,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on next-20210125]
[cannot apply to balbi-usb/testing/next char-misc/char-misc-testing v5.11-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heikki-Krogerus/usb-Handle-device-properties-with-software-node-API/20210202-205900
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ca804315a8e0060b2be65411c7d32cbf4396a030
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heikki-Krogerus/usb-Handle-device-properties-with-software-node-API/20210202-205900
        git checkout ca804315a8e0060b2be65411c7d32cbf4396a030
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/usb/dwc3/dwc3-qcom.c: In function 'dwc3_qcom_acpi_register_core':
>> drivers/usb/dwc3/dwc3-qcom.c:620:51: error: incompatible type for argument 2 of 'device_add_software_node'
     620 |  ret = device_add_software_node(&qcom->dwc3->dev, dwc3_qcom_swnode);
         |                                                   ^~~~~~~~~~~~~~~~
         |                                                   |
         |                                                   const struct software_node
   In file included from include/linux/of.h:22,
                    from include/linux/irqdomain.h:35,
                    from include/linux/acpi.h:13,
                    from drivers/usb/dwc3/dwc3-qcom.c:7:
   include/linux/property.h:491:78: note: expected 'const struct software_node *' but argument is of type 'const struct software_node'
     491 | int device_add_software_node(struct device *dev, const struct software_node *swnode);
         |                                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~


vim +/device_add_software_node +620 drivers/usb/dwc3/dwc3-qcom.c

   573	
   574	static int dwc3_qcom_acpi_register_core(struct platform_device *pdev)
   575	{
   576		struct dwc3_qcom	*qcom = platform_get_drvdata(pdev);
   577		struct device		*dev = &pdev->dev;
   578		struct resource		*res, *child_res = NULL;
   579		struct platform_device	*pdev_irq = qcom->urs_usb ? qcom->urs_usb :
   580								    pdev;
   581		int			irq;
   582		int			ret;
   583	
   584		qcom->dwc3 = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
   585		if (!qcom->dwc3)
   586			return -ENOMEM;
   587	
   588		qcom->dwc3->dev.parent = dev;
   589		qcom->dwc3->dev.type = dev->type;
   590		qcom->dwc3->dev.dma_mask = dev->dma_mask;
   591		qcom->dwc3->dev.dma_parms = dev->dma_parms;
   592		qcom->dwc3->dev.coherent_dma_mask = dev->coherent_dma_mask;
   593	
   594		child_res = kcalloc(2, sizeof(*child_res), GFP_KERNEL);
   595		if (!child_res)
   596			return -ENOMEM;
   597	
   598		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   599		if (!res) {
   600			dev_err(&pdev->dev, "failed to get memory resource\n");
   601			ret = -ENODEV;
   602			goto out;
   603		}
   604	
   605		child_res[0].flags = res->flags;
   606		child_res[0].start = res->start;
   607		child_res[0].end = child_res[0].start +
   608			qcom->acpi_pdata->dwc3_core_base_size;
   609	
   610		irq = platform_get_irq(pdev_irq, 0);
   611		child_res[1].flags = IORESOURCE_IRQ;
   612		child_res[1].start = child_res[1].end = irq;
   613	
   614		ret = platform_device_add_resources(qcom->dwc3, child_res, 2);
   615		if (ret) {
   616			dev_err(&pdev->dev, "failed to add resources\n");
   617			goto out;
   618		}
   619	
 > 620		ret = device_add_software_node(&qcom->dwc3->dev, dwc3_qcom_swnode);
   621		if (ret < 0) {
   622			dev_err(&pdev->dev, "failed to add properties\n");
   623			goto out;
   624		}
   625	
   626		ret = platform_device_add(qcom->dwc3);
   627		if (ret) {
   628			dev_err(&pdev->dev, "failed to add device\n");
   629			device_remove_software_node(&qcom->dwc3->dev);
   630		}
   631	
   632	out:
   633		kfree(child_res);
   634		return ret;
   635	}
   636	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54166 bytes --]

Re: [PATCH 3/6] usb: dwc3: haps: Constify the software node

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4344 bytes --]

Hi Heikki,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on next-20210125]
[cannot apply to balbi-usb/testing/next char-misc/char-misc-testing v5.11-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heikki-Krogerus/usb-Handle-device-properties-with-software-node-API/20210202-205900
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-a006-20210202 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 275c6af7d7f1ed63a03d05b4484413e447133269)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/399031fcde9483395785bd0eec474c584bd1e9fd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heikki-Krogerus/usb-Handle-device-properties-with-software-node-API/20210202-205900
        git checkout 399031fcde9483395785bd0eec474c584bd1e9fd
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/usb/dwc3/dwc3-haps.c:84:50: error: passing 'const struct software_node' to parameter of incompatible type 'const struct software_node *'; take the address with &
           ret = device_add_software_node(&dwc->dwc3->dev, dwc3_haps_swnode);
                                                           ^~~~~~~~~~~~~~~~
                                                           &
   include/linux/property.h:491:78: note: passing argument to parameter 'swnode' here
   int device_add_software_node(struct device *dev, const struct software_node *swnode);
                                                                                ^
   1 error generated.


vim +84 drivers/usb/dwc3/dwc3-haps.c

    39	
    40	static int dwc3_haps_probe(struct pci_dev *pci,
    41				   const struct pci_device_id *id)
    42	{
    43		struct dwc3_haps	*dwc;
    44		struct device		*dev = &pci->dev;
    45		struct resource		res[2];
    46		int			ret;
    47	
    48		ret = pcim_enable_device(pci);
    49		if (ret) {
    50			dev_err(dev, "failed to enable pci device\n");
    51			return -ENODEV;
    52		}
    53	
    54		pci_set_master(pci);
    55	
    56		dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
    57		if (!dwc)
    58			return -ENOMEM;
    59	
    60		dwc->dwc3 = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
    61		if (!dwc->dwc3)
    62			return -ENOMEM;
    63	
    64		memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));
    65	
    66		res[0].start	= pci_resource_start(pci, 0);
    67		res[0].end	= pci_resource_end(pci, 0);
    68		res[0].name	= "dwc_usb3";
    69		res[0].flags	= IORESOURCE_MEM;
    70	
    71		res[1].start	= pci->irq;
    72		res[1].name	= "dwc_usb3";
    73		res[1].flags	= IORESOURCE_IRQ;
    74	
    75		ret = platform_device_add_resources(dwc->dwc3, res, ARRAY_SIZE(res));
    76		if (ret) {
    77			dev_err(dev, "couldn't add resources to dwc3 device\n");
    78			goto err;
    79		}
    80	
    81		dwc->pci = pci;
    82		dwc->dwc3->dev.parent = dev;
    83	
  > 84		ret = device_add_software_node(&dwc->dwc3->dev, dwc3_haps_swnode);
    85		if (ret)
    86			goto err;
    87	
    88		ret = platform_device_add(dwc->dwc3);
    89		if (ret) {
    90			dev_err(dev, "failed to register dwc3 device\n");
    91			goto err;
    92		}
    93	
    94		pci_set_drvdata(pci, dwc);
    95	
    96		return 0;
    97	err:
    98		device_remove_software_node(&dwc->dwc3->dev);
    99		platform_device_put(dwc->dwc3);
   100		return ret;
   101	}
   102	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39814 bytes --]

Re: [PATCH 4/6] usb: dwc3: qcom: Constify the software node

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4641 bytes --]

Hi Heikki,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on next-20210125]
[cannot apply to balbi-usb/testing/next char-misc/char-misc-testing v5.11-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heikki-Krogerus/usb-Handle-device-properties-with-software-node-API/20210202-205900
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: riscv-randconfig-r031-20210202 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 275c6af7d7f1ed63a03d05b4484413e447133269)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/ca804315a8e0060b2be65411c7d32cbf4396a030
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heikki-Krogerus/usb-Handle-device-properties-with-software-node-API/20210202-205900
        git checkout ca804315a8e0060b2be65411c7d32cbf4396a030
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/usb/dwc3/dwc3-qcom.c:620:51: error: passing 'const struct software_node' to parameter of incompatible type 'const struct software_node *'; take the address with &
           ret = device_add_software_node(&qcom->dwc3->dev, dwc3_qcom_swnode);
                                                            ^~~~~~~~~~~~~~~~
                                                            &
   include/linux/property.h:491:78: note: passing argument to parameter 'swnode' here
   int device_add_software_node(struct device *dev, const struct software_node *swnode);
                                                                                ^
   1 error generated.


vim +620 drivers/usb/dwc3/dwc3-qcom.c

   573	
   574	static int dwc3_qcom_acpi_register_core(struct platform_device *pdev)
   575	{
   576		struct dwc3_qcom	*qcom = platform_get_drvdata(pdev);
   577		struct device		*dev = &pdev->dev;
   578		struct resource		*res, *child_res = NULL;
   579		struct platform_device	*pdev_irq = qcom->urs_usb ? qcom->urs_usb :
   580								    pdev;
   581		int			irq;
   582		int			ret;
   583	
   584		qcom->dwc3 = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
   585		if (!qcom->dwc3)
   586			return -ENOMEM;
   587	
   588		qcom->dwc3->dev.parent = dev;
   589		qcom->dwc3->dev.type = dev->type;
   590		qcom->dwc3->dev.dma_mask = dev->dma_mask;
   591		qcom->dwc3->dev.dma_parms = dev->dma_parms;
   592		qcom->dwc3->dev.coherent_dma_mask = dev->coherent_dma_mask;
   593	
   594		child_res = kcalloc(2, sizeof(*child_res), GFP_KERNEL);
   595		if (!child_res)
   596			return -ENOMEM;
   597	
   598		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   599		if (!res) {
   600			dev_err(&pdev->dev, "failed to get memory resource\n");
   601			ret = -ENODEV;
   602			goto out;
   603		}
   604	
   605		child_res[0].flags = res->flags;
   606		child_res[0].start = res->start;
   607		child_res[0].end = child_res[0].start +
   608			qcom->acpi_pdata->dwc3_core_base_size;
   609	
   610		irq = platform_get_irq(pdev_irq, 0);
   611		child_res[1].flags = IORESOURCE_IRQ;
   612		child_res[1].start = child_res[1].end = irq;
   613	
   614		ret = platform_device_add_resources(qcom->dwc3, child_res, 2);
   615		if (ret) {
   616			dev_err(&pdev->dev, "failed to add resources\n");
   617			goto out;
   618		}
   619	
 > 620		ret = device_add_software_node(&qcom->dwc3->dev, dwc3_qcom_swnode);
   621		if (ret < 0) {
   622			dev_err(&pdev->dev, "failed to add properties\n");
   623			goto out;
   624		}
   625	
   626		ret = platform_device_add(qcom->dwc3);
   627		if (ret) {
   628			dev_err(&pdev->dev, "failed to add device\n");
   629			device_remove_software_node(&qcom->dwc3->dev);
   630		}
   631	
   632	out:
   633		kfree(child_res);
   634		return ret;
   635	}
   636	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38512 bytes --]

Re: [PATCH 3/6] usb: dwc3: haps: Constify the software node

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4192 bytes --]

Hi Heikki,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on next-20210125]
[cannot apply to balbi-usb/testing/next char-misc/char-misc-testing v5.11-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heikki-Krogerus/usb-Handle-device-properties-with-software-node-API/20210202-205900
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-c002-20210202 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/399031fcde9483395785bd0eec474c584bd1e9fd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heikki-Krogerus/usb-Handle-device-properties-with-software-node-API/20210202-205900
        git checkout 399031fcde9483395785bd0eec474c584bd1e9fd
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/usb/dwc3/dwc3-haps.c: In function 'dwc3_haps_probe':
>> drivers/usb/dwc3/dwc3-haps.c:84:50: error: incompatible type for argument 2 of 'device_add_software_node'
      84 |  ret = device_add_software_node(&dwc->dwc3->dev, dwc3_haps_swnode);
         |                                                  ^~~~~~~~~~~~~~~~
         |                                                  |
         |                                                  const struct software_node
   In file included from drivers/usb/dwc3/dwc3-haps.c:16:
   include/linux/property.h:491:78: note: expected 'const struct software_node *' but argument is of type 'const struct software_node'
     491 | int device_add_software_node(struct device *dev, const struct software_node *swnode);
         |                                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~


vim +/device_add_software_node +84 drivers/usb/dwc3/dwc3-haps.c

    39	
    40	static int dwc3_haps_probe(struct pci_dev *pci,
    41				   const struct pci_device_id *id)
    42	{
    43		struct dwc3_haps	*dwc;
    44		struct device		*dev = &pci->dev;
    45		struct resource		res[2];
    46		int			ret;
    47	
    48		ret = pcim_enable_device(pci);
    49		if (ret) {
    50			dev_err(dev, "failed to enable pci device\n");
    51			return -ENODEV;
    52		}
    53	
    54		pci_set_master(pci);
    55	
    56		dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
    57		if (!dwc)
    58			return -ENOMEM;
    59	
    60		dwc->dwc3 = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
    61		if (!dwc->dwc3)
    62			return -ENOMEM;
    63	
    64		memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));
    65	
    66		res[0].start	= pci_resource_start(pci, 0);
    67		res[0].end	= pci_resource_end(pci, 0);
    68		res[0].name	= "dwc_usb3";
    69		res[0].flags	= IORESOURCE_MEM;
    70	
    71		res[1].start	= pci->irq;
    72		res[1].name	= "dwc_usb3";
    73		res[1].flags	= IORESOURCE_IRQ;
    74	
    75		ret = platform_device_add_resources(dwc->dwc3, res, ARRAY_SIZE(res));
    76		if (ret) {
    77			dev_err(dev, "couldn't add resources to dwc3 device\n");
    78			goto err;
    79		}
    80	
    81		dwc->pci = pci;
    82		dwc->dwc3->dev.parent = dev;
    83	
  > 84		ret = device_add_software_node(&dwc->dwc3->dev, dwc3_haps_swnode);
    85		if (ret)
    86			goto err;
    87	
    88		ret = platform_device_add(dwc->dwc3);
    89		if (ret) {
    90			dev_err(dev, "failed to register dwc3 device\n");
    91			goto err;
    92		}
    93	
    94		pci_set_drvdata(pci, dwc);
    95	
    96		return 0;
    97	err:
    98		device_remove_software_node(&dwc->dwc3->dev);
    99		platform_device_put(dwc->dwc3);
   100		return ret;
   101	}
   102	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32860 bytes --]

Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Heikki Krogerus]

On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote:
> It looks like there is a use case that cannot be addressed by using
> device_add_properties() and that's why you need this new function.
> 
> Can you describe that use case, please, and explain what the problem
> with using device_add_properties() in it is?

The problem with device_add_properties() is that it gives false
impression that the device properties are somehow directly attached to
the devices, which is not true. Now, that should not be a major issue,
but it seems that it is. I think Lee Jones basically used that as an
argument to refuse changes (and pretty minor changes) that would have
allowed us to use software nodes with the MFD drivers.

Nevertheless, I was not planning to provide a replacement for it
originally. We do in any case have the real issue caused by that
device_remove_properties() call in device_del() which has to be fixed.
I started to fix that by moving device_add_properties() under
drivers/base/swnode.c so I can implement it like I've implemented now
that new function, but after that when I started to tackle the second
issue by removing the subsystem wrappers like
platform_device_add_device_properties() and replacing them with things
like platform_device_add_software_node() in order to give the drivers
a chance to actually use software nodes, I realised that there isn't
much use for device_add_properties() after that.

Also, even though I'm not super happy about adding more API to this
thing, this function - device_create_managed_software_node() - I do
like. Not only is it implemented so that we don't have to rely on
anything else in drivers core in order to achieve the lifetime link
with the device, it is also much more descriptive. The function name
alone does not leave any questions about what is going to happen if
that function is called. You'll end up with a software node that just
happens to be attached to the device.

On top of those two things, by adding the new function it also gives
me a nice stepping stone to do these changes in the normal stages:

        1. Add feature/modification.
        2. Convert users.
        3. Cleanup.

And finally, even though we may not have any users left for
device_add_properties() after I have updated all the subsystems and
drivers, this new function will still be useful. That is because, even
though it can be used as a drop-in replacement for
device_add_properties(), it does add that one small but important
change. It allows the nodes created with it to be part of node
hierarchy, and that alone is useful to me, and I'm planning on using
that feature later.

I'm sorry about the long explanation.

Br,

-- 
heikki

Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Rafael J. Wysocki]

On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote:
> > It looks like there is a use case that cannot be addressed by using
> > device_add_properties() and that's why you need this new function.
> >
> > Can you describe that use case, please, and explain what the problem
> > with using device_add_properties() in it is?
>
> The problem with device_add_properties() is that it gives false
> impression that the device properties are somehow directly attached to
> the devices, which is not true. Now, that should not be a major issue,
> but it seems that it is. I think Lee Jones basically used that as an
> argument to refuse changes (and pretty minor changes) that would have
> allowed us to use software nodes with the MFD drivers.
>
> Nevertheless, I was not planning to provide a replacement for it
> originally. We do in any case have the real issue caused by that
> device_remove_properties() call in device_del() which has to be fixed.

What's that issue, specifically?

> I started to fix that by moving device_add_properties() under
> drivers/base/swnode.c so I can implement it like I've implemented now
> that new function, but after that when I started to tackle the second
> issue by removing the subsystem wrappers like
> platform_device_add_device_properties() and replacing them with things
> like platform_device_add_software_node() in order to give the drivers
> a chance to actually use software nodes, I realised that there isn't
> much use for device_add_properties() after that.
>
> Also, even though I'm not super happy about adding more API to this
> thing, this function - device_create_managed_software_node() - I do
> like. Not only is it implemented so that we don't have to rely on
> anything else in drivers core in order to achieve the lifetime link
> with the device, it is also much more descriptive. The function name
> alone does not leave any questions about what is going to happen if
> that function is called. You'll end up with a software node that just
> happens to be attached to the device.
>
> On top of those two things, by adding the new function it also gives
> me a nice stepping stone to do these changes in the normal stages:
>
>         1. Add feature/modification.
>         2. Convert users.
>         3. Cleanup.
>
> And finally, even though we may not have any users left for
> device_add_properties() after I have updated all the subsystems and
> drivers, this new function will still be useful. That is because, even
> though it can be used as a drop-in replacement for
> device_add_properties(), it does add that one small but important
> change. It allows the nodes created with it to be part of node
> hierarchy, and that alone is useful to me, and I'm planning on using
> that feature later.
>
> I'm sorry about the long explanation.

No need to be sorry,  now I know what this really is about. :-)

I'm not against this patch, but I IMO the "motivation" part of the
changelog needs to be improved.

If the final goal is to get rid of device_add_properties(), please
spell it out and say what problems there are with it and why the new
function will be better.

Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Heikki Krogerus]

On Wed, Feb 03, 2021 at 02:50:24PM +0100, Rafael J. Wysocki wrote:
> On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote:
> > > It looks like there is a use case that cannot be addressed by using
> > > device_add_properties() and that's why you need this new function.
> > >
> > > Can you describe that use case, please, and explain what the problem
> > > with using device_add_properties() in it is?
> >
> > The problem with device_add_properties() is that it gives false
> > impression that the device properties are somehow directly attached to
> > the devices, which is not true. Now, that should not be a major issue,
> > but it seems that it is. I think Lee Jones basically used that as an
> > argument to refuse changes (and pretty minor changes) that would have
> > allowed us to use software nodes with the MFD drivers.
> >
> > Nevertheless, I was not planning to provide a replacement for it
> > originally. We do in any case have the real issue caused by that
> > device_remove_properties() call in device_del() which has to be fixed.
> 
> What's that issue, specifically?

The problem is that we can't now reuse or share if necessary, or just
in general be in charge of the lifetime of the software nodes because
that call is in device_del(). Now the lifetime of the software nodes
is always tied to the devices they are attached, no questions asked.

> > I started to fix that by moving device_add_properties() under
> > drivers/base/swnode.c so I can implement it like I've implemented now
> > that new function, but after that when I started to tackle the second
> > issue by removing the subsystem wrappers like
> > platform_device_add_device_properties() and replacing them with things
> > like platform_device_add_software_node() in order to give the drivers
> > a chance to actually use software nodes, I realised that there isn't
> > much use for device_add_properties() after that.
> >
> > Also, even though I'm not super happy about adding more API to this
> > thing, this function - device_create_managed_software_node() - I do
> > like. Not only is it implemented so that we don't have to rely on
> > anything else in drivers core in order to achieve the lifetime link
> > with the device, it is also much more descriptive. The function name
> > alone does not leave any questions about what is going to happen if
> > that function is called. You'll end up with a software node that just
> > happens to be attached to the device.
> >
> > On top of those two things, by adding the new function it also gives
> > me a nice stepping stone to do these changes in the normal stages:
> >
> >         1. Add feature/modification.
> >         2. Convert users.
> >         3. Cleanup.
> >
> > And finally, even though we may not have any users left for
> > device_add_properties() after I have updated all the subsystems and
> > drivers, this new function will still be useful. That is because, even
> > though it can be used as a drop-in replacement for
> > device_add_properties(), it does add that one small but important
> > change. It allows the nodes created with it to be part of node
> > hierarchy, and that alone is useful to me, and I'm planning on using
> > that feature later.
> >
> > I'm sorry about the long explanation.
> 
> No need to be sorry,  now I know what this really is about. :-)
> 
> I'm not against this patch, but I IMO the "motivation" part of the
> changelog needs to be improved.
> 
> If the final goal is to get rid of device_add_properties(), please
> spell it out and say what problems there are with it and why the new
> function will be better.

Sure thing. Thanks Rafael.


Br,

-- 
heikki

Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Rafael J. Wysocki]

On Wed, Feb 3, 2021 at 3:27 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Feb 03, 2021 at 02:50:24PM +0100, Rafael J. Wysocki wrote:
> > On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote:
> > > > It looks like there is a use case that cannot be addressed by using
> > > > device_add_properties() and that's why you need this new function.
> > > >
> > > > Can you describe that use case, please, and explain what the problem
> > > > with using device_add_properties() in it is?
> > >
> > > The problem with device_add_properties() is that it gives false
> > > impression that the device properties are somehow directly attached to
> > > the devices, which is not true. Now, that should not be a major issue,
> > > but it seems that it is. I think Lee Jones basically used that as an
> > > argument to refuse changes (and pretty minor changes) that would have
> > > allowed us to use software nodes with the MFD drivers.
> > >
> > > Nevertheless, I was not planning to provide a replacement for it
> > > originally. We do in any case have the real issue caused by that
> > > device_remove_properties() call in device_del() which has to be fixed.
> >
> > What's that issue, specifically?
>
> The problem is that we can't now reuse or share if necessary, or just
> in general be in charge of the lifetime of the software nodes because
> that call is in device_del(). Now the lifetime of the software nodes
> is always tied to the devices they are attached, no questions asked.

I see and so instead you want the reference counting to trigger the
cleanup when the count gets to 0.

Sounds reasonable to me and please put this information into the patch
changelog.

Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Heikki Krogerus]

On Wed, Feb 03, 2021 at 03:39:02PM +0100, Rafael J. Wysocki wrote:
> On Wed, Feb 3, 2021 at 3:27 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Wed, Feb 03, 2021 at 02:50:24PM +0100, Rafael J. Wysocki wrote:
> > > On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote:
> > > > > It looks like there is a use case that cannot be addressed by using
> > > > > device_add_properties() and that's why you need this new function.
> > > > >
> > > > > Can you describe that use case, please, and explain what the problem
> > > > > with using device_add_properties() in it is?
> > > >
> > > > The problem with device_add_properties() is that it gives false
> > > > impression that the device properties are somehow directly attached to
> > > > the devices, which is not true. Now, that should not be a major issue,
> > > > but it seems that it is. I think Lee Jones basically used that as an
> > > > argument to refuse changes (and pretty minor changes) that would have
> > > > allowed us to use software nodes with the MFD drivers.
> > > >
> > > > Nevertheless, I was not planning to provide a replacement for it
> > > > originally. We do in any case have the real issue caused by that
> > > > device_remove_properties() call in device_del() which has to be fixed.
> > >
> > > What's that issue, specifically?
> >
> > The problem is that we can't now reuse or share if necessary, or just
> > in general be in charge of the lifetime of the software nodes because
> > that call is in device_del(). Now the lifetime of the software nodes
> > is always tied to the devices they are attached, no questions asked.
> 
> I see and so instead you want the reference counting to trigger the
> cleanup when the count gets to 0.
> 
> Sounds reasonable to me and please put this information into the patch
> changelog.

Yes. I'll do that.

thanks,

-- 
heikki