[PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[Ashok Raj]

When Mechanical Retention Lock (MRL) is present, Linux doesn't process
those change events.

The following changes need to be enabled when MRL is present.

1. Subscribe to MRL change events in SlotControl.
2. When MRL is closed,
   - If there is no ATTN button, then POWER on the slot.
   - If there is ATTN button, and an MRL event pending, ignore
     Presence Detect. Since we want ATTN button to drive the
     hotplug event.


Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Co-developed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/hotplug/pciehp.h      |  1 +
 drivers/pci/hotplug/pciehp_ctrl.c | 69 +++++++++++++++++++++++++++++++++++++++
 drivers/pci/hotplug/pciehp_hpc.c  | 27 ++++++++++++++-
 3 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 4fd200d8b0a9..24a1c9c8ac78 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -155,6 +155,7 @@ void pciehp_request(struct controller *ctrl, int action);
 void pciehp_handle_button_press(struct controller *ctrl);
 void pciehp_handle_disable_request(struct controller *ctrl);
 void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events);
+void pciehp_handle_mrl_change(struct controller *ctrl);
 int pciehp_configure_device(struct controller *ctrl);
 void pciehp_unconfigure_device(struct controller *ctrl, bool presence);
 void pciehp_queue_pushbutton_work(struct work_struct *work);
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 9f85815b4f53..c4310ee3678b 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -227,6 +227,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;
+	u8 getstatus = 0;
 
 	/*
 	 * If the slot is on and presence or link has changed, turn it off.
@@ -275,6 +276,16 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 		if (link_active)
 			ctrl_info(ctrl, "Slot(%s): Link Up\n",
 				  slot_name(ctrl));
+		if (MRL_SENS(ctrl)) {
+			pciehp_get_latch_status(ctrl, &getstatus);
+			/*
+			 * If slot is closed && ATTN button exists
+			 * don't continue, let the ATTN button
+			 * drive the hot-plug
+			 */
+			if (!getstatus && ATTN_BUTTN(ctrl))
+				return;
+		}
 		ctrl->request_result = pciehp_enable_slot(ctrl);
 		break;
 	default:
@@ -283,6 +294,64 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	}
 }
 
+void pciehp_handle_mrl_change(struct controller *ctrl)
+{
+	u8 getstatus = 0;
+	int present, link_active;
+
+	pciehp_get_latch_status(ctrl, &getstatus);
+
+	present = pciehp_card_present(ctrl);
+	link_active = pciehp_check_link_active(ctrl);
+
+	ctrl_info(ctrl, "Slot(%s): Card %spresent\n",
+		  slot_name(ctrl), present ? "" : "not ");
+
+	ctrl_info(ctrl, "Slot(%s): Link %s\n",
+		  slot_name(ctrl), link_active ? "Up" : "Down");
+
+	ctrl_info(ctrl, "Slot(%s): Latch %s\n",
+			  slot_name(ctrl), getstatus ? "Open" : "Closed");
+
+	/*
+	 * Need to handle only MRL Open. When MRL is closed with
+	 * a Card Present, either the ATTN button, or the PDC indication
+	 * should power the slot and add the card in the slot
+	 */
+	if (getstatus) {
+		/*
+		 * If slot was powered on, time to power off
+		 * and remove the card
+		 */
+		mutex_lock(&ctrl->state_lock);
+		if (ctrl->state == ON_STATE) {
+			mutex_unlock(&ctrl->state_lock);
+			pciehp_handle_disable_request(ctrl);
+		} else
+			mutex_unlock(&ctrl->state_lock);
+	} else {
+		/*
+		 * If latch is closed, and previous state is OFF
+		 * Then enable the slot
+		 */
+		mutex_lock(&ctrl->state_lock);
+		if (ctrl->state == OFF_STATE) {
+			/*
+			 * Only continue to power on the slot when the
+			 * Attention button is not present. When button
+			 * present, button press event will process the
+			 * hot-add part of the flow.
+			 */
+			if ((present || link_active) && !ATTN_BUTTN(ctrl)) {
+				ctrl->state = POWERON_STATE;
+				mutex_unlock(&ctrl->state_lock);
+				ctrl->request_result = pciehp_enable_slot(ctrl);
+			} else
+				mutex_unlock(&ctrl->state_lock);
+		}
+	}
+}
+
 static int __pciehp_enable_slot(struct controller *ctrl)
 {
 	u8 getstatus = 0;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 53433b37e181..5a4b5cfbfe48 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -605,7 +605,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	 */
 	status &= PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
 		  PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_CC |
-		  PCI_EXP_SLTSTA_DLLSC;
+		  PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_MRLSC;
 
 	/*
 	 * If we've already reported a power fault, don't report it again
@@ -658,6 +658,12 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 		return IRQ_HANDLED;
 	}
 
+	/*
+	 * If MRL is triggered, if ATTN button exists, ignore the event.
+	 */
+	if (ATTN_BUTTN(ctrl) && (events & PCI_EXP_SLTSTA_MRLSC))
+		events &= ~PCI_EXP_SLTSTA_PDC;
+
 	/* Save pending events for consumption by IRQ thread. */
 	atomic_or(events, &ctrl->pending_events);
 	return IRQ_WAKE_THREAD;
@@ -688,6 +694,13 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
 		goto out;
 	}
 
