[PATCH v2 0/3] software node: Introduce software_node_find_by_name()

[PATCH v2 0/3] software node: Introduce software_node_find_by_name()

[Heikki Krogerus]

Hi,

This is the second version of this series where I'm introducing that
helper.

Hans and Andy! Because of the changes I made to patch 2/3, I'm not
carrying your reviewed-by tags in it. I would appreciate if you
could take another look at that patch.

I added a note to the kernel-doc comment in patch 1/3 that the caller
of the helper function needs to release the ref count after use as
proposed by Andy.

In patch 2/3, since we have to now modify the role switch descriptor,
I'm filling the structure in stack memory and removing the constant
static version. The content of the descriptor is copied during switch
registration in any case, so we don't need to store it in the driver.

I also noticed a bug in 2/3. I never properly destroyed the software
node when the mux was removed. That leak is now also fixed.

thanks,


Heikki Krogerus (3):
  software node: Add software_node_find_by_name()
  usb: roles: intel_xhci: Supplying software node for the role mux
  platform/x86: intel_cht_int33fe: Use new API to gain access to the
    role switch

 drivers/base/swnode.c                         | 37 ++++++++++++
 drivers/platform/x86/intel_cht_int33fe.c      | 57 ++++---------------
 .../usb/roles/intel-xhci-usb-role-switch.c    | 23 ++++++--
 include/linux/property.h                      |  4 ++
 4 files changed, 68 insertions(+), 53 deletions(-)

-- 
2.23.0.rc1

[PATCH v2 1/3] software node: Add software_node_find_by_name()

[Heikki Krogerus]

