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@xxxxxxxxxxxxxxx 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().
