LinuxLists.cc - [PATCH v10 0/8] Expose and manage PCI device reset

[permalink] [raw]

Subject: [PATCH v10 3/8] PCI: Remove reset_fn field from pci_dev

reset_fn field is used to indicate whether the device supports any reset
mechanism or not. Remove the use of reset_fn in favor of new reset_methods
array which can be used to keep track of all supported reset mechanisms of
a device and their ordering.

The octeon driver is incorrectly using reset_fn field to detect if the
device supports FLR or not. Use pcie_reset_flr() to probe whether it
supports FLR or not.

Reviewed-by: Alex Williamson <[email protected]>
Reviewed-by: Raphael Norwitz <[email protected]>
Co-developed-by: Alex Williamson <[email protected]>
Signed-off-by: Alex Williamson <[email protected]>
Signed-off-by: Amey Narkhede <[email protected]>
---
drivers/net/ethernet/cavium/liquidio/lio_vf_main.c | 2 +-
drivers/pci/pci-sysfs.c | 2 +-
drivers/pci/pci.c | 6 +++---
drivers/pci/probe.c | 1 -
drivers/pci/quirks.c | 2 +-
drivers/pci/remove.c | 1 -
include/linux/pci.h | 1 -
7 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
index 516f166ce..336d149ee 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
@@ -526,7 +526,7 @@ static void octeon_destroy_resources(struct octeon_device *oct)
oct->irq_name_storage = NULL;
}
/* Soft reset the octeon device before exiting */
- if (oct->pci_dev->reset_fn)
+ if (!pcie_reset_flr(oct->pci_dev, 1))
octeon_pci_flr(oct);
else
cn23xx_vf_ask_pf_to_do_flr(oct);
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index beb8d1f4f..316f70c3e 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1367,7 +1367,7 @@ static umode_t pci_dev_reset_attr_is_visible(struct kobject *kobj,
{
struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));

- if (!pdev->reset_fn)
+ if (!pci_reset_supported(pdev))
return 0;

return a->mode;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 42440cb10..d5c16492c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5221,7 +5221,7 @@ int pci_reset_function(struct pci_dev *dev)
{
int rc;

- if (!dev->reset_fn)
+ if (!pci_reset_supported(dev))
return -ENOTTY;

pci_dev_lock(dev);
@@ -5257,7 +5257,7 @@ int pci_reset_function_locked(struct pci_dev *dev)
{
int rc;

- if (!dev->reset_fn)
+ if (!pci_reset_supported(dev))
return -ENOTTY;

pci_dev_save_and_disable(dev);
@@ -5280,7 +5280,7 @@ int pci_try_reset_function(struct pci_dev *dev)
{
int rc;

- if (!dev->reset_fn)
+ if (!pci_reset_supported(dev))
return -ENOTTY;

if (!pci_dev_trylock(dev))
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index bc4af914a..c272e23db 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2407,7 +2407,6 @@ static void pci_init_capabilities(struct pci_dev *dev)

pcie_report_downtraining(dev);
pci_init_reset_methods(dev);
- dev->reset_fn = pci_reset_supported(dev);
}

/*
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f977ba79a..e86cf4a3b 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5589,7 +5589,7 @@ static void quirk_reset_lenovo_thinkpad_p50_nvgpu(struct pci_dev *pdev)

if (pdev->subsystem_vendor != PCI_VENDOR_ID_LENOVO ||
pdev->subsystem_device != 0x222e ||
- !pdev->reset_fn)
+ !pci_reset_supported(pdev))
return;

if (pci_enable_device_mem(pdev))
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index dd12c2fcc..4c54c7505 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -19,7 +19,6 @@ static void pci_stop_dev(struct pci_dev *dev)
pci_pme_active(dev, false);

if (pci_dev_is_added(dev)) {
- dev->reset_fn = 0;

device_release_driver(&dev->dev);
pci_proc_detach_device(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9f3e85f33..f34563831 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -431,7 +431,6 @@ struct pci_dev {
unsigned int state_saved:1;
unsigned int is_physfn:1;
unsigned int is_virtfn:1;
- unsigned int reset_fn:1;
unsigned int is_hotplug_bridge:1;
unsigned int shpc_managed:1; /* SHPC owned by shpchp */
unsigned int is_thunderbolt:1; /* Thunderbolt controller */
--
2.32.0

2021-07-09 12:40:37

[permalink] [raw]

Subject: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
FLR to avoid reading PCI_EXP_DEVCAP multiple times.

Currently there is separate function pcie_has_flr() to probe if PCIe FLR
is supported by the device which does not match the calling convention
followed by reset methods which use second function argument to decide
whether to probe or not. Add new function pcie_reset_flr() that follows
the calling convention of reset methods.

Signed-off-by: Amey Narkhede <[email protected]>
---
drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
drivers/pci/pci.c | 59 +++++++++++-----------
drivers/pci/pcie/aer.c | 12 ++---
drivers/pci/probe.c | 6 ++-
drivers/pci/quirks.c | 9 ++--
include/linux/pci.h | 3 +-
6 files changed, 45 insertions(+), 48 deletions(-)

diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
index facc8e6bc..15d6c8452 100644
--- a/drivers/crypto/cavium/nitrox/nitrox_main.c
+++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
@@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
return -ENOMEM;
}

- /* check flr support */
- if (pcie_has_flr(pdev))
- pcie_flr(pdev);
+ pcie_reset_flr(pdev, 0);

pci_restore_state(pdev);

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 452351025..fefa6d7b3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4611,32 +4611,12 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
}
EXPORT_SYMBOL(pci_wait_for_pending_transaction);

-/**
- * pcie_has_flr - check if a device supports function level resets
- * @dev: device to check
- *
- * Returns true if the device advertises support for PCIe function level
- * resets.
- */
-bool pcie_has_flr(struct pci_dev *dev)
-{
- u32 cap;
-
- if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
- return false;
-
- pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
- return cap & PCI_EXP_DEVCAP_FLR;
-}
-EXPORT_SYMBOL_GPL(pcie_has_flr);
-
/**
* pcie_flr - initiate a PCIe function level reset
* @dev: device to reset
*
- * Initiate a function level reset on @dev. The caller should ensure the
- * device supports FLR before calling this function, e.g. by using the
- * pcie_has_flr() helper.
+ * Initiate a function level reset unconditionally on @dev without
+ * checking any flags and DEVCAP
*/
int pcie_flr(struct pci_dev *dev)
{
@@ -4659,6 +4639,28 @@ int pcie_flr(struct pci_dev *dev)
}
EXPORT_SYMBOL_GPL(pcie_flr);

+/**
+ * pcie_reset_flr - initiate a PCIe function level reset
+ * @dev: device to reset
+ * @probe: If set, only check if the device can be reset this way.
+ *
+ * Initiate a function level reset on @dev.
+ */
+int pcie_reset_flr(struct pci_dev *dev, int probe)
+{
+ if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
+ return -ENOTTY;
+
+ if (!dev->has_pcie_flr)
+ return -ENOTTY;
+
+ if (probe)
+ return 0;
+
+ return pcie_flr(dev);
+}
+EXPORT_SYMBOL_GPL(pcie_reset_flr);
+
static int pci_af_flr(struct pci_dev *dev, int probe)
{
int pos;
@@ -5139,11 +5141,9 @@ int __pci_reset_function_locked(struct pci_dev *dev)
rc = pci_dev_specific_reset(dev, 0);
if (rc != -ENOTTY)
return rc;
- if (pcie_has_flr(dev)) {
- rc = pcie_flr(dev);
- if (rc != -ENOTTY)
- return rc;
- }
+ rc = pcie_reset_flr(dev, 0);
+ if (rc != -ENOTTY)
+ return rc;
rc = pci_af_flr(dev, 0);
if (rc != -ENOTTY)
return rc;
@@ -5174,8 +5174,9 @@ int pci_probe_reset_function(struct pci_dev *dev)
rc = pci_dev_specific_reset(dev, 1);
if (rc != -ENOTTY)
return rc;
- if (pcie_has_flr(dev))
- return 0;
+ rc = pcie_reset_flr(dev, 1);
+ if (rc != -ENOTTY)
+ return rc;
rc = pci_af_flr(dev, 1);
if (rc != -ENOTTY)
return rc;
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index ec943cee5..98077595a 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1405,13 +1405,11 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
}

if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
- if (pcie_has_flr(dev)) {
- rc = pcie_flr(dev);
- pci_info(dev, "has been reset (%d)\n", rc);
- } else {
- pci_info(dev, "not reset (no FLR support)\n");
- rc = -ENOTTY;
- }
+ rc = pcie_reset_flr(dev, 0);
+ if (!rc)
+ pci_info(dev, "has been reset\n");
+ else
+ pci_info(dev, "not reset (no FLR support: %d)\n", rc);
} else {
rc = pci_bus_error_reset(dev);
pci_info(dev, "%s Port link has been reset (%d)\n",
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3a62d09b8..072a3d4dc 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1487,6 +1487,7 @@ void set_pcie_port_type(struct pci_dev *pdev)
{
int pos;
u16 reg16;
+ u32 reg32;
int type;
struct pci_dev *parent;

@@ -1497,8 +1498,9 @@ void set_pcie_port_type(struct pci_dev *pdev)
pdev->pcie_cap = pos;
pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
pdev->pcie_flags_reg = reg16;
- pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
- pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
+ pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &reg32);
+ pdev->pcie_mpss = reg32 & PCI_EXP_DEVCAP_PAYLOAD;
+ pdev->has_pcie_flr = !!(reg32 & PCI_EXP_DEVCAP_FLR);

parent = pci_upstream_bridge(pdev);
if (!parent)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index d85914afe..f977ba79a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3819,7 +3819,7 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe)
u32 cfg;

if (dev->class != PCI_CLASS_STORAGE_EXPRESS ||
- !pcie_has_flr(dev) || !pci_resource_start(dev, 0))
+ pcie_reset_flr(dev, 1) || !pci_resource_start(dev, 0))
return -ENOTTY;

if (probe)
@@ -3888,13 +3888,10 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe)
*/
static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
{
- if (!pcie_has_flr(dev))
- return -ENOTTY;
+ int ret = pcie_reset_flr(dev, probe);

if (probe)
- return 0;
-
- pcie_flr(dev);
+ return ret;

msleep(250);

diff --git a/include/linux/pci.h b/include/linux/pci.h
index c20211e59..d432428fd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -337,6 +337,7 @@ struct pci_dev {
u8 msi_cap; /* MSI capability offset */
u8 msix_cap; /* MSI-X capability offset */
u8 pcie_mpss:3; /* PCIe Max Payload Size Supported */
+ u8 has_pcie_flr:1; /* PCIe FLR supported */
u8 rom_base_reg; /* Config register controlling ROM */
u8 pin; /* Interrupt pin this device uses */
u16 pcie_flags_reg; /* Cached PCIe Capabilities Register */
@@ -1225,7 +1226,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
enum pci_bus_speed *speed,
enum pcie_link_width *width);
void pcie_print_link_status(struct pci_dev *dev);
-bool pcie_has_flr(struct pci_dev *dev);
+int pcie_reset_flr(struct pci_dev *dev, int probe);
int pcie_flr(struct pci_dev *dev);
int __pci_reset_function_locked(struct pci_dev *dev);
int pci_reset_function(struct pci_dev *dev);
--
2.32.0

2021-07-09 12:40:54

[permalink] [raw]

Subject: [PATCH v10 2/8] PCI: Add new array for keeping track of ordering of reset methods

Introduce a new array reset_methods in struct pci_dev to keep track of
reset mechanisms supported by the device and their ordering.

Also refactor probing and reset functions to take advantage of calling
convention of reset functions.

Co-developed-by: Alex Williamson <[email protected]>
Signed-off-by: Alex Williamson <[email protected]>
Signed-off-by: Amey Narkhede <[email protected]>
---
drivers/pci/pci.c | 92 ++++++++++++++++++++++++++-------------------
drivers/pci/pci.h | 9 ++++-
drivers/pci/probe.c | 5 +--
include/linux/pci.h | 7 ++++
4 files changed, 70 insertions(+), 43 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index fefa6d7b3..42440cb10 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -72,6 +72,14 @@ static void pci_dev_d3_sleep(struct pci_dev *dev)
msleep(delay);
}

+int pci_reset_supported(struct pci_dev *dev)
+{
+ u8 null_reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
+
+ return memcmp(null_reset_methods,
+ dev->reset_methods, sizeof(null_reset_methods));
+}
+
#ifdef CONFIG_PCI_DOMAINS
int pci_domains_supported = 1;
#endif
@@ -5104,6 +5112,15 @@ static void pci_dev_restore(struct pci_dev *dev)
err_handler->reset_done(dev);
}

+const struct pci_reset_fn_method pci_reset_fn_methods[] = {
+ { },
+ { &pci_dev_specific_reset, .name = "device_specific" },
+ { &pcie_reset_flr, .name = "flr" },
+ { &pci_af_flr, .name = "af_flr" },
+ { &pci_pm_reset, .name = "pm" },
+ { &pci_reset_bus_function, .name = "bus" },
+};
+
/**
* __pci_reset_function_locked - reset a PCI device function while holding
* the @dev mutex lock.
@@ -5126,65 +5143,62 @@ static void pci_dev_restore(struct pci_dev *dev)
*/
int __pci_reset_function_locked(struct pci_dev *dev)
{
- int rc;
+ int i, m, rc = -ENOTTY;

might_sleep();

/*
- * A reset method returns -ENOTTY if it doesn't support this device
- * and we should try the next method.
+ * A reset method returns -ENOTTY if it doesn't support this device and
+ * we should try the next method.
*
- * If it returns 0 (success), we're finished. If it returns any
- * other error, we're also finished: this indicates that further
- * reset mechanisms might be broken on the device.
+ * If it returns 0 (success), we're finished. If it returns any other
+ * error, we're also finished: this indicates that further reset
+ * mechanisms might be broken on the device.
*/
- rc = pci_dev_specific_reset(dev, 0);
- if (rc != -ENOTTY)
- return rc;
- rc = pcie_reset_flr(dev, 0);
- if (rc != -ENOTTY)
- return rc;
- rc = pci_af_flr(dev, 0);
- if (rc != -ENOTTY)
- return rc;
- rc = pci_pm_reset(dev, 0);
- if (rc != -ENOTTY)
- return rc;
- return pci_reset_bus_function(dev, 0);
+ for (i = 0; i < PCI_NUM_RESET_METHODS && (m = dev->reset_methods[i]); i++) {
+ rc = pci_reset_fn_methods[m].reset_fn(dev, 0);
+ if (!rc)
+ return 0;
+ if (rc != -ENOTTY)
+ return rc;
+ }
+
+ return -ENOTTY;
}
EXPORT_SYMBOL_GPL(__pci_reset_function_locked);

/**
- * pci_probe_reset_function - check whether the device can be safely reset
- * @dev: PCI device to reset
+ * pci_init_reset_methods - check whether device can be safely reset
+ * and store supported reset mechanisms.
+ * @dev: PCI device to check for reset mechanisms
*
* Some devices allow an individual function to be reset without affecting
* other functions in the same device. The PCI device must be responsive
- * to PCI config space in order to use this function.
+ * to reads and writes to its PCI config space in order to use this function.
*
- * Returns 0 if the device function can be reset or negative if the
- * device doesn't support resetting a single function.
+ * Stores reset mechanisms supported by device in reset_methods byte array
+ * which is a member of struct pci_dev.
*/
-int pci_probe_reset_function(struct pci_dev *dev)
+void pci_init_reset_methods(struct pci_dev *dev)
{
- int rc;
+ int i, n, rc;
+ u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
+
+ n = 0;
+
+ BUILD_BUG_ON(ARRAY_SIZE(pci_reset_fn_methods) != PCI_NUM_RESET_METHODS);

might_sleep();

- rc = pci_dev_specific_reset(dev, 1);
- if (rc != -ENOTTY)
- return rc;
- rc = pcie_reset_flr(dev, 1);
- if (rc != -ENOTTY)
- return rc;
- rc = pci_af_flr(dev, 1);
- if (rc != -ENOTTY)
- return rc;
- rc = pci_pm_reset(dev, 1);
- if (rc != -ENOTTY)
- return rc;
+ for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
+ rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
+ if (!rc)
+ reset_methods[n++] = i;
+ else if (rc != -ENOTTY)
+ break;
+ }

- return pci_reset_bus_function(dev, 1);
+ memcpy(dev->reset_methods, reset_methods, sizeof(reset_methods));
}

/**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 37c913bbc..db1ad94e7 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -33,7 +33,8 @@ enum pci_mmap_api {
int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
enum pci_mmap_api mmap_api);

-int pci_probe_reset_function(struct pci_dev *dev);
+int pci_reset_supported(struct pci_dev *dev);
+void pci_init_reset_methods(struct pci_dev *dev);
int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
int pci_bus_error_reset(struct pci_dev *dev);

@@ -606,6 +607,12 @@ struct pci_dev_reset_methods {
int (*reset)(struct pci_dev *dev, int probe);
};

+struct pci_reset_fn_method {
+ int (*reset_fn)(struct pci_dev *pdev, int probe);
+ char *name;
+};
+
+extern const struct pci_reset_fn_method pci_reset_fn_methods[];
#ifdef CONFIG_PCI_QUIRKS
int pci_dev_specific_reset(struct pci_dev *dev, int probe);
#else
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 072a3d4dc..bc4af914a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2406,9 +2406,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
pci_rcec_init(dev); /* Root Complex Event Collector */

pcie_report_downtraining(dev);
-
- if (pci_probe_reset_function(dev) == 0)
- dev->reset_fn = 1;
+ pci_init_reset_methods(dev);
+ dev->reset_fn = pci_reset_supported(dev);
}

/*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d432428fd..9f3e85f33 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -49,6 +49,9 @@
PCI_STATUS_SIG_TARGET_ABORT | \
PCI_STATUS_PARITY)

+/* Number of reset methods used in pci_reset_fn_methods array in pci.c */
+#define PCI_NUM_RESET_METHODS 6
+
/*
* The PCI interface treats multi-function devices as independent
* devices. The slot/function address of each device is encoded
@@ -506,6 +509,10 @@ struct pci_dev {
char *driver_override; /* Driver name to force a match */

unsigned long priv_flags; /* Private flags for the PCI driver */
+ /*
+ * See pci_reset_fn_methods array in pci.c for ordering.
+ */
+ u8 reset_methods[PCI_NUM_RESET_METHODS]; /* Reset methods ordered by priority */
};

static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
--
2.32.0

2021-07-09 12:41:20

[permalink] [raw]

Subject: [PATCH v10 5/8] PCI: Define a function to set ACPI_COMPANION in pci_dev

From: Shanker Donthineni <[email protected]>

Move the existing code logic from acpi_pci_bridge_d3() to a separate
function pci_set_acpi_fwnode() to set the ACPI fwnode.

No functional change with this patch.

Signed-off-by: Shanker Donthineni <[email protected]>
---
drivers/pci/pci-acpi.c | 12 ++++++++----
drivers/pci/pci.h | 2 ++
2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 36bc23e21..eaddbf701 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -934,6 +934,13 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)

static struct acpi_device *acpi_pci_find_companion(struct device *dev);

+void pci_set_acpi_fwnode(struct pci_dev *dev)
+{
+ if (!ACPI_COMPANION(&dev->dev) && !pci_dev_is_added(dev))
+ ACPI_COMPANION_SET(&dev->dev,
+ acpi_pci_find_companion(&dev->dev));
+}
+
static bool acpi_pci_bridge_d3(struct pci_dev *dev)
{
const struct fwnode_handle *fwnode;
@@ -945,11 +952,8 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev)
return false;

/* Assume D3 support if the bridge is power-manageable by ACPI. */
+ pci_set_acpi_fwnode(dev);
adev = ACPI_COMPANION(&dev->dev);
- if (!adev && !pci_dev_is_added(dev)) {
- adev = acpi_pci_find_companion(&dev->dev);
- ACPI_COMPANION_SET(&dev->dev, adev);
- }

if (adev && acpi_device_power_manageable(adev))
return true;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index db1ad94e7..990b73e90 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -704,7 +704,9 @@ static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL
#ifdef CONFIG_ACPI
int pci_acpi_program_hp_params(struct pci_dev *dev);
extern const struct attribute_group pci_dev_acpi_attr_group;
+void pci_set_acpi_fwnode(struct pci_dev *dev);
#else
+static inline void pci_set_acpi_fwnode(struct pci_dev *dev) {}
static inline int pci_acpi_program_hp_params(struct pci_dev *dev)
{
return -ENODEV;
--
2.32.0

2021-07-09 12:41:22

[permalink] [raw]

Subject: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

Add reset_method sysfs attribute to enable user to query and set user
preferred device reset methods and their ordering.

Co-developed-by: Alex Williamson <[email protected]>
Signed-off-by: Alex Williamson <[email protected]>
Signed-off-by: Amey Narkhede <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-pci | 19 +++++
drivers/pci/pci-sysfs.c | 103 ++++++++++++++++++++++++
2 files changed, 122 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index ef00fada2..43f4e33c7 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -121,6 +121,25 @@ Description:
child buses, and re-discover devices removed earlier
from this part of the device tree.

+What: /sys/bus/pci/devices/.../reset_method
+Date: March 2021
+Contact: Amey Narkhede <[email protected]>
+Description:
+ Some devices allow an individual function to be reset
+ without affecting other functions in the same slot.
+
+ For devices that have this support, a file named
+ reset_method will be present in sysfs. Initially reading
+ this file will give names of the device supported reset
+ methods and their ordering. After write, this file will
+ give names and ordering of currently enabled reset methods.
+ Writing the name or comma separated list of names of any of
+ the device supported reset methods to this file will set
+ the reset methods and their ordering to be used when
+ resetting the device. Writing empty string to this file
+ will disable ability to reset the device and writing
+ "default" will return to the original value.
+
What: /sys/bus/pci/devices/.../reset
Date: July 2009
Contact: Michael S. Tsirkin <[email protected]>
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 316f70c3e..8a740e211 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1334,6 +1334,108 @@ static const struct attribute_group pci_dev_rom_attr_group = {
.is_bin_visible = pci_dev_rom_attr_is_visible,
};

+static ssize_t reset_method_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ ssize_t len = 0;
+ int i, idx;
+
+ for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
+ idx = pdev->reset_methods[i];
+ if (!idx)
+ break;
+
+ len += sysfs_emit_at(buf, len, "%s%s", len ? "," : "",
+ pci_reset_fn_methods[idx].name);
+ }
+
+ if (len)
+ len += sysfs_emit_at(buf, len, "\n");
+
+ return len;
+}
+
+static ssize_t reset_method_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ int n = 0;
+ char *name, *options = NULL;
+ u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
+
+ if (count >= (PAGE_SIZE - 1))
+ return -EINVAL;
+
+ if (sysfs_streq(buf, "")) {
+ pci_warn(pdev, "All device reset methods disabled by user");
+ goto set_reset_methods;
+ }
+
+ if (sysfs_streq(buf, "default")) {
+ pci_init_reset_methods(pdev);
+ return count;
+ }
+
+ options = kstrndup(buf, count, GFP_KERNEL);
+ if (!options)
+ return -ENOMEM;
+
+ while ((name = strsep(&options, ",")) != NULL) {
+ int i;
+
+ if (sysfs_streq(name, ""))
+ continue;
+
+ name = strim(name);
+
+ for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
+ if (sysfs_streq(name, pci_reset_fn_methods[i].name) &&
+ !pci_reset_fn_methods[i].reset_fn(pdev, 1)) {
+ reset_methods[n++] = i;
+ break;
+ }
+ }
+
+ if (i == PCI_NUM_RESET_METHODS) {
+ kfree(options);
+ return -EINVAL;
+ }
+ }
+
+ if (!pci_reset_fn_methods[1].reset_fn(pdev, 1) && reset_methods[0] != 1)
+ pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");
+
+set_reset_methods:
+ memcpy(pdev->reset_methods, reset_methods, sizeof(reset_methods));
+ kfree(options);
+ return count;
+}
+static DEVICE_ATTR_RW(reset_method);
+
+static struct attribute *pci_dev_reset_method_attrs[] = {
+ &dev_attr_reset_method.attr,
+ NULL,
+};
+
+static umode_t pci_dev_reset_method_attr_is_visible(struct kobject *kobj,
+ struct attribute *a, int n)
+{
+ struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
+
+ if (!pci_reset_supported(pdev))
+ return 0;
+
+ return a->mode;
+}
+
+static const struct attribute_group pci_dev_reset_method_attr_group = {
+ .attrs = pci_dev_reset_method_attrs,
+ .is_visible = pci_dev_reset_method_attr_is_visible,
+};
+
static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
@@ -1491,6 +1593,7 @@ const struct attribute_group *pci_dev_groups[] = {
&pci_dev_config_attr_group,
&pci_dev_rom_attr_group,
&pci_dev_reset_attr_group,
+ &pci_dev_reset_method_attr_group,
&pci_dev_vpd_attr_group,
#ifdef CONFIG_DMI
&pci_dev_smbios_attr_group,
--
2.32.0

2021-07-09 12:42:29

[permalink] [raw]

Subject: [PATCH v10 8/8] PCI: Change the type of probe argument in reset functions

Introduce a new enum pci_reset_mode_t to make the context of probe argument
in reset functions clear and the code easier to read. Change the type of
probe argument in functions which implement reset methods from int to
pci_reset_mode_t to make the intent clear.

Add a new line in return statement of pci_reset_bus_function().

Suggested-by: Alex Williamson <[email protected]>
Suggested-by: Krzysztof Wilczyński <[email protected]>
Signed-off-by: Amey Narkhede <[email protected]>
---
drivers/crypto/cavium/nitrox/nitrox_main.c | 2 +-
.../ethernet/cavium/liquidio/lio_vf_main.c | 2 +-
drivers/pci/hotplug/pciehp.h | 2 +-
drivers/pci/hotplug/pciehp_hpc.c | 4 +-
drivers/pci/hotplug/pnv_php.c | 4 +-
drivers/pci/pci-acpi.c | 10 ++-
drivers/pci/pci-sysfs.c | 6 +-
drivers/pci/pci.c | 85 ++++++++++++-------
drivers/pci/pci.h | 12 +--
drivers/pci/pcie/aer.c | 2 +-
drivers/pci/quirks.c | 37 ++++----
include/linux/pci.h | 8 +-
include/linux/pci_hotplug.h | 2 +-
13 files changed, 107 insertions(+), 69 deletions(-)

diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
index 15d6c8452..f97fa8e99 100644
--- a/drivers/crypto/cavium/nitrox/nitrox_main.c
+++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
@@ -306,7 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
return -ENOMEM;
}

- pcie_reset_flr(pdev, 0);
+ pcie_reset_flr(pdev, PCI_RESET_DO_RESET);

pci_restore_state(pdev);

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
index 336d149ee..6e666be69 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
@@ -526,7 +526,7 @@ static void octeon_destroy_resources(struct octeon_device *oct)
oct->irq_name_storage = NULL;
}
/* Soft reset the octeon device before exiting */
- if (!pcie_reset_flr(oct->pci_dev, 1))
+ if (!pcie_reset_flr(oct->pci_dev, PCI_RESET_PROBE))
octeon_pci_flr(oct);
else
cn23xx_vf_ask_pf_to_do_flr(oct);
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 4fd200d8b..87da03adc 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -181,7 +181,7 @@ void pciehp_release_ctrl(struct controller *ctrl);

int pciehp_sysfs_enable_slot(struct hotplug_slot *hotplug_slot);
int pciehp_sysfs_disable_slot(struct hotplug_slot *hotplug_slot);
-int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, int probe);
+int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, pci_reset_mode_t mode);
int pciehp_get_attention_status(struct hotplug_slot *hotplug_slot, u8 *status);
int pciehp_set_raw_indicator_status(struct hotplug_slot *h_slot, u8 status);
int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index fb3840e22..24b3c8787 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -834,14 +834,14 @@ void pcie_disable_interrupt(struct controller *ctrl)
* momentarily, if we see that they could interfere. Also, clear any spurious
* events after.
*/
-int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, int probe)
+int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, pci_reset_mode_t mode)
{
struct controller *ctrl = to_ctrl(hotplug_slot);
struct pci_dev *pdev = ctrl_dev(ctrl);
u16 stat_mask = 0, ctrl_mask = 0;
int rc;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

down_write(&ctrl->reset_lock);
diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
index 04565162a..77df598c8 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -526,7 +526,7 @@ static int pnv_php_enable(struct pnv_php_slot *php_slot, bool rescan)
return 0;
}

-static int pnv_php_reset_slot(struct hotplug_slot *slot, int probe)
+static int pnv_php_reset_slot(struct hotplug_slot *slot, pci_reset_mode_t mode)
{
struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
struct pci_dev *bridge = php_slot->pdev;
@@ -537,7 +537,7 @@ static int pnv_php_reset_slot(struct hotplug_slot *slot, int probe)
* which don't have a bridge. Only claim to support
* reset_slot() if we have a bridge device (for now...)
*/
- if (probe)
+ if (mode == PCI_RESET_PROBE)
return !bridge;

/* mask our interrupt while resetting the bridge */
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index b6de71d15..a92ed574a 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -944,16 +944,20 @@ void pci_set_acpi_fwnode(struct pci_dev *dev)
/**
* pci_dev_acpi_reset - do a function level reset using _RST method
* @dev: device to reset
- * @probe: check if _RST method is included in the acpi_device context.
+ * @probe: If PCI_RESET_PROBE, check whether _RST method is included
+ * in the acpi_device context.
*/
-int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
+int pci_dev_acpi_reset(struct pci_dev *dev, pci_reset_mode_t mode)
{
acpi_handle handle = ACPI_HANDLE(&dev->dev);

+ if (mode >= PCI_RESET_MODE_MAX)
+ return -EINVAL;
+
if (!handle || !acpi_has_method(handle, "_RST"))
return -ENOTTY;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 8a740e211..dcf19f6d6 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1393,7 +1393,8 @@ static ssize_t reset_method_store(struct device *dev,

for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
if (sysfs_streq(name, pci_reset_fn_methods[i].name) &&
- !pci_reset_fn_methods[i].reset_fn(pdev, 1)) {
+ !pci_reset_fn_methods[i].reset_fn(pdev,
+ PCI_RESET_PROBE)) {
reset_methods[n++] = i;
break;
}
@@ -1405,7 +1406,8 @@ static ssize_t reset_method_store(struct device *dev,
}
}

- if (!pci_reset_fn_methods[1].reset_fn(pdev, 1) && reset_methods[0] != 1)
+ if (!pci_reset_fn_methods[1].reset_fn(pdev, PCI_RESET_PROBE) &&
+ reset_methods[0] != 1)
pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");

set_reset_methods:
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1e64dbd3e..60204cee6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4650,30 +4650,36 @@ EXPORT_SYMBOL_GPL(pcie_flr);
/**
* pcie_reset_flr - initiate a PCIe function level reset
* @dev: device to reset
- * @probe: If set, only check if the device can be reset this way.
+ * @mode: If PCI_RESET_PROBE, only check if the device can be reset this way.
*
* Initiate a function level reset on @dev.
*/
-int pcie_reset_flr(struct pci_dev *dev, int probe)
+int pcie_reset_flr(struct pci_dev *dev, pci_reset_mode_t mode)
{
+ if (mode >= PCI_RESET_MODE_MAX)
+ return -EINVAL;
+
if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
return -ENOTTY;

if (!dev->has_pcie_flr)
return -ENOTTY;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

return pcie_flr(dev);
}
EXPORT_SYMBOL_GPL(pcie_reset_flr);

-static int pci_af_flr(struct pci_dev *dev, int probe)
+static int pci_af_flr(struct pci_dev *dev, pci_reset_mode_t mode)
{
int pos;
u8 cap;

+ if (mode >= PCI_RESET_MODE_MAX)
+ return -EINVAL;
+
pos = pci_find_capability(dev, PCI_CAP_ID_AF);
if (!pos)
return -ENOTTY;
@@ -4685,7 +4691,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
if (!(cap & PCI_AF_CAP_TP) || !(cap & PCI_AF_CAP_FLR))
return -ENOTTY;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

/*
@@ -4716,7 +4722,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
/**
* pci_pm_reset - Put device into PCI_D3 and back into PCI_D0.
* @dev: Device to reset.
- * @probe: If set, only check if the device can be reset this way.
+ * @mode: If PCI_RESET_PROBE, only check if the device can be reset this way.
*
* If @dev supports native PCI PM and its PCI_PM_CTRL_NO_SOFT_RESET flag is
* unset, it will be reinitialized internally when going from PCI_D3hot to
@@ -4728,10 +4734,13 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
* by default (i.e. unless the @dev's d3hot_delay field has a different value).
* Moreover, only devices in D0 can be reset by this function.
*/
-static int pci_pm_reset(struct pci_dev *dev, int probe)
+static int pci_pm_reset(struct pci_dev *dev, pci_reset_mode_t mode)
{
u16 csr;

+ if (mode >= PCI_RESET_MODE_MAX)
+ return -EINVAL;
+
if (!dev->pm_cap || dev->dev_flags & PCI_DEV_FLAGS_NO_PM_RESET)
return -ENOTTY;

@@ -4739,7 +4748,7 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
if (csr & PCI_PM_CTRL_NO_SOFT_RESET)
return -ENOTTY;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

if (dev->current_state != PCI_D0)
@@ -4988,10 +4997,13 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
}
EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);

-static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
+static int pci_parent_bus_reset(struct pci_dev *dev, pci_reset_mode_t mode)
{
struct pci_dev *pdev;

+ if (mode >= PCI_RESET_MODE_MAX)
+ return -EINVAL;
+
if (pci_is_root_bus(dev->bus) || dev->subordinate ||
!dev->bus->self || dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
return -ENOTTY;
@@ -5000,44 +5012,47 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
if (pdev != dev)
return -ENOTTY;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

return pci_bridge_secondary_bus_reset(dev->bus->self);
}

-static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, int probe)
+static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, pci_reset_mode_t mode)
{
int rc = -ENOTTY;

+ if (mode >= PCI_RESET_MODE_MAX)
+ return -EINVAL;
+
if (!hotplug || !try_module_get(hotplug->owner))
return rc;

if (hotplug->ops->reset_slot)
- rc = hotplug->ops->reset_slot(hotplug, probe);
+ rc = hotplug->ops->reset_slot(hotplug, mode);

module_put(hotplug->owner);

return rc;
}

-static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
+static int pci_dev_reset_slot_function(struct pci_dev *dev, pci_reset_mode_t mode)
{
if (dev->multifunction || dev->subordinate || !dev->slot ||
dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
return -ENOTTY;

- return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
+ return pci_reset_hotplug_slot(dev->slot->hotplug, mode);
}

-static int pci_reset_bus_function(struct pci_dev *dev, int probe)
+static int pci_reset_bus_function(struct pci_dev *dev, pci_reset_mode_t mode)
{
int rc;

- rc = pci_dev_reset_slot_function(dev, probe);
+ rc = pci_dev_reset_slot_function(dev, mode);
if (rc != -ENOTTY)
return rc;
- return pci_parent_bus_reset(dev, probe);
+ return pci_parent_bus_reset(dev, mode);
}

static void pci_dev_lock(struct pci_dev *dev)
@@ -5157,7 +5172,7 @@ int __pci_reset_function_locked(struct pci_dev *dev)
* mechanisms might be broken on the device.
*/
for (i = 0; i < PCI_NUM_RESET_METHODS && (m = dev->reset_methods[i]); i++) {
- rc = pci_reset_fn_methods[m].reset_fn(dev, 0);
+ rc = pci_reset_fn_methods[m].reset_fn(dev, PCI_RESET_DO_RESET);
if (!rc)
return 0;
if (rc != -ENOTTY)
@@ -5192,7 +5207,7 @@ void pci_init_reset_methods(struct pci_dev *dev)
might_sleep();

for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
- rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
+ rc = pci_reset_fn_methods[i].reset_fn(dev, PCI_RESET_PROBE);
if (!rc)
reset_methods[n++] = i;
else if (rc != -ENOTTY)
@@ -5509,21 +5524,24 @@ static void pci_slot_restore_locked(struct pci_slot *slot)
}
}

-static int pci_slot_reset(struct pci_slot *slot, int probe)
+static int pci_slot_reset(struct pci_slot *slot, pci_reset_mode_t mode)
{
int rc;

+ if (mode >= PCI_RESET_MODE_MAX)
+ return -EINVAL;
+
if (!slot || !pci_slot_resetable(slot))
return -ENOTTY;

- if (!probe)
+ if (mode != PCI_RESET_PROBE)
pci_slot_lock(slot);

might_sleep();

- rc = pci_reset_hotplug_slot(slot->hotplug, probe);
+ rc = pci_reset_hotplug_slot(slot->hotplug, mode);

- if (!probe)
+ if (mode != PCI_RESET_PROBE)
pci_slot_unlock(slot);

return rc;
@@ -5537,7 +5555,7 @@ static int pci_slot_reset(struct pci_slot *slot, int probe)
*/
int pci_probe_reset_slot(struct pci_slot *slot)
{
- return pci_slot_reset(slot, 1);
+ return pci_slot_reset(slot, PCI_RESET_PROBE);
}
EXPORT_SYMBOL_GPL(pci_probe_reset_slot);

@@ -5560,14 +5578,14 @@ static int __pci_reset_slot(struct pci_slot *slot)
{
int rc;

- rc = pci_slot_reset(slot, 1);
+ rc = pci_slot_reset(slot, PCI_RESET_PROBE);
if (rc)
return rc;

if (pci_slot_trylock(slot)) {
pci_slot_save_and_disable_locked(slot);
might_sleep();
- rc = pci_reset_hotplug_slot(slot->hotplug, 0);
+ rc = pci_reset_hotplug_slot(slot->hotplug, PCI_RESET_DO_RESET);
pci_slot_restore_locked(slot);
pci_slot_unlock(slot);
} else
@@ -5576,14 +5594,17 @@ static int __pci_reset_slot(struct pci_slot *slot)
return rc;
}

-static int pci_bus_reset(struct pci_bus *bus, int probe)
+static int pci_bus_reset(struct pci_bus *bus, pci_reset_mode_t mode)
{
int ret;

+ if (mode >= PCI_RESET_MODE_MAX)
+ return -EINVAL;
+
if (!bus->self || !pci_bus_resetable(bus))
return -ENOTTY;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

pci_bus_lock(bus);
@@ -5622,14 +5643,14 @@ int pci_bus_error_reset(struct pci_dev *bridge)
goto bus_reset;

list_for_each_entry(slot, &bus->slots, list)
- if (pci_slot_reset(slot, 0))
+ if (pci_slot_reset(slot, PCI_RESET_DO_RESET))
goto bus_reset;

mutex_unlock(&pci_slot_mutex);
return 0;
bus_reset:
mutex_unlock(&pci_slot_mutex);
- return pci_bus_reset(bridge->subordinate, 0);
+ return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
}

/**
@@ -5640,7 +5661,7 @@ int pci_bus_error_reset(struct pci_dev *bridge)
*/
int pci_probe_reset_bus(struct pci_bus *bus)
{
- return pci_bus_reset(bus, 1);
+ return pci_bus_reset(bus, PCI_RESET_PROBE);
}
EXPORT_SYMBOL_GPL(pci_probe_reset_bus);

@@ -5654,7 +5675,7 @@ static int __pci_reset_bus(struct pci_bus *bus)
{
int rc;

- rc = pci_bus_reset(bus, 1);
+ rc = pci_bus_reset(bus, PCI_RESET_PROBE);
if (rc)
return rc;

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2c12017ed..06be9e6fa 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -604,19 +604,19 @@ static inline int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
struct pci_dev_reset_methods {
u16 vendor;
u16 device;
- int (*reset)(struct pci_dev *dev, int probe);
+ int (*reset)(struct pci_dev *dev, pci_reset_mode_t mode);
};

struct pci_reset_fn_method {
- int (*reset_fn)(struct pci_dev *pdev, int probe);
+ int (*reset_fn)(struct pci_dev *pdev, pci_reset_mode_t mode);
char *name;
};

extern const struct pci_reset_fn_method pci_reset_fn_methods[];
#ifdef CONFIG_PCI_QUIRKS
-int pci_dev_specific_reset(struct pci_dev *dev, int probe);
+int pci_dev_specific_reset(struct pci_dev *dev, pci_reset_mode_t mode);
#else
-static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
+static inline int pci_dev_specific_reset(struct pci_dev *dev, pci_reset_mode_t mode)
{
return -ENOTTY;
}
@@ -705,9 +705,9 @@ static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL
int pci_acpi_program_hp_params(struct pci_dev *dev);
extern const struct attribute_group pci_dev_acpi_attr_group;
void pci_set_acpi_fwnode(struct pci_dev *dev);
-int pci_dev_acpi_reset(struct pci_dev *dev, int probe);
+int pci_dev_acpi_reset(struct pci_dev *dev, pci_reset_mode_t mode);
#else
-static inline int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
+static inline int pci_dev_acpi_reset(struct pci_dev *dev, pci_reset_mode_t mode)
{
return -ENOTTY;
}
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 98077595a..cfa7a1775 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1405,7 +1405,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
}

if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
- rc = pcie_reset_flr(dev, 0);
+ rc = pcie_reset_flr(dev, PCI_RESET_DO_RESET);
if (!rc)
pci_info(dev, "has been reset\n");
else
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index e86cf4a3b..e7f15fc02 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3669,7 +3669,7 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
* reset a single function if other methods (e.g. FLR, PM D0->D3) are
* not available.
*/
-static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
+static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, pci_reset_mode_t mode)
{
/*
* http://www.intel.com/content/dam/doc/datasheet/82599-10-gbe-controller-datasheet.pdf
@@ -3679,7 +3679,7 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
* Thus we must call pcie_flr() directly without first checking if it is
* supported.
*/
- if (!probe)
+ if (mode == PCI_RESET_DO_RESET)
pcie_flr(dev);
return 0;
}
@@ -3691,13 +3691,13 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
#define NSDE_PWR_STATE 0xd0100
#define IGD_OPERATION_TIMEOUT 10000 /* set timeout 10 seconds */

-static int reset_ivb_igd(struct pci_dev *dev, int probe)
+static int reset_ivb_igd(struct pci_dev *dev, pci_reset_mode_t mode)
{
void __iomem *mmio_base;
unsigned long timeout;
u32 val;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

mmio_base = pci_iomap(dev, 0, 0);
@@ -3734,7 +3734,7 @@ static int reset_ivb_igd(struct pci_dev *dev, int probe)
}

/* Device-specific reset method for Chelsio T4-based adapters */
-static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
+static int reset_chelsio_generic_dev(struct pci_dev *dev, pci_reset_mode_t mode)
{
u16 old_command;
u16 msix_flags;
@@ -3750,7 +3750,7 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
* If this is the "probe" phase, return 0 indicating that we can
* reset this device.
*/
- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

/*
@@ -3812,17 +3812,17 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
* Chapter 3: NVMe control registers
* Chapter 7.3: Reset behavior
*/
-static int nvme_disable_and_flr(struct pci_dev *dev, int probe)
+static int nvme_disable_and_flr(struct pci_dev *dev, pci_reset_mode_t mode)
{
void __iomem *bar;
u16 cmd;
u32 cfg;

if (dev->class != PCI_CLASS_STORAGE_EXPRESS ||
- pcie_reset_flr(dev, 1) || !pci_resource_start(dev, 0))
+ pcie_reset_flr(dev, PCI_RESET_PROBE) || !pci_resource_start(dev, 0))
return -ENOTTY;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

bar = pci_iomap(dev, 0, NVME_REG_CC + sizeof(cfg));
@@ -3886,11 +3886,13 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe)
* device too soon after FLR. A 250ms delay after FLR has heuristically
* proven to produce reliably working results for device assignment cases.
*/
-static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
+static int delay_250ms_after_flr(struct pci_dev *dev, pci_reset_mode_t mode)
{
- int ret = pcie_reset_flr(dev, probe);
+ int ret;
+
+ ret = pcie_reset_flr(dev, mode);

- if (probe)
+ if (ret || mode == PCI_RESET_PROBE)
return ret;

msleep(250);
@@ -3906,13 +3908,13 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
#define HINIC_OPERATION_TIMEOUT 15000 /* 15 seconds */

/* Device-specific reset method for Huawei Intelligent NIC virtual functions */
-static int reset_hinic_vf_dev(struct pci_dev *pdev, int probe)
+static int reset_hinic_vf_dev(struct pci_dev *pdev, pci_reset_mode_t mode)
{
unsigned long timeout;
void __iomem *bar;
u32 val;

- if (probe)
+ if (mode == PCI_RESET_PROBE)
return 0;

bar = pci_iomap(pdev, 0, 0);
@@ -3983,16 +3985,19 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
* because when a host assigns a device to a guest VM, the host may need
* to reset the device but probably doesn't have a driver for it.
*/
-int pci_dev_specific_reset(struct pci_dev *dev, int probe)
+int pci_dev_specific_reset(struct pci_dev *dev, pci_reset_mode_t mode)
{
const struct pci_dev_reset_methods *i;

+ if (mode >= PCI_RESET_MODE_MAX)
+ return -EINVAL;
+
for (i = pci_dev_reset_methods; i->reset; i++) {
if ((i->vendor == dev->vendor ||
i->vendor == (u16)PCI_ANY_ID) &&
(i->device == dev->device ||
i->device == (u16)PCI_ANY_ID))
- return i->reset(dev, probe);
+ return i->reset(dev, mode);
}

return -ENOTTY;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c3b0d771c..0d650c873 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -52,6 +52,12 @@
/* Number of reset methods used in pci_reset_fn_methods array in pci.c */
#define PCI_NUM_RESET_METHODS 7

+typedef enum pci_reset_mode {
+ PCI_RESET_DO_RESET,
+ PCI_RESET_PROBE,
+ PCI_RESET_MODE_MAX,
+} pci_reset_mode_t;
+
/*
* The PCI interface treats multi-function devices as independent
* devices. The slot/function address of each device is encoded
@@ -1232,7 +1238,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
enum pci_bus_speed *speed,
enum pcie_link_width *width);
void pcie_print_link_status(struct pci_dev *dev);
-int pcie_reset_flr(struct pci_dev *dev, int probe);
+int pcie_reset_flr(struct pci_dev *dev, pci_reset_mode_t mode);
int pcie_flr(struct pci_dev *dev);
int __pci_reset_function_locked(struct pci_dev *dev);
int pci_reset_function(struct pci_dev *dev);
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index b482e42d7..9e8da46e7 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -44,7 +44,7 @@ struct hotplug_slot_ops {
int (*get_attention_status) (struct hotplug_slot *slot, u8 *value);
int (*get_latch_status) (struct hotplug_slot *slot, u8 *value);
int (*get_adapter_status) (struct hotplug_slot *slot, u8 *value);
- int (*reset_slot) (struct hotplug_slot *slot, int probe);
+ int (*reset_slot) (struct hotplug_slot *slot, pci_reset_mode_t mode);
};

/**
--
2.32.0

2021-07-09 12:44:24

[permalink] [raw]

Subject: [PATCH v10 6/8] PCI: Setup ACPI fwnode early and at the same time with OF

From: Shanker Donthineni <[email protected]>

The pci_dev objects are created through two mechanisms 1) during PCI
bus scan and 2) from I/O Virtualization. The fwnode in pci_dev object
is being set at different places depends on the type of firmware used,
device creation mechanism, and acpi_pci_bridge_d3() WAR.

The software features which have a dependency on ACPI fwnode properties
and need to be handled before device_add() will not work. One use case,
the software has to check the existence of _RST method to support ACPI
based reset method.

This patch does the two changes in order to provide fwnode consistently.
- Set ACPI and OF fwnodes from pci_setup_device().
- Remove pci_set_acpi_fwnode() in acpi_pci_bridge_d3().

After this patch, ACPI/OF firmware properties are visible at the same
time during the early stage of pci_dev setup. And also call sites should
be able to use firmware agnostic functions device_property_xxx() for the
early PCI quirks in the future.

Signed-off-by: Shanker Donthineni <[email protected]>
---
drivers/pci/pci-acpi.c | 1 -
drivers/pci/probe.c | 7 ++++---
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index eaddbf701..dae021322 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev)
return false;

/* Assume D3 support if the bridge is power-manageable by ACPI. */
- pci_set_acpi_fwnode(dev);
adev = ACPI_COMPANION(&dev->dev);

if (adev && acpi_device_power_manageable(adev))
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c272e23db..c911d6a5c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1790,6 +1790,9 @@ int pci_setup_device(struct pci_dev *dev)
dev->error_state = pci_channel_io_normal;
set_pcie_port_type(dev);

+ pci_set_of_node(dev);
+ pci_set_acpi_fwnode(dev);
+
pci_dev_assign_slot(dev);

/*
@@ -1925,6 +1928,7 @@ int pci_setup_device(struct pci_dev *dev)
default: /* unknown header */
pci_err(dev, "unknown header type %02x, ignoring device\n",
dev->hdr_type);
+ pci_release_of_node(dev);
return -EIO;

bad:
@@ -2352,10 +2356,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
dev->vendor = l & 0xffff;
dev->device = (l >> 16) & 0xffff;

- pci_set_of_node(dev);
-
if (pci_setup_device(dev)) {
- pci_release_of_node(dev);
pci_bus_put(dev->bus);
kfree(dev);
return NULL;
--
2.32.0

2021-07-09 12:45:10

[permalink] [raw]

Subject: [PATCH v10 7/8] PCI: Add support for ACPI _RST reset method

From: Shanker Donthineni <[email protected]>

The _RST is a standard method specified in the ACPI specification. It
provides a function level reset when it is described in the acpi_device
context associated with PCI-device. Implement a new reset function
pci_dev_acpi_reset() for probing RST method and execute if it is defined
in the firmware.

The default priority of the ACPI reset is set to below device-specific
and above hardware resets.

Signed-off-by: Shanker Donthineni <[email protected]>
Suggested-by: Alex Williamson <[email protected]>
Reviewed-by: Sinan Kaya <[email protected]>
---
drivers/pci/pci-acpi.c | 23 +++++++++++++++++++++++
drivers/pci/pci.c | 1 +
drivers/pci/pci.h | 6 ++++++
include/linux/pci.h | 2 +-
4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index dae021322..b6de71d15 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -941,6 +941,29 @@ void pci_set_acpi_fwnode(struct pci_dev *dev)
acpi_pci_find_companion(&dev->dev));
}

+/**
+ * pci_dev_acpi_reset - do a function level reset using _RST method
+ * @dev: device to reset
+ * @probe: check if _RST method is included in the acpi_device context.
+ */
+int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
+{
+ acpi_handle handle = ACPI_HANDLE(&dev->dev);
+
+ if (!handle || !acpi_has_method(handle, "_RST"))
+ return -ENOTTY;
+
+ if (probe)
+ return 0;
+
+ if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
+ pci_warn(dev, "ACPI _RST failed\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static bool acpi_pci_bridge_d3(struct pci_dev *dev)
{
const struct fwnode_handle *fwnode;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d5c16492c..1e64dbd3e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5115,6 +5115,7 @@ static void pci_dev_restore(struct pci_dev *dev)
const struct pci_reset_fn_method pci_reset_fn_methods[] = {
{ },
{ &pci_dev_specific_reset, .name = "device_specific" },
+ { &pci_dev_acpi_reset, .name = "acpi" },
{ &pcie_reset_flr, .name = "flr" },
{ &pci_af_flr, .name = "af_flr" },
{ &pci_pm_reset, .name = "pm" },
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 990b73e90..2c12017ed 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -705,7 +705,13 @@ static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL
int pci_acpi_program_hp_params(struct pci_dev *dev);
extern const struct attribute_group pci_dev_acpi_attr_group;
void pci_set_acpi_fwnode(struct pci_dev *dev);
+int pci_dev_acpi_reset(struct pci_dev *dev, int probe);
#else
+static inline int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
+{
+ return -ENOTTY;
+}
+
static inline void pci_set_acpi_fwnode(struct pci_dev *dev) {}
static inline int pci_acpi_program_hp_params(struct pci_dev *dev)
{
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f34563831..c3b0d771c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -50,7 +50,7 @@
PCI_STATUS_PARITY)

/* Number of reset methods used in pci_reset_fn_methods array in pci.c */
-#define PCI_NUM_RESET_METHODS 6
+#define PCI_NUM_RESET_METHODS 7

/*
* The PCI interface treats multi-function devices as independent
--
2.32.0

2021-07-09 12:47:17

[permalink] [raw]

Subject: Re: [PATCH v10 0/8] Expose and manage PCI device reset

On 21/07/09 06:08PM, Amey Narkhede wrote:
> PCI and PCIe devices may support a number of possible reset mechanisms
> for example Function Level Reset (FLR) provided via Advanced Feature or
> PCIe capabilities, Power Management reset, bus reset, or device specific reset.
> Currently the PCI subsystem creates a policy prioritizing these reset methods
> which provides neither visibility nor control to userspace.
>
> Expose the reset methods available per device to userspace, via sysfs
> and allow an administrative user or device owner to have ability to
> manage per device reset method priorities or exclusions.
> This feature aims to allow greater control of a device for use cases
> as device assignment, where specific device or platform issues may
> interact poorly with a given reset method, and for which device specific
> quirks have not been developed.
>
> Changes in v10:
> - Fix build error on ppc as reported by build bot
>
Aplogies for late response. For some reason I did not get email from
test bot. I checked spam folder too. Not sure if gmail messed something
up.

[...]
Amey

2021-07-12 22:08:34

[permalink] [raw]

Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

On Fri, 9 Jul 2021 18:08:06 +0530
Amey Narkhede <[email protected]> wrote:

> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
> FLR to avoid reading PCI_EXP_DEVCAP multiple times.
>
> Currently there is separate function pcie_has_flr() to probe if PCIe FLR
> is supported by the device which does not match the calling convention
> followed by reset methods which use second function argument to decide
> whether to probe or not. Add new function pcie_reset_flr() that follows
> the calling convention of reset methods.
>
> Signed-off-by: Amey Narkhede <[email protected]>
> ---
> drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
> drivers/pci/pci.c | 59 +++++++++++-----------
> drivers/pci/pcie/aer.c | 12 ++---
> drivers/pci/probe.c | 6 ++-
> drivers/pci/quirks.c | 9 ++--
> include/linux/pci.h | 3 +-
> 6 files changed, 45 insertions(+), 48 deletions(-)

Looks good to me,

Reviewed-by: Alex Williamson <[email protected]>

2021-07-12 22:26:01

[permalink] [raw]

Subject: Re: [PATCH v10 8/8] PCI: Change the type of probe argument in reset functions

On Fri, 9 Jul 2021 18:08:13 +0530
Amey Narkhede <[email protected]> wrote:

> Introduce a new enum pci_reset_mode_t to make the context of probe argument
> in reset functions clear and the code easier to read. Change the type of
> probe argument in functions which implement reset methods from int to
> pci_reset_mode_t to make the intent clear.
>
> Add a new line in return statement of pci_reset_bus_function().
>
> Suggested-by: Alex Williamson <[email protected]>
> Suggested-by: Krzysztof Wilczyński <[email protected]>
> Signed-off-by: Amey Narkhede <[email protected]>
> ---
> drivers/crypto/cavium/nitrox/nitrox_main.c | 2 +-
> .../ethernet/cavium/liquidio/lio_vf_main.c | 2 +-
> drivers/pci/hotplug/pciehp.h | 2 +-
> drivers/pci/hotplug/pciehp_hpc.c | 4 +-
> drivers/pci/hotplug/pnv_php.c | 4 +-
> drivers/pci/pci-acpi.c | 10 ++-
> drivers/pci/pci-sysfs.c | 6 +-
> drivers/pci/pci.c | 85 ++++++++++++-------
> drivers/pci/pci.h | 12 +--
> drivers/pci/pcie/aer.c | 2 +-
> drivers/pci/quirks.c | 37 ++++----
> include/linux/pci.h | 8 +-
> include/linux/pci_hotplug.h | 2 +-
> 13 files changed, 107 insertions(+), 69 deletions(-)

Reviewed-by: Alex Williamson <[email protected]>

2021-07-12 22:30:58

[permalink] [raw]

Subject: Re: [PATCH v10 5/8] PCI: Define a function to set ACPI_COMPANION in pci_dev

On Fri, 9 Jul 2021 18:08:10 +0530
Amey Narkhede <[email protected]> wrote:

> From: Shanker Donthineni <[email protected]>
>
> Move the existing code logic from acpi_pci_bridge_d3() to a separate
> function pci_set_acpi_fwnode() to set the ACPI fwnode.
>
> No functional change with this patch.
>
> Signed-off-by: Shanker Donthineni <[email protected]>
> ---
> drivers/pci/pci-acpi.c | 12 ++++++++----
> drivers/pci/pci.h | 2 ++
> 2 files changed, 10 insertions(+), 4 deletions(-)

Reviewed-by: Alex Williamson <[email protected]>

2021-07-12 23:10:52

[permalink] [raw]

Subject: Re: [PATCH v10 7/8] PCI: Add support for ACPI _RST reset method

On Fri, 9 Jul 2021 18:08:12 +0530
Amey Narkhede <[email protected]> wrote:

> From: Shanker Donthineni <[email protected]>
>
> The _RST is a standard method specified in the ACPI specification. It
> provides a function level reset when it is described in the acpi_device
> context associated with PCI-device. Implement a new reset function
> pci_dev_acpi_reset() for probing RST method and execute if it is defined
> in the firmware.
>
> The default priority of the ACPI reset is set to below device-specific
> and above hardware resets.
>
> Signed-off-by: Shanker Donthineni <[email protected]>
> Suggested-by: Alex Williamson <[email protected]>
> Reviewed-by: Sinan Kaya <[email protected]>
> ---
> drivers/pci/pci-acpi.c | 23 +++++++++++++++++++++++
> drivers/pci/pci.c | 1 +
> drivers/pci/pci.h | 6 ++++++
> include/linux/pci.h | 2 +-
> 4 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index dae021322..b6de71d15 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -941,6 +941,29 @@ void pci_set_acpi_fwnode(struct pci_dev *dev)
> acpi_pci_find_companion(&dev->dev));
> }
>
> +/**
> + * pci_dev_acpi_reset - do a function level reset using _RST method
> + * @dev: device to reset
> + * @probe: check if _RST method is included in the acpi_device context.
> + */
> +int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
> +{
> + acpi_handle handle = ACPI_HANDLE(&dev->dev);
> +
> + if (!handle || !acpi_has_method(handle, "_RST"))
> + return -ENOTTY;
> +
> + if (probe)
> + return 0;
> +
> + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
> + pci_warn(dev, "ACPI _RST failed\n");
> + return -EINVAL;

Should we return -ENOTTY here instead to give a possible secondary
reset method a chance? Thanks,

Alex

> + }
> +
> + return 0;
> +}
> +
> static bool acpi_pci_bridge_d3(struct pci_dev *dev)
> {
> const struct fwnode_handle *fwnode;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d5c16492c..1e64dbd3e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5115,6 +5115,7 @@ static void pci_dev_restore(struct pci_dev *dev)
> const struct pci_reset_fn_method pci_reset_fn_methods[] = {
> { },
> { &pci_dev_specific_reset, .name = "device_specific" },
> + { &pci_dev_acpi_reset, .name = "acpi" },
> { &pcie_reset_flr, .name = "flr" },
> { &pci_af_flr, .name = "af_flr" },
> { &pci_pm_reset, .name = "pm" },
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 990b73e90..2c12017ed 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -705,7 +705,13 @@ static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL
> int pci_acpi_program_hp_params(struct pci_dev *dev);
> extern const struct attribute_group pci_dev_acpi_attr_group;
> void pci_set_acpi_fwnode(struct pci_dev *dev);
> +int pci_dev_acpi_reset(struct pci_dev *dev, int probe);
> #else
> +static inline int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
> +{
> + return -ENOTTY;
> +}
> +
> static inline void pci_set_acpi_fwnode(struct pci_dev *dev) {}
> static inline int pci_acpi_program_hp_params(struct pci_dev *dev)
> {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index f34563831..c3b0d771c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -50,7 +50,7 @@
> PCI_STATUS_PARITY)
>
> /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> -#define PCI_NUM_RESET_METHODS 6
> +#define PCI_NUM_RESET_METHODS 7
>
> /*
> * The PCI interface treats multi-function devices as independent

2021-07-12 23:11:22

[permalink] [raw]

Subject: Re: [PATCH v10 6/8] PCI: Setup ACPI fwnode early and at the same time with OF

On Fri, 9 Jul 2021 18:08:11 +0530
Amey Narkhede <[email protected]> wrote:

> From: Shanker Donthineni <[email protected]>
>
> The pci_dev objects are created through two mechanisms 1) during PCI
> bus scan and 2) from I/O Virtualization. The fwnode in pci_dev object
> is being set at different places depends on the type of firmware used,
> device creation mechanism, and acpi_pci_bridge_d3() WAR.
>
> The software features which have a dependency on ACPI fwnode properties
> and need to be handled before device_add() will not work. One use case,
> the software has to check the existence of _RST method to support ACPI
> based reset method.
>
> This patch does the two changes in order to provide fwnode consistently.
> - Set ACPI and OF fwnodes from pci_setup_device().
> - Remove pci_set_acpi_fwnode() in acpi_pci_bridge_d3().
>
> After this patch, ACPI/OF firmware properties are visible at the same
> time during the early stage of pci_dev setup. And also call sites should
> be able to use firmware agnostic functions device_property_xxx() for the
> early PCI quirks in the future.
>
> Signed-off-by: Shanker Donthineni <[email protected]>
> ---
> drivers/pci/pci-acpi.c | 1 -
> drivers/pci/probe.c | 7 ++++---
> 2 files changed, 4 insertions(+), 4 deletions(-)

Looks ok to me.

Reviewed-by: Alex Williamson <[email protected]>

2021-07-13 00:53:00

[permalink] [raw]

Subject: Re: [PATCH v10 7/8] PCI: Add support for ACPI _RST reset method

Hi Alex,

On 7/12/21 6:09 PM, Alex Williamson wrote:
>> +/**
>> + * pci_dev_acpi_reset - do a function level reset using _RST method
>> + * @dev: device to reset
>> + * @probe: check if _RST method is included in the acpi_device context.
>> + */
>> +int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
>> +{
>> + acpi_handle handle = ACPI_HANDLE(&dev->dev);
>> +
>> + if (!handle || !acpi_has_method(handle, "_RST"))
>> + return -ENOTTY;
>> +
>> + if (probe)
>> + return 0;
>> +
>> + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
>> + pci_warn(dev, "ACPI _RST failed\n");
>> + return -EINVAL;
> Should we return -ENOTTY here instead to give a possible secondary
> reset method a chance? Thanks,
Thanks for reviewing patches.

ACPI/AML _RST method type is VOID. The only possibility of failure would be
either system is running out of memory or bugs in ACPICA. There is no strong
reason not to return -NOTTY.

I'll fix in the next version. Is there opportunity to include reset feature in v5.14-rc2?

Can I add your reviewed-by since no other comments to this patch?

-Shanker

2021-07-15 00:23:11

[permalink] [raw]

Subject: Re: [PATCH v10 7/8] PCI: Add support for ACPI _RST reset method

On Mon, 12 Jul 2021 19:51:41 -0500
Shanker R Donthineni <[email protected]> wrote:

> Hi Alex,
>
> On 7/12/21 6:09 PM, Alex Williamson wrote:
> >> +/**
> >> + * pci_dev_acpi_reset - do a function level reset using _RST method
> >> + * @dev: device to reset
> >> + * @probe: check if _RST method is included in the acpi_device context.
> >> + */
> >> +int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
> >> +{
> >> + acpi_handle handle = ACPI_HANDLE(&dev->dev);
> >> +
> >> + if (!handle || !acpi_has_method(handle, "_RST"))
> >> + return -ENOTTY;
> >> +
> >> + if (probe)
> >> + return 0;
> >> +
> >> + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
> >> + pci_warn(dev, "ACPI _RST failed\n");
> >> + return -EINVAL;
> > Should we return -ENOTTY here instead to give a possible secondary
> > reset method a chance? Thanks,
> Thanks for reviewing patches.
>
> ACPI/AML _RST method type is VOID. The only possibility of failure would be
> either system is running out of memory or bugs in ACPICA. There is no strong
> reason not to return -NOTTY.
>
> I'll fix in the next version. Is there opportunity to include reset feature in v5.14-rc2?

Sounds good, it's a corner case but since we've got a series of methods
we can try and part of the point of Amey's series is giving the user
control of the order and methods to try, we might as well make use of
it. I think there's also some precedence in the quirks that they can
fail and fall through to standard resets.

I'll leave any upstream timing questions to Bjorn, but we've passed the
v5.14 merge window when new functionality is generally accepted.

> Can I add your reviewed-by since no other comments to this patch?

Yeah, s/-EINVAL/-ENOTTY/

Reviewed-by: Alex Williamson <[email protected]>

Thanks!

2021-07-27 22:14:33

[permalink] [raw]

Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

On Fri, Jul 09, 2021 at 06:08:06PM +0530, Amey Narkhede wrote:
> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
> FLR to avoid reading PCI_EXP_DEVCAP multiple times.
>
> Currently there is separate function pcie_has_flr() to probe if PCIe FLR
> is supported by the device which does not match the calling convention
> followed by reset methods which use second function argument to decide
> whether to probe or not. Add new function pcie_reset_flr() that follows
> the calling convention of reset methods.
>
> Signed-off-by: Amey Narkhede <[email protected]>
> ---
> drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
> drivers/pci/pci.c | 59 +++++++++++-----------
> drivers/pci/pcie/aer.c | 12 ++---
> drivers/pci/probe.c | 6 ++-
> drivers/pci/quirks.c | 9 ++--
> include/linux/pci.h | 3 +-
> 6 files changed, 45 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
> index facc8e6bc..15d6c8452 100644
> --- a/drivers/crypto/cavium/nitrox/nitrox_main.c
> +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
> @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
> return -ENOMEM;
> }
>
> - /* check flr support */
> - if (pcie_has_flr(pdev))
> - pcie_flr(pdev);
> + pcie_reset_flr(pdev, 0);

I'm not really a fan of exposing the "probe" argument outside
drivers/pci/. I think this would be the only occurrence. Is there a
way to avoid that?

Can we just make pcie_flr() do the equivalent of pcie_has_flr()
internally?

> static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
> {
> - if (!pcie_has_flr(dev))
> - return -ENOTTY;
> + int ret = pcie_reset_flr(dev, probe);
>
> if (probe)
> - return 0;
> -
> - pcie_flr(dev);
> + return ret;
>
> msleep(250);

Can we structure this like the following? I think it's easier to
understand.

if (probe)
return pcie_reset_flr(dev, 1);

pcie_reset_flr(dev, 0);
msleep(250);
return 0;

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c20211e59..d432428fd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -337,6 +337,7 @@ struct pci_dev {
> u8 msi_cap; /* MSI capability offset */
> u8 msix_cap; /* MSI-X capability offset */
> u8 pcie_mpss:3; /* PCIe Max Payload Size Supported */
> + u8 has_pcie_flr:1; /* PCIe FLR supported */

Let's add a devcap member instead. Then we can use it for some
ASPM-related things as well. We *could* use it to replace pcie_mpss,
since that comes from PCI_EXP_DEVCAP, too, but for now I think it's
easier to just keep it because it's encoded, and some drivers and
quirks use it so it would be a fair amount of work to change that.
Example patch below that could become the first in the series.

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aacf575c15cf..5a99061ea53a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4635,8 +4635,7 @@ bool pcie_has_flr(struct pci_dev *dev)
if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
return false;

- pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
- return cap & PCI_EXP_DEVCAP_FLR;
+ return dev->devcap & PCI_EXP_DEVCAP_FLR;
}
EXPORT_SYMBOL_GPL(pcie_has_flr);

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 79177ac37880..52ae26bcc68c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1498,8 +1498,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
pdev->pcie_cap = pos;
pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
pdev->pcie_flags_reg = reg16;
- pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
- pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
+ pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
+ pdev->pcie_mpss = pdev->devcap & PCI_EXP_DEVCAP_PAYLOAD;

parent = pci_upstream_bridge(pdev);
if (!parent)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 540b377ca8f6..294d1c857a57 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -334,6 +334,7 @@ struct pci_dev {
struct pci_dev *rcec; /* Associated RCEC device */
#endif
u8 pcie_cap; /* PCIe capability offset */
+ u32 devcap; /* PCIe Device Capabilities */
u8 msi_cap; /* MSI capability offset */
u8 msix_cap; /* MSI-X capability offset */
u8 pcie_mpss:3; /* PCIe Max Payload Size Supported */

2021-07-27 22:24:57

[permalink] [raw]

Subject: Re: [PATCH v10 8/8] PCI: Change the type of probe argument in reset functions

On Fri, Jul 09, 2021 at 06:08:13PM +0530, Amey Narkhede wrote:
> Introduce a new enum pci_reset_mode_t to make the context of probe argument
> in reset functions clear and the code easier to read. Change the type of
> probe argument in functions which implement reset methods from int to
> pci_reset_mode_t to make the intent clear.

Not sure adding an enum and a PCI_RESET_MODE_MAX seems worth it to me.
It's really a boolean parameter, and I'd be happy to change it to a
bool. But I don't think it's worth checking against
PCI_RESET_MODE_MAX unless we need more than two options.

> Add a new line in return statement of pci_reset_bus_function().

Drop this line, since I don't think it's correct and there's no need
to describe whitespace changes anyway.

> Suggested-by: Alex Williamson <[email protected]>
> Suggested-by: Krzysztof Wilczyński <[email protected]>
> Signed-off-by: Amey Narkhede <[email protected]>

> @@ -306,7 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
> return -ENOMEM;
> }
>
> - pcie_reset_flr(pdev, 0);
> + pcie_reset_flr(pdev, PCI_RESET_DO_RESET);
>
> pci_restore_state(pdev);
>

> @@ -526,7 +526,7 @@ static void octeon_destroy_resources(struct octeon_device *oct)
> oct->irq_name_storage = NULL;
> }
> /* Soft reset the octeon device before exiting */
> - if (!pcie_reset_flr(oct->pci_dev, 1))
> + if (!pcie_reset_flr(oct->pci_dev, PCI_RESET_PROBE))
> octeon_pci_flr(oct);
> else
> cn23xx_vf_ask_pf_to_do_flr(oct);

> +typedef enum pci_reset_mode {
> + PCI_RESET_DO_RESET,
> + PCI_RESET_PROBE,
> + PCI_RESET_MODE_MAX,
> +} pci_reset_mode_t;

2021-07-27 23:01:22

[permalink] [raw]

Subject: Re: [PATCH v10 2/8] PCI: Add new array for keeping track of ordering of reset methods

On Fri, Jul 09, 2021 at 06:08:07PM +0530, Amey Narkhede wrote:
> Introduce a new array reset_methods in struct pci_dev to keep track of
> reset mechanisms supported by the device and their ordering.
>
> Also refactor probing and reset functions to take advantage of calling
> convention of reset functions.
>
> Co-developed-by: Alex Williamson <[email protected]>
> Signed-off-by: Alex Williamson <[email protected]>
> Signed-off-by: Amey Narkhede <[email protected]>
> ---
> drivers/pci/pci.c | 92 ++++++++++++++++++++++++++-------------------
> drivers/pci/pci.h | 9 ++++-
> drivers/pci/probe.c | 5 +--
> include/linux/pci.h | 7 ++++
> 4 files changed, 70 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index fefa6d7b3..42440cb10 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -72,6 +72,14 @@ static void pci_dev_d3_sleep(struct pci_dev *dev)
> msleep(delay);
> }
>
> +int pci_reset_supported(struct pci_dev *dev)
> +{
> + u8 null_reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
> +
> + return memcmp(null_reset_methods,
> + dev->reset_methods, sizeof(null_reset_methods));

I think "return dev->reset_methods[0] != 0;" is sufficient, isn't it?

> +}
> +
> #ifdef CONFIG_PCI_DOMAINS
> int pci_domains_supported = 1;
> #endif
> @@ -5104,6 +5112,15 @@ static void pci_dev_restore(struct pci_dev *dev)
> err_handler->reset_done(dev);
> }
>
> +const struct pci_reset_fn_method pci_reset_fn_methods[] = {
> + { },
> + { &pci_dev_specific_reset, .name = "device_specific" },
> + { &pcie_reset_flr, .name = "flr" },
> + { &pci_af_flr, .name = "af_flr" },
> + { &pci_pm_reset, .name = "pm" },
> + { &pci_reset_bus_function, .name = "bus" },
> +};

No need for "&" before the function names.

This should be static until it's needed outside this file. You can
remove the "static" in the patch that adds the use in another file.

Looks like the only use is in pci-sysfs.c. I think it might be better
to move the pci_dev_reset_method_attr_group and related stuff here to
pci.c and add an extern for it, as is done for aspm_ctrl_attr_group.

I know there's some trick that relies on the first element being
empty, and it's probably worth a one-line comment there to point it
out. Maybe something like this?

/* dev->reset_methods[] is a 0-terminated list of indices into this */

> /**
> * __pci_reset_function_locked - reset a PCI device function while holding
> * the @dev mutex lock.
> @@ -5126,65 +5143,62 @@ static void pci_dev_restore(struct pci_dev *dev)
> */
> int __pci_reset_function_locked(struct pci_dev *dev)
> {
> - int rc;
> + int i, m, rc = -ENOTTY;
>
> might_sleep();
>
> /*
> - * A reset method returns -ENOTTY if it doesn't support this device
> - * and we should try the next method.
> + * A reset method returns -ENOTTY if it doesn't support this device and
> + * we should try the next method.
> *
> - * If it returns 0 (success), we're finished. If it returns any
> - * other error, we're also finished: this indicates that further
> - * reset mechanisms might be broken on the device.
> + * If it returns 0 (success), we're finished. If it returns any other
> + * error, we're also finished: this indicates that further reset
> + * mechanisms might be broken on the device.
> */
> - rc = pci_dev_specific_reset(dev, 0);
> - if (rc != -ENOTTY)
> - return rc;
> - rc = pcie_reset_flr(dev, 0);
> - if (rc != -ENOTTY)
> - return rc;
> - rc = pci_af_flr(dev, 0);
> - if (rc != -ENOTTY)
> - return rc;
> - rc = pci_pm_reset(dev, 0);
> - if (rc != -ENOTTY)
> - return rc;
> - return pci_reset_bus_function(dev, 0);
> + for (i = 0; i < PCI_NUM_RESET_METHODS && (m = dev->reset_methods[i]); i++) {

I'm not a huge fan of using assignments as a loop condition because
it's a little subtle. Is there a way to restructure this, e.g.,

for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
m = dev->reset_methods[i];
if (!m)
return -ENOTTY;

rc = pci_reset_fn_methods[m].reset_fn(dev, 0);

> + rc = pci_reset_fn_methods[m].reset_fn(dev, 0);
> + if (!rc)
> + return 0;
> + if (rc != -ENOTTY)
> + return rc;
> + }
> +
> + return -ENOTTY;
> }
> EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
>
> /**
> - * pci_probe_reset_function - check whether the device can be safely reset
> - * @dev: PCI device to reset
> + * pci_init_reset_methods - check whether device can be safely reset
> + * and store supported reset mechanisms.
> + * @dev: PCI device to check for reset mechanisms
> *
> * Some devices allow an individual function to be reset without affecting
> * other functions in the same device. The PCI device must be responsive
> - * to PCI config space in order to use this function.
> + * to reads and writes to its PCI config space in order to use this function.

Since you're reworking this comment anyway, maybe update the "must be
responsive to PCI config space" comment to explicitly say the device
must be in D0-D3hot.

> - * Returns 0 if the device function can be reset or negative if the
> - * device doesn't support resetting a single function.
> + * Stores reset mechanisms supported by device in reset_methods byte array
> + * which is a member of struct pci_dev.
> */
> -int pci_probe_reset_function(struct pci_dev *dev)
> +void pci_init_reset_methods(struct pci_dev *dev)
> {
> - int rc;
> + int i, n, rc;
> + u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
> +
> + n = 0;

Move this init down to just before the loop that uses it.

> + BUILD_BUG_ON(ARRAY_SIZE(pci_reset_fn_methods) != PCI_NUM_RESET_METHODS);
>
> might_sleep();
>
> - rc = pci_dev_specific_reset(dev, 1);
> - if (rc != -ENOTTY)
> - return rc;
> - rc = pcie_reset_flr(dev, 1);
> - if (rc != -ENOTTY)
> - return rc;
> - rc = pci_af_flr(dev, 1);
> - if (rc != -ENOTTY)
> - return rc;
> - rc = pci_pm_reset(dev, 1);
> - if (rc != -ENOTTY)
> - return rc;
> + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
> + rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
> + if (!rc)
> + reset_methods[n++] = i;

Why do we need this local reset_methods[] array? Can we just fill
in dev->reset_methods[] directly and skip the memcpy() below?

> + else if (rc != -ENOTTY)
> + break;
> + }
>
> - return pci_reset_bus_function(dev, 1);
> + memcpy(dev->reset_methods, reset_methods, sizeof(reset_methods));
> }
>
> /**
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 37c913bbc..db1ad94e7 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -33,7 +33,8 @@ enum pci_mmap_api {
> int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
> enum pci_mmap_api mmap_api);
>
> -int pci_probe_reset_function(struct pci_dev *dev);
> +int pci_reset_supported(struct pci_dev *dev);
> +void pci_init_reset_methods(struct pci_dev *dev);
> int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
> int pci_bus_error_reset(struct pci_dev *dev);
>
> @@ -606,6 +607,12 @@ struct pci_dev_reset_methods {
> int (*reset)(struct pci_dev *dev, int probe);
> };
>
> +struct pci_reset_fn_method {
> + int (*reset_fn)(struct pci_dev *pdev, int probe);
> + char *name;
> +};
> +
> +extern const struct pci_reset_fn_method pci_reset_fn_methods[];
> #ifdef CONFIG_PCI_QUIRKS
> int pci_dev_specific_reset(struct pci_dev *dev, int probe);
> #else
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 072a3d4dc..bc4af914a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2406,9 +2406,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
> pci_rcec_init(dev); /* Root Complex Event Collector */
>
> pcie_report_downtraining(dev);
> -
> - if (pci_probe_reset_function(dev) == 0)
> - dev->reset_fn = 1;
> + pci_init_reset_methods(dev);
> + dev->reset_fn = pci_reset_supported(dev);
> }
>
> /*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d432428fd..9f3e85f33 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -49,6 +49,9 @@
> PCI_STATUS_SIG_TARGET_ABORT | \
> PCI_STATUS_PARITY)
>
> +/* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> +#define PCI_NUM_RESET_METHODS 6
> +
> /*
> * The PCI interface treats multi-function devices as independent
> * devices. The slot/function address of each device is encoded
> @@ -506,6 +509,10 @@ struct pci_dev {
> char *driver_override; /* Driver name to force a match */
>
> unsigned long priv_flags; /* Private flags for the PCI driver */
> + /*
> + * See pci_reset_fn_methods array in pci.c for ordering.
> + */
> + u8 reset_methods[PCI_NUM_RESET_METHODS]; /* Reset methods ordered by priority */
> };
>
> static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> --
> 2.32.0
>

2021-07-27 23:31:04

[permalink] [raw]

Subject: Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

On Fri, Jul 09, 2021 at 06:08:09PM +0530, Amey Narkhede wrote:
> Add reset_method sysfs attribute to enable user to query and set user
> preferred device reset methods and their ordering.
>
> Co-developed-by: Alex Williamson <[email protected]>
> Signed-off-by: Alex Williamson <[email protected]>
> Signed-off-by: Amey Narkhede <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-pci | 19 +++++
> drivers/pci/pci-sysfs.c | 103 ++++++++++++++++++++++++
> 2 files changed, 122 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index ef00fada2..43f4e33c7 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -121,6 +121,25 @@ Description:
> child buses, and re-discover devices removed earlier
> from this part of the device tree.
>
> +What: /sys/bus/pci/devices/.../reset_method
> +Date: March 2021
> +Contact: Amey Narkhede <[email protected]>
> +Description:
> + Some devices allow an individual function to be reset
> + without affecting other functions in the same slot.
> +
> + For devices that have this support, a file named
> + reset_method will be present in sysfs. Initially reading
> + this file will give names of the device supported reset
> + methods and their ordering. After write, this file will
> + give names and ordering of currently enabled reset methods.
> + Writing the name or comma separated list of names of any of
> + the device supported reset methods to this file will set
> + the reset methods and their ordering to be used when
> + resetting the device. Writing empty string to this file
> + will disable ability to reset the device and writing
> + "default" will return to the original value.
> +
> What: /sys/bus/pci/devices/.../reset
> Date: July 2009
> Contact: Michael S. Tsirkin <[email protected]>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 316f70c3e..8a740e211 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1334,6 +1334,108 @@ static const struct attribute_group pci_dev_rom_attr_group = {
> .is_bin_visible = pci_dev_rom_attr_is_visible,
> };
>
> +static ssize_t reset_method_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + ssize_t len = 0;
> + int i, idx;
> +
> + for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
> + idx = pdev->reset_methods[i];
> + if (!idx)

I know it's totally OCD, but can you use the same "m" for indexing
pci_reset_fn_methods[] that you used in __pci_reset_function_locked()
and ... oops, I meant to say pci_init_reset_methods(), but there you
used "i". Maybe pci_init_reset_methods() could use "m" as well, and
maybe it could use "i" instead of "n" to index dev->reset_methods[]?

> + break;
> +
> + len += sysfs_emit_at(buf, len, "%s%s", len ? "," : "",
> + pci_reset_fn_methods[idx].name);

Maybe separate with spaces instead of commas? E.g.,

device_specific flr pm bus

instead of

device_specific,flr,pm,bus

I think the former is less error-prone when parsing. Easier to split
in a shell script, for example.

> + }
> +
> + if (len)
> + len += sysfs_emit_at(buf, len, "\n");
> +
> + return len;
> +}
> +
> +static ssize_t reset_method_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + int n = 0;
> + char *name, *options = NULL;
> + u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
> +
> + if (count >= (PAGE_SIZE - 1))
> + return -EINVAL;

I'm not the sysfs expert, but surely the sysfs infrastructure already
guarantees this?

> +
> + if (sysfs_streq(buf, "")) {
> + pci_warn(pdev, "All device reset methods disabled by user");

Can we just do:

pdev->reset_methods[0] = 0;

here and dispense with the memcpy()?

> + goto set_reset_methods;
> + }
> +
> + if (sysfs_streq(buf, "default")) {
> + pci_init_reset_methods(pdev);
> + return count;
> + }
> +
> + options = kstrndup(buf, count, GFP_KERNEL);

I assume the kstrndup() is because strsep() writes into the buffer?
Aren't we allowed to write into the buffer we get from sysfs? Does
the user ever see the buffer contents again? I would think sysfs
would have already done a copy_from_user() or whatever.

> + if (!options)
> + return -ENOMEM;
> +
> + while ((name = strsep(&options, ",")) != NULL) {

*If* you change the show to use spaces, obviously this would have to
change as well (as well as the doc).

> + int i;
> +
> + if (sysfs_streq(name, ""))
> + continue;
> +
> + name = strim(name);
> +
> + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
> + if (sysfs_streq(name, pci_reset_fn_methods[i].name) &&
> + !pci_reset_fn_methods[i].reset_fn(pdev, 1)) {
> + reset_methods[n++] = i;

Can we build this directly in pdev->reset_methods[] so we don't need
the memcpy() below?

> + break;
> + }
> + }

Same "m" for indexing pci_reset_fn_methods[], "i" for
pdev->reset_methods[] comment here.

> + if (i == PCI_NUM_RESET_METHODS) {
> + kfree(options);
> + return -EINVAL;
> + }
> + }
> +
> + if (!pci_reset_fn_methods[1].reset_fn(pdev, 1) && reset_methods[0] != 1)
> + pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");

Hmmm. I sort of see the point here, but I wish we didn't have the
implicit dependency on pci_reset_fn_methods[1] being
pci_dev_specific_reset().

I know we've talked about this before. I'm still not 100% sure either
of these warnings is worthwhile, especially since we're not *using*
the reset here. It might be useful at the point where we try to *do*
a reset. I dunno. Maybe this is the best place since this is where
the user potentially screwed up.

> +set_reset_methods:
> + memcpy(pdev->reset_methods, reset_methods, sizeof(reset_methods));
> + kfree(options);
> + return count;
> +}
> +static DEVICE_ATTR_RW(reset_method);
> +
> +static struct attribute *pci_dev_reset_method_attrs[] = {
> + &dev_attr_reset_method.attr,
> + NULL,
> +};
> +
> +static umode_t pci_dev_reset_method_attr_is_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> +
> + if (!pci_reset_supported(pdev))
> + return 0;
> +
> + return a->mode;
> +}
> +
> +static const struct attribute_group pci_dev_reset_method_attr_group = {
> + .attrs = pci_dev_reset_method_attrs,
> + .is_visible = pci_dev_reset_method_attr_is_visible,
> +};
> +
> static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> @@ -1491,6 +1593,7 @@ const struct attribute_group *pci_dev_groups[] = {
> &pci_dev_config_attr_group,
> &pci_dev_rom_attr_group,
> &pci_dev_reset_attr_group,
> + &pci_dev_reset_method_attr_group,
> &pci_dev_vpd_attr_group,
> #ifdef CONFIG_DMI
> &pci_dev_smbios_attr_group,
> --
> 2.32.0
>

2021-07-27 23:34:14

[permalink] [raw]

Subject: Re: [PATCH v10 6/8] PCI: Setup ACPI fwnode early and at the same time with OF

On Fri, Jul 09, 2021 at 06:08:11PM +0530, Amey Narkhede wrote:
> From: Shanker Donthineni <[email protected]>
>
> The pci_dev objects are created through two mechanisms 1) during PCI
> bus scan and 2) from I/O Virtualization. The fwnode in pci_dev object
> is being set at different places depends on the type of firmware used,
> device creation mechanism, and acpi_pci_bridge_d3() WAR.

WAR?

> The software features which have a dependency on ACPI fwnode properties
> and need to be handled before device_add() will not work. One use case,
> the software has to check the existence of _RST method to support ACPI
> based reset method.
>
> This patch does the two changes in order to provide fwnode consistently.
> - Set ACPI and OF fwnodes from pci_setup_device().
> - Remove pci_set_acpi_fwnode() in acpi_pci_bridge_d3().
>
> After this patch, ACPI/OF firmware properties are visible at the same
> time during the early stage of pci_dev setup. And also call sites should
> be able to use firmware agnostic functions device_property_xxx() for the
> early PCI quirks in the future.
>
> Signed-off-by: Shanker Donthineni <[email protected]>
> ---
> drivers/pci/pci-acpi.c | 1 -
> drivers/pci/probe.c | 7 ++++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index eaddbf701..dae021322 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev)
> return false;
>
> /* Assume D3 support if the bridge is power-manageable by ACPI. */
> - pci_set_acpi_fwnode(dev);
> adev = ACPI_COMPANION(&dev->dev);
>
> if (adev && acpi_device_power_manageable(adev))
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index c272e23db..c911d6a5c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1790,6 +1790,9 @@ int pci_setup_device(struct pci_dev *dev)
> dev->error_state = pci_channel_io_normal;
> set_pcie_port_type(dev);
>
> + pci_set_of_node(dev);
> + pci_set_acpi_fwnode(dev);
> +
> pci_dev_assign_slot(dev);
>
> /*
> @@ -1925,6 +1928,7 @@ int pci_setup_device(struct pci_dev *dev)
> default: /* unknown header */
> pci_err(dev, "unknown header type %02x, ignoring device\n",
> dev->hdr_type);
> + pci_release_of_node(dev);
> return -EIO;
>
> bad:
> @@ -2352,10 +2356,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
> dev->vendor = l & 0xffff;
> dev->device = (l >> 16) & 0xffff;
>
> - pci_set_of_node(dev);
> -
> if (pci_setup_device(dev)) {
> - pci_release_of_node(dev);
> pci_bus_put(dev->bus);
> kfree(dev);
> return NULL;
> --
> 2.32.0
>

2021-07-27 23:53:20

[permalink] [raw]

Subject: Re: [PATCH v10 6/8] PCI: Setup ACPI fwnode early and at the same time with OF

Hi Bjorn

On 7/27/21 6:30 PM, Bjorn Helgaas wrote:
>> From: Shanker Donthineni <[email protected]>
>>
>> The pci_dev objects are created through two mechanisms 1) during PCI
>> bus scan and 2) from I/O Virtualization. The fwnode in pci_dev object
>> is being set at different places depends on the type of firmware used,
>> device creation mechanism, and acpi_pci_bridge_d3() WAR.
> WAR?
>
Thanks for catching a mistake, will remove it in the next patch.