Function that searches software nodes by node name.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/base/swnode.c    | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  4 ++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index e7b3aa3bd55a..ee2a405cca9a 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -620,6 +620,43 @@ static const struct fwnode_operations software_node_ops = {
 
 /* -------------------------------------------------------------------------- */
 
+/**
+ * software_node_find_by_name - Find software node by name
+ * @parent: Parent of the software node
+ * @name: Name of the software node
+ *
+ * The function will find a node that is child of @parent and that is named
+ * @name. If no node is found, the function returns NULL.
+ *
+ * NOTE: you will need to drop the reference with fwnode_handle_put() after use.
+ */
+const struct software_node *
+software_node_find_by_name(const struct software_node *parent, const char *name)
+{
+	struct swnode *swnode;
+	struct kobject *k;
+
+	if (!name)
+		return NULL;
+
+	spin_lock(&swnode_kset->list_lock);
+
+	list_for_each_entry(k, &swnode_kset->list, entry) {
+		swnode = kobj_to_swnode(k);
+		if (parent == swnode->node->parent && swnode->node->name &&
+		    !strcmp(name, swnode->node->name)) {
+			kobject_get(&swnode->kobj);
+			break;
+		}
+		swnode = NULL;
+	}
+
+	spin_unlock(&swnode_kset->list_lock);
+
+	return swnode ? swnode->node : NULL;
+}
+EXPORT_SYMBOL_GPL(software_node_find_by_name);
+
 static int
 software_node_register_properties(struct software_node *node,
 				  const struct property_entry *properties)
diff --git a/include/linux/property.h b/include/linux/property.h
index 5a910ad79591..9b3d4ca3a73a 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -421,6 +421,10 @@ bool is_software_node(const struct fwnode_handle *fwnode);
 const struct software_node *to_software_node(struct fwnode_handle *fwnode);
 struct fwnode_handle *software_node_fwnode(const struct software_node *node);
 
+const struct software_node *
+software_node_find_by_name(const struct software_node *parent,
+			   const char *name);
+
 int software_node_register_nodes(const struct software_node *nodes);
 void software_node_unregister_nodes(const struct software_node *nodes);
 
-- 
2.23.0.rc1

Re: [PATCH v2 1/3] software node: Add software_node_find_by_name()

[Andy Shevchenko]

On Fri, Aug 16, 2019 at 1:45 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Function that searches software nodes by node name.
>

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/base/swnode.c    | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/property.h |  4 ++++
>  2 files changed, 41 insertions(+)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index e7b3aa3bd55a..ee2a405cca9a 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -620,6 +620,43 @@ static const struct fwnode_operations software_node_ops = {
>
>  /* -------------------------------------------------------------------------- */
>
> +/**
> + * software_node_find_by_name - Find software node by name
> + * @parent: Parent of the software node
> + * @name: Name of the software node
> + *
> + * The function will find a node that is child of @parent and that is named
> + * @name. If no node is found, the function returns NULL.
> + *
> + * NOTE: you will need to drop the reference with fwnode_handle_put() after use.
> + */
> +const struct software_node *
> +software_node_find_by_name(const struct software_node *parent, const char *name)
> +{
> +       struct swnode *swnode;
> +       struct kobject *k;
> +
> +       if (!name)
> +               return NULL;
> +
> +       spin_lock(&swnode_kset->list_lock);
> +
> +       list_for_each_entry(k, &swnode_kset->list, entry) {
> +               swnode = kobj_to_swnode(k);
> +               if (parent == swnode->node->parent && swnode->node->name &&
> +                   !strcmp(name, swnode->node->name)) {
> +                       kobject_get(&swnode->kobj);
> +                       break;
> +               }
> +               swnode = NULL;
> +       }
> +
> +       spin_unlock(&swnode_kset->list_lock);
> +
> +       return swnode ? swnode->node : NULL;
> +}
> +EXPORT_SYMBOL_GPL(software_node_find_by_name);
> +
>  static int
>  software_node_register_properties(struct software_node *node,
>                                   const struct property_entry *properties)
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 5a910ad79591..9b3d4ca3a73a 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -421,6 +421,10 @@ bool is_software_node(const struct fwnode_handle *fwnode);
>  const struct software_node *to_software_node(struct fwnode_handle *fwnode);
>  struct fwnode_handle *software_node_fwnode(const struct software_node *node);
>
> +const struct software_node *
> +software_node_find_by_name(const struct software_node *parent,
> +                          const char *name);
> +
>  int software_node_register_nodes(const struct software_node *nodes);
>  void software_node_unregister_nodes(const struct software_node *nodes);
>
> --
> 2.23.0.rc1
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/3] usb: roles: intel_xhci: Supplying software node for the role mux

[Heikki Krogerus]

The primary purpose for this node will be to allow linking
the users of the switch to it. The users will be for example
USB Type-C connectors. By supplying a reference to this
node in the software nodes representing the USB Type-C
controllers or connectors, the drivers for those devices can
access the switch.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 .../usb/roles/intel-xhci-usb-role-switch.c    | 23 ++++++++++++++-----
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index 277de96181f9..2b1fe2700915 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -39,6 +39,10 @@ struct intel_xhci_usb_data {
 	void __iomem *base;
 };
 
+static const struct software_node intel_xhci_usb_node = {
+	"intel-xhci-usb-sw",
+};
+
 static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
 {
 	struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
@@ -122,17 +126,13 @@ static enum usb_role intel_xhci_usb_get_role(struct device *dev)
 	return role;
 }
 
-static const struct usb_role_switch_desc sw_desc = {
-	.set = intel_xhci_usb_set_role,
-	.get = intel_xhci_usb_get_role,
-	.allow_userspace_control = true,
-};
-
 static int intel_xhci_usb_probe(struct platform_device *pdev)
 {
+	struct usb_role_switch_desc sw_desc = { };
 	struct device *dev = &pdev->dev;
 	struct intel_xhci_usb_data *data;
 	struct resource *res;
+	int ret;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -147,6 +147,15 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
+	ret = software_node_register(&intel_xhci_usb_node);
+	if (ret)
+		return ret;
+
+	sw_desc.set = intel_xhci_usb_set_role,
+	sw_desc.get = intel_xhci_usb_get_role,
+	sw_desc.allow_userspace_control = true,
+	sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
+
 	data->role_sw = usb_role_switch_register(dev, &sw_desc);
 	if (IS_ERR(data->role_sw))
 		return PTR_ERR(data->role_sw);
@@ -164,6 +173,8 @@ static int intel_xhci_usb_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 
 	usb_role_switch_unregister(data->role_sw);
+	fwnode_handle_put(software_node_fwnode(&intel_xhci_usb_node));
+
 	return 0;
 }
 
-- 
2.23.0.rc1

Re: [PATCH v2 2/3] usb: roles: intel_xhci: Supplying software node for the role mux

[Andy Shevchenko]

On Fri, Aug 16, 2019 at 1:45 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> The primary purpose for this node will be to allow linking
> the users of the switch to it. The users will be for example
> USB Type-C connectors. By supplying a reference to this
> node in the software nodes representing the USB Type-C
> controllers or connectors, the drivers for those devices can
> access the switch.

> +       ret = software_node_register(&intel_xhci_usb_node);
> +       if (ret)
> +               return ret;
> +
> +       sw_desc.set = intel_xhci_usb_set_role,
> +       sw_desc.get = intel_xhci_usb_get_role,
> +       sw_desc.allow_userspace_control = true,
> +       sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
> +
>         data->role_sw = usb_role_switch_register(dev, &sw_desc);
>         if (IS_ERR(data->role_sw))
>                 return PTR_ERR(data->role_sw);

Sounds to me like more fwnode_handle_put() calls are missed.

> @@ -164,6 +173,8 @@ static int intel_xhci_usb_remove(struct platform_device *pdev)
>         pm_runtime_disable(&pdev->dev);
>
>         usb_role_switch_unregister(data->role_sw);
> +       fwnode_handle_put(software_node_fwnode(&intel_xhci_usb_node));


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] usb: roles: intel_xhci: Supplying software node for the role mux

[Heikki Krogerus]

On Fri, Aug 16, 2019 at 04:45:50PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 16, 2019 at 1:45 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > The primary purpose for this node will be to allow linking
> > the users of the switch to it. The users will be for example
> > USB Type-C connectors. By supplying a reference to this
> > node in the software nodes representing the USB Type-C
> > controllers or connectors, the drivers for those devices can
> > access the switch.
> 
> > +       ret = software_node_register(&intel_xhci_usb_node);
> > +       if (ret)
> > +               return ret;
> > +
> > +       sw_desc.set = intel_xhci_usb_set_role,
> > +       sw_desc.get = intel_xhci_usb_get_role,
> > +       sw_desc.allow_userspace_control = true,
> > +       sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
> > +
> >         data->role_sw = usb_role_switch_register(dev, &sw_desc);
> >         if (IS_ERR(data->role_sw))
> >                 return PTR_ERR(data->role_sw);
> 
> Sounds to me like more fwnode_handle_put() calls are missed.

True. I'll fix it.

thanks,

-- 
heikki

Re: [PATCH v2 2/3] usb: roles: intel_xhci: Supplying software node for the role mux

[Hans de Goede]

Hi,

On 8/16/19 3:52 PM, Heikki Krogerus wrote:
> On Fri, Aug 16, 2019 at 04:45:50PM +0300, Andy Shevchenko wrote:
>> On Fri, Aug 16, 2019 at 1:45 PM Heikki Krogerus
>> <heikki.krogerus@linux.intel.com> wrote:
>>>
>>> The primary purpose for this node will be to allow linking
>>> the users of the switch to it. The users will be for example
>>> USB Type-C connectors. By supplying a reference to this
>>> node in the software nodes representing the USB Type-C
>>> controllers or connectors, the drivers for those devices can
>>> access the switch.
>>
>>> +       ret = software_node_register(&intel_xhci_usb_node);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       sw_desc.set = intel_xhci_usb_set_role,
>>> +       sw_desc.get = intel_xhci_usb_get_role,
>>> +       sw_desc.allow_userspace_control = true,
>>> +       sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
>>> +
>>>          data->role_sw = usb_role_switch_register(dev, &sw_desc);
>>>          if (IS_ERR(data->role_sw))
>>>                  return PTR_ERR(data->role_sw);
>>
>> Sounds to me like more fwnode_handle_put() calls are missed.
> 
> True. I'll fix it.

