[PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Rajat Jain]

A lot of systems do not have the fancy buttons and LEDs, and instead
want to rely only on the Link state change events to drive the hotplug
and removal state machinery.
(http://www.spinics.net/lists/hotplug/msg05802.html)

This patch adds support for that functionality. Here are the details
about the patch itself:

* Define and use interrupt events for linkup / linkdown.

* Introduce the functions to handle link-up and link-down events and
  queue the work in the slot->wq to be processed by pciehp_power_thread

* The current code bails out from device removal, if an adapter is not
  detected. That is not right, because if an adapter is not present at
  all, it provides all the more reason to REMOVE the device. This is
  specially a problem for link state hot-plug, because some ports use
  in band mechanism to detect the presence detection. Thus when link
  goes down, presence detect also goes down. We _want_ that the devices
  should be removed in this case.

* The current pciehp driver disabled the link in case of a hot-removal.
  Since for link change based hot-plug to work, we need that enabled,
  hence make sure to not disable the link permanently if link state
  based hot-plug is to be used. Also have to remove
  pciehp_link_disable() and pcie_wait_link_not_active() static functions
  since they are not being used anywhere else.

* pciehp_reset_slot - reset of secondary bus may cause undesirable
  spurious link notifications. Thus disable these around the secondary
  bus reset.

Signed-off-by: Rajat Jain <rajatjain@juniper.net>
Signed-off-by: Guenter Roeck <groeck@juniper.net>
---
 v2: - drop the use_link_state_events module parameter as discussed here:
       http://marc.info/?t=138513966800006&r=1&w=2
     - removed the static functions left unused after this patch.
     - make the pciehp_handle_linkstate_change() return void.
     - dropped the "RFC" from subject, and added Guenter's signature

 drivers/pci/hotplug/pciehp.h      |    3 +
 drivers/pci/hotplug/pciehp_ctrl.c |  130 ++++++++++++++++++++++++++++++++++---
 drivers/pci/hotplug/pciehp_hpc.c  |   56 ++++++++--------
 3 files changed, 150 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index fc322ed..353edda 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -110,6 +110,8 @@ struct controller {
 #define INT_BUTTON_PRESS		7
 #define INT_BUTTON_RELEASE		8
 #define INT_BUTTON_CANCEL		9
+#define INT_LINK_UP			10
+#define INT_LINK_DOWN			11
 
 #define STATIC_STATE			0
 #define BLINKINGON_STATE		1
@@ -133,6 +135,7 @@ u8 pciehp_handle_attention_button(struct slot *p_slot);
 u8 pciehp_handle_switch_change(struct slot *p_slot);
 u8 pciehp_handle_presence_change(struct slot *p_slot);
 u8 pciehp_handle_power_fault(struct slot *p_slot);
+void pciehp_handle_linkstate_change(struct slot *p_slot);
 int pciehp_configure_device(struct slot *p_slot);
 int pciehp_unconfigure_device(struct slot *p_slot);
 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 38f0186..4c2544c 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -150,6 +150,27 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
 	return 1;
 }
 
+void pciehp_handle_linkstate_change(struct slot *p_slot)
+{
+	u32 event_type;
+	struct controller *ctrl = p_slot->ctrl;
+
+	/* Link Status Change */
+	ctrl_dbg(ctrl, "Data Link Layer State change\n");
+
+	if (pciehp_check_link_active(ctrl)) {
+		ctrl_info(ctrl, "slot(%s): Link Up event\n",
+			  slot_name(p_slot));
+		event_type = INT_LINK_UP;
+	} else {
+		ctrl_info(ctrl, "slot(%s): Link Down event\n",
+			  slot_name(p_slot));
+		event_type = INT_LINK_DOWN;
+	}
+
+	queue_interrupt_event(p_slot, event_type);
+}
+
 /* The following routines constitute the bulk of the
    hotplug controller logic
  */
@@ -442,6 +463,100 @@ static void handle_surprise_event(struct slot *p_slot)
 	queue_work(p_slot->wq, &info->work);
 }
 
+/*
+ * Note: This function must be called with slot->lock held
+ */
+static void handle_link_up_event(struct slot *p_slot)
+{
+	struct controller *ctrl = p_slot->ctrl;
+	struct power_work_info *info;
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
+			 __func__);
+		return;
+	}
+	info->p_slot = p_slot;
+	INIT_WORK(&info->work, pciehp_power_thread);
+
+	switch (p_slot->state) {
+	case BLINKINGON_STATE:
+	case BLINKINGOFF_STATE:
+		cancel_delayed_work(&p_slot->work);
+		/* Fall through */
+	case STATIC_STATE:
+		p_slot->state = POWERON_STATE;
+		queue_work(p_slot->wq, &info->work);
+		break;
+	case POWERON_STATE:
+		ctrl_info(ctrl,
+			  "Link Up event ignored on slot(%s): already powering on\n",
+			  slot_name(p_slot));
+		kfree(info);
+		break;
+	case POWEROFF_STATE:
+		ctrl_info(ctrl,
+			  "Link Up event queued on slot(%s): currently getting powered off\n",
+			  slot_name(p_slot));
+		p_slot->state = POWERON_STATE;
+		queue_work(p_slot->wq, &info->work);
+		break;
+	default:
+		ctrl_err(ctrl, "Not a valid state on slot(%s)\n",
+			 slot_name(p_slot));
+		kfree(info);
+		break;
+	}
+}
+
+/*
+ * Note: This function must be called with slot->lock held
+ */
+static void handle_link_down_event(struct slot *p_slot)
+{
+	struct controller *ctrl = p_slot->ctrl;
+	struct power_work_info *info;
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
+			 __func__);
+		return;
+	}
+	info->p_slot = p_slot;
+	INIT_WORK(&info->work, pciehp_power_thread);
+
+	switch (p_slot->state) {
+	case BLINKINGON_STATE:
+	case BLINKINGOFF_STATE:
+		cancel_delayed_work(&p_slot->work);
+		/* Fall through */
+	case STATIC_STATE:
+		p_slot->state = POWEROFF_STATE;
+		queue_work(p_slot->wq, &info->work);
+		break;
+	case POWEROFF_STATE:
+		ctrl_info(ctrl,
+			  "Link Down event ignored on slot(%s): already powering off\n",
+			  slot_name(p_slot));
+		kfree(info);
+		break;
+	case POWERON_STATE:
+		ctrl_info(ctrl,
+			  "Link Down event queued on slot(%s): currently getting powered on\n",
+			  slot_name(p_slot));
+		p_slot->state = POWEROFF_STATE;
+		queue_work(p_slot->wq, &info->work);
+		break;
+	default:
+		ctrl_err(ctrl, "Not a valid state on slot %s\n",
+			 slot_name(p_slot));
+		kfree(info);
+		break;
+	}
+}
+
 static void interrupt_event_handler(struct work_struct *work)
 {
 	struct event_info *info = container_of(work, struct event_info, work);
@@ -468,6 +583,12 @@ static void interrupt_event_handler(struct work_struct *work)
 		ctrl_dbg(ctrl, "Surprise Removal\n");
 		handle_surprise_event(p_slot);
 		break;
+	case INT_LINK_UP:
+		handle_link_up_event(p_slot);
+		break;
+	case INT_LINK_DOWN:
+		handle_link_down_event(p_slot);
+		break;
 	default:
 		break;
 	}
@@ -524,15 +645,6 @@ int pciehp_disable_slot(struct slot *p_slot)
 	if (!p_slot->ctrl)
 		return 1;
 
-	if (!HP_SUPR_RM(p_slot->ctrl)) {
-		ret = pciehp_get_adapter_status(p_slot, &getstatus);
-		if (ret || !getstatus) {
-			ctrl_info(ctrl, "No adapter on slot(%s)\n",
-				  slot_name(p_slot));
-			return -ENODEV;
-		}
-	}
-
 	if (MRL_SENS(p_slot->ctrl)) {
 		ret = pciehp_get_latch_status(p_slot, &getstatus);
 		if (ret || getstatus) {
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 3a5eee7..1f152f3 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -278,11 +278,6 @@ static void pcie_wait_link_active(struct controller *ctrl)
 	__pcie_wait_link_active(ctrl, true);
 }
 
-static void pcie_wait_link_not_active(struct controller *ctrl)
-{
-	__pcie_wait_link_active(ctrl, false);
-}
-
 static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
 {
 	u32 l;
@@ -383,11 +378,6 @@ static int pciehp_link_enable(struct controller *ctrl)
 	return __pciehp_link_set(ctrl, true);
 }
 
-static int pciehp_link_disable(struct controller *ctrl)
-{
-	return __pciehp_link_set(ctrl, false);
-}
-
 int pciehp_get_attention_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;
@@ -620,13 +610,10 @@ int pciehp_power_off_slot(struct slot * slot)
 	u16 cmd_mask;
 	int retval;
 
-	/* Disable the link at first */
-	pciehp_link_disable(ctrl);
-	/* wait the link is down */
-	if (ctrl->link_active_reporting)
-		pcie_wait_link_not_active(ctrl);
-	else
-		msleep(1000);
+	/*
+	 * We do not disable the link, since a future link-up event can now
+	 * be used to initiate hot-plug
+	 */
 
 	slot_cmd = POWER_OFF;
 	cmd_mask = PCI_EXP_SLTCTL_PCC;
@@ -661,7 +648,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 
 		detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
 			     PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
-			     PCI_EXP_SLTSTA_CC);
+			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
 		detected &= ~intr_loc;
 		intr_loc |= detected;
 		if (!intr_loc)
@@ -702,6 +689,10 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 		ctrl->power_fault_detected = 1;
 		pciehp_handle_power_fault(slot);
 	}
+
+	if (intr_loc & PCI_EXP_SLTSTA_DLLSC)
+		pciehp_handle_linkstate_change(slot);
+
 	return IRQ_HANDLED;
 }
 
@@ -719,7 +710,7 @@ int pcie_enable_notification(struct controller *ctrl)
 	 * when it is cleared in the interrupt service routine, and
 	 * next power fault detected interrupt was notified again.
 	 */
-	cmd = PCI_EXP_SLTCTL_PDCE;
+	cmd = PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_DLLSCE;
 	if (ATTN_BUTTN(ctrl))
 		cmd |= PCI_EXP_SLTCTL_ABPE;
 	if (MRL_SENS(ctrl))
@@ -751,31 +742,36 @@ static void pcie_disable_notification(struct controller *ctrl)
 
 /*
  * pciehp has a 1:1 bus:slot relationship so we ultimately want a secondary
- * bus reset of the bridge, but if the slot supports surprise removal we need
- * to disable presence detection around the bus reset and clear any spurious
+ * bus reset of the bridge, but if the slot supports surprise removal (or
+ * link state change based hotplug), we need to disable presence detection
+ * (or link state notifications) around the bus reset and clear any spurious
  * events after.
  */
 int pciehp_reset_slot(struct slot *slot, int probe)
 {
 	struct controller *ctrl = slot->ctrl;
+	u16 stat_mask = 0, ctrl_mask = 0;
 
 	if (probe)
 		return 0;
 
 	if (HP_SUPR_RM(ctrl)) {
-		pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_PDCE);
-		if (pciehp_poll_mode)
-			del_timer_sync(&ctrl->poll_timer);
+		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
+		stat_mask |= PCI_EXP_SLTSTA_PDC;
 	}
+	ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
+	stat_mask |= PCI_EXP_SLTSTA_DLLSC;
+
+	pcie_write_cmd(ctrl, 0, ctrl_mask);
+	if (pciehp_poll_mode)
+		del_timer_sync(&ctrl->poll_timer);
 
 	pci_reset_bridge_secondary_bus(ctrl->pcie->port);
 
-	if (HP_SUPR_RM(ctrl)) {
-		pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_PDC);
-		pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PDCE, PCI_EXP_SLTCTL_PDCE);
-		if (pciehp_poll_mode)
-			int_poll_timeout(ctrl->poll_timer.data);
-	}
+	pciehp_writew(ctrl, PCI_EXP_SLTSTA, stat_mask);
+	pcie_write_cmd(ctrl, ctrl_mask, ctrl_mask);
+	if (pciehp_poll_mode)
+		int_poll_timeout(ctrl->poll_timer.data);
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Yijing Wang]

On 2013/12/4 6:32, Rajat Jain wrote:
> A lot of systems do not have the fancy buttons and LEDs, and instead
> want to rely only on the Link state change events to drive the hotplug
> and removal state machinery.
> (http://www.spinics.net/lists/hotplug/msg05802.html)
> 
> This patch adds support for that functionality. Here are the details
> about the patch itself:
> 
> * Define and use interrupt events for linkup / linkdown.
> 
> * Introduce the functions to handle link-up and link-down events and
>   queue the work in the slot->wq to be processed by pciehp_power_thread
> 
> * The current code bails out from device removal, if an adapter is not
>   detected. That is not right, because if an adapter is not present at
>   all, it provides all the more reason to REMOVE the device. This is
>   specially a problem for link state hot-plug, because some ports use
>   in band mechanism to detect the presence detection. Thus when link
>   goes down, presence detect also goes down. We _want_ that the devices
>   should be removed in this case.
> 
> * The current pciehp driver disabled the link in case of a hot-removal.
>   Since for link change based hot-plug to work, we need that enabled,
>   hence make sure to not disable the link permanently if link state
>   based hot-plug is to be used. Also have to remove
>   pciehp_link_disable() and pcie_wait_link_not_active() static functions
>   since they are not being used anywhere else.
> 
> * pciehp_reset_slot - reset of secondary bus may cause undesirable
>   spurious link notifications. Thus disable these around the secondary
>   bus reset.
> 
> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
> Signed-off-by: Guenter Roeck <groeck@juniper.net>
> ---
>  v2: - drop the use_link_state_events module parameter as discussed here:
>        http://marc.info/?t=138513966800006&r=1&w=2
>      - removed the static functions left unused after this patch.
>      - make the pciehp_handle_linkstate_change() return void.
>      - dropped the "RFC" from subject, and added Guenter's signature
> 
>  drivers/pci/hotplug/pciehp.h      |    3 +
>  drivers/pci/hotplug/pciehp_ctrl.c |  130 ++++++++++++++++++++++++++++++++++---
>  drivers/pci/hotplug/pciehp_hpc.c  |   56 ++++++++--------
>  3 files changed, 150 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index fc322ed..353edda 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -110,6 +110,8 @@ struct controller {
>  #define INT_BUTTON_PRESS		7
>  #define INT_BUTTON_RELEASE		8
>  #define INT_BUTTON_CANCEL		9
> +#define INT_LINK_UP			10
> +#define INT_LINK_DOWN			11
>  
>  #define STATIC_STATE			0
>  #define BLINKINGON_STATE		1
> @@ -133,6 +135,7 @@ u8 pciehp_handle_attention_button(struct slot *p_slot);
>  u8 pciehp_handle_switch_change(struct slot *p_slot);
>  u8 pciehp_handle_presence_change(struct slot *p_slot);
>  u8 pciehp_handle_power_fault(struct slot *p_slot);
> +void pciehp_handle_linkstate_change(struct slot *p_slot);
>  int pciehp_configure_device(struct slot *p_slot);
>  int pciehp_unconfigure_device(struct slot *p_slot);
>  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 38f0186..4c2544c 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -150,6 +150,27 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
>  	return 1;
>  }
>  
> +void pciehp_handle_linkstate_change(struct slot *p_slot)
> +{
> +	u32 event_type;
> +	struct controller *ctrl = p_slot->ctrl;
> +
> +	/* Link Status Change */
> +	ctrl_dbg(ctrl, "Data Link Layer State change\n");
> +
> +	if (pciehp_check_link_active(ctrl)) {
> +		ctrl_info(ctrl, "slot(%s): Link Up event\n",
> +			  slot_name(p_slot));
> +		event_type = INT_LINK_UP;
> +	} else {
> +		ctrl_info(ctrl, "slot(%s): Link Down event\n",
> +			  slot_name(p_slot));
> +		event_type = INT_LINK_DOWN;
> +	}
> +
> +	queue_interrupt_event(p_slot, event_type);
> +}
> +
>  /* The following routines constitute the bulk of the
>     hotplug controller logic
>   */
> @@ -442,6 +463,100 @@ static void handle_surprise_event(struct slot *p_slot)
>  	queue_work(p_slot->wq, &info->work);
>  }
>  
> +/*
> + * Note: This function must be called with slot->lock held
> + */
> +static void handle_link_up_event(struct slot *p_slot)
> +{
> +	struct controller *ctrl = p_slot->ctrl;
> +	struct power_work_info *info;
> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
> +			 __func__);
> +		return;
> +	}
> +	info->p_slot = p_slot;
> +	INIT_WORK(&info->work, pciehp_power_thread);
> +
> +	switch (p_slot->state) {
> +	case BLINKINGON_STATE:
> +	case BLINKINGOFF_STATE:
> +		cancel_delayed_work(&p_slot->work);
> +		/* Fall through */
> +	case STATIC_STATE:
> +		p_slot->state = POWERON_STATE;
> +		queue_work(p_slot->wq, &info->work);
> +		break;
> +	case POWERON_STATE:
> +		ctrl_info(ctrl,
> +			  "Link Up event ignored on slot(%s): already powering on\n",
> +			  slot_name(p_slot));
> +		kfree(info);
> +		break;
> +	case POWEROFF_STATE:
> +		ctrl_info(ctrl,
> +			  "Link Up event queued on slot(%s): currently getting powered off\n",
> +			  slot_name(p_slot));
> +		p_slot->state = POWERON_STATE;
> +		queue_work(p_slot->wq, &info->work);
> +		break;
> +	default:
> +		ctrl_err(ctrl, "Not a valid state on slot(%s)\n",
> +			 slot_name(p_slot));
> +		kfree(info);
> +		break;
> +	}
> +}
> +
> +/*
> + * Note: This function must be called with slot->lock held
> + */
> +static void handle_link_down_event(struct slot *p_slot)
> +{
> +	struct controller *ctrl = p_slot->ctrl;
> +	struct power_work_info *info;
> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
> +			 __func__);
> +		return;
> +	}
> +	info->p_slot = p_slot;
> +	INIT_WORK(&info->work, pciehp_power_thread);
> +
> +	switch (p_slot->state) {
> +	case BLINKINGON_STATE:
> +	case BLINKINGOFF_STATE:
> +		cancel_delayed_work(&p_slot->work);
> +		/* Fall through */
> +	case STATIC_STATE:
> +		p_slot->state = POWEROFF_STATE;
> +		queue_work(p_slot->wq, &info->work);
> +		break;
> +	case POWEROFF_STATE:
> +		ctrl_info(ctrl,
> +			  "Link Down event ignored on slot(%s): already powering off\n",
> +			  slot_name(p_slot));
> +		kfree(info);
> +		break;
> +	case POWERON_STATE:
> +		ctrl_info(ctrl,
> +			  "Link Down event queued on slot(%s): currently getting powered on\n",
> +			  slot_name(p_slot));
> +		p_slot->state = POWEROFF_STATE;
> +		queue_work(p_slot->wq, &info->work);
> +		break;
> +	default:
> +		ctrl_err(ctrl, "Not a valid state on slot %s\n",
> +			 slot_name(p_slot));
> +		kfree(info);
> +		break;
> +	}
> +}

handle_link_up_event() and handle_link_down_event() are almost the same,
what about use like:
handle_link_state_change_event(p_slot, event) to reuse the the common code ?


> +
>  static void interrupt_event_handler(struct work_struct *work)
>  {
>  	struct event_info *info = container_of(work, struct event_info, work);
> @@ -468,6 +583,12 @@ static void interrupt_event_handler(struct work_struct *work)
>  		ctrl_dbg(ctrl, "Surprise Removal\n");
>  		handle_surprise_event(p_slot);
>  		break;
> +	case INT_LINK_UP:
> +		handle_link_up_event(p_slot);
> +		break;
> +	case INT_LINK_DOWN:
> +		handle_link_down_event(p_slot);
> +		break;
>  	default:
>  		break;
>  	}
> @@ -524,15 +645,6 @@ int pciehp_disable_slot(struct slot *p_slot)
>  	if (!p_slot->ctrl)
>  		return 1;
>  
> -	if (!HP_SUPR_RM(p_slot->ctrl)) {
> -		ret = pciehp_get_adapter_status(p_slot, &getstatus);
> -		if (ret || !getstatus) {
> -			ctrl_info(ctrl, "No adapter on slot(%s)\n",
> -				  slot_name(p_slot));
> -			return -ENODEV;
> -		}
> -	}
> -
>  	if (MRL_SENS(p_slot->ctrl)) {
>  		ret = pciehp_get_latch_status(p_slot, &getstatus);
>  		if (ret || getstatus) {
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 3a5eee7..1f152f3 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -278,11 +278,6 @@ static void pcie_wait_link_active(struct controller *ctrl)
>  	__pcie_wait_link_active(ctrl, true);
>  }
>  
> -static void pcie_wait_link_not_active(struct controller *ctrl)
> -{
> -	__pcie_wait_link_active(ctrl, false);
> -}
> -
>  static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>  {
>  	u32 l;
> @@ -383,11 +378,6 @@ static int pciehp_link_enable(struct controller *ctrl)
>  	return __pciehp_link_set(ctrl, true);
>  }
>  
> -static int pciehp_link_disable(struct controller *ctrl)
> -{
> -	return __pciehp_link_set(ctrl, false);
> -}
> -
>  int pciehp_get_attention_status(struct slot *slot, u8 *status)
>  {
>  	struct controller *ctrl = slot->ctrl;
> @@ -620,13 +610,10 @@ int pciehp_power_off_slot(struct slot * slot)
>  	u16 cmd_mask;
>  	int retval;
>  
> -	/* Disable the link at first */
> -	pciehp_link_disable(ctrl);
> -	/* wait the link is down */
> -	if (ctrl->link_active_reporting)
> -		pcie_wait_link_not_active(ctrl);
> -	else
> -		msleep(1000);
> +	/*
> +	 * We do not disable the link, since a future link-up event can now
> +	 * be used to initiate hot-plug
> +	 */
>  
>  	slot_cmd = POWER_OFF;
>  	cmd_mask = PCI_EXP_SLTCTL_PCC;
> @@ -661,7 +648,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
>  
>  		detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
>  			     PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
> -			     PCI_EXP_SLTSTA_CC);
> +			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
>  		detected &= ~intr_loc;
>  		intr_loc |= detected;
>  		if (!intr_loc)
> @@ -702,6 +689,10 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
>  		ctrl->power_fault_detected = 1;
>  		pciehp_handle_power_fault(slot);
>  	}
> +
> +	if (intr_loc & PCI_EXP_SLTSTA_DLLSC)
> +		pciehp_handle_linkstate_change(slot);
> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -719,7 +710,7 @@ int pcie_enable_notification(struct controller *ctrl)
>  	 * when it is cleared in the interrupt service routine, and
>  	 * next power fault detected interrupt was notified again.
>  	 */
> -	cmd = PCI_EXP_SLTCTL_PDCE;
> +	cmd = PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_DLLSCE;
>  	if (ATTN_BUTTN(ctrl))
>  		cmd |= PCI_EXP_SLTCTL_ABPE;
>  	if (MRL_SENS(ctrl))
> @@ -751,31 +742,36 @@ static void pcie_disable_notification(struct controller *ctrl)
>  
>  /*
>   * pciehp has a 1:1 bus:slot relationship so we ultimately want a secondary
> - * bus reset of the bridge, but if the slot supports surprise removal we need
> - * to disable presence detection around the bus reset and clear any spurious
> + * bus reset of the bridge, but if the slot supports surprise removal (or
> + * link state change based hotplug), we need to disable presence detection
> + * (or link state notifications) around the bus reset and clear any spurious
>   * events after.
>   */
>  int pciehp_reset_slot(struct slot *slot, int probe)
>  {
>  	struct controller *ctrl = slot->ctrl;
> +	u16 stat_mask = 0, ctrl_mask = 0;
>  
>  	if (probe)
>  		return 0;
>  
>  	if (HP_SUPR_RM(ctrl)) {
> -		pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_PDCE);
> -		if (pciehp_poll_mode)
> -			del_timer_sync(&ctrl->poll_timer);
> +		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
> +		stat_mask |= PCI_EXP_SLTSTA_PDC;
>  	}
> +	ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
> +	stat_mask |= PCI_EXP_SLTSTA_DLLSC;
> +
> +	pcie_write_cmd(ctrl, 0, ctrl_mask);
> +	if (pciehp_poll_mode)
> +		del_timer_sync(&ctrl->poll_timer);
>  
>  	pci_reset_bridge_secondary_bus(ctrl->pcie->port);
>  
> -	if (HP_SUPR_RM(ctrl)) {
> -		pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_PDC);
> -		pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PDCE, PCI_EXP_SLTCTL_PDCE);
> -		if (pciehp_poll_mode)
> -			int_poll_timeout(ctrl->poll_timer.data);
> -	}
> +	pciehp_writew(ctrl, PCI_EXP_SLTSTA, stat_mask);
> +	pcie_write_cmd(ctrl, ctrl_mask, ctrl_mask);
> +	if (pciehp_poll_mode)
> +		int_poll_timeout(ctrl->poll_timer.data);
>  
>  	return 0;
>  }
> 


-- 
Thanks!
Yijing

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Rajat Jain]

On 12/05/2013 01:07 AM, Yijing Wang wrote:
> 
> handle_link_up_event() and handle_link_down_event() are almost the same,
> what about use like:
> handle_link_state_change_event(p_slot, event) to reuse the the common code ?
> 
>

Sure, I can combine both of them to make it look more like this. Let me know
it this looks all right.

static void handle_link_event(struct slot *p_slot, unsigned int req)
{
	struct controller *ctrl = p_slot->ctrl;
	struct power_work_info *info;

	info = kmalloc(sizeof(*info), GFP_KERNEL);
	if (!info) {
		ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
			 __func__);
		return;
	}
	info->p_slot = p_slot;
	info->req = req;
	INIT_WORK(&info->work, pciehp_power_thread);

	switch (p_slot->state) {
	case BLINKINGON_STATE:
	case BLINKINGOFF_STATE:
		cancel_delayed_work(&p_slot->work);
		/* Fall through */
	case STATIC_STATE:
		if (req == ENABLE_REQ)
			p_slot->state = POWERON_STATE;
		else
			p_slot->state = POWEROFF_STATE;

		queue_work(p_slot->wq, &info->work);
		break;
	case POWERON_STATE:
		if (req == ENABLE_REQ) {
			ctrl_info(ctrl,
				  "Link Up ignored on slot(%s): already powering on\n",
				  slot_name(p_slot));
			kfree(info);
		} else {
			ctrl_info(ctrl,
				  "Link Down queued on slot(%s): currently getting powered on\n",
				  slot_name(p_slot));
			p_slot->state = POWEROFF_STATE;
			queue_work(p_slot->wq, &info->work);
		}
		break;
	case POWEROFF_STATE:
		if (req == ENABLE_REQ) {
			ctrl_info(ctrl,
				  "Link Up queued on slot(%s): currently getting powered off\n",
				  slot_name(p_slot));
			p_slot->state = POWERON_STATE;
			queue_work(p_slot->wq, &info->work);
		} else {
			ctrl_info(ctrl,
				  "Link Down ignored on slot(%s): already powering off\n",
				  slot_name(p_slot));
			kfree(info);
		}
		break;
	default:
		ctrl_err(ctrl, "Not a valid state on slot(%s)\n",
			 slot_name(p_slot));
		kfree(info);
		break;
	}
}

Bjorn: was wondering if you'd able to take a look at this patchset in
this or next week some time. I'm eagerly waiting to address any comments.

Also, can you please let me know what is the protocol here? Should
I resubmit just the "v3" of this patch? Or bump up the version of all
the patches in the patchset to "v3" and resubmit them all?

Thanks & Best Regards,

Rajat

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Bjorn Helgaas]

[+cc Yinghai]

On Tue, Dec 03, 2013 at 02:32:46PM -0800, Rajat Jain wrote:
> A lot of systems do not have the fancy buttons and LEDs, and instead
> want to rely only on the Link state change events to drive the hotplug
> and removal state machinery.
> (http://www.spinics.net/lists/hotplug/msg05802.html)
> 
> This patch adds support for that functionality. Here are the details
> about the patch itself:
> 
> * Define and use interrupt events for linkup / linkdown.

This seems like a reasonable idea.

In the ExpressCard Standard (Rel 2.0, Feb 2009), Section 6.3.1 and Figure
6-2 talk about a "Link Detect" or "Link Train Detected" interrupt being
used to trigger the ACPI hotplug flow for a non-PCIe-aware OS.  I'm not
sure what interrupt this refers to.  The Slot Control Data Link Layer State
Changed interrupt (which you're using) sounds similar, but my guess is that
"Link Detect" is a generic term for chipset-specific functionality to
generate an SMI for hotplug events.

But then Section 6.3.1.1 suggests that a PCIe-aware OS would use "Presence
Detect Changed" instead.  I don't know why a different mechanism would be
suggested, although DLLSC was added after PCIe 1.0, so older hardware
wouldn't have a PCIe-standard link detection mechanism.

In any event, I think having the Link Status Data Link Layer Link Active
bit set would certainly imply that a device is present, so it seems
reasonable to use DLLLA in addition to Presence Detect State.

> * Introduce the functions to handle link-up and link-down events and
>   queue the work in the slot->wq to be processed by pciehp_power_thread
> 
> * The current code bails out from device removal, if an adapter is not
>   detected. That is not right, because if an adapter is not present at
>   all, it provides all the more reason to REMOVE the device. This is
>   specially a problem for link state hot-plug, because some ports use
>   in band mechanism to detect the presence detection. Thus when link
>   goes down, presence detect also goes down. We _want_ that the devices
>   should be removed in this case.
> 
> * The current pciehp driver disabled the link in case of a hot-removal.
>   Since for link change based hot-plug to work, we need that enabled,
>   hence make sure to not disable the link permanently if link state
>   based hot-plug is to be used. Also have to remove
>   pciehp_link_disable() and pcie_wait_link_not_active() static functions
>   since they are not being used anywhere else.
> 
> * pciehp_reset_slot - reset of secondary bus may cause undesirable
>   spurious link notifications. Thus disable these around the secondary
>   bus reset.
> 
> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
> Signed-off-by: Guenter Roeck <groeck@juniper.net>
> ---
>  v2: - drop the use_link_state_events module parameter as discussed here:
>        http://marc.info/?t=138513966800006&r=1&w=2
>      - removed the static functions left unused after this patch.
>      - make the pciehp_handle_linkstate_change() return void.
>      - dropped the "RFC" from subject, and added Guenter's signature
> 
>  drivers/pci/hotplug/pciehp.h      |    3 +
>  drivers/pci/hotplug/pciehp_ctrl.c |  130 ++++++++++++++++++++++++++++++++++---
>  drivers/pci/hotplug/pciehp_hpc.c  |   56 ++++++++--------
>  3 files changed, 150 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index fc322ed..353edda 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -110,6 +110,8 @@ struct controller {
>  #define INT_BUTTON_PRESS		7
>  #define INT_BUTTON_RELEASE		8
>  #define INT_BUTTON_CANCEL		9
> +#define INT_LINK_UP			10
> +#define INT_LINK_DOWN			11
>  
>  #define STATIC_STATE			0
>  #define BLINKINGON_STATE		1
> @@ -133,6 +135,7 @@ u8 pciehp_handle_attention_button(struct slot *p_slot);
>  u8 pciehp_handle_switch_change(struct slot *p_slot);
>  u8 pciehp_handle_presence_change(struct slot *p_slot);
>  u8 pciehp_handle_power_fault(struct slot *p_slot);
> +void pciehp_handle_linkstate_change(struct slot *p_slot);
>  int pciehp_configure_device(struct slot *p_slot);
>  int pciehp_unconfigure_device(struct slot *p_slot);
>  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 38f0186..4c2544c 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -150,6 +150,27 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
>  	return 1;
>  }
>  
> +void pciehp_handle_linkstate_change(struct slot *p_slot)
> +{
> +	u32 event_type;
> +	struct controller *ctrl = p_slot->ctrl;
> +
> +	/* Link Status Change */
> +	ctrl_dbg(ctrl, "Data Link Layer State change\n");
> +
> +	if (pciehp_check_link_active(ctrl)) {
> +		ctrl_info(ctrl, "slot(%s): Link Up event\n",
> +			  slot_name(p_slot));
> +		event_type = INT_LINK_UP;
> +	} else {
> +		ctrl_info(ctrl, "slot(%s): Link Down event\n",
> +			  slot_name(p_slot));
> +		event_type = INT_LINK_DOWN;
> +	}
> +
> +	queue_interrupt_event(p_slot, event_type);
> +}
> +
>  /* The following routines constitute the bulk of the
>     hotplug controller logic
>   */
> @@ -442,6 +463,100 @@ static void handle_surprise_event(struct slot *p_slot)
>  	queue_work(p_slot->wq, &info->work);
>  }
>  
> +/*
> + * Note: This function must be called with slot->lock held
> + */
> +static void handle_link_up_event(struct slot *p_slot)
> +{
> +	struct controller *ctrl = p_slot->ctrl;
> +	struct power_work_info *info;
> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
> +			 __func__);
> +		return;
> +	}
> +	info->p_slot = p_slot;
> +	INIT_WORK(&info->work, pciehp_power_thread);
> +
> +	switch (p_slot->state) {
> +	case BLINKINGON_STATE:
> +	case BLINKINGOFF_STATE:
> +		cancel_delayed_work(&p_slot->work);
> +		/* Fall through */
> +	case STATIC_STATE:
> +		p_slot->state = POWERON_STATE;
> +		queue_work(p_slot->wq, &info->work);
> +		break;
> +	case POWERON_STATE:
> +		ctrl_info(ctrl,
> +			  "Link Up event ignored on slot(%s): already powering on\n",
> +			  slot_name(p_slot));
> +		kfree(info);
> +		break;
> +	case POWEROFF_STATE:
> +		ctrl_info(ctrl,
> +			  "Link Up event queued on slot(%s): currently getting powered off\n",
> +			  slot_name(p_slot));
> +		p_slot->state = POWERON_STATE;
> +		queue_work(p_slot->wq, &info->work);
> +		break;
> +	default:
> +		ctrl_err(ctrl, "Not a valid state on slot(%s)\n",
> +			 slot_name(p_slot));
> +		kfree(info);
> +		break;
> +	}
> +}
> +
> +/*
> + * Note: This function must be called with slot->lock held
> + */
> +static void handle_link_down_event(struct slot *p_slot)
> +{
> +	struct controller *ctrl = p_slot->ctrl;
> +	struct power_work_info *info;
> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
> +			 __func__);
> +		return;
> +	}
> +	info->p_slot = p_slot;
> +	INIT_WORK(&info->work, pciehp_power_thread);
> +
> +	switch (p_slot->state) {
> +	case BLINKINGON_STATE:
> +	case BLINKINGOFF_STATE:
> +		cancel_delayed_work(&p_slot->work);
> +		/* Fall through */
> +	case STATIC_STATE:
> +		p_slot->state = POWEROFF_STATE;
> +		queue_work(p_slot->wq, &info->work);
> +		break;
> +	case POWEROFF_STATE:
> +		ctrl_info(ctrl,
> +			  "Link Down event ignored on slot(%s): already powering off\n",
> +			  slot_name(p_slot));
> +		kfree(info);
> +		break;
> +	case POWERON_STATE:
> +		ctrl_info(ctrl,
> +			  "Link Down event queued on slot(%s): currently getting powered on\n",
> +			  slot_name(p_slot));
> +		p_slot->state = POWEROFF_STATE;
> +		queue_work(p_slot->wq, &info->work);
> +		break;
> +	default:
> +		ctrl_err(ctrl, "Not a valid state on slot %s\n",
> +			 slot_name(p_slot));
> +		kfree(info);
> +		break;
> +	}
> +}
> +
>  static void interrupt_event_handler(struct work_struct *work)
>  {
>  	struct event_info *info = container_of(work, struct event_info, work);
> @@ -468,6 +583,12 @@ static void interrupt_event_handler(struct work_struct *work)
>  		ctrl_dbg(ctrl, "Surprise Removal\n");
>  		handle_surprise_event(p_slot);
>  		break;
> +	case INT_LINK_UP:
> +		handle_link_up_event(p_slot);
> +		break;
> +	case INT_LINK_DOWN:
> +		handle_link_down_event(p_slot);
> +		break;
>  	default:
>  		break;
>  	}
> @@ -524,15 +645,6 @@ int pciehp_disable_slot(struct slot *p_slot)
>  	if (!p_slot->ctrl)
>  		return 1;
>  
> -	if (!HP_SUPR_RM(p_slot->ctrl)) {
> -		ret = pciehp_get_adapter_status(p_slot, &getstatus);
> -		if (ret || !getstatus) {
> -			ctrl_info(ctrl, "No adapter on slot(%s)\n",
> -				  slot_name(p_slot));
> -			return -ENODEV;
> -		}
> -	}
> -

