From: Bjorn Helgaas <helgaas@kernel.org>
To: Clayton Casciato <majortomtosourcecontrol@gmail.com>
Cc: bhelgaas@google.com, rjw@rjwysocki.net, lenb@kernel.org,
linux-pci@vger.kernel.org
Subject: Re: [PATCH] acpi: pci_irq: Fixed a control flow style issue
Date: Wed, 16 Jun 2021 15:56:18 -0500 [thread overview]
Message-ID: <20210616205618.GA3002486@bjorn-Precision-5520> (raw)
In-Reply-To: <20210612195730.1069667-1-majortomtosourcecontrol@gmail.com>
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
>
prev parent reply other threads:[~2021-06-16 20:56 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-12 19:57 [PATCH] acpi: pci_irq: Fixed a control flow style issue Clayton Casciato
2021-06-16 20:56 ` Bjorn Helgaas [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210616205618.GA3002486@bjorn-Precision-5520 \
--to=helgaas@kernel.org \
--cc=bhelgaas@google.com \
--cc=lenb@kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=majortomtosourcecontrol@gmail.com \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.