[PATCH 0/5] pci: implement function to read Device Serial Number

[PATCH 0/5] pci: implement function to read Device Serial Number

[Jacob Keller]

Several drivers read the Device Serial Number from the PCIe extended
configuration space. Each of these drivers implements a similar approach to
finding the position and then extracting the 8 bytes of data.

Implement a new helper function, pci_get_dsn, which can be used to extract
this data into an 8 byte array.

Modify the bnxt_en, qedf, ice, and ixgbe driver to use this new function.

I left the implementation in the netronome nfp driver alone because they
appear to extract parts of the DSN into separate locations and the
transformation was not as obvious.

The intent for this is to reduce duplicate code across the various drivers,
and make it easier to write future code that wants to read the DSN. In
particular the ice driver will be using the DSN as its serial number when
implementing the DEVLINK_CMD_INFO_GET.

I'm not entirely sure what tree these patches should go through, since it
includes a core PCI change, as well as changes for both networking drivers
and a scsi driver.

Jacob Keller (5):
  pci: introduce pci_get_dsn
  bnxt_en: use pci_get_dsn
  scsi: qedf: use pci_get_dsn
  ice: use pci_get_dsn
  ixgbe: use pci_get_dsn

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 14 +++-----
 drivers/net/ethernet/intel/ice/ice_main.c     | 32 ++++++++----------
 drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c | 16 ++++-----
 drivers/pci/pci.c                             | 33 +++++++++++++++++++
 drivers/scsi/qedf/qedf_main.c                 | 16 ++++-----
 include/linux/pci.h                           |  5 +++
 6 files changed, 68 insertions(+), 48 deletions(-)

-- 
2.25.0.368.g28a2d05eebfb

[PATCH] ice-shared: add macro specifying max NVM offset

[Jacob Keller]

The ice_aq_read_nvm function uses a somewhat weird construction for
verifying that the incoming offset is valid. Replace this construction
with a simple greater-than expression, and define the maximum value
(24bits) in the ice_adminq_cmd.h

By providing a macro, the check becomes more clear. Additionally the
maximum offset can be used in other locations.

Change-Id: I50993e53aca7c073d6906f48f8c9a6ecc05248d4
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Title: ice-shared: add macro specifying max NVM offset
Change-type: ImplementationChange
Reviewed-on: https://git-amr-3.devtools.intel.com/gerrit/250507
Tested-by: ND Linux CI Server <nd.linux.ci.server@intel.com>
Reviewed-by: Allan, Bruce W <bruce.w.allan@intel.com>
---
 ice_adminq_cmd.h | 1 +
 ice_nvm.c        | 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ice_adminq_cmd.h b/ice_adminq_cmd.h
index 38134e9dab11..fbbe676d1f7d 100644
--- a/ice_adminq_cmd.h
+++ b/ice_adminq_cmd.h
@@ -2778,6 +2778,7 @@ ICE_CHECK_PARAM_LEN(ice_aqc_sff_eeprom);
  * NVM Shadow RAM Dump commands (direct 0x0707)
  */
 struct ice_aqc_nvm {
+#define ICE_AQC_NVM_MAX_OFFSET		0xFFFFFF
 	__le16 offset_low;
 	u8 offset_high;
 #ifdef PREBOOT_SUPPORT
diff --git a/ice_nvm.c b/ice_nvm.c
index b075a05eb38f..0e6d8390deb8 100644
--- a/ice_nvm.c
+++ b/ice_nvm.c
@@ -35,8 +35,7 @@ ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset, u16 length,
 
 	cmd = &desc.params.nvm;
 
-	/* In offset the highest byte must be zeroed. */
-	if (offset & 0xFF000000)
+	if (offset > ICE_AQC_NVM_MAX_OFFSET)
 		return ICE_ERR_PARAM;
 
 	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_nvm_read);
-- 
2.25.0.368.g28a2d05eebfb

Re: [PATCH] ice-shared: add macro specifying max NVM offset

[Jacob Keller]

This patch was sent in error. Please ignore it, as it's not part of the
series.

Thanks,
Jake

[PATCH 1/5] pci: introduce pci_get_dsn

[Jacob Keller]

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(+)

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) { }
-- 
2.25.0.368.g28a2d05eebfb

Re: [PATCH 1/5] pci: introduce pci_get_dsn

[David Miller]

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.

Re: [PATCH 1/5] pci: introduce pci_get_dsn

[Jacob Keller]

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

Re: [PATCH 1/5] pci: introduce pci_get_dsn

[Bjorn Helgaas]

  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