+	/*
+	 * If ATTN is present and MRL is triggered
+	 * ignore the Presence Change Event.
+	 */
+	if (ATTN_BUTTN(ctrl) && (events & PCI_EXP_SLTSTA_MRLSC))
+		events &= ~PCI_EXP_SLTSTA_PDC;
+
 	/* Check Attention Button Pressed */
 	if (events & PCI_EXP_SLTSTA_ABP) {
 		ctrl_info(ctrl, "Slot(%s): Attention button pressed\n",
@@ -712,6 +725,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
 		pciehp_handle_disable_request(ctrl);
 	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
 		pciehp_handle_presence_or_link_change(ctrl, events);
+
+	if (events & PCI_EXP_SLTSTA_MRLSC)
+		pciehp_handle_mrl_change(ctrl);
+
 	up_read(&ctrl->reset_lock);
 
 	ret = IRQ_HANDLED;
@@ -768,6 +785,14 @@ static void pcie_enable_notification(struct controller *ctrl)
 		cmd |= PCI_EXP_SLTCTL_ABPE;
 	else
 		cmd |= PCI_EXP_SLTCTL_PDCE;
+
+	/*
+	 * If MRL sensor is present, then subscribe for MRL
+	 * Changes to be notified as well.
+	 */
+	if (MRL_SENS(ctrl))
+		cmd |= PCI_EXP_SLTCTL_MRLSCE;
+
 	if (!pciehp_poll_mode)
 		cmd |= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE;
 
-- 
2.7.4

Re: [PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[Raj, Ashok]

Hi Bjorn


On Fri, Sep 25, 2020 at 04:01:38PM -0700, Ashok Raj wrote:
> When Mechanical Retention Lock (MRL) is present, Linux doesn't process
> those change events.
> 
> The following changes need to be enabled when MRL is present.
> 
> 1. Subscribe to MRL change events in SlotControl.
> 2. When MRL is closed,
>    - If there is no ATTN button, then POWER on the slot.
>    - If there is ATTN button, and an MRL event pending, ignore
>      Presence Detect. Since we want ATTN button to drive the
>      hotplug event.
> 

Did you get a chance to review this? Thought i'll just check with you. 

Seems like there was a lot happening in the error handling and hotplug
side, I thought you might have wanted to wait for the dust to settle :-)

> 
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Co-developed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/hotplug/pciehp.h      |  1 +
>  drivers/pci/hotplug/pciehp_ctrl.c | 69 +++++++++++++++++++++++++++++++++++++++
>  drivers/pci/hotplug/pciehp_hpc.c  | 27 ++++++++++++++-
>  3 files changed, 96 insertions(+), 1 deletion(-)
> 

Re: [PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[Lukas Wunner]

Hi Ashok,

my sincere apologies for the delay.

On Fri, Sep 25, 2020 at 04:01:38PM -0700, Ashok Raj wrote:
> When Mechanical Retention Lock (MRL) is present, Linux doesn't process
> those change events.
> 
> The following changes need to be enabled when MRL is present.
> 
> 1. Subscribe to MRL change events in SlotControl.
> 2. When MRL is closed,
>    - If there is no ATTN button, then POWER on the slot.
>    - If there is ATTN button, and an MRL event pending, ignore
>      Presence Detect. Since we want ATTN button to drive the
>      hotplug event.

So I understand you have a hardware platform which has both MRL and
Attention Button on its hotplug slots?  It may be useful to name the
hardware platform in the commit message.

If an Attention Button is present, the current behavior is to bring up
the hotplug slot as soon as presence or link is detected.  We don't wait
for a button press.  This is intended as a convience feature to bring up
slots as quickly as possible, but the behavior can be changed if the
button press and 5 second delay is preferred.

In any case the behavior in response to an Attention Button press should
be the same regardless whether an MRL is present or not.  (The spec
doesn't seem to say otherwise.)


> +		if (MRL_SENS(ctrl)) {
> +			pciehp_get_latch_status(ctrl, &getstatus);
> +			/*
> +			 * If slot is closed && ATTN button exists
> +			 * don't continue, let the ATTN button
> +			 * drive the hot-plug
> +			 */
> +			if (!getstatus && ATTN_BUTTN(ctrl))
> +				return;
> +		}

For the sake of readability I'd suggest adding a pciehp_latch_closed()
helper similar to pciehp_card_present_or_link_active() which returns
true if no MRL is present (i.e. !MRL_SENS(ctrl)), otherwise retrieves
and returns the status with pciehp_get_latch_status().


> +void pciehp_handle_mrl_change(struct controller *ctrl)
> +{
> +	u8 getstatus = 0;
> +	int present, link_active;
> +
> +	pciehp_get_latch_status(ctrl, &getstatus);
> +
> +	present = pciehp_card_present(ctrl);
> +	link_active = pciehp_check_link_active(ctrl);
> +
> +	ctrl_info(ctrl, "Slot(%s): Card %spresent\n",
> +		  slot_name(ctrl), present ? "" : "not ");
> +
> +	ctrl_info(ctrl, "Slot(%s): Link %s\n",
> +		  slot_name(ctrl), link_active ? "Up" : "Down");

This duplicates a lot of code from pciehp_handle_presence_or_link_change(),
which I think suggests handling MRL events in that function.  After all,
an MRL event may lead to bringup or bringdown of a slot similar to
a link or presence event.

Basically pciehp_handle_presence_or_link_change() just needs to be
amended to bring down the slot if an MRL event occurred, and
afterwards only bring up the slot if MRL is closed.  Like this:

-	if (present <= 0 && link_active <= 0) {
+	if ((present <= 0 && link_active <= 0) || !latch_closed) {
		mutex_unlock(&ctrl->state_lock);
		return;
	}


> +	/*
> +	 * Need to handle only MRL Open. When MRL is closed with
> +	 * a Card Present, either the ATTN button, or the PDC indication
> +	 * should power the slot and add the card in the slot
> +	 */

I agree with the second sentence.  You may want to refer to PCIe Base Spec
r4.0, section 6.7.1.3 either in the commit message or a code comment,
which says:

"If an MRL Sensor is implemented without a corresponding MRL Sensor input
on the Hot-Plug Controller, it is recommended that the MRL Sensor be
routed to power fault input of the Hot-Plug Controller.
This allows an active adapter to be powered off when the MRL is opened."

This seems to suggest that the slot should be brought down as soon as MRL
is opened.


> +	/*
> +	 * If MRL is triggered, if ATTN button exists, ignore the event.
> +	 */
> +	if (ATTN_BUTTN(ctrl) && (events & PCI_EXP_SLTSTA_MRLSC))
> +		events &= ~PCI_EXP_SLTSTA_PDC;

Hm, the spec doesn't seem to imply a connection between presence of
an MRL and presence of an Attention Button, so I find this confusing.


> +	/*
> +	 * If ATTN is present and MRL is triggered
> +	 * ignore the Presence Change Event.
> +	 */
> +	if (ATTN_BUTTN(ctrl) && (events & PCI_EXP_SLTSTA_MRLSC))
> +		events &= ~PCI_EXP_SLTSTA_PDC;

An Attention Button press results in a synthesized PDC event after
5 seconds, which may get lost due to the above if-statement.

Thanks,

Lukas

Re: [PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[Raj, Ashok]

On Thu, Nov 19, 2020 at 08:51:20AM +0100, Lukas Wunner wrote:
> Hi Ashok,
> 
> my sincere apologies for the delay.

No worries.. 

> 
> On Fri, Sep 25, 2020 at 04:01:38PM -0700, Ashok Raj wrote:
> > When Mechanical Retention Lock (MRL) is present, Linux doesn't process
> > those change events.
> > 
> > The following changes need to be enabled when MRL is present.
> > 
> > 1. Subscribe to MRL change events in SlotControl.
> > 2. When MRL is closed,
> >    - If there is no ATTN button, then POWER on the slot.
> >    - If there is ATTN button, and an MRL event pending, ignore
> >      Presence Detect. Since we want ATTN button to drive the
> >      hotplug event.
> 
> So I understand you have a hardware platform which has both MRL and
> Attention Button on its hotplug slots?  It may be useful to name the
> hardware platform in the commit message.

I should, let me find out the exact intercept and include it next.

> 
> If an Attention Button is present, the current behavior is to bring up
> the hotplug slot as soon as presence or link is detected.  We don't wait
> for a button press.  This is intended as a convience feature to bring up
> slots as quickly as possible, but the behavior can be changed if the
> button press and 5 second delay is preferred.

No personal preference, I thought that is how the code in Linux was
earlier.

Looks like we still don't subscribe to PDC if ATTN is present? So you don't
get an event until someone presses the button to process hotplug right?

pciehp_hpc.c:pcie_enable_notification()
{
....

         * Always enable link events: thus link-up and link-down shall
         * always be treated as hotplug and unplug respectively. Enable
         * presence detect only if Attention Button is not present.
         */ 
        cmd = PCI_EXP_SLTCTL_DLLSCE;
        if (ATTN_BUTTN(ctrl))
                cmd |= PCI_EXP_SLTCTL_ABPE;
        else
                cmd |= PCI_EXP_SLTCTL_PDCE;
....
}


Looks like we still wait for button press to process. When you don't have a
power control yes the DLLSC would kick in and we would process them. but if
you have power control, you won't turn on until the button press?

> 
> In any case the behavior in response to an Attention Button press should
> be the same regardless whether an MRL is present or not.  (The spec
> doesn't seem to say otherwise.)

True, Looks like that is a Linux behavior, certainly not a spec
recommendation. Our validation teams tell Windows also behaves such. i.e
when ATTN exists button press drivers the hot-add. Linux doesn't need to
follow suite though. 

In that case do we need to subscribe to PDC even when ATTN is present?

> 
> 
> > +		if (MRL_SENS(ctrl)) {
> > +			pciehp_get_latch_status(ctrl, &getstatus);
> > +			/*
> > +			 * If slot is closed && ATTN button exists
> > +			 * don't continue, let the ATTN button
> > +			 * drive the hot-plug
> > +			 */
> > +			if (!getstatus && ATTN_BUTTN(ctrl))
> > +				return;
> > +		}
> 
> For the sake of readability I'd suggest adding a pciehp_latch_closed()
> helper similar to pciehp_card_present_or_link_active() which returns
> true if no MRL is present (i.e. !MRL_SENS(ctrl)), otherwise retrieves
> and returns the status with pciehp_get_latch_status().

Makes sense.. I can add that.

> 
> 
> > +void pciehp_handle_mrl_change(struct controller *ctrl)
> > +{
> > +	u8 getstatus = 0;
> > +	int present, link_active;
> > +
> > +	pciehp_get_latch_status(ctrl, &getstatus);
> > +
> > +	present = pciehp_card_present(ctrl);
> > +	link_active = pciehp_check_link_active(ctrl);
> > +
> > +	ctrl_info(ctrl, "Slot(%s): Card %spresent\n",
> > +		  slot_name(ctrl), present ? "" : "not ");
> > +
> > +	ctrl_info(ctrl, "Slot(%s): Link %s\n",
> > +		  slot_name(ctrl), link_active ? "Up" : "Down");
> 
> This duplicates a lot of code from pciehp_handle_presence_or_link_change(),
> which I think suggests handling MRL events in that function.  After all,
> an MRL event may lead to bringup or bringdown of a slot similar to
> a link or presence event.
> 
> Basically pciehp_handle_presence_or_link_change() just needs to be
> amended to bring down the slot if an MRL event occurred, and
> afterwards only bring up the slot if MRL is closed.  Like this:
> 
> -	if (present <= 0 && link_active <= 0) {
> +	if ((present <= 0 && link_active <= 0) || !latch_closed) {

Certainly looks like good simplication. I'll change and have a test run.

> 		mutex_unlock(&ctrl->state_lock);
> 		return;
> 	}
> 
> 
> > +	/*
> > +	 * Need to handle only MRL Open. When MRL is closed with
> > +	 * a Card Present, either the ATTN button, or the PDC indication
> > +	 * should power the slot and add the card in the slot
> > +	 */
> 
> I agree with the second sentence.  You may want to refer to PCIe Base Spec
> r4.0, section 6.7.1.3 either in the commit message or a code comment,
> which says:
> 
> "If an MRL Sensor is implemented without a corresponding MRL Sensor input
> on the Hot-Plug Controller, it is recommended that the MRL Sensor be
> routed to power fault input of the Hot-Plug Controller.
> This allows an active adapter to be powered off when the MRL is opened."
> 
> This seems to suggest that the slot should be brought down as soon as MRL
> is opened.

Good for commit log. I'll add a pointer in code comments too. Rarely people
go to commit log :-)

> 
> 
> > +	/*
> > +	 * If MRL is triggered, if ATTN button exists, ignore the event.
> > +	 */
> > +	if (ATTN_BUTTN(ctrl) && (events & PCI_EXP_SLTSTA_MRLSC))
> > +		events &= ~PCI_EXP_SLTSTA_PDC;
> 
> Hm, the spec doesn't seem to imply a connection between presence of
> an MRL and presence of an Attention Button, so I find this confusing.

This isn't spec required. But to enforce the earlier behavior that only
ATTN drives the hot-add event. Sometimes you close the MRL, even if you
didn't subscribe for notification of PDC, the bit is set and
un-intentionally acted upon. If the event wasn't subscribed for to start
with PDC getting pulled in is a side effect of the MRL. 

Validation folks look for consistency :-).. so this was mostly to enforce
that.

> 
> 
> > +	/*
> > +	 * If ATTN is present and MRL is triggered
> > +	 * ignore the Presence Change Event.
> > +	 */
> > +	if (ATTN_BUTTN(ctrl) && (events & PCI_EXP_SLTSTA_MRLSC))
> > +		events &= ~PCI_EXP_SLTSTA_PDC;
> 
> An Attention Button press results in a synthesized PDC event after
> 5 seconds, which may get lost due to the above if-statement.

When its synthesized you don't get the MRLSC? So we won't nuke the PDC then
correct?

Cheers,
Ashok

Re: [PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[Bjorn Helgaas]

Hi Ashok,

When you update this patch, can you run "git log --oneline
drivers/pci/hotplug/pciehp*" and make yours match?  It is a severe
character defect of mine, but seeing subject lines that are obviously
different than the convention is a disincentive to look at the patch.

On Thu, Nov 19, 2020 at 02:08:07PM -0800, Raj, Ashok wrote:
> On Thu, Nov 19, 2020 at 08:51:20AM +0100, Lukas Wunner wrote:
> > Hi Ashok,
> > 
> > my sincere apologies for the delay.
> 
> No worries.. 
> 
> > 
> > On Fri, Sep 25, 2020 at 04:01:38PM -0700, Ashok Raj wrote:
> > > When Mechanical Retention Lock (MRL) is present, Linux doesn't process
> > > those change events.
> > > 
> > > The following changes need to be enabled when MRL is present.
> > > 
> > > 1. Subscribe to MRL change events in SlotControl.
> > > 2. When MRL is closed,
> > >    - If there is no ATTN button, then POWER on the slot.
> > >    - If there is ATTN button, and an MRL event pending, ignore
> > >      Presence Detect. Since we want ATTN button to drive the
> > >      hotplug event.

Re: [PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[Raj, Ashok]

On Thu, Nov 19, 2020 at 04:27:41PM -0600, Bjorn Helgaas wrote:
> Hi Ashok,
> 
> When you update this patch, can you run "git log --oneline
> drivers/pci/hotplug/pciehp*" and make yours match?  It is a severe
> character defect of mine, but seeing subject lines that are obviously

:-)

> different than the convention is a disincentive to look at the patch.
> 

Thanks for the reminder. forgot the cap the PCI, and some inadvertant '.'
at the end of the line...

Will fix when i resend. 

Re: [PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[Kuppuswamy, Sathyanarayanan]

Hi,

On 11/19/20 2:08 PM, Raj, Ashok wrote:
>> If an Attention Button is present, the current behavior is to bring up
>> the hotplug slot as soon as presence or link is detected.  We don't wait
>> for a button press.  This is intended as a convience feature to bring up
>> slots as quickly as possible, but the behavior can be changed if the
>> button press and 5 second delay is preferred.
> No personal preference, I thought that is how the code in Linux was
> earlier.
>
> Looks like we still don't subscribe to PDC if ATTN is present? So you don't
> get an event until someone presses the button to process hotplug right?
>
> pciehp_hpc.c:pcie_enable_notification()
> {
> ....
>
>           * Always enable link events: thus link-up and link-down shall
>           * always be treated as hotplug and unplug respectively. Enable
>           * presence detect only if Attention Button is not present.
>           */
>          cmd = PCI_EXP_SLTCTL_DLLSCE;
>          if (ATTN_BUTTN(ctrl))
>                  cmd |= PCI_EXP_SLTCTL_ABPE;
>          else
>                  cmd |= PCI_EXP_SLTCTL_PDCE;
> ....
> }
>
>
> Looks like we still wait for button press to process. When you don't have a
> power control yes the DLLSC would kick in and we would process them. but if
> you have power control, you won't turn on until the button press?
>
Yes, as per current logic, if attention button is present, then we don't
subscribe to PDC event changes. we wait for button press  to turn on/off
the slot.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[Lukas Wunner]

On Thu, Nov 19, 2020 at 02:08:07PM -0800, Raj, Ashok wrote:
> On Thu, Nov 19, 2020 at 08:51:20AM +0100, Lukas Wunner wrote:
> > If an Attention Button is present, the current behavior is to bring up
> > the hotplug slot as soon as presence or link is detected.  We don't wait
> > for a button press.  This is intended as a convience feature to bring up
> > slots as quickly as possible, but the behavior can be changed if the
> > button press and 5 second delay is preferred.
> 
> Looks like we still don't subscribe to PDC if ATTN is present? So you don't
> get an event until someone presses the button to process hotplug right?
[...]
> Looks like we still wait for button press to process. When you don't have a
> power control yes the DLLSC would kick in and we would process them. but if
> you have power control, you won't turn on until the button press?

Right.

For hotplug ports without a power controller, we could in principle achieve
the same behavior (i.e. not bring up the slot until the button is pressed)
by not subscribing to DLLSC events as well (if an Attention Button is
present).

However there are hotplug ports out there with incorrect data in the
Slot Capabilities register:  They claim to have an Attention Button
but in reality don't have one.  In those cases we're still able to
bring up and down the slot right now.  Whereas if we changed the
behavior, those hotplug ports would no longer work.  (We'd not
bring up the slot until the button is pressed but there is no button.)

E.g. this bugzilla contains dmesg output for a 5.4 kernel which is able
to bring up and down the slot correctly even though the Slot Capabilities
register incorrectly claims to have an Attention Button as well as
Command Complete support:
https://bugzilla.kernel.org/show_bug.cgi?id=209283


> > In any case the behavior in response to an Attention Button press should
> > be the same regardless whether an MRL is present or not.  (The spec
> > doesn't seem to say otherwise.)
> 
> True, Looks like that is a Linux behavior, certainly not a spec
> recommendation. Our validation teams tell Windows also behaves such. i.e
> when ATTN exists button press drivers the hot-add. Linux doesn't need to
> follow suite though. 
> 
> In that case do we need to subscribe to PDC even when ATTN is present?

Hm, I'd just leave it the way it is for now if it works.


> > > +	/*
> > > +	 * If ATTN is present and MRL is triggered
> > > +	 * ignore the Presence Change Event.
> > > +	 */
> > > +	if (ATTN_BUTTN(ctrl) && (events & PCI_EXP_SLTSTA_MRLSC))
> > > +		events &= ~PCI_EXP_SLTSTA_PDC;
> > 
> > An Attention Button press results in a synthesized PDC event after
> > 5 seconds, which may get lost due to the above if-statement.
> 
> When its synthesized you don't get the MRLSC? So we won't nuke the PDC then
> correct?

I just meant to say, pciehp_queue_pushbutton_work() will synthesize
a PDC event after 5 seconds and with the above code snippet, if an
MRL event happens simultaneously, that synthesized PDC event would
be lost.  So I'd just drop the above code snippet.  I think you
just need to subscribe to MRL events and propagate them to
pciehp_handle_presence_or_link_change().  There, you'd bring down
the slot if an MRL event has occurred (same as DLLSC or PDC).
Then, check whether MRL is closed.  If so, and if presence or link
is up, try to bring up the slot.

If the MRL is open when slot or presence has gone up, the slot is not
brought up.  But once MRL is closed, there'll be another MRL event and
*then* the slot is brought up.

Thanks,

Lukas

Re: [PATCH 1/1] pci: pciehp: Handle MRL interrupts to enable slot for hotplug.

[Raj, Ashok]

On Sat, Nov 21, 2020 at 12:10:50PM +0100, Lukas Wunner wrote:
> 
> > > > +	/*
> > > > +	 * If ATTN is present and MRL is triggered
> > > > +	 * ignore the Presence Change Event.
> > > > +	 */
> > > > +	if (ATTN_BUTTN(ctrl) && (events & PCI_EXP_SLTSTA_MRLSC))
> > > > +		events &= ~PCI_EXP_SLTSTA_PDC;
> > > 
> > > An Attention Button press results in a synthesized PDC event after
> > > 5 seconds, which may get lost due to the above if-statement.
> > 
> > When its synthesized you don't get the MRLSC? So we won't nuke the PDC then
> > correct?
> 
> I just meant to say, pciehp_queue_pushbutton_work() will synthesize
> a PDC event after 5 seconds and with the above code snippet, if an
> MRL event happens simultaneously, that synthesized PDC event would
> be lost.  So I'd just drop the above code snippet.  I think you
> just need to subscribe to MRL events and propagate them to
> pciehp_handle_presence_or_link_change().  There, you'd bring down
> the slot if an MRL event has occurred (same as DLLSC or PDC).
> Then, check whether MRL is closed.  If so, and if presence or link
> is up, try to bring up the slot.
> 
> If the MRL is open when slot or presence has gone up, the slot is not
> brought up.  But once MRL is closed, there'll be another MRL event and
> *then* the slot is brought up.
> 

Sounds good.. I'll send the update patch.