Thread (54 messages) 54 messages, 10 authors, 2021-04-21

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

From: <hidden>
Date: 2021-02-10 10:07:19
Also in: linux-acpi, linux-clk, linux-doc, linux-pm, lkml
Subsystem: common clk framework, the rest · Maintainers: Michael Turquette, Stephen Boyd, Linus Torvalds

On 2/10/21 10:54 AM, Saravana Kannan wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Wed, Feb 10, 2021 at 12:19 AM [off-list ref] wrote:
quoted
Hi, Saravana,

On 2/6/21 12:26 AM, Saravana Kannan wrote:
quoted
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.
Looks like in my case FWNODE_FLAG_INITIALIZED is not set, because
drivers/clk/at91/sama5d2.c uses of_clk_add_hw_provider().
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?
I tried it, didn't solve my boot problem. The following patch makes the
sama5d2_xplained boot again, even without the patch from [1]:
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 27ff90eacb1f..9370e4dfecae 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4594,6 +4594,8 @@ int of_clk_add_hw_provider(struct device_node *np,
        if (ret < 0)
                of_clk_del_provider(np);
 
+       fwnode_dev_initialized(&np->fwnode, true);
+
        return ret;
 }
 EXPORT_SYMBOL_GPL(of_clk_add_hw_provider);
Cheers,
ta
quoted hunk ↗ jump to hunk
-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();


quoted
Cheers,
ta

[1] https://lore.kernel.org/lkml/20210203154332.470587-1-tudor.ambarus@microchip.com/ (local)
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help