-Shanker

2021-07-28 01:29:39

by Krzysztof Wilczyński

[permalink] [raw]

Subject: Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

Hi Bjorn,

[...]
> > + if (count >= (PAGE_SIZE - 1))
> > + return -EINVAL;
>
> I'm not the sysfs expert, but surely the sysfs infrastructure already
> guarantees this?

We don't need to store any value, since we are processing the input from
the userspace, thus ensuring that we have room for the newline is not
needed, especially since the show() function dynamically builds the
content to show, so indeed this check can be dropped.

To add, there aren't any guarantees other from sysfs than we get a up to
a PAGE_SIZE worth of data in the buffer.

[...]
> > + options = kstrndup(buf, count, GFP_KERNEL);
>
> I assume the kstrndup() is because strsep() writes into the buffer?

Yes, Amey added kstrndup() in v6 following my recommendation as per:

https://lore.kernel.org/linux-pci/[email protected]/

This was to avoid removing the const quantifier through a type cast
given that the signature of the function denotes that the buffer is
a pointer to immutable string, as per:

https://elixir.bootlin.com/linux/v5.14-rc3/source/include/linux/device/driver.h#L137

Some other sysfs users do employ the cast when using strtok() and I am
not so such it's the right way to do it, as per:

drivers/s390/net/qeth_l3_sys.c
232: tmp = strsep((char **)&buf, "\n");

