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
