| Message ID | 20200321045448.15192-1-saravanak@google.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v1] driver core: Add device links from fwnode only for the primary device | expand |
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 >
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?
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
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);
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(-)