[PATCH 2/2] PCI: pciehp: Deduplicate presence check on probe & resume

[PATCH 2/2] PCI: pciehp: Deduplicate presence check on probe & resume

[Lukas Wunner]

On driver probe and on resume from system sleep, pciehp checks the
Presence Detect State bit in the Slot Status register to bring up an
occupied slot or bring down an unoccupied slot.  Both code paths are
identical, so deduplicate them per Mika's request.

On probe, an additional check is performed to disable power of an
unoccupied slot.  This can e.g. happen if power was enabled by BIOS.
It cannot happen once pciehp has taken control, hence is not necessary
on resume:  The Slot Control register is set to the same value that it
had on suspend by pci_restore_state(), so if the slot was occupied,
power is enabled and if it wasn't, power is disabled.  Should occupancy
have changed during the system sleep transition, power is adjusted by
bringing up or down the slot per the paragraph above.

To allow for deduplication of the presence check, move the power check
to pcie_init().  This seems safer anyway, because right now it is
performed while interrupts are already enabled, and although I can't
think of a scenario where pciehp_power_off_slot() and the IRQ thread
collide, it does feel brittle.

However this means that pcie_init() may now write to the Slot Control
register before the IRQ is requested.  If both the CCIE and HPIE bits
happen to be set, pcie_wait_cmd() will wait for an interrupt (instead
of polling the Command Completed bit) and eventually emit a timeout
message.  Additionally, if a level-triggered INTx interrupt is used,
the user may see a spurious interrupt splat.  Avoid by disabling
interrupts before disabling power.  (Normally the HPIE and CCIE bits
should be clear on probe, but conceivably they may already have been
set e.g. by BIOS.)

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_core.c | 63 ++++++++++++++++---------------
 drivers/pci/hotplug/pciehp_hpc.c  | 14 +++++++
 2 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 01fcf1fa0f66..ec48c9433ae5 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -200,12 +200,40 @@ static int reset_slot(struct hotplug_slot *hotplug_slot, int probe)
 	return pciehp_reset_slot(slot, probe);
 }
 
+/**
+ * pciehp_check_presence() - synthesize event if presence has changed
+ *
+ * On probe and resume, an explicit presence check is necessary to bring up an
+ * occupied slot or bring down an unoccupied slot.  This can't be triggered by
+ * events in the Slot Status register, they may be stale and are therefore
+ * cleared.  Secondly, sending an interrupt for "events that occur while
+ * interrupt generation is disabled [when] interrupt generation is subsequently
+ * enabled" is optional per PCIe r4.0, sec 6.7.3.4.
+ */
+static void pciehp_check_presence(struct controller *ctrl)
+{
+	struct slot *slot = ctrl->slot;
+	u8 occupied;
+
+	down_read(&ctrl->reset_lock);
+	mutex_lock(&slot->lock);
+
+	pciehp_get_adapter_status(slot, &occupied);
+	if ((occupied && (slot->state == OFF_STATE ||
+			  slot->state == BLINKINGON_STATE)) ||
+	    (!occupied && (slot->state == ON_STATE ||
+			   slot->state == BLINKINGOFF_STATE)))
+		pciehp_request(ctrl, PCI_EXP_SLTSTA_PDC);
+
+	mutex_unlock(&slot->lock);
+	up_read(&ctrl->reset_lock);
+}
+
 static int pciehp_probe(struct pcie_device *dev)
 {
 	int rc;
 	struct controller *ctrl;
 	struct slot *slot;
-	u8 occupied, poweron;
 
 	/* If this is not a "hotplug" service, we have no business here. */
 	if (dev->service != PCIE_PORT_SERVICE_HP)
@@ -250,21 +278,7 @@ static int pciehp_probe(struct pcie_device *dev)
 		goto err_out_shutdown_notification;
 	}
 
-	/* Check if slot is occupied */
-	down_read(&ctrl->reset_lock);
-	mutex_lock(&slot->lock);
-	pciehp_get_adapter_status(slot, &occupied);
-	pciehp_get_power_status(slot, &poweron);
-	if ((occupied && (slot->state == OFF_STATE ||
-			  slot->state == BLINKINGON_STATE)) ||
-	    (!occupied && (slot->state == ON_STATE ||
-			   slot->state == BLINKINGOFF_STATE)))
-		pciehp_request(ctrl, PCI_EXP_SLTSTA_PDC);
-	/* If empty slot's power status is on, turn power off */
-	if (!occupied && poweron && POWER_CTRL(ctrl))
-		pciehp_power_off_slot(slot);
-	mutex_unlock(&slot->lock);
-	up_read(&ctrl->reset_lock);
+	pciehp_check_presence(ctrl);
 
 	return 0;
 
@@ -311,22 +325,9 @@ static int pciehp_resume_noirq(struct pcie_device *dev)
 
 static int pciehp_resume(struct pcie_device *dev)
 {
-	struct controller *ctrl;
-	struct slot *slot;
-	u8 status;
-
-	ctrl = get_service_data(dev);
-	slot = ctrl->slot;
+	struct controller *ctrl = get_service_data(dev);
 
-	/* Check if slot is occupied */
-	pciehp_get_adapter_status(slot, &status);
-	mutex_lock(&slot->lock);
-	if ((status && (slot->state == OFF_STATE ||
-			slot->state == BLINKINGON_STATE)) ||
-	    (!status && (slot->state == ON_STATE ||
-			 slot->state == BLINKINGOFF_STATE)))
-		pciehp_request(ctrl, PCI_EXP_SLTSTA_PDC);
-	mutex_unlock(&slot->lock);
+	pciehp_check_presence(ctrl);
 
 	return 0;
 }
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 6f7de0819c88..5b15e76f3564 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -856,6 +856,7 @@ struct controller *pcie_init(struct pcie_device *dev)
 {
 	struct controller *ctrl;
 	u32 slot_cap, link_cap;
+	u8 occupied, poweron;
 	struct pci_dev *pdev = dev->port;
 
 	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
@@ -910,6 +911,19 @@ struct controller *pcie_init(struct pcie_device *dev)
 	if (pcie_init_slot(ctrl))
 		goto abort_ctrl;
 
+	/*
+	 * If empty slot's power status is on, turn power off.  The IRQ isn't
+	 * requested yet, so avoid triggering a notification with this command.
+	 */
+	if (POWER_CTRL(ctrl)) {
+		pciehp_get_adapter_status(ctrl->slot, &occupied);
+		pciehp_get_power_status(ctrl->slot, &poweron);
+		if (!occupied && poweron) {
+			pcie_disable_notification(ctrl);
+			pciehp_power_off_slot(ctrl->slot);
+		}
+	}
+
 	return ctrl;
 
 abort_ctrl:
-- 
2.18.0

[PATCH 1/2] PCI: pciehp: Avoid implicit fallthroughs in switch statements

[Lukas Wunner]

Per Mika's request, add an explicit break to the last case of switch
statements everywhere in pciehp to be more defensive towards future
amendments.

Per Gustavo's request, mark all non-empty implicit fallthroughs with a
comment to silence warnings triggered by -Wimplicit-fallthrough=2.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 6855933ab372..da7c72372ffc 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -211,6 +211,7 @@ void pciehp_handle_disable_request(struct slot *slot)
 	case BLINKINGON_STATE:
 	case BLINKINGOFF_STATE:
 		cancel_delayed_work(&slot->work);
+		break;
 	}
 	slot->state = POWEROFF_STATE;
 	mutex_unlock(&slot->lock);
@@ -232,6 +233,7 @@ void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events)
 	switch (slot->state) {
 	case BLINKINGOFF_STATE:
 		cancel_delayed_work(&slot->work);
+		/* fall through */
 	case ON_STATE:
 		slot->state = POWEROFF_STATE;
 		mutex_unlock(&slot->lock);
@@ -245,6 +247,7 @@ void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events)
 		break;
 	default:
 		mutex_unlock(&slot->lock);
+		break;
 	}
 
 	/* Turn the slot on if it's occupied or link is up */
@@ -259,6 +262,7 @@ void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events)
 	switch (slot->state) {
 	case BLINKINGON_STATE:
 		cancel_delayed_work(&slot->work);
+		/* fall through */
 	case OFF_STATE:
 		slot->state = POWERON_STATE;
 		mutex_unlock(&slot->lock);
@@ -272,6 +276,7 @@ void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events)
 		break;
 	default:
 		mutex_unlock(&slot->lock);
+		break;
 	}
 }
 
-- 
2.18.0

Re: [PATCH 1/2] PCI: pciehp: Avoid implicit fallthroughs in switch statements

[Mika Westerberg]

On Sat, Jul 28, 2018 at 07:22:00AM +0200, Lukas Wunner wrote:
> Per Mika's request, add an explicit break to the last case of switch
> statements everywhere in pciehp to be more defensive towards future
> amendments.
> 
> Per Gustavo's request, mark all non-empty implicit fallthroughs with a
> comment to silence warnings triggered by -Wimplicit-fallthrough=2.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 2/2] PCI: pciehp: Deduplicate presence check on probe & resume

[Mika Westerberg]

On Sat, Jul 28, 2018 at 07:22:00AM +0200, Lukas Wunner wrote:
> On driver probe and on resume from system sleep, pciehp checks the
> Presence Detect State bit in the Slot Status register to bring up an
> occupied slot or bring down an unoccupied slot.  Both code paths are
> identical, so deduplicate them per Mika's request.
> 
> On probe, an additional check is performed to disable power of an
> unoccupied slot.  This can e.g. happen if power was enabled by BIOS.
> It cannot happen once pciehp has taken control, hence is not necessary
> on resume:  The Slot Control register is set to the same value that it
> had on suspend by pci_restore_state(), so if the slot was occupied,
> power is enabled and if it wasn't, power is disabled.  Should occupancy
> have changed during the system sleep transition, power is adjusted by
> bringing up or down the slot per the paragraph above.
> 
> To allow for deduplication of the presence check, move the power check
> to pcie_init().  This seems safer anyway, because right now it is
> performed while interrupts are already enabled, and although I can't
> think of a scenario where pciehp_power_off_slot() and the IRQ thread
> collide, it does feel brittle.
> 
> However this means that pcie_init() may now write to the Slot Control
> register before the IRQ is requested.  If both the CCIE and HPIE bits
> happen to be set, pcie_wait_cmd() will wait for an interrupt (instead
> of polling the Command Completed bit) and eventually emit a timeout
> message.  Additionally, if a level-triggered INTx interrupt is used,
> the user may see a spurious interrupt splat.  Avoid by disabling
> interrupts before disabling power.  (Normally the HPIE and CCIE bits
> should be clear on probe, but conceivably they may already have been
> set e.g. by BIOS.)
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

One small nit, see below:

> ---
>  drivers/pci/hotplug/pciehp_core.c | 63 ++++++++++++++++---------------
>  drivers/pci/hotplug/pciehp_hpc.c  | 14 +++++++
>  2 files changed, 46 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 01fcf1fa0f66..ec48c9433ae5 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -200,12 +200,40 @@ static int reset_slot(struct hotplug_slot *hotplug_slot, int probe)
>  	return pciehp_reset_slot(slot, probe);
>  }
>  
> +/**
> + * pciehp_check_presence() - synthesize event if presence has changed

You should document the argument as well so that the tool that is used
to process these knows about it:

  @ctrl: ....

> + *
> + * On probe and resume, an explicit presence check is necessary to bring up an
> + * occupied slot or bring down an unoccupied slot.  This can't be triggered by
> + * events in the Slot Status register, they may be stale and are therefore
> + * cleared.  Secondly, sending an interrupt for "events that occur while
> + * interrupt generation is disabled [when] interrupt generation is subsequently
> + * enabled" is optional per PCIe r4.0, sec 6.7.3.4.
> + */
> +static void pciehp_check_presence(struct controller *ctrl)

