linux-kernel - Re: [PATCH v3] PCI: pciehp: Make sure pciehp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 9 Feb 2020 16:03:28 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     Stuart Hayes <stuart.w.hayes@...il.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Austin Bolen <austin_bolen@...l.com>,
        Keith Busch <kbusch@...nel.org>,
        Alexandru Gagniuc <mr.nuke.me@...il.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        "Gustavo A . R . Silva" <gustavo@...eddedor.com>,
        Sinan Kaya <okaya@...nel.org>,
        Oza Pawandeep <poza@...eaurora.org>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, narendra_k@...l.com
Subject: Re: [PATCH v3] PCI: pciehp: Make sure pciehp_isr clears interrupt
 events

On Fri, Feb 07, 2020 at 02:54:50PM -0500, Stuart Hayes wrote:
> +/*
> + * Set a limit to how many times the ISR will loop reading and writing the
> + * slot status register trying to clear the event bits.  These bits should
> + * not toggle rapidly, and there are only six possible events that could
> + * generate this interrupt.  If we still see events after this many reads,
> + * there is likely a bit stuck.
> + */
> +#define MAX_ISR_STATUS_READS 6

Actually only *three* possible events could generate this interrupt
because pcie_enable_notification() only enables DLLSC, CCIE and
either of ABP or PDC.


> -	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, events);
> +	if (status) {
> +		pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, status);

Writing "events" instead of "status" would seem to be more advantageous
because it reduces the number of loops.  Say you read PDC in the first
loop iteration, then DLLSC in the second loop iteration and shortly
before writing the register, PDC transitions to 1.  If you write
"events", you can make do with 2 loop iterations, if you write "status"
you'll need 3.


> +
> +		/*
> +		 * Unless the MSI happens to be masked, all of the event
> +		 * bits must be zero before the port will send a new
> +		 * interrupt (see PCI Express Base Specification Rev 5.0
> +		 * Version 1.0, section 6.7.3.4, "Software Notification of
> +		 * Hot-Plug Events"). So, if an event bit gets set between
> +		 * the read and the write of PCI_EXP_SLTSTA, we need to
> +		 * loop back and try again.
> +		 */
> +		if (status_reads++ < MAX_ISR_STATUS_READS)
> +			goto read_status;

Please use "pci_dev_msi_enabled(pdev)" as conditional for the if-clause,
we don't need this with INTx.


Using a for (;;) or do/while loop that you jump out of if
(!status || !pci_dev_msi_enabled(pdev)) might be more readable
than a goto, but I'm not sure.

Thanks,

Lukas