[PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume

[PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume

[Yijing Wang]

v1->v2: Modify pci_get_dsn to pci_device_serial_number,
	    power off slot before remove the old device during resume to avoid
		old .remove() method to touch new hardware.
		Fix other typo and fail check problems.
		Split the list_empty() guard into new patch.
		

Yijing Wang (3):
  PCI: introduce PCIe Device Serial Number Capability support
  PCI,pciehp: avoid add a device already exist before suspend during
    resume
  PCI,pciehp: use PCIe DSN to identify device change during suspend

 drivers/pci/hotplug/pciehp_core.c |   54 ++++++++++++++++++++++++++++++++++--
 drivers/pci/pci.c                 |   27 ++++++++++++++++++
 drivers/pci/probe.c               |    2 +
 include/linux/pci.h               |    3 ++
 4 files changed, 83 insertions(+), 3 deletions(-)

[PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Yijing Wang]

Introduce PCIe Ext Capability Device Serial Number support,
so we can use the unique device serial number to identify
the physical device. During system suspend, if the PCIe
device was removed and inserted a new same device, after
system resume there is no good way to identify it, maybe
Device Serial Number is a good choice if device support.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pci.c   |   27 +++++++++++++++++++++++++++
 drivers/pci/probe.c |    2 ++
 include/linux/pci.h |    3 +++
 3 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e37fea6..2e855b5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2048,6 +2048,33 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
 }
 
 /**
+ * pci_device_serial_number - get device serial number
+ * @dev: the PCI device
+ *
+ * return the device serial number if device support,
+ * otherwise return 0.
+ */
+u64 pci_device_serial_number(struct pci_dev *dev)
+{
+	int pos;
+	u64 sn;
+	u32 lo, hi;
+
+	if (!pci_is_pcie(dev))
+		return 0;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
+	if (!pos)
+		return 0;
+
+	pci_read_config_dword(dev, pos + 4, &lo);
+	pci_read_config_dword(dev, pos + 8, &hi);
+	sn = ((u64)hi << 32) | lo;
+	return sn;
+}
+EXPORT_SYMBOL(pci_device_serial_number);
+
+/**
  * pci_configure_ari - enable or disable ARI forwarding
  * @dev: the PCI device
  *
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 46ada5c..c4c1a2b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	/* Power Management */
 	pci_pm_init(dev);
 
+	dev->sn = pci_device_serial_number(dev);
+
 	/* Vital Product Data */
 	pci_vpd_pci22_init(dev);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0fd1f15..10d190b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -342,6 +342,7 @@ struct pci_dev {
 	struct list_head msi_list;
 	struct kset *msi_kset;
 #endif
+	u64 sn;		/* device serieal number, 0 if not support */
 	struct pci_vpd *vpd;
 #ifdef CONFIG_PCI_ATS
 	union {
@@ -995,6 +996,8 @@ ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
 ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
 int pci_vpd_truncate(struct pci_dev *dev, size_t size);
 
+u64 pci_device_serial_number(struct pci_dev *dev);
+
 /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */
 resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx);
 void pci_bus_assign_resources(const struct pci_bus *bus);
-- 
1.7.1

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Don Dutile]

On 07/11/2013 05:43 AM, Yijing Wang wrote:
> Introduce PCIe Ext Capability Device Serial Number support,
> so we can use the unique device serial number to identify
> the physical device. During system suspend, if the PCIe
> device was removed and inserted a new same device, after
> system resume there is no good way to identify it, maybe
> Device Serial Number is a good choice if device support.
>
> Signed-off-by: Yijing Wang<wangyijing@huawei.com>
> ---
>   drivers/pci/pci.c   |   27 +++++++++++++++++++++++++++
>   drivers/pci/probe.c |    2 ++
>   include/linux/pci.h |    3 +++
>   3 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e37fea6..2e855b5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2048,6 +2048,33 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>   }
>
>   /**
> + * pci_device_serial_number - get device serial number
> + * @dev: the PCI device
> + *
> + * return the device serial number if device support,
> + * otherwise return 0.
> + */
> +u64 pci_device_serial_number(struct pci_dev *dev)
See comment below:
void  pci_device_serial_number(struct pci_dev *dev)