I will wait for v3 before testing again then.

Regards,

Hans

[PATCH v2 3/3] platform/x86: intel_cht_int33fe: Use new API to gain access to the role switch

[Heikki Krogerus]

The driver for the Intel USB role mux now always supplies
software node for the role switch, so no longer checking
that, and never creating separate node for the role switch.
From now on using software_node_find_by_name() function to
get the handle to the USB role switch.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 57 +++++-------------------
 1 file changed, 10 insertions(+), 47 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 4fbdff48a4b5..1d5d877b9582 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -34,7 +34,6 @@ enum {
 	INT33FE_NODE_MAX17047,
 	INT33FE_NODE_PI3USB30532,
 	INT33FE_NODE_DISPLAYPORT,
-	INT33FE_NODE_ROLE_SWITCH,
 	INT33FE_NODE_USB_CONNECTOR,
 	INT33FE_NODE_MAX,
 };
@@ -45,7 +44,6 @@ struct cht_int33fe_data {
 	struct i2c_client *pi3usb30532;
 
 	struct fwnode_handle *dp;
-	struct fwnode_handle *mux;
 };
 
 static const struct software_node nodes[];
@@ -139,46 +137,10 @@ static const struct software_node nodes[] = {
 	{ "max17047", NULL, max17047_props },
 	{ "pi3usb30532" },
 	{ "displayport" },
-	{ "usb-role-switch" },
 	{ "connector", &nodes[0], usb_connector_props, usb_connector_refs },
 	{ }
 };
 
-static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
-{
-	struct fwnode_handle *fwnode;
-	struct device *dev;
-	struct device *p;
-
-	fwnode = software_node_fwnode(&nodes[INT33FE_NODE_ROLE_SWITCH]);
-	if (!fwnode)
-		return -ENODEV;
-
-	/* First finding the platform device */
-	p = bus_find_device_by_name(&platform_bus_type, NULL,
-				    "intel_xhci_usb_sw");
-	if (!p)
-		return -EPROBE_DEFER;
-
-	/* Then the mux child device */
-	dev = device_find_child_by_name(p, "intel_xhci_usb_sw-role-switch");
-	put_device(p);
-	if (!dev)
-		return -EPROBE_DEFER;
-
-	/* If there already is a node for the mux, using that one. */
-	if (dev->fwnode)
-		fwnode_remove_software_node(fwnode);
-	else
-		dev->fwnode = fwnode;
-
-	data->mux = fwnode_handle_get(dev->fwnode);
-	put_device(dev);
-	mux_ref.node = to_software_node(data->mux);
-
-	return 0;
-}
-
 static int cht_int33fe_setup_dp(struct cht_int33fe_data *data)
 {
 	struct fwnode_handle *fwnode;
@@ -211,10 +173,9 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 {
 	software_node_unregister_nodes(nodes);
 
-	if (data->mux) {
-		fwnode_handle_put(data->mux);
+	if (mux_ref.node) {
+		fwnode_handle_put(software_node_fwnode(mux_ref.node));
 		mux_ref.node = NULL;
-		data->mux = NULL;
 	}
 
 	if (data->dp) {
@@ -235,14 +196,16 @@ static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
 	/* The devices that are not created in this driver need extra steps. */
 
 	/*
-	 * There is no ACPI device node for the USB role mux, so we need to find
-	 * the mux device and assign our node directly to it. That means we
-	 * depend on the mux driver. This function will return -PROBE_DEFER
-	 * until the mux device is registered.
+	 * There is no ACPI device node for the USB role mux, so we need to wait
+	 * until the mux driver has created software node for the mux device.
+	 * It means we depend on the mux driver. This function will return
+	 * -EPROBE_DEFER until the mux device is registered.
 	 */
-	ret = cht_int33fe_setup_mux(data);
-	if (ret)
+	mux_ref.node = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
+	if (!mux_ref.node) {
+		ret = -EPROBE_DEFER;
 		goto err_remove_nodes;
+	}
 
 	/*
 	 * The DP connector does have ACPI device node. In this case we can just
-- 
2.23.0.rc1