[PATCH v2 0/3] Remove one more platform_device_add_properties() call

[PATCH v2 0/3] Remove one more platform_device_add_properties() call

[Heikki Krogerus]

Hi Felipe, Rafael,

This is the second version of this series. There are no real changes,
but I added the Tiger Lake ID patch to this series in hope that it
will make your life a bit easier, assuming that Rafael will still pick
these.


The original over letter:

I originally introduced these as part of my series where I was
proposing PM ops for software nodes [1], but since that still needs
work, I'm sending these two separately.

So basically I'm only modifying dwc3-pci.c so it registers a software
node directly at this point. That will remove one more user of
platform_device_add_properties().

[1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/

thanks,

Heikki Krogerus (3):
  software node: Introduce device_add_software_node()
  usb: dwc3: pci: Register a software node for the dwc3 platform device
  usb: dwc3: pci: ID for Tiger Lake CPU

 drivers/base/swnode.c       | 69 ++++++++++++++++++++++++++++++++-----
 drivers/usb/dwc3/dwc3-pci.c | 65 +++++++++++++++++++++-------------
 include/linux/property.h    |  3 ++
 3 files changed, 104 insertions(+), 33 deletions(-)

-- 
2.29.2

[PATCH v2 1/3] software node: Introduce device_add_software_node()

[Heikki Krogerus]

This helper will register a software node and then assign
it to device at the same time. The function will also make
sure that the device can't have more than one software node.

Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c    | 69 ++++++++++++++++++++++++++++++++++------
 include/linux/property.h |  3 ++
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 4a4b2008fbc26..60a8501a4976c 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -48,6 +48,19 @@ EXPORT_SYMBOL_GPL(is_software_node);
 				     struct swnode, fwnode) : NULL;	\
 	})
 
+static inline struct swnode *dev_to_swnode(struct device *dev)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+
+	if (!fwnode)
+		return NULL;
+
+	if (!is_software_node(fwnode))
+		fwnode = fwnode->secondary;
+
+	return to_swnode(fwnode);
+}
+
 static struct swnode *
 software_node_to_swnode(const struct software_node *node)
 {
@@ -843,22 +856,60 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_remove_software_node);
 