> +{
> +	int pos;
> +	u64 sn;
> +	u32 lo, hi;
> +
> +	if (!pci_is_pcie(dev))
> +		return 0;
> +
See comment below:
if (!pci_is_pcie(dev)) {
	dev->sn = 0;
	return;
}
> +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
> +	if (!pos)
> +		return 0;
> +
> +	pci_read_config_dword(dev, pos + 4,&lo);
> +	pci_read_config_dword(dev, pos + 8,&hi);
> +	sn = ((u64)hi<<  32) | lo;
See comment below:
	dev->sn = ((u64)hi<<  32) | lo;
	return;
> +	return sn;
> +}
> +EXPORT_SYMBOL(pci_device_serial_number);
> +
> +/**
>    * pci_configure_ari - enable or disable ARI forwarding
>    * @dev: the PCI device
>    *
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 46ada5c..c4c1a2b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
>   	/* Power Management */
>   	pci_pm_init(dev);
>
> +	dev->sn = pci_device_serial_number(dev);
> +
Finally, 'the comment below':
I know you were following Bjorn's suggestion, which I thought
was an improvement, but why not do above assignment in pci_device_serial_number() ?
See above....
- Don

>   	/* Vital Product Data */
>   	pci_vpd_pci22_init(dev);
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0fd1f15..10d190b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -342,6 +342,7 @@ struct pci_dev {
>   	struct list_head msi_list;
>   	struct kset *msi_kset;
>   #endif
> +	u64 sn;		/* device serieal number, 0 if not support */
>   	struct pci_vpd *vpd;
>   #ifdef CONFIG_PCI_ATS
>   	union {
> @@ -995,6 +996,8 @@ ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
>   ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
>   int pci_vpd_truncate(struct pci_dev *dev, size_t size);
>
> +u64 pci_device_serial_number(struct pci_dev *dev);
> +
>   /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */
>   resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx);
>   void pci_bus_assign_resources(const struct pci_bus *bus);

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Bjorn Helgaas]

On Thu, Jul 11, 2013 at 3:51 AM, Don Dutile <ddutile@redhat.com> wrote:
> On 07/11/2013 05:43 AM, Yijing Wang wrote:
>>
>> Introduce PCIe Ext Capability Device Serial Number support,
>> so we can use the unique device serial number to identify
>> the physical device. During system suspend, if the PCIe
>> device was removed and inserted a new same device, after
>> system resume there is no good way to identify it, maybe
>> Device Serial Number is a good choice if device support.
>>
>> Signed-off-by: Yijing Wang<wangyijing@huawei.com>
>> ---
>>   drivers/pci/pci.c   |   27 +++++++++++++++++++++++++++
>>   drivers/pci/probe.c |    2 ++
>>   include/linux/pci.h |    3 +++
>>   3 files changed, 32 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index e37fea6..2e855b5 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -2048,6 +2048,33 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>>   }
>>
>>   /**
>> + * pci_device_serial_number - get device serial number
>> + * @dev: the PCI device
>> + *
>> + * return the device serial number if device support,
>> + * otherwise return 0.
>> + */
>> +u64 pci_device_serial_number(struct pci_dev *dev)
>
> See comment below:
> void  pci_device_serial_number(struct pci_dev *dev)
>
>
>> +{
>> +       int pos;
>> +       u64 sn;
>> +       u32 lo, hi;
>> +
>> +       if (!pci_is_pcie(dev))
>> +               return 0;
>> +
>
> See comment below:
> if (!pci_is_pcie(dev)) {
>         dev->sn = 0;
>         return;
>
> }
>>
>> +       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
>> +       if (!pos)
>> +               return 0;
>> +
>> +       pci_read_config_dword(dev, pos + 4,&lo);
>> +       pci_read_config_dword(dev, pos + 8,&hi);
>> +       sn = ((u64)hi<<  32) | lo;
>
> See comment below:
>         dev->sn = ((u64)hi<<  32) | lo;
>         return;
>
>> +       return sn;
>> +}
>> +EXPORT_SYMBOL(pci_device_serial_number);
>> +
>> +/**
>>    * pci_configure_ari - enable or disable ARI forwarding
>>    * @dev: the PCI device
>>    *
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 46ada5c..c4c1a2b 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>> *dev)
>>         /* Power Management */
>>         pci_pm_init(dev);
>>
>> +       dev->sn = pci_device_serial_number(dev);
>> +
>
> Finally, 'the comment below':
> I know you were following Bjorn's suggestion, which I thought
> was an improvement, but why not do above assignment in
> pci_device_serial_number() ?
> See above....

pci_device_serial_number() would then have the side-effect of saving
the result somewhere, and callers would have to know where to look.
Personally, I think it's simpler to return the serial number directly
and avoid the side-effect, but maybe this is just bike-shedding.

As long as we are bike-shedding, I'd just drop "sn" and do:

    return ((u64)hi << 32) | lo;

And of course, you need spaces before "&hi" and "&lo" in the
pci_read_config_dword() calls.

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Don Dutile]

