| Message ID | 1485127546-18859-2-git-send-email-gwshan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > This removes the unnecessary nested if statements in function > rtas_initialize(), to simplify the code. No functional changes > introduced. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > arch/powerpc/kernel/rtas.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index 112cc3b..9ba0f67 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -1145,31 +1145,30 @@ asmlinkage int ppc_rtas(struct rtas_args __user *uargs) > void __init rtas_initialize(void) > { > unsigned long rtas_region = RTAS_INSTANTIATE_MAX; > + const __be32 *basep, *entryp, *sizep; > > /* Get RTAS dev node and fill up our "rtas" structure with infos > * about it. > */ > rtas.dev = of_find_node_by_name(NULL, "rtas"); > - if (rtas.dev) { > - const __be32 *basep, *entryp, *sizep; > - > - basep = of_get_property(rtas.dev, "linux,rtas-base", NULL); > - sizep = of_get_property(rtas.dev, "rtas-size", NULL); > - if (basep != NULL && sizep != NULL) { ... > - } else Previously we set rtas.dev to NULL if either basep or sizep was NULL. > - rtas.dev = NULL; > - } > if (!rtas.dev) > return; > > + basep = of_get_property(rtas.dev, "linux,rtas-base", NULL); > + sizep = of_get_property(rtas.dev, "rtas-size", NULL); > + if (basep == NULL && sizep == NULL) { But now you set it to NULL only if BOTH basep and sizep are NULL. Was that intentional? If so you need to mention it in the change log. > + rtas.dev = NULL; > + return; > + } The proper negation of: if (basep != NULL && sizep != NULL) { is: if (basep == NULL || sizep == NULL) { cheers
On Mon, Jan 23, 2017 at 08:08:20PM +1100, Michael Ellerman wrote: >Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > >> This removes the unnecessary nested if statements in function >> rtas_initialize(), to simplify the code. No functional changes >> introduced. >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> arch/powerpc/kernel/rtas.c | 33 ++++++++++++++++----------------- >> 1 file changed, 16 insertions(+), 17 deletions(-) >> >> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c >> index 112cc3b..9ba0f67 100644 >> --- a/arch/powerpc/kernel/rtas.c >> +++ b/arch/powerpc/kernel/rtas.c >> @@ -1145,31 +1145,30 @@ asmlinkage int ppc_rtas(struct rtas_args __user *uargs) >> void __init rtas_initialize(void) >> { >> unsigned long rtas_region = RTAS_INSTANTIATE_MAX; >> + const __be32 *basep, *entryp, *sizep; >> >> /* Get RTAS dev node and fill up our "rtas" structure with infos >> * about it. >> */ >> rtas.dev = of_find_node_by_name(NULL, "rtas"); >> - if (rtas.dev) { >> - const __be32 *basep, *entryp, *sizep; >> - >> - basep = of_get_property(rtas.dev, "linux,rtas-base", NULL); >> - sizep = of_get_property(rtas.dev, "rtas-size", NULL); >> - if (basep != NULL && sizep != NULL) { > ... >> - } else > >Previously we set rtas.dev to NULL if either basep or sizep was NULL. > >> - rtas.dev = NULL; >> - } >> if (!rtas.dev) >> return; >> >> + basep = of_get_property(rtas.dev, "linux,rtas-base", NULL); >> + sizep = of_get_property(rtas.dev, "rtas-size", NULL); >> + if (basep == NULL && sizep == NULL) { > >But now you set it to NULL only if BOTH basep and sizep are NULL. > >Was that intentional? If so you need to mention it in the change log. > >> + rtas.dev = NULL; >> + return; >> + } > >The proper negation of: > > if (basep != NULL && sizep != NULL) { >is: > if (basep == NULL || sizep == NULL) { > Thanks, Michael. It's not intentional. I'll update in v2. Thanks, Gavin
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index 112cc3b..9ba0f67 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -1145,31 +1145,30 @@ asmlinkage int ppc_rtas(struct rtas_args __user *uargs) void __init rtas_initialize(void) { unsigned long rtas_region = RTAS_INSTANTIATE_MAX; + const __be32 *basep, *entryp, *sizep; /* Get RTAS dev node and fill up our "rtas" structure with infos * about it. */ rtas.dev = of_find_node_by_name(NULL, "rtas"); - if (rtas.dev) { - const __be32 *basep, *entryp, *sizep; - - basep = of_get_property(rtas.dev, "linux,rtas-base", NULL); - sizep = of_get_property(rtas.dev, "rtas-size", NULL); - if (basep != NULL && sizep != NULL) { - rtas.base = __be32_to_cpu(*basep); - rtas.size = __be32_to_cpu(*sizep); - entryp = of_get_property(rtas.dev, - "linux,rtas-entry", NULL); - if (entryp == NULL) /* Ugh */ - rtas.entry = rtas.base; - else - rtas.entry = __be32_to_cpu(*entryp); - } else - rtas.dev = NULL; - } if (!rtas.dev) return; + basep = of_get_property(rtas.dev, "linux,rtas-base", NULL); + sizep = of_get_property(rtas.dev, "rtas-size", NULL); + if (basep == NULL && sizep == NULL) { + rtas.dev = NULL; + return; + } + + rtas.base = __be32_to_cpu(*basep); + rtas.size = __be32_to_cpu(*sizep); + entryp = of_get_property(rtas.dev, "linux,rtas-entry", NULL); + if (entryp == NULL) /* Ugh */ + rtas.entry = rtas.base; + else + rtas.entry = __be32_to_cpu(*entryp); + /* If RTAS was found, allocate the RMO buffer for it and look for * the stop-self token if any */
This removes the unnecessary nested if statements in function rtas_initialize(), to simplify the code. No functional changes introduced. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- arch/powerpc/kernel/rtas.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)