drivers/media/rc/rc-main.c
1167: while ((tmp = strsep((char **)&buf, " \n")) != NULL) {

> Aren't we allowed to write into the buffer we get from sysfs? Does
> the user ever see the buffer contents again? I would think sysfs
> would have already done a copy_from_user() or whatever.

I might be wrong about this, but I suppose this might be to stop people
from accidentally freeing the buffer as kernfs_fop_write_iter() would do
it after all the internal housekeeping is done, provided that someone
pays attention to compile time warnings.

Krzysztof

2021-07-28 15:37:50

[permalink] [raw]

Subject: Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

On Wed, Jul 28, 2021 at 03:27:40AM +0200, Krzysztof Wilczyński wrote:

> > > + options = kstrndup(buf, count, GFP_KERNEL);
> >
> > I assume the kstrndup() is because strsep() writes into the buffer?
>
> Yes, Amey added kstrndup() in v6 following my recommendation as per:
>
> https://lore.kernel.org/linux-pci/[email protected]/
>
> This was to avoid removing the const quantifier through a type cast
> given that the signature of the function denotes that the buffer is
> a pointer to immutable string, as per:
>
> https://elixir.bootlin.com/linux/v5.14-rc3/source/include/linux/device/driver.h#L137

Ah, right, thanks! Definitely prefer not to cast away the constness.

I guess the strings here are short (<100 chars max), so no big deal to
duplicate them. Sorry for the noise!

2021-07-28 17:12:28

[permalink] [raw]

Subject: Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

[+cc Serge, linux-security-module: should we check CAP_SYS_ADMIN or
similar for changing PCI reset mechanisms for a device?]

On Fri, Jul 09, 2021 at 06:08:09PM +0530, Amey Narkhede wrote:
> Add reset_method sysfs attribute to enable user to query and set user
> preferred device reset methods and their ordering.
>
> Co-developed-by: Alex Williamson <[email protected]>
> Signed-off-by: Alex Williamson <[email protected]>
> Signed-off-by: Amey Narkhede <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-pci | 19 +++++
> drivers/pci/pci-sysfs.c | 103 ++++++++++++++++++++++++
> 2 files changed, 122 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index ef00fada2..43f4e33c7 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -121,6 +121,25 @@ Description:
> child buses, and re-discover devices removed earlier
> from this part of the device tree.
>
> +What: /sys/bus/pci/devices/.../reset_method
> +Date: March 2021
> +Contact: Amey Narkhede <[email protected]>
> +Description:
> + Some devices allow an individual function to be reset
> + without affecting other functions in the same slot.
> +
> + For devices that have this support, a file named
> + reset_method will be present in sysfs. Initially reading
> + this file will give names of the device supported reset
> + methods and their ordering. After write, this file will
> + give names and ordering of currently enabled reset methods.
> + Writing the name or comma separated list of names of any of
> + the device supported reset methods to this file will set
> + the reset methods and their ordering to be used when
> + resetting the device. Writing empty string to this file
> + will disable ability to reset the device and writing
> + "default" will return to the original value.
> +
> What: /sys/bus/pci/devices/.../reset
> Date: July 2009
> Contact: Michael S. Tsirkin <[email protected]>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 316f70c3e..8a740e211 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1334,6 +1334,108 @@ static const struct attribute_group pci_dev_rom_attr_group = {
> .is_bin_visible = pci_dev_rom_attr_is_visible,
> };
>
> +static ssize_t reset_method_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + ssize_t len = 0;
> + int i, idx;
> +
> + for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
> + idx = pdev->reset_methods[i];
> + if (!idx)
> + break;
> +
> + len += sysfs_emit_at(buf, len, "%s%s", len ? "," : "",
> + pci_reset_fn_methods[idx].name);
> + }
> +
> + if (len)
> + len += sysfs_emit_at(buf, len, "\n");
> +
> + return len;
> +}
> +
> +static ssize_t reset_method_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + int n = 0;
> + char *name, *options = NULL;
> + u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };

Should this check "capable(CAP_SYS_ADMIN)" or similar? The sysfs file
is mode 0644, so writable only by root.

I do note that Documentation/process/adding-syscalls.rst suggests
"avoid adding new uses of the already overly-general CAP_SYS_ADMIN
capability." But CAP_SYS_ADMIN is used for all the other checks in
pci-sysfs.c.

> + if (count >= (PAGE_SIZE - 1))
> + return -EINVAL;
> +
> + if (sysfs_streq(buf, "")) {
> + pci_warn(pdev, "All device reset methods disabled by user");
> + goto set_reset_methods;
> + }
> +
> + if (sysfs_streq(buf, "default")) {
> + pci_init_reset_methods(pdev);
> + return count;
> + }
> +
> + options = kstrndup(buf, count, GFP_KERNEL);
> + if (!options)
> + return -ENOMEM;
> +
> + while ((name = strsep(&options, ",")) != NULL) {
> + int i;
> +
> + if (sysfs_streq(name, ""))
> + continue;
> +
> + name = strim(name);
> +
> + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
> + if (sysfs_streq(name, pci_reset_fn_methods[i].name) &&
> + !pci_reset_fn_methods[i].reset_fn(pdev, 1)) {
> + reset_methods[n++] = i;
> + break;
> + }
> + }
> +
> + if (i == PCI_NUM_RESET_METHODS) {
> + kfree(options);
> + return -EINVAL;
> + }
> + }
> +
> + if (!pci_reset_fn_methods[1].reset_fn(pdev, 1) && reset_methods[0] != 1)
> + pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");
> +
> +set_reset_methods:
> + memcpy(pdev->reset_methods, reset_methods, sizeof(reset_methods));
> + kfree(options);
> + return count;
> +}
> +static DEVICE_ATTR_RW(reset_method);
> +
> +static struct attribute *pci_dev_reset_method_attrs[] = {
> + &dev_attr_reset_method.attr,
> + NULL,
> +};
> +
> +static umode_t pci_dev_reset_method_attr_is_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> +
> + if (!pci_reset_supported(pdev))
> + return 0;
> +
> + return a->mode;
> +}
> +
> +static const struct attribute_group pci_dev_reset_method_attr_group = {
> + .attrs = pci_dev_reset_method_attrs,
> + .is_visible = pci_dev_reset_method_attr_is_visible,
> +};
> +
> static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> @@ -1491,6 +1593,7 @@ const struct attribute_group *pci_dev_groups[] = {
> &pci_dev_config_attr_group,
> &pci_dev_rom_attr_group,
> &pci_dev_reset_attr_group,
> + &pci_dev_reset_method_attr_group,
> &pci_dev_vpd_attr_group,
> #ifdef CONFIG_DMI
> &pci_dev_smbios_attr_group,
> --
> 2.32.0
>

2021-07-28 17:37:40

[permalink] [raw]

Subject: Re: [PATCH v10 8/8] PCI: Change the type of probe argument in reset functions

On 21/07/27 05:22PM, Bjorn Helgaas wrote:
> On Fri, Jul 09, 2021 at 06:08:13PM +0530, Amey Narkhede wrote:
> > Introduce a new enum pci_reset_mode_t to make the context of probe argument
> > in reset functions clear and the code easier to read. Change the type of
> > probe argument in functions which implement reset methods from int to
> > pci_reset_mode_t to make the intent clear.
>
> Not sure adding an enum and a PCI_RESET_MODE_MAX seems worth it to me.
> It's really a boolean parameter, and I'd be happy to change it to a
> bool. But I don't think it's worth checking against
> PCI_RESET_MODE_MAX unless we need more than two options.
>
Is it okay to use PCI_RESET_PROBE and PCI_RESET_DO_RESET as bool.
That would be less confusing than directly using true/false.

Thanks,
Amey

> > Add a new line in return statement of pci_reset_bus_function().
>
> Drop this line, since I don't think it's correct and there's no need
> to describe whitespace changes anyway.
>
> > Suggested-by: Alex Williamson <[email protected]>
> > Suggested-by: Krzysztof Wilczyński <[email protected]>
> > Signed-off-by: Amey Narkhede <[email protected]>
>
> > @@ -306,7 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
> > return -ENOMEM;
> > }
> >
> > - pcie_reset_flr(pdev, 0);
> > + pcie_reset_flr(pdev, PCI_RESET_DO_RESET);
> >
> > pci_restore_state(pdev);
> >
>
> > @@ -526,7 +526,7 @@ static void octeon_destroy_resources(struct octeon_device *oct)
> > oct->irq_name_storage = NULL;
> > }
> > /* Soft reset the octeon device before exiting */
> > - if (!pcie_reset_flr(oct->pci_dev, 1))
> > + if (!pcie_reset_flr(oct->pci_dev, PCI_RESET_PROBE))
> > octeon_pci_flr(oct);
> > else
> > cn23xx_vf_ask_pf_to_do_flr(oct);
>
> > +typedef enum pci_reset_mode {
> > + PCI_RESET_DO_RESET,
> > + PCI_RESET_PROBE,
> > + PCI_RESET_MODE_MAX,
> > +} pci_reset_mode_t;

2021-07-28 17:46:52

[permalink] [raw]

Subject: Re: [PATCH v10 2/8] PCI: Add new array for keeping track of ordering of reset methods

On 21/07/27 05:59PM, Bjorn Helgaas wrote:
> On Fri, Jul 09, 2021 at 06:08:07PM +0530, Amey Narkhede wrote:
> > Introduce a new array reset_methods in struct pci_dev to keep track of
> > reset mechanisms supported by the device and their ordering.
> >
> > Also refactor probing and reset functions to take advantage of calling
> > convention of reset functions.
> >
> > Co-developed-by: Alex Williamson <[email protected]>
> > Signed-off-by: Alex Williamson <[email protected]>
> > Signed-off-by: Amey Narkhede <[email protected]>
> > ---
> > drivers/pci/pci.c | 92 ++++++++++++++++++++++++++-------------------
> > drivers/pci/pci.h | 9 ++++-
> > drivers/pci/probe.c | 5 +--
> > include/linux/pci.h | 7 ++++
> > 4 files changed, 70 insertions(+), 43 deletions(-)
> >
>
[...]
> > + BUILD_BUG_ON(ARRAY_SIZE(pci_reset_fn_methods) != PCI_NUM_RESET_METHODS);
> >
> > might_sleep();
> >
> > - rc = pci_dev_specific_reset(dev, 1);
> > - if (rc != -ENOTTY)
> > - return rc;
> > - rc = pcie_reset_flr(dev, 1);
> > - if (rc != -ENOTTY)
> > - return rc;
> > - rc = pci_af_flr(dev, 1);
> > - if (rc != -ENOTTY)
> > - return rc;
> > - rc = pci_pm_reset(dev, 1);
> > - if (rc != -ENOTTY)
> > - return rc;
> > + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
> > + rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
> > + if (!rc)
> > + reset_methods[n++] = i;
>
> Why do we need this local reset_methods[] array? Can we just fill
> in dev->reset_methods[] directly and skip the memcpy() below?
>
This is for avoiding caching of previously supported reset methods.
Is it okay if I use memset(dev->reset_methods, 0,
sizeof(dev->reset_methods)) instead to clear the values in
dev->reset_methods?