On 07/11/2013 04:09 PM, Bjorn Helgaas wrote:
> On Thu, Jul 11, 2013 at 3:51 AM, Don Dutile<ddutile@redhat.com>  wrote:
>> On 07/11/2013 05:43 AM, Yijing Wang wrote:
>>>
>>> Introduce PCIe Ext Capability Device Serial Number support,
>>> so we can use the unique device serial number to identify
>>> the physical device. During system suspend, if the PCIe
>>> device was removed and inserted a new same device, after
>>> system resume there is no good way to identify it, maybe
>>> Device Serial Number is a good choice if device support.
>>>
>>> Signed-off-by: Yijing Wang<wangyijing@huawei.com>
>>> ---
>>>    drivers/pci/pci.c   |   27 +++++++++++++++++++++++++++
>>>    drivers/pci/probe.c |    2 ++
>>>    include/linux/pci.h |    3 +++
>>>    3 files changed, 32 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index e37fea6..2e855b5 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -2048,6 +2048,33 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>>>    }
>>>
>>>    /**
>>> + * pci_device_serial_number - get device serial number
>>> + * @dev: the PCI device
>>> + *
>>> + * return the device serial number if device support,
>>> + * otherwise return 0.
>>> + */
>>> +u64 pci_device_serial_number(struct pci_dev *dev)
>>
>> See comment below:
>> void  pci_device_serial_number(struct pci_dev *dev)
>>
>>
>>> +{
>>> +       int pos;
>>> +       u64 sn;
>>> +       u32 lo, hi;
>>> +
>>> +       if (!pci_is_pcie(dev))
>>> +               return 0;
>>> +
>>
>> See comment below:
>> if (!pci_is_pcie(dev)) {
>>          dev->sn = 0;
>>          return;
>>
>> }
>>>
>>> +       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
>>> +       if (!pos)
>>> +               return 0;
>>> +
>>> +       pci_read_config_dword(dev, pos + 4,&lo);
>>> +       pci_read_config_dword(dev, pos + 8,&hi);
>>> +       sn = ((u64)hi<<   32) | lo;
>>
>> See comment below:
>>          dev->sn = ((u64)hi<<   32) | lo;
>>          return;
>>
>>> +       return sn;
>>> +}
>>> +EXPORT_SYMBOL(pci_device_serial_number);
>>> +
>>> +/**
>>>     * pci_configure_ari - enable or disable ARI forwarding
>>>     * @dev: the PCI device
>>>     *
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 46ada5c..c4c1a2b 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>>> *dev)
>>>          /* Power Management */
>>>          pci_pm_init(dev);
>>>
>>> +       dev->sn = pci_device_serial_number(dev);
>>> +
>>
>> Finally, 'the comment below':
>> I know you were following Bjorn's suggestion, which I thought
>> was an improvement, but why not do above assignment in
>> pci_device_serial_number() ?
>> See above....
>
> pci_device_serial_number() would then have the side-effect of saving
> the result somewhere, and callers would have to know where to look.
Ah, like so many other features of a PCI device?  -- what are all those
flags/status bits in pdev for ??? ;-)

> Personally, I think it's simpler to return the serial number directly
> and avoid the side-effect, but maybe this is just bike-shedding.
>
What struck me about rtning a value is in pci_init_capabilities(),
it was the only function with a rtn value that had to be put into a pci_dev 
struct element,
while all the others stored their related capabilities in the pdev, or other struct
related to it.

So, maybe there should be a "pci_device_serial_number_init()" function
which does the storage in the pdev struct, and another (wrapper,
dare I shoot myself, inline)  that is "pci_device_serial_number()" that returns
the value of the pdev's sn element, if another module or other core component
want to get it. .... and do that in a future patch... :-/

> As long as we are bike-shedding, I'd just drop "sn" and do:
>
>      return ((u64)hi<<  32) | lo;
>
> And of course, you need spaces before "&hi" and "&lo" in the
> pci_read_config_dword() calls.

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Yijing Wang]

