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

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

On 9/24/2022 1:32 AM, Lukas Wunner wrote:
> On Fri, Jul 08, 2022 at 10:35:15AM -0600, Jonathan Derrick wrote:
>> 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.
>>
>> 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?
> 
> Another report has cropped up of spurious DLLSC events:
> https://bugzilla.kernel.org/show_bug.cgi?id=216511
> 
> That other case differs from yours in that a spurious DLLSC event
> is seen on plugging *in* a card, whereas in your case the event
> seems to occur on *removing* a card.  In both cases, the spurious
> event is seen on the hotplug port's sibling.
> 
> I'm starting to think that we should probably disable DLLSC events
> entirely if they're known to be unreliable.  The hotplug port
> solely relies on PDC events then.  Otherwise we'd have to clutter
> the event handling with all sorts of special cases.  The code would
> become fairly difficult to follow.
I'm not sure we can do that either. Think of a non-logical interposer.
PDC will be static but DLLSC may be the only hotplug status received.


> 
> I've attached an experimental patch to the bug report which disables
> DLLSC events on a hotplug port if a P5608 SSD is plugged into it:
> https://bugzilla.kernel.org/attachment.cgi?id=301845
> 
> Would this approach work for you?
It looks correct to me


> 
> One other question:  What if the SSD is not bifurcated (i.e. x8
> instead of x4x4), don't we need avoid applying the quirk in that case?
> Your patch doesn't seem to do that.  Can we recognize somehow whether
> the card is bifurcated or not?  Is it sufficient to just look at the
> Maximum Link Width in the Link Capabilities Register?  Does the SSD
> report x4 there if it's bifurcate
Good question. Unique to the subsystem device id?

> 
> Thanks,
> 
> Lukas