+/**
+ * device_add_software_node - Assign software node to a device
+ * @dev: The device the software node is meant for.
+ * @swnode: The software node.
+ *
+ * This function will register @swnode and make it the secondary firmware node
+ * pointer of @dev. If @dev has no primary node, then @swnode will become the primary
+ * node.
+ */
+int device_add_software_node(struct device *dev, const struct software_node *swnode)
+{
+	int ret;
+
+	/* Only one software node per device. */
+	if (dev_to_swnode(dev))
+		return -EBUSY;
+
+	ret = software_node_register(swnode);
+	if (ret)
+		return ret;
+
+	set_secondary_fwnode(dev, software_node_fwnode(swnode));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_add_software_node);
+
+/**
+ * device_remove_software_node - Remove device's software node
+ * @dev: The device with the software node.
+ *
+ * This function will unregister the software node of @dev.
+ */
+void device_remove_software_node(struct device *dev)
+{
+	struct swnode *swnode;
+
+	swnode = dev_to_swnode(dev);
+	if (!swnode)
+		return;
+
+	kobject_put(&swnode->kobj);
+}
+EXPORT_SYMBOL_GPL(device_remove_software_node);
+
 int software_node_notify(struct device *dev, unsigned long action)
 {
-	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct swnode *swnode;
 	int ret;
 
-	if (!fwnode)
-		return 0;
-
-	if (!is_software_node(fwnode))
-		fwnode = fwnode->secondary;
-	if (!is_software_node(fwnode))
+	swnode = dev_to_swnode(dev);
+	if (!swnode)
 		return 0;
 
-	swnode = to_swnode(fwnode);
-
 	switch (action) {
 	case KOBJ_ADD:
 		ret = sysfs_create_link(&dev->kobj, &swnode->kobj,
diff --git a/include/linux/property.h b/include/linux/property.h
index 0a9001fe7aeab..b0e413dc59271 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -488,4 +488,7 @@ fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent);
 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);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.29.2

[PATCH v2 2/3] usb: dwc3: pci: Register a software node for the dwc3 platform device

[Heikki Krogerus]

By registering the software node directly instead of just
the properties in it, the driver can take advantage of also
the other features the software nodes have.

Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/dwc3-pci.c | 61 ++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index bae6a70664c80..037bc21bffa66 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -142,6 +142,18 @@ static const struct property_entry dwc3_pci_amd_properties[] = {
 	{}
 };
 
+static const struct software_node dwc3_pci_intel_swnode = {
+	.properties = dwc3_pci_intel_properties,
+};
+
+static const struct software_node dwc3_pci_intel_mrfld_swnode = {
+	.properties = dwc3_pci_mrfld_properties,
+};
+
+static const struct software_node dwc3_pci_amd_swnode = {
+	.properties = dwc3_pci_amd_properties,
+};
+
 static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 {
 	struct pci_dev			*pdev = dwc->pci;
@@ -222,7 +234,6 @@ static void dwc3_pci_resume_work(struct work_struct *work)
 
 static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 {
-	struct property_entry *p = (struct property_entry *)id->driver_data;
 	struct dwc3_pci		*dwc;
 	struct resource		res[2];
 	int			ret;
@@ -265,7 +276,7 @@ static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	dwc->dwc3->dev.parent = dev;
 	ACPI_COMPANION_SET(&dwc->dwc3->dev, ACPI_COMPANION(dev));
 
-	ret = platform_device_add_properties(dwc->dwc3, p);
+	ret = device_add_software_node(&dwc->dwc3->dev, (void *)id->driver_data);
 	if (ret < 0)
 		goto err;
 
@@ -288,6 +299,7 @@ static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 
 	return 0;
 err:
+	device_remove_software_node(&dwc->dwc3->dev);
 	platform_device_put(dwc->dwc3);
 	return ret;
 }
@@ -304,75 +316,76 @@ static void dwc3_pci_remove(struct pci_dev *pci)
 #endif
 	device_init_wakeup(&pci->dev, false);
 	pm_runtime_get(&pci->dev);
+	device_remove_software_node(&dwc->dwc3->dev);
 	platform_device_unregister(dwc->dwc3);
 }
 
 static const struct pci_device_id dwc3_pci_id_table[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BSW),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BYT),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_MRFLD),
-	  (kernel_ulong_t) &dwc3_pci_mrfld_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_mrfld_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CMLLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CMLH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SPTLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SPTH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BXT),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BXT_M),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_APL),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_KBP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_GLK),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPV),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICLLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_EHLLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGPLP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGPH),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_JSP),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADLS),
-	  (kernel_ulong_t) &dwc3_pci_intel_properties, },
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_NL_USB),
-	  (kernel_ulong_t) &dwc3_pci_amd_properties, },
+	  (kernel_ulong_t) &dwc3_pci_amd_swnode, },
 	{  }	/* Terminating Entry */
 };
 MODULE_DEVICE_TABLE(pci, dwc3_pci_id_table);
-- 
2.29.2

[PATCH v2 3/3] usb: dwc3: pci: ID for Tiger Lake CPU

[Heikki Krogerus]

Tiger Lake SOC (the versions of it that have integrated USB4
controller) may have two DWC3 controllers. One is part of
the PCH (Platform Controller Hub, i.e. the chipset) as
usual, and the other is inside the actual CPU block.

On all Intel platforms that have the two separate DWC3
controllers, the one inside the CPU handles USB3 and only
USB3 traffic, while the PCH version handles USB2 and USB2
alone. The reason for splitting the two busses like this is
to allow easy USB3 tunneling over USB4 connections. As USB2
is not tunneled over USB4, it has dedicated USB controllers
(both xHCI and DWC3).

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

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 037bc21bffa66..51029cec119ed 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -41,6 +41,7 @@
 #define PCI_DEVICE_ID_INTEL_TGPH		0x43ee
 #define PCI_DEVICE_ID_INTEL_JSP			0x4dee
 #define PCI_DEVICE_ID_INTEL_ADLS		0x7ae1
+#define PCI_DEVICE_ID_INTEL_TGL			0x9a15
 
 #define PCI_INTEL_BXT_DSM_GUID		"732b85d5-b7a7-4a1b-9ba0-4bbd00ffd511"
 #define PCI_INTEL_BXT_FUNC_PMU_PWR	4
