[PATCH v1] driver core: Add device links from fwnode only for the primary device

[PATCH v1] driver core: Add device links from fwnode only for the primary device

[Saravana Kannan]

Sometimes, more than one (generally two) device can point to the same
fwnode.  However, only one device is set as the fwnode's device
(fwnode->dev) and can be looked up from the fwnode.

Typically, only one of these devices actually have a driver and actually
probe. If we create device links for all these devices, then the
suppliers' of these devices (with the same fwnode) will never get a
sync_state() call because one of their consumer devices will never probe
(because they don't have a driver).

So, create device links only for the device that is considered as the
fwnode's device.

One such example of this is the PCI bridge platform_device and the
corresponding pci_bus device. Both these devices will have the same
fwnode. It's the platform_device that is registered first and is set as
the fwnode's device. Also the platform_device is the one that actually
probes. Without this patch none of the suppliers of a PCI bridge
platform_device would get a sync_state() callback.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index fc6a60998cd6..5e3cc1651c78 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2404,6 +2404,7 @@ int device_add(struct device *dev)
 	struct class_interface *class_intf;
 	int error = -EINVAL, fw_ret;
 	struct kobject *glue_dir = NULL;
+	bool is_fwnode_dev = false;
 
 	dev = get_device(dev);
 	if (!dev)
@@ -2501,8 +2502,10 @@ int device_add(struct device *dev)
 
 	kobject_uevent(&dev->kobj, KOBJ_ADD);
 
-	if (dev->fwnode && !dev->fwnode->dev)
+	if (dev->fwnode && !dev->fwnode->dev) {
 		dev->fwnode->dev = dev;
+		is_fwnode_dev = true;
+	}
 
 	/*
 	 * Check if any of the other devices (consumers) have been waiting for
@@ -2518,7 +2521,8 @@ int device_add(struct device *dev)
 	 */
 	device_link_add_missing_supplier_links();
 
-	if (fw_devlink_flags && fwnode_has_op(dev->fwnode, add_links)) {
+	if (fw_devlink_flags && is_fwnode_dev &&
+	    fwnode_has_op(dev->fwnode, add_links)) {
 		fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
 		if (fw_ret == -ENODEV)
 			device_link_wait_for_mandatory_supplier(dev);
-- 
2.25.1.696.g5e7596f4ac-goog

Re: [PATCH v1] driver core: Add device links from fwnode only for the primary device

[Rafael J. Wysocki]

On Sat, Mar 21, 2020 at 5:55 AM Saravana Kannan <saravanak@google.com> wrote:
>
> Sometimes, more than one (generally two) device can point to the same
> fwnode.  However, only one device is set as the fwnode's device
> (fwnode->dev) and can be looked up from the fwnode.
>
> Typically, only one of these devices actually have a driver and actually
> probe. If we create device links for all these devices, then the
> suppliers' of these devices (with the same fwnode) will never get a
> sync_state() call because one of their consumer devices will never probe
> (because they don't have a driver).
>
> So, create device links only for the device that is considered as the
> fwnode's device.
>
> One such example of this is the PCI bridge platform_device and the
> corresponding pci_bus device. Both these devices will have the same
> fwnode. It's the platform_device that is registered first and is set as
> the fwnode's device. Also the platform_device is the one that actually
> probes. Without this patch none of the suppliers of a PCI bridge
> platform_device would get a sync_state() callback.

For the record, I think that this is a PCI subsystem problem, but I
agree with the patch here.

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Saravana Kannan <saravanak@google.com>

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

> ---
>  drivers/base/core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index fc6a60998cd6..5e3cc1651c78 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2404,6 +2404,7 @@ int device_add(struct device *dev)
>         struct class_interface *class_intf;
>         int error = -EINVAL, fw_ret;
>         struct kobject *glue_dir = NULL;
> +       bool is_fwnode_dev = false;
>
>         dev = get_device(dev);
>         if (!dev)
> @@ -2501,8 +2502,10 @@ int device_add(struct device *dev)
>
>         kobject_uevent(&dev->kobj, KOBJ_ADD);
>
> -       if (dev->fwnode && !dev->fwnode->dev)
> +       if (dev->fwnode && !dev->fwnode->dev) {
>                 dev->fwnode->dev = dev;
> +               is_fwnode_dev = true;
> +       }
>
>         /*
>          * Check if any of the other devices (consumers) have been waiting for
> @@ -2518,7 +2521,8 @@ int device_add(struct device *dev)
>          */
>         device_link_add_missing_supplier_links();
>
> -       if (fw_devlink_flags && fwnode_has_op(dev->fwnode, add_links)) {
> +       if (fw_devlink_flags && is_fwnode_dev &&
> +           fwnode_has_op(dev->fwnode, add_links)) {
>                 fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
>                 if (fw_ret == -ENODEV)
>                         device_link_wait_for_mandatory_supplier(dev);
> --
> 2.25.1.696.g5e7596f4ac-goog
>

Re: [PATCH v1] driver core: Add device links from fwnode only for the primary device

[Bjorn Helgaas]

On Sat, Mar 21, 2020 at 11:20:07AM +0100, Rafael J. Wysocki wrote:
> On Sat, Mar 21, 2020 at 5:55 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Sometimes, more than one (generally two) device can point to the same
> > fwnode.  However, only one device is set as the fwnode's device
> > (fwnode->dev) and can be looked up from the fwnode.
> >
> > Typically, only one of these devices actually have a driver and actually
> > probe. If we create device links for all these devices, then the
> > suppliers' of these devices (with the same fwnode) will never get a
> > sync_state() call because one of their consumer devices will never probe
> > (because they don't have a driver).
> >
> > So, create device links only for the device that is considered as the
> > fwnode's device.
> >
> > One such example of this is the PCI bridge platform_device and the
> > corresponding pci_bus device. Both these devices will have the same
> > fwnode. It's the platform_device that is registered first and is set as
> > the fwnode's device. Also the platform_device is the one that actually
> > probes. Without this patch none of the suppliers of a PCI bridge
> > platform_device would get a sync_state() callback.
> 
> For the record, I think that this is a PCI subsystem problem, but I
> agree with the patch here.

I don't understand the issue here.  Can somebody educate me?  I'm
guessing this is related to pci_set_bus_of_node(), which does (for
PCI-to-PCI bridges):

  bus->dev.of_node = of_node_get(bus->self->dev.of_node);
  bus->dev.fwnode = &bus->dev.of_node->fwnode;

where "bus" points to a struct pci_bus and "bus->self" points to the
struct pci_dev for the bridge leading to the bus?

Is this related to the fact that we have a struct device for both a
PCI-to-PCI bridge and for its downstream bus?

Any suggestions for how could we fix this problem in the PCI
subsystem?

Re: [PATCH v1] driver core: Add device links from fwnode only for the primary device

[Saravana Kannan]

On Mon, Mar 23, 2020 at 3:28 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Sat, Mar 21, 2020 at 11:20:07AM +0100, Rafael J. Wysocki wrote:
> > On Sat, Mar 21, 2020 at 5:55 AM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Sometimes, more than one (generally two) device can point to the same
> > > fwnode.  However, only one device is set as the fwnode's device
> > > (fwnode->dev) and can be looked up from the fwnode.
> > >
> > > Typically, only one of these devices actually have a driver and actually
> > > probe. If we create device links for all these devices, then the
> > > suppliers' of these devices (with the same fwnode) will never get a
> > > sync_state() call because one of their consumer devices will never probe
> > > (because they don't have a driver).
> > >
> > > So, create device links only for the device that is considered as the
> > > fwnode's device.
> > >
> > > One such example of this is the PCI bridge platform_device and the
> > > corresponding pci_bus device. Both these devices will have the same
> > > fwnode. It's the platform_device that is registered first and is set as
> > > the fwnode's device. Also the platform_device is the one that actually
> > > probes. Without this patch none of the suppliers of a PCI bridge
> > > platform_device would get a sync_state() callback.
> >
> > For the record, I think that this is a PCI subsystem problem, but I
> > agree with the patch here.
>
> I don't understand the issue here.  Can somebody educate me?  I'm
> guessing this is related to pci_set_bus_of_node(), which does (for
> PCI-to-PCI bridges):
>
>   bus->dev.of_node = of_node_get(bus->self->dev.of_node);
>   bus->dev.fwnode = &bus->dev.of_node->fwnode;

Assuming you intentionally simplified the code here, yes, it's related to that.

> where "bus" points to a struct pci_bus and "bus->self" points to the
> struct pci_dev for the bridge leading to the bus?
>
> Is this related to the fact that we have a struct device for both a
> PCI-to-PCI bridge and for its downstream bus?

This patch at least isn't talking about how many devices we have.

The patch is referring to the fact that more than one device has their
dev.fwnode point to the same fwnode. fwnode is just a generic way to
point to devicetree nodes (of_node) or ACPI nodes. So the concerns
raised for fwnode apply to of_node too, but ignore of_node for now
(I'll get to that part later).

dev.fwnode is supposed to point to the firmware node from which the
device is created or represents. Having more than one struct device
point to the same fwnode is unusual as it's unlikely one firmware node
is creating two different devices.. Maybe for MFD (multi function
devices) it *might* make sense but even then it's questionable.

In the specific case of the PCI + a device tree based system, the pcie
root controller/bridge(?) has a platform_device (bridge->dev.parent)
that points to a fwnode (that corresponds to the DT node from which
the platform device was created). Somehow (the code path is very
confusing) the pci_bus->dev.fwnode ends up pointing to the same fwnode
the platform_device is pointing to.

The pci_bus is just a run time allocated struct device used to form
some kind of device hierarchy you are trying to maintain. I think (and
maybe this is the part Rafael is referring to) the pci_bus is not
really representing the firmware node and maybe should have fwnode set
to NULL. And this fwnode issue is made more unusual because this
device doesn't even probe.

> Any suggestions for how could we fix this problem in the PCI
> subsystem?

If you set a device's of_node = something, then you really should be
setting the device's fwnode to point to the corresponding fwnode for
that of_node. So the real question is why you need to set
pci_bus->dev.of_node (instead of leaving it NULL). Sometimes devices
have their of_node set to some other device's of_node because
regulator_get()/clk_get/whatever_get() they call look at dev.of_node
to find the resource. But I don't think that's the case here.

So, if you can simply skip setting pci_bus of_node, then that's the
simplest fix. If not, not setting fwnode (while setting of_node) might
be an acceptable hack to reduce the weirdness (of setting fwnode =
some other device's fwnode). If not, then the fix would be to unwind
the need for setting pci_bus's of_node.

Hope that makes some sense.

-Saravana

-Saravana