On Fri, Jun 5, 2020 at 5:36 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> Consider this example where -> means LHS device is a consumer of RHS
> device and indentation represents "child of" of the previous device.
>
> Device A -> Device C
>
> Device B -> Device A
> Device C
>
> Without this commit:
> 1. Device A is added.
> 2. Device A is added to waiting for supplier list (Device C)
> 3. Device B is added
> 4. Device B is linked as a consumer to Device A
> 5. Device A doesn't probe because it's waiting for Device C to be added.
> 6. Device B doesn't probe because Device A hasn't probed.
> 7. Device C will never be added because it's parent hasn't probed.
>
> So, Device A, B and C will be in a probe/add deadlock.
>
> This commit detects this scenario and stops trying to create a device
> link between Device A and Device C since doing so would create a cycle:
> Device A -> Devic C -(parent)-> Device B -> Device A.
>
> With this commit:
> 1. Device A is added.
> 3. Device B is added
> 4. Device B is linked as a consumer to Device A
> 5. Device A probes.
> 6. Device B probes because Device A has probed.
> 7. Device C is added and probed.
>
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> ---
> drivers/of/property.c | 44 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 1f2086f4e7ce..7eebe21274a4 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1014,6 +1014,20 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
> return false;
> }
>
> +static struct device *of_get_next_parent_dev(struct device_node *np)
> +{
> + struct device *dev = NULL;
> +
> + of_node_get(np);
> + do {
> + np = of_get_next_parent(np);
> + if (np)
> + dev = get_dev_from_fwnode(&np->fwnode);
> + } while (np && !dev);
> + of_node_put(np);
> + return dev;
> +}
> +
> /**
> * of_link_to_phandle - Add device link to supplier from supplier phandle
> * @dev: consumer device
> @@ -1035,10 +1049,9 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
> static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> u32 dl_flags)
> {
> - struct device *sup_dev;
> + struct device *sup_dev, *sup_par_dev;
> int ret = 0;
> struct device_node *tmp_np = sup_np;
> - int is_populated;
>
> of_node_get(sup_np);
> /*
> @@ -1075,16 +1088,35 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> return -EINVAL;
> }
> sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
> - is_populated = of_node_check_flag(sup_np, OF_POPULATED);
> - of_node_put(sup_np);
> - if (!sup_dev && is_populated) {
> + if (!sup_dev && of_node_check_flag(sup_np, OF_POPULATED)) {
> /* Early device without struct device. */
> dev_dbg(dev, "Not linking to %pOFP - No struct device\n",
> sup_np);
> + of_node_put(sup_np);
> return -ENODEV;
> } else if (!sup_dev) {
> - return -EAGAIN;
> + sup_par_dev = of_get_next_parent_dev(sup_np);
> + of_node_put(sup_np);
> +
> + /*
> + * DL_FLAG_SYNC_STATE_ONLY doesn't block probing, so cycle
> + * detection isn't necessary and shouldn't be done.
> + */
> + if (dl_flags & DL_FLAG_SYNC_STATE_ONLY)
> + return -EAGAIN;
I need to put_device(sup_par_dev) before I return here and at other places
below. I'll send a v2 later to fix this.
-Saravana
> +
> + /*
> + * If devices haven't been created for any of the ancestors, we
> + * can't check for cycles. So let's try again later.
> + */
> + if (!sup_par_dev)
> + return -EAGAIN;
> +
> + /* Cyclic dependency detected, don't try to link */
> + if (device_is_dependent(dev, sup_par_dev))
> + return -EINVAL;
> }
> + of_node_put(sup_np);
> if (!device_link_add(dev, sup_dev, dl_flags))
> ret = -EINVAL;
> put_device(sup_dev);
> --
> 2.27.0.278.ge193c7cf3a9-goog
>