> 

Re: [PATCH 1/5] pci: introduce pci_get_dsn

[Jacob Keller]

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

Re: [PATCH 1/5] pci: introduce pci_get_dsn

[Bjorn Helgaas]

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

Re: [PATCH 1/5] pci: introduce pci_get_dsn

[Jacob Keller]

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

Re: [PATCH 1/5] pci: introduce pci_get_dsn

[Bjorn Helgaas]

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.

[PATCH v2 0/6] PCI: Implement function to read Device Serial Number

[Jacob Keller]

Several drivers read the Device Serial Number from the PCIe extended
configuration space. Each of these drivers implements a similar approach to
finding the position and then extracting the 8 bytes of data.

Implement a new helper function, pci_get_dsn, which can be used to extract
this data into an 8 byte array.

Modify the bnxt_en, qedf, ice, ixgbe and nfp drivers to use this new
function.

The intent for this is to reduce duplicate code across the various drivers,
and make it easier to write future code that wants to read the DSN. In
particular the ice driver will be using the DSN as its serial number when
implementing the DEVLINK_CMD_INFO_GET.

The new implementation in v2 significantly simplifies some of the callers
which just want to print the value out in MSB order. By returning things as
a u64 in CPU Endian order, the "%016llX" printf format specifier can be used
to correctly format the value.

Per patch changes since v1
  PCI: Introduce pci_get_dsn
  * Update commit message based on feedback from Bjorn Helgaas
  * Modify the function to return a u64 (zero on no capability)
  * This new implementation ensures that the first dword is the lower 32
    bits and the second dword is the upper 32 bits.

  bnxt_en: Use pci_get_dsn()
  * Use the u64 return value from pci_get_dsn()
  * Copy it into the dsn[] array by using put_unaligned_le64
  * Fix a pre-existing typo in the netdev_info error message

  scsi: qedf: Use pci_get_dsn()
  * Use the u64 return value from pci_get_dsn()
  * simplify the snprintf to use "%016llX"
  * remove the unused 'i' variable

  ice: Use pci_get_dsn()
  * Use the u64 return value from pci_get_dsn()
  * simplify the snprintf to use "%016llX"

  ixgbe: Use pci_get_dsn()
  * Use the u64 return value from pci_get_dsn()
  * simplify the snprintf to use "%016llX"

  nfp: Use pci_get_dsn()
  * Added in v2

Jacob Keller (6):
  PCI: Introduce pci_get_dsn
  bnxt_en: Use pci_get_dsn()
  scsi: qedf: Use pci_get_dsn()
  ice: Use pci_get_dsn()
  ixgbe: Use pci_get_dsn()
  nfp: Use pci_get_dsn()

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 14 +++-----
 drivers/net/ethernet/intel/ice/ice_main.c     | 30 ++++++----------
 drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c | 18 ++++------
 .../netronome/nfp/nfpcore/nfp6000_pcie.c      | 24 +++++--------
 drivers/pci/pci.c                             | 34 +++++++++++++++++++
 drivers/scsi/qedf/qedf_main.c                 | 19 ++++-------
 include/linux/pci.h                           |  5 +++
 7 files changed, 76 insertions(+), 68 deletions(-)

-- 
2.25.0.368.g28a2d05eebfb

[PATCH v2 1/6] PCI: Introduce pci_get_dsn

[Jacob Keller]

Several device drivers read their Device Serial Number from the PCIe
extended config space.

Introduce a new helper function, pci_get_dsn(). This function reads the
eight bytes of the DSN and returns them as a u64. If the capability does not
exist for the device, the function returns 0.

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: Michael Chan <michael.chan@broadcom.com>
---
 drivers/pci/pci.c   | 34 ++++++++++++++++++++++++++++++++++
 include/linux/pci.h |  5 +++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d828ca835a98..03f50e706c0d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -557,6 +557,40 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap)
 }
 EXPORT_SYMBOL_GPL(pci_find_ext_capability);
 
+/**
+ * pci_get_dsn - Read and return the 8-byte Device Serial Number
+ * @dev: PCI device to query
+ *
+ * Looks up the PCI_EXT_CAP_ID_DSN and reads the 8 bytes of the Device Serial
+ * Number.
+ *
+ * Returns the DSN, or zero if the capability does not exist.
+ */
+u64 pci_get_dsn(struct pci_dev *dev)
+{
+	u32 dword;
+	u64 dsn;
+	int pos;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
+	if (!pos)
+		return 0;
+
+	/*
+	 * The Device Serial Number is two dwords offset 4 bytes from the
+	 * capability position. The specification says that the first dword is
+	 * the lower half, and the second dword is the upper half.
+	 */
+	pos += 4;
+	pci_read_config_dword(dev, pos, &dword);
+	dsn = (u64)dword;
+	pci_read_config_dword(dev, pos + 4, &dword);
+	dsn |= ((u64)dword) << 32;
+
+	return dsn;
+}
+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..d92cf2da1ae1 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);
 
