| Message ID | 1446857519-5908-2-git-send-email-lho@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 06, 2015 at 05:51:58PM -0700, Loc Ho wrote: > The function acpi_gsi_to_irq should returns 0 on success as upper function > caller expect an 0 for sucesss. > > Signed-off-by: Tuan Phan <tphan@apm.com> > Signed-off-by: Loc Ho <lho@apm.com> > --- > drivers/acpi/gsi.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c > index fa4585a..0ed1003 100644 > --- a/drivers/acpi/gsi.c > +++ b/drivers/acpi/gsi.c > @@ -43,7 +43,7 @@ static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) > * > * irq location updated with irq value [>0 on success, 0 on failure] > * > - * Returns: linux IRQ number on success (>0) > + * Returns: 0 on success > * -EINVAL on failure > */ > int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > @@ -56,7 +56,7 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > * *irq == 0 means no mapping, that should > * be reported as a failure > */ > - return (*irq > 0) ? *irq : -EINVAL; > + return (*irq > 0) ? 0 : -EINVAL; > } > EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); That function can be simplified. It should be made to return the irq number on success and 0 on failure. No need for that *irq output argument.
On Sun, Jan 10, 2016 at 3:07 AM, Borislav Petkov <bp@alien8.de> wrote: > > On Fri, Nov 06, 2015 at 05:51:58PM -0700, Loc Ho wrote: > > The function acpi_gsi_to_irq should returns 0 on success as upper function > > caller expect an 0 for sucesss. > > > > Signed-off-by: Tuan Phan <tphan@apm.com> > > Signed-off-by: Loc Ho <lho@apm.com> > > --- > > drivers/acpi/gsi.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c > > index fa4585a..0ed1003 100644 > > --- a/drivers/acpi/gsi.c > > +++ b/drivers/acpi/gsi.c > > @@ -43,7 +43,7 @@ static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) > > * > > * irq location updated with irq value [>0 on success, 0 on failure] > > * > > - * Returns: linux IRQ number on success (>0) > > + * Returns: 0 on success > > * -EINVAL on failure > > */ > > int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > > @@ -56,7 +56,7 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > > * *irq == 0 means no mapping, that should > > * be reported as a failure > > */ > > - return (*irq > 0) ? *irq : -EINVAL; > > + return (*irq > 0) ? 0 : -EINVAL; > > } > > EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); > > That function can be simplified. It should be made to return the irq > number on success and 0 on failure. No need for that *irq output > argument. > > -- > Regards/Gruss, > Boris. > > ECO tip #101: Trim your mails when you reply. Hi Boris, The same function which is implemented for x86 (arch/x86/kernel/acpi/boot.c) also returns 0 on success and -1 on failure. If we need to change the API, need to change for X86 too which we don't have a way to test it.
On Mon, Jan 11, 2016 at 02:04:52PM -0800, Tuan Phan wrote: > Hi Boris, > The same function which is implemented for x86 > (arch/x86/kernel/acpi/boot.c) also returns 0 on success and -1 on > failure. If we need to change the API, need to change for X86 too > which we don't have a way to test it. Well, fortunately, I have an x86 box to test it :-)
On Mon, Jan 11, 2016 at 2:13 PM, Borislav Petkov <bp@alien8.de> wrote: >That function can be simplified. It should be made to return the irq >number on success and 0 on failure. No need for that *irq output >argument. It will be problem because irq number can be 0 on X86?
On Mon, Jan 11, 2016 at 02:41:41PM -0800, Tuan Phan wrote:
> It will be problem because irq number can be 0 on X86?
tglx says not really but historically irq 0 has been the timer irq.
How about returning a negative value on error and positive or 0 for an
IRQ?
On Tue, Jan 12, 2016 at 6:36 AM, Borislav Petkov <bp@alien8.de> wrote: > On Mon, Jan 11, 2016 at 02:41:41PM -0800, Tuan Phan wrote: >> It will be problem because irq number can be 0 on X86? > > tglx says not really but historically irq 0 has been the timer irq. > > How about returning a negative value on error and positive or 0 for an > IRQ? > That's exactly the patch tries to do: int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) @@ -56,7 +56,7 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) * *irq == 0 means no mapping, that should * be reported as a failure */ - return (*irq > 0) ? *irq : -EINVAL; + return (*irq > 0) ? 0 : -EINVAL; } So are you good with it?
On 07/11/15 00:51, Loc Ho wrote: > The function acpi_gsi_to_irq should returns 0 on success as upper function > caller expect an 0 for sucesss. I have to ask: why do you feel the need to change an API that behaves like the rest of the IRQ allocation functions (at least when it comes to its return value)? 0 is defined as an invalid interrupt, and I wish it stayed that way. Thanks, M.
On Tue, Jan 12, 2016 at 10:59 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 07/11/15 00:51, Loc Ho wrote: >> The function acpi_gsi_to_irq should returns 0 on success as upper function >> caller expect an 0 for sucesss. > > I have to ask: why do you feel the need to change an API that behaves > like the rest of the IRQ allocation functions (at least when it comes to > its return value)? > > 0 is defined as an invalid interrupt, and I wish it stayed that way. The upper caller expects 0 for success. In drivers/acpi/apei/ghes.c: case ACPI_HEST_NOTIFY_EXTERNAL: /* External interrupt vector is GSI */ rc = acpi_gsi_to_irq(generic->notify.vector, &ghes->irq); if (rc) { pr_err(GHES_PFX "Failed to map GSI to IRQ for generic hardware error source: %d\n", generic->header.source_id); goto err_edac_unreg; } Also the implementation of acpi_gsi_to_irq for X86 in arch/x86/kernel/acpi/boot.c also return 0 for success and -1 for failure. int acpi_gsi_to_irq(u32 gsi, unsigned int *irqp) { int rc, irq, trigger, polarity; if (acpi_irq_model == ACPI_IRQ_MODEL_PIC) { *irqp = gsi; return 0; } rc = acpi_get_override_irq(gsi, &trigger, &polarity); if (rc == 0) { trigger = trigger ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE; polarity = polarity ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH; irq = acpi_register_gsi(NULL, gsi, trigger, polarity); if (irq >= 0) { *irqp = irq; return 0; } } return -1; }
On 12/01/16 19:13, Tuan Phan wrote: > On Tue, Jan 12, 2016 at 10:59 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On 07/11/15 00:51, Loc Ho wrote: >>> The function acpi_gsi_to_irq should returns 0 on success as upper function >>> caller expect an 0 for sucesss. >> >> I have to ask: why do you feel the need to change an API that behaves >> like the rest of the IRQ allocation functions (at least when it comes to >> its return value)? >> >> 0 is defined as an invalid interrupt, and I wish it stayed that way. > > The upper caller expects 0 for success. > In drivers/acpi/apei/ghes.c: > case ACPI_HEST_NOTIFY_EXTERNAL: > /* External interrupt vector is GSI */ > rc = acpi_gsi_to_irq(generic->notify.vector, &ghes->irq); > if (rc) { > pr_err(GHES_PFX "Failed to map GSI to IRQ for > generic hardware error source: %d\n", > generic->header.source_id); > goto err_edac_unreg; > } > > Also the implementation of acpi_gsi_to_irq for X86 in > arch/x86/kernel/acpi/boot.c also return 0 for success and -1 for > failure. > int acpi_gsi_to_irq(u32 gsi, unsigned int *irqp) > { > int rc, irq, trigger, polarity; > > if (acpi_irq_model == ACPI_IRQ_MODEL_PIC) { > *irqp = gsi; > return 0; > } > > rc = acpi_get_override_irq(gsi, &trigger, &polarity); > if (rc == 0) { > trigger = trigger ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE; > polarity = polarity ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH; > irq = acpi_register_gsi(NULL, gsi, trigger, polarity); > if (irq >= 0) { > *irqp = irq; > return 0; > } > } > > return -1; > } > Right. In which case please document this properly in the commit log. Thanks, M.
On Tue, Jan 12, 2016 at 11:32 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > Right. In which case please document this properly in the commit log. > Do we need to post a new patch with updated description or wait Rafael's input?
On 12/01/16 20:01, Tuan Phan wrote: > On Tue, Jan 12, 2016 at 11:32 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> > Right. In which case please document this properly in the commit log. >> > > Do we need to post a new patch with updated description or wait Rafael's input? Given that there is quite a few comments on this series already, it would make sense to update it and repost it. > -- CONFIDENTIALITY NOTICE: This e-mail message, including any > attachments, is for the sole use of the intended recipient(s) and > contains information that is confidential and proprietary to Applied > Micro Circuits Corporation or its subsidiaries. It is to be used solely > for the purpose of furthering the parties' business relationship. All > unauthorized review, use, disclosure or distribution is prohibited. If > you are not the intended recipient, please contact the sender by reply > e-mail and destroy all copies of the original message. And please fix your email setup. By the look of it, I'm not even supposed to read this. Thanks, M.
diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c index fa4585a..0ed1003 100644 --- a/drivers/acpi/gsi.c +++ b/drivers/acpi/gsi.c @@ -43,7 +43,7 @@ static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) * * irq location updated with irq value [>0 on success, 0 on failure] * - * Returns: linux IRQ number on success (>0) + * Returns: 0 on success * -EINVAL on failure */ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) @@ -56,7 +56,7 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) * *irq == 0 means no mapping, that should * be reported as a failure */ - return (*irq > 0) ? *irq : -EINVAL; + return (*irq > 0) ? 0 : -EINVAL; } EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);