@@ -384,6 +385,9 @@ static const struct pci_device_id dwc3_pci_id_table[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADLS),
 	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
 
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGL),
+	  (kernel_ulong_t) &dwc3_pci_intel_swnode, },
+
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_NL_USB),
 	  (kernel_ulong_t) &dwc3_pci_amd_swnode, },
 	{  }	/* Terminating Entry */
-- 
2.29.2

Re: [PATCH v2 0/3] Remove one more platform_device_add_properties() call

[Felipe Balbi]

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

Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:

> Hi Felipe, Rafael,
>
> This is the second version of this series. There are no real changes,
> but I added the Tiger Lake ID patch to this series in hope that it
> will make your life a bit easier, assuming that Rafael will still pick
> these.
>
>
> The original over letter:
>
> I originally introduced these as part of my series where I was
> proposing PM ops for software nodes [1], but since that still needs
> work, I'm sending these two separately.
>
> So basically I'm only modifying dwc3-pci.c so it registers a software
> node directly at this point. That will remove one more user of
> platform_device_add_properties().
>
> [1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/
>
> thanks,
>
> Heikki Krogerus (3):
>   software node: Introduce device_add_software_node()
>   usb: dwc3: pci: Register a software node for the dwc3 platform device
>   usb: dwc3: pci: ID for Tiger Lake CPU

Looks good to me.

Acked-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v2 0/3] Remove one more platform_device_add_properties() call

[Greg KH]

On Mon, Jan 11, 2021 at 05:10:42PM +0300, Heikki Krogerus wrote:
> Hi Felipe, Rafael,
> 
> This is the second version of this series. There are no real changes,
> but I added the Tiger Lake ID patch to this series in hope that it
> will make your life a bit easier, assuming that Rafael will still pick
> these.

I can take all 3 of these if that makes it easier.  Rafael, let me know
what you want to do, either is fine with me.

thanks,

greg k-h

Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()

[Daniel Scally]

Hi Heikki

On 11/01/2021 14:10, Heikki Krogerus wrote:
> This helper will register a software node and then assign
> it to device at the same time. The function will also make
> sure that the device can't have more than one software node.
> 
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---

I like this change. One comment below, but for what it's worth:

Reviewed-by: Daniel Scally <djrscally@gmail.com>

> +/**
> + * device_remove_software_node - Remove device's software node
> + * @dev: The device with the software node.
> + *
> + * This function will unregister the software node of @dev.
> + */
> +void device_remove_software_node(struct device *dev)
> +{
> +	struct swnode *swnode;
> +
> +	swnode = dev_to_swnode(dev);
> +	if (!swnode)
> +		return;
> +
> +	kobject_put(&swnode->kobj);
> +}
> +EXPORT_SYMBOL_GPL(device_remove_software_node);

I wonder if this also ought to set dev_fwnode(dev)->secondary back to
ERR_PTR(-ENODEV)?