+u64 pci_get_dsn(struct pci_dev *dev);
+
 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 u64 pci_get_dsn(struct pci_dev *dev)
+{ return 0; }
+
 /* 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

Re: [PATCH v2 1/6] PCI: Introduce pci_get_dsn

[Bjorn Helgaas]

On Mon, Mar 02, 2020 at 06:25:00PM -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(). This function reads the
> eight bytes of the DSN and returns them as a u64. If the capability does not
> exist for the device, the function returns 0.
> 
> 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: Michael Chan <michael.chan@broadcom.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Looks good, thanks!  Feel free to merge this via the net tree.

> ---
>  drivers/pci/pci.c   | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |  5 +++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d828ca835a98..03f50e706c0d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -557,6 +557,40 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap)
>  }
>  EXPORT_SYMBOL_GPL(pci_find_ext_capability);
>  
> +/**
> + * pci_get_dsn - Read and return the 8-byte Device Serial Number
> + * @dev: PCI device to query
> + *
> + * Looks up the PCI_EXT_CAP_ID_DSN and reads the 8 bytes of the Device Serial
> + * Number.
> + *
> + * Returns the DSN, or zero if the capability does not exist.
> + */
> +u64 pci_get_dsn(struct pci_dev *dev)
> +{
> +	u32 dword;
> +	u64 dsn;
> +	int pos;
> +
> +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
> +	if (!pos)
> +		return 0;
> +
> +	/*
> +	 * The Device Serial Number is two dwords offset 4 bytes from the
> +	 * capability position. The specification says that the first dword is
> +	 * the lower half, and the second dword is the upper half.
> +	 */
> +	pos += 4;
> +	pci_read_config_dword(dev, pos, &dword);
> +	dsn = (u64)dword;
> +	pci_read_config_dword(dev, pos + 4, &dword);
> +	dsn |= ((u64)dword) << 32;
> +
> +	return dsn;
> +}
> +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..d92cf2da1ae1 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);
>  
> +u64 pci_get_dsn(struct pci_dev *dev);
> +
>  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 u64 pci_get_dsn(struct pci_dev *dev)
> +{ return 0; }
> +
>  /* 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
> 

[PATCH v2 2/6] bnxt_en: Use pci_get_dsn()

[Jacob Keller]

Replace the open-coded implementation for reading the PCIe DSN with
pci_get_dsn().

Use of put_unaligned_le64 should be correct. pci_get_dsn() will perform
two pci_read_config_dword calls. The first dword will be placed in the
first 32 bits of the u64, while the second dword will be placed in the
upper 32 bits of the u64.

On Little Endian systems, the least significant byte comes first, which
will be the least significant byte of the first dword, followed by the
least significant byte of the second dword. Since the _le32 variations
do not perform byte swapping, we will correctly copy the dwords into the
dsn[] array in the same order as before.

On Big Endian systems, the most significant byte of the second dword
will come first. put_unaligned_le64 will perform a CPU_TO_LE64, which
will swap things correctly before copying. This should also end up with
the correct bytes in the dsn[] array.

While at it, fix a small typo in the netdev_info error message when the
DSN cannot be read.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 597e6fd5bfea..49874079084d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11755,20 +11755,16 @@ static int bnxt_init_mac_addr(struct bnxt *bp)
 static int bnxt_pcie_dsn_get(struct bnxt *bp, u8 dsn[])
 {
 	struct pci_dev *pdev = bp->pdev;
-	int pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DSN);
-	u32 dw;
+	u64 qword;
 
-	if (!pos) {
-		netdev_info(bp->dev, "Unable do read adapter's DSN");
+	qword = pci_get_dsn(pdev);
+	if (!qword) {
+		netdev_info(bp->dev, "Unable to read adapter's DSN");
 		return -EOPNOTSUPP;
 	}
 
-	/* DSN (two dw) is at an offset of 4 from the cap pos */
-	pos += 4;
-	pci_read_config_dword(pdev, pos, &dw);
-	put_unaligned_le32(dw, &dsn[0]);
-	pci_read_config_dword(pdev, pos + 4, &dw);
-	put_unaligned_le32(dw, &dsn[4]);
+	put_unaligned_le64(qword, dsn);
+
 	bp->flags |= BNXT_FLAG_DSN_VALID;
 	return 0;
 }
