[PATCH 0/3] Trivial pci hotplug changes

[PATCH 0/3] Trivial pci hotplug changes

[Yijing Wang]

Yijing Wang (3):
  PCI: Use "slot" and "pci_slot" for struct hotplug_slot and struct
    pci_slot
  PCI: Simplify pcie_poll_cmd()
  PCI: Use usleep_range() instead of msleep() for better accuracy

 drivers/pci/hotplug/pci_hotplug_core.c |  122 ++++++++++++++++----------------
 drivers/pci/hotplug/pciehp_hpc.c       |   14 ++---
 2 files changed, 66 insertions(+), 70 deletions(-)

[PATCH 1/3] PCI: Use "slot" and "pci_slot" for struct hotplug_slot and struct pci_slot

[Yijing Wang]

Now in pci_hotplug_core.c, we randomly name a struct hotplug_slot
and a struct pci_slot. It's easy to confuse them, so let us
use "slot" for a struct hotplug_slot and "pci_slot" for a struct pci_slot.
This should no functional change.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/hotplug/pci_hotplug_core.c |  122 ++++++++++++++++----------------
 1 files changed, 61 insertions(+), 61 deletions(-)

diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 56d8486..d1fab97 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -83,12 +83,12 @@ GET_STATUS(attention_status, u8)
 GET_STATUS(latch_status, u8)
 GET_STATUS(adapter_status, u8)
 
