The changeset code checks for a property in the deadprops list when
adding/updating a property, but of_add_property() and
of_update_property() do not. As the users of these functions are pretty
simple, they have not hit this scenario or else the property lists
would get corrupted.

With this there are 3 cases of removing a property from either deadprops
or properties lists, so add a helper to find and remove a matching
property.

Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - Make the helper remove a property from either deadprops or properties
   lists
 - Keep existing style in deadprops loop
v2:
 - Add helper to remove property from deadprops list
---
 drivers/of/base.c    | 37 ++++++++++++++++++++++++-------------
 drivers/of/dynamic.c | 19 -------------------
 2 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index e235f3a57ea8..3f9ddd18ff4b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1530,6 +1530,20 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 }
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
+static struct property *__of_remove_property_from_list(struct property **list, struct property *prop)
+{
+	struct property **next;
+
+	for (next = list; *next; next = &(*next)->next) {
+		if (*next == prop) {
+			*next = prop->next;
+			prop->next = NULL;
+			return prop;
+		}
+	}
+	return NULL;
+}
+
 /**
  * __of_add_property - Add a property to a node without lock operations
  * @np:		Caller's Device Node
@@ -1539,6 +1553,8 @@ int __of_add_property(struct device_node *np, struct property *prop)
 {
 	struct property **next;
 
+	__of_remove_property_from_list(&np->deadprops, prop);
+
 	prop->next = NULL;
 	next = &np->properties;
 	while (*next) {
@@ -1583,21 +1599,14 @@ EXPORT_SYMBOL_GPL(of_add_property);
 
 int __of_remove_property(struct device_node *np, struct property *prop)
 {
-	struct property **next;
-
-	for (next = &np->properties; *next; next = &(*next)->next) {
-		if (*next == prop)
-			break;
+	if (__of_remove_property_from_list(&np->properties, prop)) {
+		/* Found the property, add it to deadprops list */
+		prop->next = np->deadprops;
+		np->deadprops = prop;
+		return 0;
 	}
-	if (*next == NULL)
-		return -ENODEV;
-
-	/* found the node */
-	*next = prop->next;
-	prop->next = np->deadprops;
-	np->deadprops = prop;
 
-	return 0;
+	return -ENODEV;
 }
 
 /**
@@ -1641,6 +1650,8 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 {
 	struct property **next, *oldprop;
 
+	__of_remove_property_from_list(&np->deadprops, newprop);
+
 	for (next = &np->properties; *next; next = &(*next)->next) {
 		if (of_prop_cmp((*next)->name, newprop->name) == 0)
 			break;
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 1876af5b01fc..d3ad970ad7b8 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -564,7 +564,6 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,
 
 static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 {
-	struct property **propp;
 	unsigned long flags;
 	int ret = 0;
 
@@ -579,15 +578,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		__of_detach_node(ce->np);
 		break;
 	case OF_RECONFIG_ADD_PROPERTY:
-		/* If the property is in deadprops then it must be removed */
-		for (propp = &ce->np->deadprops; *propp; propp = &(*propp)->next) {
-			if (*propp == ce->prop) {
-				*propp = ce->prop->next;
-				ce->prop->next = NULL;
-				break;
-			}
-		}
-
 		ret = __of_add_property(ce->np, ce->prop);
 		break;
 	case OF_RECONFIG_REMOVE_PROPERTY:
@@ -595,15 +585,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		break;
 
 	case OF_RECONFIG_UPDATE_PROPERTY:
-		/* If the property is in deadprops then it must be removed */
-		for (propp = &ce->np->deadprops; *propp; propp = &(*propp)->next) {
-			if (*propp == ce->prop) {
-				*propp = ce->prop->next;
-				ce->prop->next = NULL;
-				break;
-			}
-		}
-
 		ret = __of_update_property(ce->np, ce->prop, &ce->old_prop);
 		break;
 	default:

-- 
2.40.1

Hi Rob,

Thanks for your patch!


On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
> The changeset code checks for a property in the deadprops list when
> adding/updating a property, but of_add_property() and
> of_update_property() do not. As the users of these functions are pretty
> simple, they have not hit this scenario or else the property lists
> would get corrupted.
>
> With this there are 3 cases of removing a property from either deadprops
> or properties lists, so add a helper to find and remove a matching
> property.
>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Perhaps this needs a Fixes tag?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Mon, Aug 21, 2023 at 8:05 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> Thanks for your patch!
>
>
> On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
> > The changeset code checks for a property in the deadprops list when
> > adding/updating a property, but of_add_property() and
> > of_update_property() do not. As the users of these functions are pretty
> > simple, they have not hit this scenario or else the property lists
> > would get corrupted.
> >
> > With this there are 3 cases of removing a property from either deadprops
> > or properties lists, so add a helper to find and remove a matching
> > property.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

>
> Perhaps this needs a Fixes tag?

I didn't simply because in the decades that these functions existed,
no one cared. It would require a specific sequence of calls which we
could pretty much determine doesn't happen just looking at the callers
in the kernel.

Rob

On Fri, Aug 18, 2023 at 03:41:00PM -0500, Rob Herring wrote:
> The changeset code checks for a property in the deadprops list when
> adding/updating a property, but of_add_property() and
> of_update_property() do not. As the users of these functions are pretty
> simple, they have not hit this scenario or else the property lists
> would get corrupted.
> 
> With this there are 3 cases of removing a property from either deadprops
> or properties lists, so add a helper to find and remove a matching
> property.

...

> v3:
>  - Keep existing style in deadprops loop

Not sure where exactly in the code that one, but...

...

>  int __of_remove_property(struct device_node *np, struct property *prop)
>  {
> -	struct property **next;
> -
> -	for (next = &np->properties; *next; next = &(*next)->next) {
> -		if (*next == prop)
> -			break;
> +	if (__of_remove_property_from_list(&np->properties, prop)) {
> +		/* Found the property, add it to deadprops list */
> +		prop->next = np->deadprops;
> +		np->deadprops = prop;
> +		return 0;
>  	}
> -	if (*next == NULL)
> -		return -ENODEV;
> -
> -	/* found the node */
> -	*next = prop->next;
> -	prop->next = np->deadprops;
> -	np->deadprops = prop;
>  
> -	return 0;
> +	return -ENODEV;
>  }


...if it's this one, I don't see how it's better than

	if (!__of_remove_property_from_list(&np->properties, prop))
		return -ENODEV;

	/* Found the property, add it to deadprops list */
	prop->next = np->deadprops;
	np->deadprops = prop;
	return 0;

Note, with --patience in use it may produce even nice-looking diff.

-- 
With Best Regards,
Andy Shevchenko

On Mon, Aug 21, 2023 at 5:49 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 18, 2023 at 03:41:00PM -0500, Rob Herring wrote:
> > The changeset code checks for a property in the deadprops list when
> > adding/updating a property, but of_add_property() and
> > of_update_property() do not. As the users of these functions are pretty
> > simple, they have not hit this scenario or else the property lists
> > would get corrupted.
> >
> > With this there are 3 cases of removing a property from either deadprops
> > or properties lists, so add a helper to find and remove a matching
> > property.
>
> ...
>
> > v3:
> >  - Keep existing style in deadprops loop
>
> Not sure where exactly in the code that one, but...

That was your previous comment...

>
> ...
>
> >  int __of_remove_property(struct device_node *np, struct property *prop)
> >  {
> > -     struct property **next;
> > -
> > -     for (next = &np->properties; *next; next = &(*next)->next) {
> > -             if (*next == prop)
> > -                     break;
> > +     if (__of_remove_property_from_list(&np->properties, prop)) {
> > +             /* Found the property, add it to deadprops list */
> > +             prop->next = np->deadprops;
> > +             np->deadprops = prop;
> > +             return 0;
> >       }
> > -     if (*next == NULL)
> > -             return -ENODEV;
> > -
> > -     /* found the node */
> > -     *next = prop->next;
> > -     prop->next = np->deadprops;
> > -     np->deadprops = prop;
> >
> > -     return 0;
> > +     return -ENODEV;
> >  }
>
>
> ...if it's this one, I don't see how it's better than
>
>         if (!__of_remove_property_from_list(&np->properties, prop))
>                 return -ENODEV;

Because this way doesn't work well when we move the spinlock in here.
Maybe cleanup.h will help, but I'm not going to do that now. If we do,
then I'll do it for the whole subsystem/file.

Rob

On Mon, Aug 21, 2023 at 07:24:43AM -0500, Rob Herring wrote:
> On Mon, Aug 21, 2023 at 5:49 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Aug 18, 2023 at 03:41:00PM -0500, Rob Herring wrote:

...

> > > v3:
> > >  - Keep existing style in deadprops loop
> >
> > Not sure where exactly in the code that one, but...
> 
> That was your previous comment...

I admit that I haven't heard about cleanup.h that time.

...

> > >  int __of_remove_property(struct device_node *np, struct property *prop)
> > >  {
> > > -     struct property **next;
> > > -
> > > -     for (next = &np->properties; *next; next = &(*next)->next) {
> > > -             if (*next == prop)
> > > -                     break;
> > > +     if (__of_remove_property_from_list(&np->properties, prop)) {
> > > +             /* Found the property, add it to deadprops list */
> > > +             prop->next = np->deadprops;
> > > +             np->deadprops = prop;
> > > +             return 0;
> > >       }
> > > -     if (*next == NULL)
> > > -             return -ENODEV;
> > > -
> > > -     /* found the node */
> > > -     *next = prop->next;
> > > -     prop->next = np->deadprops;
> > > -     np->deadprops = prop;
> > >
> > > -     return 0;
> > > +     return -ENODEV;
> > >  }
> >
> >
> > ...if it's this one, I don't see how it's better than
> >
> >         if (!__of_remove_property_from_list(&np->properties, prop))
> >                 return -ENODEV;
> 
> Because this way doesn't work well when we move the spinlock in here.
> Maybe cleanup.h will help, but I'm not going to do that now. If we do,
> then I'll do it for the whole subsystem/file.

Fair enough.

Although we may also use goto approach in the next patch. Anyway,
I leave it to you for what you think is the best.

-- 
With Best Regards,
Andy Shevchenko

On Mon, Aug 21, 2023 at 01:49:20PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 18, 2023 at 03:41:00PM -0500, Rob Herring wrote:

...

> > v3:
> >  - Keep existing style in deadprops loop
> 
> Not sure where exactly in the code that one, but...

> ...if it's this one, I don't see how it's better than

Also check the comment for the next patch.

-- 
With Best Regards,
Andy Shevchenko