-- 
2.25.0.368.g28a2d05eebfb

[PATCH v2 3/6] scsi: qedf: Use pci_get_dsn()

[Jacob Keller]

Replace the open-coded implementation for reading the PCIe DSN with
pci_get_dsn().

The original code used a for-loop that looped over each of the 8 bytes
and copied them into a temporary buffer. pci_get_dsn() uses two calls to
pci_read_config_dword, and correctly bitwise ORs them into a u64. Thus,
we can simplify the snprintf significantly using %016llX on a u64 value.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
The QLogic-Storage-Upstream@cavium.com appears to be a dud. I'm not sure who
maintains this driver, and thus am not sure who to add to the Cc.

 drivers/scsi/qedf/qedf_main.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index 604856e72cfb..5b19f5175c5c 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -1577,8 +1577,7 @@ static void qedf_setup_fdmi(struct qedf_ctx *qedf)
 {
 	struct fc_lport *lport = qedf->lport;
 	struct fc_host_attrs *fc_host = shost_to_fc_host(lport->host);
-	u8 buf[8];
-	int i, pos;
+	u64 dsn;
 
 	/*
 	 * fdmi_enabled needs to be set for libfc to execute FDMI registration.
@@ -1591,18 +1590,11 @@ static void qedf_setup_fdmi(struct qedf_ctx *qedf)
 	 */
 
 	/* Get the PCI-e Device Serial Number Capability */
-	pos = pci_find_ext_capability(qedf->pdev, PCI_EXT_CAP_ID_DSN);
-	if (pos) {
-		pos += 4;
-		for (i = 0; i < 8; i++)
-			pci_read_config_byte(qedf->pdev, pos + i, &buf[i]);
-
+	dsn = pci_get_dsn(qedf->pdev);
+	if (dsn)
 		snprintf(fc_host->serial_number,
-		    sizeof(fc_host->serial_number),
-		    "%02X%02X%02X%02X%02X%02X%02X%02X",
-		    buf[7], buf[6], buf[5], buf[4],
-		    buf[3], buf[2], buf[1], buf[0]);
-	} else
+		    sizeof(fc_host->serial_number), "%016llX", dsn);
+	else
 		snprintf(fc_host->serial_number,
 		    sizeof(fc_host->serial_number), "Unknown");
 
-- 
2.25.0.368.g28a2d05eebfb

[PATCH v2 4/6] ice: Use pci_get_dsn()

[Jacob Keller]

Replace the open-coded implementation for reading the PCIe DSN with
pci_get_dsn().

The pci_get_dsn() function will perform two pci_read_config_dword calls
to read the lower and upper config dwords. It bitwise ORs them into
a u64 value. Instead of using put_unaligned_le32 to convert the value to
LE32 format, just use the %016llX printf specifier. This will print the
u64 correct, putting the most significant byte of the value first. Since
pci_get_dsn() correctly orders the two dwords into a u64, this should
produce equivalent results in less code.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 30 +++++++++--------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 5ae671609f98..c633519b1555 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3099,30 +3099,22 @@ static char *ice_get_opt_fw_name(struct ice_pf *pf)
 	 * followed by a EUI-64 identifier (PCIe Device Serial Number)
 	 */
 	struct pci_dev *pdev = pf->pdev;
-	char *opt_fw_filename = NULL;
-	u32 dword;
-	u8 dsn[8];
-	int pos;
+	char *opt_fw_filename;
+	u64 dsn;
 
 	/* Determine the name of the optional file using the DSN (two
 	 * dwords following the start of the DSN Capability).
 	 */
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DSN);
-	if (pos) {
-		opt_fw_filename = kzalloc(NAME_MAX, GFP_KERNEL);
-		if (!opt_fw_filename)
-			return NULL;
+	dsn = pci_get_dsn(pdev);
+	if (!dsn)
+		return NULL;
 
-		pci_read_config_dword(pdev, pos + 4, &dword);
-		put_unaligned_le32(dword, &dsn[0]);
-		pci_read_config_dword(pdev, pos + 8, &dword);
-		put_unaligned_le32(dword, &dsn[4]);
-		snprintf(opt_fw_filename, NAME_MAX,
-			 "%sice-%02x%02x%02x%02x%02x%02x%02x%02x.pkg",
-			 ICE_DDP_PKG_PATH,
-			 dsn[7], dsn[6], dsn[5], dsn[4],
-			 dsn[3], dsn[2], dsn[1], dsn[0]);
-	}
+	opt_fw_filename = kzalloc(NAME_MAX, GFP_KERNEL);
+	if (!opt_fw_filename)
+		return NULL;
+
+	snprintf(opt_fw_filename, NAME_MAX, "%sice-%016llX.pkg",
+		 ICE_DDP_PKG_PATH, dsn);
 
 	return opt_fw_filename;
 }
