Re: Regression (sort of): PCI/portdrv: Turn off PCIe services during shutdown

[okaya@codeaurora.org]

On 2018-01-13 02:32, Lukas Wunner wrote:
> On Fri, Jan 12, 2018 at 10:31:12AM -0500, Sinan Kaya wrote:
>> On 1/12/2018 10:12 AM, Lukas Wunner wrote:
>> > On Fri, Jan 12, 2018 at 09:26:48AM -0500, Sinan Kaya wrote:
>> >> On 1/12/2018 5:49 AM, Lukas Wunner wrote:
>> >> I wonder if we can separate remove from shutdown and just disable the IRQs
>> >> in shutdown case rather than turning off the slot power etc.
>> >
>> > But don't we risk "IRQ xx: nobody cared" splats if we do that?
>> 
>> I assumed code was turning off the slot power etc. aggressively.
>> 
>> After looking at the code some more time, it seems to be doing the
>> right thing and telling pcie controller not to generate interrupts for
>> hotplug.
>> 
>> I think this is what is failing for you probably because by the time 
>> you are
>> shutting down there is nobody to issue the command completion. This 
>> would
>> repeat for each hotplug capable pcie slot.
> 
> You mean we disable Command Completed interrupts and thus the port
> can't notify that the command was completed?  It seems the code
> accommodates to that by polling the PCI_EXP_SLTSTA_CC bit:
> 
> 	if (ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE &&
> 	    ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE)
> 		rc = wait_event_timeout(ctrl->queue, !ctrl->cmd_busy, timeout);
> 	else
> 		rc = pcie_poll_cmd(ctrl, jiffies_to_msecs(timeout));
> 
> The problem is that these Thunderbolt controllers never seem to set
> the PCI_EXP_SLTSTA_CC bit, resulting in "Timeout on hotplug command"
> messages.


I waa thinking of using nowait variant of write function in notification 
disable function in order to not introduce new behavior for existing 
silicon.


> 
> The pciehp code is okay, we just need a workaround for the broken
> Thunderbolt 1 chips.  This has been a pain point all along, but
> your patch made the brokenness visible enough that investigating
> and fixing it became unavoidable.  So don't worry about your patch,
> it's all fine. ;-)
> 
> Thanks,
> 
> Lukas