This seems like a candidate for splitting into a separate patch.

>  	if (MRL_SENS(p_slot->ctrl)) {
>  		ret = pciehp_get_latch_status(p_slot, &getstatus);
>  		if (ret || getstatus) {
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 3a5eee7..1f152f3 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -278,11 +278,6 @@ static void pcie_wait_link_active(struct controller *ctrl)
>  	__pcie_wait_link_active(ctrl, true);
>  }
>  
> -static void pcie_wait_link_not_active(struct controller *ctrl)
> -{
> -	__pcie_wait_link_active(ctrl, false);
> -}
> -
>  static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>  {
>  	u32 l;
> @@ -383,11 +378,6 @@ static int pciehp_link_enable(struct controller *ctrl)
>  	return __pciehp_link_set(ctrl, true);
>  }
>  
> -static int pciehp_link_disable(struct controller *ctrl)
> -{
> -	return __pciehp_link_set(ctrl, false);
> -}
> -
>  int pciehp_get_attention_status(struct slot *slot, u8 *status)
>  {
>  	struct controller *ctrl = slot->ctrl;
> @@ -620,13 +610,10 @@ int pciehp_power_off_slot(struct slot * slot)
>  	u16 cmd_mask;
>  	int retval;
>  
> -	/* Disable the link at first */
> -	pciehp_link_disable(ctrl);
> -	/* wait the link is down */
> -	if (ctrl->link_active_reporting)
> -		pcie_wait_link_not_active(ctrl);
> -	else
> -		msleep(1000);

2debd9289997 ("PCI: pciehp: Disable/enable link during slot power off/on")
added this code that disables the link to fix problems.  So we need to make
sure we don't reintroduce those problems by leaving the link enabled.

One problem was spurious "card present/not present" messages.  I suspect
that topology has an attention button, and I'm not sure it makes sense to
enable the presence detect interrupt in that case.  As far as I can tell,
if there's an attention button, the OS should not do anything on card
insertion or removal; it should only do something when the attention button
is pressed.  So maybe we can deal with the message issue that way.

The changelog also hints that disabling the link might be needed to allow a
repeater to be reset, but I don't know the details.  I don't remember any
spec language that requires the link to be disabled on hotplug; maybe this
is a platform-specific quirk.

This patch is pretty large and if it could be split up a bit, especially
this particular part, it might help Yinghai verify that his system keeps
working.

Bjorn

> +	/*
> +	 * We do not disable the link, since a future link-up event can now
> +	 * be used to initiate hot-plug
> +	 */
>  
>  	slot_cmd = POWER_OFF;
>  	cmd_mask = PCI_EXP_SLTCTL_PCC;
> @@ -661,7 +648,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
>  
>  		detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
>  			     PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
> -			     PCI_EXP_SLTSTA_CC);
> +			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
>  		detected &= ~intr_loc;
>  		intr_loc |= detected;
>  		if (!intr_loc)
> @@ -702,6 +689,10 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
>  		ctrl->power_fault_detected = 1;
>  		pciehp_handle_power_fault(slot);
>  	}
> +
> +	if (intr_loc & PCI_EXP_SLTSTA_DLLSC)
> +		pciehp_handle_linkstate_change(slot);
> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -719,7 +710,7 @@ int pcie_enable_notification(struct controller *ctrl)
>  	 * when it is cleared in the interrupt service routine, and
>  	 * next power fault detected interrupt was notified again.
>  	 */
> -	cmd = PCI_EXP_SLTCTL_PDCE;
> +	cmd = PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_DLLSCE;
>  	if (ATTN_BUTTN(ctrl))
>  		cmd |= PCI_EXP_SLTCTL_ABPE;
>  	if (MRL_SENS(ctrl))
> @@ -751,31 +742,36 @@ static void pcie_disable_notification(struct controller *ctrl)
>  
>  /*
>   * pciehp has a 1:1 bus:slot relationship so we ultimately want a secondary
> - * bus reset of the bridge, but if the slot supports surprise removal we need
> - * to disable presence detection around the bus reset and clear any spurious
> + * bus reset of the bridge, but if the slot supports surprise removal (or
> + * link state change based hotplug), we need to disable presence detection
> + * (or link state notifications) around the bus reset and clear any spurious
>   * events after.
>   */
>  int pciehp_reset_slot(struct slot *slot, int probe)
>  {
>  	struct controller *ctrl = slot->ctrl;
> +	u16 stat_mask = 0, ctrl_mask = 0;
>  
>  	if (probe)
>  		return 0;
>  
>  	if (HP_SUPR_RM(ctrl)) {
> -		pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_PDCE);
> -		if (pciehp_poll_mode)
> -			del_timer_sync(&ctrl->poll_timer);
> +		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
> +		stat_mask |= PCI_EXP_SLTSTA_PDC;
>  	}
> +	ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
> +	stat_mask |= PCI_EXP_SLTSTA_DLLSC;
> +
> +	pcie_write_cmd(ctrl, 0, ctrl_mask);
> +	if (pciehp_poll_mode)
> +		del_timer_sync(&ctrl->poll_timer);
>  
>  	pci_reset_bridge_secondary_bus(ctrl->pcie->port);
>  
> -	if (HP_SUPR_RM(ctrl)) {
> -		pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_PDC);
> -		pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PDCE, PCI_EXP_SLTCTL_PDCE);
> -		if (pciehp_poll_mode)
> -			int_poll_timeout(ctrl->poll_timer.data);
> -	}
> +	pciehp_writew(ctrl, PCI_EXP_SLTSTA, stat_mask);
> +	pcie_write_cmd(ctrl, ctrl_mask, ctrl_mask);
> +	if (pciehp_poll_mode)
> +		int_poll_timeout(ctrl->poll_timer.data);
>  
>  	return 0;
>  }
> -- 
> 1.7.9.5
> 

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Yinghai Lu]