-- 
2.25.0.368.g28a2d05eebfb

[PATCH v2 5/6] ixgbe: Use pci_get_dsn()

[Jacob Keller]

Replace the open-coded implementation for reading the PCIe DSN with
pci_get_dsn().

The original code used a simple for-loop to read the bytes in order into
a buffer one byte at a time.

The pci_get_dsn() function returns the DSN as a u64, correctly ordering
the upper and lower 32 bit dwords. Simplify the display code by using
%016llX to display the u64 DSN.

This should have equivalent behavior on both Little and Big Endian
systems. The bus will have correctly ordered the dwords in the CPU
endian format, while pci_get_dsn() will correctly order the lower and
higher dwords into a u64.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
index ccd852ad62a4..d733165c4a2e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
@@ -968,8 +968,8 @@ int ixgbe_fcoe_get_hbainfo(struct net_device *netdev,
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	struct ixgbe_hw *hw = &adapter->hw;
-	int i, pos;
-	u8 buf[8];
+	u64 dsn;
+	int i;
 
 	if (!info)
 		return -EINVAL;
@@ -985,17 +985,11 @@ int ixgbe_fcoe_get_hbainfo(struct net_device *netdev,
 	/* Serial Number */
 
 	/* Get the PCI-e Device Serial Number Capability */
-	pos = pci_find_ext_capability(adapter->pdev, PCI_EXT_CAP_ID_DSN);
-	if (pos) {
-		pos += 4;
-		for (i = 0; i < 8; i++)
-			pci_read_config_byte(adapter->pdev, pos + i, &buf[i]);
-
+	dsn = pci_get_dsn(adapter->pdev);
+	if (dsn)
 		snprintf(info->serial_number, sizeof(info->serial_number),
-			 "%02X%02X%02X%02X%02X%02X%02X%02X",
-			 buf[7], buf[6], buf[5], buf[4],
-			 buf[3], buf[2], buf[1], buf[0]);
-	} else
+			 "%016llX", dsn);
+	else
 		snprintf(info->serial_number, sizeof(info->serial_number),
 			 "Unknown");
 
-- 
2.25.0.368.g28a2d05eebfb

[PATCH v2 6/6] nfp: Use pci_get_dsn()

[Jacob Keller]

Use the newly added pci_get_dsn() function for obtaining the 64-bit
Device Serial Number in the nfp6000_read_serial and
nfp_6000_get_interface functions.

pci_get_dsn() reports the Device Serial number as a u64 value created by
combining two pci_read_config_dword functions. The lower 16 bits
represent the device interface value, and the next 48 bits represent the
serial value. Use put_unaligned_be32 and put_unaligned_be16 to convert
the serial value portion into a Big Endian formatted serial u8 array.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jakub Kicinski <kuba@kernel.org>
---
 .../netronome/nfp/nfpcore/nfp6000_pcie.c      | 24 +++++++------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
index b454db283aef..8fde6c1f681b 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
@@ -1247,19 +1247,16 @@ static void nfp6000_free(struct nfp_cpp *cpp)
 static int nfp6000_read_serial(struct device *dev, u8 *serial)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	int pos;
-	u32 reg;
+	u64 dsn;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DSN);
-	if (!pos) {
+	dsn = pci_get_dsn(pdev);
+	if (!dsn) {
 		dev_err(dev, "can't find PCIe Serial Number Capability\n");
 		return -EINVAL;
 	}
 
-	pci_read_config_dword(pdev, pos + 4, &reg);
-	put_unaligned_be16(reg >> 16, serial + 4);
-	pci_read_config_dword(pdev, pos + 8, &reg);
-	put_unaligned_be32(reg, serial);
+	put_unaligned_be32((u32)(dsn >> 32), serial);
+	put_unaligned_be16((u16)(dsn >> 16), serial + 4);
 
 	return 0;
 }