> +
>  int software_node_notify(struct device *dev, unsigned long action)
>  {
> -	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct swnode *swnode;
>  	int ret;
>  
> -	if (!fwnode)
> -		return 0;
> -
> -	if (!is_software_node(fwnode))
> -		fwnode = fwnode->secondary;
> -	if (!is_software_node(fwnode))
> +	swnode = dev_to_swnode(dev);
> +	if (!swnode)
>  		return 0;
>  
> -	swnode = to_swnode(fwnode);
> -
>  	switch (action) {
>  	case KOBJ_ADD:
>  		ret = sysfs_create_link(&dev->kobj, &swnode->kobj,
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 0a9001fe7aeab..b0e413dc59271 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -488,4 +488,7 @@ fwnode_create_software_node(const struct property_entry *properties,
>  			    const struct fwnode_handle *parent);
>  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);
> +
>  #endif /* _LINUX_PROPERTY_H_ */
> 

Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()

[Heikki Krogerus]

Hi Daniel,

On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote:
> Hi Heikki
> 
> On 11/01/2021 14:10, Heikki Krogerus wrote:
> > This helper will register a software node and then assign
> > it to device at the same time. The function will also make
> > sure that the device can't have more than one software node.
> > 
> > Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> 
> I like this change. One comment below, but for what it's worth:
> 
> Reviewed-by: Daniel Scally <djrscally@gmail.com>

Thanks!

> > +/**
> > + * device_remove_software_node - Remove device's software node
> > + * @dev: The device with the software node.
> > + *
> > + * This function will unregister the software node of @dev.
> > + */
> > +void device_remove_software_node(struct device *dev)
> > +{
> > +	struct swnode *swnode;
> > +
> > +	swnode = dev_to_swnode(dev);
> > +	if (!swnode)
> > +		return;
> > +
> > +	kobject_put(&swnode->kobj);
> > +}
> > +EXPORT_SYMBOL_GPL(device_remove_software_node);
> 
> I wonder if this also ought to set dev_fwnode(dev)->secondary back to
> ERR_PTR(-ENODEV)?

We can't do that here unfortunately. Other places still have a
reference to the swnode at this point and they may still need to
access it using the dev_fwnode(dev)->secondary pointer.

-- 
heikki

Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()

[Andy Shevchenko]

On Wed, Jan 13, 2021 at 01:39:18PM +0200, Heikki Krogerus wrote:
> On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote:
> > On 11/01/2021 14:10, Heikki Krogerus wrote:

...

> > > +/**
> > > + * device_remove_software_node - Remove device's software node
> > > + * @dev: The device with the software node.
> > > + *
> > > + * This function will unregister the software node of @dev.
> > > + */
> > > +void device_remove_software_node(struct device *dev)
> > > +{
> > > +	struct swnode *swnode;
> > > +
> > > +	swnode = dev_to_swnode(dev);
> > > +	if (!swnode)
> > > +		return;
> > > +
> > > +	kobject_put(&swnode->kobj);
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_remove_software_node);
> > 
> > I wonder if this also ought to set dev_fwnode(dev)->secondary back to
> > ERR_PTR(-ENODEV)?

Actually it's a good question.

> We can't do that here unfortunately. Other places still have a
> reference to the swnode at this point and they may still need to
> access it using the dev_fwnode(dev)->secondary pointer.

Yeah, but in this case we potentially leave a dangling pointer when last of the
user gone and kobject_put() will call for release.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()

[Andy Shevchenko]

On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote:
> On 11/01/2021 14:10, Heikki Krogerus wrote:

...

> > +/**
> > + * device_remove_software_node - Remove device's software node
> > + * @dev: The device with the software node.
> > + *
> > + * This function will unregister the software node of @dev.
> > + */
> > +void device_remove_software_node(struct device *dev)
> > +{
> > +	struct swnode *swnode;
> > +
> > +	swnode = dev_to_swnode(dev);
> > +	if (!swnode)
> > +		return;
> > +
> > +	kobject_put(&swnode->kobj);
> > +}
> > +EXPORT_SYMBOL_GPL(device_remove_software_node);
> 
> I wonder if this also ought to set dev_fwnode(dev)->secondary back to
> ERR_PTR(-ENODEV)?

Looking more into this code I think we need to call

	set_secondary_fwnode(dev, NULL);

among these lines.

The real problem is that set_primary_fwnode() and set_secondary_fwnode() have
no reference counting. If we have a chain ->primary->secondary->-ENODEV is
being used somewhere we can't tell from here.

So, in practice it means that we lack of some kind of primary node to increment
reference count of the secondary node when the latter is chained to the given
primary. But it makes things too complicated. Any other options for shared
primary-secondary chain? Standalone primary along with standalone (exclusive)
secondary doesn't need this AFAICS. Perhaps a flag to primary like shared /
exclusive that will prevent breaking the chain in set_secondary_fwnode()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()

[Andy Shevchenko]

On Wed, Jan 13, 2021 at 05:55:04PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote:
> > On 11/01/2021 14:10, Heikki Krogerus wrote:
> 
> ...
> 
> > > +/**
> > > + * device_remove_software_node - Remove device's software node
> > > + * @dev: The device with the software node.
> > > + *
> > > + * This function will unregister the software node of @dev.
> > > + */
> > > +void device_remove_software_node(struct device *dev)
> > > +{
> > > +	struct swnode *swnode;
> > > +
> > > +	swnode = dev_to_swnode(dev);
> > > +	if (!swnode)
> > > +		return;
> > > +
> > > +	kobject_put(&swnode->kobj);
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_remove_software_node);
> > 
> > I wonder if this also ought to set dev_fwnode(dev)->secondary back to
> > ERR_PTR(-ENODEV)?
> 
> Looking more into this code I think we need to call
> 
> 	set_secondary_fwnode(dev, NULL);
> 
> among these lines.
> 
> The real problem is that set_primary_fwnode() and set_secondary_fwnode() have
> no reference counting. If we have a chain ->primary->secondary->-ENODEV is
> being used somewhere we can't tell from here.
> 
> So, in practice it means that we lack of some kind of primary node to increment
> reference count of the secondary node when the latter is chained to the given
> primary. But it makes things too complicated. Any other options for shared
> primary-secondary chain? Standalone primary along with standalone (exclusive)
> secondary doesn't need this AFAICS. Perhaps a flag to primary like shared /
> exclusive that will prevent breaking the chain in set_secondary_fwnode()?

Or maybe I imagined only theoretical cases and we have no such issue?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()

[Heikki Krogerus]

On Wed, Jan 13, 2021 at 05:30:03PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 13, 2021 at 01:39:18PM +0200, Heikki Krogerus wrote:
> > On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote:
> > > On 11/01/2021 14:10, Heikki Krogerus wrote:
> 
> ...
> 
> > > > +/**
> > > > + * device_remove_software_node - Remove device's software node
> > > > + * @dev: The device with the software node.
> > > > + *
> > > > + * This function will unregister the software node of @dev.
> > > > + */
> > > > +void device_remove_software_node(struct device *dev)
> > > > +{
> > > > +	struct swnode *swnode;
> > > > +
> > > > +	swnode = dev_to_swnode(dev);
> > > > +	if (!swnode)
> > > > +		return;
> > > > +
> > > > +	kobject_put(&swnode->kobj);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_remove_software_node);
> > > 
> > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to
> > > ERR_PTR(-ENODEV)?
> 
> Actually it's a good question.
> 
> > We can't do that here unfortunately. Other places still have a
> > reference to the swnode at this point and they may still need to
> > access it using the dev_fwnode(dev)->secondary pointer.
> 
> Yeah, but in this case we potentially leave a dangling pointer when last of the
> user gone and kobject_put() will call for release.

The caller has to be responsible of setting the secondary back to
ERR_PTR(-ENODEV). We can not do anything here like I explained. We can
not even do that in software_node_notify() when the association to the
struct device is removed, because the fwnode->secondary is still
accessed after that. The caller needs to remove both the node and the
device, and only after that it is safe to set the secondary back to
ERR_PTR(-ENODEV).

This clearly needs to explained in the comment... I'll fix it.


thanks,

-- 
heikki

Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()

[Heikki Krogerus]

On Wed, Jan 13, 2021 at 05:58:12PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 13, 2021 at 05:55:04PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote:
> > > On 11/01/2021 14:10, Heikki Krogerus wrote:
> > 
> > ...
> > 
> > > > +/**
> > > > + * device_remove_software_node - Remove device's software node
> > > > + * @dev: The device with the software node.
> > > > + *
> > > > + * This function will unregister the software node of @dev.
> > > > + */
> > > > +void device_remove_software_node(struct device *dev)
> > > > +{
> > > > +	struct swnode *swnode;
> > > > +
> > > > +	swnode = dev_to_swnode(dev);
> > > > +	if (!swnode)
> > > > +		return;
> > > > +
> > > > +	kobject_put(&swnode->kobj);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_remove_software_node);
> > > 
> > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to
> > > ERR_PTR(-ENODEV)?
> > 
> > Looking more into this code I think we need to call
> > 
> > 	set_secondary_fwnode(dev, NULL);
> > 
> > among these lines.
> > 
> > The real problem is that set_primary_fwnode() and set_secondary_fwnode() have
> > no reference counting. If we have a chain ->primary->secondary->-ENODEV is
> > being used somewhere we can't tell from here.
> > 
> > So, in practice it means that we lack of some kind of primary node to increment
> > reference count of the secondary node when the latter is chained to the given
> > primary. But it makes things too complicated. Any other options for shared
> > primary-secondary chain? Standalone primary along with standalone (exclusive)
> > secondary doesn't need this AFAICS. Perhaps a flag to primary like shared /
> > exclusive that will prevent breaking the chain in set_secondary_fwnode()?
> 
> Or maybe I imagined only theoretical cases and we have no such issue?

I think we should really start looking into the possibility of
removing the whole secondary coupling, because that is the thing that
is crippling us.

Br,

-- 
heikki

Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()

[Heikki Krogerus]

On Thu, Jan 14, 2021 at 03:19:52PM +0200, Heikki Krogerus wrote:
> On Wed, Jan 13, 2021 at 05:30:03PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 13, 2021 at 01:39:18PM +0200, Heikki Krogerus wrote:
> > > On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote:
> > > > On 11/01/2021 14:10, Heikki Krogerus wrote:
> > 
> > ...
> > 
> > > > > +/**
> > > > > + * device_remove_software_node - Remove device's software node
> > > > > + * @dev: The device with the software node.
> > > > > + *
> > > > > + * This function will unregister the software node of @dev.
> > > > > + */
> > > > > +void device_remove_software_node(struct device *dev)
> > > > > +{
> > > > > +	struct swnode *swnode;
> > > > > +
> > > > > +	swnode = dev_to_swnode(dev);
> > > > > +	if (!swnode)
> > > > > +		return;
> > > > > +
> > > > > +	kobject_put(&swnode->kobj);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(device_remove_software_node);
> > > > 
> > > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to
> > > > ERR_PTR(-ENODEV)?
> > 
> > Actually it's a good question.
> > 
> > > We can't do that here unfortunately. Other places still have a
> > > reference to the swnode at this point and they may still need to
> > > access it using the dev_fwnode(dev)->secondary pointer.
> > 
> > Yeah, but in this case we potentially leave a dangling pointer when last of the
> > user gone and kobject_put() will call for release.
> 
> The caller has to be responsible of setting the secondary back to
> ERR_PTR(-ENODEV). We can not do anything here like I explained. We can
> not even do that in software_node_notify() when the association to the
> struct device is removed, because the fwnode->secondary is still
> accessed after that. The caller needs to remove both the node and the
> device, and only after that it is safe to set the secondary back to
> ERR_PTR(-ENODEV).

I studied the code again, and it actually looks like this is only a
problem when device_add_properties() is used and there is an
expectation that the node/properties are removed automatically in
device_del().

When this new API is used, the only place that needs to access the
swnode using the secondary pointer is software_node_notify(), so if we
simply handle that separately here, we should be able to clear the
secondary pointer after all. It would look something like this:

        void device_remove_software_node(struct device *dev)
        {
        	struct swnode *swnode;
        
        	swnode = dev_to_swnode(dev);
        	if (!swnode)
        		return;
        
                software_node_notify(dev, KOBJ_REMOVE);
                set_secondary_fwnode(dev, NULL);
        	kobject_put(&swnode->kobj);
        }

I'll test that, and if it works, and you guys don't see any problems
with it, I'll use it in v3.


Br,

-- 
heikki

Re: [PATCH v2 0/3] Remove one more platform_device_add_properties() call

[Greg KH]

On Tue, Jan 12, 2021 at 12:49:14PM +0100, Greg KH wrote:
> On Mon, Jan 11, 2021 at 05:10:42PM +0300, Heikki Krogerus wrote:
> > Hi Felipe, Rafael,
> > 
> > This is the second version of this series. There are no real changes,
> > but I added the Tiger Lake ID patch to this series in hope that it
> > will make your life a bit easier, assuming that Rafael will still pick
> > these.
> 
> I can take all 3 of these if that makes it easier.  Rafael, let me know
> what you want to do, either is fine with me.

I've added it to my usb-next branch now.

thanks,

greg k-h

Re: [PATCH v2 0/3] Remove one more platform_device_add_properties() call

[Greg KH]

On Fri, Jan 15, 2021 at 04:01:37PM +0100, Greg KH wrote:
> On Tue, Jan 12, 2021 at 12:49:14PM +0100, Greg KH wrote:
> > On Mon, Jan 11, 2021 at 05:10:42PM +0300, Heikki Krogerus wrote:
> > > Hi Felipe, Rafael,
> > > 
> > > This is the second version of this series. There are no real changes,
> > > but I added the Tiger Lake ID patch to this series in hope that it
> > > will make your life a bit easier, assuming that Rafael will still pick
> > > these.
> > 
> > I can take all 3 of these if that makes it easier.  Rafael, let me know
> > what you want to do, either is fine with me.
> 
> I've added it to my usb-next branch now.

Argh, grabbed the wrong one, will drop this and grab v3...