Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Sun, 8 May 2022 14:31:16 +0200
From:   Lukas Wunner <lukas@wunner.de>
To:     Pavel Machek <pavel@denx.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Liguang Zhang <zhangliguang@linux.alibaba.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH 5.10 126/599] PCI: pciehp: Clear cmd_busy bit in polling
 mode
Message-ID: <20220508123116.GA27352@wunner.de>
References: <20220405070258.802373272@linuxfoundation.org>
 <20220405070302.589741179@linuxfoundation.org>
 <20220409081314.GA19452@amd>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220409081314.GA19452@amd>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Sat, Apr 09, 2022 at 10:13:15AM +0200, Pavel Machek wrote:
> > From: Liguang Zhang <zhangliguang@linux.alibaba.com>
> > 
> > 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.
> 
> > +++ 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?

The cmd_busy flag is set by pcie_do_write_cmd() before writing the
Slot Control register and it is then cleared by pciehp_isr().

The purpose of the memory barriers is to ensure that order.
IOW, we want to avoid a scenario where the write to cmd_busy in
pcie_do_write_cmd() hasn't been committed to memory yet, the Slot Control
write is performed, an interrupt occurs and is handled, the interrupt
handler writes cmd_busy = 0 and only then is the cmd_busy = 1 write
in pcie_do_write_cmd() committed to memory.

That said, you're right that such a scenario is impossible if
cmd_busy is cleared by the synchronous pcie_poll_cmd()
instead of the asynchronous pciehp_isr().

Care to submit a patch to remove the memory barrier in this single
location?

A WRITE_ONCE() (i.e. a mere compiler barrier instead of a proper
cacheline flush) is not sufficient to avoid the above scenario.
An atomic bitop would work, but wouldn't offer advantages compared
to the status quo.

Thanks,

Lukas