* [PATCH 1/2] drivers/of: Add check for null property in of_remove_property() @ 2016-04-28 5:34 Suraj Jitindar Singh [not found] ` <1461821695-19204-1-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Suraj Jitindar Singh @ 2016-04-28 5:34 UTC (permalink / raw) To: devicetree-u79uwXL29TY76Z2rM5mHXA, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, frowand.list-Re5JQEeQqe8AvxtiuMwx3w, grant.likely-QSEj5FYQhm4dnm+yROfE0A, benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r, paulus-eUNUBHrolfbYtjvyW6yDsg, mpe-Gsx/Oe8HsFggBc27wqDAHg, Suraj Jitindar Singh The validity of the property input argument to of_remove_property() is never checked within the function and thus it is possible to pass a null value. It happens that this will be picked up in __of_remove_property() as no matching property of the device node will be found and thus an error will be returned, however once again there is no explicit check for a null value. By the time this is detected 2 locks have already been acquired which is completely unnecessary if the property to remove is null. Add an explicit check in the function of_remove_property() for a null property value and return -ENODEV in this case, this is consistent with what the previous return value would have been when the null value was not detected and passed to __of_remove_property(). By moving an explicit check for the property paramenter into the of_remove_property() function, this will remove the need to perform this check in calling code before invocation of the of_remove_property() function. Signed-off-by: Suraj Jitindar Singh <sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- drivers/of/base.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/of/base.c b/drivers/of/base.c index b299de2..64018eb 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -1777,6 +1777,9 @@ int of_remove_property(struct device_node *np, struct property *prop) unsigned long flags; int rc; + if (!prop) + return -ENODEV; + mutex_lock(&of_mutex); raw_spin_lock_irqsave(&devtree_lock, flags); -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <1461821695-19204-1-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* [PATCH 2/2] powerpc: Update of_remove_property() call sites to remove null checking [not found] ` <1461821695-19204-1-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2016-04-28 5:34 ` Suraj Jitindar Singh [not found] ` <1461821695-19204-2-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-05-03 13:06 ` [PATCH 1/2] drivers/of: Add check for null property in of_remove_property() Rob Herring 2016-05-04 22:40 ` [1/2] " Michael Ellerman 2 siblings, 1 reply; 10+ messages in thread From: Suraj Jitindar Singh @ 2016-04-28 5:34 UTC (permalink / raw) To: devicetree-u79uwXL29TY76Z2rM5mHXA, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, frowand.list-Re5JQEeQqe8AvxtiuMwx3w, grant.likely-QSEj5FYQhm4dnm+yROfE0A, benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r, paulus-eUNUBHrolfbYtjvyW6yDsg, mpe-Gsx/Oe8HsFggBc27wqDAHg, Suraj Jitindar Singh After obtaining a property from of_find_property() and before calling of_remove_property() most code checks to ensure that the property returned from of_find_property() is not null. The previous patch moved this check to the start of the function of_remove_property() in order to avoid the case where this check isn't done and a null value is passed. This ensures the check is always conducted before taking locks and attempting to remove the property. Thus it is no longer necessary to perform a check for null values before invoking of_remove_property(). Update of_remove_property() call sites in order to remove redundant checking for null property value as check is now performed within the of_remove_property function(). Signed-off-by: Suraj Jitindar Singh <sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- arch/powerpc/kernel/machine_kexec.c | 19 ++++++------------- arch/powerpc/kernel/machine_kexec_64.c | 11 ++++------- arch/powerpc/platforms/pseries/mobility.c | 4 ++-- arch/powerpc/platforms/pseries/reconfig.c | 5 +---- 4 files changed, 13 insertions(+), 26 deletions(-) diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c index 015ae55..55744a8 100644 --- a/arch/powerpc/kernel/machine_kexec.c +++ b/arch/powerpc/kernel/machine_kexec.c @@ -228,17 +228,12 @@ static struct property memory_limit_prop = { static void __init export_crashk_values(struct device_node *node) { - struct property *prop; - /* There might be existing crash kernel properties, but we can't * be sure what's in them, so remove them. */ - prop = of_find_property(node, "linux,crashkernel-base", NULL); - if (prop) - of_remove_property(node, prop); - - prop = of_find_property(node, "linux,crashkernel-size", NULL); - if (prop) - of_remove_property(node, prop); + of_remove_property(node, of_find_property(node, + "linux,crashkernel-base", NULL)); + of_remove_property(node, of_find_property(node, + "linux,crashkernel-size", NULL)); if (crashk_res.start != 0) { crashk_base = cpu_to_be_ulong(crashk_res.start), @@ -258,16 +253,14 @@ static void __init export_crashk_values(struct device_node *node) static int __init kexec_setup(void) { struct device_node *node; - struct property *prop; node = of_find_node_by_path("/chosen"); if (!node) return -ENOENT; /* remove any stale properties so ours can be found */ - prop = of_find_property(node, kernel_end_prop.name, NULL); - if (prop) - of_remove_property(node, prop); + of_remove_property(node, of_find_property(node, kernel_end_prop.name, + NULL)); /* information needed by userspace when using default_machine_kexec */ kernel_end = cpu_to_be_ulong(__pa(_end)); diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c index 0fbd75d..2608192 100644 --- a/arch/powerpc/kernel/machine_kexec_64.c +++ b/arch/powerpc/kernel/machine_kexec_64.c @@ -401,7 +401,6 @@ static struct property htab_size_prop = { static int __init export_htab_values(void) { struct device_node *node; - struct property *prop; /* On machines with no htab htab_address is NULL */ if (!htab_address) @@ -412,12 +411,10 @@ static int __init export_htab_values(void) return -ENODEV; /* remove any stale propertys so ours can be found */ - prop = of_find_property(node, htab_base_prop.name, NULL); - if (prop) - of_remove_property(node, prop); - prop = of_find_property(node, htab_size_prop.name, NULL); - if (prop) - of_remove_property(node, prop); + of_remove_property(node, of_find_property(node, htab_base_prop.name, + NULL)); + of_remove_property(node, of_find_property(node, htab_size_prop.name, + NULL)); htab_base = cpu_to_be64(__pa(htab_address)); of_add_property(node, &htab_base_prop); diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c index ceb18d3..a560a98 100644 --- a/arch/powerpc/platforms/pseries/mobility.c +++ b/arch/powerpc/platforms/pseries/mobility.c @@ -191,8 +191,8 @@ static int update_dt_node(__be32 phandle, s32 scope) break; case 0x80000000: - prop = of_find_property(dn, prop_name, NULL); - of_remove_property(dn, prop); + of_remove_property(dn, of_find_property(dn, + prop_name, NULL)); prop = NULL; break; diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c index 7c7fcc0..cc66c49 100644 --- a/arch/powerpc/platforms/pseries/reconfig.c +++ b/arch/powerpc/platforms/pseries/reconfig.c @@ -303,7 +303,6 @@ static int do_remove_property(char *buf, size_t bufsize) { struct device_node *np; char *tmp; - struct property *prop; buf = parse_node(buf, bufsize, &np); if (!np) @@ -316,9 +315,7 @@ static int do_remove_property(char *buf, size_t bufsize) if (strlen(buf) == 0) return -EINVAL; - prop = of_find_property(np, buf, NULL); - - return of_remove_property(np, prop); + return of_remove_property(np, of_find_property(np, buf, NULL)); } static int do_update_property(char *buf, size_t bufsize) -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <1461821695-19204-2-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 2/2] powerpc: Update of_remove_property() call sites to remove null checking [not found] ` <1461821695-19204-2-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2016-05-03 22:32 ` Tyrel Datwyler [not found] ` <57292702.5070507-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> 2016-05-04 22:40 ` [2/2] " Michael Ellerman 1 sibling, 1 reply; 10+ messages in thread From: Tyrel Datwyler @ 2016-05-03 22:32 UTC (permalink / raw) To: Suraj Jitindar Singh, devicetree-u79uwXL29TY76Z2rM5mHXA, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, paulus-eUNUBHrolfbYtjvyW6yDsg, grant.likely-QSEj5FYQhm4dnm+yROfE0A, frowand.list-Re5JQEeQqe8AvxtiuMwx3w On 04/27/2016 10:34 PM, Suraj Jitindar Singh wrote: > After obtaining a property from of_find_property() and before calling > of_remove_property() most code checks to ensure that the property > returned from of_find_property() is not null. The previous patch > moved this check to the start of the function of_remove_property() in > order to avoid the case where this check isn't done and a null value is > passed. This ensures the check is always conducted before taking locks > and attempting to remove the property. Thus it is no longer necessary > to perform a check for null values before invoking of_remove_property(). > > Update of_remove_property() call sites in order to remove redundant > checking for null property value as check is now performed within the > of_remove_property function(). > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > --- > arch/powerpc/kernel/machine_kexec.c | 19 ++++++------------- > arch/powerpc/kernel/machine_kexec_64.c | 11 ++++------- > arch/powerpc/platforms/pseries/mobility.c | 4 ++-- > arch/powerpc/platforms/pseries/reconfig.c | 5 +---- > 4 files changed, 13 insertions(+), 26 deletions(-) > > diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c > index 015ae55..55744a8 100644 > --- a/arch/powerpc/kernel/machine_kexec.c > +++ b/arch/powerpc/kernel/machine_kexec.c > @@ -228,17 +228,12 @@ static struct property memory_limit_prop = { > > static void __init export_crashk_values(struct device_node *node) > { > - struct property *prop; > - > /* There might be existing crash kernel properties, but we can't > * be sure what's in them, so remove them. */ > - prop = of_find_property(node, "linux,crashkernel-base", NULL); > - if (prop) > - of_remove_property(node, prop); > - > - prop = of_find_property(node, "linux,crashkernel-size", NULL); > - if (prop) > - of_remove_property(node, prop); > + of_remove_property(node, of_find_property(node, > + "linux,crashkernel-base", NULL)); > + of_remove_property(node, of_find_property(node, > + "linux,crashkernel-size", NULL)); > > if (crashk_res.start != 0) { > crashk_base = cpu_to_be_ulong(crashk_res.start), > @@ -258,16 +253,14 @@ static void __init export_crashk_values(struct device_node *node) > static int __init kexec_setup(void) > { > struct device_node *node; > - struct property *prop; > > node = of_find_node_by_path("/chosen"); > if (!node) > return -ENOENT; > > /* remove any stale properties so ours can be found */ > - prop = of_find_property(node, kernel_end_prop.name, NULL); > - if (prop) > - of_remove_property(node, prop); > + of_remove_property(node, of_find_property(node, kernel_end_prop.name, > + NULL)); > > /* information needed by userspace when using default_machine_kexec */ > kernel_end = cpu_to_be_ulong(__pa(_end)); > diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c > index 0fbd75d..2608192 100644 > --- a/arch/powerpc/kernel/machine_kexec_64.c > +++ b/arch/powerpc/kernel/machine_kexec_64.c > @@ -401,7 +401,6 @@ static struct property htab_size_prop = { > static int __init export_htab_values(void) > { > struct device_node *node; > - struct property *prop; > > /* On machines with no htab htab_address is NULL */ > if (!htab_address) > @@ -412,12 +411,10 @@ static int __init export_htab_values(void) > return -ENODEV; > > /* remove any stale propertys so ours can be found */ > - prop = of_find_property(node, htab_base_prop.name, NULL); > - if (prop) > - of_remove_property(node, prop); > - prop = of_find_property(node, htab_size_prop.name, NULL); > - if (prop) > - of_remove_property(node, prop); > + of_remove_property(node, of_find_property(node, htab_base_prop.name, > + NULL)); > + of_remove_property(node, of_find_property(node, htab_size_prop.name, > + NULL)); > > htab_base = cpu_to_be64(__pa(htab_address)); > of_add_property(node, &htab_base_prop); > diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c > index ceb18d3..a560a98 100644 > --- a/arch/powerpc/platforms/pseries/mobility.c > +++ b/arch/powerpc/platforms/pseries/mobility.c > @@ -191,8 +191,8 @@ static int update_dt_node(__be32 phandle, s32 scope) > break; > > case 0x80000000: > - prop = of_find_property(dn, prop_name, NULL); > - of_remove_property(dn, prop); > + of_remove_property(dn, of_find_property(dn, > + prop_name, NULL)); > prop = NULL; > break; > You haven't removed a NULL check here, as suggested by the changelog, but instead made a cosmetic change to the code that still leaves behind a now unnecessary "prop = NULL;" to bit rot. > diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c > index 7c7fcc0..cc66c49 100644 > --- a/arch/powerpc/platforms/pseries/reconfig.c > +++ b/arch/powerpc/platforms/pseries/reconfig.c > @@ -303,7 +303,6 @@ static int do_remove_property(char *buf, size_t bufsize) > { > struct device_node *np; > char *tmp; > - struct property *prop; > buf = parse_node(buf, bufsize, &np); > > if (!np) > @@ -316,9 +315,7 @@ static int do_remove_property(char *buf, size_t bufsize) > if (strlen(buf) == 0) > return -EINVAL; > > - prop = of_find_property(np, buf, NULL); > - > - return of_remove_property(np, prop); > + return of_remove_property(np, of_find_property(np, buf, NULL)); > } Again, you are not removing a NULL check as suggested by the changelog. -Tyrel > > static int do_update_property(char *buf, size_t bufsize) > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <57292702.5070507-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 2/2] powerpc: Update of_remove_property() call sites to remove null checking [not found] ` <57292702.5070507-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> @ 2016-05-05 6:50 ` Michael Ellerman 2016-05-06 1:01 ` Suraj Jitindar Singh [not found] ` <1462431031.12473.3.camel-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org> 0 siblings, 2 replies; 10+ messages in thread From: Michael Ellerman @ 2016-05-05 6:50 UTC (permalink / raw) To: Tyrel Datwyler, Suraj Jitindar Singh, devicetree-u79uwXL29TY76Z2rM5mHXA, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ Cc: grant.likely-QSEj5FYQhm4dnm+yROfE0A, frowand.list-Re5JQEeQqe8AvxtiuMwx3w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, paulus-eUNUBHrolfbYtjvyW6yDsg On Tue, 2016-05-03 at 15:32 -0700, Tyrel Datwyler wrote: > On 04/27/2016 10:34 PM, Suraj Jitindar Singh wrote: > > diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c > > index ceb18d3..a560a98 100644 > > --- a/arch/powerpc/platforms/pseries/mobility.c > > +++ b/arch/powerpc/platforms/pseries/mobility.c > > @@ -191,8 +191,8 @@ static int update_dt_node(__be32 phandle, s32 scope) > > break; > > > > case 0x80000000: > > - prop = of_find_property(dn, prop_name, NULL); > > - of_remove_property(dn, prop); > > + of_remove_property(dn, of_find_property(dn, > > + prop_name, NULL)); > > prop = NULL; > > break; > > > > You haven't removed a NULL check here, as suggested by the changelog, > but instead made a cosmetic change to the code that still leaves behind > a now unnecessary "prop = NULL;" to bit rot. Yeah I think you're right. Though it's not very clear how prop is used in that function. Please one of you send me an incremental to remove the prop = NULL; cheers -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] powerpc: Update of_remove_property() call sites to remove null checking 2016-05-05 6:50 ` Michael Ellerman @ 2016-05-06 1:01 ` Suraj Jitindar Singh [not found] ` <1462431031.12473.3.camel-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org> 1 sibling, 0 replies; 10+ messages in thread From: Suraj Jitindar Singh @ 2016-05-06 1:01 UTC (permalink / raw) To: Michael Ellerman, Tyrel Datwyler, devicetree, linuxppc-dev Cc: grant.likely, paulus, robh+dt, frowand.list [-- Attachment #1.1: Type: text/plain, Size: 1383 bytes --] On 05/05/16 16:50, Michael Ellerman wrote: > On Tue, 2016-05-03 at 15:32 -0700, Tyrel Datwyler wrote: >> On 04/27/2016 10:34 PM, Suraj Jitindar Singh wrote: >>> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c >>> index ceb18d3..a560a98 100644 >>> --- a/arch/powerpc/platforms/pseries/mobility.c >>> +++ b/arch/powerpc/platforms/pseries/mobility.c >>> @@ -191,8 +191,8 @@ static int update_dt_node(__be32 phandle, s32 scope) >>> break; >>> >>> case 0x80000000: >>> - prop = of_find_property(dn, prop_name, NULL); >>> - of_remove_property(dn, prop); >>> + of_remove_property(dn, of_find_property(dn, >>> + prop_name, NULL)); >>> prop = NULL; >>> break; >>> >> >> You haven't removed a NULL check here, as suggested by the changelog, >> but instead made a cosmetic change to the code that still leaves behind >> a now unnecessary "prop = NULL;" to bit rot. > > Yeah I think you're right. Though it's not very clear how prop is used in that > function. > > Please one of you send me an incremental to remove the prop = NULL; > > cheers > I didn't delete the prop = NULL; initially as I didn't fully understand how it was used in the rest of the function and the effect of deleting it. [-- Attachment #1.2: Type: text/html, Size: 1996 bytes --] [-- Attachment #2: Type: text/plain, Size: 150 bytes --] _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <1462431031.12473.3.camel-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org>]
* Re: [PATCH 2/2] powerpc: Update of_remove_property() call sites to remove null checking [not found] ` <1462431031.12473.3.camel-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org> @ 2016-05-06 3:00 ` Suraj Jitindar Singh [not found] ` <572C08DB.9000109-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Suraj Jitindar Singh @ 2016-05-06 3:00 UTC (permalink / raw) To: Michael Ellerman, Tyrel Datwyler, devicetree-u79uwXL29TY76Z2rM5mHXA, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ Cc: grant.likely-QSEj5FYQhm4dnm+yROfE0A, frowand.list-Re5JQEeQqe8AvxtiuMwx3w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, paulus-eUNUBHrolfbYtjvyW6yDsg On 05/05/16 16:50, Michael Ellerman wrote: > On Tue, 2016-05-03 at 15:32 -0700, Tyrel Datwyler wrote: >> On 04/27/2016 10:34 PM, Suraj Jitindar Singh wrote: >>> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c >>> index ceb18d3..a560a98 100644 >>> --- a/arch/powerpc/platforms/pseries/mobility.c >>> +++ b/arch/powerpc/platforms/pseries/mobility.c >>> @@ -191,8 +191,8 @@ static int update_dt_node(__be32 phandle, s32 scope) >>> break; >>> >>> case 0x80000000: >>> - prop = of_find_property(dn, prop_name, NULL); >>> - of_remove_property(dn, prop); >>> + of_remove_property(dn, of_find_property(dn, >>> + prop_name, NULL)); >>> prop = NULL; >>> break; >>> >> You haven't removed a NULL check here, as suggested by the changelog, >> but instead made a cosmetic change to the code that still leaves behind >> a now unnecessary "prop = NULL;" to bit rot. > Yeah I think you're right. Though it's not very clear how prop is used in that > function. > > Please one of you send me an incremental to remove the prop = NULL; > > cheers > Resend of previous message due to formatting issues: I didn't delete the prop = NULL; initially as I didn't fully understand how it was used in the rest of the function and the effect of deleting it. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <572C08DB.9000109-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 2/2] powerpc: Update of_remove_property() call sites to remove null checking [not found] ` <572C08DB.9000109-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2016-05-09 9:41 ` Michael Ellerman 0 siblings, 0 replies; 10+ messages in thread From: Michael Ellerman @ 2016-05-09 9:41 UTC (permalink / raw) To: Suraj Jitindar Singh, Tyrel Datwyler, devicetree-u79uwXL29TY76Z2rM5mHXA, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ Cc: grant.likely-QSEj5FYQhm4dnm+yROfE0A, frowand.list-Re5JQEeQqe8AvxtiuMwx3w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, paulus-eUNUBHrolfbYtjvyW6yDsg On Fri, 2016-05-06 at 13:00 +1000, Suraj Jitindar Singh wrote: > On 05/05/16 16:50, Michael Ellerman wrote: > > On Tue, 2016-05-03 at 15:32 -0700, Tyrel Datwyler wrote: > > > On 04/27/2016 10:34 PM, Suraj Jitindar Singh wrote: > > > > diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c > > > > index ceb18d3..a560a98 100644 > > > > --- a/arch/powerpc/platforms/pseries/mobility.c > > > > +++ b/arch/powerpc/platforms/pseries/mobility.c > > > > @@ -191,8 +191,8 @@ static int update_dt_node(__be32 phandle, s32 scope) > > > > break; > > > > > > > > case 0x80000000: > > > > - prop = of_find_property(dn, prop_name, NULL); > > > > - of_remove_property(dn, prop); > > > > + of_remove_property(dn, of_find_property(dn, > > > > + prop_name, NULL)); > > > > prop = NULL; > > > > break; > > > > > > > You haven't removed a NULL check here, as suggested by the changelog, > > > but instead made a cosmetic change to the code that still leaves behind > > > a now unnecessary "prop = NULL;" to bit rot. > > Yeah I think you're right. Though it's not very clear how prop is used in that > > function. > > I didn't delete the prop = NULL; initially as I didn't fully understand > how it was used in the rest of the function and the effect of deleting > it. Yeah, it's pretty convoluted. I don't think you can actually prove it's safe to remove the prop = NULL for arbitrary inputs. cheers -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [2/2] powerpc: Update of_remove_property() call sites to remove null checking [not found] ` <1461821695-19204-2-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-05-03 22:32 ` Tyrel Datwyler @ 2016-05-04 22:40 ` Michael Ellerman 1 sibling, 0 replies; 10+ messages in thread From: Michael Ellerman @ 2016-05-04 22:40 UTC (permalink / raw) To: devicetree-u79uwXL29TY76Z2rM5mHXA, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, paulus-eUNUBHrolfbYtjvyW6yDsg, Suraj Jitindar Singh, grant.likely-QSEj5FYQhm4dnm+yROfE0A, frowand.list-Re5JQEeQqe8AvxtiuMwx3w On Thu, 2016-28-04 at 05:34:55 UTC, Suraj Jitindar Singh wrote: > After obtaining a property from of_find_property() and before calling > of_remove_property() most code checks to ensure that the property > returned from of_find_property() is not null. The previous patch > moved this check to the start of the function of_remove_property() in > order to avoid the case where this check isn't done and a null value is > passed. This ensures the check is always conducted before taking locks > and attempting to remove the property. Thus it is no longer necessary > to perform a check for null values before invoking of_remove_property(). > > Update of_remove_property() call sites in order to remove redundant > checking for null property value as check is now performed within the > of_remove_property function(). > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/676669b666574a23f7bd62870c cheers -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drivers/of: Add check for null property in of_remove_property() [not found] ` <1461821695-19204-1-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-04-28 5:34 ` [PATCH 2/2] powerpc: Update of_remove_property() call sites to remove null checking Suraj Jitindar Singh @ 2016-05-03 13:06 ` Rob Herring 2016-05-04 22:40 ` [1/2] " Michael Ellerman 2 siblings, 0 replies; 10+ messages in thread From: Rob Herring @ 2016-05-03 13:06 UTC (permalink / raw) To: Suraj Jitindar Singh Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linuxppc-dev, Frank Rowand, Grant Likely, Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman On Thu, Apr 28, 2016 at 12:34 AM, Suraj Jitindar Singh <sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > The validity of the property input argument to of_remove_property() is > never checked within the function and thus it is possible to pass a null > value. It happens that this will be picked up in __of_remove_property() > as no matching property of the device node will be found and thus an > error will be returned, however once again there is no explicit check > for a null value. By the time this is detected 2 locks have already been > acquired which is completely unnecessary if the property to remove is > null. > > Add an explicit check in the function of_remove_property() for a null > property value and return -ENODEV in this case, this is consistent with > what the previous return value would have been when the null value was > not detected and passed to __of_remove_property(). > > By moving an explicit check for the property paramenter into the > of_remove_property() function, this will remove the need to perform this > check in calling code before invocation of the of_remove_property() > function. > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> For both patches: Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [1/2] drivers/of: Add check for null property in of_remove_property() [not found] ` <1461821695-19204-1-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-04-28 5:34 ` [PATCH 2/2] powerpc: Update of_remove_property() call sites to remove null checking Suraj Jitindar Singh 2016-05-03 13:06 ` [PATCH 1/2] drivers/of: Add check for null property in of_remove_property() Rob Herring @ 2016-05-04 22:40 ` Michael Ellerman 2 siblings, 0 replies; 10+ messages in thread From: Michael Ellerman @ 2016-05-04 22:40 UTC (permalink / raw) To: devicetree-u79uwXL29TY76Z2rM5mHXA, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, paulus-eUNUBHrolfbYtjvyW6yDsg, Suraj Jitindar Singh, grant.likely-QSEj5FYQhm4dnm+yROfE0A, frowand.list-Re5JQEeQqe8AvxtiuMwx3w On Thu, 2016-28-04 at 05:34:54 UTC, Suraj Jitindar Singh wrote: > The validity of the property input argument to of_remove_property() is > never checked within the function and thus it is possible to pass a null > value. It happens that this will be picked up in __of_remove_property() > as no matching property of the device node will be found and thus an > error will be returned, however once again there is no explicit check > for a null value. By the time this is detected 2 locks have already been > acquired which is completely unnecessary if the property to remove is > null. > > Add an explicit check in the function of_remove_property() for a null > property value and return -ENODEV in this case, this is consistent with > what the previous return value would have been when the null value was > not detected and passed to __of_remove_property(). > > By moving an explicit check for the property paramenter into the > of_remove_property() function, this will remove the need to perform this > check in calling code before invocation of the of_remove_property() > function. > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/1c173cb23486f540ea08a5050a cheers -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-05-09 9:41 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-28 5:34 [PATCH 1/2] drivers/of: Add check for null property in of_remove_property() Suraj Jitindar Singh [not found] ` <1461821695-19204-1-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-04-28 5:34 ` [PATCH 2/2] powerpc: Update of_remove_property() call sites to remove null checking Suraj Jitindar Singh [not found] ` <1461821695-19204-2-git-send-email-sjitindarsingh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-05-03 22:32 ` Tyrel Datwyler [not found] ` <57292702.5070507-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> 2016-05-05 6:50 ` Michael Ellerman 2016-05-06 1:01 ` Suraj Jitindar Singh [not found] ` <1462431031.12473.3.camel-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org> 2016-05-06 3:00 ` Suraj Jitindar Singh [not found] ` <572C08DB.9000109-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-05-09 9:41 ` Michael Ellerman 2016-05-04 22:40 ` [2/2] " Michael Ellerman 2016-05-03 13:06 ` [PATCH 1/2] drivers/of: Add check for null property in of_remove_property() Rob Herring 2016-05-04 22:40 ` [1/2] " Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).