-static ssize_t power_read_file(struct pci_slot *slot, char *buf)
+static ssize_t power_read_file(struct pci_slot *pci_slot, char *buf)
 {
 	int retval;
 	u8 value;
 
-	retval = get_power_status(slot->hotplug, &value);
+	retval = get_power_status(pci_slot->hotplug, &value);
 	if (retval)
 		return retval;
 
@@ -140,22 +140,22 @@ static struct pci_slot_attribute hotplug_slot_attr_power = {
 	.store = power_write_file
 };
 
-static ssize_t attention_read_file(struct pci_slot *slot, char *buf)
+static ssize_t attention_read_file(struct pci_slot *pci_slot, char *buf)
 {
 	int retval;
 	u8 value;
 
-	retval = get_attention_status(slot->hotplug, &value);
+	retval = get_attention_status(pci_slot->hotplug, &value);
 	if (retval)
 		return retval;
 
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t attention_write_file(struct pci_slot *slot, const char *buf,
+static ssize_t attention_write_file(struct pci_slot *pci_slot, const char *buf,
 				    size_t count)
 {
-	struct hotplug_slot_ops *ops = slot->hotplug->ops;
+	struct hotplug_slot_ops *ops = pci_slot->hotplug->ops;
 	unsigned long lattention;
 	u8 attention;
 	int retval = 0;
@@ -169,7 +169,7 @@ static ssize_t attention_write_file(struct pci_slot *slot, const char *buf,
 		goto exit;
 	}
 	if (ops->set_attention_status)
-		retval = ops->set_attention_status(slot->hotplug, attention);
+		retval = ops->set_attention_status(pci_slot->hotplug, attention);
 	module_put(ops->owner);
 
 exit:
@@ -184,12 +184,12 @@ static struct pci_slot_attribute hotplug_slot_attr_attention = {
 	.store = attention_write_file
 };
 
-static ssize_t latch_read_file(struct pci_slot *slot, char *buf)
+static ssize_t latch_read_file(struct pci_slot *pci_slot, char *buf)
 {
 	int retval;
 	u8 value;
 
-	retval = get_latch_status(slot->hotplug, &value);
+	retval = get_latch_status(pci_slot->hotplug, &value);
 	if (retval)
 		return retval;
 
@@ -201,12 +201,12 @@ static struct pci_slot_attribute hotplug_slot_attr_latch = {
 	.show = latch_read_file,
 };
 
-static ssize_t presence_read_file(struct pci_slot *slot, char *buf)
+static ssize_t presence_read_file(struct pci_slot *pci_slot, char *buf)
 {
 	int retval;
 	u8 value;
 
-	retval = get_adapter_status(slot->hotplug, &value);
+	retval = get_adapter_status(pci_slot->hotplug, &value);
 	if (retval)
 		return retval;
 
@@ -307,43 +307,43 @@ static bool has_test_file(struct pci_slot *pci_slot)
 	return false;
 }
 
-static int fs_add_slot(struct pci_slot *slot)
+static int fs_add_slot(struct pci_slot *pci_slot)
 {
 	int retval = 0;
 
 	/* Create symbolic link to the hotplug driver module */
-	pci_hp_create_module_link(slot);
+	pci_hp_create_module_link(pci_slot);
 
-	if (has_power_file(slot)) {
-		retval = sysfs_create_file(&slot->kobj,
+	if (has_power_file(pci_slot)) {
+		retval = sysfs_create_file(&pci_slot->kobj,
 					   &hotplug_slot_attr_power.attr);
 		if (retval)
 			goto exit_power;
 	}
 
-	if (has_attention_file(slot)) {
-		retval = sysfs_create_file(&slot->kobj,
+	if (has_attention_file(pci_slot)) {
+		retval = sysfs_create_file(&pci_slot->kobj,
 					   &hotplug_slot_attr_attention.attr);
 		if (retval)
 			goto exit_attention;
 	}
 
-	if (has_latch_file(slot)) {
-		retval = sysfs_create_file(&slot->kobj,
+	if (has_latch_file(pci_slot)) {
+		retval = sysfs_create_file(&pci_slot->kobj,
 					   &hotplug_slot_attr_latch.attr);
 		if (retval)
 			goto exit_latch;
 	}
 
-	if (has_adapter_file(slot)) {
-		retval = sysfs_create_file(&slot->kobj,
+	if (has_adapter_file(pci_slot)) {
+		retval = sysfs_create_file(&pci_slot->kobj,
 					   &hotplug_slot_attr_presence.attr);
 		if (retval)
 			goto exit_adapter;
 	}
 
-	if (has_test_file(slot)) {
-		retval = sysfs_create_file(&slot->kobj,
+	if (has_test_file(pci_slot)) {
+		retval = sysfs_create_file(&pci_slot->kobj,
 					   &hotplug_slot_attr_test.attr);
 		if (retval)
 			goto exit_test;
@@ -352,45 +352,45 @@ static int fs_add_slot(struct pci_slot *slot)
 	goto exit;
 
 exit_test:
-	if (has_adapter_file(slot))
-		sysfs_remove_file(&slot->kobj,
+	if (has_adapter_file(pci_slot))
+		sysfs_remove_file(&pci_slot->kobj,
 				  &hotplug_slot_attr_presence.attr);
 exit_adapter:
-	if (has_latch_file(slot))
-		sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_latch.attr);
+	if (has_latch_file(pci_slot))
+		sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_latch.attr);
 exit_latch:
-	if (has_attention_file(slot))
-		sysfs_remove_file(&slot->kobj,
+	if (has_attention_file(pci_slot))
+		sysfs_remove_file(&pci_slot->kobj,
 				  &hotplug_slot_attr_attention.attr);
 exit_attention:
-	if (has_power_file(slot))
-		sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_power.attr);
+	if (has_power_file(pci_slot))
+		sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_power.attr);
 exit_power:
-	pci_hp_remove_module_link(slot);
+	pci_hp_remove_module_link(pci_slot);
 exit:
 	return retval;
 }
 
-static void fs_remove_slot(struct pci_slot *slot)
+static void fs_remove_slot(struct pci_slot *pci_slot)
 {
-	if (has_power_file(slot))
-		sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_power.attr);
+	if (has_power_file(pci_slot))
+		sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_power.attr);
 
-	if (has_attention_file(slot))
-		sysfs_remove_file(&slot->kobj,
+	if (has_attention_file(pci_slot))
+		sysfs_remove_file(&pci_slot->kobj,
 				  &hotplug_slot_attr_attention.attr);
 
-	if (has_latch_file(slot))
-		sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_latch.attr);
+	if (has_latch_file(pci_slot))
+		sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_latch.attr);
 
-	if (has_adapter_file(slot))
-		sysfs_remove_file(&slot->kobj,
+	if (has_adapter_file(pci_slot))
+		sysfs_remove_file(&pci_slot->kobj,
 				  &hotplug_slot_attr_presence.attr);
 
-	if (has_test_file(slot))
-		sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_test.attr);
+	if (has_test_file(pci_slot))
+		sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_test.attr);
 
-	pci_hp_remove_module_link(slot);
+	pci_hp_remove_module_link(pci_slot);
 }
 
 static struct hotplug_slot *get_slot_from_name(const char *name)
@@ -467,37 +467,37 @@ EXPORT_SYMBOL_GPL(__pci_hp_register);
 
 /**
  * pci_hp_deregister - deregister a hotplug_slot with the PCI hotplug subsystem
- * @hotplug: pointer to the &struct hotplug_slot to deregister
+ * @slot: pointer to the &struct hotplug_slot to deregister
  *
  * The @slot must have been registered with the pci hotplug subsystem
  * previously with a call to pci_hp_register().
  *
  * Returns 0 if successful, anything else for an error.
  */
-int pci_hp_deregister(struct hotplug_slot *hotplug)
+int pci_hp_deregister(struct hotplug_slot *slot)
 {
 	struct hotplug_slot *temp;
-	struct pci_slot *slot;
+	struct pci_slot *pci_slot;
 
-	if (!hotplug)
+	if (!slot)
 		return -ENODEV;
 
 	mutex_lock(&pci_hp_mutex);
-	temp = get_slot_from_name(hotplug_slot_name(hotplug));
-	if (temp != hotplug) {
+	temp = get_slot_from_name(hotplug_slot_name(slot));
+	if (temp != slot) {
 		mutex_unlock(&pci_hp_mutex);
 		return -ENODEV;
 	}
 
-	list_del(&hotplug->slot_list);
+	list_del(&slot->slot_list);
 
-	slot = hotplug->pci_slot;
-	fs_remove_slot(slot);
-	dbg("Removed slot %s from the list\n", hotplug_slot_name(hotplug));
+	pci_slot = slot->pci_slot;
+	fs_remove_slot(pci_slot);
+	dbg("Removed slot %s from the list\n", hotplug_slot_name(slot));
 
-	hotplug->release(hotplug);
-	slot->hotplug = NULL;
-	pci_destroy_slot(slot);
+	slot->release(slot);
+	pci_slot->hotplug = NULL;
+	pci_destroy_slot(pci_slot);
 	mutex_unlock(&pci_hp_mutex);
 
 	return 0;
@@ -506,7 +506,7 @@ EXPORT_SYMBOL_GPL(pci_hp_deregister);
 
 /**
  * pci_hp_change_slot_info - changes the slot's information structure in the core
- * @hotplug: pointer to the slot whose info has changed
+ * @slot: pointer to the slot whose info has changed
  * @info: pointer to the info copy into the slot's info structure
  *
  * @slot must have been registered with the pci
@@ -514,13 +514,13 @@ EXPORT_SYMBOL_GPL(pci_hp_deregister);
  *
  * Returns 0 if successful, anything else for an error.
  */
-int pci_hp_change_slot_info(struct hotplug_slot *hotplug,
+int pci_hp_change_slot_info(struct hotplug_slot *slot,
 			    struct hotplug_slot_info *info)
 {
-	if (!hotplug || !info)
+	if (!slot || !info)
 		return -ENODEV;
 
-	memcpy(hotplug->info, info, sizeof(struct hotplug_slot_info));
+	memcpy(slot->info, info, sizeof(struct hotplug_slot_info));
 
 	return 0;
 }
-- 
1.7.1

[PATCH 2/3] PCI: Simplify pcie_poll_cmd()

[Yijing Wang]

Move first slot status read into while to simplify code.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 2913f7e..daf54be 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -109,21 +109,17 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
 	struct pci_dev *pdev = ctrl_dev(ctrl);
 	u16 slot_status;
 
-	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
-	if (slot_status & PCI_EXP_SLTSTA_CC) {
-		pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
-					   PCI_EXP_SLTSTA_CC);
-		return 1;
-	}
-	while (timeout > 0) {
-		msleep(10);
-		timeout -= 10;
+	while (true) {
 		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
 		if (slot_status & PCI_EXP_SLTSTA_CC) {
 			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
 						   PCI_EXP_SLTSTA_CC);
 			return 1;
 		}
+		if (timeout < 0)
+			break;
+		msleep(10);
+		timeout -= 10;
 	}
 	return 0;	/* timeout */
 }
-- 
1.7.1

[PATCH 3/3] PCI: Use usleep_range() instead of msleep() for better accuracy

[Yijing Wang]

Msleep < 20ms can sleep for up to 20ms, see
Documentation/timers/timers-howto.txt, so we could
use usleep_range instead.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index daf54be..4553728 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -118,7 +118,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
 		}
 		if (timeout < 0)
 			break;
-		msleep(10);
+		usleep_range(10000, 11000);
 		timeout -= 10;
 	}
 	return 0;	/* timeout */
-- 
1.7.1

Re: [PATCH 3/3] PCI: Use usleep_range() instead of msleep() for better accuracy

[Bjorn Helgaas]

On Fri, Jun 19, 2015 at 03:57:46PM +0800, Yijing Wang wrote:
> Msleep < 20ms can sleep for up to 20ms, see
> Documentation/timers/timers-howto.txt, so we could
> use usleep_range instead.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index daf54be..4553728 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -118,7 +118,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
>  		}
>  		if (timeout < 0)
>  			break;
> -		msleep(10);
> +		usleep_range(10000, 11000);

timers-howto.txt also says to use msleep for 10ms+ delays, so the guidance
is a bit ambiguous.

This particular delay does not need to be precise, and if we delay 20ms
instead of 10ms (1/50th of a second vs 1/100th of a second), I don't think
it makes any difference at all.

If we *did* make a change here, I think we should use a range of at least
10ms.  There's no need to tighten the wakeup time to the 1ms window between
10ms and 11ms.  Any time in the range of 10ms to 50ms would probably be
fine.

But I don't think a change here is necessary, and it does make it a bit
harder to analyze the code because we have some things in microseconds and
others in milliseconds.

>  		timeout -= 10;
>  	}
>  	return 0;	/* timeout */
> -- 
> 1.7.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

Re: [PATCH 3/3] PCI: Use usleep_range() instead of msleep() for better accuracy

[Yijing Wang]

On 2015/6/20 1:12, Bjorn Helgaas wrote:
> On Fri, Jun 19, 2015 at 03:57:46PM +0800, Yijing Wang wrote:
>> Msleep < 20ms can sleep for up to 20ms, see
>> Documentation/timers/timers-howto.txt, so we could
>> use usleep_range instead.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>>  drivers/pci/hotplug/pciehp_hpc.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index daf54be..4553728 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -118,7 +118,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
>>  		}
>>  		if (timeout < 0)
>>  			break;
>> -		msleep(10);
>> +		usleep_range(10000, 11000);
> 
> timers-howto.txt also says to use msleep for 10ms+ delays, so the guidance
> is a bit ambiguous.
> 
> This particular delay does not need to be precise, and if we delay 20ms
> instead of 10ms (1/50th of a second vs 1/100th of a second), I don't think
> it makes any difference at all.
> 
> If we *did* make a change here, I think we should use a range of at least
> 10ms.  There's no need to tighten the wakeup time to the 1ms window between
> 10ms and 11ms.  Any time in the range of 10ms to 50ms would probably be
> fine.
> 
> But I don't think a change here is necessary, and it does make it a bit
> harder to analyze the code because we have some things in microseconds and
> others in milliseconds.

OK， I got it, thanks for your explanation.

Thanks!
Yijing.


> 
>>  		timeout -= 10;
>>  	}
>>  	return 0;	/* timeout */
>> -- 
>> 1.7.1
>>
> 
> 


-- 
Thanks!
Yijing

Re: [PATCH 0/3] Trivial pci hotplug changes

[Bjorn Helgaas]

On Fri, Jun 19, 2015 at 03:57:43PM +0800, Yijing Wang wrote:
> 
> Yijing Wang (3):
>   PCI: Use "slot" and "pci_slot" for struct hotplug_slot and struct
>     pci_slot
>   PCI: Simplify pcie_poll_cmd()
>   PCI: Use usleep_range() instead of msleep() for better accuracy

I applied the first two to pci/hotplug for v4.2, thanks!

I dropped the third, as explained in my response to it.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

Re: [PATCH 0/3] Trivial pci hotplug changes

[Yijing Wang]

On 2015/6/20 1:14, Bjorn Helgaas wrote:
> On Fri, Jun 19, 2015 at 03:57:43PM +0800, Yijing Wang wrote:
>>
>> Yijing Wang (3):
>>   PCI: Use "slot" and "pci_slot" for struct hotplug_slot and struct
>>     pci_slot
>>   PCI: Simplify pcie_poll_cmd()
>>   PCI: Use usleep_range() instead of msleep() for better accuracy
> 
> I applied the first two to pci/hotplug for v4.2, thanks!
> 
> I dropped the third, as explained in my response to it.

Thanks!

> 
> Bjorn
> 
> 


-- 
Thanks!
Yijing