All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/6] usb: Handle device properties with software node API
@ 2021-02-04 14:17 Heikki Krogerus
  2021-02-04 14:17 ` [PATCH v2 1/6] software node: Provide replacement for device_add_properties() Heikki Krogerus
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Heikki Krogerus @ 2021-02-04 14:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	linux-kernel, linux-usb

Hi,

I've now rewritten the commit message of the first patch introducing
the new function device_create_managed_software_node(). It should now
explanation why we need that function in more detail. I think
that is what Rafael wanted. Also, everything should now compile.


v1 cover letter:

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/6] software node: Provide replacement for device_add_properties()
  2021-02-04 14:17 [PATCH v2 0/6] usb: Handle device properties with software node API Heikki Krogerus
@ 2021-02-04 14:17 ` Heikki Krogerus
  2021-02-04 16:06   ` Rafael J. Wysocki
  2021-02-04 14:17 ` [PATCH v2 2/6] usb: dwc2: pci: Drop the empty quirk function Heikki Krogerus
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2021-02-04 14:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	linux-kernel, linux-usb

At the moment the function device_del() is calling
device_remove_properties() unconditionally. That will result into the
reference count of the software node attached to the device being
decremented, and in most cases it will hit 0 at that point. So in
practice device_del() will unregister the software node attached to
the device, even if that was not the intention of the caller. Right
now software nodes can not be reused or shared because of that.

So device_del() can not unregister the software nodes unconditionally
like that. Unfortunately some of the users of device_add_properties()
are now relying on this behaviour. Because of that, and also in
general, we do need a function that can offer similar behaviour where
the lifetime of the software node is bound to the lifetime of the
device. But it just has to be a separate function so the behaviour is
optional. We can not remove the device_remove_properties() call from
device_del() before we have that new function, and before we have
replaced device_add_properties() calls with it in all the places that
require that behaviour.

This adds function device_create_managed_software_node() that can be
used for exactly that purpose. Software nodes created with it are
declared "managed", and separate handling for those nodes is added to
the software node code. The reference count of the "managed" nodes is
decremented when the device they are attached to is removed. This will
not affect the other nodes that are not declared "managed".

The function device_create_managed_software_node() has also one
additional feature that device_add_properties() does not have. It
allows the software nodes created with it to be part of a node
hierarchy by taking also an optional parent node as parameter.

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/6] usb: dwc2: pci: Drop the empty quirk function
  2021-02-04 14:17 [PATCH v2 0/6] usb: Handle device properties with software node API Heikki Krogerus
  2021-02-04 14:17 ` [PATCH v2 1/6] software node: Provide replacement for device_add_properties() Heikki Krogerus
@ 2021-02-04 14:17 ` Heikki Krogerus
  2021-02-04 14:40   ` Greg Kroah-Hartman
  2021-02-04 14:17 ` [PATCH v2 3/6] usb: dwc3: haps: Constify the software node Heikki Krogerus
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2021-02-04 14:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	linux-kernel, linux-usb, Minas Harutyunyan

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/6] usb: dwc3: haps: Constify the software node
  2021-02-04 14:17 [PATCH v2 0/6] usb: Handle device properties with software node API Heikki Krogerus
  2021-02-04 14:17 ` [PATCH v2 1/6] software node: Provide replacement for device_add_properties() Heikki Krogerus
  2021-02-04 14:17 ` [PATCH v2 2/6] usb: dwc2: pci: Drop the empty quirk function Heikki Krogerus
@ 2021-02-04 14:17 ` Heikki Krogerus
  2021-02-04 14:42   ` Greg Kroah-Hartman
  2021-02-04 14:17 ` [PATCH v2 4/6] usb: dwc3: qcom: " Heikki Krogerus
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2021-02-04 14:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	linux-kernel, linux-usb

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..f6e3817fa7af0 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/6] usb: dwc3: qcom: Constify the software node
  2021-02-04 14:17 [PATCH v2 0/6] usb: Handle device properties with software node API Heikki Krogerus
                   ` (2 preceding siblings ...)
  2021-02-04 14:17 ` [PATCH v2 3/6] usb: dwc3: haps: Constify the software node Heikki Krogerus
@ 2021-02-04 14:17 ` Heikki Krogerus
  2021-02-04 14:17 ` [PATCH v2 5/6] usb: dwc3: host: Use software node API with the properties Heikki Krogerus
  2021-02-04 14:17 ` [PATCH v2 6/6] xhci: ext-caps: " Heikki Krogerus
  5 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2021-02-04 14:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	linux-kernel, linux-usb

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..846a47be6df7f 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 5/6] usb: dwc3: host: Use software node API with the properties
  2021-02-04 14:17 [PATCH v2 0/6] usb: Handle device properties with software node API Heikki Krogerus
                   ` (3 preceding siblings ...)
  2021-02-04 14:17 ` [PATCH v2 4/6] usb: dwc3: qcom: " Heikki Krogerus
