Re: [PATCH v4 0/8] Make fw_devlink=on more forgiving

[Saravana Kannan <saravanak@google.com>]

On Wed, Feb 10, 2021 at 12:19 AM <Tudor.Ambarus@microchip.com> wrote:
>
> Hi, Saravana,
>
> On 2/6/21 12:26 AM, Saravana Kannan wrote:
> > There are a lot of devices/drivers where they never have a struct device
> > created for them or the driver initializes the hardware without ever
> > binding to the struct device.
> >
> > This series is intended to avoid any boot regressions due to such
> > devices/drivers when fw_devlink=on and also address the handling of
> > optional suppliers.
> >
> > Patch 1 and 2 addresses the issue of firmware nodes that look like
> > they'll have struct devices created for them, but will never actually
> > have struct devices added for them. For example, DT nodes with a
> > compatible property that don't have devices added for them.
> >
> > Patch 3 and 4 allow for handling optional DT bindings.
> >
> > Patch 5 sets up a generic API to handle drivers that never bind with
> > their devices.
> >
> > Patch 6 through 8 update different frameworks to use the new API.
> >
> > Thanks,
> > Saravana
> >
> > Saravana Kannan (8):
> >   driver core: fw_devlink: Detect supplier devices that will never be
> >     added
> >   of: property: Don't add links to absent suppliers
> >   driver core: Add fw_devlink.strict kernel param
> >   of: property: Add fw_devlink support for optional properties
> >   driver core: fw_devlink: Handle suppliers that don't use driver core
> >   irqdomain: Mark fwnodes when their irqdomain is added/removed
> >   PM: domains: Mark fwnodes when their powerdomain is added/removed
> >   clk: Mark fwnodes when their clock provider is added/removed
> >
> >  .../admin-guide/kernel-parameters.txt         |  5 ++
> >  drivers/base/core.c                           | 58 ++++++++++++++++++-
> >  drivers/base/power/domain.c                   |  2 +
> >  drivers/clk/clk.c                             |  3 +
> >  drivers/of/property.c                         | 16 +++--
> >  include/linux/fwnode.h                        | 20 ++++++-
> >  kernel/irq/irqdomain.c                        |  2 +
> >  7 files changed, 98 insertions(+), 8 deletions(-)
> >
>
> Even with this patch set applied, sama5d2_xplained can not boot.
> Patch at [1] makes sama5d2_xplained boot again. Stephen applied it
> to clk-next.

I'm glad you won't actually have any boot issues in 5.12, but the fact
you need [1] with this series doesn't make a lot of sense to me
because:

1. The FWNODE_FLAG_INITIALIZED flag will be set for the clock fwnode
in question way before any consumer devices are added.
2. Any consumer device added after (1) will stop trying to link to the
clock device.

Are you somehow adding a consumer to the clock fwnode before (1)?

Can you try this patch without your clk fix? I was trying to avoid
looping through a list, but looks like your case might somehow need
it?

-Saravana

+++ b/drivers/base/core.c
@@ -943,6 +943,31 @@ static void device_links_missing_supplier(struct
device *dev)
        }
 }

+static int fw_devlink_check_suppliers(struct device *dev)
+{
+       struct fwnode_link *link;
+       int ret = 0;
+
+       if (!dev->fwnode ||fw_devlink_is_permissive())
+               return 0;
+
+       /*
+        * Device waiting for supplier to become available is not allowed to
+        * probe.
+        */
+       mutex_lock(&fwnode_link_lock);
+       list_for_each_entry(link, &dev->fwnode->suppliers, c_hook) {
+               if (link->supplier->flags & FWNODE_FLAG_INITIALIZED)
+                       continue;
+
+               ret = -EPROBE_DEFER;
+               break;
+       }
+       mutex_unlock(&fwnode_link_lock);
+
+       return ret;
+}
+
 /**
  * device_links_check_suppliers - Check presence of supplier drivers.
  * @dev: Consumer device.
@@ -964,21 +989,13 @@ int device_links_check_suppliers(struct device *dev)
        struct device_link *link;
        int ret = 0;

-       /*
-        * Device waiting for supplier to become available is not allowed to
-        * probe.
-        */
-       mutex_lock(&fwnode_link_lock);
-       if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
-           !fw_devlink_is_permissive()) {
+       if (fw_devlink_check_suppliers(dev)) {
                dev_dbg(dev, "probe deferral - wait for supplier %pfwP\n",
                        list_first_entry(&dev->fwnode->suppliers,
                        struct fwnode_link,
                        c_hook)->supplier);
-               mutex_unlock(&fwnode_link_lock);
                return -EPROBE_DEFER;
        }
-       mutex_unlock(&fwnode_link_lock);

        device_links_write_lock();



>
> Cheers,
> ta
>
> [1] https://lore.kernel.org/lkml/20210203154332.470587-1-tudor.ambarus@microchip.com/