diff mbox series

[v2,1/1] PCI: pciehp: Add support for handling MRL events

Message ID 20201122014203.4706-1-ashok.raj@intel.com
State New
Headers show
Series [v2,1/1] PCI: pciehp: Add support for handling MRL events | expand

Commit Message

Raj, Ashok Nov. 22, 2020, 1:42 a.m. UTC
When Mechanical Retention Lock (MRL) is present, Linux doesn't process
those change events.

Support for these can be found starting Icelake Server.

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.
   - If currently slot is powered on, but MRL is open,
     PCIe Base Spec 5.0 Chapter 6.7.1.3 states.
     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.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Co-developed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
Changes since v1:
- Changes suggested by Lucas Wunner
  https://lore.kernel.org/linux-pci/20201119223749.GA103783@otc-nc-03/T/#m1f661ae901e7dedad73dea370bb63abd52c610eb
  - Consolidate MRL handling in pciehp_handle_presence_or_link_change()
  - Added helped latch_closed()
  - Add comments why MRL open should function as hot-remove.
  - Don't nuke PDC, it might mask a button PUSH synthesized event after 5
    secs.
- Bjorn: Fix Subject to be consistent with other commits.
---
 drivers/pci/hotplug/pciehp_ctrl.c | 36 +++++++++++++++++++++++++++++++++++-
 drivers/pci/hotplug/pciehp_hpc.c  | 14 ++++++++++++--
 2 files changed, 47 insertions(+), 3 deletions(-)

Comments

Lukas Wunner Nov. 22, 2020, 9:08 a.m. UTC | #1
On Sat, Nov 21, 2020 at 05:42:03PM -0800, Ashok Raj wrote:
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>  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.
> @@ -246,6 +259,20 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  		if (events & PCI_EXP_SLTSTA_PDC)
>  			ctrl_info(ctrl, "Slot(%s): Card not present\n",
>  				  slot_name(ctrl));
> +		if (events & PCI_EXP_SLTSTA_MRLSC)
> +			ctrl_info(ctrl, "Slot(%s): Latch %s\n",
> +				  slot_name(ctrl), getstatus ? "Open" : "Closed");

This message will currently always be "Latch closed".  It should be
"Latch open" instead because if the slot was up, the latch must have
been closed.  So an MRLSC event can only mean that the latch is now open.
The "getstatus" variable can be removed.


> +		/*
> +		 * PCIe Base Spec 5.0 Chapter 6.7.1.3 states.
> +		 *
> +		 * 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.
> +		 */
>  		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
>  		break;

The code comment is not wrapped at 80 chars and a bit long.
I'd move it to the commit message and keep only a shortened version here.

The "SURPRISE_REMOVAL" may now be problematic because the card may still
be in the slot (both presence and link still up) with only the MRL open.
My suggestion would be to add a local variable "bool safe_removal"
which is initialized to "SAFE_REMOVAL".  In the two if-clauses for
DLLSC and PDC, it is set to SURPRISE_REMOVAL.


> @@ -275,6 +302,13 @@ 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 slot is closed && ATTN button exists
> +		 * don't continue, let the ATTN button
> +		 * drive the hot-plug
> +		 */
> +		if (((events & PCI_EXP_SLTSTA_MRLSC) && ATTN_BUTTN(ctrl)))
> +			return;
>  		ctrl->request_result = pciehp_enable_slot(ctrl);
>  		break;

Hm, if the Attention Button is pressed with MRL still open, the slot is
not brought up.  If the MRL is subsequently closed, it is still not
brought up.  I guess the slot keeps blinking and one has to push the
button to abort the operation, then press it once more to attempt
another slot bringup.  The spec doesn't seem to say how such a situation
should be handled. Oh well.

I'm wondering if this is the right place to bail out:  Immediately
before the above hunk, the button_work is canceled, so it can't later
trigger bringup of the slot.  Shouldn't the above check be in the
code block with the "Turn the slot on if it's occupied or link is up"
comment?

You're also not unlocking the state_lock here before bailing out of
the function.


> @@ -710,8 +710,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
>  	down_read(&ctrl->reset_lock);
>  	if (events & DISABLE_SLOT)
>  		pciehp_handle_disable_request(ctrl);
> -	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
> +	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC |
> +			   PCI_EXP_SLTSTA_MRLSC))
>  		pciehp_handle_presence_or_link_change(ctrl, events);
> +
>  	up_read(&ctrl->reset_lock);

Unnecessary newline added.


> @@ -768,6 +770,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;
> +

The code comment doesn't add much information, so can probably be
dropped.

You need to add PCI_EXP_SLTCTL_MRLSCE to the "mask" variable in this
function (before PFDE, as in pcie_disable_notification()).
I don't think the interrupt is enabled at all if it's not added to
"mask", has this patch been tested at all?

Something else:  When pciehp probes, it should check whether the slot
is up even though MRL is open.  (E.g. the machine is booted, the card
in the slot was enumerated but the latch is open.)  I think in that
case we need to bring down the slot.  I suggest adding a check to
pciehp_check_presence() whether the latch is open.  If so,
a PCI_EXP_SLTSTA_MRLSC event should be synthesized.  You could rename
the latch_closed() function to pciehp_latch_closed() and remove its
"static" attribute so that you can call it from pciehp_core.c.

Thanks,

Lukas
Raj, Ashok Nov. 25, 2020, 10:40 p.m. UTC | #2
Hi Lukas

On Sun, Nov 22, 2020 at 10:08:52AM +0100, Lukas Wunner wrote:
> On Sat, Nov 21, 2020 at 05:42:03PM -0800, Ashok Raj wrote:
> > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> >  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.
> > @@ -246,6 +259,20 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> >  		if (events & PCI_EXP_SLTSTA_PDC)
> >  			ctrl_info(ctrl, "Slot(%s): Card not present\n",
> >  				  slot_name(ctrl));
> > +		if (events & PCI_EXP_SLTSTA_MRLSC)
> > +			ctrl_info(ctrl, "Slot(%s): Latch %s\n",
> > +				  slot_name(ctrl), getstatus ? "Open" : "Closed");
> 
> This message will currently always be "Latch closed".  It should be
> "Latch open" instead because if the slot was up, the latch must have
> been closed.  So an MRLSC event can only mean that the latch is now open.
> The "getstatus" variable can be removed.

We only report if there was an MRLSC event. What if there is a link event,
but MRL is closed? This just reflects current state rather than hardcoding
a value which could be wrong in cases where you try to remove due to DLLSC
event?

> 
> 
> > +		/*
> > +		 * PCIe Base Spec 5.0 Chapter 6.7.1.3 states.
> > +		 *
> > +		 * 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.
> > +		 */
> >  		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
> >  		break;
> 
> The code comment is not wrapped at 80 chars and a bit long.
> I'd move it to the commit message and keep only a shortened version here.

Make sense. I'll clean this up.
> 
> The "SURPRISE_REMOVAL" may now be problematic because the card may still
> be in the slot (both presence and link still up) with only the MRL open.
> My suggestion would be to add a local variable "bool safe_removal"
> which is initialized to "SAFE_REMOVAL".  In the two if-clauses for
> DLLSC and PDC, it is set to SURPRISE_REMOVAL.

I see, so for MRL we want to treat it as safe-removal, for other two its
surprise. Got it.

> 
> 
> > @@ -275,6 +302,13 @@ 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 slot is closed && ATTN button exists
> > +		 * don't continue, let the ATTN button
> > +		 * drive the hot-plug
> > +		 */
> > +		if (((events & PCI_EXP_SLTSTA_MRLSC) && ATTN_BUTTN(ctrl)))
> > +			return;
> >  		ctrl->request_result = pciehp_enable_slot(ctrl);
> >  		break;
> 
> Hm, if the Attention Button is pressed with MRL still open, the slot is
> not brought up.  If the MRL is subsequently closed, it is still not
> brought up.  I guess the slot keeps blinking and one has to push the
> button to abort the operation, then press it once more to attempt
> another slot bringup.  The spec doesn't seem to say how such a situation
> should be handled. Oh well.

Looks like we are in the same boat today even without MRL. If no card in
slot and someone presses ATTN, after 5 sec blink, we call the synthetic PDC
event. But the check for present || link_active would fail and return
immediately. So the light would keep blinking until someone presses ATTN to
cancel?

Maybe in that we should simply cancel if it was blinking before we return?

> 
> I'm wondering if this is the right place to bail out:  Immediately
> before the above hunk, the button_work is canceled, so it can't later
> trigger bringup of the slot.  Shouldn't the above check be in the
> code block with the "Turn the slot on if it's occupied or link is up"
> comment?

