Re: [PATCH v2] PCI: pciehp: Fix re-enabling the slot marked for safe removal

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Mar 12, 2019 at 03:05:48PM +0300, Sergey Miroshnichenko wrote:
> During the safe removal procedure, a Data Link Layer State Changed event
> may occur after pciehp_power_off_slot(), and it is handled when the slot is
> already set to OFF_STATE. This results in re-enabling the device and makes
> it impossible to actually safely remove it.
> 
> Clear out the Presence Detect Changed and Data Link Layer State Changed
> events when the disabled slot has settled down.
> 
> It is still possible to re-enable the device if it remains in the slot
> after pressing the Attention Button by pressing it again.
> 
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 3f3df4c29f6e..905282a8ddaa 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -115,6 +115,10 @@ static void remove_board(struct controller *ctrl, bool safe_removal)
>  		 * removed from the slot/adapter.
>  		 */
>  		msleep(1000);
> +
> +		/* Ignore link or presence changes caused by power off */
> +		atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),
> +			   &ctrl->pending_events);

I did apply this and I'm told that it fixes an issue where pressing an
NVMe drive power button does not turn off the drive.

But I wonder if it would be feasible to just turn off reporting of the
events we don't care about, as opposed to figuring out that we should
ignore them if they do occur.  E.g., maybe when we write
PCI_EXP_SLTCTL_PWR_OFF, we should clear PCI_EXP_SLTCTL_DLLSCE and
PCI_EXP_SLTCTL_PDCE at the same time.  Of course, then we'd have to clear
any pending events and re-enable them later.

It's somewhat non-obvious that we ignore these events by clearing out
bits in pending_events that might have been set somewhere else (I
assume by pciehp_isr()) and will be consumed in a third place (maybe
pciehp_ist()?)

Bjorn