On 2013/7/12 2:18, Don Dutile wrote:
> On 07/11/2013 04:09 PM, Bjorn Helgaas wrote:
>> On Thu, Jul 11, 2013 at 3:51 AM, Don Dutile<ddutile@redhat.com>  wrote:
>>> On 07/11/2013 05:43 AM, Yijing Wang wrote:
>>>>
>>>> Introduce PCIe Ext Capability Device Serial Number support,
>>>> so we can use the unique device serial number to identify
>>>> the physical device. During system suspend, if the PCIe
>>>> device was removed and inserted a new same device, after
>>>> system resume there is no good way to identify it, maybe
>>>> Device Serial Number is a good choice if device support.
>>>>
>>>> Signed-off-by: Yijing Wang<wangyijing@huawei.com>
>>>> ---
>>>>    drivers/pci/pci.c   |   27 +++++++++++++++++++++++++++
>>>>    drivers/pci/probe.c |    2 ++
>>>>    include/linux/pci.h |    3 +++
>>>>    3 files changed, 32 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index e37fea6..2e855b5 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -2048,6 +2048,33 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>>>>    }
>>>>
>>>>    /**
>>>> + * pci_device_serial_number - get device serial number
>>>> + * @dev: the PCI device
>>>> + *
>>>> + * return the device serial number if device support,
>>>> + * otherwise return 0.
>>>> + */
>>>> +u64 pci_device_serial_number(struct pci_dev *dev)
>>>
>>> See comment below:
>>> void  pci_device_serial_number(struct pci_dev *dev)
>>>
>>>
>>>> +{
>>>> +       int pos;
>>>> +       u64 sn;
>>>> +       u32 lo, hi;
>>>> +
>>>> +       if (!pci_is_pcie(dev))
>>>> +               return 0;
>>>> +
>>>
>>> See comment below:
>>> if (!pci_is_pcie(dev)) {
>>>          dev->sn = 0;
>>>          return;
>>>
>>> }
>>>>
>>>> +       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
>>>> +       if (!pos)
>>>> +               return 0;
>>>> +
>>>> +       pci_read_config_dword(dev, pos + 4,&lo);
>>>> +       pci_read_config_dword(dev, pos + 8,&hi);
>>>> +       sn = ((u64)hi<<   32) | lo;
>>>
>>> See comment below:
>>>          dev->sn = ((u64)hi<<   32) | lo;
>>>          return;
>>>
>>>> +       return sn;
>>>> +}
>>>> +EXPORT_SYMBOL(pci_device_serial_number);
>>>> +
>>>> +/**
>>>>     * pci_configure_ari - enable or disable ARI forwarding
>>>>     * @dev: the PCI device
>>>>     *
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index 46ada5c..c4c1a2b 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>>>> *dev)
>>>>          /* Power Management */
>>>>          pci_pm_init(dev);
>>>>
>>>> +       dev->sn = pci_device_serial_number(dev);
>>>> +
>>>
>>> Finally, 'the comment below':
>>> I know you were following Bjorn's suggestion, which I thought
>>> was an improvement, but why not do above assignment in
>>> pci_device_serial_number() ?
>>> See above....
>>
>> pci_device_serial_number() would then have the side-effect of saving
>> the result somewhere, and callers would have to know where to look.
> Ah, like so many other features of a PCI device?  -- what are all those
> flags/status bits in pdev for ??? ;-)
> 
>> Personally, I think it's simpler to return the serial number directly
>> and avoid the side-effect, but maybe this is just bike-shedding.
>>
> What struck me about rtning a value is in pci_init_capabilities(),
> it was the only function with a rtn value that had to be put into a pci_dev struct element,
> while all the others stored their related capabilities in the pdev, or other struct
> related to it.
> 
> So, maybe there should be a "pci_device_serial_number_init()" function
> which does the storage in the pdev struct, and another (wrapper,
> dare I shoot myself, inline)  that is "pci_device_serial_number()" that returns

add an wrapper function may be ok, as I mentioned in pre-reply, what about,

static void pci_init_capabilities(struct pci_dev *dev)
     ...............
     pci_dsn_init(dev);
     .................


void pci_dsn_init(dev)
{
     dev->sn = pci_device_serial_number(dev);
}

> the value of the pdev's sn element, if another module or other core component
> want to get it. .... and do that in a future patch... :-/
> 
>> As long as we are bike-shedding, I'd just drop "sn" and do:
>>
>>      return ((u64)hi<<  32) | lo;
>>
>> And of course, you need spaces before "&hi" and "&lo" in the
>> pci_read_config_dword() calls.
> 
> 
> .
> 


-- 
Thanks!
Yijing

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Bjorn Helgaas]

