[PATCH v3 0/2] acpi: apei: Bug fix to enable APEI support for ARMv8

[PATCH v3 0/2] acpi: apei: Bug fix to enable APEI support for ARMv8

[Loc Ho]

v3
* Change the title of the patch to reflect the patch
* Completely dropped the driver. Instead, move completely to the FW by
  accessing the GIC directly.
* Fix a bug with function acpi_gsi_to_irq
* Enable APEI multiple GHES source to share an single external IRQ

v2
* Make all code more generic naming
* Still waiting for comment from Linaro folks on APEI

---
Loc Ho (2):
  acpi: Fix proper return code for function acpi_gsi_to_irq
  acpi: apei: Enable APEI multiple GHES source to share an single
    external IRQ

 drivers/acpi/apei/ghes.c |    2 +-
 drivers/acpi/gsi.c       |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Loc Ho]

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);
 
-- 
1.7.1

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Borislav Petkov]

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.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Tuan Phan]

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.

-- 
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.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Borislav Petkov]

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 :-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Tuan Phan]

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?

-- 
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.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Borislav Petkov]

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?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Tuan Phan]

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?

-- 
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.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Borislav Petkov]

On Tue, Jan 12, 2016 at 10:26:04AM -0800, Tuan Phan wrote:
> So are you good with it?

How about this instead:

---
diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
index fa4585a6914e..e42f6b7eac13 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/gsi.c
@@ -46,17 +46,22 @@ static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity)
  * Returns: linux IRQ number on success (>0)
  *          -EINVAL on failure
  */
-int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
+int acpi_gsi_to_irq(u32 gsi)
 {
+	unsigned int irq;
+
 	struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
 							DOMAIN_BUS_ANY);
 
-	*irq = irq_find_mapping(d, gsi);
+	irq = irq_find_mapping(d, gsi);
 	/*
 	 * *irq == 0 means no mapping, that should
 	 * be reported as a failure
 	 */
-	return (*irq > 0) ? *irq : -EINVAL;
+	if (irq > 0)
+		return irq;
+	else
+		return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Tuan Phan]

On Tue, Jan 12, 2016 at 10:32 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Jan 12, 2016 at 10:26:04AM -0800, Tuan Phan wrote:
>> So are you good with it?
>
> How about this instead:
>
> ---
> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
> index fa4585a6914e..e42f6b7eac13 100644
> --- a/drivers/acpi/gsi.c
> +++ b/drivers/acpi/gsi.c
> @@ -46,17 +46,22 @@ static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity)
>   * Returns: linux IRQ number on success (>0)
>   *          -EINVAL on failure
>   */
> -int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> +int acpi_gsi_to_irq(u32 gsi)
>  {
> +       unsigned int irq;
> +
>         struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
>                                                         DOMAIN_BUS_ANY);
>
> -       *irq = irq_find_mapping(d, gsi);
> +       irq = irq_find_mapping(d, gsi);
>         /*
>          * *irq == 0 means no mapping, that should
>          * be reported as a failure
>          */
> -       return (*irq > 0) ? *irq : -EINVAL;
> +       if (irq > 0)
> +               return irq;
> +       else
> +               return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);

Well, it basically the same as old code except the change of API
interface. And it requires some changes in ACPI core and X86/I64 arch
code. Do we really want to go with this solution instead of just
return 0 on success and -EINVAL on failure?

-- 
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.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Borislav Petkov]

On Tue, Jan 12, 2016 at 10:46:15AM -0800, Tuan Phan wrote:
> Well, it basically the same as old code except the change of API
> interface. And it requires some changes in ACPI core and X86/I64 arch
> code.

That's why I said:

"Patch 1 could be improved. It is Rafael's call in the end of the day."

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Tuan Phan]

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;
}

-- 
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.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Tuan Phan]

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?

-- 
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.

[PATCH v3 1/2] acpi: Fix proper return code for function acpi_gsi_to_irq

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 2/2] acpi: apei: Enable APEI multiple GHES source to share an single external IRQ

[Loc Ho]

This patch allows APEI generic error source table with external
IRQ to share an single IRQ.

Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Loc Ho <lho@apm.com>
---
 drivers/acpi/apei/ghes.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 3dd9c46..edc8a39 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -1011,7 +1011,7 @@ static int ghes_probe(struct platform_device *ghes_dev)
 			       generic->header.source_id);
 			goto err_edac_unreg;
 		}
-		rc = request_irq(ghes->irq, ghes_irq_func, 0, "GHES IRQ", ghes);
+		rc = request_irq(ghes->irq, ghes_irq_func, IRQF_SHARED, "GHES IRQ", ghes);
 		if (rc) {
 			pr_err(GHES_PFX "Failed to register IRQ for generic hardware error source: %d\n",
 			       generic->header.source_id);
-- 
1.7.1

[PATCH v3 0/2] acpi: apei: Bug fix to enable APEI support for ARMv8

[Loc Ho]

Hi Borislav/Doung/etc,

> v3
> * Change the title of the patch to reflect the patch
> * Completely dropped the driver. Instead, move completely to the FW by
>   accessing the GIC directly.
> * Fix a bug with function acpi_gsi_to_irq
> * Enable APEI multiple GHES source to share an single external IRQ
>
> v2
> * Make all code more generic naming
> * Still waiting for comment from Linaro folks on APEI
>
> ---
> Loc Ho (2):
>   acpi: Fix proper return code for function acpi_gsi_to_irq
>   acpi: apei: Enable APEI multiple GHES source to share an single
>     external IRQ

It has been awhile and I thought I check again on this. Any issue with
these two changes as it is needed for ARMv8 64-bit APEI support.
Without these two changes, APEI support will NOT work with X-Gene
platforms.

-Loc

[PATCH v3 0/2] acpi: apei: Bug fix to enable APEI support for ARMv8

[Borislav Petkov]

On Fri, Jan 08, 2016 at 04:30:48PM -0800, Loc Ho wrote:
> It has been awhile and I thought I check again on this. Any issue with
> these two changes as it is needed for ARMv8 64-bit APEI support.
> Without these two changes, APEI support will NOT work with X-Gene
> platforms.

Patch 1 could be improved. It is Rafael's call in the end of the day.

Patch 2 looks ok to me.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.