@ 2021-02-04 14:17 ` Heikki Krogerus
  2021-02-04 14:17 ` [PATCH v2 6/6] xhci: ext-caps: " Heikki Krogerus
  5 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2021-02-04 14:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	linux-kernel, linux-usb

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 6/6] xhci: ext-caps: Use software node API with the properties
  2021-02-04 14:17 [PATCH v2 0/6] usb: Handle device properties with software node API Heikki Krogerus
                   ` (4 preceding siblings ...)
  2021-02-04 14:17 ` [PATCH v2 5/6] usb: dwc3: host: Use software node API with the properties Heikki Krogerus
@ 2021-02-04 14:17 ` Heikki Krogerus
  5 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2021-02-04 14:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	linux-kernel, linux-usb

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>
Reviewed-by: 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/6] usb: dwc2: pci: Drop the empty quirk function
  2021-02-04 14:17 ` [PATCH v2 2/6] usb: dwc2: pci: Drop the empty quirk function Heikki Krogerus
@ 2021-02-04 14:40   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2021-02-04 14:40 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Rafael J. Wysocki, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	linux-kernel, linux-usb, Minas Harutyunyan

On Thu, Feb 04, 2021 at 05:17:07PM +0300, Heikki Krogerus wrote:
> 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;

This is crazy.  I'll go apply this right now :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/6] usb: dwc3: haps: Constify the software node
  2021-02-04 14:17 ` [PATCH v2 3/6] usb: dwc3: haps: Constify the software node Heikki Krogerus
@ 2021-02-04 14:42   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2021-02-04 14:42 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Rafael J. Wysocki, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	linux-kernel, linux-usb

On Thu, Feb 04, 2021 at 05:17:08PM +0300, Heikki Krogerus wrote:
> 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(-)

I've also now applied this, and patch 4/6 of the series as they have
nothing to do with your real work here :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/6] software node: Provide replacement for device_add_properties()
  2021-02-04 14:17 ` [PATCH v2 1/6] software node: Provide replacement for device_add_properties() Heikki Krogerus
@ 2021-02-04 16:06   ` Rafael J. Wysocki
  2021-02-04 16:13     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 12+ messages in thread
From: Rafael J. Wysocki @ 2021-02-04 16:06 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Andy Shevchenko,
	Felipe Balbi, Mathias Nyman, Linux Kernel Mailing List,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Thu, Feb 4, 2021 at 3:17 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> At the moment the function device_del() is calling
> device_remove_properties() unconditionally. That will result into the
> reference count of the software node attached to the device being
> decremented, and in most cases it will hit 0 at that point. So in
> practice device_del() will unregister the software node attached to
> the device, even if that was not the intention of the caller. Right
> now software nodes can not be reused or shared because of that.
>
> So device_del() can not unregister the software nodes unconditionally
> like that. Unfortunately some of the users of device_add_properties()
> are now relying on this behaviour. Because of that, and also in
> general, we do need a function that can offer similar behaviour where
> the lifetime of the software node is bound to the lifetime of the
> device. But it just has to be a separate function so the behaviour is
> optional. We can not remove the device_remove_properties() call from
> device_del() before we have that new function, and before we have
> replaced device_add_properties() calls with it in all the places that
> require that behaviour.
>
> This adds function device_create_managed_software_node() that can be
> used for exactly that purpose. Software nodes created with it are
> declared "managed", and separate handling for those nodes is added to
> the software node code. The reference count of the "managed" nodes is
> decremented when the device they are attached to is removed. This will
> not affect the other nodes that are not declared "managed".
>
> The function device_create_managed_software_node() has also one
> additional feature that device_add_properties() does not have. It
> allows the software nodes created with it to be part of a node
> hierarchy by taking also an optional parent node as parameter.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

The rationale is clear now, so

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and I'm assuming that this will be routed via the USB tree.

> ---
>  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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/6] software node: Provide replacement for device_add_properties()
  2021-02-04 16:06   ` Rafael J. Wysocki
