| Message ID | 1373535825-49972-2-git-send-email-wangyijing@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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); > > > . >
>> > [...] >> 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 > > > . >
>> } >>> >>> + 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. > > . >
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. > > > . >
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
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
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);
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(-)