On Thu, Dec 12, 2013 at 2:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Yinghai]
>
> On Tue, Dec 03, 2013 at 02:32:46PM -0800, Rajat Jain wrote:
>> A lot of systems do not have the fancy buttons and LEDs, and instead
>> want to rely only on the Link state change events to drive the hotplug
>> and removal state machinery.
>> (http://www.spinics.net/lists/hotplug/msg05802.html)

"A lot of systems", are you kidding? You count it? Rajat.

>>
>> This patch adds support for that functionality. Here are the details
>> about the patch itself:
>>
>> * Define and use interrupt events for linkup / linkdown.
>
> This seems like a reasonable idea.
>
> In the ExpressCard Standard (Rel 2.0, Feb 2009), Section 6.3.1 and Figure
> 6-2 talk about a "Link Detect" or "Link Train Detected" interrupt being
> used to trigger the ACPI hotplug flow for a non-PCIe-aware OS.  I'm not
> sure what interrupt this refers to.  The Slot Control Data Link Layer State
> Changed interrupt (which you're using) sounds similar, but my guess is that
> "Link Detect" is a generic term for chipset-specific functionality to
> generate an SMI for hotplug events.
>
> But then Section 6.3.1.1 suggests that a PCIe-aware OS would use "Presence
> Detect Changed" instead.  I don't know why a different mechanism would be
> suggested, although DLLSC was added after PCIe 1.0, so older hardware
> wouldn't have a PCIe-standard link detection mechanism.
>
> In any event, I think having the Link Status Data Link Layer Link Active
> bit set would certainly imply that a device is present, so it seems
> reasonable to use DLLLA in addition to Presence Detect State.

Yes, if Attention attention is not there and Surprise removal is supported.

>
>> * Introduce the functions to handle link-up and link-down events and
>>   queue the work in the slot->wq to be processed by pciehp_power_thread
>>
>> * The current code bails out from device removal, if an adapter is not
>>   detected. That is not right, because if an adapter is not present at
>>   all, it provides all the more reason to REMOVE the device. This is
>>   specially a problem for link state hot-plug, because some ports use
>>   in band mechanism to detect the presence detection. Thus when link
>>   goes down, presence detect also goes down. We _want_ that the devices
>>   should be removed in this case.
>>
>> * The current pciehp driver disabled the link in case of a hot-removal.
>>   Since for link change based hot-plug to work, we need that enabled,
>>   hence make sure to not disable the link permanently if link state
>>   based hot-plug is to be used. Also have to remove
>>   pciehp_link_disable() and pcie_wait_link_not_active() static functions
>>   since they are not being used anywhere else.
>>
>> * pciehp_reset_slot - reset of secondary bus may cause undesirable
>>   spurious link notifications. Thus disable these around the secondary
>>   bus reset.
>>
>> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
>> Signed-off-by: Guenter Roeck <groeck@juniper.net>
>> ---
>>  v2: - drop the use_link_state_events module parameter as discussed here:
>>        http://marc.info/?t=138513966800006&r=1&w=2
>>      - removed the static functions left unused after this patch.
>>      - make the pciehp_handle_linkstate_change() return void.
>>      - dropped the "RFC" from subject, and added Guenter's signature
>>
>>  drivers/pci/hotplug/pciehp.h      |    3 +
>>  drivers/pci/hotplug/pciehp_ctrl.c |  130 ++++++++++++++++++++++++++++++++++---
>>  drivers/pci/hotplug/pciehp_hpc.c  |   56 ++++++++--------
>>  3 files changed, 150 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
>> index fc322ed..353edda 100644
>> --- a/drivers/pci/hotplug/pciehp.h
>> +++ b/drivers/pci/hotplug/pciehp.h
>> @@ -110,6 +110,8 @@ struct controller {
>>  #define INT_BUTTON_PRESS             7
>>  #define INT_BUTTON_RELEASE           8
>>  #define INT_BUTTON_CANCEL            9
>> +#define INT_LINK_UP                  10
>> +#define INT_LINK_DOWN                        11
>>
>>  #define STATIC_STATE                 0
>>  #define BLINKINGON_STATE             1
>> @@ -133,6 +135,7 @@ u8 pciehp_handle_attention_button(struct slot *p_slot);
>>  u8 pciehp_handle_switch_change(struct slot *p_slot);
>>  u8 pciehp_handle_presence_change(struct slot *p_slot);
>>  u8 pciehp_handle_power_fault(struct slot *p_slot);
>> +void pciehp_handle_linkstate_change(struct slot *p_slot);
>>  int pciehp_configure_device(struct slot *p_slot);
>>  int pciehp_unconfigure_device(struct slot *p_slot);
>>  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 38f0186..4c2544c 100644
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -150,6 +150,27 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
>>       return 1;
>>  }
>>
>> +void pciehp_handle_linkstate_change(struct slot *p_slot)
>> +{
>> +     u32 event_type;
>> +     struct controller *ctrl = p_slot->ctrl;
>> +
>> +     /* Link Status Change */
>> +     ctrl_dbg(ctrl, "Data Link Layer State change\n");
>> +
>> +     if (pciehp_check_link_active(ctrl)) {
>> +             ctrl_info(ctrl, "slot(%s): Link Up event\n",
>> +                       slot_name(p_slot));
>> +             event_type = INT_LINK_UP;
>> +     } else {
>> +             ctrl_info(ctrl, "slot(%s): Link Down event\n",
>> +                       slot_name(p_slot));
>> +             event_type = INT_LINK_DOWN;
>> +     }
>> +
>> +     queue_interrupt_event(p_slot, event_type);
>> +}
>> +
>>  /* The following routines constitute the bulk of the
>>     hotplug controller logic
>>   */
>> @@ -442,6 +463,100 @@ static void handle_surprise_event(struct slot *p_slot)
>>       queue_work(p_slot->wq, &info->work);
>>  }
>>
>> +/*
>> + * Note: This function must be called with slot->lock held
>> + */
>> +static void handle_link_up_event(struct slot *p_slot)
>> +{
>> +     struct controller *ctrl = p_slot->ctrl;
>> +     struct power_work_info *info;
>> +
>> +     info = kmalloc(sizeof(*info), GFP_KERNEL);
>> +     if (!info) {
>> +             ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
>> +                      __func__);
>> +             return;
>> +     }
>> +     info->p_slot = p_slot;
>> +     INIT_WORK(&info->work, pciehp_power_thread);
>> +
>> +     switch (p_slot->state) {
>> +     case BLINKINGON_STATE:
>> +     case BLINKINGOFF_STATE:
>> +             cancel_delayed_work(&p_slot->work);
>> +             /* Fall through */
>> +     case STATIC_STATE:
>> +             p_slot->state = POWERON_STATE;
>> +             queue_work(p_slot->wq, &info->work);
>> +             break;
>> +     case POWERON_STATE:
>> +             ctrl_info(ctrl,
>> +                       "Link Up event ignored on slot(%s): already powering on\n",
>> +                       slot_name(p_slot));
>> +             kfree(info);
>> +             break;
>> +     case POWEROFF_STATE:
>> +             ctrl_info(ctrl,
>> +                       "Link Up event queued on slot(%s): currently getting powered off\n",
>> +                       slot_name(p_slot));
>> +             p_slot->state = POWERON_STATE;
>> +             queue_work(p_slot->wq, &info->work);
>> +             break;
>> +     default:
>> +             ctrl_err(ctrl, "Not a valid state on slot(%s)\n",
>> +                      slot_name(p_slot));
>> +             kfree(info);
>> +             break;
>> +     }
>> +}
>> +
>> +/*
>> + * Note: This function must be called with slot->lock held
>> + */
>> +static void handle_link_down_event(struct slot *p_slot)
>> +{
>> +     struct controller *ctrl = p_slot->ctrl;
>> +     struct power_work_info *info;
>> +
>> +     info = kmalloc(sizeof(*info), GFP_KERNEL);
>> +     if (!info) {
>> +             ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
>> +                      __func__);
>> +             return;
>> +     }
>> +     info->p_slot = p_slot;
>> +     INIT_WORK(&info->work, pciehp_power_thread);
>> +
>> +     switch (p_slot->state) {
>> +     case BLINKINGON_STATE:
>> +     case BLINKINGOFF_STATE:
>> +             cancel_delayed_work(&p_slot->work);
>> +             /* Fall through */
>> +     case STATIC_STATE:
>> +             p_slot->state = POWEROFF_STATE;
>> +             queue_work(p_slot->wq, &info->work);
>> +             break;
>> +     case POWEROFF_STATE:
>> +             ctrl_info(ctrl,
>> +                       "Link Down event ignored on slot(%s): already powering off\n",
>> +                       slot_name(p_slot));
>> +             kfree(info);
>> +             break;
>> +     case POWERON_STATE:
>> +             ctrl_info(ctrl,
>> +                       "Link Down event queued on slot(%s): currently getting powered on\n",
>> +                       slot_name(p_slot));
>> +             p_slot->state = POWEROFF_STATE;
>> +             queue_work(p_slot->wq, &info->work);
>> +             break;
>> +     default:
>> +             ctrl_err(ctrl, "Not a valid state on slot %s\n",
>> +                      slot_name(p_slot));
>> +             kfree(info);
>> +             break;
>> +     }
>> +}
>> +
>>  static void interrupt_event_handler(struct work_struct *work)
>>  {
>>       struct event_info *info = container_of(work, struct event_info, work);
>> @@ -468,6 +583,12 @@ static void interrupt_event_handler(struct work_struct *work)
>>               ctrl_dbg(ctrl, "Surprise Removal\n");
>>               handle_surprise_event(p_slot);
>>               break;
>> +     case INT_LINK_UP:
>> +             handle_link_up_event(p_slot);
>> +             break;
>> +     case INT_LINK_DOWN:
>> +             handle_link_down_event(p_slot);
>> +             break;
>>       default:
>>               break;
>>       }
>> @@ -524,15 +645,6 @@ int pciehp_disable_slot(struct slot *p_slot)
>>       if (!p_slot->ctrl)
>>               return 1;
>>
>> -     if (!HP_SUPR_RM(p_slot->ctrl)) {
>> -             ret = pciehp_get_adapter_status(p_slot, &getstatus);
>> -             if (ret || !getstatus) {
>> -                     ctrl_info(ctrl, "No adapter on slot(%s)\n",
>> -                               slot_name(p_slot));
>> -                     return -ENODEV;
>> -             }
>> -     }
>> -
>
> This seems like a candidate for splitting into a separate patch.
>
>>       if (MRL_SENS(p_slot->ctrl)) {
>>               ret = pciehp_get_latch_status(p_slot, &getstatus);
>>               if (ret || getstatus) {
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index 3a5eee7..1f152f3 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -278,11 +278,6 @@ static void pcie_wait_link_active(struct controller *ctrl)
>>       __pcie_wait_link_active(ctrl, true);
>>  }
>>
>> -static void pcie_wait_link_not_active(struct controller *ctrl)
>> -{
>> -     __pcie_wait_link_active(ctrl, false);
>> -}
>> -
>>  static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>>  {
>>       u32 l;
>> @@ -383,11 +378,6 @@ static int pciehp_link_enable(struct controller *ctrl)
>>       return __pciehp_link_set(ctrl, true);
>>  }
>>
>> -static int pciehp_link_disable(struct controller *ctrl)
>> -{
>> -     return __pciehp_link_set(ctrl, false);
>> -}
>> -
>>  int pciehp_get_attention_status(struct slot *slot, u8 *status)
>>  {
>>       struct controller *ctrl = slot->ctrl;
>> @@ -620,13 +610,10 @@ int pciehp_power_off_slot(struct slot * slot)
>>       u16 cmd_mask;
>>       int retval;
>>
>> -     /* Disable the link at first */
>> -     pciehp_link_disable(ctrl);
>> -     /* wait the link is down */
>> -     if (ctrl->link_active_reporting)
>> -             pcie_wait_link_not_active(ctrl);
>> -     else
>> -             msleep(1000);
>
> 2debd9289997 ("PCI: pciehp: Disable/enable link during slot power off/on")
> added this code that disables the link to fix problems.  So we need to make
> sure we don't reintroduce those problems by leaving the link enabled.
>
> One problem was spurious "card present/not present" messages.  I suspect
> that topology has an attention button, and I'm not sure it makes sense to
> enable the presence detect interrupt in that case.  As far as I can tell,
> if there's an attention button, the OS should not do anything on card
> insertion or removal; it should only do something when the attention button
> is pressed.  So maybe we can deal with the message issue that way.

Agreed.

>
> The changelog also hints that disabling the link might be needed to allow a
> repeater to be reset, but I don't know the details.  I don't remember any
> spec language that requires the link to be disabled on hotplug; maybe this
> is a platform-specific quirk.

yes, that is workaround to to reset repeater in between.

Also we can get rid of annoying aer during power off slot.

Also at least other os will turn on link after turn on the power,
as hotplug is working if BIOS have link disabled when boot system
without card inserted.

>
> This patch is pretty large and if it could be split up a bit, especially
> this particular part, it might help Yinghai verify that his system keeps
> working.

I suggest that link event handling will be enabled automatically when
attention button is not there and surprise removal is supported.
And when that enabled, should disable Present event handling.

Thanks

Yinghai

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Bjorn Helgaas]

On Thu, Dec 12, 2013 at 11:26 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Dec 12, 2013 at 2:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

>>> * Define and use interrupt events for linkup / linkdown.
>>
>> This seems like a reasonable idea.
>>
>> In the ExpressCard Standard (Rel 2.0, Feb 2009), Section 6.3.1 and Figure
>> 6-2 talk about a "Link Detect" or "Link Train Detected" interrupt being
>> used to trigger the ACPI hotplug flow for a non-PCIe-aware OS.  I'm not
>> sure what interrupt this refers to.  The Slot Control Data Link Layer State
>> Changed interrupt (which you're using) sounds similar, but my guess is that
>> "Link Detect" is a generic term for chipset-specific functionality to
>> generate an SMI for hotplug events.
>>
>> But then Section 6.3.1.1 suggests that a PCIe-aware OS would use "Presence
>> Detect Changed" instead.  I don't know why a different mechanism would be
>> suggested, although DLLSC was added after PCIe 1.0, so older hardware
>> wouldn't have a PCIe-standard link detection mechanism.
>>
>> In any event, I think having the Link Status Data Link Layer Link Active
>> bit set would certainly imply that a device is present, so it seems
>> reasonable to use DLLLA in addition to Presence Detect State.
>
> Yes, if Attention attention is not there and Surprise removal is supported.

I'm not convinced that it's a good idea to make link state support
conditional on the attention button and surprise removal bits.  That
might cover Rajat's particular case, but I don't think there's a
logical connection between link state and those bits, so making it
conditional might just make the code more complicated and less general
for no good reason.

>>> @@ -620,13 +610,10 @@ int pciehp_power_off_slot(struct slot * slot)
>>>       u16 cmd_mask;
>>>       int retval;
>>>
>>> -     /* Disable the link at first */
>>> -     pciehp_link_disable(ctrl);
>>> -     /* wait the link is down */
>>> -     if (ctrl->link_active_reporting)
>>> -             pcie_wait_link_not_active(ctrl);
>>> -     else
>>> -             msleep(1000);
>>
>> 2debd9289997 ("PCI: pciehp: Disable/enable link during slot power off/on")
>> added this code that disables the link to fix problems.  So we need to make
>> sure we don't reintroduce those problems by leaving the link enabled.
>>
>> One problem was spurious "card present/not present" messages.  I suspect
>> that topology has an attention button, and I'm not sure it makes sense to
>> enable the presence detect interrupt in that case.  As far as I can tell,
>> if there's an attention button, the OS should not do anything on card
>> insertion or removal; it should only do something when the attention button
>> is pressed.  So maybe we can deal with the message issue that way.
>
> Agreed.
>
>>
>> The changelog also hints that disabling the link might be needed to allow a
>> repeater to be reset, but I don't know the details.  I don't remember any
>> spec language that requires the link to be disabled on hotplug; maybe this
>> is a platform-specific quirk.
>
> yes, that is workaround to to reset repeater in between.

What does the "other OS" do about this repeater?  Did you verify that
it disables the link when powering off the slot?  If we were smarter
about presence detect, I wonder if that would be enough.

> Also we can get rid of annoying aer during power off slot.

I don't know the details of this issue.  It may be possible to avoid
the AER in some way other than turning off the link.

> Also at least other os will turn on link after turn on the power,
> as hotplug is working if BIOS have link disabled when boot system
> without card inserted.

If the link is disabled after turning on power, it certainly makes
sense to me to enable the link.  I don't think Rajat changed the
pciehp_power_on_slot() path, so this part should still work.

> I suggest that link event handling will be enabled automatically when
> attention button is not there and surprise removal is supported.
> And when that enabled, should disable Present event handling.

I think it should be as simple as possible, i.e., if we have an
attention button, ignore presence detect.  If we make it more
complicated that necessary, we run the risk of enforcing assumptions
that may not hold in the future.

Bjorn

RE: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Rajat Jain]

Hello folks,

Firstly, thanks a lot for taking a look at my patch set.

> 
> On Thu, Dec 12, 2013 at 11:26 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Thu, Dec 12, 2013 at 2:44 PM, Bjorn Helgaas <bhelgaas@google.com>
> wrote:
> 
> >>> * Define and use interrupt events for linkup / linkdown.
> >>
> >> This seems like a reasonable idea.
> >>
> >> In the ExpressCard Standard (Rel 2.0, Feb 2009), Section 6.3.1 and
> >> Figure
> >> 6-2 talk about a "Link Detect" or "Link Train Detected" interrupt
> >> being used to trigger the ACPI hotplug flow for a non-PCIe-aware OS.
> >> I'm not sure what interrupt this refers to.  The Slot Control Data
> >> Link Layer State Changed interrupt (which you're using) sounds
> >> similar, but my guess is that "Link Detect" is a generic term for
> >> chipset-specific functionality to generate an SMI for hotplug events.
> >>
> >> But then Section 6.3.1.1 suggests that a PCIe-aware OS would use
> >> "Presence Detect Changed" instead.  I don't know why a different
> >> mechanism would be suggested, although DLLSC was added after PCIe
> >> 1.0, so older hardware wouldn't have a PCIe-standard link detection
> mechanism.
> >>
> >> In any event, I think having the Link Status Data Link Layer Link
> >> Active bit set would certainly imply that a device is present, so it
> >> seems reasonable to use DLLLA in addition to Presence Detect State.
> >
> > Yes, if Attention attention is not there and Surprise removal is
> supported.
> 
> I'm not convinced that it's a good idea to make link state support
> conditional on the attention button and surprise removal bits.  That
> might cover Rajat's particular case, but I don't think there's a logical
> connection between link state and those bits, so making it conditional
> might just make the code more complicated and less general for no good
> reason.

Ok. 

> 
> >>> @@ -620,13 +610,10 @@ int pciehp_power_off_slot(struct slot * slot)
> >>>       u16 cmd_mask;
> >>>       int retval;
> >>>
> >>> -     /* Disable the link at first */
> >>> -     pciehp_link_disable(ctrl);
> >>> -     /* wait the link is down */
> >>> -     if (ctrl->link_active_reporting)
> >>> -             pcie_wait_link_not_active(ctrl);
> >>> -     else
> >>> -             msleep(1000);

I'll separate this out as a separate patch. 

> >>
> >> 2debd9289997 ("PCI: pciehp: Disable/enable link during slot power
> >> off/on") added this code that disables the link to fix problems.  So
> >> we need to make sure we don't reintroduce those problems by leaving
> the link enabled.
> >>
> >> One problem was spurious "card present/not present" messages.  I
> >> suspect that topology has an attention button, and I'm not sure it
> >> makes sense to enable the presence detect interrupt in that case.  As
> >> far as I can tell, if there's an attention button, the OS should not
> >> do anything on card insertion or removal; it should only do something
> >> when the attention button is pressed.  So maybe we can deal with the
> message issue that way.
> >
> > Agreed.

Ok. I'll disable the presence detect interrupt when attention button is present. Note that however, if the link comes up or goes down without the user pressing the attention button, the device will be added or removed. It may be a non-issue, because I think ultimately all the attention button press does during device addition, is to power on the slot to bring the Link up. In this case since the link is already Up, there really isn't anything else the SW would or could do on attention button press. 

Also, I think the device removal should _always_ be initiated (if not done already) whenever the Link goes down for any reason (irrespective of whether the attention button is implemented or not, or whether the surprise events are supported or not). I think this is logical as it makes no sense for the software to think the device is accessible when in reality it is not. In fact I think we should also remove the checks in pciehp_disable_slot() that ensure that the adapter should be populated and latch should be closed.


> >
> >>
> >> The changelog also hints that disabling the link might be needed to
> >> allow a repeater to be reset, but I don't know the details.  I don't
> >> remember any spec language that requires the link to be disabled on
> >> hotplug; maybe this is a platform-specific quirk.
> >
> > yes, that is workaround to to reset repeater in between.

I guess my question is what breaks if we don't disable the link on a hot-unplug? And if it is a platform specific problem, may be it should be dealt with in a platform specific way? I guess, it would become more clearer once I split out the patch, and you could just test NOT disabling the link while unplugging.

> 
> What does the "other OS" do about this repeater?  Did you verify that it
> disables the link when powering off the slot?  If we were smarter about
> presence detect, I wonder if that would be enough.
> 
> > Also we can get rid of annoying aer during power off slot.
> 
> I don't know the details of this issue.  It may be possible to avoid the
> AER in some way other than turning off the link.

Sorry, I could not understand what we are talking about here. I'd appreciate if you could elaborate if you see this as a problem related to the patch?

> 
> > Also at least other os will turn on link after turn on the power, as
> > hotplug is working if BIOS have link disabled when boot system without
> > card inserted.
> 
> If the link is disabled after turning on power, it certainly makes sense
> to me to enable the link.  I don't think Rajat changed the
> pciehp_power_on_slot() path, so this part should still work.

Yes, I think so.

> 
> > I suggest that link event handling will be enabled automatically when
> > attention button is not there and surprise removal is supported.
> > And when that enabled, should disable Present event handling.
> 
> I think it should be as simple as possible, i.e., if we have an
> attention button, ignore presence detect.  If we make it more
> complicated that necessary, we run the risk of enforcing assumptions
> that may not hold in the future.

Once again: the way I interpret this is:
* Always enable Link events.
* Disable presence events if attention button is present.


Thanks,

Rajat

> 
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org More majordomo info
> at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Bjorn Helgaas]

On Fri, Dec 13, 2013 at 12:04 PM, Rajat Jain <rajatjain@juniper.net> wrote:
>> On Thu, Dec 12, 2013 at 11:26 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > On Thu, Dec 12, 2013 at 2:44 PM, Bjorn Helgaas <bhelgaas@google.com>

> Also, I think the device removal should _always_ be initiated (if not done already) whenever the Link goes down for any reason (irrespective of whether the attention button is implemented or not, or whether the surprise events are supported or not). I think this is logical as it makes no sense for the software to think the device is accessible when in reality it is not. In fact I think we should also remove the checks in pciehp_disable_slot() that ensure that the adapter should be populated and latch should be closed.

I agree.

>> What does the "other OS" do about this repeater?  Did you verify that it
>> disables the link when powering off the slot?  If we were smarter about
>> presence detect, I wonder if that would be enough.
>>
>> > Also we can get rid of annoying aer during power off slot.
>>
>> I don't know the details of this issue.  It may be possible to avoid the
>> AER in some way other than turning off the link.
>
> Sorry, I could not understand what we are talking about here. I'd appreciate if you could elaborate if you see this as a problem related to the patch?

I assume Yinghai means that when we power off the slot, if AER is
still enabled, it may report errors caused by the link going down.
PCIe r3.0 sec. 3.2.1 says some of these cases must not be considered
errors if the link is disabled, the port is associated with a
hot-pluggable slot, etc.  Maybe his platform doesn't follow those
rules, or maybe there's some other AER error that's not covered by
them.

It's related to your patch because you are removing the link disable,
and Yinghai says that if we leave the link enabled, he gets unwanted
AER errors when powering off the slot.  Sorry if this was all obvious
to you.  I don't know any more details.  It'd be nice to know the
exact AER errors and the PCIe capability info.  Then we might be able
to figure out a way to leave the link enabled while still suppressing
the AER errors.

> Once again: the way I interpret this is:
> * Always enable Link events.
> * Disable presence events if attention button is present.

That sounds like a good plan to me.

I'm also dubious about this use of presence detect:

    pciehp_power_thread
      pciehp_enable_slot
        pciehp_get_adapter_status
          pciehp_readw(ctrl, PCI_EXP_SLTSTA, &slot_status)
          *status = !!(slot_status & PCI_EXP_SLTSTA_PDS)
        board_added
          pciehp_power_on_slot

because we apparently look at PCI_EXP_SLTSTA_PDS when the slot is
powered off.  Only in-band presence detection is required by spec, and
in-band detection only works when power is applied.  So I think this
pciehp_get_adapter_status() call should probably just be removed.

Bjorn

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Yinghai Lu]

On Fri, Dec 13, 2013 at 1:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Dec 13, 2013 at 12:04 PM, Rajat Jain <rajatjain@juniper.net> wrote:
>
>> Once again: the way I interpret this is:
>> * Always enable Link events.
>> * Disable presence events if attention button is present.
>
> That sounds like a good plan to me.

How about Diag_Reset from MPT2SAS and others?
link could up and down

Yinghai

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Guenter Roeck]

