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(structdevice *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)