Re: [PATCH v3] PCI: pciehp: Add quirk to handle spurious DLLSC on a x4x4 SSD

[Lukas Wunner <lukas@wunner.de>]

On Mon, Aug 30, 2021 at 09:56:28AM -0600, Jon Derrick wrote:
> When an Intel P5608 SSD is bifurcated into x4x4 mode, and the upstream
> ports both support hotplugging on each respective x4 device, a slot
> management system for the SSD requires both x4 slots to have power
> removed via sysfs (echo 0 > slot/N/power), from the OS before it can
> safely turn-off physical power for the whole x8 device. The implications
> are that slot status will display powered off and link inactive statuses
> for the x4 devices where the devices are actually powered until both
> ports have powered off.

Just to get a better understanding, does the P5608 have an internal
PCIe switch with hotplug capability on the Downstream Ports or
does it plug into two separate PCIe slots?  I recall previous patches
mentioned a CEM interposer?  (An lspci listing might be helpful.)


> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -225,6 +225,7 @@ void pciehp_handle_disable_request(struct controller *ctrl)
>  void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  {
>  	int present, link_active;
> +	struct pci_dev *pdev = ctrl->pcie->port;

Nit: Reverse christmas tree.


> @@ -265,6 +266,12 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  		cancel_delayed_work(&ctrl->button_work);
>  		fallthrough;
>  	case OFF_STATE:
> +		if (pdev->shared_pcc_and_link_slot &&
> +		    (events & PCI_EXP_SLTSTA_DLLSC) && !link_active) {
> +			mutex_unlock(&ctrl->state_lock);
> +			break;
> +		}
> +

I think you also need to add...

			pdev->shared_pcc_and_link_slot = false;

... here to reset the shared_pcc_and_link_slot attribute in case the
next card plugged into the slot doesn't have the quirk.

(This can't be done in pciehp_unconfigure_device() because the attribute
is queried *after* the slot has been brought down.)


> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5750,3 +5750,37 @@ static void apex_pci_fixup_class(struct pci_dev *pdev)
>  }
>  DECLARE_PCI_FIXUP_CLASS_HEADER(0x1ac1, 0x089a,
>  			       PCI_CLASS_NOT_DEFINED, 8, apex_pci_fixup_class);
> +
> +#ifdef CONFIG_HOTPLUG_PCI_PCIE

It's possible to put the quirk at the bottom of pciehp_ctrl.c and
thus avoid the need for the #ifdef here.  (We've got another
pciehp-specific quirk at the bottom of pciehp_hpc.c.)

Otherwise LGTM.

Thanks,

Lukas