Thanks,
Amey
> > + else if (rc != -ENOTTY)
> > + break;
> > + }
> >
> > - return pci_reset_bus_function(dev, 1);
> > + memcpy(dev->reset_methods, reset_methods, sizeof(reset_methods));
> > }
> >
> > /**
[...]

2021-07-28 17:56:55

[permalink] [raw]

Subject: Re: [PATCH v10 8/8] PCI: Change the type of probe argument in reset functions

On Wed, Jul 28, 2021 at 11:05:14PM +0530, Amey Narkhede wrote:
> On 21/07/27 05:22PM, Bjorn Helgaas wrote:
> > On Fri, Jul 09, 2021 at 06:08:13PM +0530, Amey Narkhede wrote:
> > > Introduce a new enum pci_reset_mode_t to make the context of probe argument
> > > in reset functions clear and the code easier to read. Change the type of
> > > probe argument in functions which implement reset methods from int to
> > > pci_reset_mode_t to make the intent clear.
> >
> > Not sure adding an enum and a PCI_RESET_MODE_MAX seems worth it to me.
> > It's really a boolean parameter, and I'd be happy to change it to a
> > bool. But I don't think it's worth checking against
> > PCI_RESET_MODE_MAX unless we need more than two options.
> >
> Is it okay to use PCI_RESET_PROBE and PCI_RESET_DO_RESET as bool.
> That would be less confusing than directly using true/false.

You mean like this?

#define PCI_RESET_DO_RESET false
#define PCI_RESET_PROBE true

I don't think there's a huge amount of value, but I guess that's OK as
long as it's confined to drivers/pci/, i.e., not exposed via
include/linux/pci.h.

2021-07-28 18:01:12

[permalink] [raw]

Subject: Re: [PATCH v10 2/8] PCI: Add new array for keeping track of ordering of reset methods

On Wed, Jul 28, 2021 at 11:15:19PM +0530, Amey Narkhede wrote:
> On 21/07/27 05:59PM, Bjorn Helgaas wrote:
> > On Fri, Jul 09, 2021 at 06:08:07PM +0530, Amey Narkhede wrote:
> > > Introduce a new array reset_methods in struct pci_dev to keep track of
> > > reset mechanisms supported by the device and their ordering.
> > >
> > > Also refactor probing and reset functions to take advantage of calling
> > > convention of reset functions.
> > >
> > > Co-developed-by: Alex Williamson <[email protected]>
> > > Signed-off-by: Alex Williamson <[email protected]>
> > > Signed-off-by: Amey Narkhede <[email protected]>
> > > ---
> > > drivers/pci/pci.c | 92 ++++++++++++++++++++++++++-------------------
> > > drivers/pci/pci.h | 9 ++++-
> > > drivers/pci/probe.c | 5 +--
> > > include/linux/pci.h | 7 ++++
> > > 4 files changed, 70 insertions(+), 43 deletions(-)
> > >
> >
> [...]
> > > + BUILD_BUG_ON(ARRAY_SIZE(pci_reset_fn_methods) != PCI_NUM_RESET_METHODS);
> > >
> > > might_sleep();
> > >
> > > - rc = pci_dev_specific_reset(dev, 1);
> > > - if (rc != -ENOTTY)
> > > - return rc;
> > > - rc = pcie_reset_flr(dev, 1);
> > > - if (rc != -ENOTTY)
> > > - return rc;
> > > - rc = pci_af_flr(dev, 1);
> > > - if (rc != -ENOTTY)
> > > - return rc;
> > > - rc = pci_pm_reset(dev, 1);
> > > - if (rc != -ENOTTY)
> > > - return rc;
> > > + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
> > > + rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
> > > + if (!rc)
> > > + reset_methods[n++] = i;
> >
> > Why do we need this local reset_methods[] array? Can we just fill
> > in dev->reset_methods[] directly and skip the memcpy() below?
> >
> This is for avoiding caching of previously supported reset methods.
> Is it okay if I use memset(dev->reset_methods, 0,
> sizeof(dev->reset_methods)) instead to clear the values in
> dev->reset_methods?

I don't think there's ever a case where you look at a
dev->reset_methods[] element past a zero value, so we shouldn't care
about any previously-supported methods left in the array.

If we *do* look at something past a zero value, why do we do that? It
sounds like it would be a bug.

> > > + else if (rc != -ENOTTY)
> > > + break;
> > > + }
> > >
> > > - return pci_reset_bus_function(dev, 1);
> > > + memcpy(dev->reset_methods, reset_methods, sizeof(reset_methods));
> > > }
> > >
> > > /**
> [...]

2021-07-28 18:02:24

[permalink] [raw]

Subject: Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

On 21/07/27 06:28PM, Bjorn Helgaas wrote:
> On Fri, Jul 09, 2021 at 06:08:09PM +0530, Amey Narkhede wrote:
> > Add reset_method sysfs attribute to enable user to query and set user
> > preferred device reset methods and their ordering.
> >
> > Co-developed-by: Alex Williamson <[email protected]>
> > Signed-off-by: Alex Williamson <[email protected]>
> > Signed-off-by: Amey Narkhede <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-pci | 19 +++++
> > drivers/pci/pci-sysfs.c | 103 ++++++++++++++++++++++++
> > 2 files changed, 122 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> > index ef00fada2..43f4e33c7 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > @@ -121,6 +121,25 @@ Description:
> > child buses, and re-discover devices removed earlier
> > from this part of the device tree.
> >
> > +What: /sys/bus/pci/devices/.../reset_method
> > +Date: March 2021
> > +Contact: Amey Narkhede <[email protected]>
> > +Description:
> > + Some devices allow an individual function to be reset
> > + without affecting other functions in the same slot.
> > +
> > + For devices that have this support, a file named
> > + reset_method will be present in sysfs. Initially reading
> > + this file will give names of the device supported reset
> > + methods and their ordering. After write, this file will
> > + give names and ordering of currently enabled reset methods.
> > + Writing the name or comma separated list of names of any of
> > + the device supported reset methods to this file will set
> > + the reset methods and their ordering to be used when
> > + resetting the device. Writing empty string to this file
> > + will disable ability to reset the device and writing
> > + "default" will return to the original value.
> > +
> > What: /sys/bus/pci/devices/.../reset
> > Date: July 2009
> > Contact: Michael S. Tsirkin <[email protected]>
>
[...]

> > + int i;
> > +
> > + if (sysfs_streq(name, ""))
> > + continue;
> > +
> > + name = strim(name);
> > +
> > + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
> > + if (sysfs_streq(name, pci_reset_fn_methods[i].name) &&
> > + !pci_reset_fn_methods[i].reset_fn(pdev, 1)) {
> > + reset_methods[n++] = i;
>
> Can we build this directly in pdev->reset_methods[] so we don't need
> the memcpy() below?
>
This is to avoid writing partial values directly to dev->reset_methods.
So for example if user writes flr,unsupported_value then
dev->reset_methods should not be modified even though flr is valid reset
method in this example to avoid partial writes. Either operation(in this
case writing supported reset methods to reset_method attr) succeeds
completely or it fails othewise.

Thanks,
Amey

[...]

2021-07-28 18:10:27

[permalink] [raw]

Subject: Re: [PATCH v10 2/8] PCI: Add new array for keeping track of ordering of reset methods

Hi Amey,

On 7/28/21 12:45 PM, Amey Narkhede wrote:
>>> + BUILD_BUG_ON(ARRAY_SIZE(pci_reset_fn_methods) != PCI_NUM_RESET_METHODS);
>>>
>>> might_sleep();
>>>
>>> - rc = pci_dev_specific_reset(dev, 1);
>>> - if (rc != -ENOTTY)
>>> - return rc;
>>> - rc = pcie_reset_flr(dev, 1);
>>> - if (rc != -ENOTTY)
>>> - return rc;
>>> - rc = pci_af_flr(dev, 1);
>>> - if (rc != -ENOTTY)
>>> - return rc;
>>> - rc = pci_pm_reset(dev, 1);
>>> - if (rc != -ENOTTY)
>>> - return rc;
>>> + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
>>> + rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
>>> + if (!rc)
>>> + reset_methods[n++] = i;
>> Why do we need this local reset_methods[] array? Can we just fill
>> in dev->reset_methods[] directly and skip the memcpy() below?
>>
> This is for avoiding caching of previously supported reset methods.
> Is it okay if I use memset(dev->reset_methods, 0,
> sizeof(dev->reset_methods)) instead to clear the values in
> dev->reset_methods?

Clearing the array before the loop might a better option, we can get rid of a local variable.

void pci_init_reset_methods(struct pci_dev *dev)
{
        int i, n, rc;

        BUILD_BUG_ON(ARRAY_SIZE(pci_reset_fn_methods) != PCI_NUM_RESET_METHODS);

        might_sleep();

        memset(dev->reset_methods, 0x0, sizeof(reset_methods));

        n = 0;
        for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
                rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
                if (!rc)
                        dev->reset_methods[n++] = i;
                else if (rc != -ENOTTY)
                        break;
        }
}

2021-07-28 18:16:28

[permalink] [raw]

Subject: Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

On Wed, Jul 28, 2021 at 11:29:50PM +0530, Amey Narkhede wrote:
> On 21/07/27 06:28PM, Bjorn Helgaas wrote:
> > On Fri, Jul 09, 2021 at 06:08:09PM +0530, Amey Narkhede wrote:
> > > Add reset_method sysfs attribute to enable user to query and set user
> > > preferred device reset methods and their ordering.
> > >
> > > Co-developed-by: Alex Williamson <[email protected]>
> > > Signed-off-by: Alex Williamson <[email protected]>
> > > Signed-off-by: Amey Narkhede <[email protected]>
> > > ---
> > > Documentation/ABI/testing/sysfs-bus-pci | 19 +++++
> > > drivers/pci/pci-sysfs.c | 103 ++++++++++++++++++++++++
> > > 2 files changed, 122 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> > > index ef00fada2..43f4e33c7 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > > @@ -121,6 +121,25 @@ Description:
> > > child buses, and re-discover devices removed earlier
> > > from this part of the device tree.
> > >
> > > +What: /sys/bus/pci/devices/.../reset_method
> > > +Date: March 2021
> > > +Contact: Amey Narkhede <[email protected]>
> > > +Description:
> > > + Some devices allow an individual function to be reset
> > > + without affecting other functions in the same slot.
> > > +
> > > + For devices that have this support, a file named
> > > + reset_method will be present in sysfs. Initially reading
> > > + this file will give names of the device supported reset
> > > + methods and their ordering. After write, this file will
> > > + give names and ordering of currently enabled reset methods.
> > > + Writing the name or comma separated list of names of any of
> > > + the device supported reset methods to this file will set
> > > + the reset methods and their ordering to be used when
> > > + resetting the device. Writing empty string to this file
> > > + will disable ability to reset the device and writing
> > > + "default" will return to the original value.
> > > +
> > > What: /sys/bus/pci/devices/.../reset
> > > Date: July 2009
> > > Contact: Michael S. Tsirkin <[email protected]>
> >
> [...]
>
> > > + int i;
> > > +
> > > + if (sysfs_streq(name, ""))
> > > + continue;
> > > +
> > > + name = strim(name);
> > > +
> > > + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
> > > + if (sysfs_streq(name, pci_reset_fn_methods[i].name) &&
> > > + !pci_reset_fn_methods[i].reset_fn(pdev, 1)) {
> > > + reset_methods[n++] = i;
> >
> > Can we build this directly in pdev->reset_methods[] so we don't need
> > the memcpy() below?
> >
> This is to avoid writing partial values directly to dev->reset_methods.
> So for example if user writes flr,unsupported_value then
> dev->reset_methods should not be modified even though flr is valid reset
> method in this example to avoid partial writes. Either operation(in this
> case writing supported reset methods to reset_method attr) succeeds
> completely or it fails othewise.

I guess the question is, if somebody writes

flr junk bus

and we set the supported methods to "flr bus", is that OK? It seems
OK to me; obviously we have to protect ourselves from attack, but
over-zealous checking can make things unnecessarily complicated.

2021-07-28 18:19:56

[permalink] [raw]

Subject: Re: [PATCH v10 2/8] PCI: Add new array for keeping track of ordering of reset methods

Hi Bjorn,

On 7/28/21 12:59 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Jul 28, 2021 at 11:15:19PM +0530, Amey Narkhede wrote:
>> On 21/07/27 05:59PM, Bjorn Helgaas wrote:
>>> On Fri, Jul 09, 2021 at 06:08:07PM +0530, Amey Narkhede wrote:
>>>> Introduce a new array reset_methods in struct pci_dev to keep track of
>>>> reset mechanisms supported by the device and their ordering.
>>>>
>>>> Also refactor probing and reset functions to take advantage of calling
>>>> convention of reset functions.
>>>>
>>>> Co-developed-by: Alex Williamson <[email protected]>
>>>> Signed-off-by: Alex Williamson <[email protected]>
>>>> Signed-off-by: Amey Narkhede <[email protected]>
>>>> ---
>>>> drivers/pci/pci.c | 92 ++++++++++++++++++++++++++-------------------
>>>> drivers/pci/pci.h | 9 ++++-
>>>> drivers/pci/probe.c | 5 +--
>>>> include/linux/pci.h | 7 ++++
>>>> 4 files changed, 70 insertions(+), 43 deletions(-)
>>>>
>> [...]
>>>> + BUILD_BUG_ON(ARRAY_SIZE(pci_reset_fn_methods) != PCI_NUM_RESET_METHODS);
>>>>
>>>> might_sleep();
>>>>
>>>> - rc = pci_dev_specific_reset(dev, 1);
>>>> - if (rc != -ENOTTY)
>>>> - return rc;
>>>> - rc = pcie_reset_flr(dev, 1);
>>>> - if (rc != -ENOTTY)
>>>> - return rc;
>>>> - rc = pci_af_flr(dev, 1);
>>>> - if (rc != -ENOTTY)
>>>> - return rc;
>>>> - rc = pci_pm_reset(dev, 1);
>>>> - if (rc != -ENOTTY)
>>>> - return rc;
>>>> + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
>>>> + rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
>>>> + if (!rc)
>>>> + reset_methods[n++] = i;
>>> Why do we need this local reset_methods[] array? Can we just fill
>>> in dev->reset_methods[] directly and skip the memcpy() below?
>>>
>> This is for avoiding caching of previously supported reset methods.
>> Is it okay if I use memset(dev->reset_methods, 0,
>> sizeof(dev->reset_methods)) instead to clear the values in
>> dev->reset_methods?
> I don't think there's ever a case where you look at a
> dev->reset_methods[] element past a zero value, so we shouldn't care
> about any previously-supported methods left in the array.
>
> If we *do* look at something past a zero value, why do we do that? It
> sounds like it would be a bug.
>

I think either we need memset or clear 0th/last element. Can we set the last
index explicitly to zero?

void pci_init_reset_methods(struct pci_dev *dev)
{
        int i, n, rc;

        BUILD_BUG_ON(ARRAY_SIZE(pci_reset_fn_methods) != PCI_NUM_RESET_METHODS);

        might_sleep();

        n = 0;
        for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
                rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
                if (!rc)
                        dev->reset_methods[n++] = i;
                else if (rc != -ENOTTY)
                        break;
        }
        dev->reset_methods[n] = 0;
}

2021-07-28 18:35:09

[permalink] [raw]

Subject: Re: [PATCH v10 2/8] PCI: Add new array for keeping track of ordering of reset methods

Hi Bjorn,

On 7/27/21 5:59 PM, Bjorn Helgaas wrote:
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index fefa6d7b3..42440cb10 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -72,6 +72,14 @@ static void pci_dev_d3_sleep(struct pci_dev *dev)
>> msleep(delay);
>> }
>>
>> +int pci_reset_supported(struct pci_dev *dev)
>> +{
>> + u8 null_reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
>> +
>> + return memcmp(null_reset_methods,
>> + dev->reset_methods, sizeof(null_reset_methods));
> I think "return dev->reset_methods[0] != 0;" is sufficient, isn't it?
>

Agree with you, it simplifies code logic and can be moved to "include/linux/pci.h"
with inline definition. Can we change return type to 'bool' instead of 'int' ?

static inline bool pci_reset_supported(struct pci_dev *dev)
{
return !!dev->reset_methods[0];
}

2021-07-28 18:58:01

[permalink] [raw]

Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

Hi Bjorn,

On 7/27/21 5:12 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Fri, Jul 09, 2021 at 06:08:06PM +0530, Amey Narkhede wrote:
>> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
>> FLR to avoid reading PCI_EXP_DEVCAP multiple times.
>>
>> Currently there is separate function pcie_has_flr() to probe if PCIe FLR
>> is supported by the device which does not match the calling convention
>> followed by reset methods which use second function argument to decide
>> whether to probe or not. Add new function pcie_reset_flr() that follows
>> the calling convention of reset methods.
>>
>> Signed-off-by: Amey Narkhede <[email protected]>
>> ---
>> drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
>> drivers/pci/pci.c | 59 +++++++++++-----------
>> drivers/pci/pcie/aer.c | 12 ++---
>> drivers/pci/probe.c | 6 ++-
>> drivers/pci/quirks.c | 9 ++--
>> include/linux/pci.h | 3 +-
>> 6 files changed, 45 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
>> index facc8e6bc..15d6c8452 100644
>> --- a/drivers/crypto/cavium/nitrox/nitrox_main.c
>> +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
>> @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
>> return -ENOMEM;
>> }
>>
>> - /* check flr support */
>> - if (pcie_has_flr(pdev))
>> - pcie_flr(pdev);
>> + pcie_reset_flr(pdev, 0);
> I'm not really a fan of exposing the "probe" argument outside
> drivers/pci/. I think this would be the only occurrence. Is there a
> way to avoid that?
>
> Can we just make pcie_flr() do the equivalent of pcie_has_flr()
> internally?
>
I like your suggestion don't change the existing definition of pcie_has_flr()/pcie_flr()
and define a new function pcie_reset_flr() to fit into the reset framework. This way
no need to modify logic/drivers outside of driver/pci/xxx.

int pcie_reset_flr(struct pci_dev *dev, int probe)
{
        if (!pcie_has_flr(dev))
                return -ENOTTY;

        if (probe)
                return 0;

        return pcie_flr(dev);
}

And add a new patch to begging of the series for caching 'devcap' in pci_dev structure.

--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -333,6 +333,7 @@ struct pci_dev {
        struct rcec_ea *rcec_ea;       /* RCEC cached endpoint association */
        struct pci_dev *rcec;          /* Associated RCEC device */
#endif
+       u32             devcap;         /* Cached PCIe device capabilities */
        u8              pcie_cap;       /* PCIe capability offset */
        u8              msi_cap;        /* MSI capability offset */
        u8              msix_cap;       /* MSI-X capability offset */

--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -31,6 +31,7 @@
#include <linux/vmalloc.h>
#include <asm/dma.h>
#include <linux/aer.h>
+#include <linux/bitfield.h>
#include "pci.h"

DEFINE_MUTEX(pci_slot_mutex);
@@ -4630,13 +4631,10 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
*/
bool pcie_has_flr(struct pci_dev *dev)
{
-       u32 cap;
-
        if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
                return false;

-       pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
-       return cap & PCI_EXP_DEVCAP_FLR;
+       return !!FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap);
}

--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -19,6 +19,7 @@
#include <linux/hypervisor.h>
#include <linux/irqdomain.h>
#include <linux/pm_runtime.h>
+#include <linux/bitfield.h>
#include "pci.h"

#define CARDBUS_LATENCY_TIMER 176     /* secondary latency timer */
@@ -1498,8 +1499,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
        pdev->pcie_cap = pos;
        pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
        pdev->pcie_flags_reg = reg16;
-       pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
-       pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
+       pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
+       pdev->pcie_mpss = FIELD_GET(PCI_EXP_DEVCAP_PAYLOAD, pdev->devcap);

2021-07-28 19:01:27

[permalink] [raw]

Subject: Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

