[PATCH 1/2] powerpc/powernv/opal-dump : Handles opal_dump_info properly

[PATCH 1/2] powerpc/powernv/opal-dump : Handles opal_dump_info properly

[Mukesh Ojha]

Moves the return value check of 'opal_dump_info' to a proper place which
was previously unnecessarily filling all the dump info even on failure.

Signed-off-by: Mukesh Ojha <mukesh02@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/opal-dump.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-dump.c b/arch/powerpc/platforms/powernv/opal-dump.c
index 4c82782..ae32212 100644
--- a/arch/powerpc/platforms/powernv/opal-dump.c
+++ b/arch/powerpc/platforms/powernv/opal-dump.c
@@ -225,13 +225,16 @@ static int64_t dump_read_info(uint32_t *dump_id, uint32_t *dump_size, uint32_t *
 	if (rc == OPAL_PARAMETER)
 		rc = opal_dump_info(&id, &size);
 
+	if (rc) {
+		pr_warn("%s: Failed to get dump info (%d)\n",
+			__func__, rc);
+		return rc;
+	}
+
 	*dump_id = be32_to_cpu(id);
 	*dump_size = be32_to_cpu(size);
 	*dump_type = be32_to_cpu(type);
 
-	if (rc)
-		pr_warn("%s: Failed to get dump info (%d)\n",
-			__func__, rc);
 	return rc;
 }
 
-- 
2.7.4

[PATCH 2/2] powerpc/powernv/opal-dump : Use IRQ_HANDLED instead of numbers in interrupt handler

[Mukesh Ojha]

Converts all the return explicit number to a more proper IRQ_HANDLED,
which looks proper incase of interrupt handler returning case.

Signed-off-by: Mukesh Ojha <mukesh02@linux.vnet.ibm.com>
Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/opal-dump.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-dump.c b/arch/powerpc/platforms/powernv/opal-dump.c
index ae32212..71cf907 100644
--- a/arch/powerpc/platforms/powernv/opal-dump.c
+++ b/arch/powerpc/platforms/powernv/opal-dump.c
@@ -371,13 +371,12 @@ static irqreturn_t process_dump(int irq, void *data)
 {
 	int rc;
 	uint32_t dump_id, dump_size, dump_type;
-	struct dump_obj *dump;
 	char name[22];
 	struct kobject *kobj;
 
 	rc = dump_read_info(&dump_id, &dump_size, &dump_type);
 	if (rc != OPAL_SUCCESS)
-		return rc;
+		return IRQ_HANDLED;
 
 	sprintf(name, "0x%x-0x%x", dump_type, dump_id);
 
@@ -389,12 +388,10 @@ static irqreturn_t process_dump(int irq, void *data)
 	if (kobj) {
 		/* Drop reference added by kset_find_obj() */
 		kobject_put(kobj);
-		return 0;
+		return IRQ_HANDLED;
 	}
 
-	dump = create_dump_obj(dump_id, dump_size, dump_type);
-	if (!dump)
-		return -1;
+	create_dump_obj(dump_id, dump_size, dump_type);
 
 	return IRQ_HANDLED;
 }
-- 
2.7.4

Re: [PATCH 2/2] powerpc/powernv/opal-dump : Use IRQ_HANDLED instead of numbers in interrupt handler

[Jeremy Kerr]

Hi Mukesh,

> Converts all the return explicit number to a more proper IRQ_HANDLED,
> which looks proper incase of interrupt handler returning case.

This looks good to me, but can you describe the effects of those changes
to the interrupt handler's return code? ie, what happened in the
erroneous case where we returned 0 (== IRQ_NONE) - does this fix a
user-visible issue?

Cheers,


Jeremy

Re: [PATCH 2/2] powerpc/powernv/opal-dump : Use IRQ_HANDLED instead of numbers in interrupt handler

[Mukesh Ojha]

Hi Jeremy,


On Wednesday 15 February 2017 08:48 AM, Jeremy Kerr wrote:
> Hi Mukesh,
>
>> Converts all the return explicit number to a more proper IRQ_HANDLED,
>> which looks proper incase of interrupt handler returning case.
> This looks good to me, but can you describe the effects of those changes
> to the interrupt handler's return code? ie, what happened in the
> erroneous case where we returned 0 (== IRQ_NONE) - does this fix a
> user-visible issue?
>
> Cheers,

The return value of an interrupt handler is the special type 
irqreturn_t. An interrupt handler can return two special values,
IRQ_NONE or IRQ_HANDLED. The former is returned when the interrupt 
handler detects an interrupt for which its device was not
the originator. The latter is returned if the interrupt handler was 
correctly invoked, and its device did indeed cause the interrupt.

No, this is not user visible issue..and also here it does not matter 
what we return from here as we
are not handling the return value of the handler. This handler gets 
triggered when we get interrupt
from opal and in handler we do a opal call which on successful scenario 
disable the interrupt bit which
was set.

-Mukesh

>
>
> Jeremy
>

Re: [PATCH 2/2] powerpc/powernv/opal-dump : Use IRQ_HANDLED instead of numbers in interrupt handler

[Jeremy Kerr]

