| Message ID | 52F9896D.7090908@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, Feb 10, 2014 at 7:22 PM, Rajat Jain <rajatxjain@gmail.com> 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@gmail.com> > Signed-off-by: Rajat Jain <rajatjain@juniper.net> > Signed-off-by: Guenter Roeck <groeck@juniper.net> > --- > 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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;