Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 14 Nov 2018 16:50:17 +0200
From:   "mika.westerberg@linux.intel.com" <mika.westerberg@linux.intel.com>
To:     Lukas Wunner <lukas@wunner.de>
Cc:     Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Wangzhou (B)" <wangzhou1@hisilicon.com>,
        Linuxarm <linuxarm@huawei.com>
Subject: Re: Qemu Guest kernel 4.20-rc1 PCIe hotplug issue
Message-ID: <20181114145017.GQ2500@lahna.fi.intel.com>
References: <5FC3163CFD30C246ABAA99954A238FA8387DD344@FRAEML521-MBX.china.huawei.com>
 <20181113122522.GA2500@lahna.fi.intel.com>
 <5FC3163CFD30C246ABAA99954A238FA8387DF43F@FRAEML521-MBX.china.huawei.com>
 <20181113125910.GB2500@lahna.fi.intel.com>
 <5FC3163CFD30C246ABAA99954A238FA8387DF51F@FRAEML521-MBX.china.huawei.com>
 <20181113150749.GC2500@lahna.fi.intel.com>
 <5FC3163CFD30C246ABAA99954A238FA8387DF7B5@FRAEML521-MBX.china.huawei.com>
 <20181114095225.GN2500@lahna.fi.intel.com>
 <20181114133014.ge7cy2r2vrrtt6xx@wunner.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181114133014.ge7cy2r2vrrtt6xx@wunner.de>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Nov 14, 2018 at 02:30:14PM +0100, Lukas Wunner wrote:
> On Wed, Nov 14, 2018 at 11:52:25AM +0200, mika.westerberg@linux.intel.com wrote:
> > On Tue, Nov 13, 2018 at 03:57:47PM +0000, Shameerali Kolothum Thodi wrote:
> > > > The smb_mb() thing is not that clear (at least to me) because it is used
> > > > in two places in the driver and both seem to be making write to
> > > > ctrl->cmd_busy visible to other CPUs but I don't see where we deal with
> > > > the read part.
> > > > 
> > > > I may be missing something, though.
> > > 
> > > I think the read part is in wait_event_timeout() which evaluates the
> > > condition. The wake_up is called from the pciehp_isr().  Since the flag
> > > is being updated in both process level and interrupt handler context,
> > > smp_mb() is used. I think the same now applies to  ctrl->slot_ctrl now
> > > as this being used in process context and interrupt context as well.
> > 
> > Right, but that would require to use another read/general barrier in the
> > pciehp_isr() before we read the variable in case interrupt happens
> > immediately on another CPU (at least that's my understanding).
> 
> In pcie_do_write_cmd(), please just move the
> 
> 	ctrl->slot_ctrl = slot_ctrl;
> 
> above the call to pcie_capability_write_word().
> 
> AFAICS an explicit memory barrier isn't needed here because of the call to
> pcie_capability_write_word(), which "will [ordinarily] be guaranteed to be
> fully ordered and uncombined" (Documentation/memory-barriers.txt, section
> "KERNEL I/O BARRIER EFFECTS").
> 
> The memory barrier in pciehp_isr() is also bogus because the following
> wake_up() implies a memory barrier if a task was woken.  (And if none
> was woken, who cares.)
> 
> 
> > Since I'm
> > not too comfortable with all these barriers to be honest I would prefer
> > reading the slot control register directly in pciehp_isr() :-)
> 
> That is an approach I'd strongly object to:  While pciehp itself only
> signals very few interrupts (making an additional mmio read appear to
> be negligible), it may share its interrupt with other devices.  On my
> MacBookPro9,1, a hotplug port of the Thunderbolt controller shares
> its interrupt line with the Wifi card and SD card reader, and those
> may signal a huge number of interrupts.  On such a machine an additional
> mmio read per interrupt becomes a problem.

OK.

I just sent a patch moving ctrl->slot_ctrl assignment to happen before
pcie_capability_write_word().