[PATCH] acpi: pci_irq: Fixed a control flow style issue

[PATCH] acpi: pci_irq: Fixed a control flow style issue

[Clayton Casciato]

Fixed coding style issue.

Signed-off-by: Clayton Casciato <majortomtosourcecontrol@gmail.com>
---
 drivers/acpi/pci_irq.c | 45 +++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 08e15774fb9f..6eea3cf7b158 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -260,30 +260,29 @@ static int bridge_has_boot_interrupt_variant(struct pci_bus *bus)
 static int acpi_reroute_boot_interrupt(struct pci_dev *dev,
 				       struct acpi_prt_entry *entry)
 {
-	if (noioapicquirk || noioapicreroute) {
+	if (noioapicquirk || noioapicreroute)
 		return 0;
-	} else {
-		switch (bridge_has_boot_interrupt_variant(dev->bus)) {
-		case 0:
-			/* no rerouting necessary */
-			return 0;
-		case INTEL_IRQ_REROUTE_VARIANT:
-			/*
-			 * Remap according to INTx routing table in 6700PXH
-			 * specs, intel order number 302628-002, section
-			 * 2.15.2. Other chipsets (80332, ...) have the same
-			 * mapping and are handled here as well.
-			 */
-			dev_info(&dev->dev, "PCI IRQ %d -> rerouted to legacy "
-				 "IRQ %d\n", entry->index,
-				 (entry->index % 4) + 16);
-			entry->index = (entry->index % 4) + 16;
-			return 1;
-		default:
-			dev_warn(&dev->dev, "Cannot reroute IRQ %d to legacy "
-				 "IRQ: unknown mapping\n", entry->index);
-			return -1;
-		}
+
+	switch (bridge_has_boot_interrupt_variant(dev->bus)) {
+	case 0:
+		/* no rerouting necessary */
+		return 0;
+	case INTEL_IRQ_REROUTE_VARIANT:
+		/*
+		 * Remap according to INTx routing table in 6700PXH
+		 * specs, intel order number 302628-002, section
+		 * 2.15.2. Other chipsets (80332, ...) have the same
+		 * mapping and are handled here as well.
+		 */
+		dev_info(&dev->dev, "PCI IRQ %d -> rerouted to legacy "
+			 "IRQ %d\n", entry->index,
+			 (entry->index % 4) + 16);
+		entry->index = (entry->index % 4) + 16;
+		return 1;
+	default:
+		dev_warn(&dev->dev, "Cannot reroute IRQ %d to legacy "
+			 "IRQ: unknown mapping\n", entry->index);
+		return -1;
 	}
 }
 #endif /* CONFIG_X86_IO_APIC */
-- 
2.31.1

Re: [PATCH] acpi: pci_irq: Fixed a control flow style issue

[Bjorn Helgaas]

On Sat, Jun 12, 2021 at 01:57:31PM -0600, Clayton Casciato wrote:
> Fixed coding style issue.

I don't object to this, and in fact, I prefer the style you propose.
I don't know whether Rafael thinks it's worth the churn since it
doesn't actually fix anything.

But do take note of the commit log conventions.  Run

  $ git log --oneline drivers/acpi/pci_irq.c

and match the style.  Probably something like this:

  ACPI: PCI: Simplify acpi_reroute_boot_interrupt() coding style

And note that the commit log should use imperative mood and be a
little more specific about what the commit does.  [1] has several
great hints.

More below.

> Signed-off-by: Clayton Casciato <majortomtosourcecontrol@gmail.com>
> ---
>  drivers/acpi/pci_irq.c | 45 +++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> index 08e15774fb9f..6eea3cf7b158 100644
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -260,30 +260,29 @@ static int bridge_has_boot_interrupt_variant(struct pci_bus *bus)
>  static int acpi_reroute_boot_interrupt(struct pci_dev *dev,
>  				       struct acpi_prt_entry *entry)
>  {
> -	if (noioapicquirk || noioapicreroute) {
> +	if (noioapicquirk || noioapicreroute)
>  		return 0;
> -	} else {
> -		switch (bridge_has_boot_interrupt_variant(dev->bus)) {
> -		case 0:
> -			/* no rerouting necessary */
> -			return 0;
> -		case INTEL_IRQ_REROUTE_VARIANT:
> -			/*
> -			 * Remap according to INTx routing table in 6700PXH
> -			 * specs, intel order number 302628-002, section
> -			 * 2.15.2. Other chipsets (80332, ...) have the same
> -			 * mapping and are handled here as well.
> -			 */
> -			dev_info(&dev->dev, "PCI IRQ %d -> rerouted to legacy "
> -				 "IRQ %d\n", entry->index,
> -				 (entry->index % 4) + 16);
> -			entry->index = (entry->index % 4) + 16;
> -			return 1;
> -		default:
> -			dev_warn(&dev->dev, "Cannot reroute IRQ %d to legacy "
> -				 "IRQ: unknown mapping\n", entry->index);
> -			return -1;
> -		}
> +
> +	switch (bridge_has_boot_interrupt_variant(dev->bus)) {
> +	case 0:
> +		/* no rerouting necessary */
> +		return 0;
> +	case INTEL_IRQ_REROUTE_VARIANT:
> +		/*
> +		 * Remap according to INTx routing table in 6700PXH
> +		 * specs, intel order number 302628-002, section
> +		 * 2.15.2. Other chipsets (80332, ...) have the same
> +		 * mapping and are handled here as well.
> +		 */
> +		dev_info(&dev->dev, "PCI IRQ %d -> rerouted to legacy "
> +			 "IRQ %d\n", entry->index,
> +			 (entry->index % 4) + 16);
> +		entry->index = (entry->index % 4) + 16;
> +		return 1;
> +	default:
> +		dev_warn(&dev->dev, "Cannot reroute IRQ %d to legacy "
> +			 "IRQ: unknown mapping\n", entry->index);
> +		return -1;

Looking at this a little closer, this was added by e1d3a90846b4 ("pci,
acpi: reroute PCI interrupt to legacy boot interrupt equivalent") [2].

It looks like it might be a little over-engineered.  It added
MAX_IRQ_REROUTE_VARIANTS, which is an indication that
irq_reroute_variant is a 2-bit bitfield.  But in the last 13 years
we've never needed more than the single INTEL_IRQ_REROUTE_VARIANT bit.

So I would consider reworking it like this:

  unsigned int intel_irq_reroute:1;

  static void acpi_reroute_boot_interrupt(...)
  {
    if (noioapicquirk || noioapicreroute)
      return;

    if (bridge_has_intel_irq_reroute(dev->bus)) {
      dev_info(..., "PCI IRQ %d -> rerouted ...");
      entry->index = (entry->index % 4) + 16;
    }
  }

[1] https://chris.beams.io/posts/git-commit/
[2] https://git.kernel.org/linus/e1d3a90846b4

>  	}
>  }
>  #endif /* CONFIG_X86_IO_APIC */
> -- 
> 2.31.1
>