Or maybe remove the check !latch_closed(ctrl), and let if fall through
anyway. 
> 
> You're also not unlocking the state_lock here before bailing out of
> the function.
> 
> 
> > @@ -710,8 +710,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
> >  	down_read(&ctrl->reset_lock);
> >  	if (events & DISABLE_SLOT)
> >  		pciehp_handle_disable_request(ctrl);
> > -	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
> > +	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC |
> > +			   PCI_EXP_SLTSTA_MRLSC))
> >  		pciehp_handle_presence_or_link_change(ctrl, events);
> > +
> >  	up_read(&ctrl->reset_lock);
> 
> Unnecessary newline added.

Will remove.

> 
> 
> > @@ -768,6 +770,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;
> > +
> 
> The code comment doesn't add much information, so can probably be
> dropped.

Make sense.

> 
> You need to add PCI_EXP_SLTCTL_MRLSCE to the "mask" variable in this
> function (before PFDE, as in pcie_disable_notification()).
> I don't think the interrupt is enabled at all if it's not added to
> "mask", has this patch been tested at all?

The first patch was tested, but I didn't have that in the mask variable
even then. 

> 
> Something else:  When pciehp probes, it should check whether the slot
> is up even though MRL is open.  (E.g. the machine is booted, the card
> in the slot was enumerated but the latch is open.)  I think in that
> case we need to bring down the slot.  I suggest adding a check to
> pciehp_check_presence() whether the latch is open.  If so,
> a PCI_EXP_SLTSTA_MRLSC event should be synthesized.  You could rename
> the latch_closed() function to pciehp_latch_closed() and remove its
> "static" attribute so that you can call it from pciehp_core.c.

Good point. I missed that. I'll have another version spun after a test.
diff mbox series

Patch

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 9f85815b4f53..aa8b187ff769 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -224,9 +224,22 @@  void pciehp_handle_disable_request(struct controller *ctrl)
 	ctrl->request_result = pciehp_disable_slot(ctrl, SAFE_REMOVAL);
 }
 
+static bool latch_closed(struct controller *ctrl)
+{
+	u8 getstatus = 0;
+
+	if (!MRL_SENS(ctrl))
+		return true;
+
+	pciehp_get_latch_status(ctrl, &getstatus);
+
+	return (getstatus == 0 ? true : false);
+}
+
 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.
@@ -246,6 +259,20 @@  void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 		if (events & PCI_EXP_SLTSTA_PDC)
 			ctrl_info(ctrl, "Slot(%s): Card not present\n",
 				  slot_name(ctrl));
+		if (events & PCI_EXP_SLTSTA_MRLSC)
+			ctrl_info(ctrl, "Slot(%s): Latch %s\n",
+				  slot_name(ctrl), getstatus ? "Open" : "Closed");
+		/*
+		 * PCIe Base Spec 5.0 Chapter 6.7.1.3 states.
+		 *
+		 * 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.
+		 */
 		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
 		break;
 	default:
@@ -257,7 +284,7 @@  void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	mutex_lock(&ctrl->state_lock);
 	present = pciehp_card_present(ctrl);
 	link_active = pciehp_check_link_active(ctrl);
-	if (present <= 0 && link_active <= 0) {
+	if ((present <= 0 && link_active <= 0) || !latch_closed(ctrl)) {
 		mutex_unlock(&ctrl->state_lock);
 		return;
 	}
@@ -275,6 +302,13 @@  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 slot is closed && ATTN button exists
+		 * don't continue, let the ATTN button
+		 * drive the hot-plug
+		 */
+		if (((events & PCI_EXP_SLTSTA_MRLSC) && ATTN_BUTTN(ctrl)))
+			return;
 		ctrl->request_result = pciehp_enable_slot(ctrl);
 		break;
 	default:
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 53433b37e181..7cfa27bcf951 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
@@ -710,8 +710,10 @@  static irqreturn_t pciehp_ist(int irq, void *dev_id)
 	down_read(&ctrl->reset_lock);
 	if (events & DISABLE_SLOT)
 		pciehp_handle_disable_request(ctrl);
-	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
+	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC |
+			   PCI_EXP_SLTSTA_MRLSC))
 		pciehp_handle_presence_or_link_change(ctrl, events);
+
 	up_read(&ctrl->reset_lock);
 
 	ret = IRQ_HANDLED;
@@ -768,6 +770,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;