@ 2021-02-04 16:13     ` Greg Kroah-Hartman
  2021-02-05  9:08       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 12+ messages in thread
From: Greg Kroah-Hartman @ 2021-02-04 16:13 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Heikki Krogerus, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	Linux Kernel Mailing List,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Thu, Feb 04, 2021 at 05:06:53PM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 4, 2021 at 3:17 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > At the moment the function device_del() is calling
> > device_remove_properties() unconditionally. That will result into the
> > reference count of the software node attached to the device being
> > decremented, and in most cases it will hit 0 at that point. So in
> > practice device_del() will unregister the software node attached to
> > the device, even if that was not the intention of the caller. Right
> > now software nodes can not be reused or shared because of that.
> >
> > So device_del() can not unregister the software nodes unconditionally
> > like that. Unfortunately some of the users of device_add_properties()
> > are now relying on this behaviour. Because of that, and also in
> > general, we do need a function that can offer similar behaviour where
> > the lifetime of the software node is bound to the lifetime of the
> > device. But it just has to be a separate function so the behaviour is
> > optional. We can not remove the device_remove_properties() call from
> > device_del() before we have that new function, and before we have
> > replaced device_add_properties() calls with it in all the places that
> > require that behaviour.
> >
> > This adds function device_create_managed_software_node() that can be
> > used for exactly that purpose. Software nodes created with it are
> > declared "managed", and separate handling for those nodes is added to
> > the software node code. The reference count of the "managed" nodes is
> > decremented when the device they are attached to is removed. This will
> > not affect the other nodes that are not declared "managed".
> >
> > The function device_create_managed_software_node() has also one
> > additional feature that device_add_properties() does not have. It
> > allows the software nodes created with it to be part of a node
> > hierarchy by taking also an optional parent node as parameter.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> The rationale is clear now, so
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> and I'm assuming that this will be routed via the USB tree.

Yes, I will do so, thanks.

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/6] software node: Provide replacement for device_add_properties()
  2021-02-04 16:13     ` Greg Kroah-Hartman
@ 2021-02-05  9:08       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2021-02-05  9:08 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Heikki Krogerus, Andy Shevchenko, Felipe Balbi, Mathias Nyman,
	Linux Kernel Mailing List,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Thu, Feb 04, 2021 at 05:13:04PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 04, 2021 at 05:06:53PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Feb 4, 2021 at 3:17 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > At the moment the function device_del() is calling
> > > device_remove_properties() unconditionally. That will result into the
> > > reference count of the software node attached to the device being
> > > decremented, and in most cases it will hit 0 at that point. So in
> > > practice device_del() will unregister the software node attached to
> > > the device, even if that was not the intention of the caller. Right
> > > now software nodes can not be reused or shared because of that.
> > >
> > > So device_del() can not unregister the software nodes unconditionally
> > > like that. Unfortunately some of the users of device_add_properties()
> > > are now relying on this behaviour. Because of that, and also in
> > > general, we do need a function that can offer similar behaviour where
> > > the lifetime of the software node is bound to the lifetime of the
> > > device. But it just has to be a separate function so the behaviour is
> > > optional. We can not remove the device_remove_properties() call from
> > > device_del() before we have that new function, and before we have
> > > replaced device_add_properties() calls with it in all the places that
> > > require that behaviour.
> > >
> > > This adds function device_create_managed_software_node() that can be
> > > used for exactly that purpose. Software nodes created with it are
> > > declared "managed", and separate handling for those nodes is added to
> > > the software node code. The reference count of the "managed" nodes is
> > > decremented when the device they are attached to is removed. This will
> > > not affect the other nodes that are not declared "managed".
> > >
> > > The function device_create_managed_software_node() has also one
> > > additional feature that device_add_properties() does not have. It
> > > allows the software nodes created with it to be part of a node
> > > hierarchy by taking also an optional parent node as parameter.
> > >
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > The rationale is clear now, so
> > 
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > and I'm assuming that this will be routed via the USB tree.
> 
> Yes, I will do so, thanks.

Now queued up, thanks!

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-02-05  9:11 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-04 14:17 [PATCH v2 0/6] usb: Handle device properties with software node API Heikki Krogerus
2021-02-04 14:17 ` [PATCH v2 1/6] software node: Provide replacement for device_add_properties() Heikki Krogerus
2021-02-04 16:06   ` Rafael J. Wysocki
2021-02-04 16:13     ` Greg Kroah-Hartman
2021-02-05  9:08       ` Greg Kroah-Hartman
2021-02-04 14:17 ` [PATCH v2 2/6] usb: dwc2: pci: Drop the empty quirk function Heikki Krogerus
2021-02-04 14:40   ` Greg Kroah-Hartman
2021-02-04 14:17 ` [PATCH v2 3/6] usb: dwc3: haps: Constify the software node Heikki Krogerus
2021-02-04 14:42   ` Greg Kroah-Hartman
2021-02-04 14:17 ` [PATCH v2 4/6] usb: dwc3: qcom: " Heikki Krogerus
2021-02-04 14:17 ` [PATCH v2 5/6] usb: dwc3: host: Use software node API with the properties Heikki Krogerus
2021-02-04 14:17 ` [PATCH v2 6/6] xhci: ext-caps: " Heikki Krogerus

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.