On Sat, Jun 16, 2018 at 09:25:00PM +0200, Lukas Wunner wrote:
> @@ -240,13 +240,19 @@ static int pciehp_probe(struct pcie_device *dev)
> }
>
> /* Check if slot is occupied */
> + mutex_lock(&slot->lock);
> pciehp_get_adapter_status(slot, &occupied);
> pciehp_get_power_status(slot, &poweron);
> - if (occupied && pciehp_force)
> - pciehp_enable_slot(slot);
> + if (pciehp_force &&
> + ((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);
This...
> /* 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);
>
> return 0;
>
> @@ -290,10 +296,14 @@ static int pciehp_resume(struct pcie_device *dev)
>
> /* Check if slot is occupied */
> pciehp_get_adapter_status(slot, &status);
> - if (status)
> - pciehp_enable_slot(slot);
> - else
> - pciehp_disable_slot(slot);
> + 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);
... and this look the same (I think there are other places as well).
Perhaps you could factor this to a helper function?
> + mutex_unlock(&slot->lock);
> +
> return 0;
> }
> #endif /* PM */
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 627e846df802..70bad847a450 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -122,22 +122,26 @@ static void remove_board(struct slot *p_slot)
> pciehp_green_led_off(p_slot);
> }
>
> +void pciehp_request(struct controller *ctrl, int action)
> +{
> + atomic_or(action, &ctrl->pending_events);
> + if (!pciehp_poll_mode)
> + irq_wake_thread(ctrl->pcie->irq, ctrl);
> +}
> +
> void pciehp_queue_pushbutton_work(struct work_struct *work)
> {
> struct slot *p_slot = container_of(work, struct slot, work.work);
> + struct controller *ctrl = p_slot->ctrl;
>
> mutex_lock(&p_slot->lock);
> switch (p_slot->state) {
> case BLINKINGOFF_STATE:
> - p_slot->state = POWEROFF_STATE;
> - mutex_unlock(&p_slot->lock);
> - pciehp_disable_slot(p_slot);
> - return;
> + pciehp_request(ctrl, DISABLE_SLOT);
> + break;
> case BLINKINGON_STATE:
> - p_slot->state = POWERON_STATE;
> - mutex_unlock(&p_slot->lock);
> - pciehp_enable_slot(p_slot);
> - return;
> + pciehp_request(ctrl, PCI_EXP_SLTSTA_PDC);
> + break;
> default:
> break;
> }
> @@ -186,16 +190,6 @@ void pciehp_handle_button_press(struct slot *p_slot)
> ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
> slot_name(p_slot));
> break;
> - case POWEROFF_STATE:
> - case POWERON_STATE:
> - /*
> - * Ignore if the slot is on power-on or power-off state;
> - * this means that the previous attention button action
> - * to hot-add or hot-remove is undergoing
> - */
> - ctrl_info(ctrl, "Slot(%s): Button ignored\n",
> - slot_name(p_slot));
> - break;
> default:
> ctrl_err(ctrl, "Slot(%s): Ignoring invalid state %#x\n",
> slot_name(p_slot), p_slot->state);
> @@ -204,6 +198,22 @@ void pciehp_handle_button_press(struct slot *p_slot)
> mutex_unlock(&p_slot->lock);
> }
>
> +void pciehp_handle_disable_request(struct slot *slot)
> +{
> + struct controller *ctrl = slot->ctrl;
> +
> + mutex_lock(&slot->lock);
> + switch (slot->state) {
> + case BLINKINGON_STATE:
> + case BLINKINGOFF_STATE:
> + cancel_delayed_work(&slot->work);
Missing break here (even though it does nothing) but if someone extends
this in future it may cause problems.
> + }
> + slot->state = POWEROFF_STATE;
> + mutex_unlock(&slot->lock);
> +
> + ctrl->request_result = pciehp_disable_slot(slot);
> +}
> +
> void pciehp_handle_link_change(struct slot *p_slot)
> {
> struct controller *ctrl = p_slot->ctrl;
> @@ -232,32 +242,6 @@ void pciehp_handle_link_change(struct slot *p_slot)
> }
> return;
> break;
> - case POWERON_STATE:
> - if (link_active) {
> - ctrl_info(ctrl, "Slot(%s): Link Up event ignored; already powering on\n",
> - slot_name(p_slot));
> - } else {
> - p_slot->state = POWEROFF_STATE;
> - mutex_unlock(&p_slot->lock);
> - ctrl_info(ctrl, "Slot(%s): Link Down event queued; currently getting powered on\n",
> - slot_name(p_slot));
> - pciehp_disable_slot(p_slot);
> - return;
> - }
> - break;
> - case POWEROFF_STATE:
> - if (link_active) {
> - p_slot->state = POWERON_STATE;
> - mutex_unlock(&p_slot->lock);
> - ctrl_info(ctrl, "Slot(%s): Link Up event queued; currently getting powered off\n",
> - slot_name(p_slot));
> - pciehp_enable_slot(p_slot);
> - return;
> - } else {
> - ctrl_info(ctrl, "Slot(%s): Link Down event ignored; already powering off\n",
> - slot_name(p_slot));
> - }
> - break;
> default:
> ctrl_err(ctrl, "Slot(%s): Ignoring invalid state %#x\n",
> slot_name(p_slot), p_slot->state);
> @@ -272,6 +256,12 @@ void pciehp_handle_presence_change(struct slot *slot)
> u8 present;
>
> mutex_lock(&slot->lock);
> + switch (slot->state) {
> + case BLINKINGON_STATE:
> + case BLINKINGOFF_STATE:
> + cancel_delayed_work(&slot->work);
Ditto.
