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

[Jon Derrick <jonathan.derrick@intel.com>]

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
>