On 9/14/21 9:46 AM, Lukas Wunner wrote:
> On Mon, Sep 13, 2021 at 04:07:22PM -0500, Jon Derrick wrote:
>> On 9/12/21 3:45 AM, Lukas Wunner wrote:
>>> 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.)
>>
>> It looks like 2 NVMe endpoints plugged into two different root ports, ex,
>> 80:00.0 Root port to [81-86]
>> 80:01.0 Root port to [87-8b]
>> 81:00.0 NVMe
>> 87:00.0 NVMe
>>
>> The x8 is bifurcated to x4x4. Physically they share the same slot
>> power/clock/reset but are logically separate per root port.
>
> So are these two P5608 drives attached to a single Root Port with an
> interposer in-between?
>
> I assume the Root Port needs to know that it's bifurcated and has to
> appear as two slots on the bus. Is this configured with a BIOS setting?
>
> If these assumptions are true, the quirk isn't really specific to
> the P5608 but should rather apply to the bifurcation-capable Root Port
> and the quirk should set the flag if the Root Port is indeed configured
> for bifurcation.
It's a function of the slot + card combination, but we can't distinguish this
slot's special power handling behavior from the vanilla behavior. It's modified
to handle power on the logically bifurcated, singular physical device.
>
>
>>>> @@ -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.)
>>
>> Agreed. I'll find a good spot for it.
>
> Adding it in the if-clause above should work. The if-clause is only
> entered when the sibling card has had its power removed, and this
> only happens once. When power is reinstated via sysfs, the device
> in the slot is reenumerated and pdev->shared_pcc_and_link_slot is
> set to true again if there's a quirked device in the slot.
>
> Thanks,
>
> Lukas
>
