This patch series adds a "post-init-providers" device tree binding that
can be used to break dependency cycles in device tree and enforce a more
determinstic probe/suspend/resume order. This will also improve the
stability of global async probing and async suspend/resume and allow us
to enable them more easily. Yet another step away from playing initcall
chicken with probing and step towards fully async probing and
suspend/resume.
Patch 3 (the binding documentation) provides a lot more details and
examples.
v2->v3:
- Changes doc/code from "post-init-supplier" to "post-init-providers"
- Fixed some wording that was ambiguous for Conor.
- Fixed indentation, additionalProperties and white space issues in the
yaml syntax.
- Fixed syntax errors in the example.
v1->v2:
- Addressed Documentation/commit text errors pointed out by Rob
- Reordered MAINTAINERS chunk as pointed out by Krzysztof
Saravana Kannan (4):
driver core: Adds flags param to fwnode_link_add()
driver core: Add FWLINK_FLAG_IGNORE to completely ignore a fwnode link
dt-bindings: Add post-init-providers property
of: property: fw_devlink: Add support for "post-init-providers"
property
.../bindings/post-init-providers.yaml | 105 ++++++++++++++++++
MAINTAINERS | 13 ++-
drivers/base/core.c | 14 ++-
drivers/firmware/efi/sysfb_efi.c | 2 +-
drivers/of/property.c | 17 ++-
include/linux/fwnode.h | 5 +-
6 files changed, 142 insertions(+), 14 deletions(-)
create mode 100644 Documentation/devicetree/bindings/post-init-providers.yaml
--
2.44.0.rc0.258.g7320e95886-goog
Allow the callers to set fwnode link flags when adding fwnode links.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/core.c | 5 +++--
drivers/firmware/efi/sysfb_efi.c | 2 +-
drivers/of/property.c | 2 +-
include/linux/fwnode.h | 3 ++-
4 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 9828da9b933c..adeff041d472 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -92,12 +92,13 @@ static int __fwnode_link_add(struct fwnode_handle *con,
return 0;
}
-int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
+int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
+ u8 flags)
{
int ret;
mutex_lock(&fwnode_link_lock);
- ret = __fwnode_link_add(con, sup, 0);
+ ret = __fwnode_link_add(con, sup, flags);
mutex_unlock(&fwnode_link_lock);
return ret;
}
diff --git a/drivers/firmware/efi/sysfb_efi.c b/drivers/firmware/efi/sysfb_efi.c
index 456d0e5eaf78..cc807ed35aed 100644
--- a/drivers/firmware/efi/sysfb_efi.c
+++ b/drivers/firmware/efi/sysfb_efi.c
@@ -336,7 +336,7 @@ static int efifb_add_links(struct fwnode_handle *fwnode)
if (!sup_np)
return 0;
- fwnode_link_add(fwnode, of_fwnode_handle(sup_np));
+ fwnode_link_add(fwnode, of_fwnode_handle(sup_np), 0);
of_node_put(sup_np);
return 0;
diff --git a/drivers/of/property.c b/drivers/of/property.c
index b71267c6667c..bce849f21ae2 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1085,7 +1085,7 @@ static void of_link_to_phandle(struct device_node *con_np,
tmp_np = of_get_next_parent(tmp_np);
}
- fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
+ fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np), 0);
}
/**
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 2a72f55d26eb..c964749953e3 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -210,7 +210,8 @@ static inline void fwnode_dev_initialized(struct fwnode_handle *fwnode,
}
extern bool fw_devlink_is_strict(void);
-int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup);
+int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
+ u8 flags);
void fwnode_links_purge(struct fwnode_handle *fwnode);
void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode);
--
2.44.0.rc0.258.g7320e95886-goog
A fwnode link between specific supplier-consumer fwnodes can be added
multiple times for multiple reasons. If that dependency doesn't exist,
deleting the fwnode link once doesn't guarantee that it won't get created
again.
So, add FWLINK_FLAG_IGNORE flag to mark a fwnode link as one that needs to
be completely ignored. Since a fwnode link's flags is an OR of all the
flags passed to all the fwnode_link_add() calls to create that specific
fwnode link, the FWLINK_FLAG_IGNORE flag is preserved and can be used to
mark a fwnode link as on that need to be completely ignored until it is
deleted.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/core.c | 9 ++++++++-
include/linux/fwnode.h | 2 ++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index adeff041d472..fac017657d25 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1012,7 +1012,8 @@ static struct fwnode_handle *fwnode_links_check_suppliers(
return NULL;
list_for_each_entry(link, &fwnode->suppliers, c_hook)
- if (!(link->flags & FWLINK_FLAG_CYCLE))
+ if (!(link->flags &
+ (FWLINK_FLAG_CYCLE | FWLINK_FLAG_IGNORE)))
return link->supplier;
return NULL;
@@ -1963,6 +1964,9 @@ static bool __fw_devlink_relax_cycles(struct device *con,
}
list_for_each_entry(link, &sup_handle->suppliers, c_hook) {
+ if (link->flags & FWLINK_FLAG_IGNORE)
+ continue;
+
if (__fw_devlink_relax_cycles(con, link->supplier)) {
__fwnode_link_cycle(link);
ret = true;
@@ -2041,6 +2045,9 @@ static int fw_devlink_create_devlink(struct device *con,
int ret = 0;
u32 flags;
+ if (link->flags & FWLINK_FLAG_IGNORE)
+ return 0;
+
if (con->fwnode == link->consumer)
flags = fw_devlink_get_flags(link->flags);
else
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index c964749953e3..21699eee9641 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -53,8 +53,10 @@ struct fwnode_handle {
* fwnode link flags
*
* CYCLE: The fwnode link is part of a cycle. Don't defer probe.
+ * IGNORE: Completely ignore this link, even during cycle detection.
*/
#define FWLINK_FLAG_CYCLE BIT(0)
+#define FWLINK_FLAG_IGNORE BIT(1)
struct fwnode_link {
struct fwnode_handle *supplier;
--
2.44.0.rc0.258.g7320e95886-goog
The post-init-providers property can be used to break a dependency cycle by
marking some provider(s) as a post device initialization provider(s). This
allows an OS to do a better job at ordering initialization and
suspend/resume of the devices in a dependency cycle.
Signed-off-by: Saravana Kannan <[email protected]>
---
.../bindings/post-init-providers.yaml | 105 ++++++++++++++++++
MAINTAINERS | 13 ++-
2 files changed, 112 insertions(+), 6 deletions(-)
create mode 100644 Documentation/devicetree/bindings/post-init-providers.yaml
diff --git a/Documentation/devicetree/bindings/post-init-providers.yaml b/Documentation/devicetree/bindings/post-init-providers.yaml
new file mode 100644
index 000000000000..92eb9a027443
--- /dev/null
+++ b/Documentation/devicetree/bindings/post-init-providers.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2020, Google LLC. All rights reserved.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/post-init-providers.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Post device initialization providers
+
+maintainers:
+ - Saravana Kannan <[email protected]>
+
+description: |
+ This property is used to indicate that the device(s) pointed to by the
+ property are not needed for the initialization of the device that lists this
+ property. This property does not make a device (that's previously not a
+ provider) into a provider. It simply downgrades an existing provider to a
+ post device initialization provider.
+
+ A device can list its providers in devicetree using one or more of the
+ standard devicetree bindings. By default, it is assumed that the provider
+ device can be initialized before the consumer device is initialized.
+
+ However, that assumption cannot be made when there are cyclic dependencies
+ between devices. Since each device is a provider (directly or indirectly) of
+ the others in the cycle, there is no guaranteed safe order for initializing
+ the devices in a cycle. We can try to initialize them in an arbitrary order
+ and eventually successfully initialize all of them, but that doesn't always
+ work well.
+
+ For example, say,
+ * The device tree has the following cyclic dependency X -> Y -> Z -> X (where
+ -> denotes "depends on").
+ * But X is not needed to fully initialize Z (X might be needed only when a
+ specific functionality is requested post initialization).
+
+ If all the other -> are mandatory initialization dependencies, then trying to
+ initialize the devices in a loop (or arbitrarily) will always eventually end
+ up with the devices being initialized in the order Z, Y and X.
+
+ However, if Y is an optional provider for X (where X provides limited
+ functionality when Y is not initialized and providing its services), then
+ trying to initialize the devices in a loop (or arbitrarily) could end up with
+ the devices being initialized in the following order:
+
+ * Z, Y and X - All devices provide full functionality
+ * Z, X and Y - X provides partial functionality
+ * X, Z and Y - X provides partial functionality
+
+ However, we always want to initialize the devices in the order Z, Y and X
+ since that provides the full functionality without interruptions.
+
+ One alternate option that might be suggested is to have the driver for X
+ notice that Y became available at a later point and adjust the functionality
+ it provides. However, other userspace applications could have started using X
+ with the limited functionality before Y was available and it might not be
+ possible to transparently transition X or the users of X to full
+ functionality while X is in use.
+
+ Similarly, when it comes to suspend (resume) ordering, it's unclear which
+ device in a dependency cycle needs to be suspended/resumed first and trying
+ arbitrary orders can result in system crashes or instability.
+
+ Explicitly calling out which link in a cycle needs to be broken when
+ determining the order, simplifies things a lot, improves efficiency, makes
+ the behavior more deterministic and maximizes the functionality that can be
+ provided without interruption.
+
+ This property is used to provide this additional information between devices
+ in a cycle by telling which provider(s) is not needed for initializing the
+ device that lists this property.
+
+ In the example above, Z would list X as a post-init-providers and the
+ initialization dependency would become X -> Y -> Z -/-> X. So the best order
+ to initialize them becomes clear: Z, Y and then X.
+
+select: true
+
+properties:
+ post-init-providers:
+ # One or more providers can be marked as post initialization provider
+ description:
+ List of phandles to providers that are not needed for initializing or
+ resuming this device.
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+ items:
+ maxItems: 1
+
+additionalProperties: true
+
+examples:
+ - |
+ gcc: clock-controller@1000 {
+ compatible = "vendor,soc4-gcc", "vendor,soc1-gcc";
+ reg = <0x1000 0x80>;
+ clocks = <&dispcc 0x1>;
+ #clock-cells = <1>;
+ post-init-providers = <&dispcc>;
+ };
+ dispcc: clock-controller@2000 {
+ compatible = "vendor,soc4-dispcc", "vendor,soc1-dispcc";
+ reg = <0x2000 0x80>;
+ clocks = <&gcc 0xdd>;
+ #clock-cells = <1>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 9ed4d3868539..5c97a0b5e9c1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6060,12 +6060,6 @@ S: Maintained
F: drivers/base/devcoredump.c
F: include/linux/devcoredump.h
-DEVICE DEPENDENCY HELPER SCRIPT
-M: Saravana Kannan <[email protected]>
-L: [email protected]
-S: Maintained
-F: scripts/dev-needs.sh
-
DEVICE DIRECT ACCESS (DAX)
M: Dan Williams <[email protected]>
M: Vishal Verma <[email protected]>
@@ -8300,6 +8294,13 @@ F: include/linux/firewire.h
F: include/uapi/linux/firewire*.h
F: tools/firewire/
+FIRMWARE DEVICE LINK (fw_devlink)
+M: Saravana Kannan <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/post-init-supplier.yaml
+F: scripts/dev-needs.sh
+
FIRMWARE FRAMEWORK FOR ARMV8-A
M: Sudeep Holla <[email protected]>
L: [email protected] (moderated for non-subscribers)
--
2.44.0.rc0.258.g7320e95886-goog
Add support for this property so that dependency cycles can be broken and
fw_devlink can do better probe/suspend/resume ordering between devices in a
dependency cycle.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/of/property.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index bce849f21ae2..15ccad6cba4a 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1066,7 +1066,8 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
}
static void of_link_to_phandle(struct device_node *con_np,
- struct device_node *sup_np)
+ struct device_node *sup_np,
+ u8 flags)
{
struct device_node *tmp_np = of_node_get(sup_np);
@@ -1085,7 +1086,8 @@ static void of_link_to_phandle(struct device_node *con_np,
tmp_np = of_get_next_parent(tmp_np);
}
- fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np), 0);
+ fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np),
+ flags);
}
/**
@@ -1198,6 +1200,8 @@ static struct device_node *parse_##fname(struct device_node *np, \
* to a struct device, implement this ops so fw_devlink can use it
* to find the true consumer.
* @optional: Describes whether a supplier is mandatory or not
+ * @fwlink_flags: Optional fwnode link flags to use when creating a fwnode link
+ * for this property.
*
* Returns:
* parse_prop() return values are
@@ -1210,6 +1214,7 @@ struct supplier_bindings {
const char *prop_name, int index);
struct device_node *(*get_con_dev)(struct device_node *np);
bool optional;
+ u8 fwlink_flags;
};
DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
@@ -1240,6 +1245,7 @@ DEFINE_SIMPLE_PROP(leds, "leds", NULL)
DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
DEFINE_SIMPLE_PROP(panel, "panel", NULL)
DEFINE_SIMPLE_PROP(msi_parent, "msi-parent", "#msi-cells")
+DEFINE_SIMPLE_PROP(post_init_providers, "post-init-providers", NULL)
DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
@@ -1349,6 +1355,10 @@ static const struct supplier_bindings of_supplier_bindings[] = {
{ .parse_prop = parse_regulators, },
{ .parse_prop = parse_gpio, },
{ .parse_prop = parse_gpios, },
+ {
+ .parse_prop = parse_post_init_providers,
+ .fwlink_flags = FWLINK_FLAG_IGNORE,
+ },
{}
};
@@ -1393,7 +1403,8 @@ static int of_link_property(struct device_node *con_np, const char *prop_name)
: of_node_get(con_np);
matched = true;
i++;
- of_link_to_phandle(con_dev_np, phandle);
+ of_link_to_phandle(con_dev_np, phandle,
+ s->fwlink_flags);
of_node_put(phandle);
of_node_put(con_dev_np);
}
--
2.44.0.rc0.258.g7320e95886-goog
On Wed, 21 Feb 2024 15:30:23 -0800, Saravana Kannan wrote:
> The post-init-providers property can be used to break a dependency cycle by
> marking some provider(s) as a post device initialization provider(s). This
> allows an OS to do a better job at ordering initialization and
> suspend/resume of the devices in a dependency cycle.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> ---
> .../bindings/post-init-providers.yaml | 105 ++++++++++++++++++
> MAINTAINERS | 13 ++-
> 2 files changed, 112 insertions(+), 6 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/post-init-providers.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@1000: failed to match any schema with compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@1000: failed to match any schema with compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@2000: failed to match any schema with compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@2000: failed to match any schema with compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
doc reference errors (make refcheckdocs):
Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/post-init-supplier.yaml
MAINTAINERS: Documentation/devicetree/bindings/post-init-supplier.yaml
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On Wed, Feb 21, 2024 at 4:23 PM Rob Herring <[email protected]> wrote:
>
>
> On Wed, 21 Feb 2024 15:30:23 -0800, Saravana Kannan wrote:
> > The post-init-providers property can be used to break a dependency cycle by
> > marking some provider(s) as a post device initialization provider(s). This
> > allows an OS to do a better job at ordering initialization and
> > suspend/resume of the devices in a dependency cycle.
> >
> > Signed-off-by: Saravana Kannan <[email protected]>
> > ---
> > .../bindings/post-init-providers.yaml | 105 ++++++++++++++++++
> > MAINTAINERS | 13 ++-
> > 2 files changed, 112 insertions(+), 6 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/post-init-providers.yaml
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@1000: failed to match any schema with compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
> Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@1000: failed to match any schema with compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
> Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@2000: failed to match any schema with compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
> Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@2000: failed to match any schema with compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
I'm assuming it's okay to ignore these warnings about made up
compatible string names.
> doc reference errors (make refcheckdocs):
> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/post-init-supplier.yaml
> MAINTAINERS: Documentation/devicetree/bindings/post-init-supplier.yaml
Will fix this and send out v4. Ignore the v3 series please.
-Saravana
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>
On 22/02/2024 04:41, Saravana Kannan wrote:
> On Wed, Feb 21, 2024 at 4:23 PM Rob Herring <[email protected]> wrote:
>>
>>
>> On Wed, 21 Feb 2024 15:30:23 -0800, Saravana Kannan wrote:
>>> The post-init-providers property can be used to break a dependency cycle by
>>> marking some provider(s) as a post device initialization provider(s). This
>>> allows an OS to do a better job at ordering initialization and
>>> suspend/resume of the devices in a dependency cycle.
>>>
>>> Signed-off-by: Saravana Kannan <[email protected]>
>>> ---
>>> .../bindings/post-init-providers.yaml | 105 ++++++++++++++++++
>>> MAINTAINERS | 13 ++-
>>> 2 files changed, 112 insertions(+), 6 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/post-init-providers.yaml
>>>
>>
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@1000: failed to match any schema with compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
>> Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@1000: failed to match any schema with compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
>> Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@2000: failed to match any schema with compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
>> Documentation/devicetree/bindings/post-init-providers.example.dtb: /example-0/clock-controller@2000: failed to match any schema with compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
>
> I'm assuming it's okay to ignore these warnings about made up
> compatible string names.
No, unfortunately not. I think you need to come with a real example or
just drop compatibles.
BTW, I still don't see any users of this binding.
Best regards,
Krzysztof
On Wed, Feb 21, 2024 at 03:30:24PM -0800, Saravana Kannan wrote:
> Add support for this property so that dependency cycles can be broken and
> fw_devlink can do better probe/suspend/resume ordering between devices in a
> dependency cycle.
..
> - fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np), 0);
> + fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np),
> + flags);
I would leave it one line despite being 83 characters long.
..
> - of_link_to_phandle(con_dev_np, phandle);
> + of_link_to_phandle(con_dev_np, phandle,
> + s->fwlink_flags);
I would leave this on one line, it's only 81 characters.
--
With Best Regards,
Andy Shevchenko
On Wed, Feb 21, 2024 at 03:30:20PM -0800, Saravana Kannan wrote:
> This patch series adds a "post-init-providers" device tree binding that
> can be used to break dependency cycles in device tree and enforce a more
> determinstic probe/suspend/resume order. This will also improve the
> stability of global async probing and async suspend/resume and allow us
> to enable them more easily. Yet another step away from playing initcall
> chicken with probing and step towards fully async probing and
> suspend/resume.
Do you know what is the state of affairs in ACPI? Is there any (similar)
issue even possible?
--
With Best Regards,
Andy Shevchenko
On Thu, Feb 22, 2024 at 5:34 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Feb 21, 2024 at 03:30:20PM -0800, Saravana Kannan wrote:
> > This patch series adds a "post-init-providers" device tree binding that
> > can be used to break dependency cycles in device tree and enforce a more
> > determinstic probe/suspend/resume order. This will also improve the
> > stability of global async probing and async suspend/resume and allow us
> > to enable them more easily. Yet another step away from playing initcall
> > chicken with probing and step towards fully async probing and
> > suspend/resume.
>
> Do you know what is the state of affairs in ACPI? Is there any (similar)
> issue even possible?
I'm not very familiar with ACPI, but I wouldn't be surprised if ACPI
devices have cyclic dependencies. But then ACPI on a PC doesn't
typically have as many devices/drivers and ACPI might be hiding the
dependencies from the kernel. So maybe the possibility of a cycle
visible to the kernel might be low.
I would really like to see fw_devlink extended to ACPI (it's written
in a way to make that possible), but don't have enough knowledge to do
it.
-Saravana
On Thu, Feb 22, 2024 at 5:32 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Feb 21, 2024 at 03:30:24PM -0800, Saravana Kannan wrote:
> > Add support for this property so that dependency cycles can be broken and
> > fw_devlink can do better probe/suspend/resume ordering between devices in a
> > dependency cycle.
>
> ...
>
> > - fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np), 0);
> > + fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np),
> > + flags);
>
> I would leave it one line despite being 83 characters long.
>
> ...
>
> > - of_link_to_phandle(con_dev_np, phandle);
> > + of_link_to_phandle(con_dev_np, phandle,
> > + s->fwlink_flags);
>
> I would leave this on one line, it's only 81 characters.
I don't have a strong opinion either way. If I need to send another
revision out, I'll address this (if checkpatch doesn't complain).
-Saravana
On Thu, Feb 22, 2024 at 5:04 PM Saravana Kannan <[email protected]> wrote:
>
> On Thu, Feb 22, 2024 at 5:32 AM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Wed, Feb 21, 2024 at 03:30:24PM -0800, Saravana Kannan wrote:
> > > Add support for this property so that dependency cycles can be broken and
> > > fw_devlink can do better probe/suspend/resume ordering between devices in a
> > > dependency cycle.
> >
> > ...
> >
> > > - fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np), 0);
> > > + fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np),
> > > + flags);
> >
> > I would leave it one line despite being 83 characters long.
> >
> > ...
> >
> > > - of_link_to_phandle(con_dev_np, phandle);
> > > + of_link_to_phandle(con_dev_np, phandle,
> > > + s->fwlink_flags);
> >
> > I would leave this on one line, it's only 81 characters.
>
> I don't have a strong opinion either way. If I need to send another
> revision out, I'll address this (if checkpatch doesn't complain).
My terminal is >80 chars, so 1 line is good.
Rob
On Fri, Feb 23, 2024 at 1:03 AM Saravana Kannan <[email protected]> wrote:
>
> On Thu, Feb 22, 2024 at 5:34 AM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Wed, Feb 21, 2024 at 03:30:20PM -0800, Saravana Kannan wrote:
> > > This patch series adds a "post-init-providers" device tree binding that
> > > can be used to break dependency cycles in device tree and enforce a more
> > > determinstic probe/suspend/resume order. This will also improve the
> > > stability of global async probing and async suspend/resume and allow us
> > > to enable them more easily. Yet another step away from playing initcall
> > > chicken with probing and step towards fully async probing and
> > > suspend/resume.
> >
> > Do you know what is the state of affairs in ACPI? Is there any (similar)
> > issue even possible?
>
> I'm not very familiar with ACPI, but I wouldn't be surprised if ACPI
> devices have cyclic dependencies. But then ACPI on a PC doesn't
> typically have as many devices/drivers and ACPI might be hiding the
> dependencies from the kernel. So maybe the possibility of a cycle
> visible to the kernel might be low.
>
> I would really like to see fw_devlink extended to ACPI (it's written
> in a way to make that possible), but don't have enough knowledge to do
> it.
This might happen one day, for example in the _DEP handling context
(for now it is very limited, but I'm not actually sure how much more
capable it needs to be).
I don't think that ACPI will ever need device links between parents
and children, though.
On a related note, RISC-V people seem to want to use it on ACPI
systems for interrupt controller dependency tracking.