@@ -1267,18 +1264,15 @@ static int nfp6000_read_serial(struct device *dev, u8 *serial)
 static int nfp6000_get_interface(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	int pos;
-	u32 reg;
+	u64 dsn;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DSN);
-	if (!pos) {
+	dsn = pci_get_dsn(pdev);
+	if (!dsn) {
 		dev_err(dev, "can't find PCIe Serial Number Capability\n");
 		return -EINVAL;
 	}
 
-	pci_read_config_dword(pdev, pos + 4, &reg);
-
-	return reg & 0xffff;
+	return dsn & 0xffff;
 }
 
 static const struct nfp_cpp_operations nfp6000_pcie_ops = {
-- 
2.25.0.368.g28a2d05eebfb

Re: [PATCH v2 6/6] nfp: Use pci_get_dsn()

[Jakub Kicinski]

On Mon,  2 Mar 2020 18:25:05 -0800 Jacob Keller wrote:
> Use the newly added pci_get_dsn() function for obtaining the 64-bit
> Device Serial Number in the nfp6000_read_serial and
> nfp_6000_get_interface functions.
> 
> pci_get_dsn() reports the Device Serial number as a u64 value created by
> combining two pci_read_config_dword functions. The lower 16 bits
> represent the device interface value, and the next 48 bits represent the
> serial value. Use put_unaligned_be32 and put_unaligned_be16 to convert
> the serial value portion into a Big Endian formatted serial u8 array.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Cc: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Thanks!

> -	pci_read_config_dword(pdev, pos + 4, &reg);
> -	put_unaligned_be16(reg >> 16, serial + 4);
> -	pci_read_config_dword(pdev, pos + 8, &reg);
> -	put_unaligned_be32(reg, serial);
> +	put_unaligned_be32((u32)(dsn >> 32), serial);
> +	put_unaligned_be16((u16)(dsn >> 16), serial + 4);

nit: the casts and extra brackets should be unnecessary, in case
     you're respinning..

Re: [PATCH v2 6/6] nfp: Use pci_get_dsn()

[Jacob Keller]

On 3/2/2020 7:40 PM, Jakub Kicinski wrote:
> On Mon,  2 Mar 2020 18:25:05 -0800 Jacob Keller wrote:
>> Use the newly added pci_get_dsn() function for obtaining the 64-bit
>> Device Serial Number in the nfp6000_read_serial and
>> nfp_6000_get_interface functions.
>>
>> pci_get_dsn() reports the Device Serial number as a u64 value created by
>> combining two pci_read_config_dword functions. The lower 16 bits
>> represent the device interface value, and the next 48 bits represent the
>> serial value. Use put_unaligned_be32 and put_unaligned_be16 to convert
>> the serial value portion into a Big Endian formatted serial u8 array.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>
> 
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>
> 
> Thanks!
> 
>> -	pci_read_config_dword(pdev, pos + 4, &reg);
>> -	put_unaligned_be16(reg >> 16, serial + 4);
>> -	pci_read_config_dword(pdev, pos + 8, &reg);
>> -	put_unaligned_be32(reg, serial);
>> +	put_unaligned_be32((u32)(dsn >> 32), serial);
>> +	put_unaligned_be16((u16)(dsn >> 16), serial + 4);
> 
> nit: the casts and extra brackets should be unnecessary, in case
>      you're respinning..
> 

Ah, yea because the argument will get converted properly by the
function. It's a bit of a habit since we use one of the -W warnings that
warns about implicit conversions that use truncation. I can remove them
if I need to respin.

It looks like this got picked up by one of the kbuild bots but got
applied without the main PCI patch that implemented pci_get_dsn.

Thanks,
Jake

Re: [PATCH v2 0/6] PCI: Implement function to read Device Serial Number

[David Miller]

From: Jacob Keller <jacob.e.keller@intel.com>
Date: Mon,  2 Mar 2020 18:24:59 -0800

> Several drivers read the Device Serial Number from the PCIe extended
> configuration space. Each of these drivers implements a similar approach to
> finding the position and then extracting the 8 bytes of data.
> 
> Implement a new helper function, pci_get_dsn, which can be used to extract
> this data into an 8 byte array.
 ...

Bjorn, another series that needs your review and could go via my networking
tree.

Please take a look, thanks.

Re: [PATCH v2 0/6] PCI: Implement function to read Device Serial Number

[David Miller]

From: Jacob Keller <jacob.e.keller@intel.com>
Date: Mon,  2 Mar 2020 18:24:59 -0800

> Several drivers read the Device Serial Number from the PCIe extended
> configuration space. Each of these drivers implements a similar approach to
> finding the position and then extracting the 8 bytes of data.
> 
> Implement a new helper function, pci_get_dsn, which can be used to extract
> this data into an 8 byte array.
 ...

Series applied to net-next, thanks.

[PATCH 2/5] bnxt_en: use pci_get_dsn

[Jacob Keller]

Replace the open-coded implementation for reading the PCIe DSN with
pci_get_dsn.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 597e6fd5bfea..55f078fc067e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11755,20 +11755,14 @@ static int bnxt_init_mac_addr(struct bnxt *bp)
 static int bnxt_pcie_dsn_get(struct bnxt *bp, u8 dsn[])
 {
 	struct pci_dev *pdev = bp->pdev;
-	int pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DSN);
-	u32 dw;
+	int err;
 
-	if (!pos) {
+	err = pci_get_dsn(pdev, dsn);
+	if (err) {
 		netdev_info(bp->dev, "Unable do read adapter's DSN");
-		return -EOPNOTSUPP;
+		return err;
 	}
 
-	/* DSN (two dw) is at an offset of 4 from the cap pos */
-	pos += 4;
-	pci_read_config_dword(pdev, pos, &dw);
-	put_unaligned_le32(dw, &dsn[0]);
-	pci_read_config_dword(pdev, pos + 4, &dw);
-	put_unaligned_le32(dw, &dsn[4]);
 	bp->flags |= BNXT_FLAG_DSN_VALID;
 	return 0;
 }
-- 
2.25.0.368.g28a2d05eebfb

Re: [PATCH 2/5] bnxt_en: use pci_get_dsn

[Bjorn Helgaas]

On Thu, Feb 27, 2020 at 02:36:32PM -0800, Jacob Keller wrote:
> Replace the open-coded implementation for reading the PCIe DSN with
> pci_get_dsn.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Cc: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 597e6fd5bfea..55f078fc067e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -11755,20 +11755,14 @@ static int bnxt_init_mac_addr(struct bnxt *bp)
>  static int bnxt_pcie_dsn_get(struct bnxt *bp, u8 dsn[])
>  {
>  	struct pci_dev *pdev = bp->pdev;
> -	int pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DSN);
> -	u32 dw;
> +	int err;
>  
> -	if (!pos) {
> +	err = pci_get_dsn(pdev, dsn);
> +	if (err) {
>  		netdev_info(bp->dev, "Unable do read adapter's DSN");

"Unable *to* read..."

Not your fault, I guess :)

> -		return -EOPNOTSUPP;
> +		return err;
>  	}
>  
> -	/* DSN (two dw) is at an offset of 4 from the cap pos */
> -	pos += 4;
> -	pci_read_config_dword(pdev, pos, &dw);
> -	put_unaligned_le32(dw, &dsn[0]);
> -	pci_read_config_dword(pdev, pos + 4, &dw);
> -	put_unaligned_le32(dw, &dsn[4]);
>  	bp->flags |= BNXT_FLAG_DSN_VALID;
>  	return 0;
>  }
> -- 
> 2.25.0.368.g28a2d05eebfb
> 