On Fri, Dec 13, 2013 at 05:58:08PM -0800, Yinghai Lu wrote:
> On Fri, Dec 13, 2013 at 1:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Fri, Dec 13, 2013 at 12:04 PM, Rajat Jain <rajatjain@juniper.net> wrote:
> >
> >> Once again: the way I interpret this is:
> >> * Always enable Link events.
> >> * Disable presence events if attention button is present.
> >
> > That sounds like a good plan to me.
> 
> How about Diag_Reset from MPT2SAS and others?
> link could up and down
> 

Good question. Another question is how this would play together
with AER functions, specifically link_reset and slot_reset.

In this context ... is it possible that the link_reset function from struct
pci_error_handlers is never called, or am I missing something ?

Guenter

RE: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Rajat Jain]

Hello,


> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> 
> On Fri, Dec 13, 2013 at 12:04 PM, Rajat Jain <rajatjain@juniper.net>
> wrote:
> >> On Thu, Dec 12, 2013 at 11:26 PM, Yinghai Lu <yinghai@kernel.org>
> wrote:
> >> > On Thu, Dec 12, 2013 at 2:44 PM, Bjorn Helgaas
> >> > <bhelgaas@google.com>
> 
> > Also, I think the device removal should _always_ be initiated (if not
> done already) whenever the Link goes down for any reason (irrespective
> of whether the attention button is implemented or not, or whether the
> surprise events are supported or not). I think this is logical as it
> makes no sense for the software to think the device is accessible when
> in reality it is not. In fact I think we should also remove the checks
> in pciehp_disable_slot() that ensure that the adapter should be
> populated and latch should be closed.
> 
> I agree.
> 
> >> What does the "other OS" do about this repeater?  Did you verify that
> >> it disables the link when powering off the slot?  If we were smarter
> >> about presence detect, I wonder if that would be enough.
> >>
> >> > Also we can get rid of annoying aer during power off slot.
> >>
> >> I don't know the details of this issue.  It may be possible to avoid
> >> the AER in some way other than turning off the link.
> >
> > Sorry, I could not understand what we are talking about here. I'd
> appreciate if you could elaborate if you see this as a problem related
> to the patch?
> 
> I assume Yinghai means that when we power off the slot, if AER is still
> enabled, it may report errors caused by the link going down.
> PCIe r3.0 sec. 3.2.1 says some of these cases must not be considered
> errors if the link is disabled, the port is associated with a hot-
> pluggable slot, etc.  Maybe his platform doesn't follow those rules, or
> maybe there's some other AER error that's not covered by them.
> 
> It's related to your patch because you are removing the link disable,
> and Yinghai says that if we leave the link enabled, he gets unwanted AER
> errors when powering off the slot.  Sorry if this was all obvious to
> you.  I don't know any more details.  It'd be nice to know the exact AER
> errors and the PCIe capability info.  Then we might be able to figure
> out a way to leave the link enabled while still suppressing the AER
> errors.