Re: [PATCH 1/2] PCI: pciehp: Avoid implicit fallthroughs in switch statements

[Gustavo A. R. Silva]

On 07/28/2018 12:22 AM, Lukas Wunner wrote:
> Per Mika's request, add an explicit break to the last case of switch
> statements everywhere in pciehp to be more defensive towards future
> amendments.
> 
> Per Gustavo's request, mark all non-empty implicit fallthroughs with a
> comment to silence warnings triggered by -Wimplicit-fallthrough=2.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---


Acked-by: Gustavo A. R. Silva <gustavo@embeddedor.com>


Thanks
--
Gustavo

Re: [PATCH 1/2] PCI: pciehp: Avoid implicit fallthroughs in switch statements

[Bjorn Helgaas]

On Sat, Jul 28, 2018 at 07:22:00AM +0200, Lukas Wunner wrote:
> Per Mika's request, add an explicit break to the last case of switch
> statements everywhere in pciehp to be more defensive towards future
> amendments.
> 
> Per Gustavo's request, mark all non-empty implicit fallthroughs with a
> comment to silence warnings triggered by -Wimplicit-fallthrough=2.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Both applied, with Mika's reviewed-by on both and Gustavo's ack on the
first, to pci/hotplug for v4.19, thanks!

> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 6855933ab372..da7c72372ffc 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -211,6 +211,7 @@ void pciehp_handle_disable_request(struct slot *slot)
>  	case BLINKINGON_STATE:
>  	case BLINKINGOFF_STATE:
>  		cancel_delayed_work(&slot->work);
> +		break;
>  	}
>  	slot->state = POWEROFF_STATE;
>  	mutex_unlock(&slot->lock);
> @@ -232,6 +233,7 @@ void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events)
>  	switch (slot->state) {
>  	case BLINKINGOFF_STATE:
>  		cancel_delayed_work(&slot->work);
> +		/* fall through */
>  	case ON_STATE:
>  		slot->state = POWEROFF_STATE;
>  		mutex_unlock(&slot->lock);
> @@ -245,6 +247,7 @@ void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events)
>  		break;
>  	default:
>  		mutex_unlock(&slot->lock);
> +		break;
>  	}
>  
>  	/* Turn the slot on if it's occupied or link is up */
> @@ -259,6 +262,7 @@ void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events)
>  	switch (slot->state) {
>  	case BLINKINGON_STATE:
>  		cancel_delayed_work(&slot->work);
> +		/* fall through */
>  	case OFF_STATE:
>  		slot->state = POWERON_STATE;
>  		mutex_unlock(&slot->lock);
> @@ -272,6 +276,7 @@ void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events)
>  		break;
>  	default:
>  		mutex_unlock(&slot->lock);
> +		break;
>  	}
>  }
>  
> -- 
> 2.18.0
> 

Re: [PATCH 2/2] PCI: pciehp: Deduplicate presence check on probe & resume

[Lukas Wunner]

On Tue, Jul 31, 2018 at 11:03:42AM +0300, Mika Westerberg wrote:
> On Sat, Jul 28, 2018 at 07:22:00AM +0200, Lukas Wunner wrote:
> One small nit, see below:
> 
> > +/**
> > + * pciehp_check_presence() - synthesize event if presence has changed
> 
> You should document the argument as well so that the tool that is used
> to process these knows about it:
> 
>   @ctrl: ....

Right, I'll fix that up in a follow-up commit since Bjorn has already
pushed this version.

@Bjorn: I've double-checked all the rebased and newly applied patches
on the pci/hotplug branch, looks good.  Thanks for adding the reference
to the "Link Status Mapped to the LTSSM" table to the commit message,
and thanks to Sinan for pointing that table out.

Superb job everyone, thanks a lot!

Lukas