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

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

Comments

Yijing Wang - July 11, 2013, 9:43 a.m.
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(-)
Don Dutile - July 11, 2013, 9:51 a.m.
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);

--
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:22 p.m.
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

--
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
Don Dutile - July 11, 2013, 6:18 p.m.
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.

--
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
Bjorn Helgaas - July 11, 2013, 8:09 p.m.
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.
--
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, 1:38 a.m.
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);
> 
> 
> .
>
Yijing Wang - July 12, 2013, 1:55 a.m.
>>
> [...] 
>> 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
> 
> 
> .
>
Yijing Wang - July 12, 2013, 2:30 a.m.
>> }
>>>
>>> +       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.


> 



> .
>
Yijing Wang - July 12, 2013, 2:39 a.m.
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.
> 
> 
> .
>
Don Dutile - July 12, 2013, 2:43 a.m.
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);
>>
>>
>> .
>>
>
>

--
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
Bjorn Helgaas - July 12, 2013, 3:37 a.m.
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
--
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

Patch

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