| Message ID | 20191028220027.251605-6-saravanak@google.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Improve of_devlink to handle "proxy cycles" | expand |
On Mon, Oct 28, 2019 at 5:00 PM Saravana Kannan <saravanak@google.com> wrote: > > Some devices need to be initialized really early and can't wait for > driver core or drivers to be functional. These devices are typically > initialized without creating a struct device for their device nodes. > > If a supplier ends up being one of these devices, skip trying to add > device links to them. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/of/property.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index f16f85597ccc..21c9d251318a 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1038,6 +1038,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np, > struct device *sup_dev; > int ret = 0; > struct device_node *tmp_np = sup_np; > + int is_populated; > > of_node_get(sup_np); > /* > @@ -1062,9 +1063,10 @@ 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) > - return -EAGAIN; > + return is_populated ? 0 : -EAGAIN; You're only using the flag in one spot and a comment would be good here, so I'd just do: if (of_node_check_flag(sup_np, OF_POPULATED)) return 0; /* Early device without a struct device */ > if (!device_link_add(dev, sup_dev, dl_flags)) > ret = -EAGAIN; > put_device(sup_dev); > -- > 2.24.0.rc0.303.g954a862665-goog >
On Mon, Nov 4, 2019 at 7:18 AM Rob Herring <robh+dt@kernel.org> wrote: > > On Mon, Oct 28, 2019 at 5:00 PM Saravana Kannan <saravanak@google.com> wrote: > > > > Some devices need to be initialized really early and can't wait for > > driver core or drivers to be functional. These devices are typically > > initialized without creating a struct device for their device nodes. > > > > If a supplier ends up being one of these devices, skip trying to add > > device links to them. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/of/property.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index f16f85597ccc..21c9d251318a 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1038,6 +1038,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np, > > struct device *sup_dev; > > int ret = 0; > > struct device_node *tmp_np = sup_np; > > + int is_populated; > > > > of_node_get(sup_np); > > /* > > @@ -1062,9 +1063,10 @@ 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) > > - return -EAGAIN; > > + return is_populated ? 0 : -EAGAIN; > > You're only using the flag in one spot and a comment would be good > here, so I'd just do: > > if (of_node_check_flag(sup_np, OF_POPULATED)) > return 0; /* Early device without a struct device */ Hi Rob, Thanks for the review. I'm using the flag to keep the error handling code simple/cleaner. I can't do the check like that after I do a put on the sup_np. Yeah, I was actually planning to add a dev_dbg() message when this happens and returning a -EINVAL (that'll be ignored by the caller) instead of -EAGAIN (that's NOT ignored by the caller). Looks like these changes go pulled into driver-core-next. So I'll send a delta patch to add the dbg message and also address you nit on the other patch. Thanks, Saravana
On Mon, Nov 4, 2019 at 1:02 PM Saravana Kannan <saravanak@google.com> wrote: > > On Mon, Nov 4, 2019 at 7:18 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Mon, Oct 28, 2019 at 5:00 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > Some devices need to be initialized really early and can't wait for > > > driver core or drivers to be functional. These devices are typically > > > initialized without creating a struct device for their device nodes. > > > > > > If a supplier ends up being one of these devices, skip trying to add > > > device links to them. > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > --- > > > drivers/of/property.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > index f16f85597ccc..21c9d251318a 100644 > > > --- a/drivers/of/property.c > > > +++ b/drivers/of/property.c > > > @@ -1038,6 +1038,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np, > > > struct device *sup_dev; > > > int ret = 0; > > > struct device_node *tmp_np = sup_np; > > > + int is_populated; > > > > > > of_node_get(sup_np); > > > /* > > > @@ -1062,9 +1063,10 @@ 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) > > > - return -EAGAIN; > > > + return is_populated ? 0 : -EAGAIN; > > > > You're only using the flag in one spot and a comment would be good > > here, so I'd just do: > > > > if (of_node_check_flag(sup_np, OF_POPULATED)) > > return 0; /* Early device without a struct device */ > > Hi Rob, > > Thanks for the review. > > I'm using the flag to keep the error handling code simple/cleaner. I > can't do the check like that after I do a put on the sup_np. Ah, right. Nevermind. > Yeah, I was actually planning to add a dev_dbg() message when this > happens and returning a -EINVAL (that'll be ignored by the caller) > instead of -EAGAIN (that's NOT ignored by the caller). > > Looks like these changes go pulled into driver-core-next. So I'll send > a delta patch to add the dbg message and also address you nit on the > other patch. I didn't notice Greg applied already. Don't worry about the nit then. Rob
diff --git a/drivers/of/property.c b/drivers/of/property.c index f16f85597ccc..21c9d251318a 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1038,6 +1038,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np, struct device *sup_dev; int ret = 0; struct device_node *tmp_np = sup_np; + int is_populated; of_node_get(sup_np); /* @@ -1062,9 +1063,10 @@ 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) - return -EAGAIN; + return is_populated ? 0 : -EAGAIN; if (!device_link_add(dev, sup_dev, dl_flags)) ret = -EAGAIN; put_device(sup_dev);
Some devices need to be initialized really early and can't wait for driver core or drivers to be functional. These devices are typically initialized without creating a struct device for their device nodes. If a supplier ends up being one of these devices, skip trying to add device links to them. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/of/property.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)