On Thu, Jul 11, 2013 at 12:18 PM, Don Dutile <ddutile@redhat.com> wrote:
> On 07/11/2013 04:09 PM, Bjorn Helgaas wrote:
>>
>> On Thu, Jul 11, 2013 at 3:51 AM, Don Dutile<ddutile@redhat.com>  wrote:
>>>
>>> On 07/11/2013 05:43 AM, Yijing Wang wrote:
>>>>
>>>>
>>>> Introduce PCIe Ext Capability Device Serial Number support,
>>>> so we can use the unique device serial number to identify
>>>> the physical device. During system suspend, if the PCIe
>>>> device was removed and inserted a new same device, after
>>>> system resume there is no good way to identify it, maybe
>>>> Device Serial Number is a good choice if device support.
>>>>
>>>> Signed-off-by: Yijing Wang<wangyijing@huawei.com>
>>>> ---
>>>>    drivers/pci/pci.c   |   27 +++++++++++++++++++++++++++
>>>>    drivers/pci/probe.c |    2 ++
>>>>    include/linux/pci.h |    3 +++
>>>>    3 files changed, 32 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index e37fea6..2e855b5 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -2048,6 +2048,33 @@ void pci_free_cap_save_buffers(struct pci_dev
>>>> *dev)
>>>>    }
>>>>
>>>>    /**
>>>> + * pci_device_serial_number - get device serial number
>>>> + * @dev: the PCI device
>>>> + *
>>>> + * return the device serial number if device support,
>>>> + * otherwise return 0.
>>>> + */
>>>> +u64 pci_device_serial_number(struct pci_dev *dev)
>>>
>>>
>>> See comment below:
>>> void  pci_device_serial_number(struct pci_dev *dev)
>>>
>>>
>>>> +{
>>>> +       int pos;
>>>> +       u64 sn;
>>>> +       u32 lo, hi;
>>>> +
>>>> +       if (!pci_is_pcie(dev))
>>>> +               return 0;
>>>> +
>>>
>>>
>>> See comment below:
>>> if (!pci_is_pcie(dev)) {
>>>          dev->sn = 0;
>>>          return;
>>>
>>> }
>>>>
>>>>
>>>> +       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
>>>> +       if (!pos)
>>>> +               return 0;
>>>> +
>>>> +       pci_read_config_dword(dev, pos + 4,&lo);
>>>> +       pci_read_config_dword(dev, pos + 8,&hi);
>>>> +       sn = ((u64)hi<<   32) | lo;
>>>
>>>
>>> See comment below:
>>>          dev->sn = ((u64)hi<<   32) | lo;
>>>          return;
>>>
>>>> +       return sn;
>>>> +}
>>>> +EXPORT_SYMBOL(pci_device_serial_number);
>>>> +
>>>> +/**
>>>>     * pci_configure_ari - enable or disable ARI forwarding
>>>>     * @dev: the PCI device
>>>>     *
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index 46ada5c..c4c1a2b 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>>>> *dev)
>>>>          /* Power Management */
>>>>          pci_pm_init(dev);
>>>>
>>>> +       dev->sn = pci_device_serial_number(dev);
>>>> +
>>>
>>>
>>> Finally, 'the comment below':
>>> I know you were following Bjorn's suggestion, which I thought
>>> was an improvement, but why not do above assignment in
>>> pci_device_serial_number() ?
>>> See above....
>>
>>
>> pci_device_serial_number() would then have the side-effect of saving
>> the result somewhere, and callers would have to know where to look.
>
> Ah, like so many other features of a PCI device?  -- what are all those
> flags/status bits in pdev for ??? ;-)
>
>
>> Personally, I think it's simpler to return the serial number directly
>> and avoid the side-effect, but maybe this is just bike-shedding.
>>
> What struck me about rtning a value is in pci_init_capabilities(),
> it was the only function with a rtn value that had to be put into a pci_dev
> struct element,
> while all the others stored their related capabilities in the pdev, or other
> struct
> related to it.

Good point, BTW.  I hadn't thought about it from that angle.

> So, maybe there should be a "pci_device_serial_number_init()" function
> which does the storage in the pdev struct, and another (wrapper,
> dare I shoot myself, inline)  that is "pci_device_serial_number()" that
> returns
> the value of the pdev's sn element, if another module or other core
> component
> want to get it. .... and do that in a future patch... :-/

Of course, for Yijing's use, it's important that we're able to get the
*non-cached* serial number, because the cached one belongs to the old,
pre-suspend device, and we want to look at the post-suspend device to
see if it's the same.

Sigh.  I dunno, I'll have to finish sorting through this in a week or
so...  (I'll be on vacation for the next few days.)

Bjorn

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Yijing Wang]

>> }
>>>
>>> +       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
>>> +       if (!pos)
>>> +               return 0;
>>> +
>>> +       pci_read_config_dword(dev, pos + 4,&lo);
>>> +       pci_read_config_dword(dev, pos + 8,&hi);
>>> +       sn = ((u64)hi<<  32) | lo;
>>
>> See comment below:
>>         dev->sn = ((u64)hi<<  32) | lo;
>>         return;
>>
>>> +       return sn;
>>> +}
>>> +EXPORT_SYMBOL(pci_device_serial_number);
>>> +
>>> +/**
>>>    * pci_configure_ari - enable or disable ARI forwarding
>>>    * @dev: the PCI device
>>>    *
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 46ada5c..c4c1a2b 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>>> *dev)
>>>         /* Power Management */
>>>         pci_pm_init(dev);
>>>
>>> +       dev->sn = pci_device_serial_number(dev);
>>> +
>>
>> Finally, 'the comment below':
>> I know you were following Bjorn's suggestion, which I thought
>> was an improvement, but why not do above assignment in
>> pci_device_serial_number() ?
>> See above....
> 
> pci_device_serial_number() would then have the side-effect of saving
> the result somewhere, and callers would have to know where to look.
> Personally, I think it's simpler to return the serial number directly
> and avoid the side-effect, but maybe this is just bike-shedding.
> 
> As long as we are bike-shedding, I'd just drop "sn" and do:
> 
>     return ((u64)hi << 32) | lo;
> 
> And of course, you need spaces before "&hi" and "&lo" in the
> pci_read_config_dword() calls.

