[PATCH 0/2] Add device-specific VPD timeout

[PATCH 0/2] Add device-specific VPD timeout

[Matthew R. Ochs]

This small patch series introduces support for a driver to increase the
VPD wait timeout for devices with slower interfaces.

The series is based on v4.9-rc6 and is intended for v4.10.

Matthew R. Ochs (2):
  PCI: Add pci_set_vpd_timeout() to set VPD access timeout
  cxl: Set VPD timeout to avoid access failures

 drivers/misc/cxl/pci.c |  7 +++++++
 drivers/pci/access.c   | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci.h      |  2 ++
 include/linux/pci.h    |  1 +
 4 files changed, 54 insertions(+), 1 deletion(-)

-- 
2.1.0

[PATCH 1/2] PCI: Add pci_set_vpd_timeout() to set VPD access timeout

[Matthew R. Ochs]

The PCI core uses a fixed 50ms timeout when waiting for VPD accesses to
complete. When an access does not complete within this period, a warning
is logged and an error returned to the caller.

While this default timeout is valid for most hardware, some devices can
experience longer access delays under certain circumstances. For example,
one of the IBM CXL Flash devices can take up to ~120ms in a worst-case
scenario. These types of devices can benefit from an extended timeout that
is specific to their hardware constraints.

To support per-device VPD access timeouts, pci_set_vpd_timeout() is added
as an exported service. PCI devices will continue to default with the 50ms
timeout and use a per-device timeout when a driver calls this new service.

Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 drivers/pci/access.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci.h    |  2 ++
 include/linux/pci.h  |  1 +
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d11cdbb..2734a3a 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -288,6 +288,21 @@ int pci_set_vpd_size(struct pci_dev *dev, size_t len)
 }
 EXPORT_SYMBOL(pci_set_vpd_size);
 
+/**
+ * pci_set_vpd_timeout - Set wait timeout for Vital Product Data accesses
+ * @dev:	pci device struct
+ * @timeout:	jiffies to wait for completion
+ */
+int pci_set_vpd_timeout(struct pci_dev *dev, unsigned long timeout)
+{
+	if (!dev->vpd || !dev->vpd->ops)
+		return -ENODEV;
+	return dev->vpd->ops->set_timeout(dev, timeout);
+}
+EXPORT_SYMBOL(pci_set_vpd_timeout);
+
+#define PCI_VPD_DEF_TIMEOUT msecs_to_jiffies(50)
+#define PCI_VPD_MAX_TIMEOUT msecs_to_jiffies(250)
 #define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1)
 
 /**
@@ -355,7 +370,7 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 static int pci_vpd_wait(struct pci_dev *dev)
 {
 	struct pci_vpd *vpd = dev->vpd;
-	unsigned long timeout = jiffies + msecs_to_jiffies(50);
+	unsigned long timeout = jiffies + vpd->timeout;
 	unsigned long max_sleep = 16;
 	u16 status;
 	int ret;
@@ -524,10 +539,22 @@ static int pci_vpd_set_size(struct pci_dev *dev, size_t len)
 	return 0;
 }
 
+static int pci_vpd_set_timeout(struct pci_dev *dev, unsigned long timeout)
+{
+	struct pci_vpd *vpd = dev->vpd;
+
+	if (timeout < PCI_VPD_DEF_TIMEOUT || timeout > PCI_VPD_MAX_TIMEOUT)
+		return -EINVAL;
+
+	vpd->timeout = timeout;
+	return 0;
+}
+
 static const struct pci_vpd_ops pci_vpd_ops = {
 	.read = pci_vpd_read,
 	.write = pci_vpd_write,
 	.set_size = pci_vpd_set_size,
+	.set_timeout = pci_vpd_set_timeout,
 };
 
 static ssize_t pci_vpd_f0_read(struct pci_dev *dev, loff_t pos, size_t count,
@@ -574,10 +601,25 @@ static int pci_vpd_f0_set_size(struct pci_dev *dev, size_t len)
 	return ret;
 }
 
+static int pci_vpd_f0_set_timeout(struct pci_dev *dev, unsigned long timeout)
+{
+	struct pci_dev *tdev = pci_get_slot(dev->bus,
+					    PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
+	int ret;
+
+	if (!tdev)
+		return -ENODEV;
+
+	ret = pci_set_vpd_timeout(tdev, timeout);
+	pci_dev_put(tdev);
+	return ret;
+}
+
 static const struct pci_vpd_ops pci_vpd_f0_ops = {
 	.read = pci_vpd_f0_read,
 	.write = pci_vpd_f0_write,
 	.set_size = pci_vpd_f0_set_size,
+	.set_timeout = pci_vpd_f0_set_timeout,
 };
 
 int pci_vpd_init(struct pci_dev *dev)
@@ -594,6 +636,7 @@ int pci_vpd_init(struct pci_dev *dev)
 		return -ENOMEM;
 
 	vpd->len = PCI_VPD_MAX_SIZE;
+	vpd->timeout = PCI_VPD_DEF_TIMEOUT;
 	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
 		vpd->ops = &pci_vpd_f0_ops;
 	else
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4518562..99f089a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -112,12 +112,14 @@ struct pci_vpd_ops {
 	ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
 	ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
 	int (*set_size)(struct pci_dev *dev, size_t len);
+	int (*set_timeout)(struct pci_dev *dev, unsigned long timeout);
 };
 
 struct pci_vpd {
 	const struct pci_vpd_ops *ops;
 	struct bin_attribute *attr; /* descriptor for sysfs VPD entry */
 	struct mutex	lock;