Thanks for the explanation. I understand now.

Yinghai: would you be able to give any details on what AER errors were seen, and a dump of all of the capabilities? 

> 
> > Once again: the way I interpret this is:
> > * Always enable Link events.
> > * Disable presence events if attention button is present.
> 
> That sounds like a good plan to me.


Of course, this'll be done only if the link state change notification capability is present, otherwise I'll leave it to the current default state :-) 

Just FYI: Today, I once again used a truth table to convince myself that all possible combinations of (attention button support, surprise event support, power controller support) will work fine and if there is going to be a difference in behavior before and after this change. It looks the user experience is going to be exactly the same except for the following 2 which also seem to be fine: 

a) If a port has none of them: Attention button, Surprise event support, power controller. Then the current behavior is that it can only be added by doing "echo 1 > /sys/...../power". After this change, no need to do this. The device will be automatically added as soon as the link comes up (which would come up as soon as it is plugged in, since there is no power controller).

b) If a port has all of them: Attention button, Surprise event support, power controller. Then the current behavior is that it shall be enabled as soon as it is plugged in. But with this change the user has to push the attention button to enable it. Which may be fine, because we just seem to have said that the attention button presence indicates that it takes priority over everything (including the surprise bit).

> 
> I'm also dubious about this use of presence detect:
> 
>     pciehp_power_thread
>       pciehp_enable_slot
>         pciehp_get_adapter_status
>           pciehp_readw(ctrl, PCI_EXP_SLTSTA, &slot_status)
>           *status = !!(slot_status & PCI_EXP_SLTSTA_PDS)
>         board_added
>           pciehp_power_on_slot
> 
> because we apparently look at PCI_EXP_SLTSTA_PDS when the slot is
> powered off.  Only in-band presence detection is required by spec, and
> in-band detection only works when power is applied.  So I think this
> pciehp_get_adapter_status() call should probably just be removed.

Hummm, right, understood. However, then I think we also need cobbling in pciehp_probe(), pciehp_resume() because there also I see the same problem:

        /* Check if slot is occupied */
        slot = ctrl->slot;
        pciehp_get_adapter_status(slot, &occupied);
        pciehp_get_power_status(slot, &poweron);
        if (occupied && pciehp_force) {
                mutex_lock(&slot->hotplug_lock);
                pciehp_enable_slot(slot);
                mutex_unlock(&slot->hotplug_lock);
        }

I think 1 way to overcome this may be to always turn the power on to the slot (if not powered on) in pciehp_get_adapter_status()?

In fact going by the logic of the spec, I'd think we should actually NEVER turn off the power to the slot, since present detect depends on it? And if power is Off, in-band presence detection will fail (needed for surprise hotplug). 

Nevertheless, I think this needs a separate discussion thread (did not want to tie it to this patch) since the changes shall be more, and not directly related to this link state change?

Thanks,

Rajat

> 
> Bjorn
> 

RE: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Rajat Jain]

> > >
> > >> Once again: the way I interpret this is:
> > >> * Always enable Link events.
> > >> * Disable presence events if attention button is present.
> > >
> > > That sounds like a good plan to me.
> >
> > How about Diag_Reset from MPT2SAS and others?
> > link could up and down
> >

I am assuming you are referring to 

static int
_base_diag_reset(struct MPT2SAS_ADAPTER *ioc, int sleep_flag)

Which as far as I could understand would cause link to go down and come up
 again without the kernel knowing anything about it?

Please see below since I think point made by Guenter includes this. 

> 
> Good question. Another question is how this would play together with AER
> functions, specifically link_reset and slot_reset.
> 
> In this context ... is it possible that the link_reset function from
> struct pci_error_handlers is never called, or am I missing something ?
>

Very good catch. That seems to be the case, at least as far as I can see.
In fact I think this may be uncovering a more serious problem 
with current code (with no link state events used for hotplug). 

A fatal error is discovered by AER
=> Calls do_recovery()
   => reset_link()
      => Which will  reset the link at downstream / root port.

[PS: There is a clear problem the link_reset() is never broadcasted to all
The drivers, but skimming over that for now] 

In general, I guess the question is when a link goes down and back up, whether
or not we want to treat it as a hot unplug followed by a hotplug. I think there
may be cases such as AER (or the one Yinghai mentions) where we don't want to
treat it as a hotplug (see note below). And there may be cases that we
definitely want to treat it as hotplug (need link events!). Situation gets more
complex since there may be pciehp slots downstream of a link getting reset. 

It seems to me that we are saying that a mechanism is needed so that a voluntary
Link flap is NOT treated like a hotplug temporarily?

I found these:

/**
 * pcie_port_device_suspend - suspend port services associated with a PCIe port
 * @dev: PCI Express port to handle
 */
int pcie_port_device_suspend(struct device *dev)
{
        return device_for_each_child(dev, NULL, suspend_iter);
}

/**
 * pcie_port_device_suspend - resume port services associated with a PCIe port
 * @dev: PCI Express port to handle
 */
int pcie_port_device_suspend(struct device *dev)
{
        return device_for_each_child(dev, NULL, suspend_iter);
}

May be either these can be used or enhancements can be provided to disable a specific
Service on a particular port. 

Thanks,

Rajat


Notes: 
* it may not OK, if the kernel thinks the device is accessible when it is really not.
  What if during this downtime, someone tries to access the device (say userspace)?
* How do we know after the link up, that the device is really the same. 
  If during this reset, the device changed its "character", say a different class?
  I think a rescan should be mandated after every such event.
 * Do we need to unload and reload the driver after the link reset, since it can be a different device?

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Bjorn Helgaas]

On Sun, Dec 15, 2013 at 4:24 PM, Rajat Jain <rajatjain@juniper.net> wrote:
>> > >
>> > >> Once again: the way I interpret this is:
>> > >> * Always enable Link events.
>> > >> * Disable presence events if attention button is present.
>> > >
>> > > That sounds like a good plan to me.
>> >
>> > How about Diag_Reset from MPT2SAS and others?
>> > link could up and down
>> >
>
> I am assuming you are referring to
>
> static int
> _base_diag_reset(struct MPT2SAS_ADAPTER *ioc, int sleep_flag)
>
> Which as far as I could understand would cause link to go down and come up
>  again without the kernel knowing anything about it?
> ...

