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@xxxxxxxxxx> 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@xxxxxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>

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

> ---
>  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
>