[PATCH 3/5] scsi: qedf: use pci_get_dsn

[Jacob Keller]

Replace the open-coded implementation for reading the PCIe DSN with
pci_get_dsn.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: QLogic-Storage-Upstream@cavium.com
---
 drivers/scsi/qedf/qedf_main.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index 604856e72cfb..6ef688ef465c 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -1578,7 +1578,7 @@ static void qedf_setup_fdmi(struct qedf_ctx *qedf)
 	struct fc_lport *lport = qedf->lport;
 	struct fc_host_attrs *fc_host = shost_to_fc_host(lport->host);
 	u8 buf[8];
-	int i, pos;
+	int i, err;
 
 	/*
 	 * fdmi_enabled needs to be set for libfc to execute FDMI registration.
@@ -1591,20 +1591,16 @@ static void qedf_setup_fdmi(struct qedf_ctx *qedf)
 	 */
 
 	/* Get the PCI-e Device Serial Number Capability */
-	pos = pci_find_ext_capability(qedf->pdev, PCI_EXT_CAP_ID_DSN);
-	if (pos) {
-		pos += 4;
-		for (i = 0; i < 8; i++)
-			pci_read_config_byte(qedf->pdev, pos + i, &buf[i]);
-
+	err = pci_get_dsn(qedf->pdev, buf);
+	if (err)
+		snprintf(fc_host->serial_number,
+		    sizeof(fc_host->serial_number), "Unknown");
+	else
 		snprintf(fc_host->serial_number,
 		    sizeof(fc_host->serial_number),
 		    "%02X%02X%02X%02X%02X%02X%02X%02X",
 		    buf[7], buf[6], buf[5], buf[4],
 		    buf[3], buf[2], buf[1], buf[0]);
-	} else
-		snprintf(fc_host->serial_number,
-		    sizeof(fc_host->serial_number), "Unknown");
 
 	snprintf(fc_host->manufacturer,
 	    sizeof(fc_host->manufacturer), "%s", "Cavium Inc.");
