diff mbox series

[1/5] pci: introduce pci_get_dsn

Message ID 20200227223635.1021197-3-jacob.e.keller@intel.com
State New
Headers show
Series pci: implement function to read Device Serial Number | expand

Commit Message

Jacob Keller Feb. 27, 2020, 10:36 p.m. UTC
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(+)

Comments

David Miller March 1, 2020, 5:27 a.m. UTC | #1
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.
Jacob Keller March 2, 2020, 7:58 p.m. UTC | #2
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
Bjorn Helgaas March 2, 2020, 10:25 p.m. UTC | #3
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
>
Jacob Keller March 2, 2020, 10:33 p.m. UTC | #4
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
>>
Bjorn Helgaas March 2, 2020, 11:20 p.m. UTC | #5
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
> >>
Jacob Keller March 2, 2020, 11:24 p.m. UTC | #6
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
Bjorn Helgaas March 2, 2020, 11:39 p.m. UTC | #7
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 mbox series

Patch

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) { }