[v1,5/5] of: property: Skip adding device links to suppliers that aren't devices
diff mbox series

Message ID 20191028220027.251605-6-saravanak@google.com
State Accepted
Commit 15956dad5c1016155c82d094f8c1273a30f79c3d
Headers show
Series
  • Improve of_devlink to handle "proxy cycles"
Related show

Commit Message

Saravana Kannan Oct. 28, 2019, 10 p.m. UTC
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(-)

Comments

Rob Herring Nov. 4, 2019, 3:18 p.m. UTC | #1
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
>
Saravana Kannan Nov. 4, 2019, 7:01 p.m. UTC | #2
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
Rob Herring Nov. 4, 2019, 7:14 p.m. UTC | #3
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

Patch
diff mbox series

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);