Patchwork [-v2,3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend

login
register
mail settings
Submitter Yijing Wang
Date July 11, 2013, 9:43 a.m.
Message ID <1373535825-49972-4-git-send-email-wangyijing@huawei.com>
Download mbox | patch
Permalink /patch/258390/
State Superseded
Headers show

Comments

Yijing Wang - July 11, 2013, 9:43 a.m.
If device was removed from slot and reinsert a new device during
suspend, pciehp can not identify the physical device change now.
So the old driver .resume() method will be called for the new device,
this is bad. If device support device serial number capability,
we can identify this by DSN. So the reasonable way is first remove
the old device, then enable the new device.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Paul Bolle <pebolle@tiscali.nl>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/hotplug/pciehp_core.c |   45 +++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)
Don Dutile - July 11, 2013, 10:04 a.m.
Sorry, did a 'reply' instead of 'reply all' on first reply to this patch...
excuse any repeat...
- Don

On 07/11/2013 05:43 AM, Yijing Wang wrote:
> If device was removed from slot and reinsert a new device during
> suspend, pciehp can not identify the physical device change now.
> So the old driver .resume() method will be called for the new device,
> this is bad. If device support device serial number capability,
> we can identify this by DSN. So the reasonable way is first remove
> the old device, then enable the new device.
>
> Signed-off-by: Yijing Wang<wangyijing@huawei.com>
> Cc: Paul Bolle<pebolle@tiscali.nl>
> Cc: "Rafael J. Wysocki"<rjw@sisk.pl>
> Cc: Oliver Neukum<oneukum@suse.de>
> Cc: Gu Zheng<guz.fnst@cn.fujitsu.com>
> Cc: linux-pci@vger.kernel.org
> ---
>   drivers/pci/hotplug/pciehp_core.c |   45 +++++++++++++++++++++++++++++++++++++
>   1 files changed, 45 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 1542735..f2eb214 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -296,11 +296,38 @@ static int pciehp_suspend (struct pcie_device *dev)
>   	return 0;
>   }
>
> +/*
> + * check the func 0 device serial number is changed,
> + * if device does not support device serial number,
> + * return false.
> + */
> +static bool device_serial_number_changed(struct pci_bus *pbus)
> +{
> +	u64 old_dsn, new_dsn;
> +	struct pci_dev *pdev;
> +
> +	pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0));
> +	if (!pdev)
> +		return false;
> +
> +	old_dsn = pdev->sn;
> +
> +	/* get func 0 device serial number */
> +	new_dsn = pci_device_serial_number(pdev);
Due to previous recommended change to 1/3 patch, above would be:
     pci_device_serial_number(pdev);

> +	pci_dev_put(pdev);
> +
> +	if (old_dsn != new_dsn)
> +		return true;
> +
and above would be:
     new_dsn = pdev->dsn;
     pci_dev_put(pdev);
     if(pdev->dsn != old_dsn)
         return true;