Hi Mukesh,

> The return value of an interrupt handler is the special type
> irqreturn_t. An interrupt handler can return two special values,
> IRQ_NONE or IRQ_HANDLED.

Yep, you can assume that the reader knows that level of the interrupt
handler API :) What we want to know is how that change in behaviour of
the handler code interacts with the core irq handler code.

The change you're proposing always returns IRQ_HANDLED, whereas the
previous code had cases where it returned:

  - IRQ_NONE, or
  - the (invalid) value -1.

Unless I'm mistaken, there are two things I can see happening with the
old code: if we hit the IRQ_NONE path enough, we'll report a "nobody
cared" error (see __report_bad_irq) and disable the interrupt, or for
the -1 case, we'll immediately log a "bogus return value" error (and, it
looks like a "no thread function available" error too, from
warn_no_thread).

So, it would be nice to mention that this change will fix errors that
result in those log messages. This means that someone debugging those
log messages in futures can find your patch, and see that it addresses
the issue.

> No, this is not user visible issue..

I consider the kernel log output to be user-visible.

> and also here it does not matter what we return from here as we
> are not handling the return value of the handler. 

*We* are not handling the return value of the handler, but the core IRQ
code is.

Regards,


Jeremy

Re: [PATCH 2/2] powerpc/powernv/opal-dump : Use IRQ_HANDLED instead of numbers in interrupt handler

[Mukesh Ojha]

On Thursday 16 February 2017 08:10 AM, Jeremy Kerr wrote:
> Hi Mukesh,
>
>> The return value of an interrupt handler is the special type
>> irqreturn_t. An interrupt handler can return two special values,
>> IRQ_NONE or IRQ_HANDLED.
> Yep, you can assume that the reader knows that level of the interrupt
> handler API :)

Sorry i misunderstood your question.

>   What we want to know is how that change in behaviour of
> the handler code interacts with the core irq handler code.
>
> The change you're proposing always returns IRQ_HANDLED, whereas the
> previous code had cases where it returned:
>
>    - IRQ_NONE, or
>    - the (invalid) value -1.

Agree.

>
> Unless I'm mistaken, there are two things I can see happening with the
> old code: if we hit the IRQ_NONE path enough, we'll report a "nobody
> cared" error (see __report_bad_irq) and disable the interrupt, or for
> the -1 case, we'll immediately log a "bogus return value" error (and, it
> looks like a "no thread function available" error too, from
> warn_no_thread).

bogus return value" error will not come as

action_ret <= (IRQ_HANDLED | IRQ_WAKE_THREAD)
i.e
         0/-1 <= 3 (true)

and also "no thread function available" as
we are not returning IRQ_WAKE_THREAD from handler.

Changing the patch description in v2.

Regards,
Mukesh
>
> So, it would be nice to mention that this change will fix errors that
> result in those log messages. This means that someone debugging those
> log messages in futures can find your patch, and see that it addresses
> the issue.
>
>> No, this is not user visible issue..
> I consider the kernel log output to be user-visible.
>
>> and also here it does not matter what we return from here as we
>> are not handling the return value of the handler.
> *We* are not handling the return value of the handler, but the core IRQ
> code is.
>
> Regards,
>
>
> Jeremy
>

Re: [PATCH 2/2] powerpc/powernv/opal-dump : Use IRQ_HANDLED instead of numbers in interrupt handler

[Jeremy Kerr]

Hi Mukesh,

>> Unless I'm mistaken, there are two things I can see happening with the
>> old code: if we hit the IRQ_NONE path enough, we'll report a "nobody
>> cared" error (see __report_bad_irq) and disable the interrupt, or for
>> the -1 case, we'll immediately log a "bogus return value" error (and, it
>> looks like a "no thread function available" error too, from
>> warn_no_thread).
> 
> bogus return value" error will not come as
> 
> action_ret <= (IRQ_HANDLED | IRQ_WAKE_THREAD)
> i.e
>         0/-1 <= 3 (true)

Well, no. irqreturn_t is an enum, and is being treated as an unsigned
value (the signed/unsigned behaviour of an enum is
implementation-defined), and:

          0xffffffff > 3

so we will *probably* see the bogus return value warning here (based on
some brief experimentation with gcc), but that does depend on the
compiler.

I've submitted a patch to make that comparison more obvious and
explicit:

  https://marc.info/?l=linux-kernel&m=148721917302673

Cheers,


Jeremy

Re: [PATCH 2/2] powerpc/powernv/opal-dump : Use IRQ_HANDLED instead of numbers in interrupt handler

[Stewart Smith]

Mukesh Ojha <mukesh02@linux.vnet.ibm.com> writes:

> Converts all the return explicit number to a more proper IRQ_HANDLED,
> which looks proper incase of interrupt handler returning case.
>
> Signed-off-by: Mukesh Ojha <mukesh02@linux.vnet.ibm.com>
> Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/opal-dump.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

Should also have:
Fixes: 8034f715f ("powernv/opal-dump: Convert to irq domain")

?

-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCH 2/2] powerpc/powernv/opal-dump : Use IRQ_HANDLED instead of numbers in interrupt handler