+	unsigned long	timeout; /* wait timeout in jiffies */
 	unsigned int	len;
 	u16		flag;
 	u8		cap;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0e49f70..3038246 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1123,6 +1123,7 @@ void pci_unlock_rescan_remove(void);
 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_set_vpd_size(struct pci_dev *dev, size_t len);
+int pci_set_vpd_timeout(struct pci_dev *dev, unsigned long timeout);
 
 /* 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);
-- 
2.1.0

Re: [PATCH 1/2] PCI: Add pci_set_vpd_timeout() to set VPD access timeout

[Bjorn Helgaas]

Hi Matthew,

On Mon, Nov 21, 2016 at 03:09:49PM -0600, Matthew R. Ochs wrote:
> The PCI core uses a fixed 50ms timeout when waiting for VPD accesses to
> complete. When an access does not complete within this period, a warning
> is logged and an error returned to the caller.
> 
> While this default timeout is valid for most hardware, some devices can
> experience longer access delays under certain circumstances. For example,
> one of the IBM CXL Flash devices can take up to ~120ms in a worst-case
> scenario. These types of devices can benefit from an extended timeout that
> is specific to their hardware constraints.
> 
> To support per-device VPD access timeouts, pci_set_vpd_timeout() is added
> as an exported service. PCI devices will continue to default with the 50ms
> timeout and use a per-device timeout when a driver calls this new service.

Can you include a pointer to something in the spec that's behind the
default 50ms timeout, or did somebody just pull that number out of the
air?

I'm wondering how we know 50ms or 120ms or 250ms or whatever is the
right number.  What bad things would happen if we just increased the
timeout from 50 to 125ms for *all* devices?

I don't really want to end up with a bunch of device-specific quirks
here.  If we have a quirk to work around one defective device, that's
one thing.  If the spec allows a huge variation in VPD access time,
that might be something we want to handle 

Bjorn

Re: [PATCH 1/2] PCI: Add pci_set_vpd_timeout() to set VPD access timeout

[Andrew Donnellan]

On 22/11/16 09:05, Bjorn Helgaas wrote:
> Hi Matthew,
>
> On Mon, Nov 21, 2016 at 03:09:49PM -0600, Matthew R. Ochs wrote:
>> The PCI core uses a fixed 50ms timeout when waiting for VPD accesses to
>> complete. When an access does not complete within this period, a warning
>> is logged and an error returned to the caller.
>>
>> While this default timeout is valid for most hardware, some devices can
>> experience longer access delays under certain circumstances. For example,
>> one of the IBM CXL Flash devices can take up to ~120ms in a worst-case
>> scenario. These types of devices can benefit from an extended timeout that
>> is specific to their hardware constraints.
>>
>> To support per-device VPD access timeouts, pci_set_vpd_timeout() is added
>> as an exported service. PCI devices will continue to default with the 50ms
>> timeout and use a per-device timeout when a driver calls this new service.
>
> Can you include a pointer to something in the spec that's behind the
> default 50ms timeout, or did somebody just pull that number out of the
> air?

It looks like Stephen Hemminger added the 50ms timeout in 1120f8b8169f, 
which seems to indicate that 50ms was chosen because it's longer than 
the 13ms per word that was measured on one device.

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

RE: [PATCH 1/2] PCI: Add pci_set_vpd_timeout() to set VPD access timeout

[Stephen Hemminger]

I had old Marvell Sky2 hardware that had slow flash.

-----Original Message-----
From: Andrew Donnellan [mailto:andrew.donnellan@au1.ibm.com]=20
Sent: Monday, November 21, 2016 4:16 PM
To: Bjorn Helgaas <helgaas@kernel.org>; Matthew R. Ochs <mrochs@linux.vnet.=
ibm.com>
Cc: linux-pci@vger.kernel.org; Frederic Barrat <fbarrat@linux.vnet.ibm.com>=
; Uma Krishnan <ukrishn@linux.vnet.ibm.com>; Ian Munsie <imunsie@au1.ibm.co=
m>; Bjorn Helgaas <bhelgaas@google.com>; linuxppc-dev@lists.ozlabs.org; Ste=
phen Hemminger <sthemmin@microsoft.com>
Subject: Re: [PATCH 1/2] PCI: Add pci_set_vpd_timeout() to set VPD access t=
imeout

On 22/11/16 09:05, Bjorn Helgaas wrote:
> Hi Matthew,
>
> On Mon, Nov 21, 2016 at 03:09:49PM -0600, Matthew R. Ochs wrote:
>> The PCI core uses a fixed 50ms timeout when waiting for VPD accesses=20
>> to complete. When an access does not complete within this period, a=20
>> warning is logged and an error returned to the caller.
>>
>> While this default timeout is valid for most hardware, some devices=20
>> can experience longer access delays under certain circumstances. For=20
>> example, one of the IBM CXL Flash devices can take up to ~120ms in a=20
>> worst-case scenario. These types of devices can benefit from an=20
>> extended timeout that is specific to their hardware constraints.
>>
>> To support per-device VPD access timeouts, pci_set_vpd_timeout() is=20
>> added as an exported service. PCI devices will continue to default=20
>> with the 50ms timeout and use a per-device timeout when a driver calls t=
his new service.
>
> Can you include a pointer to something in the spec that's behind the=20
> default 50ms timeout, or did somebody just pull that number out of the=20
> air?

It looks like Stephen Hemminger added the 50ms timeout in 1120f8b8169f, whi=
ch seems to indicate that 50ms was chosen because it's longer than the 13ms=
 per word that was measured on one device.

--=20
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH 1/2] PCI: Add pci_set_vpd_timeout() to set VPD access timeout

[Matthew R. Ochs]

Hi Bjorn,

> On Nov 21, 2016, at 4:05 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>=20
> Hi Matthew,
>=20
> On Mon, Nov 21, 2016 at 03:09:49PM -0600, Matthew R. Ochs wrote:
>> The PCI core uses a fixed 50ms timeout when waiting for VPD accesses =
to
>> complete. When an access does not complete within this period, a =
warning
>> is logged and an error returned to the caller.
>>=20
>> While this default timeout is valid for most hardware, some devices =
can
>> experience longer access delays under certain circumstances. For =
example,
>> one of the IBM CXL Flash devices can take up to ~120ms in a =
worst-case
>> scenario. These types of devices can benefit from an extended timeout =
that
>> is specific to their hardware constraints.
>>=20
>> To support per-device VPD access timeouts, pci_set_vpd_timeout() is =
added
>> as an exported service. PCI devices will continue to default with the =
50ms
>> timeout and use a per-device timeout when a driver calls this new =
service.
>=20
> Can you include a pointer to something in the spec that's behind the
> default 50ms timeout, or did somebody just pull that number out of the
> air?

AFAIK the PCI spec is silent on VPD access timeouts.

The current 50ms timeout can be traced to Commit 1120f8b8169f ("PCI: =
handle
long delays in VPD access") where the timeout was increased to =
accommodate
specific hardware. Prior to that the wait timeout was dependent upon a =
read or
write with the write waiting up to a maximum of 10ms.

> I'm wondering how we know 50ms or 120ms or 250ms or whatever is the
> right number.  What bad things would happen if we just increased the
> timeout from 50 to 125ms for *all* devices?

You're asking the right questions. The timeout chosen for the CXL flash =
device
was derived through instrumentation, and this was only after witnessing =
VPD
timeout messages in the kernel log at random times.

I originally thought about proposing a blanket increase, but figured the =
scope
might be too broad. There are 2 downsides I see with simply replacing =
50ms
to a larger value:

 - Raising the timeout bar [potentially] raises the total time it takes =
to complete
a VPD access. One would hope that scenarios where every access times out
are very rare and that the max limits are only rarely encountered.

 - It's difficult to settle on a single 'catch all' value. What might be =
fine for h/w
A may end up not working for h/w B (as is the case here). That said, =
given that
50ms has served as the value for roughly nine years I think this point =
doesn't
carry much weight.

>=20
> I don't really want to end up with a bunch of device-specific quirks
> here.  If we have a quirk to work around one defective device, that's
> one thing.  If the spec allows a huge variation in VPD access time,
> that might be something we want to handle=20

I agree 100% and would be more than happy with submitting a patch that
simply increases the value.

-matt

[PATCH 2/2] cxl: Set VPD timeout to avoid access failures

[Matthew R. Ochs]

Some IBM CXL devices can take up to ~120ms to complete a VPD access
transaction when under heavy load. With an existing default VPD timeout
of 50ms, reads/writes can fail despite there not being an issue with the
underlying hardware.

To avoid these false failures, increase the VPD wait timeout for certain
CXL devices to a value that allows appropriate VPD access delay when faced
with a worst-case scenario.

Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 drivers/misc/cxl/pci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index e96be9c..56a67cf 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -1675,6 +1675,10 @@ bool cxl_slot_is_supported(struct pci_dev *dev, int flags)
 }
 EXPORT_SYMBOL_GPL(cxl_slot_is_supported);
 
+static bool cxl_pci_is_slow_vpd_device(struct pci_dev *dev)
+{
+	return dev->device == 0x0601;
+}
 
 static int cxl_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
@@ -1692,6 +1696,9 @@ static int cxl_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return -ENODEV;
 	}
 
+	if (cxl_pci_is_slow_vpd_device(dev))
+		pci_set_vpd_timeout(dev, msecs_to_jiffies(125));
+
 	if (cxl_verbose)
 		dump_cxl_config_space(dev);
 
-- 
2.1.0

Re: [PATCH 2/2] cxl: Set VPD timeout to avoid access failures

[Andrew Donnellan]

On 22/11/16 08:10, Matthew R. Ochs wrote:
> Some IBM CXL devices can take up to ~120ms to complete a VPD access

Which devices?

> transaction when under heavy load. With an existing default VPD timeout
> of 50ms, reads/writes can fail despite there not being an issue with the
> underlying hardware.
>
> To avoid these false failures, increase the VPD wait timeout for certain
> CXL devices to a value that allows appropriate VPD access delay when faced
> with a worst-case scenario.
>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>

Contingent on Bjorn applying patch 1, and with comment below:

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

> ---
>  drivers/misc/cxl/pci.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index e96be9c..56a67cf 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -1675,6 +1675,10 @@ bool cxl_slot_is_supported(struct pci_dev *dev, int flags)
>  }
>  EXPORT_SYMBOL_GPL(cxl_slot_is_supported);
>
> +static bool cxl_pci_is_slow_vpd_device(struct pci_dev *dev)
> +{
> +	return dev->device == 0x0601;

Comment with device name.

> +}
>
>  static int cxl_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
> @@ -1692,6 +1696,9 @@ static int cxl_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		return -ENODEV;
>  	}
>
> +	if (cxl_pci_is_slow_vpd_device(dev))
> +		pci_set_vpd_timeout(dev, msecs_to_jiffies(125));
> +
>  	if (cxl_verbose)
>  		dump_cxl_config_space(dev);
>
>

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH 2/2] cxl: Set VPD timeout to avoid access failures

[Matthew R. Ochs]

> On Nov 21, 2016, at 6:32 PM, Andrew Donnellan =
<andrew.donnellan@au1.ibm.com> wrote:
> On 22/11/16 08:10, Matthew R. Ochs wrote:
>> Some IBM CXL devices can take up to ~120ms to complete a VPD access
>=20
> Which devices?

Flash GT

Re: [PATCH 2/2] cxl: Set VPD timeout to avoid access failures

[Frederic Barrat]

Le 21/11/2016 à 22:10, Matthew R. Ochs a écrit :
> Some IBM CXL devices can take up to ~120ms to complete a VPD access
> transaction when under heavy load. With an existing default VPD timeout
> of 50ms, reads/writes can fail despite there not being an issue with the
> underlying hardware.
>
> To avoid these false failures, increase the VPD wait timeout for certain
> CXL devices to a value that allows appropriate VPD access delay when faced
> with a worst-case scenario.
>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
> ---

If the new pci API gets in, then I'm ok with the cxl change, at least 
for now. For longer term, we should consider opening up the cxl kernel 
API to give access to the new pci API so that it can be tuned by the AFU 
driver directly, since it would know better than cxl what the proper vpd 
access timeout should be.

Acked-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com>

   Fred