> +	return false;
> +}
> +
>   static int pciehp_resume (struct pcie_device *dev)
>   {
>   	struct controller *ctrl;
>   	struct slot *slot;
>   	struct pci_bus *pbus = dev->port->subordinate;
> +	int retval = 0;
>   	u8 status;
>
>   	ctrl = get_service_data(dev);
> @@ -315,6 +342,24 @@ static int pciehp_resume (struct pcie_device *dev)
>   	if (status) {
>   		if (list_empty(&pbus->devices))
>   			pciehp_enable_slot(slot);
> +
> +		if (device_serial_number_changed(pbus)) {
> +			/*
> +			 * first power off slot, avoid the old driver
> +			 * .remove() method touch the new hardware
> +			 */
> +			if (POWER_CTRL(ctrl)) {
> +				retval = pciehp_power_off_slot(slot);
> +				if (retval) {
> +					ctrl_err(ctrl,
> +						"Issue of Slot Disable command failed\n");
> +					return 0;
> +				}
> +				msleep(1000);
> +				pciehp_unconfigure_device(slot);
> +				pciehp_enable_slot(slot);
> +			}
> +		}
>   	} else if (!list_empty(&pbus->devices))
>   		pciehp_disable_slot(slot);
>   	

--
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
Paul Bolle - July 11, 2013, 2:33 p.m.
On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
[...]
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 1542735..f2eb214 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
[...]
> @@ -315,6 +342,24 @@ static int pciehp_resume (struct pcie_device *dev)
>  	if (status) {
>  		if (list_empty(&pbus->devices))
>  			pciehp_enable_slot(slot);
> +
> +		if (device_serial_number_changed(pbus)) {
> +			/*
> +			 * first power off slot, avoid the old driver
> +			 * .remove() method touch the new hardware
> +			 */
> +			if (POWER_CTRL(ctrl)) {
> +				retval = pciehp_power_off_slot(slot);
> +				if (retval) {
> +					ctrl_err(ctrl,
> +						"Issue of Slot Disable command failed\n");
> +					return 0;
> +				}
> +				msleep(1000);
> +				pciehp_unconfigure_device(slot);
> +				pciehp_enable_slot(slot);
> +			}
> +		}
>  	} else if (!list_empty(&pbus->devices))
>  		pciehp_disable_slot(slot);
>  	

It was surprisingly hard to see why the patch wouldn't apply to v3.10.
It turns out the very last line of context is a line consisting of just
a single tab. And in v3.10 it is an empty line. 

Is that lone tab perhaps an editing mistake on your side?


Paul Bolle

--
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
Yijing Wang - July 12, 2013, 2:19 a.m.
On 2013/7/11 22:33, Paul Bolle wrote:
> On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
> [...]
>> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
>> index 1542735..f2eb214 100644
>> --- a/drivers/pci/hotplug/pciehp_core.c
>> +++ b/drivers/pci/hotplug/pciehp_core.c
> [...]
>> @@ -315,6 +342,24 @@ static int pciehp_resume (struct pcie_device *dev)
>>  	if (status) {
>>  		if (list_empty(&pbus->devices))
>>  			pciehp_enable_slot(slot);
>> +
>> +		if (device_serial_number_changed(pbus)) {
>> +			/*
>> +			 * first power off slot, avoid the old driver
>> +			 * .remove() method touch the new hardware
>> +			 */
>> +			if (POWER_CTRL(ctrl)) {
>> +				retval = pciehp_power_off_slot(slot);
>> +				if (retval) {
>> +					ctrl_err(ctrl,
>> +						"Issue of Slot Disable command failed\n");
>> +					return 0;
>> +				}
>> +				msleep(1000);
>> +				pciehp_unconfigure_device(slot);
>> +				pciehp_enable_slot(slot);
>> +			}
>> +		}
>>  	} else if (!list_empty(&pbus->devices))
>>  		pciehp_disable_slot(slot);
>>  	
> 
> It was surprisingly hard to see why the patch wouldn't apply to v3.10.
> It turns out the very last line of context is a line consisting of just
> a single tab. And in v3.10 it is an empty line. 
> 
> Is that lone tab perhaps an editing mistake on your side?

I will check this, thanks!

> 
> 
> Paul Bolle
> 
> 
> .
>

Patch

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 1542735..f2eb214 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -296,11 +296,38 @@  static int pciehp_suspend (struct pcie_device *dev)
 	return 0;
 }
 
+/*
+ * check the func 0 device serial number is changed,
+ * if device does not support device serial number,
+ * return false.
+ */
+static bool device_serial_number_changed(struct pci_bus *pbus)
+{
+	u64 old_dsn, new_dsn;
+	struct pci_dev *pdev;
+
+	pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0));
+	if (!pdev)
+		return false;
+
+	old_dsn = pdev->sn;
+
+	/* get func 0 device serial number */
+	new_dsn = pci_device_serial_number(pdev);
+	pci_dev_put(pdev);
+
+	if (old_dsn != new_dsn)
+		return true;
+
+	return false;
+}
+
 static int pciehp_resume (struct pcie_device *dev)
 {
 	struct controller *ctrl;
 	struct slot *slot;
 	struct pci_bus *pbus = dev->port->subordinate;
+	int retval = 0;
 	u8 status;
 
 	ctrl = get_service_data(dev);
@@ -315,6 +342,24 @@  static int pciehp_resume (struct pcie_device *dev)
 	if (status) {
 		if (list_empty(&pbus->devices))
 			pciehp_enable_slot(slot);
+
+		if (device_serial_number_changed(pbus)) {
+			/*
+			 * first power off slot, avoid the old driver
+			 * .remove() method touch the new hardware
+			 */
+			if (POWER_CTRL(ctrl)) {
+				retval = pciehp_power_off_slot(slot);
+				if (retval) {
+					ctrl_err(ctrl,
+						"Issue of Slot Disable command failed\n");
+					return 0;
+				}
+				msleep(1000);
+				pciehp_unconfigure_device(slot);
+				pciehp_enable_slot(slot);
+			}
+		}
 	} else if (!list_empty(&pbus->devices))
 		pciehp_disable_slot(slot);