[PATCH] ACPI: PCI: IRQ: Handle INTx GSIs as u32 values not int

Lorenzo Pieralisi posted 1 patch 1 month, 1 week ago
drivers/acpi/pci_irq.c      | 19 ++++++++++--------
drivers/acpi/pci_link.c     | 39 ++++++++++++++++++++++++-------------
drivers/xen/acpi.c          | 13 +++++++------
include/acpi/acpi_drivers.h |  2 +-
4 files changed, 44 insertions(+), 29 deletions(-)
[PATCH] ACPI: PCI: IRQ: Handle INTx GSIs as u32 values not int
Posted by Lorenzo Pieralisi 1 month, 1 week ago
In ACPI Global System Interrupts (GSIs) are described using a 32-bit
value.

ACPI/PCI legacy interrupts (INTx) parsing code treats GSIs as 'int', which
poses issues if the GSI interrupt value is a 32-bit value with the MSB set
(as required in some interrupt configurations - eg ARM64 GICv5 systems).

Fix ACPI/PCI legacy INTx parsing by converting variables representing
GSIs from 'int' to 'u32' bringing the code in line with the ACPI
specification.

Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
---
 drivers/acpi/pci_irq.c      | 19 ++++++++++--------
 drivers/acpi/pci_link.c     | 39 ++++++++++++++++++++++++-------------
 drivers/xen/acpi.c          | 13 +++++++------
 include/acpi/acpi_drivers.h |  2 +-
 4 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index ad81aa03fe2f..c416942ff3e2 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -188,7 +188,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev,
 	 * the IRQ value, which is hardwired to specific interrupt inputs on
 	 * the interrupt controller.
 	 */
-	pr_debug("%04x:%02x:%02x[%c] -> %s[%d]\n",
+	pr_debug("%04x:%02x:%02x[%c] -> %s[%u]\n",
 		 entry->id.segment, entry->id.bus, entry->id.device,
 		 pin_name(entry->pin), prt->source, entry->index);
 
@@ -384,7 +384,7 @@ static inline bool acpi_pci_irq_valid(struct pci_dev *dev, u8 pin)
 int acpi_pci_irq_enable(struct pci_dev *dev)
 {
 	struct acpi_prt_entry *entry;
-	int gsi;
+	u32 gsi;
 	u8 pin;
 	int triggering = ACPI_LEVEL_SENSITIVE;
 	/*
@@ -422,18 +422,21 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
 			return 0;
 	}
 
+	rc = -ENODEV;
+
 	if (entry) {
 		if (entry->link)
-			gsi = acpi_pci_link_allocate_irq(entry->link,
+			rc = acpi_pci_link_allocate_irq(entry->link,
 							 entry->index,
 							 &triggering, &polarity,
-							 &link);
-		else
+							 &link, &gsi);
+		else {
 			gsi = entry->index;
-	} else
-		gsi = -1;
+			rc = 0;
+		}
+	}
 
-	if (gsi < 0) {
+	if (rc < 0) {
 		/*
 		 * No IRQ known to the ACPI subsystem - maybe the BIOS /
 		 * driver reported one, then use it. Exit in any case.
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index bed7dc85612e..b91b039a3d20 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -448,7 +448,7 @@ static int acpi_isa_irq_penalty[ACPI_MAX_ISA_IRQS] = {
 	/* >IRQ15 */
 };
 
-static int acpi_irq_pci_sharing_penalty(int irq)
+static int acpi_irq_pci_sharing_penalty(u32 irq)
 {
 	struct acpi_pci_link *link;
 	int penalty = 0;
@@ -474,7 +474,7 @@ static int acpi_irq_pci_sharing_penalty(int irq)
 	return penalty;
 }
 
-static int acpi_irq_get_penalty(int irq)
+static int acpi_irq_get_penalty(u32 irq)
 {
 	int penalty = 0;
 
@@ -528,7 +528,7 @@ static int acpi_irq_balance = -1;	/* 0: static, 1: balance */
 static int acpi_pci_link_allocate(struct acpi_pci_link *link)
 {
 	acpi_handle handle = link->device->handle;
-	int irq;
+	u32 irq;
 	int i;
 
 	if (link->irq.initialized) {
@@ -598,44 +598,53 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
 	return 0;
 }
 
-/*
- * acpi_pci_link_allocate_irq
- * success: return IRQ >= 0
- * failure: return -1
+/**
+ * acpi_pci_link_allocate_irq(): Retrieve a link device GSI
+ *
+ * @handle: Handle for the link device
+ * @index: GSI index
+ * @triggering: pointer to store the GSI trigger
+ * @polarity: pointer to store GSI polarity
+ * @name: pointer to store link device name
+ * @gsi: pointer to store GSI number
+ *
+ * Returns:
+ *	0 on success with @triggering, @polarity, @name, @gsi initialized.
+ *	-ENODEV on failure
  */
 int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
-			       int *polarity, char **name)
+			       int *polarity, char **name, u32 *gsi)
 {
 	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	struct acpi_pci_link *link;
 
 	if (!device) {
 		acpi_handle_err(handle, "Invalid link device\n");
-		return -1;
+		return -ENODEV;
 	}
 
 	link = acpi_driver_data(device);
 	if (!link) {
 		acpi_handle_err(handle, "Invalid link context\n");
-		return -1;
+		return -ENODEV;
 	}
 
 	/* TBD: Support multiple index (IRQ) entries per Link Device */
 	if (index) {
 		acpi_handle_err(handle, "Invalid index %d\n", index);
-		return -1;
+		return -ENODEV;
 	}
 
 	mutex_lock(&acpi_link_lock);
 	if (acpi_pci_link_allocate(link)) {
 		mutex_unlock(&acpi_link_lock);
-		return -1;
+		return -ENODEV;
 	}
 
 	if (!link->irq.active) {
 		mutex_unlock(&acpi_link_lock);
 		acpi_handle_err(handle, "Link active IRQ is 0!\n");
-		return -1;
+		return -ENODEV;
 	}
 	link->refcnt++;
 	mutex_unlock(&acpi_link_lock);
@@ -647,7 +656,9 @@ int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
 	if (name)
 		*name = acpi_device_bid(link->device);
 	acpi_handle_debug(handle, "Link is referenced\n");
-	return link->irq.active;
+	*gsi = link->irq.active;
+
+	return 0;
 }
 
 /*
diff --git a/drivers/xen/acpi.c b/drivers/xen/acpi.c
index d2ee605c5ca1..eab28cfe9939 100644
--- a/drivers/xen/acpi.c
+++ b/drivers/xen/acpi.c
@@ -89,11 +89,11 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev,
 						  int *trigger_out,
 						  int *polarity_out)
 {
-	int gsi;
+	u32 gsi;
 	u8 pin;
 	struct acpi_prt_entry *entry;
 	int trigger = ACPI_LEVEL_SENSITIVE;
-	int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ?
+	int ret, polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ?
 				      ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW;
 
 	if (!dev || !gsi_out || !trigger_out || !polarity_out)
@@ -105,17 +105,18 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev,
 
 	entry = acpi_pci_irq_lookup(dev, pin);
 	if (entry) {
+		ret = 0;
 		if (entry->link)
-			gsi = acpi_pci_link_allocate_irq(entry->link,
+			ret = acpi_pci_link_allocate_irq(entry->link,
 							 entry->index,
 							 &trigger, &polarity,
-							 NULL);
+							 NULL, &gsi);
 		else
 			gsi = entry->index;
 	} else
-		gsi = -1;
+		ret = -ENODEV;
 
-	if (gsi < 0)
+	if (ret < 0)
 		return -EINVAL;
 
 	*gsi_out = gsi;
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index b14d165632e7..402b97d12138 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -51,7 +51,7 @@
 
 int acpi_irq_penalty_init(void);
 int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
-			       int *polarity, char **name);
+			       int *polarity, char **name, u32 *gsi);
 int acpi_pci_link_free_irq(acpi_handle handle);
 
 /* ACPI PCI Device Binding */
-- 
2.50.1
Re: [PATCH] ACPI: PCI: IRQ: Handle INTx GSIs as u32 values not int
Posted by Bjorn Helgaas 1 month, 1 week ago
On Wed, Dec 31, 2025 at 10:26:15AM +0100, Lorenzo Pieralisi wrote:
> In ACPI Global System Interrupts (GSIs) are described using a 32-bit
> value.
> 
> ACPI/PCI legacy interrupts (INTx) parsing code treats GSIs as 'int', which
> poses issues if the GSI interrupt value is a 32-bit value with the MSB set
> (as required in some interrupt configurations - eg ARM64 GICv5 systems).
> 
> Fix ACPI/PCI legacy INTx parsing by converting variables representing
> GSIs from 'int' to 'u32' bringing the code in line with the ACPI
> specification.

Looks good to me.  Is there any symptom of what the issue looks like
that could be mentioned here?  Might also be useful in the subject,
which currently describes the C code without really saying why we want
to do this.

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

> Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> ---
>  drivers/acpi/pci_irq.c      | 19 ++++++++++--------
>  drivers/acpi/pci_link.c     | 39 ++++++++++++++++++++++++-------------
>  drivers/xen/acpi.c          | 13 +++++++------
>  include/acpi/acpi_drivers.h |  2 +-
>  4 files changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> index ad81aa03fe2f..c416942ff3e2 100644
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -188,7 +188,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev,
>  	 * the IRQ value, which is hardwired to specific interrupt inputs on
>  	 * the interrupt controller.
>  	 */
> -	pr_debug("%04x:%02x:%02x[%c] -> %s[%d]\n",
> +	pr_debug("%04x:%02x:%02x[%c] -> %s[%u]\n",
>  		 entry->id.segment, entry->id.bus, entry->id.device,
>  		 pin_name(entry->pin), prt->source, entry->index);
>  
> @@ -384,7 +384,7 @@ static inline bool acpi_pci_irq_valid(struct pci_dev *dev, u8 pin)
>  int acpi_pci_irq_enable(struct pci_dev *dev)
>  {
>  	struct acpi_prt_entry *entry;
> -	int gsi;
> +	u32 gsi;
>  	u8 pin;
>  	int triggering = ACPI_LEVEL_SENSITIVE;
>  	/*
> @@ -422,18 +422,21 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>  			return 0;
>  	}
>  
> +	rc = -ENODEV;
> +
>  	if (entry) {
>  		if (entry->link)
> -			gsi = acpi_pci_link_allocate_irq(entry->link,
> +			rc = acpi_pci_link_allocate_irq(entry->link,
>  							 entry->index,
>  							 &triggering, &polarity,
> -							 &link);
> -		else
> +							 &link, &gsi);
> +		else {
>  			gsi = entry->index;
> -	} else
> -		gsi = -1;
> +			rc = 0;
> +		}
> +	}
>  
> -	if (gsi < 0) {
> +	if (rc < 0) {
>  		/*
>  		 * No IRQ known to the ACPI subsystem - maybe the BIOS /
>  		 * driver reported one, then use it. Exit in any case.
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index bed7dc85612e..b91b039a3d20 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -448,7 +448,7 @@ static int acpi_isa_irq_penalty[ACPI_MAX_ISA_IRQS] = {
>  	/* >IRQ15 */
>  };
>  
> -static int acpi_irq_pci_sharing_penalty(int irq)
> +static int acpi_irq_pci_sharing_penalty(u32 irq)
>  {
>  	struct acpi_pci_link *link;
>  	int penalty = 0;
> @@ -474,7 +474,7 @@ static int acpi_irq_pci_sharing_penalty(int irq)
>  	return penalty;
>  }
>  
> -static int acpi_irq_get_penalty(int irq)
> +static int acpi_irq_get_penalty(u32 irq)
>  {
>  	int penalty = 0;
>  
> @@ -528,7 +528,7 @@ static int acpi_irq_balance = -1;	/* 0: static, 1: balance */
>  static int acpi_pci_link_allocate(struct acpi_pci_link *link)
>  {
>  	acpi_handle handle = link->device->handle;
> -	int irq;
> +	u32 irq;
>  	int i;
>  
>  	if (link->irq.initialized) {
> @@ -598,44 +598,53 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
>  	return 0;
>  }
>  
> -/*
> - * acpi_pci_link_allocate_irq
> - * success: return IRQ >= 0
> - * failure: return -1
> +/**
> + * acpi_pci_link_allocate_irq(): Retrieve a link device GSI
> + *
> + * @handle: Handle for the link device
> + * @index: GSI index
> + * @triggering: pointer to store the GSI trigger
> + * @polarity: pointer to store GSI polarity
> + * @name: pointer to store link device name
> + * @gsi: pointer to store GSI number
> + *
> + * Returns:
> + *	0 on success with @triggering, @polarity, @name, @gsi initialized.
> + *	-ENODEV on failure
>   */
>  int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
> -			       int *polarity, char **name)
> +			       int *polarity, char **name, u32 *gsi)
>  {
>  	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
>  	struct acpi_pci_link *link;
>  
>  	if (!device) {
>  		acpi_handle_err(handle, "Invalid link device\n");
> -		return -1;
> +		return -ENODEV;
>  	}
>  
>  	link = acpi_driver_data(device);
>  	if (!link) {
>  		acpi_handle_err(handle, "Invalid link context\n");
> -		return -1;
> +		return -ENODEV;
>  	}
>  
>  	/* TBD: Support multiple index (IRQ) entries per Link Device */
>  	if (index) {
>  		acpi_handle_err(handle, "Invalid index %d\n", index);
> -		return -1;
> +		return -ENODEV;
>  	}
>  
>  	mutex_lock(&acpi_link_lock);
>  	if (acpi_pci_link_allocate(link)) {
>  		mutex_unlock(&acpi_link_lock);
> -		return -1;
> +		return -ENODEV;
>  	}
>  
>  	if (!link->irq.active) {
>  		mutex_unlock(&acpi_link_lock);
>  		acpi_handle_err(handle, "Link active IRQ is 0!\n");
> -		return -1;
> +		return -ENODEV;
>  	}
>  	link->refcnt++;
>  	mutex_unlock(&acpi_link_lock);
> @@ -647,7 +656,9 @@ int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
>  	if (name)
>  		*name = acpi_device_bid(link->device);
>  	acpi_handle_debug(handle, "Link is referenced\n");
> -	return link->irq.active;
> +	*gsi = link->irq.active;
> +
> +	return 0;
>  }
>  
>  /*
> diff --git a/drivers/xen/acpi.c b/drivers/xen/acpi.c
> index d2ee605c5ca1..eab28cfe9939 100644
> --- a/drivers/xen/acpi.c
> +++ b/drivers/xen/acpi.c
> @@ -89,11 +89,11 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev,
>  						  int *trigger_out,
>  						  int *polarity_out)
>  {
> -	int gsi;
> +	u32 gsi;
>  	u8 pin;
>  	struct acpi_prt_entry *entry;
>  	int trigger = ACPI_LEVEL_SENSITIVE;
> -	int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ?
> +	int ret, polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ?
>  				      ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW;
>  
>  	if (!dev || !gsi_out || !trigger_out || !polarity_out)
> @@ -105,17 +105,18 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev,
>  
>  	entry = acpi_pci_irq_lookup(dev, pin);
>  	if (entry) {
> +		ret = 0;
>  		if (entry->link)
> -			gsi = acpi_pci_link_allocate_irq(entry->link,
> +			ret = acpi_pci_link_allocate_irq(entry->link,
>  							 entry->index,
>  							 &trigger, &polarity,
> -							 NULL);
> +							 NULL, &gsi);
>  		else
>  			gsi = entry->index;
>  	} else
> -		gsi = -1;
> +		ret = -ENODEV;
>  
> -	if (gsi < 0)
> +	if (ret < 0)
>  		return -EINVAL;
>  
>  	*gsi_out = gsi;
> diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> index b14d165632e7..402b97d12138 100644
> --- a/include/acpi/acpi_drivers.h
> +++ b/include/acpi/acpi_drivers.h
> @@ -51,7 +51,7 @@
>  
>  int acpi_irq_penalty_init(void);
>  int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
> -			       int *polarity, char **name);
> +			       int *polarity, char **name, u32 *gsi);
>  int acpi_pci_link_free_irq(acpi_handle handle);
>  
>  /* ACPI PCI Device Binding */
> -- 
> 2.50.1
>
Re: [PATCH] ACPI: PCI: IRQ: Handle INTx GSIs as u32 values not int
Posted by Lorenzo Pieralisi 1 month ago
On Wed, Dec 31, 2025 at 11:01:50AM -0600, Bjorn Helgaas wrote:
> On Wed, Dec 31, 2025 at 10:26:15AM +0100, Lorenzo Pieralisi wrote:
> > In ACPI Global System Interrupts (GSIs) are described using a 32-bit
> > value.
> > 
> > ACPI/PCI legacy interrupts (INTx) parsing code treats GSIs as 'int', which
> > poses issues if the GSI interrupt value is a 32-bit value with the MSB set
> > (as required in some interrupt configurations - eg ARM64 GICv5 systems).
> > 
> > Fix ACPI/PCI legacy INTx parsing by converting variables representing
> > GSIs from 'int' to 'u32' bringing the code in line with the ACPI
> > specification.
> 
> Looks good to me.  Is there any symptom of what the issue looks like
> that could be mentioned here?  Might also be useful in the subject,
> which currently describes the C code without really saying why we want
> to do this.

Thanks ! Happy New Year !

acpi_pci_link_allocate_irq() would return a GSI with MSB set, that the
logic in acpi_pci_irq_enable() would treat as a failure because that's
a negative value.

After fixing that - passing a 32-bit value with MSB set to
acpi_irq_get_penalty() causes an array acpi_isa_irq_penalty dereference
with an an index that is way beyond the array size.

This is not a fix per-se because GICv5 ACPI is not in the mainline, yet.

I can try to summarize the issue in the commit log.

Thanks,
Lorenzo

> 
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > ---
> >  drivers/acpi/pci_irq.c      | 19 ++++++++++--------
> >  drivers/acpi/pci_link.c     | 39 ++++++++++++++++++++++++-------------
> >  drivers/xen/acpi.c          | 13 +++++++------
> >  include/acpi/acpi_drivers.h |  2 +-
> >  4 files changed, 44 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> > index ad81aa03fe2f..c416942ff3e2 100644
> > --- a/drivers/acpi/pci_irq.c
> > +++ b/drivers/acpi/pci_irq.c
> > @@ -188,7 +188,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev,
> >  	 * the IRQ value, which is hardwired to specific interrupt inputs on
> >  	 * the interrupt controller.
> >  	 */
> > -	pr_debug("%04x:%02x:%02x[%c] -> %s[%d]\n",
> > +	pr_debug("%04x:%02x:%02x[%c] -> %s[%u]\n",
> >  		 entry->id.segment, entry->id.bus, entry->id.device,
> >  		 pin_name(entry->pin), prt->source, entry->index);
> >  
> > @@ -384,7 +384,7 @@ static inline bool acpi_pci_irq_valid(struct pci_dev *dev, u8 pin)
> >  int acpi_pci_irq_enable(struct pci_dev *dev)
> >  {
> >  	struct acpi_prt_entry *entry;
> > -	int gsi;
> > +	u32 gsi;
> >  	u8 pin;
> >  	int triggering = ACPI_LEVEL_SENSITIVE;
> >  	/*
> > @@ -422,18 +422,21 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
> >  			return 0;
> >  	}
> >  
> > +	rc = -ENODEV;
> > +
> >  	if (entry) {
> >  		if (entry->link)
> > -			gsi = acpi_pci_link_allocate_irq(entry->link,
> > +			rc = acpi_pci_link_allocate_irq(entry->link,
> >  							 entry->index,
> >  							 &triggering, &polarity,
> > -							 &link);
> > -		else
> > +							 &link, &gsi);
> > +		else {
> >  			gsi = entry->index;
> > -	} else
> > -		gsi = -1;
> > +			rc = 0;
> > +		}
> > +	}
> >  
> > -	if (gsi < 0) {
> > +	if (rc < 0) {
> >  		/*
> >  		 * No IRQ known to the ACPI subsystem - maybe the BIOS /
> >  		 * driver reported one, then use it. Exit in any case.
> > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> > index bed7dc85612e..b91b039a3d20 100644
> > --- a/drivers/acpi/pci_link.c
> > +++ b/drivers/acpi/pci_link.c
> > @@ -448,7 +448,7 @@ static int acpi_isa_irq_penalty[ACPI_MAX_ISA_IRQS] = {
> >  	/* >IRQ15 */
> >  };
> >  
> > -static int acpi_irq_pci_sharing_penalty(int irq)
> > +static int acpi_irq_pci_sharing_penalty(u32 irq)
> >  {
> >  	struct acpi_pci_link *link;
> >  	int penalty = 0;
> > @@ -474,7 +474,7 @@ static int acpi_irq_pci_sharing_penalty(int irq)
> >  	return penalty;
> >  }
> >  
> > -static int acpi_irq_get_penalty(int irq)
> > +static int acpi_irq_get_penalty(u32 irq)
> >  {
> >  	int penalty = 0;
> >  
> > @@ -528,7 +528,7 @@ static int acpi_irq_balance = -1;	/* 0: static, 1: balance */
> >  static int acpi_pci_link_allocate(struct acpi_pci_link *link)
> >  {
> >  	acpi_handle handle = link->device->handle;
> > -	int irq;
> > +	u32 irq;
> >  	int i;
> >  
> >  	if (link->irq.initialized) {
> > @@ -598,44 +598,53 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
> >  	return 0;
> >  }
> >  
> > -/*
> > - * acpi_pci_link_allocate_irq
> > - * success: return IRQ >= 0
> > - * failure: return -1
> > +/**
> > + * acpi_pci_link_allocate_irq(): Retrieve a link device GSI
> > + *
> > + * @handle: Handle for the link device
> > + * @index: GSI index
> > + * @triggering: pointer to store the GSI trigger
> > + * @polarity: pointer to store GSI polarity
> > + * @name: pointer to store link device name
> > + * @gsi: pointer to store GSI number
> > + *
> > + * Returns:
> > + *	0 on success with @triggering, @polarity, @name, @gsi initialized.
> > + *	-ENODEV on failure
> >   */
> >  int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
> > -			       int *polarity, char **name)
> > +			       int *polarity, char **name, u32 *gsi)
> >  {
> >  	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
> >  	struct acpi_pci_link *link;
> >  
> >  	if (!device) {
> >  		acpi_handle_err(handle, "Invalid link device\n");
> > -		return -1;
> > +		return -ENODEV;
> >  	}
> >  
> >  	link = acpi_driver_data(device);
> >  	if (!link) {
> >  		acpi_handle_err(handle, "Invalid link context\n");
> > -		return -1;
> > +		return -ENODEV;
> >  	}
> >  
> >  	/* TBD: Support multiple index (IRQ) entries per Link Device */
> >  	if (index) {
> >  		acpi_handle_err(handle, "Invalid index %d\n", index);
> > -		return -1;
> > +		return -ENODEV;
> >  	}
> >  
> >  	mutex_lock(&acpi_link_lock);
> >  	if (acpi_pci_link_allocate(link)) {
> >  		mutex_unlock(&acpi_link_lock);
> > -		return -1;
> > +		return -ENODEV;
> >  	}
> >  
> >  	if (!link->irq.active) {
> >  		mutex_unlock(&acpi_link_lock);
> >  		acpi_handle_err(handle, "Link active IRQ is 0!\n");
> > -		return -1;
> > +		return -ENODEV;
> >  	}
> >  	link->refcnt++;
> >  	mutex_unlock(&acpi_link_lock);
> > @@ -647,7 +656,9 @@ int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
> >  	if (name)
> >  		*name = acpi_device_bid(link->device);
> >  	acpi_handle_debug(handle, "Link is referenced\n");
> > -	return link->irq.active;
> > +	*gsi = link->irq.active;
> > +
> > +	return 0;
> >  }
> >  
> >  /*
> > diff --git a/drivers/xen/acpi.c b/drivers/xen/acpi.c
> > index d2ee605c5ca1..eab28cfe9939 100644
> > --- a/drivers/xen/acpi.c
> > +++ b/drivers/xen/acpi.c
> > @@ -89,11 +89,11 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev,
> >  						  int *trigger_out,
> >  						  int *polarity_out)
> >  {
> > -	int gsi;
> > +	u32 gsi;
> >  	u8 pin;
> >  	struct acpi_prt_entry *entry;
> >  	int trigger = ACPI_LEVEL_SENSITIVE;
> > -	int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ?
> > +	int ret, polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ?
> >  				      ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW;
> >  
> >  	if (!dev || !gsi_out || !trigger_out || !polarity_out)
> > @@ -105,17 +105,18 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev,
> >  
> >  	entry = acpi_pci_irq_lookup(dev, pin);
> >  	if (entry) {
> > +		ret = 0;
> >  		if (entry->link)
> > -			gsi = acpi_pci_link_allocate_irq(entry->link,
> > +			ret = acpi_pci_link_allocate_irq(entry->link,
> >  							 entry->index,
> >  							 &trigger, &polarity,
> > -							 NULL);
> > +							 NULL, &gsi);
> >  		else
> >  			gsi = entry->index;
> >  	} else
> > -		gsi = -1;
> > +		ret = -ENODEV;
> >  
> > -	if (gsi < 0)
> > +	if (ret < 0)
> >  		return -EINVAL;
> >  
> >  	*gsi_out = gsi;
> > diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> > index b14d165632e7..402b97d12138 100644
> > --- a/include/acpi/acpi_drivers.h
> > +++ b/include/acpi/acpi_drivers.h
> > @@ -51,7 +51,7 @@
> >  
> >  int acpi_irq_penalty_init(void);
> >  int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
> > -			       int *polarity, char **name);
> > +			       int *polarity, char **name, u32 *gsi);
> >  int acpi_pci_link_free_irq(acpi_handle handle);
> >  
> >  /* ACPI PCI Device Binding */
> > -- 
> > 2.50.1
> >
Re: [PATCH] ACPI: PCI: IRQ: Handle INTx GSIs as u32 values not int
Posted by Bjorn Helgaas 1 month ago
On Fri, Jan 02, 2026 at 12:24:55PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Dec 31, 2025 at 11:01:50AM -0600, Bjorn Helgaas wrote:
> > On Wed, Dec 31, 2025 at 10:26:15AM +0100, Lorenzo Pieralisi wrote:
> > > In ACPI Global System Interrupts (GSIs) are described using a 32-bit
> > > value.
> > > 
> > > ACPI/PCI legacy interrupts (INTx) parsing code treats GSIs as 'int', which
> > > poses issues if the GSI interrupt value is a 32-bit value with the MSB set
> > > (as required in some interrupt configurations - eg ARM64 GICv5 systems).
> > > 
> > > Fix ACPI/PCI legacy INTx parsing by converting variables representing
> > > GSIs from 'int' to 'u32' bringing the code in line with the ACPI
> > > specification.
> > 
> > Looks good to me.  Is there any symptom of what the issue looks like
> > that could be mentioned here?  Might also be useful in the subject,
> > which currently describes the C code without really saying why we want
> > to do this.
> 
> Thanks ! Happy New Year !

Happy New Year!

> acpi_pci_link_allocate_irq() would return a GSI with MSB set, that the
> logic in acpi_pci_irq_enable() would treat as a failure because that's
> a negative value.
> 
> After fixing that - passing a 32-bit value with MSB set to
> acpi_irq_get_penalty() causes an array acpi_isa_irq_penalty dereference
> with an an index that is way beyond the array size.

Ouch, out-of-bounds reference.  Best case, maybe a data page fault or
a "No IRQ available" message or just a suboptimal IRQ choice.