Re: [PATCH v2] PCI: pciehp: Quirk to ignore spurious DLLSC when off

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

On 8/23/21 6:41 PM, Raj, Ashok wrote:
> On Mon, Aug 23, 2021 at 12:49:19PM -0600, Derrick, Jonathan wrote:
>> From: James Puthukattukaran <james.puthukattukaran@oracle.com>
>>
>> When a specific x8 CEM card is bifurcated into x4x4 mode, and the
>> upstream ports both support hotplugging on each respective x4 device, a
>> slot management system for the CEM card requires both x4 devices to be
>> sysfs removed from the OS before it can safely turn-off physical power.
> 
> sysfs removed from the OS seems a bit confusing.. Do you mean removed via
> sysfs by echo 0 > power?
Yes it seems to be related to slot power and not just virtual removal


> 
>> The implications are that Slot Control will display Powered Off status
>> for the device where the device is actually powered until both ports
>> have powered off.
>>
>> When power is removed from the first half, real power and link remains
>> active while waiting for the second half to have power removed. When
>> power is then removed from the second half, the first half starts
>> shutdown sequence and will trigger a DLLSC event. This is misinterpreted
>> as an enabling event and causes the first half to be re-enabled.
>>
>> The spurious enable can be resolved by ignoring link status change
>> events when no link is active when in the off state. This patch adds a
>> quirk for the card.
>>
>> Acked-by: Jon Derrick <jonathan.derrick@intel.com>
>> Signed-off-by: James Puthukattukaran <james.puthukattukaran@oracle.com>
>> ---
>> v1->v2: Device-specific quirk
>>
>>  drivers/pci/hotplug/pciehp_ctrl.c |  7 +++++++
>>  drivers/pci/quirks.c              | 30 ++++++++++++++++++++++++++++++
>>  include/linux/pci.h               |  1 +
>>  3 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
>> index 529c34808440..db41f78bfac8 100644
>> --- 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;
>>  
>>  	/*
>>  	 * If the slot is on and presence or link has changed, turn it off.
>> @@ -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) {
> 
> Is it !link_active? Based on the explanation, until we turn off both slots
> you would see link is still active correct? 
My understanding is the internal link is active but link_active shows negative
status while the one side is faking being 'powered off'.

Then if a DLLSC occurs while the slot is already OFF_STATE and !link_active, I
believe we can assume it was previously !link_active as well and it's an event
from the other side.


> 
>> +			mutex_unlock(&ctrl->state_lock);
>> +			break;
>> +		}
>> +