-- 
2.25.0.368.g28a2d05eebfb

[PATCH 4/5] ice: use pci_get_dsn

[Jacob Keller]

Replace the open-coded implementation for reading the PCIe DSN with
pci_get_dsn.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 32 ++++++++++-------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 5ae671609f98..77b56dcae331 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3099,30 +3099,26 @@ static char *ice_get_opt_fw_name(struct ice_pf *pf)
 	 * followed by a EUI-64 identifier (PCIe Device Serial Number)
 	 */
 	struct pci_dev *pdev = pf->pdev;
-	char *opt_fw_filename = NULL;
-	u32 dword;
+	char *opt_fw_filename;
 	u8 dsn[8];
-	int pos;
+	int err;
 
 	/* Determine the name of the optional file using the DSN (two
 	 * dwords following the start of the DSN Capability).
 	 */
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DSN);
-	if (pos) {
-		opt_fw_filename = kzalloc(NAME_MAX, GFP_KERNEL);
-		if (!opt_fw_filename)
-			return NULL;
+	err = pci_get_dsn(pdev, dsn);
+	if (err)
+		return NULL;
 
-		pci_read_config_dword(pdev, pos + 4, &dword);
-		put_unaligned_le32(dword, &dsn[0]);
-		pci_read_config_dword(pdev, pos + 8, &dword);
-		put_unaligned_le32(dword, &dsn[4]);
-		snprintf(opt_fw_filename, NAME_MAX,
-			 "%sice-%02x%02x%02x%02x%02x%02x%02x%02x.pkg",
-			 ICE_DDP_PKG_PATH,
-			 dsn[7], dsn[6], dsn[5], dsn[4],
-			 dsn[3], dsn[2], dsn[1], dsn[0]);
-	}
+	opt_fw_filename = kzalloc(NAME_MAX, GFP_KERNEL);
+	if (!opt_fw_filename)
+		return NULL;
+
+	snprintf(opt_fw_filename, NAME_MAX,
+		 "%sice-%02x%02x%02x%02x%02x%02x%02x%02x.pkg",
+		 ICE_DDP_PKG_PATH,
+		 dsn[7], dsn[6], dsn[5], dsn[4],
+		 dsn[3], dsn[2], dsn[1], dsn[0]);
 
 	return opt_fw_filename;
 }
-- 
2.25.0.368.g28a2d05eebfb

[PATCH 5/5] ixgbe: use pci_get_dsn

[Jacob Keller]

Replace the open-coded implementation for reading the PCIe DSN with
pci_get_dsn.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
index ccd852ad62a4..74ee12b87fc3 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
@@ -968,7 +968,7 @@ int ixgbe_fcoe_get_hbainfo(struct net_device *netdev,
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	struct ixgbe_hw *hw = &adapter->hw;
-	int i, pos;
+	int i, err;
 	u8 buf[8];
 
 	if (!info)
@@ -985,19 +985,15 @@ int ixgbe_fcoe_get_hbainfo(struct net_device *netdev,
 	/* Serial Number */
 
 	/* Get the PCI-e Device Serial Number Capability */
-	pos = pci_find_ext_capability(adapter->pdev, PCI_EXT_CAP_ID_DSN);
-	if (pos) {
-		pos += 4;
-		for (i = 0; i < 8; i++)
-			pci_read_config_byte(adapter->pdev, pos + i, &buf[i]);
-
+	err = pci_get_dsn(adapter->pdev, buf);
+	if (err)
+		snprintf(info->serial_number, sizeof(info->serial_number),
+			 "Unknown");
+	else
 		snprintf(info->serial_number, sizeof(info->serial_number),
 			 "%02X%02X%02X%02X%02X%02X%02X%02X",
 			 buf[7], buf[6], buf[5], buf[4],
 			 buf[3], buf[2], buf[1], buf[0]);
-	} else
-		snprintf(info->serial_number, sizeof(info->serial_number),
-			 "Unknown");
 
 	/* Hardware Version */
 	snprintf(info->hardware_version,
-- 
2.25.0.368.g28a2d05eebfb