> In general, I guess the question is when a link goes down and back up, whether
> or not we want to treat it as a hot unplug followed by a hotplug. I think there
> may be cases such as AER (or the one Yinghai mentions) where we don't want to
> treat it as a hotplug (see note below). And there may be cases that we
> definitely want to treat it as hotplug (need link events!). Situation gets more
> complex since there may be pciehp slots downstream of a link getting reset.
>
> It seems to me that we are saying that a mechanism is needed so that a voluntary
> Link flap is NOT treated like a hotplug temporarily?
> ...

> Notes:
> * it may not OK, if the kernel thinks the device is accessible when it is really not.
>   What if during this downtime, someone tries to access the device (say userspace)?
> * How do we know after the link up, that the device is really the same.
>   If during this reset, the device changed its "character", say a different class?
>   I think a rescan should be mandated after every such event.
>  * Do we need to unload and reload the driver after the link reset, since it can be a different device?

I am quite dubious about the idea of a voluntary link flap.  If the
link goes down and comes back up, I don't see how we can make any
assumptions about what device is there.

I let Alex talk me into pciehp_reset_slot(), which disables presence
detect interrupts while resetting a device, so we already have a bit
of precedent for the idea.  But even in that case, the device could
easily come out of reset as a different device, e.g., if the reset
caused it to load updated firmware.

I would feel much better if we treated link down as a remove and did a
rescan on the link up.

Bjorn

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Guenter Roeck]

On Sun, Dec 15, 2013 at 05:18:26PM -0700, Bjorn Helgaas wrote:
> On Sun, Dec 15, 2013 at 4:24 PM, Rajat Jain <rajatjain@juniper.net> wrote:
> >> > >
> >> > >> Once again: the way I interpret this is: * Always enable Link events.
> >> > >> * Disable presence events if attention button is present.
> >> > >
> >> > > That sounds like a good plan to me.
> >> >
> >> > How about Diag_Reset from MPT2SAS and others?  link could up and down
> >> >
> >
> > I am assuming you are referring to
> >
> > static int _base_diag_reset(struct MPT2SAS_ADAPTER *ioc, int sleep_flag)
> >
> > Which as far as I could understand would cause link to go down and come up
> > again without the kernel knowing anything about it?  ...
> 
> > In general, I guess the question is when a link goes down and back up,
> > whether or not we want to treat it as a hot unplug followed by a hotplug. I
> > think there may be cases such as AER (or the one Yinghai mentions) where we
> > don't want to treat it as a hotplug (see note below). And there may be cases
> > that we definitely want to treat it as hotplug (need link events!).
> > Situation gets more complex since there may be pciehp slots downstream of a
> > link getting reset.
> >
> > It seems to me that we are saying that a mechanism is needed so that a
> > voluntary Link flap is NOT treated like a hotplug temporarily?  ...
> 
> > Notes: * it may not OK, if the kernel thinks the device is accessible when
> > it is really not.  What if during this downtime, someone tries to access the
> > device (say userspace)?  * How do we know after the link up, that the device
> > is really the same.  If during this reset, the device changed its
> > "character", say a different class?  I think a rescan should be mandated
> > after every such event.  * Do we need to unload and reload the driver after
> > the link reset, since it can be a different device?
> 
> I am quite dubious about the idea of a voluntary link flap.  If the link goes
> down and comes back up, I don't see how we can make any assumptions about what
> device is there.
> 
> I let Alex talk me into pciehp_reset_slot(), which disables presence detect
> interrupts while resetting a device, so we already have a bit of precedent for
> the idea.  But even in that case, the device could easily come out of reset as
> a different device, e.g., if the reset caused it to load updated firmware.
> 
> I would feel much better if we treated link down as a remove and did a rescan
> on the link up.
> 
Agreed. Question is if we might need some means for a driver to tell the PCIe
core about an upcoming "planned" link flap. pciehp_reset_slot() doesn't seem
to address the condition where a driver resets a connected chip by other means
than by calling pciehp_reset_slot(). Still not sure what happens when the
mpt2sas driver issues its diagnostic reset, to take Yinghai's example (or if
there would be a cleaner way to implement such a reset).

Guenter

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Bjorn Helgaas]

On Mon, Dec 16, 2013 at 10:39 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Sun, Dec 15, 2013 at 05:18:26PM -0700, Bjorn Helgaas wrote:
>> On Sun, Dec 15, 2013 at 4:24 PM, Rajat Jain <rajatjain@juniper.net> wrote:
>> >> > >
>> >> > >> Once again: the way I interpret this is: * Always enable Link events.
>> >> > >> * Disable presence events if attention button is present.
>> >> > >
>> >> > > That sounds like a good plan to me.
>> >> >
>> >> > How about Diag_Reset from MPT2SAS and others?  link could up and down
>> >> >
>> >
>> > I am assuming you are referring to
>> >
>> > static int _base_diag_reset(struct MPT2SAS_ADAPTER *ioc, int sleep_flag)
>> >
>> > Which as far as I could understand would cause link to go down and come up
>> > again without the kernel knowing anything about it?  ...
>>
>> > In general, I guess the question is when a link goes down and back up,
>> > whether or not we want to treat it as a hot unplug followed by a hotplug. I
>> > think there may be cases such as AER (or the one Yinghai mentions) where we
>> > don't want to treat it as a hotplug (see note below). And there may be cases
>> > that we definitely want to treat it as hotplug (need link events!).
>> > Situation gets more complex since there may be pciehp slots downstream of a
>> > link getting reset.
>> >
>> > It seems to me that we are saying that a mechanism is needed so that a
>> > voluntary Link flap is NOT treated like a hotplug temporarily?  ...
>>
>> > Notes: * it may not OK, if the kernel thinks the device is accessible when
>> > it is really not.  What if during this downtime, someone tries to access the
>> > device (say userspace)?  * How do we know after the link up, that the device
>> > is really the same.  If during this reset, the device changed its
>> > "character", say a different class?  I think a rescan should be mandated
>> > after every such event.  * Do we need to unload and reload the driver after
>> > the link reset, since it can be a different device?
>>
>> I am quite dubious about the idea of a voluntary link flap.  If the link goes
>> down and comes back up, I don't see how we can make any assumptions about what
>> device is there.
>>
>> I let Alex talk me into pciehp_reset_slot(), which disables presence detect
>> interrupts while resetting a device, so we already have a bit of precedent for
>> the idea.  But even in that case, the device could easily come out of reset as
>> a different device, e.g., if the reset caused it to load updated firmware.
>>
>> I would feel much better if we treated link down as a remove and did a rescan
>> on the link up.
>>
> Agreed. Question is if we might need some means for a driver to tell the PCIe
> core about an upcoming "planned" link flap. pciehp_reset_slot() doesn't seem
> to address the condition where a driver resets a connected chip by other means
> than by calling pciehp_reset_slot(). Still not sure what happens when the
> mpt2sas driver issues its diagnostic reset, to take Yinghai's example (or if
> there would be a cleaner way to implement such a reset).

In my opinion we should not add the concept of a planned link flap.
We already have pci_reset_function(), and we can probably make that
deal with link up/down events internally, so I think we should try to
use that if we can.  I think it's too much of a mess to try to support
link flaps for random driver-initiated resets that don't go through
the PCI core.

That probably means going through and identifying all the drivers we
can find that do their own internal resets, and converting them.
We'll likely miss some, since the mechanisms are driver-specific.  And
maybe there are some driver resets that think they add value over the
core's pci_reset_function() (I'm not sure what that would be, but I'm
open to discussion about it).

Bjorn

Re: [PATCH v2 2/4] pciehp: Use link change notifications for hot-plug and removal

[Guenter Roeck]

On 12/16/2013 05:14 PM, Bjorn Helgaas wrote:
> On Mon, Dec 16, 2013 at 10:39 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Sun, Dec 15, 2013 at 05:18:26PM -0700, Bjorn Helgaas wrote:
>>> On Sun, Dec 15, 2013 at 4:24 PM, Rajat Jain <rajatjain@juniper.net> wrote:
>>>>>>>
>>>>>>>> Once again: the way I interpret this is: * Always enable Link events.
>>>>>>>> * Disable presence events if attention button is present.
>>>>>>>
>>>>>>> That sounds like a good plan to me.
>>>>>>
>>>>>> How about Diag_Reset from MPT2SAS and others?  link could up and down
>>>>>>
>>>>
>>>> I am assuming you are referring to
>>>>
>>>> static int _base_diag_reset(struct MPT2SAS_ADAPTER *ioc, int sleep_flag)
>>>>
>>>> Which as far as I could understand would cause link to go down and come up
>>>> again without the kernel knowing anything about it?  ...
>>>
>>>> In general, I guess the question is when a link goes down and back up,
>>>> whether or not we want to treat it as a hot unplug followed by a hotplug. I
>>>> think there may be cases such as AER (or the one Yinghai mentions) where we
>>>> don't want to treat it as a hotplug (see note below). And there may be cases
>>>> that we definitely want to treat it as hotplug (need link events!).
>>>> Situation gets more complex since there may be pciehp slots downstream of a
>>>> link getting reset.
>>>>
>>>> It seems to me that we are saying that a mechanism is needed so that a
>>>> voluntary Link flap is NOT treated like a hotplug temporarily?  ...
>>>
>>>> Notes: * it may not OK, if the kernel thinks the device is accessible when
>>>> it is really not.  What if during this downtime, someone tries to access the
>>>> device (say userspace)?  * How do we know after the link up, that the device
>>>> is really the same.  If during this reset, the device changed its
>>>> "character", say a different class?  I think a rescan should be mandated
>>>> after every such event.  * Do we need to unload and reload the driver after
>>>> the link reset, since it can be a different device?
>>>
>>> I am quite dubious about the idea of a voluntary link flap.  If the link goes
>>> down and comes back up, I don't see how we can make any assumptions about what
>>> device is there.
>>>
>>> I let Alex talk me into pciehp_reset_slot(), which disables presence detect
>>> interrupts while resetting a device, so we already have a bit of precedent for
>>> the idea.  But even in that case, the device could easily come out of reset as
>>> a different device, e.g., if the reset caused it to load updated firmware.
>>>
>>> I would feel much better if we treated link down as a remove and did a rescan
>>> on the link up.
>>>
>> Agreed. Question is if we might need some means for a driver to tell the PCIe
>> core about an upcoming "planned" link flap. pciehp_reset_slot() doesn't seem
>> to address the condition where a driver resets a connected chip by other means
>> than by calling pciehp_reset_slot(). Still not sure what happens when the
>> mpt2sas driver issues its diagnostic reset, to take Yinghai's example (or if
>> there would be a cleaner way to implement such a reset).
>
> In my opinion we should not add the concept of a planned link flap.
> We already have pci_reset_function(), and we can probably make that
> deal with link up/down events internally, so I think we should try to
> use that if we can.  I think it's too much of a mess to try to support
> link flaps for random driver-initiated resets that don't go through
> the PCI core.
>
Perfectly fine with me.

> That probably means going through and identifying all the drivers we
> can find that do their own internal resets, and converting them.
> We'll likely miss some, since the mechanisms are driver-specific.  And

Also might be difficult - that kind of work really asks for having
the hardware available for testing.

Guenter

> maybe there are some driver resets that think they add value over the
> core's pci_reset_function() (I'm not sure what that would be, but I'm
> open to discussion about it).
>
> Bjorn
>
>