| Message ID | 1479762589-51557-1-git-send-email-mrochs@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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.
Hi Bjorn, > On Nov 21, 2016, at 4:05 PM, Bjorn Helgaas <helgaas@kernel.org> 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? 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. > > 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 I agree 100% and would be more than happy with submitting a patch that simply increases the value. -matt
I had old Marvell Sky2 hardware that had slow flash. -----Original Message----- From: Andrew Donnellan [mailto:andrew.donnellan@au1.ibm.com] 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.com>; Bjorn Helgaas <bhelgaas@google.com>; linuxppc-dev@lists.ozlabs.org; Stephen Hemminger <sthemmin@microsoft.com> Subject: Re: [PATCH 1/2] PCI: Add pci_set_vpd_timeout() to set VPD access timeout 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.
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);
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(-)