| Message ID | 1480583289-6380-2-git-send-email-mukesh02@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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") ?
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
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 >
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
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 >
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
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; }