* [PATCH 1/3] powerpc/kernel: Remove nested if statements in rtas_initialize()
2017-01-22 23:25 [PATCH 0/3] powerpc/kernel: Simplify rtas_initialize() Gavin Shan
@ 2017-01-22 23:25 ` Gavin Shan
2017-01-23 9:08 ` Michael Ellerman
2017-01-22 23:25 ` [PATCH 2/3] powerpc/kernel: Use of_property_read_u32() " Gavin Shan
2017-01-22 23:25 ` [PATCH 3/3] powerpc/kernel: Fix unbalanced refcount on RTAS device node Gavin Shan
2 siblings, 1 reply; 6+ messages in thread
From: Gavin Shan @ 2017-01-22 23:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mpe, Gavin Shan
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) {
- 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
*/
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] powerpc/kernel: Remove nested if statements in rtas_initialize()
2017-01-22 23:25 ` [PATCH 1/3] powerpc/kernel: Remove nested if statements in rtas_initialize() Gavin Shan
@ 2017-01-23 9:08 ` Michael Ellerman
2017-01-23 22:26 ` Gavin Shan
0 siblings, 1 reply; 6+ messages in thread
From: Michael Ellerman @ 2017-01-23 9:08 UTC (permalink / raw)
To: Gavin Shan, linuxppc-dev; +Cc: Gavin Shan
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] powerpc/kernel: Remove nested if statements in rtas_initialize()
2017-01-23 9:08 ` Michael Ellerman
@ 2017-01-23 22:26 ` Gavin Shan
0 siblings, 0 replies; 6+ messages in thread
From: Gavin Shan @ 2017-01-23 22:26 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Gavin Shan, linuxppc-dev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] powerpc/kernel: Use of_property_read_u32() in rtas_initialize()
2017-01-22 23:25 [PATCH 0/3] powerpc/kernel: Simplify rtas_initialize() Gavin Shan
2017-01-22 23:25 ` [PATCH 1/3] powerpc/kernel: Remove nested if statements in rtas_initialize() Gavin Shan
@ 2017-01-22 23:25 ` Gavin Shan
2017-01-22 23:25 ` [PATCH 3/3] powerpc/kernel: Fix unbalanced refcount on RTAS device node Gavin Shan
2 siblings, 0 replies; 6+ messages in thread
From: Gavin Shan @ 2017-01-22 23:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mpe, Gavin Shan
This uses of_property_read_u32() in rtas_initialize() so that we
needn't explicitly care the CPU's endian.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
arch/powerpc/kernel/rtas.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 9ba0f67..d13b92d 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -1145,7 +1145,8 @@ 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;
+ u32 base, size, entry;
+ int no_base, no_size, no_entry;
/* Get RTAS dev node and fill up our "rtas" structure with infos
* about it.
@@ -1154,20 +1155,17 @@ void __init rtas_initialize(void)
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) {
+ no_base = of_property_read_u32(rtas.dev, "linux,rtas-base", &base);
+ no_size = of_property_read_u32(rtas.dev, "rtas-size", &size);
+ if (no_base && no_size) {
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);
+ rtas.base = base;
+ rtas.size = size;
+ no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry);
+ rtas.entry = no_entry ? rtas.base : entry;
/* If RTAS was found, allocate the RMO buffer for it and look for
* the stop-self token if any
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] powerpc/kernel: Fix unbalanced refcount on RTAS device node
2017-01-22 23:25 [PATCH 0/3] powerpc/kernel: Simplify rtas_initialize() Gavin Shan
2017-01-22 23:25 ` [PATCH 1/3] powerpc/kernel: Remove nested if statements in rtas_initialize() Gavin Shan
2017-01-22 23:25 ` [PATCH 2/3] powerpc/kernel: Use of_property_read_u32() " Gavin Shan
@ 2017-01-22 23:25 ` Gavin Shan
2 siblings, 0 replies; 6+ messages in thread
From: Gavin Shan @ 2017-01-22 23:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mpe, Gavin Shan
The RTAS device-tree node's refcount has been increased by one in
the function call of_find_node_by_name(), but it's missed to be
decreased by one in the error path. It leads to unbalanced refcount
on RTAS device-tree node.
This fixes above issue by decreasing RTAS device-tree node's refcount
in error path.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
arch/powerpc/kernel/rtas.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index d13b92d..0823fe3 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -1158,6 +1158,7 @@ void __init rtas_initialize(void)
no_base = of_property_read_u32(rtas.dev, "linux,rtas-base", &base);
no_size = of_property_read_u32(rtas.dev, "rtas-size", &size);
if (no_base && no_size) {
+ of_node_put(rtas.dev);
rtas.dev = NULL;
return;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread