linux-kernel - Re: [PATCH 5.10 126/599] PCI: pciehp: Clear cmd

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:   Tue, 19 Apr 2022 09:13:15 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Pavel Machek <pavel@...x.de>,
        Liguang Zhang <zhangliguang@...ux.alibaba.com>,
        Lukas Wunner <lukas@...ner.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH 5.10 126/599] PCI: pciehp: Clear cmd_busy bit in polling
 mode

On Sat, Apr 09, 2022 at 10:13:15AM +0200, Pavel Machek wrote:
> > From: Liguang Zhang <zhangliguang@...ux.alibaba.com>
> > 
> > commit 92912b175178c7e895f5e5e9f1e30ac30319162b upstream.
> > 
> > Writes to a Downstream Port's Slot Control register are PCIe hotplug
> > "commands."  If the Port supports Command Completed events, software must
> > wait for a command to complete before writing to Slot Control again.
> > 
> > pcie_do_write_cmd() sets ctrl->cmd_busy when it writes to Slot Control.  If
> > software notification is enabled, i.e., PCI_EXP_SLTCTL_HPIE and
> > PCI_EXP_SLTCTL_CCIE are set, ctrl->cmd_busy is cleared by pciehp_isr().
> > 
> > But when software notification is disabled, as it is when pcie_init()
> > powers off an empty slot, pcie_wait_cmd() uses pcie_poll_cmd() to poll for
> > command completion, and it neglects to clear ctrl->cmd_busy, which leads to
> > spurious timeouts:
> 
> I'm pretty sure this fixes the problem, but... it is still not fully
> correct.

Thanks, Pavel.  Liguang, Lukas, any comment?

> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -98,6 +98,8 @@ static int pcie_poll_cmd(struct controll
> >  		if (slot_status & PCI_EXP_SLTSTA_CC) {
> >  			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
> >  						   PCI_EXP_SLTSTA_CC);
> > +			ctrl->cmd_busy = 0;
> > +			smp_mb();
> >  			return 1;
> >  		}
> 
> Is the memory barrier neccessary? I don't see corresponding memory
> barrier for reading.
> 
> If it is neccessary, should we have WRITE_ONCE at the very least, or
> probably normal atomic operations?
> 
> Best regards,
> 								Pavel
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany