linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] driver core: Add device links from fwnode only for the primary device
@ 2020-03-21  4:54 Saravana Kannan
  2020-03-21 10:20 ` Rafael J. Wysocki
  0 siblings, 1 reply; 4+ messages in thread
From: Saravana Kannan @ 2020-03-21  4:54 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki
  Cc: Saravana Kannan, Bjorn Helgaas, linux-pci, kernel-team, linux-kernel

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


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

* Re: [PATCH v1] driver core: Add device links from fwnode only for the primary device
  2020-03-21  4:54 [PATCH v1] driver core: Add device links from fwnode only for the primary device Saravana Kannan
@ 2020-03-21 10:20 ` Rafael J. Wysocki
  2020-03-23 22:28   ` Bjorn Helgaas
  0 siblings, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2020-03-21 10:20 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Linux PCI,
	Cc: Android Kernel, Linux Kernel Mailing List

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
>

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

* Re: [PATCH v1] driver core: Add device links from fwnode only for the primary device
  2020-03-21 10:20 ` Rafael J. Wysocki
@ 2020-03-23 22:28   ` Bjorn Helgaas
  2020-03-23 23:54     ` Saravana Kannan
  0 siblings, 1 reply; 4+ messages in thread
From: Bjorn Helgaas @ 2020-03-23 22:28 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Saravana Kannan, Greg Kroah-Hartman, Linux PCI,
	Cc: Android Kernel, Linux Kernel Mailing List

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?

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

* Re: [PATCH v1] driver core: Add device links from fwnode only for the primary device
  2020-03-23 22:28   ` Bjorn Helgaas
@ 2020-03-23 23:54     ` Saravana Kannan
  0 siblings, 0 replies; 4+ messages in thread
From: Saravana Kannan @ 2020-03-23 23:54 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Linux PCI,
	Cc: Android Kernel, Linux Kernel Mailing List

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

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

end of thread, other threads:[~2020-03-23 23:54 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-21  4:54 [PATCH v1] driver core: Add device links from fwnode only for the primary device Saravana Kannan
2020-03-21 10:20 ` Rafael J. Wysocki
2020-03-23 22:28   ` Bjorn Helgaas
2020-03-23 23:54     ` Saravana Kannan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).