[Mukesh Ojha]

On Wednesday 15 February 2017 10:38 AM, Stewart Smith wrote:
> Mukesh Ojha <mukesh02@linux.vnet.ibm.com> writes:
>
>> Converts all the return explicit number to a more proper IRQ_HANDLED,
>> which looks proper incase of interrupt handler returning case.
>>
>> Signed-off-by: Mukesh Ojha <mukesh02@linux.vnet.ibm.com>
>> Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/platforms/powernv/opal-dump.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
> Should also have:
> Fixes: 8034f715f ("powernv/opal-dump: Convert to irq domain")
>
> ?
>

Yeah .. i should have mentioned which patch missed these changes.

-Mukesh

Re: [PATCH 1/2] powerpc/powernv/opal-dump : Handles opal_dump_info properly

[Mukesh Ojha]

Hi Michael,

Can you please have a look at this patchset as there is no

functional changes involve with this?

Thanks,
Mukesh




On Thursday 01 December 2016 02:38 PM, Mukesh Ojha wrote:
> Moves the return value check of 'opal_dump_info' to a proper place which
> was previously unnecessarily filling all the dump info even on failure.
>
> Signed-off-by: Mukesh Ojha <mukesh02@linux.vnet.ibm.com>
> ---
>   arch/powerpc/platforms/powernv/opal-dump.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/opal-dump.c b/arch/powerpc/platforms/powernv/opal-dump.c
> index 4c82782..ae32212 100644
> --- a/arch/powerpc/platforms/powernv/opal-dump.c
> +++ b/arch/powerpc/platforms/powernv/opal-dump.c
> @@ -225,13 +225,16 @@ static int64_t dump_read_info(uint32_t *dump_id, uint32_t *dump_size, uint32_t *
>   	if (rc == OPAL_PARAMETER)
>   		rc = opal_dump_info(&id, &size);
>
> +	if (rc) {
> +		pr_warn("%s: Failed to get dump info (%d)\n",
> +			__func__, rc);
> +		return rc;
> +	}
> +
>   	*dump_id = be32_to_cpu(id);
>   	*dump_size = be32_to_cpu(size);
>   	*dump_type = be32_to_cpu(type);
>
> -	if (rc)
> -		pr_warn("%s: Failed to get dump info (%d)\n",
> -			__func__, rc);
>   	return rc;
>   }
>

Re: [PATCH 1/2] powerpc/powernv/opal-dump : Handles opal_dump_info properly

[Mukesh Ojha]

Hi Micheal,

Can you please look at this patchset?

-Mukesh


On Tuesday 06 December 2016 12:07 PM, Mukesh Ojha wrote:
> Hi Michael,
>
> Can you please have a look at this patchset as there is no
>
> functional changes involve with this?
>
> Thanks,
> Mukesh
>
>
>
>
> On Thursday 01 December 2016 02:38 PM, Mukesh Ojha wrote:
>> Moves the return value check of 'opal_dump_info' to a proper place which
>> was previously unnecessarily filling all the dump info even on failure.
>>
>> Signed-off-by: Mukesh Ojha <mukesh02@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/platforms/powernv/opal-dump.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal-dump.c 
>> b/arch/powerpc/platforms/powernv/opal-dump.c
>> index 4c82782..ae32212 100644
>> --- a/arch/powerpc/platforms/powernv/opal-dump.c
>> +++ b/arch/powerpc/platforms/powernv/opal-dump.c
>> @@ -225,13 +225,16 @@ static int64_t dump_read_info(uint32_t 
>> *dump_id, uint32_t *dump_size, uint32_t *
>>       if (rc == OPAL_PARAMETER)
>>           rc = opal_dump_info(&id, &size);
>>
>> +    if (rc) {
>> +        pr_warn("%s: Failed to get dump info (%d)\n",
>> +            __func__, rc);
>> +        return rc;
>> +    }
>> +
>>       *dump_id = be32_to_cpu(id);
>>       *dump_size = be32_to_cpu(size);
>>       *dump_type = be32_to_cpu(type);
>>
>> -    if (rc)
>> -        pr_warn("%s: Failed to get dump info (%d)\n",
>> -            __func__, rc);
>>       return rc;
>>   }
>>
>

Re: [PATCH 1/2] powerpc/powernv/opal-dump : Handles opal_dump_info properly

[Jeremy Kerr]

Hi Mukesh,

> Moves the return value check of 'opal_dump_info' to a proper place which
> was previously unnecessarily filling all the dump info even on failure.

Acked-by: Jeremy Kerr <jk@ozlabs.org>

Thanks!


Jeremy

Re: [PATCH 1/2] powerpc/powernv/opal-dump : Handles opal_dump_info properly

[Stewart Smith]

Mukesh Ojha <mukesh02@linux.vnet.ibm.com> writes:
> Moves the return value check of 'opal_dump_info' to a proper place which
> was previously unnecessarily filling all the dump info even on failure.
>
> Signed-off-by: Mukesh Ojha <mukesh02@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/opal-dump.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)


Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com>

-- 
Stewart Smith
OPAL Architect, IBM.