[PATCH kernel v2] powerpc/pci: Fix broken INTx configuration via OF

[PATCH kernel v2] powerpc/pci: Fix broken INTx configuration via OF

[Alexey Kardashevskiy]

Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper")
replaced of_irq_parse_pci() + irq_create_of_mapping() with
of_irq_parse_and_map_pci() but this change lost virq returned by
irq_create_of_mapping() so virq remained zero causing INTx
misconfiguration.

This fixes pci_read_irq_line() not to loose a virq returned by
of_irq_parse_and_map_pci().

Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper"
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* changed the condition from <=0 to !=0 as by design
of_irq_parse_and_map_pci() can only return 0 for an error and virq>0.
---
 arch/powerpc/kernel/pci-common.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index ae2ede4..33580a9 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -362,7 +362,7 @@ struct pci_controller* pci_find_hose_for_OF_device(struct device_node* node)
  */
 static int pci_read_irq_line(struct pci_dev *pci_dev)
 {
-	unsigned int virq = 0;
+	unsigned int virq;
 
 	pr_debug("PCI: Try to map irq for %s...\n", pci_name(pci_dev));
 
@@ -370,7 +370,8 @@ static int pci_read_irq_line(struct pci_dev *pci_dev)
 	memset(&oirq, 0xff, sizeof(oirq));
 #endif
 	/* Try to get a mapping from the device-tree */
-	if (!of_irq_parse_and_map_pci(pci_dev, 0, 0)) {
+	virq = of_irq_parse_and_map_pci(pci_dev, 0, 0);
+	if (!virq) {
 		u8 line, pin;
 
 		/* If that fails, lets fallback to what is in the config
-- 
2.11.0

Re: [PATCH kernel v2] powerpc/pci: Fix broken INTx configuration via OF

[Michael Ellerman]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper")
> replaced of_irq_parse_pci() + irq_create_of_mapping() with
> of_irq_parse_and_map_pci() but this change lost virq returned by
> irq_create_of_mapping() so virq remained zero causing INTx
> misconfiguration.
>
> This fixes pci_read_irq_line() not to loose a virq returned by
> of_irq_parse_and_map_pci().
>
> Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper"
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v2:
> * changed the condition from <=0 to !=0 as by design
> of_irq_parse_and_map_pci() can only return 0 for an error and virq>0.

It returns int, so you should store the result in an int, and check the
result for <= 0.

Otherwise if it starts returning a negative error value this code will
break.

If it's a problem that of_irq_parse_and_map_pci() might internally
convert an unsigned virq to int then we need to fix that there, or make
of_irq_parse_and_map_pci() return unsigned also.

cheers

> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index ae2ede4..33580a9 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -362,7 +362,7 @@ struct pci_controller* pci_find_hose_for_OF_device(struct device_node* node)
>   */
>  static int pci_read_irq_line(struct pci_dev *pci_dev)
>  {
> -	unsigned int virq = 0;
> +	unsigned int virq;
>  
>  	pr_debug("PCI: Try to map irq for %s...\n", pci_name(pci_dev));
>  
> @@ -370,7 +370,8 @@ static int pci_read_irq_line(struct pci_dev *pci_dev)
>  	memset(&oirq, 0xff, sizeof(oirq));
>  #endif
>  	/* Try to get a mapping from the device-tree */
> -	if (!of_irq_parse_and_map_pci(pci_dev, 0, 0)) {
> +	virq = of_irq_parse_and_map_pci(pci_dev, 0, 0);
> +	if (!virq) {
>  		u8 line, pin;
>  
>  		/* If that fails, lets fallback to what is in the config
> -- 
> 2.11.0

Re: [PATCH kernel v2] powerpc/pci: Fix broken INTx configuration via OF

[Rob Herring]

On Thu, Feb 8, 2018 at 11:54 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>
>> Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper")
>> replaced of_irq_parse_pci() + irq_create_of_mapping() with
>> of_irq_parse_and_map_pci() but this change lost virq returned by
>> irq_create_of_mapping() so virq remained zero causing INTx
>> misconfiguration.
>>
>> This fixes pci_read_irq_line() not to loose a virq returned by
>> of_irq_parse_and_map_pci().
>>
>> Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper"
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v2:
>> * changed the condition from <=0 to !=0 as by design
>> of_irq_parse_and_map_pci() can only return 0 for an error and virq>0.
>
> It returns int, so you should store the result in an int, and check the
> result for <= 0.
>
> Otherwise if it starts returning a negative error value this code will
> break.

That won't happen until we're confident all callers expect NO_IRQ is
only 0 and don't treat negative as NO_IRQ. I expect that to be never,
but maybe of_irq_parse_and_map_pci calls can be audited more easily
than of_irq_parse_and_map. We ended up with of_irq_get() because we
need to return error codes.

Rob