On 21/07/28 01:13PM, Bjorn Helgaas wrote:
> On Wed, Jul 28, 2021 at 11:29:50PM +0530, Amey Narkhede wrote:
> > On 21/07/27 06:28PM, Bjorn Helgaas wrote:
> > > On Fri, Jul 09, 2021 at 06:08:09PM +0530, Amey Narkhede wrote:
> > > > Add reset_method sysfs attribute to enable user to query and set user
> > > > preferred device reset methods and their ordering.
> > > >
> > > > Co-developed-by: Alex Williamson <[email protected]>
> > > > Signed-off-by: Alex Williamson <[email protected]>
> > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > ---
> > > > Documentation/ABI/testing/sysfs-bus-pci | 19 +++++
> > > > drivers/pci/pci-sysfs.c | 103 ++++++++++++++++++++++++
> > > > 2 files changed, 122 insertions(+)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> > > > index ef00fada2..43f4e33c7 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > > > @@ -121,6 +121,25 @@ Description:
> > > > child buses, and re-discover devices removed earlier
> > > > from this part of the device tree.
> > > >
> > > > +What: /sys/bus/pci/devices/.../reset_method
> > > > +Date: March 2021
> > > > +Contact: Amey Narkhede <[email protected]>
> > > > +Description:
> > > > + Some devices allow an individual function to be reset
> > > > + without affecting other functions in the same slot.
> > > > +
> > > > + For devices that have this support, a file named
> > > > + reset_method will be present in sysfs. Initially reading
> > > > + this file will give names of the device supported reset
> > > > + methods and their ordering. After write, this file will
> > > > + give names and ordering of currently enabled reset methods.
> > > > + Writing the name or comma separated list of names of any of
> > > > + the device supported reset methods to this file will set
> > > > + the reset methods and their ordering to be used when
> > > > + resetting the device. Writing empty string to this file
> > > > + will disable ability to reset the device and writing
> > > > + "default" will return to the original value.
> > > > +
> > > > What: /sys/bus/pci/devices/.../reset
> > > > Date: July 2009
> > > > Contact: Michael S. Tsirkin <[email protected]>
> > >
> > [...]
> >
> > > > + int i;
> > > > +
> > > > + if (sysfs_streq(name, ""))
> > > > + continue;
> > > > +
> > > > + name = strim(name);
> > > > +
> > > > + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
> > > > + if (sysfs_streq(name, pci_reset_fn_methods[i].name) &&
> > > > + !pci_reset_fn_methods[i].reset_fn(pdev, 1)) {
> > > > + reset_methods[n++] = i;
> > >
> > > Can we build this directly in pdev->reset_methods[] so we don't need
> > > the memcpy() below?
> > >
> > This is to avoid writing partial values directly to dev->reset_methods.
> > So for example if user writes flr,unsupported_value then
> > dev->reset_methods should not be modified even though flr is valid reset
> > method in this example to avoid partial writes. Either operation(in this
> > case writing supported reset methods to reset_method attr) succeeds
> > completely or it fails othewise.
>
> I guess the question is, if somebody writes
>
> flr junk bus
>
> and we set the supported methods to "flr bus", is that OK? It seems
> OK to me; obviously we have to protect ourselves from attack, but
> over-zealous checking can make things unnecessarily complicated.
The problem is once we encounter "junk" we return -EINVAL so in your
example we only set flr.

Thanks,
Amey

2021-07-28 20:20:08

[permalink] [raw]

Subject: Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

On Thu, Jul 29, 2021 at 12:28:31AM +0530, Amey Narkhede wrote:
> On 21/07/28 01:13PM, Bjorn Helgaas wrote:
> > On Wed, Jul 28, 2021 at 11:29:50PM +0530, Amey Narkhede wrote:
> > > On 21/07/27 06:28PM, Bjorn Helgaas wrote:
> > > > On Fri, Jul 09, 2021 at 06:08:09PM +0530, Amey Narkhede wrote:
> > > > > Add reset_method sysfs attribute to enable user to query and set user
> > > > > preferred device reset methods and their ordering.
> > > > >
> > > > > Co-developed-by: Alex Williamson <[email protected]>
> > > > > Signed-off-by: Alex Williamson <[email protected]>
> > > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > > ---
> > > > > Documentation/ABI/testing/sysfs-bus-pci | 19 +++++
> > > > > drivers/pci/pci-sysfs.c | 103 ++++++++++++++++++++++++
> > > > > 2 files changed, 122 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> > > > > index ef00fada2..43f4e33c7 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > > > > @@ -121,6 +121,25 @@ Description:
> > > > > child buses, and re-discover devices removed earlier
> > > > > from this part of the device tree.
> > > > >
> > > > > +What: /sys/bus/pci/devices/.../reset_method
> > > > > +Date: March 2021
> > > > > +Contact: Amey Narkhede <[email protected]>
> > > > > +Description:
> > > > > + Some devices allow an individual function to be reset
> > > > > + without affecting other functions in the same slot.
> > > > > +
> > > > > + For devices that have this support, a file named
> > > > > + reset_method will be present in sysfs. Initially reading
> > > > > + this file will give names of the device supported reset
> > > > > + methods and their ordering. After write, this file will
> > > > > + give names and ordering of currently enabled reset methods.
> > > > > + Writing the name or comma separated list of names of any of
> > > > > + the device supported reset methods to this file will set
> > > > > + the reset methods and their ordering to be used when
> > > > > + resetting the device. Writing empty string to this file
> > > > > + will disable ability to reset the device and writing
> > > > > + "default" will return to the original value.
> > > > > +
> > > > > What: /sys/bus/pci/devices/.../reset
> > > > > Date: July 2009
> > > > > Contact: Michael S. Tsirkin <[email protected]>
> > > >
> > > [...]
> > >
> > > > > + int i;
> > > > > +
> > > > > + if (sysfs_streq(name, ""))
> > > > > + continue;
> > > > > +
> > > > > + name = strim(name);
> > > > > +
> > > > > + for (i = 1; i < PCI_NUM_RESET_METHODS; i++) {
> > > > > + if (sysfs_streq(name, pci_reset_fn_methods[i].name) &&
> > > > > + !pci_reset_fn_methods[i].reset_fn(pdev, 1)) {
> > > > > + reset_methods[n++] = i;
> > > >
> > > > Can we build this directly in pdev->reset_methods[] so we don't need
> > > > the memcpy() below?
> > > >
> > > This is to avoid writing partial values directly to dev->reset_methods.
> > > So for example if user writes flr,unsupported_value then
> > > dev->reset_methods should not be modified even though flr is valid reset
> > > method in this example to avoid partial writes. Either operation(in this
> > > case writing supported reset methods to reset_method attr) succeeds
> > > completely or it fails othewise.
> >
> > I guess the question is, if somebody writes
> >
> > flr junk bus
> >
> > and we set the supported methods to "flr bus", is that OK? It seems
> > OK to me; obviously we have to protect ourselves from attack, but
> > over-zealous checking can make things unnecessarily complicated.
>
> The problem is once we encounter "junk" we return -EINVAL so in your
> example we only set flr.

We're still talking about whether we need the memcpy(). We can decide
whether it's the right thing to return -EINVAL if we encounter "junk".

Maybe it's OK if we set it to "flr" and ignore everything after
"junk"? Or maybe it's OK to ignore unsupported values and set it to
"flr bus"?

2021-07-28 20:25:14

[permalink] [raw]

Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

On Wed, Jul 28, 2021 at 01:54:16PM -0500, Shanker R Donthineni wrote:
> On 7/27/21 5:12 PM, Bjorn Helgaas wrote:
> > On Fri, Jul 09, 2021 at 06:08:06PM +0530, Amey Narkhede wrote:
> >> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
> >> FLR to avoid reading PCI_EXP_DEVCAP multiple times.
> >>
> >> Currently there is separate function pcie_has_flr() to probe if PCIe FLR
> >> is supported by the device which does not match the calling convention
> >> followed by reset methods which use second function argument to decide
> >> whether to probe or not. Add new function pcie_reset_flr() that follows
> >> the calling convention of reset methods.
> >>
> >> Signed-off-by: Amey Narkhede <[email protected]>
> >> ---
> >> drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
> >> drivers/pci/pci.c | 59 +++++++++++-----------
> >> drivers/pci/pcie/aer.c | 12 ++---
> >> drivers/pci/probe.c | 6 ++-
> >> drivers/pci/quirks.c | 9 ++--
> >> include/linux/pci.h | 3 +-
> >> 6 files changed, 45 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
> >> index facc8e6bc..15d6c8452 100644
> >> --- a/drivers/crypto/cavium/nitrox/nitrox_main.c
> >> +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
> >> @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
> >> return -ENOMEM;
> >> }
> >>
> >> - /* check flr support */
> >> - if (pcie_has_flr(pdev))
> >> - pcie_flr(pdev);
> >> + pcie_reset_flr(pdev, 0);
> > I'm not really a fan of exposing the "probe" argument outside
> > drivers/pci/. I think this would be the only occurrence. Is there a
> > way to avoid that?
> >
> > Can we just make pcie_flr() do the equivalent of pcie_has_flr()
> > internally?
> >
> I like your suggestion don't change the existing definition of
> pcie_has_flr()/pcie_flr() and define a new function pcie_reset_flr()
> to fit into the reset framework. This way no need to modify
> logic/drivers outside of driver/pci/xxx.
>
> int pcie_reset_flr(struct pci_dev *dev, int probe)
> {
> ??????? if (!pcie_has_flr(dev))
> ??????????????? return -ENOTTY;
>
> ??????? if (probe)
> ??????????????? return 0;
>
> ??????? return pcie_flr(dev);
> }

Can't remember the whole context of this in the series, but I assume
this would be static?

> And add a new patch to begging of the series for caching 'devcap' in
> pci_dev structure.
>
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -333,6 +333,7 @@ struct pci_dev {
> ??????? struct rcec_ea? *rcec_ea;?????? /* RCEC cached endpoint association */
> ??????? struct pci_dev? *rcec;????????? /* Associated RCEC device */
> ?#endif
> +?????? u32???????????? devcap;???????? /* Cached PCIe device capabilities */
> ??????? u8????????????? pcie_cap;?????? /* PCIe capability offset */
> ??????? u8????????????? msi_cap;??????? /* MSI capability offset */
> ??????? u8????????????? msix_cap;?????? /* MSI-X capability offset */
>
>
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -31,6 +31,7 @@
> ?#include <linux/vmalloc.h>
> ?#include <asm/dma.h>
> ?#include <linux/aer.h>
> +#include <linux/bitfield.h>
> ?#include "pci.h"
>
> ?DEFINE_MUTEX(pci_slot_mutex);
> @@ -4630,13 +4631,10 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
> ? */
> ?bool pcie_has_flr(struct pci_dev *dev)
> ?{
> -?????? u32 cap;
> -
> ??????? if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
> ??????????????? return false;
>
> -?????? pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
> -?????? return cap & PCI_EXP_DEVCAP_FLR;
> +?????? return !!FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap);

Nice, thanks for reminding me of FIELD_GET(). I like how that works
without having to #define *_SHIFT values. I personally don't care for
"!!" and would probably write something like:

return FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap) == 1;

> ?}
>
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -19,6 +19,7 @@
> ?#include <linux/hypervisor.h>
> ?#include <linux/irqdomain.h>
> ?#include <linux/pm_runtime.h>
> +#include <linux/bitfield.h>
> ?#include "pci.h"
>
> ?#define CARDBUS_LATENCY_TIMER? 176???? /* secondary latency timer */
> @@ -1498,8 +1499,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
> ??????? pdev->pcie_cap = pos;
> ??????? pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
> ??????? pdev->pcie_flags_reg = reg16;
> -?????? pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
> -?????? pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
> +?????? pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
> +?????? pdev->pcie_mpss = FIELD_GET(PCI_EXP_DEVCAP_PAYLOAD, pdev->devcap);
>

2021-07-28 20:27:21

[permalink] [raw]

Subject: Re: [PATCH v10 2/8] PCI: Add new array for keeping track of ordering of reset methods

On Wed, Jul 28, 2021 at 01:31:19PM -0500, Shanker R Donthineni wrote:
> Hi Bjorn,
>
> On 7/27/21 5:59 PM, Bjorn Helgaas wrote:
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index fefa6d7b3..42440cb10 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -72,6 +72,14 @@ static void pci_dev_d3_sleep(struct pci_dev *dev)
> >> msleep(delay);
> >> }
> >>
> >> +int pci_reset_supported(struct pci_dev *dev)
> >> +{
> >> + u8 null_reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
> >> +
> >> + return memcmp(null_reset_methods,
> >> + dev->reset_methods, sizeof(null_reset_methods));
> > I think "return dev->reset_methods[0] != 0;" is sufficient, isn't it?
> >
>
> Agree with you, it simplifies code logic and can be moved to
> "include/linux/pci.h" with inline definition. Can we change return
> type to 'bool' instead of 'int' ?

Does it need to be exported to drivers? Looks like it's only used
inside drivers/pci/, so it shouldn't be in include/linux/pci.h.

Making it bool is fine with me.

> static inline bool pci_reset_supported(struct pci_dev *dev)
> {??
> ??? return !!dev->reset_methods[0];
> }
>

2021-07-28 21:59:12

[permalink] [raw]

Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

Hi Bjorn,

On 7/28/21 3:23 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Jul 28, 2021 at 01:54:16PM -0500, Shanker R Donthineni wrote:
>> On 7/27/21 5:12 PM, Bjorn Helgaas wrote:
>>> On Fri, Jul 09, 2021 at 06:08:06PM +0530, Amey Narkhede wrote:
>>>> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
>>>> FLR to avoid reading PCI_EXP_DEVCAP multiple times.
>>>>
>>>> Currently there is separate function pcie_has_flr() to probe if PCIe FLR
>>>> is supported by the device which does not match the calling convention
>>>> followed by reset methods which use second function argument to decide
>>>> whether to probe or not. Add new function pcie_reset_flr() that follows
>>>> the calling convention of reset methods.
>>>>
>>>> Signed-off-by: Amey Narkhede <[email protected]>
>>>> ---
>>>> drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
>>>> drivers/pci/pci.c | 59 +++++++++++-----------
>>>> drivers/pci/pcie/aer.c | 12 ++---
>>>> drivers/pci/probe.c | 6 ++-
>>>> drivers/pci/quirks.c | 9 ++--
>>>> include/linux/pci.h | 3 +-
>>>> 6 files changed, 45 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>> index facc8e6bc..15d6c8452 100644
>>>> --- a/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>> +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>> @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
>>>> return -ENOMEM;
>>>> }
>>>>
>>>> - /* check flr support */
>>>> - if (pcie_has_flr(pdev))
>>>> - pcie_flr(pdev);
>>>> + pcie_reset_flr(pdev, 0);
>>> I'm not really a fan of exposing the "probe" argument outside
>>> drivers/pci/. I think this would be the only occurrence. Is there a
>>> way to avoid that?
>>>
>>> Can we just make pcie_flr() do the equivalent of pcie_has_flr()
>>> internally?
>>>
>> I like your suggestion don't change the existing definition of
>> pcie_has_flr()/pcie_flr() and define a new function pcie_reset_flr()
>> to fit into the reset framework. This way no need to modify
>> logic/drivers outside of driver/pci/xxx.
>>
>> int pcie_reset_flr(struct pci_dev *dev, int probe)
>> {
>> if (!pcie_has_flr(dev))
>> return -ENOTTY;
>>
>> if (probe)
>> return 0;
>>
>> return pcie_flr(dev);
>> }
> Can't remember the whole context of this in the series, but I assume
> this would be static?

It should be static since it's referenced in driver/pci/qrirk.c and aer.c.

>> And add a new patch to begging of the series for caching 'devcap' in
>> pci_dev structure.
>>
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -333,6 +333,7 @@ struct pci_dev {
>> struct rcec_ea *rcec_ea; /* RCEC cached endpoint association */
>> struct pci_dev *rcec; /* Associated RCEC device */
>> #endif
>> + u32 devcap; /* Cached PCIe device capabilities */
>> u8 pcie_cap; /* PCIe capability offset */
>> u8 msi_cap; /* MSI capability offset */
>> u8 msix_cap; /* MSI-X capability offset */
>>
>>
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -31,6 +31,7 @@
>> #include <linux/vmalloc.h>
>> #include <asm/dma.h>
>> #include <linux/aer.h>
>> +#include <linux/bitfield.h>
>> #include "pci.h"
>>
>> DEFINE_MUTEX(pci_slot_mutex);
>> @@ -4630,13 +4631,10 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
>> */
>> bool pcie_has_flr(struct pci_dev *dev)
>> {
>> - u32 cap;
>> -
>> if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
>> return false;
>>
>> - pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
>> - return cap & PCI_EXP_DEVCAP_FLR;
>> + return !!FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap);
> Nice, thanks for reminding me of FIELD_GET(). I like how that works
> without having to #define *_SHIFT values. I personally don't care for
> "!!" and would probably write something like:
>
> return FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap) == 1;

Both are same since FLR is a single bit value.

>> }
>>
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -19,6 +19,7 @@
>> #include <linux/hypervisor.h>
>> #include <linux/irqdomain.h>
>> #include <linux/pm_runtime.h>
>> +#include <linux/bitfield.h>
>> #include "pci.h"
>>
>> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
>> @@ -1498,8 +1499,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
>> pdev->pcie_cap = pos;
>> pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
>> pdev->pcie_flags_reg = reg16;
>> - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
>> - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
>> + pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
>> + pdev->pcie_mpss = FIELD_GET(PCI_EXP_DEVCAP_PAYLOAD, pdev->devcap);
>>

2021-07-28 22:03:25

[permalink] [raw]

Subject: Re: [PATCH v10 2/8] PCI: Add new array for keeping track of ordering of reset methods

On 7/28/21 3:25 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Jul 28, 2021 at 01:31:19PM -0500, Shanker R Donthineni wrote:
>> Hi Bjorn,
>>
>> On 7/27/21 5:59 PM, Bjorn Helgaas wrote:
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index fefa6d7b3..42440cb10 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -72,6 +72,14 @@ static void pci_dev_d3_sleep(struct pci_dev *dev)
>>>> msleep(delay);
>>>> }
>>>>
>>>> +int pci_reset_supported(struct pci_dev *dev)
>>>> +{
>>>> + u8 null_reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
>>>> +
>>>> + return memcmp(null_reset_methods,
>>>> + dev->reset_methods, sizeof(null_reset_methods));
>>> I think "return dev->reset_methods[0] != 0;" is sufficient, isn't it?
>>>
>> Agree with you, it simplifies code logic and can be moved to
>> "include/linux/pci.h" with inline definition. Can we change return
>> type to 'bool' instead of 'int' ?
> Does it need to be exported to drivers? Looks like it's only used
> inside drivers/pci/, so it shouldn't be in include/linux/pci.h.

Yes, you're right can be moved to driver/pci/pci.h.

> Making it bool is fine with me.
>
>> static inline bool pci_reset_supported(struct pci_dev *dev)
>> {
>> return !!dev->reset_methods[0];
>> }
>>

2021-07-28 22:06:14

[permalink] [raw]

Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

On 7/28/21 4:58 PM, Shanker R Donthineni wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Bjorn,
>
> On 7/28/21 3:23 PM, Bjorn Helgaas wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Wed, Jul 28, 2021 at 01:54:16PM -0500, Shanker R Donthineni wrote:
>>> On 7/27/21 5:12 PM, Bjorn Helgaas wrote:
>>>> On Fri, Jul 09, 2021 at 06:08:06PM +0530, Amey Narkhede wrote:
>>>>> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
>>>>> FLR to avoid reading PCI_EXP_DEVCAP multiple times.
>>>>>
>>>>> Currently there is separate function pcie_has_flr() to probe if PCIe FLR
>>>>> is supported by the device which does not match the calling convention
>>>>> followed by reset methods which use second function argument to decide
>>>>> whether to probe or not. Add new function pcie_reset_flr() that follows
>>>>> the calling convention of reset methods.
>>>>>
>>>>> Signed-off-by: Amey Narkhede <[email protected]>
>>>>> ---
>>>>> drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
>>>>> drivers/pci/pci.c | 59 +++++++++++-----------
>>>>> drivers/pci/pcie/aer.c | 12 ++---
>>>>> drivers/pci/probe.c | 6 ++-
>>>>> drivers/pci/quirks.c | 9 ++--
>>>>> include/linux/pci.h | 3 +-
>>>>> 6 files changed, 45 insertions(+), 48 deletions(-)
>>>>>
>>>>> diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>>> index facc8e6bc..15d6c8452 100644
>>>>> --- a/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>>> +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>>> @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
>>>>> return -ENOMEM;
>>>>> }
>>>>>
>>>>> - /* check flr support */
>>>>> - if (pcie_has_flr(pdev))
>>>>> - pcie_flr(pdev);
>>>>> + pcie_reset_flr(pdev, 0);
>>>> I'm not really a fan of exposing the "probe" argument outside
>>>> drivers/pci/. I think this would be the only occurrence. Is there a
>>>> way to avoid that?
>>>>
>>>> Can we just make pcie_flr() do the equivalent of pcie_has_flr()
>>>> internally?
>>>>
>>> I like your suggestion don't change the existing definition of
>>> pcie_has_flr()/pcie_flr() and define a new function pcie_reset_flr()
>>> to fit into the reset framework. This way no need to modify
>>> logic/drivers outside of driver/pci/xxx.
>>>
>>> int pcie_reset_flr(struct pci_dev *dev, int probe)
>>> {
>>> if (!pcie_has_flr(dev))
>>> return -ENOTTY;
>>>
>>> if (probe)
>>> return 0;
>>>
>>> return pcie_flr(dev);
>>> }
>> Can't remember the whole context of this in the series, but I assume
>> this would be static?
> It should be static since it's referenced in driver/pci/qrirk.c and aer.c.
>
Sorry it should not be static since it's referenced in driver/pci/quirk.c and aer.c

2021-07-28 22:18:23

[permalink] [raw]

Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

On Wed, Jul 28, 2021 at 04:58:14PM -0500, Shanker R Donthineni wrote:
> On 7/28/21 3:23 PM, Bjorn Helgaas wrote:
> > On Wed, Jul 28, 2021 at 01:54:16PM -0500, Shanker R Donthineni wrote:
> >> On 7/27/21 5:12 PM, Bjorn Helgaas wrote:
> >>> On Fri, Jul 09, 2021 at 06:08:06PM +0530, Amey Narkhede wrote:
> >>>> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
> >>>> FLR to avoid reading PCI_EXP_DEVCAP multiple times.

> >> - pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
> >> - return cap & PCI_EXP_DEVCAP_FLR;
> >> + return !!FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap);
> >
> > Nice, thanks for reminding me of FIELD_GET(). I like how that works
> > without having to #define *_SHIFT values. I personally don't care for
> > "!!" and would probably write something like:
> >
> > return FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap) == 1;
>
> Both are same since FLR is a single bit value.

Right; this is a style preference, not a correctness question.

2021-07-31 19:16:12