| Message ID | 20210915170940.617415-2-saravanak@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | fw_devlink bug fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 3 of 3 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3 this patch: 3 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 26 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3 this patch: 3 |
| netdev/header_inline | success | Link |
On Wed, Sep 15, 2021 at 7:09 PM Saravana Kannan <saravanak@google.com> wrote: > > When we have a dependency of the form: > > Device-A -> Device-C > Device-B > > Device-C -> Device-B > > Where, > * Indentation denotes "child of" parent in previous line. > * X -> Y denotes X is consumer of Y based on firmware (Eg: DT). > > We have cyclic dependency: device-A -> device-C -> device-B -> device-A > > fw_devlink current treats device-C -> device-B dependency as an invalid > dependency and doesn't enforce it but leaves the rest of the > dependencies as is. > > While the current behavior is necessary, it is not sufficient if the > false dependency in this example is actually device-A -> device-C. When > this is the case, device-C will correctly probe defer waiting for > device-B to be added, but device-A will be incorrectly probe deferred by > fw_devlink waiting on device-C to probe successfully. Due to this, none > of the devices in the cycle will end up probing. > > To fix this, we need to go relax all the dependencies in the cycle like > we already do in the other instances where fw_devlink detects cycles. > A real world example of this was reported[1] and analyzed[2]. > > [1] - https://lore.kernel.org/lkml/0a2c4106-7f48-2bb5-048e-8c001a7c3fda@samsung.com/ > [2] - https://lore.kernel.org/lkml/CAGETcx8peaew90SWiux=TyvuGgvTQOmO4BFALz7aj0Za5QdNFQ@mail.gmail.com/ > Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature") > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Saravana Kannan <saravanak@google.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/base/core.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index e65dd803a453..316df6027093 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -1772,14 +1772,21 @@ static int fw_devlink_create_devlink(struct device *con, > * be broken by applying logic. Check for these types of cycles and > * break them so that devices in the cycle probe properly. > * > - * If the supplier's parent is dependent on the consumer, then > - * the consumer-supplier dependency is a false dependency. So, > - * treat it as an invalid link. > + * If the supplier's parent is dependent on the consumer, then the > + * consumer and supplier have a cyclic dependency. Since fw_devlink > + * can't tell which of the inferred dependencies are incorrect, don't > + * enforce probe ordering between any of the devices in this cyclic > + * dependency. Do this by relaxing all the fw_devlink device links in > + * this cycle and by treating the fwnode link between the consumer and > + * the supplier as an invalid dependency. > */ > sup_dev = fwnode_get_next_parent_dev(sup_handle); > if (sup_dev && device_is_dependent(con, sup_dev)) { > - dev_dbg(con, "Not linking to %pfwP - False link\n", > - sup_handle); > + dev_info(con, "Fixing up cyclic dependency with %pfwP (%s)\n", > + sup_handle, dev_name(sup_dev)); Why not dev_dbg()? Other than this, the change makes sense to me. > + device_links_write_lock(); > + fw_devlink_relax_cycle(con, sup_dev); > + device_links_write_unlock(); > ret = -EINVAL; > } else { > /* > -- > 2.33.0.309.g3052b89438-goog >
diff --git a/drivers/base/core.c b/drivers/base/core.c index e65dd803a453..316df6027093 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1772,14 +1772,21 @@ static int fw_devlink_create_devlink(struct device *con, * be broken by applying logic. Check for these types of cycles and * break them so that devices in the cycle probe properly. * - * If the supplier's parent is dependent on the consumer, then - * the consumer-supplier dependency is a false dependency. So, - * treat it as an invalid link. + * If the supplier's parent is dependent on the consumer, then the + * consumer and supplier have a cyclic dependency. Since fw_devlink + * can't tell which of the inferred dependencies are incorrect, don't + * enforce probe ordering between any of the devices in this cyclic + * dependency. Do this by relaxing all the fw_devlink device links in + * this cycle and by treating the fwnode link between the consumer and + * the supplier as an invalid dependency. */ sup_dev = fwnode_get_next_parent_dev(sup_handle); if (sup_dev && device_is_dependent(con, sup_dev)) { - dev_dbg(con, "Not linking to %pfwP - False link\n", - sup_handle); + dev_info(con, "Fixing up cyclic dependency with %pfwP (%s)\n", + sup_handle, dev_name(sup_dev)); + device_links_write_lock(); + fw_devlink_relax_cycle(con, sup_dev); + device_links_write_unlock(); ret = -EINVAL; } else { /*
When we have a dependency of the form: Device-A -> Device-C Device-B Device-C -> Device-B Where, * Indentation denotes "child of" parent in previous line. * X -> Y denotes X is consumer of Y based on firmware (Eg: DT). We have cyclic dependency: device-A -> device-C -> device-B -> device-A fw_devlink current treats device-C -> device-B dependency as an invalid dependency and doesn't enforce it but leaves the rest of the dependencies as is. While the current behavior is necessary, it is not sufficient if the false dependency in this example is actually device-A -> device-C. When this is the case, device-C will correctly probe defer waiting for device-B to be added, but device-A will be incorrectly probe deferred by fw_devlink waiting on device-C to probe successfully. Due to this, none of the devices in the cycle will end up probing. To fix this, we need to go relax all the dependencies in the cycle like we already do in the other instances where fw_devlink detects cycles. A real world example of this was reported[1] and analyzed[2]. [1] - https://lore.kernel.org/lkml/0a2c4106-7f48-2bb5-048e-8c001a7c3fda@samsung.com/ [2] - https://lore.kernel.org/lkml/CAGETcx8peaew90SWiux=TyvuGgvTQOmO4BFALz7aj0Za5QdNFQ@mail.gmail.com/ Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature") Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Signed-off-by: Saravana Kannan <saravanak@google.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/base/core.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)