OK, will drop "sn", strangely my original patch has spaces before "&hi" and "&lo",
but in reply the spaces disappeared.

Thanks!
Yijing.


> 



> .
> 


-- 
Thanks!
Yijing

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Yijing Wang]

Hi Don,
   Thanks for your review and comments very much!

>> +    dev->sn = pci_device_serial_number(dev);
>> +
> Finally, 'the comment below':
> I know you were following Bjorn's suggestion, which I thought
> was an improvement, but why not do above assignment in pci_device_serial_number() ?

I don't do assignment in pci_device_serial_number() because pci_device_serial_number()
is an EXPORT_SYMBOL, and will be used to get dsn for non-scaned device, for example:

1. we have pcie device in slot before suspend.
2. we removed the device during suspend.
3. we reinserted a new device during suspend.
4. we should check the device change during resume, so we
try to use pcie device serial number to identify changes.
But the pci_dev structure maybe the old stale data. So
I prefer to get dsn by return.

what about this:

static void pci_init_capabilities(struct pci_dev *dev)
    ........
    pci_dsn_init(dev);
    ..........


void pci_dsn_init(dev)
{
    dev->sn = pci_device_serial_number(dev);
}



> See above....
> - Don
> 
>>       /* Vital Product Data */
>>       pci_vpd_pci22_init(dev);
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 0fd1f15..10d190b 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -342,6 +342,7 @@ struct pci_dev {
>>       struct list_head msi_list;
>>       struct kset *msi_kset;
>>   #endif
>> +    u64 sn;        /* device serieal number, 0 if not support */
>>       struct pci_vpd *vpd;
>>   #ifdef CONFIG_PCI_ATS
>>       union {
>> @@ -995,6 +996,8 @@ ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
>>   ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
>>   int pci_vpd_truncate(struct pci_dev *dev, size_t size);
>>
>> +u64 pci_device_serial_number(struct pci_dev *dev);
>> +
>>   /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */
>>   resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx);
>>   void pci_bus_assign_resources(const struct pci_bus *bus);
> 
> 
> .
> 


-- 
Thanks!
Yijing

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Don Dutile]

On 07/11/2013 09:38 PM, Yijing Wang wrote:
> Hi Don,
>     Thanks for your review and comments very much!
>
>>> +    dev->sn = pci_device_serial_number(dev);
>>> +
>> Finally, 'the comment below':
>> I know you were following Bjorn's suggestion, which I thought
>> was an improvement, but why not do above assignment in pci_device_serial_number() ?
>
> I don't do assignment in pci_device_serial_number() because pci_device_serial_number()
> is an EXPORT_SYMBOL, and will be used to get dsn for non-scaned device, for example:
>
> 1. we have pcie device in slot before suspend.
> 2. we removed the device during suspend.
> 3. we reinserted a new device during suspend.
> 4. we should check the device change during resume, so we
> try to use pcie device serial number to identify changes.
> But the pci_dev structure maybe the old stale data. So
> I prefer to get dsn by return.
>
> what about this:
>
> static void pci_init_capabilities(struct pci_dev *dev)
>      ........
>      pci_dsn_init(dev);
>      ..........
>
>
> void pci_dsn_init(dev)
> {
>      dev->sn = pci_device_serial_number(dev);
> }
>
>
>
Looks good!  I think that meets all the uses (use in init, post-suspend).
- Don

>> See above....
>> - Don
>>
>>>        /* Vital Product Data */
>>>        pci_vpd_pci22_init(dev);
>>>
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 0fd1f15..10d190b 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -342,6 +342,7 @@ struct pci_dev {
>>>        struct list_head msi_list;
>>>        struct kset *msi_kset;
>>>    #endif
>>> +    u64 sn;        /* device serieal number, 0 if not support */
>>>        struct pci_vpd *vpd;
>>>    #ifdef CONFIG_PCI_ATS
>>>        union {
>>> @@ -995,6 +996,8 @@ ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
>>>    ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
>>>    int pci_vpd_truncate(struct pci_dev *dev, size_t size);
>>>
>>> +u64 pci_device_serial_number(struct pci_dev *dev);
>>> +
>>>    /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */
>>>    resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx);
>>>    void pci_bus_assign_resources(const struct pci_bus *bus);
>>
>>
>> .
>>
>
>

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Paul Bolle]

