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

[Jonathan Derrick <jonathan.derrick@linux.dev>]

On 9/20/2021 11:18 AM, Jon Derrick wrote:
> 
> 
> 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
>>
> 

Hi Bjorn, Lukas,

I need to resubmit this.

Besides the 'pdev->shared_pcc_and_link_slot = false', addition mentioned
above, is there anything else that should be changed or any reason this
wouldn't be accepted?