Hello,
This patch set enables the use of PCI Express link up and link down events
for Hotplug or Unplug. This is the pretty much the same patchset as v3, only
resending after rebasing on top of 3.14-rc1.
(As a side effect, this patch also fixes the bug
https://bugzilla.kernel.org/show_bug.cgi?id=65521 )
I'd appreciate if you could please review and provide me with any comments.
Thanks,
Rajat
Rajat Jain (8):
pciehp: Make check_link_active() non-static
pciehp: Use link change notifications for hot-plug and removal
pciehp: Enable link state change notifications
pciehp: Don't disable the link permanently, during removal
pciehp: Don't check for adapter or latch status while disabling
pciehp: Disabling the link notification across slot reset
pciehp: Ensure very fast hotplug events are also processed.
pciehp: Introduce hotplug_lock to serialize HP events
drivers/pci/hotplug/pciehp.h | 5 ++
drivers/pci/hotplug/pciehp_core.c | 7 +-
drivers/pci/hotplug/pciehp_ctrl.c | 149 ++++++++++++++++++++++++++++++-------
drivers/pci/hotplug/pciehp_hpc.c | 74 +++++++++---------
4 files changed, 169 insertions(+), 66 deletions(-)
--
1.7.9.5
Hello list,
My apologies for I forgot to attach the version history information of each individual patch.
Essentially the v4 is exactly same as v3, except that it is rebased on top of 3.14-rc1.
Thanks,
Rajat
> -----Original Message-----
> From: Rajat Jain [mailto:[email protected]]
> Sent: Tuesday, February 04, 2014 6:28 PM
> To: Bjorn Helgaas; Rafael J. Wysocki; Kenji Kaneshige; Alex Williamson;
> Yijing Wang; [email protected]; [email protected];
> Yinghai Lu
> Cc: Guenter Roeck; Rajat Jain; Rajat Jain
> Subject: [PATCH v4 0/8] Allow Link state changes for Hot-Plug
>
> Hello,
>
> This patch set enables the use of PCI Express link up and link down
> events for Hotplug or Unplug. This is the pretty much the same patchset
> as v3, only resending after rebasing on top of 3.14-rc1.
>
> (As a side effect, this patch also fixes the bug
> https://bugzilla.kernel.org/show_bug.cgi?id=65521 )
>
> I'd appreciate if you could please review and provide me with any
> comments.
>
> Thanks,
>
> Rajat
>
> Rajat Jain (8):
> pciehp: Make check_link_active() non-static
> pciehp: Use link change notifications for hot-plug and removal
> pciehp: Enable link state change notifications
> pciehp: Don't disable the link permanently, during removal
> pciehp: Don't check for adapter or latch status while disabling
> pciehp: Disabling the link notification across slot reset
> pciehp: Ensure very fast hotplug events are also processed.
> pciehp: Introduce hotplug_lock to serialize HP events
>
> drivers/pci/hotplug/pciehp.h | 5 ++
> drivers/pci/hotplug/pciehp_core.c | 7 +-
> drivers/pci/hotplug/pciehp_ctrl.c | 149
> ++++++++++++++++++++++++++++++-------
> drivers/pci/hotplug/pciehp_hpc.c | 74 +++++++++---------
> 4 files changed, 169 insertions(+), 66 deletions(-)
>
> --
> 1.7.9.5
>
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Feb 05, 2014 at 03:15:19AM +0000, Rajat Jain wrote:
> Hello list,
>
> My apologies for I forgot to attach the version history information of each individual patch.
>
> Essentially the v4 is exactly same as v3, except that it is rebased on top of 3.14-rc1.
>
> Thanks,
>
> Rajat
>
> > -----Original Message-----
> > From: Rajat Jain [mailto:[email protected]]
> > Sent: Tuesday, February 04, 2014 6:28 PM
> > To: Bjorn Helgaas; Rafael J. Wysocki; Kenji Kaneshige; Alex Williamson;
> > Yijing Wang; [email protected]; [email protected];
> > Yinghai Lu
> > Cc: Guenter Roeck; Rajat Jain; Rajat Jain
> > Subject: [PATCH v4 0/8] Allow Link state changes for Hot-Plug
> >
> > Hello,
> >
> > This patch set enables the use of PCI Express link up and link down
> > events for Hotplug or Unplug. This is the pretty much the same patchset
> > as v3, only resending after rebasing on top of 3.14-rc1.
> >
> > (As a side effect, this patch also fixes the bug
> > https://bugzilla.kernel.org/show_bug.cgi?id=65521 )
> >
> > I'd appreciate if you could please review and provide me with any
> > comments.
> >
> > Thanks,
> >
> > Rajat
> >
> > Rajat Jain (8):
> > pciehp: Make check_link_active() non-static
> > pciehp: Use link change notifications for hot-plug and removal
> > pciehp: Enable link state change notifications
> > pciehp: Don't disable the link permanently, during removal
> > pciehp: Don't check for adapter or latch status while disabling
> > pciehp: Disabling the link notification across slot reset
> > pciehp: Ensure very fast hotplug events are also processed.
> > pciehp: Introduce hotplug_lock to serialize HP events
> >
> > drivers/pci/hotplug/pciehp.h | 5 ++
> > drivers/pci/hotplug/pciehp_core.c | 7 +-
> > drivers/pci/hotplug/pciehp_ctrl.c | 149
> > ++++++++++++++++++++++++++++++-------
> > drivers/pci/hotplug/pciehp_hpc.c | 74 +++++++++---------
> > 4 files changed, 169 insertions(+), 66 deletions(-)
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.
Yinghai, if "Don't disable the link permanently, during removal" causes
trouble on your platforms, let us know so we can figure out how to deal
with it.
Bjorn
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 <[email protected]>
Signed-off-by: Rajat Jain <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
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.
Thank you so much once again,
Rajat
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)
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
On Mon, Feb 10, 2014 at 7:22 PM, Rajat Jain <[email protected]> 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 <[email protected]>
> Signed-off-by: Rajat Jain <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> 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
>