Re: [PATCH v5 8/8] Allow Link state changes for Hot-Plug

[Bjorn Helgaas]

On Mon, Feb 10, 2014 at 7:22 PM, Rajat Jain <rajatxjain@xxxxxxxxx> wrote:
> Today it is there is no protection around pciehp_enable_slot() and
> pciehp_disable_slot() to ensure that they complete before another
> hot-plug operation can be done on that particular slot.
>
> This patch introduces the slot->hotplug_lock to ensure that any
> hotplug operations (add / remove) complete before another HP event
> can begin processing on that particular slot.
>
> Signed-off-by: Rajat Jain <rajatxjain@xxxxxxxxx>
> Signed-off-by: Rajat Jain <rajatjain@xxxxxxxxxxx>
> Signed-off-by: Guenter Roeck <groeck@xxxxxxxxxxx>
> ---
> Hello Bjorn,
>
>>
>> I applied these to pci/pciehp for v3.15, thanks!  I dropped the "ret" decl
>> in 8/8, which I assume was the cause of the unused variable warning.
>>
>
> Thank you so much for accepting the patchset. The "ret" variable in 8/8
> was supposed to be used for holding the return value, so that the call to
> pciehp_green_led_off() could be moved to after releasing the lock. Here
> is the revised 8/8 patch (only a couple lines diff from v4). Please feel
> free to either use this, or just make that small change in
> pciehp_power_thread(). I was holding off sending this in case there were
> any other comments.
> ...

> v5: Remove the "unused variable" warning by using it to hold return value
>     (Thus reducing lock hold time, by moving pciehp_green_led_off() to
>      after releasing the lock)

Thanks, I folded this into my pci/pciehp branch.

> v4: same as v3, only rebased on top of 3.14-rc1
> v3: same as v2, only patch numbering changed from [4/4] to [8/8].
> v2: Same as v1, dropped the "RFC" from subject, added Guenter's signature
> v1: Initial patch
>
>  drivers/pci/hotplug/pciehp.h      |    1 +
>  drivers/pci/hotplug/pciehp_core.c |    7 ++++++-
>  drivers/pci/hotplug/pciehp_ctrl.c |   17 +++++++++++++++--
>  drivers/pci/hotplug/pciehp_hpc.c  |    1 +
>  4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index d8d0336..8a66866 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -76,6 +76,7 @@ struct slot {
>         struct hotplug_slot *hotplug_slot;
>         struct delayed_work work;       /* work for button event */
>         struct mutex lock;
> +       struct mutex hotplug_lock;
>         struct workqueue_struct *wq;
>  };
>
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 53b58de..23b4bde 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -283,8 +283,11 @@ static int pciehp_probe(struct pcie_device *dev)
>         slot = ctrl->slot;
>         pciehp_get_adapter_status(slot, &occupied);
>         pciehp_get_power_status(slot, &poweron);
> -       if (occupied && pciehp_force)
> +       if (occupied && pciehp_force) {
> +               mutex_lock(&slot->hotplug_lock);
>                 pciehp_enable_slot(slot);
> +               mutex_unlock(&slot->hotplug_lock);
> +       }
>         /* If empty slot's power status is on, turn power off */
>         if (!occupied && poweron && POWER_CTRL(ctrl))
>                 pciehp_power_off_slot(slot);
> @@ -328,10 +331,12 @@ static int pciehp_resume (struct pcie_device *dev)
>
>         /* Check if slot is occupied */
>         pciehp_get_adapter_status(slot, &status);
> +       mutex_lock(&slot->hotplug_lock);
>         if (status)
>                 pciehp_enable_slot(slot);
>         else
>                 pciehp_disable_slot(slot);
> +       mutex_unlock(&slot->hotplug_lock);
>         return 0;
>  }
>  #endif /* PM */
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 3e40ec0..fec99a1 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -293,6 +293,7 @@ static void pciehp_power_thread(struct work_struct *work)
>         struct power_work_info *info =
>                 container_of(work, struct power_work_info, work);
>         struct slot *p_slot = info->p_slot;
> +       int ret;
>
>         switch (info->req) {
>         case DISABLE_REQ:
> @@ -300,7 +301,9 @@ static void pciehp_power_thread(struct work_struct *work)
>                          "Disabling domain:bus:device=%04x:%02x:00\n",
>                          pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
>                          p_slot->ctrl->pcie->port->subordinate->number);
> +               mutex_lock(&p_slot->hotplug_lock);
>                 pciehp_disable_slot(p_slot);
> +               mutex_unlock(&p_slot->hotplug_lock);
>                 mutex_lock(&p_slot->lock);
>                 p_slot->state = STATIC_STATE;
>                 mutex_unlock(&p_slot->lock);
> @@ -310,7 +313,10 @@ static void pciehp_power_thread(struct work_struct *work)
>                          "Enabling domain:bus:device=%04x:%02x:00\n",
>                          pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
>                          p_slot->ctrl->pcie->port->subordinate->number);
> -               if (pciehp_enable_slot(p_slot))
> +               mutex_lock(&p_slot->hotplug_lock);
> +               ret = pciehp_enable_slot(p_slot);
> +               mutex_unlock(&p_slot->hotplug_lock);
> +               if (ret)
>                         pciehp_green_led_off(p_slot);
>                 mutex_lock(&p_slot->lock);
>                 p_slot->state = STATIC_STATE;
> @@ -546,6 +552,9 @@ static void interrupt_event_handler(struct work_struct *work)
>         kfree(info);
>  }
>
> +/*
> + * Note: This function must be called with slot->hotplug_lock held
> + */
>  int pciehp_enable_slot(struct slot *p_slot)
>  {
>         u8 getstatus = 0;
> @@ -584,7 +593,9 @@ int pciehp_enable_slot(struct slot *p_slot)
>         return rc;
>  }
>
> -
> +/*
> + * Note: This function must be called with slot->hotplug_lock held
> + */
>  int pciehp_disable_slot(struct slot *p_slot)
>  {
>         u8 getstatus = 0;
> @@ -617,7 +628,9 @@ int pciehp_sysfs_enable_slot(struct slot *p_slot)
>         case STATIC_STATE:
>                 p_slot->state = POWERON_STATE;
>                 mutex_unlock(&p_slot->lock);
> +               mutex_lock(&p_slot->hotplug_lock);
>                 retval = pciehp_enable_slot(p_slot);
> +               mutex_unlock(&p_slot->hotplug_lock);
>                 mutex_lock(&p_slot->lock);
>                 p_slot->state = STATIC_STATE;
>                 break;
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 55cc389..68447a1 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -686,6 +686,7 @@ static int pcie_init_slot(struct controller *ctrl)
>
>         slot->ctrl = ctrl;
>         mutex_init(&slot->lock);
> +       mutex_init(&slot->hotplug_lock);
>         INIT_DELAYED_WORK(&slot->work, pciehp_queue_pushbutton_work);
>         ctrl->slot = slot;
>         return 0;
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/