On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
> Introduce PCIe Ext Capability Device Serial Number support,
> so we can use the unique device serial number to identify
> the physical device. During system suspend, if the PCIe
> device was removed and inserted a new same device, after
> system resume there is no good way to identify it, maybe
> Device Serial Number is a good choice if device support.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/pci.c   |   27 +++++++++++++++++++++++++++
>  drivers/pci/probe.c |    2 ++
>  include/linux/pci.h |    3 +++
>  3 files changed, 32 insertions(+), 0 deletions(-)
>
[...] 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0fd1f15..10d190b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -342,6 +342,7 @@ struct pci_dev {
>  	struct list_head msi_list;
>  	struct kset *msi_kset;
>  #endif
> +	u64 sn;		/* device serieal number, 0 if not support */

Typo: serieal

>  	struct pci_vpd *vpd;
>  #ifdef CONFIG_PCI_ATS
>  	union {

[...]


Paul Bolle

Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support

[Yijing Wang]

>>
> [...] 
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 0fd1f15..10d190b 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -342,6 +342,7 @@ struct pci_dev {
>>  	struct list_head msi_list;
>>  	struct kset *msi_kset;
>>  #endif
>> +	u64 sn;		/* device serieal number, 0 if not support */
> 
> Typo: serieal


Thanks very much! Will update.


> 
>>  	struct pci_vpd *vpd;
>>  #ifdef CONFIG_PCI_ATS
>>  	union {
> 
> [...]
> 
> 
> Paul Bolle
> 
> 
> .
> 


-- 
Thanks!
Yijing

[PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume

[Yijing Wang]

Currently, pciehp_resume() try to hot add device if the slot adapter
status return true. But if there are already some devices exist,
namely list_empty(bus->devices) return false. We should not add the device
again, because the device add action will fail. Also print some uncomfortable
messages like this:
	pciehp 0000:00:1c.1:pcie04: Device 0000:03:00.0 already exists at 0000:03:00, cannot hot-add
	pciehp 0000:00:1c.1:pcie04: Cannot add device at 0000:03:00

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 |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 7d72c5e..1542735 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -300,6 +300,7 @@ static int pciehp_resume (struct pcie_device *dev)
 {
 	struct controller *ctrl;
 	struct slot *slot;
+	struct pci_bus *pbus = dev->port->subordinate;
 	u8 status;
 
 	ctrl = get_service_data(dev);
@@ -311,10 +312,12 @@ static int pciehp_resume (struct pcie_device *dev)
 
 	/* Check if slot is occupied */
 	pciehp_get_adapter_status(slot, &status);
-	if (status)
-		pciehp_enable_slot(slot);
-	else
+	if (status) {
+		if (list_empty(&pbus->devices))
+			pciehp_enable_slot(slot);
+	} else if (!list_empty(&pbus->devices))
 		pciehp_disable_slot(slot);
+
 	return 0;
 }
 #endif /* PM */
-- 
1.7.1

Re: [PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume

[Paul Bolle]

On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
> Currently, pciehp_resume() try to hot add device if the slot adapter
> status return true. But if there are already some devices exist,
> namely list_empty(bus->devices) return false. We should not add the device
> again, because the device add action will fail. Also print some uncomfortable
> messages like this:
> 	pciehp 0000:00:1c.1:pcie04: Device 0000:03:00.0 already exists at 0000:03:00, cannot hot-add
> 	pciehp 0000:00:1c.1:pcie04: Cannot add device at 0000:03:00
> 
> 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 |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 7d72c5e..1542735 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
[...]
> @@ -311,10 +312,12 @@ static int pciehp_resume (struct pcie_device *dev)
>  
>  	/* Check if slot is occupied */
>  	pciehp_get_adapter_status(slot, &status);
> -	if (status)
> -		pciehp_enable_slot(slot);
> -	else
> +	if (status) {
> +		if (list_empty(&pbus->devices))
> +			pciehp_enable_slot(slot);
> +	} else if (!list_empty(&pbus->devices))
>  		pciehp_disable_slot(slot);
> +

Coding style: braces for the "else if" branch too? Or change the first
test to "if (status && list_empty([...]))" and drop the braces?

>  	return 0;
>  }
>  #endif /* PM */


Paul Bolle

Re: [PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume

[Yijing Wang]

>> ---
>>  drivers/pci/hotplug/pciehp_core.c |    9 ++++++---
>>  1 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
>> index 7d72c5e..1542735 100644
>> --- a/drivers/pci/hotplug/pciehp_core.c
>> +++ b/drivers/pci/hotplug/pciehp_core.c
> [...]
>> @@ -311,10 +312,12 @@ static int pciehp_resume (struct pcie_device *dev)
>>  
>>  	/* Check if slot is occupied */
>>  	pciehp_get_adapter_status(slot, &status);
>> -	if (status)
>> -		pciehp_enable_slot(slot);
>> -	else
>> +	if (status) {
>> +		if (list_empty(&pbus->devices))
>> +			pciehp_enable_slot(slot);
>> +	} else if (!list_empty(&pbus->devices))
>>  		pciehp_disable_slot(slot);
>> +
> 
> Coding style: braces for the "else if" branch too? Or change the first
> test to "if (status && list_empty([...]))" and drop the braces?

Hmmm, I will add the braces for "else if "

Change the first test "if (status && list_empty([...]))" is not a good idea,
because this change will modify the code logic, for example
if a device was present before suspend and still there during resume, we should do nothing,
but after the logic change, we may disable it.

> 
>>  	return 0;
>>  }
>>  #endif /* PM */
> 
> 
> Paul Bolle
> 
> 
> .
> 


-- 
Thanks!
Yijing

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

[Yijing Wang]

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);
+	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);
 	
-- 
1.7.1

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

[Don Dutile]

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);
>   	

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

[Paul Bolle]

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

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

[Yijing Wang]

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
> 
> 
> .
> 


-- 
Thanks!
Yijing

Re: [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume

[Paul Bolle]

On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
> v1->v2: Modify pci_get_dsn to pci_device_serial_number,
> 	    power off slot before remove the old device during resume to avoid
> 		old .remove() method to touch new hardware.
> 		Fix other typo and fail check problems.
> 		Split the list_empty() guard into new patch.
> 		
> 
> Yijing Wang (3):
>   PCI: introduce PCIe Device Serial Number Capability support
>   PCI,pciehp: avoid add a device already exist before suspend during
>     resume
>   PCI,pciehp: use PCIe DSN to identify device change during suspend
> 
>  drivers/pci/hotplug/pciehp_core.c |   54 ++++++++++++++++++++++++++++++++++--
>  drivers/pci/pci.c                 |   27 ++++++++++++++++++
>  drivers/pci/probe.c               |    2 +
>  include/linux/pci.h               |    3 ++
>  4 files changed, 83 insertions(+), 3 deletions(-)

Series applies cleanly to v3.10 (but there was a small problem with 3/3,
which I'll mention in a reply to that patch). Compiles without warning.
Those two errors on every resume are now gone!

Thanks.


Paul Bolle

Re: [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume

[Yijing Wang]

On 2013/7/11 22:19, Paul Bolle wrote:
> On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
>> v1->v2: Modify pci_get_dsn to pci_device_serial_number,
>> 	    power off slot before remove the old device during resume to avoid
>> 		old .remove() method to touch new hardware.
>> 		Fix other typo and fail check problems.
>> 		Split the list_empty() guard into new patch.
>> 		
>>
>> Yijing Wang (3):
>>   PCI: introduce PCIe Device Serial Number Capability support
>>   PCI,pciehp: avoid add a device already exist before suspend during
>>     resume
>>   PCI,pciehp: use PCIe DSN to identify device change during suspend
>>
>>  drivers/pci/hotplug/pciehp_core.c |   54 ++++++++++++++++++++++++++++++++++--
>>  drivers/pci/pci.c                 |   27 ++++++++++++++++++
>>  drivers/pci/probe.c               |    2 +
>>  include/linux/pci.h               |    3 ++
>>  4 files changed, 83 insertions(+), 3 deletions(-)
> 
> Series applies cleanly to v3.10 (but there was a small problem with 3/3,
> which I'll mention in a reply to that patch). Compiles without warning.
> Those two errors on every resume are now gone!

I will check patch 3/3, Paul, Do you wireless card support Device Serial Number ?
You can confirm it by lspci -vvv.

> 
> Thanks.
> 
> 
> Paul Bolle
> 
> 
> .
> 


-- 
Thanks!
Yijing

Re: [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume

[Paul Bolle]

Yijing,

On Fri, 2013-07-12 at 09:52 +0800, Yijing Wang wrote:
> Do you wireless card support Device Serial Number ?
> You can confirm it by lspci -vvv.

Yes, it does:
    lspci -vvv -s 03:00 | grep Serial
        Capabilities: [140 v1] Device Serial Number [...]


Paul Bolle

Re: [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume

[Yijing Wang]

On 2013/7/13 18:20, Paul Bolle wrote:
> Yijing,
> 
> On Fri, 2013-07-12 at 09:52 +0800, Yijing Wang wrote:
>> Do you wireless card support Device Serial Number ?
>> You can confirm it by lspci -vvv.
> 
> Yes, it does:
>     lspci -vvv -s 03:00 | grep Serial
>         Capabilities: [140 v1] Device Serial Number [...]
> 

OK, I got it, thanks!

> 
> Paul Bolle
> 
> 
> 


-- 
Thanks!
Yijing