| Message ID | 20200227223635.1021197-3-jacob.e.keller@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | pci: implement function to read Device Serial Number | expand |
From: Jacob Keller <jacob.e.keller@intel.com> Date: Thu, 27 Feb 2020 14:36:31 -0800 > +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) > +{ > + u32 dword; > + int pos; > + > + Just one empty line after the local variable declarations please. Thank you.
On 2/29/2020 9:27 PM, David Miller wrote: > From: Jacob Keller <jacob.e.keller@intel.com> > Date: Thu, 27 Feb 2020 14:36:31 -0800 > >> +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) >> +{ >> + u32 dword; >> + int pos; >> + >> + > > Just one empty line after the local variable declarations please. > > Thank you. > I've fixed this locally, but am going to wait to see if there is any further feedback before sending a v2. Thanks, Jake
PCI: Introduce pci_get_dsn() I learned this from "git log --oneline drivers/pci/pci.c". It looks like the other patches could benefit from this as well. On Thu, Feb 27, 2020 at 02:36:31PM -0800, Jacob Keller wrote: > Several device drivers read their Device Serial Number from the PCIe > extended config space. > > Introduce a new helper function, pci_get_dsn, which will read the > eight bytes of the DSN into the provided buffer. "pci_get_dsn()" > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > Cc: QLogic-Storage-Upstream@cavium.com > Cc: Michael Chan <michael.chan@broadcom.com> > --- > drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ > include/linux/pci.h | 5 +++++ > 2 files changed, 38 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index d828ca835a98..12d8101724d7 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -33,6 +33,7 @@ > #include <linux/pci-ats.h> > #include <asm/setup.h> > #include <asm/dma.h> > +#include <asm/unaligned.h> > #include <linux/aer.h> > #include "pci.h" > > @@ -557,6 +558,38 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap) > } > EXPORT_SYMBOL_GPL(pci_find_ext_capability); > > +/** > + * pci_get_dsn - Read the 8-byte Device Serial Number > + * @dev: PCI device to query > + * @dsn: storage for the DSN. Must be at least 8 bytes > + * > + * Looks up the PCI_EXT_CAP_ID_DSN and reads the 8 bytes into the dsn storage. > + * Returns -EOPNOTSUPP if the device does not have the capability. > + */ > +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) > +{ > + u32 dword; > + int pos; > + > + > + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN); > + if (!pos) > + return -EOPNOTSUPP; > + > + /* > + * The Device Serial Number is two dwords offset 4 bytes from the > + * capability position. > + */ > + pos += 4; > + pci_read_config_dword(dev, pos, &dword); > + put_unaligned_le32(dword, &dsn[0]); > + pci_read_config_dword(dev, pos + 4, &dword); > + put_unaligned_le32(dword, &dsn[4]); Since the serial number is a 64-bit value, can we just return a u64 and let the caller worry about any alignment and byte-order issues? This would be the only use of asm/unaligned.h in driver/pci, and I don't think DSN should be that special. I think it's OK if we return 0 if the device doesn't have a DSN capability. A DSN that actually contains a zero serial number would be dubious at best. > + return 0; > +} > +EXPORT_SYMBOL_GPL(pci_get_dsn); > + > static int __pci_find_next_ht_cap(struct pci_dev *dev, int pos, int ht_cap) > { > int rc, ttl = PCI_FIND_CAP_TTL; > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 3840a541a9de..883562323df3 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1045,6 +1045,8 @@ int pci_find_ht_capability(struct pci_dev *dev, int ht_cap); > int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap); > struct pci_bus *pci_find_next_bus(const struct pci_bus *from); > > +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]); > + > struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, > struct pci_dev *from); > struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device, > @@ -1699,6 +1701,9 @@ static inline int pci_find_next_capability(struct pci_dev *dev, u8 post, > static inline int pci_find_ext_capability(struct pci_dev *dev, int cap) > { return 0; } > > +static inline int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) > +{ return -EOPNOTSUPP; } > + > /* Power management related routines */ > static inline int pci_save_state(struct pci_dev *dev) { return 0; } > static inline void pci_restore_state(struct pci_dev *dev) { } > -- > 2.25.0.368.g28a2d05eebfb >
On 3/2/2020 2:25 PM, Bjorn Helgaas wrote: > PCI: Introduce pci_get_dsn() > > I learned this from "git log --oneline drivers/pci/pci.c". It looks > like the other patches could benefit from this as well. > Sure, will follow that precedent. > On Thu, Feb 27, 2020 at 02:36:31PM -0800, Jacob Keller wrote: >> Several device drivers read their Device Serial Number from the PCIe >> extended config space. >> >> Introduce a new helper function, pci_get_dsn, which will read the >> eight bytes of the DSN into the provided buffer. > > "pci_get_dsn()" > >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >> Cc: QLogic-Storage-Upstream@cavium.com >> Cc: Michael Chan <michael.chan@broadcom.com> >> --- >> drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ >> include/linux/pci.h | 5 +++++ >> 2 files changed, 38 insertions(+) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index d828ca835a98..12d8101724d7 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -33,6 +33,7 @@ >> #include <linux/pci-ats.h> >> #include <asm/setup.h> >> #include <asm/dma.h> >> +#include <asm/unaligned.h> >> #include <linux/aer.h> >> #include "pci.h" >> >> @@ -557,6 +558,38 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap) >> } >> EXPORT_SYMBOL_GPL(pci_find_ext_capability); >> >> +/** >> + * pci_get_dsn - Read the 8-byte Device Serial Number >> + * @dev: PCI device to query >> + * @dsn: storage for the DSN. Must be at least 8 bytes >> + * >> + * Looks up the PCI_EXT_CAP_ID_DSN and reads the 8 bytes into the dsn storage. >> + * Returns -EOPNOTSUPP if the device does not have the capability. >> + */ >> +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) >> +{ >> + u32 dword; >> + int pos; >> + >> + >> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN); >> + if (!pos) >> + return -EOPNOTSUPP; >> + >> + /* >> + * The Device Serial Number is two dwords offset 4 bytes from the >> + * capability position. >> + */ >> + pos += 4; >> + pci_read_config_dword(dev, pos, &dword); >> + put_unaligned_le32(dword, &dsn[0]); >> + pci_read_config_dword(dev, pos + 4, &dword); >> + put_unaligned_le32(dword, &dsn[4]); > > Since the serial number is a 64-bit value, can we just return a u64 > and let the caller worry about any alignment and byte-order issues? > > This would be the only use of asm/unaligned.h in driver/pci, and I > don't think DSN should be that special. I suppose that's fair, but it ends up leaving most callers having to fix this immediately after calling this function. > > I think it's OK if we return 0 if the device doesn't have a DSN > capability. A DSN that actually contains a zero serial number would > be dubious at best. Hmm. I was trying to match how pre-existing code behaved, based on the ice and bnxt drivers. By returning 0s, we'd have to then perform a memcmp or something to catch it. > >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(pci_get_dsn); >> + >> static int __pci_find_next_ht_cap(struct pci_dev *dev, int pos, int ht_cap) >> { >> int rc, ttl = PCI_FIND_CAP_TTL; >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 3840a541a9de..883562323df3 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -1045,6 +1045,8 @@ int pci_find_ht_capability(struct pci_dev *dev, int ht_cap); >> int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap); >> struct pci_bus *pci_find_next_bus(const struct pci_bus *from); >> >> +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]); >> + >> struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, >> struct pci_dev *from); >> struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device, >> @@ -1699,6 +1701,9 @@ static inline int pci_find_next_capability(struct pci_dev *dev, u8 post, >> static inline int pci_find_ext_capability(struct pci_dev *dev, int cap) >> { return 0; } >> >> +static inline int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) >> +{ return -EOPNOTSUPP; } >> + >> /* Power management related routines */ >> static inline int pci_save_state(struct pci_dev *dev) { return 0; } >> static inline void pci_restore_state(struct pci_dev *dev) { } >> -- >> 2.25.0.368.g28a2d05eebfb >>
On Mon, Mar 02, 2020 at 02:33:12PM -0800, Jacob Keller wrote: > On 3/2/2020 2:25 PM, Bjorn Helgaas wrote: > >> +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) > >> +{ > >> + u32 dword; > >> + int pos; > >> + > >> + > >> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN); > >> + if (!pos) > >> + return -EOPNOTSUPP; > >> + > >> + /* > >> + * The Device Serial Number is two dwords offset 4 bytes from the > >> + * capability position. > >> + */ > >> + pos += 4; > >> + pci_read_config_dword(dev, pos, &dword); > >> + put_unaligned_le32(dword, &dsn[0]); > >> + pci_read_config_dword(dev, pos + 4, &dword); > >> + put_unaligned_le32(dword, &dsn[4]); > > > > Since the serial number is a 64-bit value, can we just return a u64 > > and let the caller worry about any alignment and byte-order issues? > > > > This would be the only use of asm/unaligned.h in driver/pci, and I > > don't think DSN should be that special. > > I suppose that's fair, but it ends up leaving most callers having to fix > this immediately after calling this function. PCIe doesn't impose any structure on the value; it just says the first dword is the lower DW and the second is the upper DW. As long as we put that together correctly into a u64, I think further interpretation is caller-specific. > > I think it's OK if we return 0 if the device doesn't have a DSN > > capability. A DSN that actually contains a zero serial number would > > be dubious at best. > > Hmm. I was trying to match how pre-existing code behaved, based on the > ice and bnxt drivers. > > By returning 0s, we'd have to then perform a memcmp or something to > catch it. Can you just do this: dsn = pci_get_dsn(pdev); if (!dsn) return NULL; snprintf(opt_fw_filename, ...); return opt_fw_filename; Or am I missing something? > >> + return 0; > >> +} > >> +EXPORT_SYMBOL_GPL(pci_get_dsn); > >> + > >> static int __pci_find_next_ht_cap(struct pci_dev *dev, int pos, int ht_cap) > >> { > >> int rc, ttl = PCI_FIND_CAP_TTL; > >> diff --git a/include/linux/pci.h b/include/linux/pci.h > >> index 3840a541a9de..883562323df3 100644 > >> --- a/include/linux/pci.h > >> +++ b/include/linux/pci.h > >> @@ -1045,6 +1045,8 @@ int pci_find_ht_capability(struct pci_dev *dev, int ht_cap); > >> int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap); > >> struct pci_bus *pci_find_next_bus(const struct pci_bus *from); > >> > >> +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]); > >> + > >> struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, > >> struct pci_dev *from); > >> struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device, > >> @@ -1699,6 +1701,9 @@ static inline int pci_find_next_capability(struct pci_dev *dev, u8 post, > >> static inline int pci_find_ext_capability(struct pci_dev *dev, int cap) > >> { return 0; } > >> > >> +static inline int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) > >> +{ return -EOPNOTSUPP; } > >> + > >> /* Power management related routines */ > >> static inline int pci_save_state(struct pci_dev *dev) { return 0; } > >> static inline void pci_restore_state(struct pci_dev *dev) { } > >> -- > >> 2.25.0.368.g28a2d05eebfb > >>
On 3/2/2020 3:20 PM, Bjorn Helgaas wrote: > On Mon, Mar 02, 2020 at 02:33:12PM -0800, Jacob Keller wrote: >> On 3/2/2020 2:25 PM, Bjorn Helgaas wrote: > >>>> +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) >>>> +{ >>>> + u32 dword; >>>> + int pos; >>>> + >>>> + >>>> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN); >>>> + if (!pos) >>>> + return -EOPNOTSUPP; >>>> + >>>> + /* >>>> + * The Device Serial Number is two dwords offset 4 bytes from the >>>> + * capability position. >>>> + */ >>>> + pos += 4; >>>> + pci_read_config_dword(dev, pos, &dword); >>>> + put_unaligned_le32(dword, &dsn[0]); >>>> + pci_read_config_dword(dev, pos + 4, &dword); >>>> + put_unaligned_le32(dword, &dsn[4]); >>> >>> Since the serial number is a 64-bit value, can we just return a u64 >>> and let the caller worry about any alignment and byte-order issues? >>> >>> This would be the only use of asm/unaligned.h in driver/pci, and I >>> don't think DSN should be that special. >> >> I suppose that's fair, but it ends up leaving most callers having to fix >> this immediately after calling this function. > > PCIe doesn't impose any structure on the value; it just says the first > dword is the lower DW and the second is the upper DW. As long as we > put that together correctly into a u64, I think further interpretation > is caller-specific. > Makes sense. So basically, convert pci_get_dsn to a simply return a u64 instead of copying to an array, and then make callers assume that a value of 0 is invalid? Thanks, Jake
On Mon, Mar 2, 2020 at 5:24 PM Jacob Keller <jacob.e.keller@intel.com> wrote: > > On 3/2/2020 3:20 PM, Bjorn Helgaas wrote: > > On Mon, Mar 02, 2020 at 02:33:12PM -0800, Jacob Keller wrote: > >> On 3/2/2020 2:25 PM, Bjorn Helgaas wrote: > > > >>>> +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) > >>>> +{ > >>>> + u32 dword; > >>>> + int pos; > >>>> + > >>>> + > >>>> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN); > >>>> + if (!pos) > >>>> + return -EOPNOTSUPP; > >>>> + > >>>> + /* > >>>> + * The Device Serial Number is two dwords offset 4 bytes from the > >>>> + * capability position. > >>>> + */ > >>>> + pos += 4; > >>>> + pci_read_config_dword(dev, pos, &dword); > >>>> + put_unaligned_le32(dword, &dsn[0]); > >>>> + pci_read_config_dword(dev, pos + 4, &dword); > >>>> + put_unaligned_le32(dword, &dsn[4]); > >>> > >>> Since the serial number is a 64-bit value, can we just return a u64 > >>> and let the caller worry about any alignment and byte-order issues? > >>> > >>> This would be the only use of asm/unaligned.h in driver/pci, and I > >>> don't think DSN should be that special. > >> > >> I suppose that's fair, but it ends up leaving most callers having to fix > >> this immediately after calling this function. > > > > PCIe doesn't impose any structure on the value; it just says the first > > dword is the lower DW and the second is the upper DW. As long as we > > put that together correctly into a u64, I think further interpretation > > is caller-specific. > > Makes sense. So basically, convert pci_get_dsn to a simply return a u64 > instead of copying to an array, and then make callers assume that a > value of 0 is invalid? Yep, that's what I would do. You might have to re-jigger the snprintfs so they still pull out the same bytes they did before.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index d828ca835a98..12d8101724d7 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -33,6 +33,7 @@ #include <linux/pci-ats.h> #include <asm/setup.h> #include <asm/dma.h> +#include <asm/unaligned.h> #include <linux/aer.h> #include "pci.h" @@ -557,6 +558,38 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap) } EXPORT_SYMBOL_GPL(pci_find_ext_capability); +/** + * pci_get_dsn - Read the 8-byte Device Serial Number + * @dev: PCI device to query + * @dsn: storage for the DSN. Must be at least 8 bytes + * + * Looks up the PCI_EXT_CAP_ID_DSN and reads the 8 bytes into the dsn storage. + * Returns -EOPNOTSUPP if the device does not have the capability. + */ +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) +{ + u32 dword; + int pos; + + + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN); + if (!pos) + return -EOPNOTSUPP; + + /* + * The Device Serial Number is two dwords offset 4 bytes from the + * capability position. + */ + pos += 4; + pci_read_config_dword(dev, pos, &dword); + put_unaligned_le32(dword, &dsn[0]); + pci_read_config_dword(dev, pos + 4, &dword); + put_unaligned_le32(dword, &dsn[4]); + + return 0; +} +EXPORT_SYMBOL_GPL(pci_get_dsn); + static int __pci_find_next_ht_cap(struct pci_dev *dev, int pos, int ht_cap) { int rc, ttl = PCI_FIND_CAP_TTL; diff --git a/include/linux/pci.h b/include/linux/pci.h index 3840a541a9de..883562323df3 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1045,6 +1045,8 @@ int pci_find_ht_capability(struct pci_dev *dev, int ht_cap); int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap); struct pci_bus *pci_find_next_bus(const struct pci_bus *from); +int pci_get_dsn(struct pci_dev *dev, u8 dsn[]); + struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, struct pci_dev *from); struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device, @@ -1699,6 +1701,9 @@ static inline int pci_find_next_capability(struct pci_dev *dev, u8 post, static inline int pci_find_ext_capability(struct pci_dev *dev, int cap) { return 0; } +static inline int pci_get_dsn(struct pci_dev *dev, u8 dsn[]) +{ return -EOPNOTSUPP; } + /* Power management related routines */ static inline int pci_save_state(struct pci_dev *dev) { return 0; } static inline void pci_restore_state(struct pci_dev *dev) { }
Several device drivers read their Device Serial Number from the PCIe extended config space. Introduce a new helper function, pci_get_dsn, which will read the eight bytes of the DSN into the provided buffer. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> Cc: QLogic-Storage-Upstream@cavium.com Cc: Michael Chan <michael.chan@broadcom.com> --- drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ include/linux/pci.h | 5 +++++ 2 files changed, 38 insertions(+)