From: Amey Narkhede <redacted>
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.
Suggested-by: Alex Williamson <redacted>
Reviewed-by: Alex Williamson <redacted>
Reviewed-by: Raphael Norwitz <redacted>
Amey Narkhede (4):
PCI: Refactor pcie_flr to follow calling convention of other reset
methods
PCI: Add new bitmap for keeping track of supported reset mechanisms
PCI: Remove reset_fn field from pci_dev
PCI/sysfs: Allow userspace to query and set device reset mechanism
Documentation/ABI/testing/sysfs-bus-pci | 15 ++
drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
drivers/crypto/qat/qat_common/adf_aer.c | 2 +-
drivers/infiniband/hw/hfi1/chip.c | 4 +-
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
.../ethernet/cavium/liquidio/lio_vf_main.c | 4 +-
.../ethernet/cavium/liquidio/octeon_mailbox.c | 2 +-
drivers/net/ethernet/freescale/enetc/enetc.c | 2 +-
.../ethernet/freescale/enetc/enetc_pci_mdio.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 +-
drivers/pci/pci-sysfs.c | 68 +++++++-
drivers/pci/pci.c | 160 ++++++++++--------
drivers/pci/pci.h | 11 +-
drivers/pci/pcie/aer.c | 12 +-
drivers/pci/probe.c | 4 +-
drivers/pci/quirks.c | 17 +-
include/linux/pci.h | 17 +-
17 files changed, 213 insertions(+), 117 deletions(-)
--
2.30.2
From: Amey Narkhede <redacted>
reset_fn field is used to indicate whether the
device supports any reset mechanism or not.
Deprecate use of reset_fn in favor of new
reset_methods bitmap which can be used to keep
track of all supported reset mechanisms of a device.
Signed-off-by: Amey Narkhede <redacted>
---
Reviewed-by: Alex Williamson <redacted>
Reviewed-by: Raphael Norwitz <redacted>
drivers/net/ethernet/cavium/liquidio/lio_vf_main.c | 2 +-
drivers/pci/pci-sysfs.c | 6 ++----
drivers/pci/pci.c | 6 +++---
drivers/pci/probe.c | 1 -
drivers/pci/quirks.c | 2 +-
include/linux/pci.h | 1 -
6 files changed, 7 insertions(+), 11 deletions(-)
From: Amey Narkhede <redacted>
Currently there is separate function pcie_has_flr to probe
whether pcie flr is supported or not by the device which does
not match the calling convention followed by all other reset
methods which use second function argument to decide whether
to probe or not. Refactor pcie_flr to follow calling convention
of reset methods and remove superfluous pcie_has_flr function.
Signed-off-by: Amey Narkhede <redacted>
---
Reviewed-by: Alex Williamson <redacted>
Reviewed-by: Raphael Norwitz <redacted>
drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
drivers/crypto/qat/qat_common/adf_aer.c | 2 +-
drivers/infiniband/hw/hfi1/chip.c | 4 +-
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
.../ethernet/cavium/liquidio/lio_vf_main.c | 2 +-
.../ethernet/cavium/liquidio/octeon_mailbox.c | 2 +-
drivers/net/ethernet/freescale/enetc/enetc.c | 2 +-
.../ethernet/freescale/enetc/enetc_pci_mdio.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 +-
drivers/pci/pci.c | 65 ++++++++++---------
drivers/pci/pcie/aer.c | 12 ++--
drivers/pci/quirks.c | 15 ++---
include/linux/pci.h | 4 +-
13 files changed, 58 insertions(+), 62 deletions(-)
@@ -14099,7 +14099,7 @@ static int init_chip(struct hfi1_devdata *dd)dd_dev_info(dd,"Resetting CSRs with FLR\n");/* do the FLR, the DC reset will remain */-pcie_flr(dd->pcidev);+pcie_flr(dd->pcidev,0);/* restore command and BARs */ret=restore_pci_variables(dd);
@@ -14111,7 +14111,7 @@ static int init_chip(struct hfi1_devdata *dd)if(is_ax(dd)){dd_dev_info(dd,"Resetting CSRs with FLR\n");-pcie_flr(dd->pcidev);+pcie_flr(dd->pcidev,0);ret=restore_pci_variables(dd);if(ret){dd_dev_err(dd,"%s: Could not restore PCI variables\n",
@@ -260,7 +260,7 @@ static int octeon_mbox_process_cmd(struct octeon_mbox *mbox,dev_info(&oct->pci_dev->dev,"got a request for FLR from VF that owns DPI ring %u\n",mbox->q_no);-pcie_flr(oct->sriov_info.dpiring_to_vfpcidev_lut[mbox->q_no]);+pcie_flr(oct->sriov_info.dpiring_to_vfpcidev_lut[mbox->q_no],0);break;caseOCTEON_PF_CHANGED_VF_MACADDR:
@@ -4574,33 +4574,13 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)EXPORT_SYMBOL(pci_wait_for_pending_transaction);/**-*pcie_has_flr-checkifadevicesupportsfunctionlevelresets-*@dev:devicetocheck-*-*ReturnstrueifthedeviceadvertisessupportforPCIefunctionlevel-*resets.-*/-boolpcie_has_flr(structpci_dev*dev)-{-u32cap;--if(dev->dev_flags&PCI_DEV_FLAGS_NO_FLR_RESET)-returnfalse;--pcie_capability_read_dword(dev,PCI_EXP_DEVCAP,&cap);-returncap&PCI_EXP_DEVCAP_FLR;-}-EXPORT_SYMBOL_GPL(pcie_has_flr);--/**-*pcie_flr-initiateaPCIefunctionlevelreset+*pcie_reset_flr-initiateaPCIefunctionlevelreset*@dev:devicetoreset*-*Initiateafunctionlevelreseton@dev.Thecallershouldensurethe-*devicesupportsFLRbeforecallingthisfunction,e.g.byusingthe-*pcie_has_flr()helper.+*Initiateafunctionlevelresetunconditionallyon@devwithout+*checkinganyflagsandDEVCAP*/-intpcie_flr(structpci_dev*dev)+intpcie_reset_flr(structpci_dev*dev){if(!pci_wait_for_pending_transaction(dev))pci_err(dev,"timed out waiting for pending transaction; performing function level reset anyway\n");
@@ -4619,6 +4599,30 @@ int pcie_flr(struct pci_dev *dev)returnpci_dev_wait(dev,"FLR",PCIE_RESET_READY_POLL_MS);}++/**+*pcie_flr-initiateaPCIefunctionlevelreset+*@dev:devicetoreset+*@probe:Ifset,onlycheckifthedevicecanberesetthisway.+*+*Initiateafunctionlevelreseton@dev.+*/+intpcie_flr(structpci_dev*dev,intprobe)+{+u32cap;++if(dev->dev_flags&PCI_DEV_FLAGS_NO_FLR_RESET)+return-ENOTTY;++pcie_capability_read_dword(dev,PCI_EXP_DEVCAP,&cap);+if(!(cap&PCI_EXP_DEVCAP_FLR))+return-ENOTTY;++if(probe)+return0;++returnpcie_reset_flr(dev);+}EXPORT_SYMBOL_GPL(pcie_flr);staticintpci_af_flr(structpci_dev*dev,intprobe)
@@ -5091,11 +5095,9 @@ int __pci_reset_function_locked(struct pci_dev *dev)rc=pci_dev_specific_reset(dev,0);if(rc!=-ENOTTY)returnrc;-if(pcie_has_flr(dev)){-rc=pcie_flr(dev);-if(rc!=-ENOTTY)-returnrc;-}+rc=pcie_flr(dev,0);+if(rc!=-ENOTTY)+returnrc;rc=pci_af_flr(dev,0);if(rc!=-ENOTTY)returnrc;
@@ -5129,8 +5131,9 @@ int pci_probe_reset_function(struct pci_dev *dev)rc=pci_dev_specific_reset(dev,1);if(rc!=-ENOTTY)returnrc;-if(pcie_has_flr(dev))-return0;+rc=pcie_flr(dev,1);+if(rc!=-ENOTTY)+returnrc;rc=pci_af_flr(dev,1);if(rc!=-ENOTTY)returnrc;
@@ -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_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",
@@ -121,6 +121,21 @@ 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 <ameynarkhede03@gmail.com>+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. Reading this file will give names+ of the device supported reset methods. Currently used methods+ are enclosed in brackets. Writing the name of any of the device+ supported reset method to this file will set the reset method to+ be used when resetting the device. Writing "none" 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 <mst@redhat.com>
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-14 12:10:36
On Fri, Mar 12, 2021 at 11:04:48PM +0530, ameynarkhede03@gmail.com wrote:
From: Amey Narkhede <redacted>
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.
Sorry, are we talking about specific devices/flows/applications that
must have this functionality or about theoretical use case?
Thanks
Hello Amey! In the list of reset methods is missing PCIe Warm Reset.
Could you extend and prepare API also for PCIe Warm Reset? According to
PCI Express mini card and m.2 electromechanical specifications, PCIe
Warm Reset can be triggered by PERST# signal and more kernel drivers can
internally control PERST#. Just there is no kernel API and therefore
PCIe Warm Reset nor PERST# signal is unified.
quoted hunk
+ { 0 },
+};
+
int isa_dma_bridge_buggy;
EXPORT_SYMBOL(isa_dma_bridge_buggy);
@@ -5080,71 +5100,59 @@ static void pci_dev_restore(struct pci_dev *dev) */ int __pci_reset_function_locked(struct pci_dev *dev) {- int rc;+ int i, rc = -ENOTTY;+ const struct pci_reset_fn_method *reset; might_sleep();- /*- * 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.- */- rc = pci_dev_specific_reset(dev, 0);- if (rc != -ENOTTY)- return rc;- rc = pcie_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;- rc = pci_dev_reset_slot_function(dev, 0);- if (rc != -ENOTTY)- return rc;- return pci_parent_bus_reset(dev, 0);+ for (i = 0, reset = pci_reset_fn_methods; reset->reset_fn; i++, reset++) {+ if (!(dev->reset_methods & (1 << i)))+ continue;++ /*+ * 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.+ */+ rc = reset->reset_fn(dev, 0);+ if (rc != -ENOTTY)+ return rc;+ }+ return rc; } 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 bitmap+ * field 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, rc;+ const struct pci_reset_fn_method *reset;- might_sleep();+ dev->reset_methods = 0;- rc = pci_dev_specific_reset(dev, 1);- if (rc != -ENOTTY)- return rc;- rc = pcie_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;- rc = pci_dev_reset_slot_function(dev, 1);- if (rc != -ENOTTY)- return rc;+ might_sleep();- return pci_parent_bus_reset(dev, 1);+ for (i = 0, reset = pci_reset_fn_methods; reset->reset_fn; i++, reset++) {+ rc = reset->reset_fn(dev, 1);+ if (!rc)+ dev->reset_methods |= (1 << i);+ else if (rc != -ENOTTY)+ break;+ } } /**
On Friday 12 March 2021 23:04:51 ameynarkhede03@gmail.com wrote:
From: Amey Narkhede <redacted>
reset_fn field is used to indicate whether the
device supports any reset mechanism or not.
Deprecate use of reset_fn in favor of new
reset_methods bitmap which can be used to keep
track of all supported reset mechanisms of a device.
Hello Amey!
You cannot trigger PCIe Hot Reset (PCI secondary bus reset) in this
simple way from sysfs via new reset methods.
I proposed very similar functionality just few days ago:
https://lore.kernel.org/linux-pci/20210301171221.3d42a55i7h5ubqsb@pali/T/#u
And I realized that it needs more steps to be done.
At least some remove-reset-rescan procedure done atomically is required.
@@ -121,6 +121,21 @@ 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 <ameynarkhede03@gmail.com>+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. Reading this file will give names+ of the device supported reset methods. Currently used methods+ are enclosed in brackets. Writing the name of any of the device+ supported reset method to this file will set the reset method to+ be used when resetting the device. Writing "none" to this file+ will disable ability to reset the device and writing "default"+ will return to the original value.+
Hello Amey!
I think that this API does not work for PCIe Hot Reset (=PCI secondary
bus reset) and PCIe Warm Reset.
First reset method is bound to the bus, not device and therefore kernel
does not have to see any registered device. So there would be no
"reset_method" sysfs file, and also no "reset" sysfs file. But PCIe Hot
Reset is in most cases needed when buggy card is not registered on bus,
to trigger this reset. And with this API this is not possible.
PCIe Warm Reset is done by PERST# signal. When signal is asserted then
device is in reset state and therefore is not registered. So again
kernel does not have to see registered device.
Moreover for mPCIe form factor cards, boards can share one PERST# signal
with more PCIe cards and control this signal via GPIO. So asserting
PERST# GPIO can trigger Warm reset for more PCIe cards, not just one. It
depends on board or topology.
So... I do not think that current approach with "reset_method" sysfs
entry bound to the PCI device does not work for PCI secondary bus reset
and also cannot be used for implementing PCIe Warm Reset.
I would rather suggest to re-design and prepare a new API which would
work also with PCIe Hot Reset and PCIe Warm Reset.
This "reset" sysfs file can work only with PCI Function Level Reset or
some PM or device specific reset. But not with reset types which are
more like slot or bus orientated.
quoted hunk
What: /sys/bus/pci/devices/.../reset
Date: July 2009
Contact: Michael S. Tsirkin [off-list ref]
@@ -121,6 +121,21 @@ 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 <ameynarkhede03@gmail.com>+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. Reading this file will give names+ of the device supported reset methods. Currently used methods+ are enclosed in brackets. Writing the name of any of the device+ supported reset method to this file will set the reset method to+ be used when resetting the device. Writing "none" to this file+ will disable ability to reset the device and writing "default"+ will return to the original value.+
Hello Amey!
I think that this API does not work for PCIe Hot Reset (=PCI secondary
bus reset) and PCIe Warm Reset.
First reset method is bound to the bus, not device and therefore kernel
does not have to see any registered device. So there would be no
"reset_method" sysfs file, and also no "reset" sysfs file. But PCIe Hot
Reset is in most cases needed when buggy card is not registered on bus,
to trigger this reset. And with this API this is not possible.
PCIe Warm Reset is done by PERST# signal. When signal is asserted then
device is in reset state and therefore is not registered. So again
kernel does not have to see registered device.
Moreover for mPCIe form factor cards, boards can share one PERST# signal
with more PCIe cards and control this signal via GPIO. So asserting
PERST# GPIO can trigger Warm reset for more PCIe cards, not just one. It
depends on board or topology.
So... I do not think that current approach with "reset_method" sysfs
entry bound to the PCI device does not work for PCI secondary bus reset
and also cannot be used for implementing PCIe Warm Reset.
I would rather suggest to re-design and prepare a new API which would
work also with PCIe Hot Reset and PCIe Warm Reset.
This "reset" sysfs file can work only with PCI Function Level Reset or
some PM or device specific reset. But not with reset types which are
more like slot or bus orientated.
The scope of this patch was to expose current reset methods
to the userspace. Also reset methods are available
for only those devices that allow an individual function to be reset
without affecting other functions in the same device.
So if those conditions are satisfied by the device then it can
use slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
Thanks,
Amey
[...]
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
From: Alex Williamson <hidden> Date: 2021-03-15 14:51:18
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
From: Alex Williamson <hidden> Date: 2021-03-15 15:04:26
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-15 15:08:36
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
Thanks,
Amey
From: Alex Williamson <hidden> Date: 2021-03-15 16:30:58
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
CC Alay, Suresh, Shyam and Felipe in case they have anything to add.
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-17 04:21:31
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
CC Alay, Suresh, Shyam and Felipe in case they have anything to add.
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
Thanks,
Amey
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-17 11:03:47
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
IMHO, it will be saner for everyone if virtualization don't try such resets.
Thanks
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
IMHO, it will be saner for everyone if virtualization don't try such resets.
Thanks
The exists reset sysfs attribute was added for exactly this case
though.
Thanks,
Amey
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-17 11:48:46
On Wed, Mar 17, 2021 at 04:53:09PM +0530, Amey Narkhede wrote:
On 21/03/17 01:02PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
I invite such hobbyists to send patches and include their FPGA in
upstream kernel.
quoted
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
I'm talking about netdev state, but whatever given the existence of
sysfs reset knob.
quoted
IMHO, it will be saner for everyone if virtualization don't try such resets.
Thanks
The exists reset sysfs attribute was added for exactly this case
though.
I didn't know the rationale behind that file till you said and I
googled libvirt discussion, so ok. Do you propose that libvirt
will manage database of devices and their working reset types?
I'm not against this patch, just want to raise an attention that the
outcome of this patch will be decrease in fixes of broken devices.
Thanks
On Wed, Mar 17, 2021 at 04:53:09PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:02PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
I invite such hobbyists to send patches and include their FPGA in
upstream kernel.
quoted
quoted
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
I'm talking about netdev state, but whatever given the existence of
sysfs reset knob.
quoted
quoted
IMHO, it will be saner for everyone if virtualization don't try such resets.
Thanks
The exists reset sysfs attribute was added for exactly this case
though.
I didn't know the rationale behind that file till you said and I
googled libvirt discussion, so ok. Do you propose that libvirt
will manage database of devices and their working reset types?
I don't have much idea about internals of libvirt but why would
it need to manage database of working reset types? It could just
read new reset_methods attribute to get the list of supported reset
methods.
I'm not against this patch, just want to raise an attention that the
outcome of this patch will be decrease in fixes of broken devices.
Thanks
That makes sense but that isn't any different from existing reset
attribute. This patch inhances it and allows selecting a device supported
reset method instead of using first available reset method according to
existing hardcoded policy.
Thanks,
Amey
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-17 13:59:43
On Wed, Mar 17, 2021 at 06:47:18PM +0530, Amey Narkhede wrote:
On 21/03/17 01:47PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 04:53:09PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:02PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
I invite such hobbyists to send patches and include their FPGA in
upstream kernel.
quoted
quoted
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
I'm talking about netdev state, but whatever given the existence of
sysfs reset knob.
quoted
quoted
IMHO, it will be saner for everyone if virtualization don't try such resets.
Thanks
The exists reset sysfs attribute was added for exactly this case
though.
I didn't know the rationale behind that file till you said and I
googled libvirt discussion, so ok. Do you propose that libvirt
will manage database of devices and their working reset types?
I don't have much idea about internals of libvirt but why would
it need to manage database of working reset types? It could just
read new reset_methods attribute to get the list of supported reset
methods.
Because the idea of this patch is to read all supported reset types and
allow to the user to chose the working one. The user will do it with
help from StackOverflow, but libvirt will need to have some sort of
database, otherwise it won't be different from simple "echo 1 > reset"
which will iterate over all supported resets anyway.
quoted
I'm not against this patch, just want to raise an attention that the
outcome of this patch will be decrease in fixes of broken devices.
Thanks
That makes sense but that isn't any different from existing reset
attribute. This patch inhances it and allows selecting a device supported
reset method instead of using first available reset method according to
existing hardcoded policy.
The difference here is that this is a workaround to solve bugs that
should be fixed in the kernel.
Thanks
From: Alex Williamson <hidden> Date: 2021-03-17 17:32:33
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
On Wed, Mar 17, 2021 at 06:47:18PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:47PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 04:53:09PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:02PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
We can increase the annoyance factor of using a modified set of reset
methods, but ultimately we can only control what goes into our kernel,
other kernels might take v1 of this series and incorporate it
regardless of what happens here.
quoted
quoted
quoted
quoted
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
I invite such hobbyists to send patches and include their FPGA in
upstream kernel.
This is potentially another good use case, how receptive are we going
to be to an FPGA design that botches a reset. Do they have a valid
device ID for us to base a quirk on, are they just squatting on one, or
using the default from a library. Maybe the next bitstream will
resolve it, maybe without any external indication. IOW, what would the
quality level be for that quirk versus using this as a workaround,
where the user probably wouldn't mind a kernel nag?
quoted
quoted
quoted
quoted
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
I'm talking about netdev state, but whatever given the existence of
sysfs reset knob.
quoted
quoted
IMHO, it will be saner for everyone if virtualization don't try such resets.
That would cause a massive regression in device assignment support. As
with other sysfs attributes, triggering them alongside a running driver
is probably not going to end well. However, pci_reset_function() is
extremely useful for stopping devices and returning them to a default
state, when either rebooting a VM or returning the device to the host.
The device is not removed and re-probed when this occurs, vfio-pci is
able to hold onto the device across these actions. Sure, don't reset a
netdev device when it's in use, that's not what these are used for.
quoted
quoted
quoted
The exists reset sysfs attribute was added for exactly this case
though.
I didn't know the rationale behind that file till you said and I
googled libvirt discussion, so ok. Do you propose that libvirt
will manage database of devices and their working reset types?
I don't have much idea about internals of libvirt but why would
it need to manage database of working reset types? It could just
read new reset_methods attribute to get the list of supported reset
methods.
Because the idea of this patch is to read all supported reset types and
allow to the user to chose the working one. The user will do it with
help from StackOverflow, but libvirt will need to have some sort of
database, otherwise it won't be different from simple "echo 1 > reset"
which will iterate over all supported resets anyway.
AFAIK, libvirt no longer attempts to do resets itself, or is at least
moving in that direction. vfio-pci will reset as device when they're
opened by a user (when available) or triggered via the API.
quoted
quoted
I'm not against this patch, just want to raise an attention that the
outcome of this patch will be decrease in fixes of broken devices.
Thanks
That makes sense but that isn't any different from existing reset
attribute. This patch inhances it and allows selecting a device supported
reset method instead of using first available reset method according to
existing hardcoded policy.
The difference here is that this is a workaround to solve bugs that
should be fixed in the kernel.
If we want to discourage using this as a primary means to resolve reset
issues on a device then we can create log warnings any time it's used.
Downstreams that really want this functionality are going to take this
patch from the list whether we accept it or not. As above, it seems
there are valid use cases. Even with mainstream vfio in QEMU, I go
through some hoops trying to determine if I can do a secondary bus
reset rather than a PM reset because it's not specified anywhere what a
"soft reset" means for any given device. This sort of interface could
make it easier to apply a system policy that a pci_reset_function()
should always perform a secondary bus reset if the only other option is
a PM reset. Maybe that policy mostly makes sense for a VM use case, so
we'd want one policy by default and another when the device is used for
this functionality. How could we accomplish that with a quirk? Thanks,
Alex
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
Can somebody test these changes on some PCIe hotplug controller what
secondary bus reset via sysfs would do? Because currently it is not
exported as reset method and there can be different race conditions and
maybe error (?) if hotplug code is going to remove device on which user
triggered bus reset via sysfs.
And in my opinion this can happen also in case when only one device is
on the bus, so it perfectly matches all conditions when sysfs can use
bus reset for one device.
I can try to implement hotplug code into aardvark driver and root bridge
emulator to test how this patch would happen. But it would take some
time...
From: Alex Williamson <hidden> Date: 2021-03-17 19:16:20
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Alex
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
From: Alex Williamson <hidden> Date: 2021-03-17 19:33:47
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected. Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
Alex
On Wednesday 17 March 2021 13:32:45 Alex Williamson wrote:
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected.
Yes, this makes slot reset "safe". But bus reset is "unsafe".
Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
That should work when slot reset is available.
Other option is that mentioned remove-reset-rescan procedure.
But quick search in drivers/pci/hotplug/ results that not all hotplug
drivers implement reset_slot method.
So there is a possible issue with hotplug driver which may eject device
during bus reset (because e.g. slot reset is not implemented)?
From: Alex Williamson <hidden> Date: 2021-03-17 20:01:18
On Wed, 17 Mar 2021 20:40:24 +0100
Pali Rohár [off-list ref] wrote:
On Wednesday 17 March 2021 13:32:45 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected.
Yes, this makes slot reset "safe". But bus reset is "unsafe".
quoted
Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
That should work when slot reset is available.
Other option is that mentioned remove-reset-rescan procedure.
That's not something we can introduce to the pci_reset_function() path
without a fair bit of collateral in using it through vfio-pci.
But quick search in drivers/pci/hotplug/ results that not all hotplug
drivers implement reset_slot method.
So there is a possible issue with hotplug driver which may eject device
during bus reset (because e.g. slot reset is not implemented)?
People aren't reporting it, so maybe those controllers aren't being
used for this use case. Or maybe introducing this patch will make
these reset methods more readily accessible for testing. We can fix or
blacklist those controllers for bus reset when reports come in. Thanks,
Alex
On Wednesday 17 March 2021 14:00:20 Alex Williamson wrote:
On Wed, 17 Mar 2021 20:40:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:32:45 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected.
Yes, this makes slot reset "safe". But bus reset is "unsafe".
quoted
Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
That should work when slot reset is available.
Other option is that mentioned remove-reset-rescan procedure.
That's not something we can introduce to the pci_reset_function() path
without a fair bit of collateral in using it through vfio-pci.
quoted
But quick search in drivers/pci/hotplug/ results that not all hotplug
drivers implement reset_slot method.
So there is a possible issue with hotplug driver which may eject device
during bus reset (because e.g. slot reset is not implemented)?
People aren't reporting it, so maybe those controllers aren't being
used for this use case. Or maybe introducing this patch will make
these reset methods more readily accessible for testing. We can fix or
blacklist those controllers for bus reset when reports come in. Thanks,
Ok! I do not know neither if those controllers are used, but looks like
that there are still changes in hotplug code.
So I guess with these patches people can test it and report issues when
such thing happen.
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-18 09:10:16
On Wed, Mar 17, 2021 at 11:31:40AM -0600, Alex Williamson wrote:
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Wed, Mar 17, 2021 at 06:47:18PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:47PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 04:53:09PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:02PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
We can increase the annoyance factor of using a modified set of reset
methods, but ultimately we can only control what goes into our kernel,
other kernels might take v1 of this series and incorporate it
regardless of what happens here.
quoted
quoted
quoted
quoted
quoted
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
I invite such hobbyists to send patches and include their FPGA in
upstream kernel.
This is potentially another good use case, how receptive are we going
to be to an FPGA design that botches a reset. Do they have a valid
device ID for us to base a quirk on, are they just squatting on one, or
using the default from a library. Maybe the next bitstream will
resolve it, maybe without any external indication. IOW, what would the
quality level be for that quirk versus using this as a workaround,
where the user probably wouldn't mind a kernel nag?
It is worth to solve it when the need arises.
quoted
quoted
quoted
quoted
quoted
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
I'm talking about netdev state, but whatever given the existence of
sysfs reset knob.
quoted
quoted
IMHO, it will be saner for everyone if virtualization don't try such resets.
That would cause a massive regression in device assignment support. As
with other sysfs attributes, triggering them alongside a running driver
is probably not going to end well. However, pci_reset_function() is
extremely useful for stopping devices and returning them to a default
state, when either rebooting a VM or returning the device to the host.
The device is not removed and re-probed when this occurs, vfio-pci is
able to hold onto the device across these actions. Sure, don't reset a
netdev device when it's in use, that's not what these are used for.
quoted
quoted
quoted
quoted
The exists reset sysfs attribute was added for exactly this case
though.
I didn't know the rationale behind that file till you said and I
googled libvirt discussion, so ok. Do you propose that libvirt
will manage database of devices and their working reset types?
I don't have much idea about internals of libvirt but why would
it need to manage database of working reset types? It could just
read new reset_methods attribute to get the list of supported reset
methods.
Because the idea of this patch is to read all supported reset types and
allow to the user to chose the working one. The user will do it with
help from StackOverflow, but libvirt will need to have some sort of
database, otherwise it won't be different from simple "echo 1 > reset"
which will iterate over all supported resets anyway.
AFAIK, libvirt no longer attempts to do resets itself, or is at least
moving in that direction. vfio-pci will reset as device when they're
opened by a user (when available) or triggered via the API.
<...>
quoted
The difference here is that this is a workaround to solve bugs that
should be fixed in the kernel.
If we want to discourage using this as a primary means to resolve reset
issues on a device then we can create log warnings any time it's used.
Downstreams that really want this functionality are going to take this
patch from the list whether we accept it or not. As above, it seems
there are valid use cases. Even with mainstream vfio in QEMU, I go
through some hoops trying to determine if I can do a secondary bus
reset rather than a PM reset because it's not specified anywhere what a
"soft reset" means for any given device. This sort of interface could
make it easier to apply a system policy that a pci_reset_function()
should always perform a secondary bus reset if the only other option is
a PM reset. Maybe that policy mostly makes sense for a VM use case, so
we'd want one policy by default and another when the device is used for
this functionality. How could we accomplish that with a quirk? Thanks,
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
Thanks
On Wed, Mar 17, 2021 at 11:31:40AM -0600, Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Wed, Mar 17, 2021 at 06:47:18PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:47PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 04:53:09PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:02PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
We can increase the annoyance factor of using a modified set of reset
methods, but ultimately we can only control what goes into our kernel,
other kernels might take v1 of this series and incorporate it
regardless of what happens here.
quoted
quoted
quoted
quoted
quoted
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
I invite such hobbyists to send patches and include their FPGA in
upstream kernel.
This is potentially another good use case, how receptive are we going
to be to an FPGA design that botches a reset. Do they have a valid
device ID for us to base a quirk on, are they just squatting on one, or
using the default from a library. Maybe the next bitstream will
resolve it, maybe without any external indication. IOW, what would the
quality level be for that quirk versus using this as a workaround,
where the user probably wouldn't mind a kernel nag?
It is worth to solve it when the need arises.
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
I'm talking about netdev state, but whatever given the existence of
sysfs reset knob.
quoted
quoted
IMHO, it will be saner for everyone if virtualization don't try such resets.
That would cause a massive regression in device assignment support. As
with other sysfs attributes, triggering them alongside a running driver
is probably not going to end well. However, pci_reset_function() is
extremely useful for stopping devices and returning them to a default
state, when either rebooting a VM or returning the device to the host.
The device is not removed and re-probed when this occurs, vfio-pci is
able to hold onto the device across these actions. Sure, don't reset a
netdev device when it's in use, that's not what these are used for.
quoted
quoted
quoted
quoted
The exists reset sysfs attribute was added for exactly this case
though.
I didn't know the rationale behind that file till you said and I
googled libvirt discussion, so ok. Do you propose that libvirt
will manage database of devices and their working reset types?
I don't have much idea about internals of libvirt but why would
it need to manage database of working reset types? It could just
read new reset_methods attribute to get the list of supported reset
methods.
Because the idea of this patch is to read all supported reset types and
allow to the user to chose the working one. The user will do it with
help from StackOverflow, but libvirt will need to have some sort of
database, otherwise it won't be different from simple "echo 1 > reset"
which will iterate over all supported resets anyway.
AFAIK, libvirt no longer attempts to do resets itself, or is at least
moving in that direction. vfio-pci will reset as device when they're
opened by a user (when available) or triggered via the API.
<...>
quoted
quoted
The difference here is that this is a workaround to solve bugs that
should be fixed in the kernel.
If we want to discourage using this as a primary means to resolve reset
issues on a device then we can create log warnings any time it's used.
Downstreams that really want this functionality are going to take this
patch from the list whether we accept it or not. As above, it seems
there are valid use cases. Even with mainstream vfio in QEMU, I go
through some hoops trying to determine if I can do a secondary bus
reset rather than a PM reset because it's not specified anywhere what a
"soft reset" means for any given device. This sort of interface could
make it easier to apply a system policy that a pci_reset_function()
should always perform a secondary bus reset if the only other option is
a PM reset. Maybe that policy mostly makes sense for a VM use case, so
we'd want one policy by default and another when the device is used for
this functionality. How could we accomplish that with a quirk? Thanks,
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
Thanks
Can you explain bit more about why supported != working?
Why would hardware indicate that it supports specific reset
method if it doesn't work? There is only an unsual quirk for intel
82599 which supports FLR but only reports in PF DEVCAP not in
VF DEVCAP so we need to directly call FLR without checking if it
is supported.
Thanks,
Amey
On Wednesday 17 March 2021 14:00:20 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:40:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:32:45 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected.
Yes, this makes slot reset "safe". But bus reset is "unsafe".
quoted
Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
That should work when slot reset is available.
Other option is that mentioned remove-reset-rescan procedure.
That's not something we can introduce to the pci_reset_function() path
without a fair bit of collateral in using it through vfio-pci.
quoted
But quick search in drivers/pci/hotplug/ results that not all hotplug
drivers implement reset_slot method.
So there is a possible issue with hotplug driver which may eject device
during bus reset (because e.g. slot reset is not implemented)?
People aren't reporting it, so maybe those controllers aren't being
used for this use case. Or maybe introducing this patch will make
these reset methods more readily accessible for testing. We can fix or
blacklist those controllers for bus reset when reports come in. Thanks,
Ok! I do not know neither if those controllers are used, but looks like
that there are still changes in hotplug code.
So I guess with these patches people can test it and report issues when
such thing happen.
So after a bit research as I understood we need to group slot
and bus reset together in a single category of reset methods and
then implicitly use slot reset if it is available when bus reset is
enabled by the user.
Is that right?
Thanks,
Amey
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-18 14:58:34
On Thu, Mar 18, 2021 at 07:52:52PM +0530, Amey Narkhede wrote:
On 21/03/18 11:09AM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 11:31:40AM -0600, Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Wed, Mar 17, 2021 at 06:47:18PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:47PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 04:53:09PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:02PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
We can increase the annoyance factor of using a modified set of reset
methods, but ultimately we can only control what goes into our kernel,
other kernels might take v1 of this series and incorporate it
regardless of what happens here.
quoted
quoted
quoted
quoted
quoted
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
I invite such hobbyists to send patches and include their FPGA in
upstream kernel.
This is potentially another good use case, how receptive are we going
to be to an FPGA design that botches a reset. Do they have a valid
device ID for us to base a quirk on, are they just squatting on one, or
using the default from a library. Maybe the next bitstream will
resolve it, maybe without any external indication. IOW, what would the
quality level be for that quirk versus using this as a workaround,
where the user probably wouldn't mind a kernel nag?
It is worth to solve it when the need arises.
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
I'm talking about netdev state, but whatever given the existence of
sysfs reset knob.
quoted
quoted
IMHO, it will be saner for everyone if virtualization don't try such resets.
That would cause a massive regression in device assignment support. As
with other sysfs attributes, triggering them alongside a running driver
is probably not going to end well. However, pci_reset_function() is
extremely useful for stopping devices and returning them to a default
state, when either rebooting a VM or returning the device to the host.
The device is not removed and re-probed when this occurs, vfio-pci is
able to hold onto the device across these actions. Sure, don't reset a
netdev device when it's in use, that's not what these are used for.
quoted
quoted
quoted
quoted
The exists reset sysfs attribute was added for exactly this case
though.
I didn't know the rationale behind that file till you said and I
googled libvirt discussion, so ok. Do you propose that libvirt
will manage database of devices and their working reset types?
I don't have much idea about internals of libvirt but why would
it need to manage database of working reset types? It could just
read new reset_methods attribute to get the list of supported reset
methods.
Because the idea of this patch is to read all supported reset types and
allow to the user to chose the working one. The user will do it with
help from StackOverflow, but libvirt will need to have some sort of
database, otherwise it won't be different from simple "echo 1 > reset"
which will iterate over all supported resets anyway.
AFAIK, libvirt no longer attempts to do resets itself, or is at least
moving in that direction. vfio-pci will reset as device when they're
opened by a user (when available) or triggered via the API.
<...>
quoted
quoted
The difference here is that this is a workaround to solve bugs that
should be fixed in the kernel.
If we want to discourage using this as a primary means to resolve reset
issues on a device then we can create log warnings any time it's used.
Downstreams that really want this functionality are going to take this
patch from the list whether we accept it or not. As above, it seems
there are valid use cases. Even with mainstream vfio in QEMU, I go
through some hoops trying to determine if I can do a secondary bus
reset rather than a PM reset because it's not specified anywhere what a
"soft reset" means for any given device. This sort of interface could
make it easier to apply a system policy that a pci_reset_function()
should always perform a secondary bus reset if the only other option is
a PM reset. Maybe that policy mostly makes sense for a VM use case, so
we'd want one policy by default and another when the device is used for
this functionality. How could we accomplish that with a quirk? Thanks,
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
Thanks
Can you explain bit more about why supported != working?
It is written in the commit message of this patch.
https://lore.kernel.org/lkml/20210312173452.3855-1-ameynarkhede03@gmail.com/
"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."
You wrote it and also repeated it a couple of times during the discussion.
If device can understand that specific reset doesn't work, it won't
perform it in first place.
Thanks
From: Alex Williamson <hidden> Date: 2021-03-18 16:40:25
On Thu, 18 Mar 2021 11:09:34 +0200
Leon Romanovsky [off-list ref] wrote:
On Wed, Mar 17, 2021 at 11:31:40AM -0600, Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Wed, Mar 17, 2021 at 06:47:18PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:47PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 04:53:09PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:02PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
We can increase the annoyance factor of using a modified set of reset
methods, but ultimately we can only control what goes into our kernel,
other kernels might take v1 of this series and incorporate it
regardless of what happens here.
quoted
quoted
quoted
quoted
quoted
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
I invite such hobbyists to send patches and include their FPGA in
upstream kernel.
This is potentially another good use case, how receptive are we going
to be to an FPGA design that botches a reset. Do they have a valid
device ID for us to base a quirk on, are they just squatting on one, or
using the default from a library. Maybe the next bitstream will
resolve it, maybe without any external indication. IOW, what would the
quality level be for that quirk versus using this as a workaround,
where the user probably wouldn't mind a kernel nag?
It is worth to solve it when the need arises.
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
I'm talking about netdev state, but whatever given the existence of
sysfs reset knob.
quoted
quoted
IMHO, it will be saner for everyone if virtualization don't try such resets.
That would cause a massive regression in device assignment support. As
with other sysfs attributes, triggering them alongside a running driver
is probably not going to end well. However, pci_reset_function() is
extremely useful for stopping devices and returning them to a default
state, when either rebooting a VM or returning the device to the host.
The device is not removed and re-probed when this occurs, vfio-pci is
able to hold onto the device across these actions. Sure, don't reset a
netdev device when it's in use, that's not what these are used for.
quoted
quoted
quoted
quoted
The exists reset sysfs attribute was added for exactly this case
though.
I didn't know the rationale behind that file till you said and I
googled libvirt discussion, so ok. Do you propose that libvirt
will manage database of devices and their working reset types?
I don't have much idea about internals of libvirt but why would
it need to manage database of working reset types? It could just
read new reset_methods attribute to get the list of supported reset
methods.
Because the idea of this patch is to read all supported reset types and
allow to the user to chose the working one. The user will do it with
help from StackOverflow, but libvirt will need to have some sort of
database, otherwise it won't be different from simple "echo 1 > reset"
which will iterate over all supported resets anyway.
AFAIK, libvirt no longer attempts to do resets itself, or is at least
moving in that direction. vfio-pci will reset as device when they're
opened by a user (when available) or triggered via the API.
<...>
quoted
quoted
The difference here is that this is a workaround to solve bugs that
should be fixed in the kernel.
If we want to discourage using this as a primary means to resolve reset
issues on a device then we can create log warnings any time it's used.
Downstreams that really want this functionality are going to take this
patch from the list whether we accept it or not. As above, it seems
there are valid use cases. Even with mainstream vfio in QEMU, I go
through some hoops trying to determine if I can do a secondary bus
reset rather than a PM reset because it's not specified anywhere what a
"soft reset" means for any given device. This sort of interface could
make it easier to apply a system policy that a pci_reset_function()
should always perform a secondary bus reset if the only other option is
a PM reset. Maybe that policy mostly makes sense for a VM use case, so
we'd want one policy by default and another when the device is used for
this functionality. How could we accomplish that with a quirk? Thanks,
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
vfio-pci uses the internal kernel API, ie. the variants of
pci_reset_function(), which is the same interface used by the existing
sysfs reset mechanism. This proposed configuration of the reset method
would affect any driver using that same core infrastructure and from my
perspective that's really the goal. In the case where a supported
reset mechanism fails for a device, continuing to quirk those out for
the best default behavior makes sense, I'd be disappointed for a vendor
to not pursue improving the default behavior where it clearly makes
sense. However, there's also a policy decision, the kernel imposes a
preferential ordering of reset mechanism. Is that ordering the best
case for all users? I've presented above a case where a userspace may
prefer a policy of preferring a bus reset to a PM reset. So I think
the question is not only are there supported mechanisms that don't
work, where this interface allows userspace to more readily identify
and work around those sorts of issues, but it also enables user
preference and easier evaluation whether all of the supported reset
mechanisms work rather than just the first one we encounter in the
ordering we've decided to impose today. Thanks,
Alex
On Thu, Mar 18, 2021 at 07:52:52PM +0530, Amey Narkhede wrote:
quoted
On 21/03/18 11:09AM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 11:31:40AM -0600, Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Wed, Mar 17, 2021 at 06:47:18PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:47PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 04:53:09PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 01:02PM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 03:54:47PM +0530, Amey Narkhede wrote:
quoted
On 21/03/17 06:20AM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 06:32:32PM +0000, Raphael Norwitz wrote:
quoted
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 21:03:41 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/15 05:07PM, Leon Romanovsky wrote:
quoted
On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here. Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex,
I asked the patch author here [1], but didn't get any response, maybe
you can answer me. What is the use case scenario for this functionality?
Thanks
[1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/
Sorry for not responding immediately. There were some buggy wifi cards
which needed FLR explicitly not sure if that behavior is fixed in
drivers. Also there is use a case at Nutanix but the engineer who
is involved is on PTO that is why I did not respond immediately as
I don't know the details yet.
And more generally, devices continue to have reset issues and we
impose a fixed priority in our ordering. We can and probably should
continue to quirk devices when we find broken resets so that we have
the best default behavior, but it's currently not easy for an end user
to experiment, ie. this reset works, that one doesn't. We might also
have platform issues where a given reset works better on a certain
platform. Exposing a way to test these things might lead to better
quirks. In the case I think Pali was looking for, they wanted a
mechanism to force a bus reset, if this was in reference to a single
function device, this could be accomplished by setting a priority for
that mechanism, which would translate to not only the sysfs reset
attribute, but also the reset mechanism used by vfio-pci. Thanks,
Alex
To confirm from our end - we have seen many such instances where default
reset methods have not worked well on our platform. Debugging these
issues is painful in practice, and this interface would make it far
easier.
Having an interface like this would also help us better communicate the
issues we find with upstream. Allowing others to more easily test our
(or other entities') findings should give better visibility into
which issues apply to the device in general and which are platform
specific. In disambiguating the former from the latter, we should be
able to better quirk devices for everyone, and in the latter cases, this
interface allows for a safer and more elegant solution than any of the
current alternatives.
So to summarize, we are talking about test and debug interface to
overcome HW bugs, am I right?
My personal experience shows that once the easy workaround exists
(and write to generally available sysfs is very simple), the vendors
and users desire for proper fix decreases drastically. IMHO, we will
see increase of copy/paste in SO and blog posts, but reduce in quirks.
My 2-cents.
I agree with your point but at least it gives the userspace ability
to use broken device until bug is fixed in upstream.
As I said, I don't expect many fixes once "userspace" will be able to
use cheap workaround. There is no incentive to fix it.
We can increase the annoyance factor of using a modified set of reset
methods, but ultimately we can only control what goes into our kernel,
other kernels might take v1 of this series and incorporate it
regardless of what happens here.
quoted
quoted
quoted
quoted
quoted
quoted
This is also applicable for obscure devices without upstream
drivers for example custom FPGA based devices.
This is not relevant to upstream kernel. Those vendors ship everything
custom, they don't need upstream, we don't need them :)
By custom I meant hobbyists who could tinker with their custom FPGA.
I invite such hobbyists to send patches and include their FPGA in
upstream kernel.
This is potentially another good use case, how receptive are we going
to be to an FPGA design that botches a reset. Do they have a valid
device ID for us to base a quirk on, are they just squatting on one, or
using the default from a library. Maybe the next bitstream will
resolve it, maybe without any external indication. IOW, what would the
quality level be for that quirk versus using this as a workaround,
where the user probably wouldn't mind a kernel nag?
It is worth to solve it when the need arises.
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Another main application which I forgot to mention is virtualization
where vmm wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.
It can work in very narrow case, because reset will cause to device
reprobe and most likely the driver will be different from the one that
started reset. I can imagine that net devices will lose their state and
config after such reset too.
Not sure if I got that 100% right. The pci_reset_function() function
saves and restores device state over the reset.
I'm talking about netdev state, but whatever given the existence of
sysfs reset knob.
quoted
quoted
IMHO, it will be saner for everyone if virtualization don't try such resets.
That would cause a massive regression in device assignment support. As
with other sysfs attributes, triggering them alongside a running driver
is probably not going to end well. However, pci_reset_function() is
extremely useful for stopping devices and returning them to a default
state, when either rebooting a VM or returning the device to the host.
The device is not removed and re-probed when this occurs, vfio-pci is
able to hold onto the device across these actions. Sure, don't reset a
netdev device when it's in use, that's not what these are used for.
quoted
quoted
quoted
quoted
The exists reset sysfs attribute was added for exactly this case
though.
I didn't know the rationale behind that file till you said and I
googled libvirt discussion, so ok. Do you propose that libvirt
will manage database of devices and their working reset types?
I don't have much idea about internals of libvirt but why would
it need to manage database of working reset types? It could just
read new reset_methods attribute to get the list of supported reset
methods.
Because the idea of this patch is to read all supported reset types and
allow to the user to chose the working one. The user will do it with
help from StackOverflow, but libvirt will need to have some sort of
database, otherwise it won't be different from simple "echo 1 > reset"
which will iterate over all supported resets anyway.
AFAIK, libvirt no longer attempts to do resets itself, or is at least
moving in that direction. vfio-pci will reset as device when they're
opened by a user (when available) or triggered via the API.
<...>
quoted
quoted
The difference here is that this is a workaround to solve bugs that
should be fixed in the kernel.
If we want to discourage using this as a primary means to resolve reset
issues on a device then we can create log warnings any time it's used.
Downstreams that really want this functionality are going to take this
patch from the list whether we accept it or not. As above, it seems
there are valid use cases. Even with mainstream vfio in QEMU, I go
through some hoops trying to determine if I can do a secondary bus
reset rather than a PM reset because it's not specified anywhere what a
"soft reset" means for any given device. This sort of interface could
make it easier to apply a system policy that a pci_reset_function()
should always perform a secondary bus reset if the only other option is
a PM reset. Maybe that policy mostly makes sense for a VM use case, so
we'd want one policy by default and another when the device is used for
this functionality. How could we accomplish that with a quirk? Thanks,
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
Thanks
Can you explain bit more about why supported != working?
It is written in the commit message of this patch.
https://lore.kernel.org/lkml/20210312173452.3855-1-ameynarkhede03@gmail.com/
"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."
You wrote it and also repeated it a couple of times during the discussion.
If device can understand that specific reset doesn't work, it won't
perform it in first place.
Thanks
Is it possible for device to understand whether or not specific reset
will work or not prior to performing reset and after it indicates
support for that reset method? Maybe theres problem with that particular
piece of hardware in that machine.
How can database be maintained if a particular machines have
particular piece of faulty HW?
If for some reason reset doesn't work it will just give -ENOTTY.
This isn't any different from existing behavior.Actually it informs user
that the reset method didn't reset the device and user can use different
reset method instead of implicitly using different reset method.
If user doesn't explicitly set preferred reset method then
we go ahead with existing implicit fall through behavior which will try all
available reset methods until any one of them works.
If you have device that doesn't support reset at all then you have
option to completely disable it unlike existing reset attribute where
you cannot disable reset. So it gives greater control where you can
disable the reset altogether when quirk isn't developed yet.
We can't expect to develop quirk for every device in existence.
For example on my laptop elantech touchpad still doesn't work in 2021
with vanilla kernel, arch linux applies the patch which was reverted in
mainline kernel for some reason.
Thanks,
Amey
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-18 17:23:23
On Thu, Mar 18, 2021 at 10:39:35AM -0600, Alex Williamson wrote:
On Thu, 18 Mar 2021 11:09:34 +0200
Leon Romanovsky [off-list ref] wrote:
<...>
quoted
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
vfio-pci uses the internal kernel API, ie. the variants of
pci_reset_function(), which is the same interface used by the existing
sysfs reset mechanism. This proposed configuration of the reset method
would affect any driver using that same core infrastructure and from my
perspective that's really the goal. In the case where a supported
reset mechanism fails for a device, continuing to quirk those out for
the best default behavior makes sense, I'd be disappointed for a vendor
to not pursue improving the default behavior where it clearly makes
sense. However, there's also a policy decision, the kernel imposes a
preferential ordering of reset mechanism. Is that ordering the best
case for all users? I've presented above a case where a userspace may
prefer a policy of preferring a bus reset to a PM reset. So I think
the question is not only are there supported mechanisms that don't
work, where this interface allows userspace to more readily identify
and work around those sorts of issues, but it also enables user
preference and easier evaluation whether all of the supported reset
mechanisms work rather than just the first one we encounter in the
ordering we've decided to impose today. Thanks,
Alex,
Which email client do you use?
Your responses are grouped as one huge block without any chance to respond
to you on specific point or answer to your question.
I see your flow and understand your position, but will repeat my
position. We need to make sure that vendors will have incentive to
supply quirks.
And regarding vendors, see Amey response below about his touchpad troubles.
The cheap electronics vendors don't care about their users.
Thanks
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-18 17:36:16
On Thu, Mar 18, 2021 at 10:31:43PM +0530, Amey Narkhede wrote:
On 21/03/18 04:57PM, Leon Romanovsky wrote:
quoted
On Thu, Mar 18, 2021 at 07:52:52PM +0530, Amey Narkhede wrote:
quoted
On 21/03/18 11:09AM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 11:31:40AM -0600, Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
<...>
quoted
quoted
quoted
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
Thanks
Can you explain bit more about why supported != working?
It is written in the commit message of this patch.
https://lore.kernel.org/lkml/20210312173452.3855-1-ameynarkhede03@gmail.com/
"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."
You wrote it and also repeated it a couple of times during the discussion.
If device can understand that specific reset doesn't work, it won't
perform it in first place.
Thanks
Is it possible for device to understand whether or not specific reset
will work or not prior to performing reset and after it indicates
support for that reset method? Maybe theres problem with that particular
piece of hardware in that machine.
How can database be maintained if a particular machines have
particular piece of faulty HW?
It was exactly the reason why I think that VM usecase presented by
you is not viable.
If for some reason reset doesn't work it will just give -ENOTTY.
This isn't any different from existing behavior.Actually it informs user
that the reset method didn't reset the device and user can use different
reset method instead of implicitly using different reset method.
If user doesn't explicitly set preferred reset method then
we go ahead with existing implicit fall through behavior which will try all
available reset methods until any one of them works.
If you have device that doesn't support reset at all then you have
option to completely disable it unlike existing reset attribute where
you cannot disable reset. So it gives greater control where you can
disable the reset altogether when quirk isn't developed yet.
I explicitly asked to hear usecase, right now, I got an explanation from
Alex for policy decision (which doesn't need sysfs) and from you about
overcoming HW bugs with expectation that user will be guru of PCI reset
methods.
We can't expect to develop quirk for every device in existence.
It doesn't give us an excuse do not try.
For example on my laptop elantech touchpad still doesn't work in 2021
with vanilla kernel, arch linux applies the patch which was reverted in
mainline kernel for some reason.
I see it as a good example of cheap solution. Vendor won't fix your
touchpad because distros provide workaround. The same will be with reset.
Thanks
On Thu, Mar 18, 2021 at 10:39:35AM -0600, Alex Williamson wrote:
quoted
On Thu, 18 Mar 2021 11:09:34 +0200
Leon Romanovsky [off-list ref] wrote:
<...>
quoted
quoted
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
vfio-pci uses the internal kernel API, ie. the variants of
pci_reset_function(), which is the same interface used by the existing
sysfs reset mechanism. This proposed configuration of the reset method
would affect any driver using that same core infrastructure and from my
perspective that's really the goal. In the case where a supported
reset mechanism fails for a device, continuing to quirk those out for
the best default behavior makes sense, I'd be disappointed for a vendor
to not pursue improving the default behavior where it clearly makes
sense. However, there's also a policy decision, the kernel imposes a
preferential ordering of reset mechanism. Is that ordering the best
case for all users? I've presented above a case where a userspace may
prefer a policy of preferring a bus reset to a PM reset. So I think
the question is not only are there supported mechanisms that don't
work, where this interface allows userspace to more readily identify
and work around those sorts of issues, but it also enables user
preference and easier evaluation whether all of the supported reset
mechanisms work rather than just the first one we encounter in the
ordering we've decided to impose today. Thanks,
[...]
And regarding vendors, see Amey response below about his touchpad troubles.
The cheap electronics vendors don't care about their users.
Thanks
On the side note that vendor probably doesn't care about
Linux users because even that reverted patch was submitted
by community member.
Many vendors are satisfied with windows only drivers.
They don't have any reason to support Linux. That doesn't
mean we should also abandon those users.
Thanks,
Amey
On Thu, Mar 18, 2021 at 10:31:43PM +0530, Amey Narkhede wrote:
quoted
On 21/03/18 04:57PM, Leon Romanovsky wrote:
quoted
On Thu, Mar 18, 2021 at 07:52:52PM +0530, Amey Narkhede wrote:
quoted
On 21/03/18 11:09AM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 11:31:40AM -0600, Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
<...>
quoted
quoted
quoted
quoted
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
Thanks
Can you explain bit more about why supported != working?
It is written in the commit message of this patch.
https://lore.kernel.org/lkml/20210312173452.3855-1-ameynarkhede03@gmail.com/
"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."
You wrote it and also repeated it a couple of times during the discussion.
If device can understand that specific reset doesn't work, it won't
perform it in first place.
Thanks
Is it possible for device to understand whether or not specific reset
will work or not prior to performing reset and after it indicates
support for that reset method? Maybe theres problem with that particular
piece of hardware in that machine.
How can database be maintained if a particular machines have
particular piece of faulty HW?
It was exactly the reason why I think that VM usecase presented by
you is not viable.
Well I didn't present it as new use case. I just gave existing
usecase based on existing reset attribute. Nothing new here.
Nothing really changes wrt that use case.
quoted
If for some reason reset doesn't work it will just give -ENOTTY.
This isn't any different from existing behavior.Actually it informs user
that the reset method didn't reset the device and user can use different
reset method instead of implicitly using different reset method.
If user doesn't explicitly set preferred reset method then
we go ahead with existing implicit fall through behavior which will try all
available reset methods until any one of them works.
If you have device that doesn't support reset at all then you have
option to completely disable it unlike existing reset attribute where
you cannot disable reset. So it gives greater control where you can
disable the reset altogether when quirk isn't developed yet.
I explicitly asked to hear usecase, right now, I got an explanation from
Alex for policy decision (which doesn't need sysfs) and from you about
overcoming HW bugs with expectation that user will be guru of PCI reset
methods.
quoted
We can't expect to develop quirk for every device in existence.
It doesn't give us an excuse do not try.
quoted
For example on my laptop elantech touchpad still doesn't work in 2021
with vanilla kernel, arch linux applies the patch which was reverted in
mainline kernel for some reason.
I see it as a good example of cheap solution. Vendor won't fix your
touchpad because distros provide workaround. The same will be with reset.
Thanks
As mentioned earlier not all vendors care about Linux and not
all of the population can afford to buy new HW just to run Linux.
Thanks,
Amey
From: "Enrico Weigelt, metux IT consult" <info@metux.net> Date: 2021-03-18 17:52:05
On 15.03.21 00:55, Pali Rohár wrote:
Moreover for mPCIe form factor cards, boards can share one PERST# signal
with more PCIe cards and control this signal via GPIO. So asserting
PERST# GPIO can trigger Warm reset for more PCIe cards, not just one. It
depends on board or topology.
The pcengines apu* boards happen to be such candidates: they've got
three m.2 slots, but not all wired in the same way (depending on actual
model, not all have pcie wired). Reset lines are driven via gpio, and
some devices (I recall some lte basebands) sometimes need an explicit
reset in order to come up properly.
I have to check the schematics for the diffrent models, how exactly
these gpios are wired. (i've got reports that some production lines
don't have them wired at all - but couldn't confirm this on my own).
BTW: any idea how to inject board specific reset methods, after the
host brigde driver is already active ? In my case, apu boards, the
pci host bridge is probed via acpi and the apu board driver (which sets
up gpios, leds, keys, ...) comes much later.
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
From: Enrico Weigelt, metux IT consult <hidden> Date: 2021-03-18 17:59:36
On 18.03.21 18:35, Leon Romanovsky wrote:
I see it as a good example of cheap solution. Vendor won't fix your
touchpad because distros provide workaround. The same will be with reset.
Usually, vendor won't fix it, anyways, regardless of any kernel
workarounds.
Most Vendors are already completely overstrained w/ anything
software-related. A good reason why we should try to get rid firmware,
as much as we can.
It's really sad. A *decent* vendor would just provide a clean DT and
(actually matching!) schematics. But that's really hard to find, these
days :(
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
From: "Enrico Weigelt, metux IT consult" <info@metux.net> Date: 2021-03-18 18:15:41
On 18.03.21 18:43, Amey Narkhede wrote:
Well I didn't present it as new use case. I just gave existing
usecase based on existing reset attribute. Nothing new here.
Nothing really changes wrt that use case.
As a board driver maintainer, I fully support your case. At least as a
development/debugging. And even if people out there play around and find
their own workarounds, these can give us maintainers valuable insights
and save us a lot of time.
As mentioned earlier not all vendors care about Linux and not
all of the population can afford to buy new HW just to run Linux.
At least in the x86 world (arm is *much* better here), even the
(supposedly) Linux-friendly ones often don't really care, especially if
the board isn't the newerst model anymore.
Unfortunately, what we do or don't do in the kernel has practically no
influence on board vendor decisions. The best we can practically achieve
at their side is slowing them down on smearing bullshit into FW and acpi
tables. Even getting some useful documentation from vendors is a really
rare thing.
ARM world with device tree, of course, is much better (except for closed
consumer devices like "smartphones" or acpi-poisoned arm64 boxes). At
least for profession embedded boards.
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
From: "Enrico Weigelt, metux IT consult" <info@metux.net> Date: 2021-03-18 18:35:43
On 18.03.21 18:22, Leon Romanovsky wrote:
Which email client do you use?
Your responses are grouped as one huge block without any chance to respond
to you on specific point or answer to your question.
I'm reading this thread in Tbird, and threading / quoting all looks
nice.
I see your flow and understand your position, but will repeat my
position. We need to make sure that vendors will have incentive to
supply quirks.
I really doubt we can influence that by any technical decision here in
the kernel.
And regarding vendors, see Amey response below about his touchpad troubles.
The cheap electronics vendors don't care about their users.
IMHO, the expensive ones don't care either.
Does eg. Dell publish board schematics ? Do they even publish exact part
lists (exact chipsets) along with their brochures, so customers can
check wether their HW is supported, before buying and trying out ?
Doesn't seem so. I've personally seen a lot cases where some supposedly
supported HW turned out to be some completely different and unsupported
HW that's sold under exactly the same product ID. One of many reasons
for not giving them a single penny anymore.
IMHO, there're only very few changes of convincing some HW vendor for
doing a better job on driver side:
a) product is targeted for a niche that can't live without Linux
(eg. embedded)
b) it's really *dangerous* for your market share if anything doesn't
work properly on Linux (eg. certan server machines)
c) somebody *really* big (like Google) is gun-pointing at some supplier,
who's got a lot to loose
d) a *massive* worldwide shitstorm against the vendor
[ And often, even a combination of them isn't enough. Did you know that
even Google doesn't get all specs necessary to replace away the ugly
FSP blob ? (it's the same w/ AMD, but meanwhile I'm pissed enought to
reverse engineer their AGESA blob). ]
You see, what we do here in the kernel has no practical influence on
those hw vendors.
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-19 13:00:29
On Thu, Mar 18, 2021 at 07:34:56PM +0100, Enrico Weigelt, metux IT consult wrote:
On 18.03.21 18:22, Leon Romanovsky wrote:
quoted
Which email client do you use?
Your responses are grouped as one huge block without any chance to respond
to you on specific point or answer to your question.
I'm reading this thread in Tbird, and threading / quoting all looks
nice.
I see your flow and understand your position, but will repeat my
position. We need to make sure that vendors will have incentive to
supply quirks.
I really doubt we can influence that by any technical decision here in
the kernel.
There are subsystems that succeeded to do it, for example netdev, RDMA e.t.c.
quoted
And regarding vendors, see Amey response below about his touchpad troubles.
The cheap electronics vendors don't care about their users.
IMHO, the expensive ones don't care either.
Does eg. Dell publish board schematics ? Do they even publish exact part
lists (exact chipsets) along with their brochures, so customers can
check wether their HW is supported, before buying and trying out ?
They do it because they are allowed to do it and not because they
explicitly want to annoyance their customers.
Doesn't seem so. I've personally seen a lot cases where some supposedly
supported HW turned out to be some completely different and unsupported
HW that's sold under exactly the same product ID. One of many reasons
for not giving them a single penny anymore.
IMHO, there're only very few changes of convincing some HW vendor for
doing a better job on driver side:
a) product is targeted for a niche that can't live without Linux
(eg. embedded)
b) it's really *dangerous* for your market share if anything doesn't
work properly on Linux (eg. certan server machines)
c) somebody *really* big (like Google) is gun-pointing at some supplier,
who's got a lot to loose
d) a *massive* worldwide shitstorm against the vendor
[ And often, even a combination of them isn't enough. Did you know that
even Google doesn't get all specs necessary to replace away the ugly
FSP blob ? (it's the same w/ AMD, but meanwhile I'm pissed enought to
reverse engineer their AGESA blob). ]
I don't know about this specific Google case, but from my previous experience.
The reasons why vendor says no to Google are usually due to licensing and legal
issues and not open source vs. proprietary.
You see, what we do here in the kernel has no practical influence on
those hw vendors.
I see it differently, but it doesn't matter. This is too theoretical
discussion to my taste.
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-19 13:06:30
On Thu, Mar 18, 2021 at 11:13:44PM +0530, Amey Narkhede wrote:
On 21/03/18 07:35PM, Leon Romanovsky wrote:
quoted
On Thu, Mar 18, 2021 at 10:31:43PM +0530, Amey Narkhede wrote:
quoted
On 21/03/18 04:57PM, Leon Romanovsky wrote:
quoted
On Thu, Mar 18, 2021 at 07:52:52PM +0530, Amey Narkhede wrote:
quoted
On 21/03/18 11:09AM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 11:31:40AM -0600, Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
<...>
quoted
quoted
quoted
quoted
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
Thanks
Can you explain bit more about why supported != working?
It is written in the commit message of this patch.
https://lore.kernel.org/lkml/20210312173452.3855-1-ameynarkhede03@gmail.com/
"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."
You wrote it and also repeated it a couple of times during the discussion.
If device can understand that specific reset doesn't work, it won't
perform it in first place.
Thanks
Is it possible for device to understand whether or not specific reset
will work or not prior to performing reset and after it indicates
support for that reset method? Maybe theres problem with that particular
piece of hardware in that machine.
How can database be maintained if a particular machines have
particular piece of faulty HW?
It was exactly the reason why I think that VM usecase presented by
you is not viable.
Well I didn't present it as new use case. I just gave existing
usecase based on existing reset attribute. Nothing new here.
Nothing really changes wrt that use case.
Of course it is new, please see Alex's response, he said that vfio uses
in-kernel API and not sysfs.
quoted
quoted
If for some reason reset doesn't work it will just give -ENOTTY.
This isn't any different from existing behavior.Actually it informs user
that the reset method didn't reset the device and user can use different
reset method instead of implicitly using different reset method.
If user doesn't explicitly set preferred reset method then
we go ahead with existing implicit fall through behavior which will try all
available reset methods until any one of them works.
If you have device that doesn't support reset at all then you have
option to completely disable it unlike existing reset attribute where
you cannot disable reset. So it gives greater control where you can
disable the reset altogether when quirk isn't developed yet.
I explicitly asked to hear usecase, right now, I got an explanation from
Alex for policy decision (which doesn't need sysfs) and from you about
overcoming HW bugs with expectation that user will be guru of PCI reset
methods.
quoted
We can't expect to develop quirk for every device in existence.
It doesn't give us an excuse do not try.
quoted
For example on my laptop elantech touchpad still doesn't work in 2021
with vanilla kernel, arch linux applies the patch which was reverted in
mainline kernel for some reason.
I see it as a good example of cheap solution. Vendor won't fix your
touchpad because distros provide workaround. The same will be with reset.
Thanks
As mentioned earlier not all vendors care about Linux and not
all of the population can afford to buy new HW just to run Linux.
Sorry, but you are not consistent. At the beginning, we talked about new HW
that has bugs but don't have quirks yet. Here we are talking about old HW
that still doesn't have quirks.
Thanks
From: "Enrico Weigelt, metux IT consult" <info@metux.net> Date: 2021-03-19 13:49:02
On 19.03.21 13:59, Leon Romanovsky wrote:
quoted
I really doubt we can influence that by any technical decision here in
the kernel.
There are subsystems that succeeded to do it, for example netdev, RDMA e.t.c.
I'd guess either hi-end / server or embedded products - already
mentioned that these are different fields. I've been talking about the
average consumer products.
OTOH, there're also very expensive vendors that are exceptionally bad,
eg. National instruments (who even are capable of breaking rpm so badly
with their proprietary packages that they open up 0day holes - i once
filed a report @FD on such a case).
quoted
IMHO, the expensive ones don't care either.
Does eg. Dell publish board schematics ? Do they even publish exact part
lists (exact chipsets) along with their brochures, so customers can
check wether their HW is supported, before buying and trying out ?
They do it because they are allowed to do it and not because they
explicitly want to annoyance their customers.
Yes, they're just ignorant. They can still do that, because buy their
pretty expensive cheap-hardware. And that's mostly driven by purchase
people inside the customer organisations, who just don't care how much
damage they do to their own employers, by dictating purchase of
expensive broken-by-design hardware. ... but that's nothing we here have
any influence on - except for dissuasion and purchase boycott ...
In any case, I still fail to see why giving operators an debug knob
should make anything worse.
quoted
[ And often, even a combination of them isn't enough. Did you know that
even Google doesn't get all specs necessary to replace away the ugly
FSP blob ? (it's the same w/ AMD, but meanwhile I'm pissed enought to
reverse engineer their AGESA blob). ]
I don't know about this specific Google case, but from my previous experience.
The reasons why vendor says no to Google are usually due to licensing and legal
issues and not open source vs. proprietary.
In short words: Google did (still does?) build their own mainboards and
FW (IIRC that's where LinuxBoot came from), but even with their HUGE
quantities (they buy cpus in quantities of truck loads) they still did
not manage to get any specs for writing their own early init w/o the
proprietary FSP.
The licensing / legal issues can either be:
a) we, the mightly Intel Corp., have been so extremly stupid for
licensing some vital IP stuff (what exactly could that be, in exactly
the prime domain of Intel ?) and signing such insane crontracts, that
we're not allowed to tell anybody how to actually use our own
products (yes: initializing the CPU and built-in interfaces belongs
exactly into that category)
b) we, the mighty Intel Corp., couldn't build something on our own, but
just stolen IP (in our primary domain) and are scared that anybody
could find out from just reading some early setup code.
c) we, the mighty Intel Corp., rule the world and we give a phrack on
what some tiny Customers like Google want from us.
d) we, the mightly Intel Corp., did do what our name tells: INTEL,
and we don't want anybody raise unpleasant questions.
choose your poison :P
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
On Thu, Mar 18, 2021 at 11:13:44PM +0530, Amey Narkhede wrote:
quoted
On 21/03/18 07:35PM, Leon Romanovsky wrote:
quoted
On Thu, Mar 18, 2021 at 10:31:43PM +0530, Amey Narkhede wrote:
quoted
On 21/03/18 04:57PM, Leon Romanovsky wrote:
quoted
On Thu, Mar 18, 2021 at 07:52:52PM +0530, Amey Narkhede wrote:
quoted
On 21/03/18 11:09AM, Leon Romanovsky wrote:
quoted
On Wed, Mar 17, 2021 at 11:31:40AM -0600, Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 15:58:40 +0200
Leon Romanovsky [off-list ref] wrote:
<...>
quoted
quoted
quoted
quoted
I'm lost here, does vfio-pci use sysfs interface or internal to the kernel API?
If it is latter then we don't really need sysfs, if not, we still need
some sort of DB to create second policy, because "supported != working".
What am I missing?
Thanks
Can you explain bit more about why supported != working?
It is written in the commit message of this patch.
https://lore.kernel.org/lkml/20210312173452.3855-1-ameynarkhede03@gmail.com/
"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."
You wrote it and also repeated it a couple of times during the discussion.
If device can understand that specific reset doesn't work, it won't
perform it in first place.
Thanks
Is it possible for device to understand whether or not specific reset
will work or not prior to performing reset and after it indicates
support for that reset method? Maybe theres problem with that particular
piece of hardware in that machine.
How can database be maintained if a particular machines have
particular piece of faulty HW?
It was exactly the reason why I think that VM usecase presented by
you is not viable.
Well I didn't present it as new use case. I just gave existing
usecase based on existing reset attribute. Nothing new here.
Nothing really changes wrt that use case.
Of course it is new, please see Alex's response, he said that vfio uses
in-kernel API and not sysfs.
Still it doesn't change in-kernel API either.
quoted
quoted
quoted
If for some reason reset doesn't work it will just give -ENOTTY.
This isn't any different from existing behavior.Actually it informs user
that the reset method didn't reset the device and user can use different
reset method instead of implicitly using different reset method.
If user doesn't explicitly set preferred reset method then
we go ahead with existing implicit fall through behavior which will try all
available reset methods until any one of them works.
If you have device that doesn't support reset at all then you have
option to completely disable it unlike existing reset attribute where
you cannot disable reset. So it gives greater control where you can
disable the reset altogether when quirk isn't developed yet.
I explicitly asked to hear usecase, right now, I got an explanation from
Alex for policy decision (which doesn't need sysfs) and from you about
overcoming HW bugs with expectation that user will be guru of PCI reset
methods.
quoted
We can't expect to develop quirk for every device in existence.
It doesn't give us an excuse do not try.
quoted
For example on my laptop elantech touchpad still doesn't work in 2021
with vanilla kernel, arch linux applies the patch which was reverted in
mainline kernel for some reason.
I see it as a good example of cheap solution. Vendor won't fix your
touchpad because distros provide workaround. The same will be with reset.
Thanks
As mentioned earlier not all vendors care about Linux and not
all of the population can afford to buy new HW just to run Linux.
Sorry, but you are not consistent. At the beginning, we talked about new HW
that has bugs but don't have quirks yet. Here we are talking about old HW
that still doesn't have quirks.
Thanks
Does it really matter whether HW is old or new?
If old HW doesn't have quirks yet how can we expect
new one to have quirks? What if new HW is made by same vendors
who don't have any interest in Linux?
Thanks,
Amey
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-19 15:38:21
On Fri, Mar 19, 2021 at 08:53:17PM +0530, Amey Narkhede wrote:
On 21/03/19 03:05PM, Leon Romanovsky wrote:
<...>
quoted
quoted
quoted
It was exactly the reason why I think that VM usecase presented by
you is not viable.
Well I didn't present it as new use case. I just gave existing
usecase based on existing reset attribute. Nothing new here.
Nothing really changes wrt that use case.
Of course it is new, please see Alex's response, he said that vfio uses
in-kernel API and not sysfs.
Still it doesn't change in-kernel API either.
Right, but the issue is with user space part of this proposal and not
in-kernel API.
<...>
quoted
quoted
As mentioned earlier not all vendors care about Linux and not
all of the population can afford to buy new HW just to run Linux.
Sorry, but you are not consistent. At the beginning, we talked about new HW
that has bugs but don't have quirks yet. Here we are talking about old HW
that still doesn't have quirks.
Thanks
Does it really matter whether HW is old or new?
If old HW doesn't have quirks yet how can we expect
new one to have quirks? What if new HW is made by same vendors
who don't have any interest in Linux?
It is pretty clear that this sysfs won't improve quirks situation but
has all potential to reduce their amount even more.
Let's stop this discussion here.
Thanks
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-19 15:52:09
On Fri, Mar 19, 2021 at 02:48:12PM +0100, Enrico Weigelt, metux IT consult wrote:
On 19.03.21 13:59, Leon Romanovsky wrote:
<...>
In any case, I still fail to see why giving operators an debug knob
should make anything worse.
I see this patch as a workaround to stop and provide quirks for reset issues.
As a way forward, we can do this sysfs visible for DEBUG/EXPERT .config builds.
What do you think?
quoted
quoted
[ And often, even a combination of them isn't enough. Did you know that
even Google doesn't get all specs necessary to replace away the ugly
FSP blob ? (it's the same w/ AMD, but meanwhile I'm pissed enought to
reverse engineer their AGESA blob). ]
I don't know about this specific Google case, but from my previous experience.
The reasons why vendor says no to Google are usually due to licensing and legal
issues and not open source vs. proprietary.
In short words: Google did (still does?) build their own mainboards and
FW (IIRC that's where LinuxBoot came from), but even with their HUGE
quantities (they buy cpus in quantities of truck loads) they still did
not manage to get any specs for writing their own early init w/o the
proprietary FSP.
The licensing / legal issues can either be:
a) we, the mightly Intel Corp., have been so extremly stupid for
licensing some vital IP stuff (what exactly could that be, in exactly
the prime domain of Intel ?) and signing such insane crontracts, that
we're not allowed to tell anybody how to actually use our own
products (yes: initializing the CPU and built-in interfaces belongs
exactly into that category)
b) we, the mighty Intel Corp., couldn't build something on our own, but
just stolen IP (in our primary domain) and are scared that anybody
could find out from just reading some early setup code.
c) we, the mighty Intel Corp., rule the world and we give a phrack on
what some tiny Customers like Google want from us.
d) we, the mightly Intel Corp., did do what our name tells: INTEL,
and we don't want anybody raise unpleasant questions.
choose your poison :P
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
On Fri, Mar 19, 2021 at 08:53:17PM +0530, Amey Narkhede wrote:
quoted
On 21/03/19 03:05PM, Leon Romanovsky wrote:
<...>
quoted
quoted
quoted
quoted
It was exactly the reason why I think that VM usecase presented by
you is not viable.
Well I didn't present it as new use case. I just gave existing
usecase based on existing reset attribute. Nothing new here.
Nothing really changes wrt that use case.
Of course it is new, please see Alex's response, he said that vfio uses
in-kernel API and not sysfs.
Still it doesn't change in-kernel API either.
Right, but the issue is with user space part of this proposal and not
in-kernel API.
Userspace part just inhances existing reset attribute still no
significant changes there.
<...>
quoted
quoted
quoted
As mentioned earlier not all vendors care about Linux and not
all of the population can afford to buy new HW just to run Linux.
Sorry, but you are not consistent. At the beginning, we talked about new HW
that has bugs but don't have quirks yet. Here we are talking about old HW
that still doesn't have quirks.
Thanks
Does it really matter whether HW is old or new?
If old HW doesn't have quirks yet how can we expect
new one to have quirks? What if new HW is made by same vendors
who don't have any interest in Linux?
It is pretty clear that this sysfs won't improve quirks situation but
has all potential to reduce their amount even more.
Let's stop this discussion here.
Thanks
IMO it does improve usability of devices which I consider to be more
important than developing quirks which are just bandages in the end
not HW fix. There's no point in using Linux if
I can't use the device in the first place and expecting to wait
for some community member to develop quirk without vendor support
is simply unrealistic.
So let's stop this discussion here.
Thanks,
Amey
On Fri, Mar 19, 2021 at 02:59:47PM +0200, Leon Romanovsky wrote:
On Thu, Mar 18, 2021 at 07:34:56PM +0100, Enrico Weigelt, metux IT consult wrote:
quoted
On 18.03.21 18:22, Leon Romanovsky wrote:
quoted
Which email client do you use? Your responses are grouped as
one huge block without any chance to respond to you on specific
point or answer to your question.
I'm reading this thread in Tbird, and threading / quoting all
looks nice.
Don't make this harder than it needs to be. I think it's totally
acceptable to just split Alex's text where you need to respond. For
example, Alex wrote this:
vfio-pci uses the internal kernel API, ie. the variants of
pci_reset_function(), which is the same interface used by the existing
sysfs reset mechanism. This proposed configuration of the reset method
would affect any driver using that same core infrastructure and from my
perspective that's really the goal. ...
If I wanted to respond to the first sentence, I would just do this:
aw> vfio-pci uses the internal kernel API, ie. the variants of
aw> pci_reset_function(), which is the same interface used by the existing
aw> sysfs reset mechanism.
I would write my response to the above here. The rest of the quote
continues on below. If the rest of Alex's message isn't relevant to
my response, I would remove it completely.
aw> This proposed configuration of the reset method
aw> would affect any driver using that same core infrastructure and from my
aw> perspective that's really the goal. ...
Bjorn
From: Alex Williamson <hidden> Date: 2021-03-19 16:24:02
On Fri, 19 Mar 2021 14:59:47 +0200
Leon Romanovsky [off-list ref] wrote:
On Thu, Mar 18, 2021 at 07:34:56PM +0100, Enrico Weigelt, metux IT consult wrote:
quoted
On 18.03.21 18:22, Leon Romanovsky wrote:
quoted
Which email client do you use?
Your responses are grouped as one huge block without any chance to respond
to you on specific point or answer to your question.
I'm reading this thread in Tbird, and threading / quoting all looks
nice.
I've never known paragraph breaks to be required to interject a reply.
Back on topic...
quoted
quoted
I see your flow and understand your position, but will repeat my
position. We need to make sure that vendors will have incentive to
supply quirks.
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
If a device probes for a reset that's broken and distros start
including systemd scripts to apply a preference to avoid it, (a) that
enables them to work with existing kernels, and (b) indicates to us to
add the trivial quirk to flag that reset as broken.
The other side of the argument that this discourages quirks is that
this interface actually makes it significantly easier to report specific
reset methods as broken for a given device.
Thanks,
Alex
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-19 16:25:05
On Fri, Mar 19, 2021 at 10:57:11AM -0500, Bjorn Helgaas wrote:
On Fri, Mar 19, 2021 at 02:59:47PM +0200, Leon Romanovsky wrote:
quoted
On Thu, Mar 18, 2021 at 07:34:56PM +0100, Enrico Weigelt, metux IT consult wrote:
quoted
On 18.03.21 18:22, Leon Romanovsky wrote:
quoted
Which email client do you use? Your responses are grouped as
one huge block without any chance to respond to you on specific
point or answer to your question.
I'm reading this thread in Tbird, and threading / quoting all
looks nice.
Don't make this harder than it needs to be. I think it's totally
acceptable to just split Alex's text where you need to respond. For
example, Alex wrote this:
vfio-pci uses the internal kernel API, ie. the variants of
pci_reset_function(), which is the same interface used by the existing
sysfs reset mechanism. This proposed configuration of the reset method
would affect any driver using that same core infrastructure and from my
perspective that's really the goal. ...
If I wanted to respond to the first sentence, I would just do this:
aw> vfio-pci uses the internal kernel API, ie. the variants of
aw> pci_reset_function(), which is the same interface used by the existing
aw> sysfs reset mechanism.
I would write my response to the above here. The rest of the quote
continues on below. If the rest of Alex's message isn't relevant to
my response, I would remove it completely.
aw> This proposed configuration of the reset method
aw> would affect any driver using that same core infrastructure and from my
aw> perspective that's really the goal. ...
Bjorn
Thanks Bjorn, you presented me how to respond on such messages, however
I was more afraid if my setup needs some adjustments and it is only me
who sees it as one chunk.
Thanks
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-20 11:54:28
On Fri, Mar 19, 2021 at 10:23:13AM -0600, Alex Williamson wrote:
On Fri, 19 Mar 2021 14:59:47 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Thu, Mar 18, 2021 at 07:34:56PM +0100, Enrico Weigelt, metux IT consult wrote:
quoted
On 18.03.21 18:22, Leon Romanovsky wrote:
quoted
Which email client do you use?
Your responses are grouped as one huge block without any chance to respond
to you on specific point or answer to your question.
I'm reading this thread in Tbird, and threading / quoting all looks
nice.
I've never known paragraph breaks to be required to interject a reply.
Of course not, but as Bjorn said if you don't do paragraphs, we will
need manually break your message, fix ">" quotation marks and half
sentences.
I just wanted to be sure that this is not my mail client.
Back on topic...
quoted
quoted
quoted
I see your flow and understand your position, but will repeat my
position. We need to make sure that vendors will have incentive to
supply quirks.
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
What does it mean "none"? Does it mean nothing supported? If yes, I think that
pci_warn() will be enough. At least for me, taint is usable during debug stages,
probably if device doesn't crash no one will look to see /proc/sys/kernel/tainted.
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
I don't know enough to discuss the PCI details, but you raised good point.
This sysfs is user visible API that is presented as is from device point
of view. It can be easily run into problems if PCI/core doesn't work with
user's choice.
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
But this patch was presented as a way to overcome situations where
supported != working and user magically knows which reset type to set.
If you want to take this patch to be policy decision tool,
it will need to accept "reset_type1,reset_type2,..." sort of input,
so fallback will work natively.
I think that it will be much more robust and cleaner solution than it is now.
Something like that:
cat /sys/..../reset_policy
reset_type1,reset_type2,...,reset_typeX
echo "reset_type3,reset_type1" > /sys/..../reset_policy
cat /sys/..../reset_policy
reset_type3,reset_type1
Thanks
From: Alex Williamson <hidden> Date: 2021-03-20 15:00:28
On Sat, 20 Mar 2021 11:10:08 +0200
Leon Romanovsky [off-list ref] wrote:
On Fri, Mar 19, 2021 at 10:23:13AM -0600, Alex Williamson wrote:
quoted
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
What does it mean "none"? Does it mean nothing supported? If yes, I think that
pci_warn() will be enough. At least for me, taint is usable during debug stages,
probably if device doesn't crash no one will look to see /proc/sys/kernel/tainted.
"none" as implemented in this patch, clearing the enabled function
reset methods.
quoted
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
I don't know enough to discuss the PCI details, but you raised good point.
This sysfs is user visible API that is presented as is from device point
of view. It can be easily run into problems if PCI/core doesn't work with
user's choice.
quoted
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
But this patch was presented as a way to overcome situations where
supported != working and user magically knows which reset type to set.
It's not magic, the new sysfs attributes expose which resets are
enabled and the order that they're used, the user can simply select the
next one. Being able to bypass a broken reset method is a helpful side
effect of getting to select a preferred reset method.
If you want to take this patch to be policy decision tool,
it will need to accept "reset_type1,reset_type2,..." sort of input,
so fallback will work natively.
I don't see that as a requirement. We have fall-through support in the
kernel, but for a given device we're really only ever going to make use
of one of those methods. If a user knows enough about a device to have
a preference, I think it can be singular. That also significantly
simplifies the interface and supporting code. Thanks,
Alex
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-21 08:42:10
On Sat, Mar 20, 2021 at 08:59:42AM -0600, Alex Williamson wrote:
On Sat, 20 Mar 2021 11:10:08 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 10:23:13AM -0600, Alex Williamson wrote:
quoted
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
What does it mean "none"? Does it mean nothing supported? If yes, I think that
pci_warn() will be enough. At least for me, taint is usable during debug stages,
probably if device doesn't crash no one will look to see /proc/sys/kernel/tainted.
"none" as implemented in this patch, clearing the enabled function
reset methods.
It is far from intuitive, the empty string will be easier to understand,
because "none" means no reset at all.
quoted
quoted
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
I don't know enough to discuss the PCI details, but you raised good point.
This sysfs is user visible API that is presented as is from device point
of view. It can be easily run into problems if PCI/core doesn't work with
user's choice.
quoted
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
But this patch was presented as a way to overcome situations where
supported != working and user magically knows which reset type to set.
It's not magic, the new sysfs attributes expose which resets are
enabled and the order that they're used, the user can simply select the
next one. Being able to bypass a broken reset method is a helpful side
effect of getting to select a preferred reset method.
Magic in a sense that user has no idea what those resets mean, the
expectation is that he will blindly iterate till something works.
quoted
If you want to take this patch to be policy decision tool,
it will need to accept "reset_type1,reset_type2,..." sort of input,
so fallback will work natively.
I don't see that as a requirement. We have fall-through support in the
kernel, but for a given device we're really only ever going to make use
of one of those methods. If a user knows enough about a device to have
a preference, I think it can be singular. That also significantly
simplifies the interface and supporting code. Thanks,
I'm struggling to get requirements from this thread. You talked about
policy decision to overtake fallback mechanism, Amey wanted to avoid
quirks.
Do you have an example of such devices or we are talking about
theoretical case?
And I don't see why simple line parser with loop iterator over strchr()
suddenly becomes complicated code.
Thanks
On Sat, Mar 20, 2021 at 08:59:42AM -0600, Alex Williamson wrote:
quoted
On Sat, 20 Mar 2021 11:10:08 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 10:23:13AM -0600, Alex Williamson wrote:
quoted
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
What does it mean "none"? Does it mean nothing supported? If yes, I think that
pci_warn() will be enough. At least for me, taint is usable during debug stages,
probably if device doesn't crash no one will look to see /proc/sys/kernel/tainted.
"none" as implemented in this patch, clearing the enabled function
reset methods.
It is far from intuitive, the empty string will be easier to understand,
because "none" means no reset at all.
quoted
quoted
quoted
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
I don't know enough to discuss the PCI details, but you raised good point.
This sysfs is user visible API that is presented as is from device point
of view. It can be easily run into problems if PCI/core doesn't work with
user's choice.
quoted
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
But this patch was presented as a way to overcome situations where
supported != working and user magically knows which reset type to set.
It's not magic, the new sysfs attributes expose which resets are
enabled and the order that they're used, the user can simply select the
next one. Being able to bypass a broken reset method is a helpful side
effect of getting to select a preferred reset method.
Magic in a sense that user has no idea what those resets mean, the
expectation is that he will blindly iterate till something works.
quoted
quoted
If you want to take this patch to be policy decision tool,
it will need to accept "reset_type1,reset_type2,..." sort of input,
so fallback will work natively.
I don't see that as a requirement. We have fall-through support in the
kernel, but for a given device we're really only ever going to make use
of one of those methods. If a user knows enough about a device to have
a preference, I think it can be singular. That also significantly
simplifies the interface and supporting code. Thanks,
I'm struggling to get requirements from this thread. You talked about
policy decision to overtake fallback mechanism, Amey wanted to avoid
quirks.
Just to clarify I don't want to avoid quirks. I just want device
to be usable even if it doesn't have quirk as the quirk for that
particular device may not be developed at all for different reasons
mentioned earlier.
[...]
Thanks,
Amey
From: Alex Williamson <hidden> Date: 2021-03-22 17:10:59
On Sun, 21 Mar 2021 10:40:55 +0200
Leon Romanovsky [off-list ref] wrote:
On Sat, Mar 20, 2021 at 08:59:42AM -0600, Alex Williamson wrote:
quoted
On Sat, 20 Mar 2021 11:10:08 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 10:23:13AM -0600, Alex Williamson wrote:
quoted
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
What does it mean "none"? Does it mean nothing supported? If yes, I think that
pci_warn() will be enough. At least for me, taint is usable during debug stages,
probably if device doesn't crash no one will look to see /proc/sys/kernel/tainted.
"none" as implemented in this patch, clearing the enabled function
reset methods.
It is far from intuitive, the empty string will be easier to understand,
because "none" means no reset at all.
"No reset at all" is what "none" achieves, the
pci_dev.reset_methods_enabled bitmap is cleared. We can use an empty
string, but I think we want a way to clear all enabled resets and a way
to return it to the default. I could see arguments for an empty string
serving either purpose, so this version proposed explicitly using
"none" and "default", as included in the ABI update.
quoted
quoted
quoted
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
I don't know enough to discuss the PCI details, but you raised good point.
This sysfs is user visible API that is presented as is from device point
of view. It can be easily run into problems if PCI/core doesn't work with
user's choice.
quoted
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
But this patch was presented as a way to overcome situations where
supported != working and user magically knows which reset type to set.
It's not magic, the new sysfs attributes expose which resets are
enabled and the order that they're used, the user can simply select the
next one. Being able to bypass a broken reset method is a helpful side
effect of getting to select a preferred reset method.
Magic in a sense that user has no idea what those resets mean, the
expectation is that he will blindly iterate till something works.
Which ought to actually be a safe thing to do. We should have quirks to
exclude resets that are known broken but still probe as present and I'd
be perfectly fine if we issue a warning if the user disables all resets
for a given device.
quoted
quoted
If you want to take this patch to be policy decision tool,
it will need to accept "reset_type1,reset_type2,..." sort of input,
so fallback will work natively.
I don't see that as a requirement. We have fall-through support in the
kernel, but for a given device we're really only ever going to make use
of one of those methods. If a user knows enough about a device to have
a preference, I think it can be singular. That also significantly
simplifies the interface and supporting code. Thanks,
I'm struggling to get requirements from this thread. You talked about
policy decision to overtake fallback mechanism, Amey wanted to avoid
quirks.
Do you have an example of such devices or we are talking about
theoretical case?
Look at any device that already has a reset quirk and the process it
took to get there. Those are more than just theoretical cases.
For policy preference, I already described how I've configured QEMU to
prefer a bus reset rather than a PM reset due to lack of specification
regarding the scope of a PM "soft reset". This interface would allow a
system policy to do that same thing.
I don't think anyone is suggesting this as a means to avoid quirks that
would resolve reset issues and create the best default general behavior.
This provides a mechanism to test various reset methods, and thereby
identify broken methods, and set a policy. Sure, that policy might be
to avoid a broken reset in the interim before it gets quirked and
there's potential for abuse there, but I think the benefits outweigh
the risks.
And I don't see why simple line parser with loop iterator over strchr()
suddenly becomes complicated code.
Setting multiple bits in a bitmap is easy. How do you then go on to
allow the user to specify an ordering preference? If you have an
algorithm you'd like to propose that allows the user to manage the
ordering when enabling multiple methods without substantially
increasing the complexity, please share. IMO, a given device will
generally use one reset method and it seems sufficient to restrict user
preference to achieve all the use cases I've noted. Thanks,
Alex
On Thursday 18 March 2021 20:01:55 Amey Narkhede wrote:
On 21/03/17 09:13PM, Pali Rohár wrote:
quoted
On Wednesday 17 March 2021 14:00:20 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:40:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:32:45 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected.
Yes, this makes slot reset "safe". But bus reset is "unsafe".
quoted
Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
That should work when slot reset is available.
Other option is that mentioned remove-reset-rescan procedure.
That's not something we can introduce to the pci_reset_function() path
without a fair bit of collateral in using it through vfio-pci.
quoted
But quick search in drivers/pci/hotplug/ results that not all hotplug
drivers implement reset_slot method.
So there is a possible issue with hotplug driver which may eject device
during bus reset (because e.g. slot reset is not implemented)?
People aren't reporting it, so maybe those controllers aren't being
used for this use case. Or maybe introducing this patch will make
these reset methods more readily accessible for testing. We can fix or
blacklist those controllers for bus reset when reports come in. Thanks,
Ok! I do not know neither if those controllers are used, but looks like
that there are still changes in hotplug code.
So I guess with these patches people can test it and report issues when
such thing happen.
So after a bit research as I understood we need to group slot
and bus reset together in a single category of reset methods and
then implicitly use slot reset if it is available when bus reset is
enabled by the user.
Is that right?
Yes, I understand it in same way. Just I do not know which name to
choose for this reset category. In PCI spec it is called Secondary Bus
Reset (as it resets whole bus with all devices; but we allow this reset
in this patch series only if on the bus is connected exactly one device).
In PCIe spec it is called Hot Reset. And if kernel detects Slot support
then kernel currently calls it Slot reset. But it is still same thing.
Any opinion? I think that we could call it Hot Reset as this patch
series exports it only for single device (so calling it _bus_ is not the
best match).
From: Alex Williamson <hidden> Date: 2021-03-23 14:45:45
On Tue, 23 Mar 2021 15:34:19 +0100
Pali Rohár [off-list ref] wrote:
On Thursday 18 March 2021 20:01:55 Amey Narkhede wrote:
quoted
On 21/03/17 09:13PM, Pali Rohár wrote:
quoted
On Wednesday 17 March 2021 14:00:20 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:40:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:32:45 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected.
Yes, this makes slot reset "safe". But bus reset is "unsafe".
quoted
Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
That should work when slot reset is available.
Other option is that mentioned remove-reset-rescan procedure.
That's not something we can introduce to the pci_reset_function() path
without a fair bit of collateral in using it through vfio-pci.
quoted
But quick search in drivers/pci/hotplug/ results that not all hotplug
drivers implement reset_slot method.
So there is a possible issue with hotplug driver which may eject device
during bus reset (because e.g. slot reset is not implemented)?
People aren't reporting it, so maybe those controllers aren't being
used for this use case. Or maybe introducing this patch will make
these reset methods more readily accessible for testing. We can fix or
blacklist those controllers for bus reset when reports come in. Thanks,
Ok! I do not know neither if those controllers are used, but looks like
that there are still changes in hotplug code.
So I guess with these patches people can test it and report issues when
such thing happen.
So after a bit research as I understood we need to group slot
and bus reset together in a single category of reset methods and
then implicitly use slot reset if it is available when bus reset is
enabled by the user.
Is that right?
Yes, I understand it in same way. Just I do not know which name to
choose for this reset category. In PCI spec it is called Secondary Bus
Reset (as it resets whole bus with all devices; but we allow this reset
in this patch series only if on the bus is connected exactly one device).
In PCIe spec it is called Hot Reset. And if kernel detects Slot support
then kernel currently calls it Slot reset. But it is still same thing.
Any opinion? I think that we could call it Hot Reset as this patch
series exports it only for single device (so calling it _bus_ is not the
best match).
A similar abstraction where our scope is not limited to a single
function calls this a bus reset:
int pci_reset_bus(struct pci_dev *pdev)
{
return (!pci_probe_reset_slot(pdev->slot)) ?
__pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
}
Thanks,
Alex
On Tue, 23 Mar 2021 15:34:19 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Thursday 18 March 2021 20:01:55 Amey Narkhede wrote:
quoted
On 21/03/17 09:13PM, Pali Rohár wrote:
quoted
On Wednesday 17 March 2021 14:00:20 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:40:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:32:45 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected.
Yes, this makes slot reset "safe". But bus reset is "unsafe".
quoted
Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
That should work when slot reset is available.
Other option is that mentioned remove-reset-rescan procedure.
That's not something we can introduce to the pci_reset_function() path
without a fair bit of collateral in using it through vfio-pci.
quoted
But quick search in drivers/pci/hotplug/ results that not all hotplug
drivers implement reset_slot method.
So there is a possible issue with hotplug driver which may eject device
during bus reset (because e.g. slot reset is not implemented)?
People aren't reporting it, so maybe those controllers aren't being
used for this use case. Or maybe introducing this patch will make
these reset methods more readily accessible for testing. We can fix or
blacklist those controllers for bus reset when reports come in. Thanks,
Ok! I do not know neither if those controllers are used, but looks like
that there are still changes in hotplug code.
So I guess with these patches people can test it and report issues when
such thing happen.
So after a bit research as I understood we need to group slot
and bus reset together in a single category of reset methods and
then implicitly use slot reset if it is available when bus reset is
enabled by the user.
Is that right?
Yes, I understand it in same way. Just I do not know which name to
choose for this reset category. In PCI spec it is called Secondary Bus
Reset (as it resets whole bus with all devices; but we allow this reset
in this patch series only if on the bus is connected exactly one device).
In PCIe spec it is called Hot Reset. And if kernel detects Slot support
then kernel currently calls it Slot reset. But it is still same thing.
Any opinion? I think that we could call it Hot Reset as this patch
series exports it only for single device (so calling it _bus_ is not the
best match).
A similar abstraction where our scope is not limited to a single
function calls this a bus reset:
int pci_reset_bus(struct pci_dev *pdev)
{
return (!pci_probe_reset_slot(pdev->slot)) ?
__pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
}
Thanks,
Alex
I was going to use similar function
int pci_bus_reset(struct pci_dev *dev, int probe)
{
return pci_dev_reset_slot_function(dev, probe) ?
pci_parent_bus_reset(dev, probe) : 0;
}
Thanks,
Amey
From: Alex Williamson <hidden> Date: 2021-03-23 16:07:29
On Tue, 23 Mar 2021 21:02:21 +0530
Amey Narkhede [off-list ref] wrote:
On 21/03/23 08:44AM, Alex Williamson wrote:
quoted
On Tue, 23 Mar 2021 15:34:19 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Thursday 18 March 2021 20:01:55 Amey Narkhede wrote:
quoted
On 21/03/17 09:13PM, Pali Rohár wrote:
quoted
On Wednesday 17 March 2021 14:00:20 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:40:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:32:45 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected.
Yes, this makes slot reset "safe". But bus reset is "unsafe".
quoted
Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
That should work when slot reset is available.
Other option is that mentioned remove-reset-rescan procedure.
That's not something we can introduce to the pci_reset_function() path
without a fair bit of collateral in using it through vfio-pci.
quoted
But quick search in drivers/pci/hotplug/ results that not all hotplug
drivers implement reset_slot method.
So there is a possible issue with hotplug driver which may eject device
during bus reset (because e.g. slot reset is not implemented)?
People aren't reporting it, so maybe those controllers aren't being
used for this use case. Or maybe introducing this patch will make
these reset methods more readily accessible for testing. We can fix or
blacklist those controllers for bus reset when reports come in. Thanks,
Ok! I do not know neither if those controllers are used, but looks like
that there are still changes in hotplug code.
So I guess with these patches people can test it and report issues when
such thing happen.
So after a bit research as I understood we need to group slot
and bus reset together in a single category of reset methods and
then implicitly use slot reset if it is available when bus reset is
enabled by the user.
Is that right?
Yes, I understand it in same way. Just I do not know which name to
choose for this reset category. In PCI spec it is called Secondary Bus
Reset (as it resets whole bus with all devices; but we allow this reset
in this patch series only if on the bus is connected exactly one device).
In PCIe spec it is called Hot Reset. And if kernel detects Slot support
then kernel currently calls it Slot reset. But it is still same thing.
Any opinion? I think that we could call it Hot Reset as this patch
series exports it only for single device (so calling it _bus_ is not the
best match).
A similar abstraction where our scope is not limited to a single
function calls this a bus reset:
int pci_reset_bus(struct pci_dev *pdev)
{
return (!pci_probe_reset_slot(pdev->slot)) ?
__pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
}
Thanks,
Alex
I was going to use similar function
int pci_bus_reset(struct pci_dev *dev, int probe)
{
return pci_dev_reset_slot_function(dev, probe) ?
pci_parent_bus_reset(dev, probe) : 0;
}
I think via the sysfs attribute we can simply call this "bus" reset,
but internally having both pci_reset_bus() and pci_bus_reset() would be
really confusing. We're doing the same thing as pci_bus_reset() but
with a different scope, so I'd probably suggest
pci_bus_reset_function().
Also, the above ternary form isn't true to the original, only -ENOTTY
allows fall-through, so something more like:
int pci_reset_bus_function(struct pci_dev *dev, int probe)
{
int rc = pci_dev_reset_slot_function(dev, probe);
return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
}
Thanks,
Alex
From: Alex Williamson <hidden> Date: 2021-03-23 16:16:35
On Tue, 23 Mar 2021 10:06:25 -0600
Alex Williamson [off-list ref] wrote:
On Tue, 23 Mar 2021 21:02:21 +0530
Amey Narkhede [off-list ref] wrote:
quoted
On 21/03/23 08:44AM, Alex Williamson wrote:
quoted
On Tue, 23 Mar 2021 15:34:19 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Thursday 18 March 2021 20:01:55 Amey Narkhede wrote:
quoted
On 21/03/17 09:13PM, Pali Rohár wrote:
quoted
On Wednesday 17 March 2021 14:00:20 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:40:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:32:45 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:24:24 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Wednesday 17 March 2021 13:15:36 Alex Williamson wrote:
quoted
On Wed, 17 Mar 2021 20:02:06 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 09:03:39 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 15:52:38 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 08:34:09 Alex Williamson wrote:
quoted
On Mon, 15 Mar 2021 14:52:26 +0100
Pali Rohár [off-list ref] wrote:
quoted
On Monday 15 March 2021 19:13:23 Amey Narkhede wrote:
quoted
slot reset (pci_dev_reset_slot_function) and secondary bus
reset(pci_parent_bus_reset) which I think are hot reset and
warm reset respectively.
No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just another
type of reset, which is currently implemented only for PCIe hot plug
bridges and for PowerPC PowerNV platform and it just call PCI secondary
bus reset with some other hook. PCIe Warm Reset does not have API in
kernel and therefore drivers do not export this type of reset via any
kernel function (yet).
Warm reset is beyond the scope of this series, but could be implemented
in a compatible way to fit within the pci_reset_fn_methods[] array
defined here.
Ok!
quoted
Note that with this series the resets available through
pci_reset_function() and the per device reset attribute is sysfs remain
exactly the same as they are currently. The bus and slot reset
methods used here are limited to devices where only a single function is
affected by the reset, therefore it is not like the patch you proposed
which performed a reset irrespective of the downstream devices. This
series only enables selection of the existing methods. Thanks,
Alex
But with this patch series, there is still an issue with PCI secondary
bus reset mechanism as exported sysfs attribute does not do that
remove-reset-rescan procedure. As discussed in other thread, this reset
let device in unconfigured / broken state.
No, there's not:
int pci_reset_function(struct pci_dev *dev)
{
int rc;
if (!dev->reset_fn)
return -ENOTTY;
pci_dev_lock(dev);
quoted
quoted
quoted
pci_dev_save_and_disable(dev);
rc = __pci_reset_function_locked(dev);
quoted
quoted
quoted
pci_dev_restore(dev);
pci_dev_unlock(dev);
return rc;
}
The remove/re-scan was discussed primarily because your patch performed
a bus reset regardless of what devices were affected by that reset and
it's difficult to manage the scope where multiple devices are affected.
Here, the bus and slot reset functions will fail unless the scope is
limited to the single device triggering this reset. Thanks,
Alex
I was thinking a bit more about it and I'm really sure how it would
behave with hotplugging PCIe bridge.
On aardvark PCIe controller I have already tested that secondary bus
reset bit is triggering Hot Reset event and then also Link Down event.
These events are not handled by aardvark driver yet (needs to
implemented into kernel's emulated root bridge code).
But I'm not sure how it would behave on real HW PCIe hotplugging bridge.
Kernel has already code which removes PCIe device if it changes presence
bit (and inform via interrupt). And Link Down event triggers this
change.
This is the difference between slot and bus resets, the slot reset is
implemented by the hotplug controller and disables presence detection
around the bus reset. Thanks,
Yes, but I'm talking about bus reset, not about slot reset.
I mean: to use bus reset via sysfs on hardware which supports slots and
hotplugging.
And if I'm reading code correctly, this combination is allowed, right?
Via these new patches it is possible to disable slot reset and enable
bus reset.
That's true, a slot reset is simply a bus reset wrapped around code
that prevents the device from getting ejected.
Yes, this makes slot reset "safe". But bus reset is "unsafe".
quoted
Maybe it would make
sense to combine the two as far as this interface is concerned, ie. a
single "bus" reset method that will always use slot reset when
available. Thanks,
That should work when slot reset is available.
Other option is that mentioned remove-reset-rescan procedure.
That's not something we can introduce to the pci_reset_function() path
without a fair bit of collateral in using it through vfio-pci.
quoted
But quick search in drivers/pci/hotplug/ results that not all hotplug
drivers implement reset_slot method.
So there is a possible issue with hotplug driver which may eject device
during bus reset (because e.g. slot reset is not implemented)?
People aren't reporting it, so maybe those controllers aren't being
used for this use case. Or maybe introducing this patch will make
these reset methods more readily accessible for testing. We can fix or
blacklist those controllers for bus reset when reports come in. Thanks,
Ok! I do not know neither if those controllers are used, but looks like
that there are still changes in hotplug code.
So I guess with these patches people can test it and report issues when
such thing happen.
So after a bit research as I understood we need to group slot
and bus reset together in a single category of reset methods and
then implicitly use slot reset if it is available when bus reset is
enabled by the user.
Is that right?
Yes, I understand it in same way. Just I do not know which name to
choose for this reset category. In PCI spec it is called Secondary Bus
Reset (as it resets whole bus with all devices; but we allow this reset
in this patch series only if on the bus is connected exactly one device).
In PCIe spec it is called Hot Reset. And if kernel detects Slot support
then kernel currently calls it Slot reset. But it is still same thing.
Any opinion? I think that we could call it Hot Reset as this patch
series exports it only for single device (so calling it _bus_ is not the
best match).
A similar abstraction where our scope is not limited to a single
function calls this a bus reset:
int pci_reset_bus(struct pci_dev *pdev)
{
return (!pci_probe_reset_slot(pdev->slot)) ?
__pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
}
Thanks,
Alex
I was going to use similar function
int pci_bus_reset(struct pci_dev *dev, int probe)
{
return pci_dev_reset_slot_function(dev, probe) ?
pci_parent_bus_reset(dev, probe) : 0;
}
I think via the sysfs attribute we can simply call this "bus" reset,
but internally having both pci_reset_bus() and pci_bus_reset() would be
really confusing. We're doing the same thing as pci_bus_reset() but
with a different scope, so I'd probably suggest
pci_bus_reset_function().
I'm already confusing them, s/bus_reset/reset_bus/ in the last sentence
above. Thanks,
Alex
Also, the above ternary form isn't true to the original, only -ENOTTY
allows fall-through, so something more like:
int pci_reset_bus_function(struct pci_dev *dev, int probe)
{
int rc = pci_dev_reset_slot_function(dev, probe);
return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
}
Thanks,
Alex
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-24 10:03:54
On Mon, Mar 22, 2021 at 11:10:03AM -0600, Alex Williamson wrote:
On Sun, 21 Mar 2021 10:40:55 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Sat, Mar 20, 2021 at 08:59:42AM -0600, Alex Williamson wrote:
quoted
On Sat, 20 Mar 2021 11:10:08 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 10:23:13AM -0600, Alex Williamson wrote:
quoted
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
What does it mean "none"? Does it mean nothing supported? If yes, I think that
pci_warn() will be enough. At least for me, taint is usable during debug stages,
probably if device doesn't crash no one will look to see /proc/sys/kernel/tainted.
"none" as implemented in this patch, clearing the enabled function
reset methods.
It is far from intuitive, the empty string will be easier to understand,
because "none" means no reset at all.
"No reset at all" is what "none" achieves, the
pci_dev.reset_methods_enabled bitmap is cleared. We can use an empty
string, but I think we want a way to clear all enabled resets and a way
to return it to the default. I could see arguments for an empty string
serving either purpose, so this version proposed explicitly using
"none" and "default", as included in the ABI update.
I will stick with "default" only and leave "none" for something else.
quoted
quoted
quoted
quoted
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
I don't know enough to discuss the PCI details, but you raised good point.
This sysfs is user visible API that is presented as is from device point
of view. It can be easily run into problems if PCI/core doesn't work with
user's choice.
quoted
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
But this patch was presented as a way to overcome situations where
supported != working and user magically knows which reset type to set.
It's not magic, the new sysfs attributes expose which resets are
enabled and the order that they're used, the user can simply select the
next one. Being able to bypass a broken reset method is a helpful side
effect of getting to select a preferred reset method.
Magic in a sense that user has no idea what those resets mean, the
expectation is that he will blindly iterate till something works.
Which ought to actually be a safe thing to do. We should have quirks to
exclude resets that are known broken but still probe as present and I'd
be perfectly fine if we issue a warning if the user disables all resets
for a given device.
quoted
quoted
quoted
If you want to take this patch to be policy decision tool,
it will need to accept "reset_type1,reset_type2,..." sort of input,
so fallback will work natively.
I don't see that as a requirement. We have fall-through support in the
kernel, but for a given device we're really only ever going to make use
of one of those methods. If a user knows enough about a device to have
a preference, I think it can be singular. That also significantly
simplifies the interface and supporting code. Thanks,
I'm struggling to get requirements from this thread. You talked about
policy decision to overtake fallback mechanism, Amey wanted to avoid
quirks.
Do you have an example of such devices or we are talking about
theoretical case?
Look at any device that already has a reset quirk and the process it
took to get there. Those are more than just theoretical cases.
So let's fix the process. The long standing kernel policy is that kernel
bugs (and missing quirk can be seen as such bug) should be fixed in the
kernel and not workaround by the users.
For policy preference, I already described how I've configured QEMU to
prefer a bus reset rather than a PM reset due to lack of specification
regarding the scope of a PM "soft reset". This interface would allow a
system policy to do that same thing.
I don't think anyone is suggesting this as a means to avoid quirks that
would resolve reset issues and create the best default general behavior.
This provides a mechanism to test various reset methods, and thereby
identify broken methods, and set a policy. Sure, that policy might be
to avoid a broken reset in the interim before it gets quirked and
there's potential for abuse there, but I think the benefits outweigh
the risks.
This interface is proposed as first class citizen in the general sysfs
layout. Of course, it will be seen as a way to bypass the kernel.
At least, put it under CONFIG_EXPERT option, so no distro will enable it
by default.
quoted
And I don't see why simple line parser with loop iterator over strchr()
suddenly becomes complicated code.
Setting multiple bits in a bitmap is easy. How do you then go on to
allow the user to specify an ordering preference? If you have an
algorithm you'd like to propose that allows the user to manage the
ordering when enabling multiple methods without substantially
increasing the complexity, please share. IMO, a given device will
generally use one reset method and it seems sufficient to restrict user
preference to achieve all the use cases I've noted. Thanks,
From: Alex Williamson <hidden> Date: 2021-03-24 14:38:43
On Wed, 24 Mar 2021 12:03:00 +0200
Leon Romanovsky [off-list ref] wrote:
On Mon, Mar 22, 2021 at 11:10:03AM -0600, Alex Williamson wrote:
quoted
On Sun, 21 Mar 2021 10:40:55 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Sat, Mar 20, 2021 at 08:59:42AM -0600, Alex Williamson wrote:
quoted
On Sat, 20 Mar 2021 11:10:08 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 10:23:13AM -0600, Alex Williamson wrote:
quoted
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
What does it mean "none"? Does it mean nothing supported? If yes, I think that
pci_warn() will be enough. At least for me, taint is usable during debug stages,
probably if device doesn't crash no one will look to see /proc/sys/kernel/tainted.
"none" as implemented in this patch, clearing the enabled function
reset methods.
It is far from intuitive, the empty string will be easier to understand,
because "none" means no reset at all.
"No reset at all" is what "none" achieves, the
pci_dev.reset_methods_enabled bitmap is cleared. We can use an empty
string, but I think we want a way to clear all enabled resets and a way
to return it to the default. I could see arguments for an empty string
serving either purpose, so this version proposed explicitly using
"none" and "default", as included in the ABI update.
I will stick with "default" only and leave "none" for something else.
Are you suggesting writing "default" restores the unmodified behavior
and writing an empty string clears all enabled reset methods?
quoted
quoted
quoted
quoted
quoted
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
I don't know enough to discuss the PCI details, but you raised good point.
This sysfs is user visible API that is presented as is from device point
of view. It can be easily run into problems if PCI/core doesn't work with
user's choice.
quoted
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
But this patch was presented as a way to overcome situations where
supported != working and user magically knows which reset type to set.
It's not magic, the new sysfs attributes expose which resets are
enabled and the order that they're used, the user can simply select the
next one. Being able to bypass a broken reset method is a helpful side
effect of getting to select a preferred reset method.
Magic in a sense that user has no idea what those resets mean, the
expectation is that he will blindly iterate till something works.
Which ought to actually be a safe thing to do. We should have quirks to
exclude resets that are known broken but still probe as present and I'd
be perfectly fine if we issue a warning if the user disables all resets
for a given device.
quoted
quoted
quoted
If you want to take this patch to be policy decision tool,
it will need to accept "reset_type1,reset_type2,..." sort of input,
so fallback will work natively.
I don't see that as a requirement. We have fall-through support in the
kernel, but for a given device we're really only ever going to make use
of one of those methods. If a user knows enough about a device to have
a preference, I think it can be singular. That also significantly
simplifies the interface and supporting code. Thanks,
I'm struggling to get requirements from this thread. You talked about
policy decision to overtake fallback mechanism, Amey wanted to avoid
quirks.
Do you have an example of such devices or we are talking about
theoretical case?
Look at any device that already has a reset quirk and the process it
took to get there. Those are more than just theoretical cases.
So let's fix the process. The long standing kernel policy is that kernel
bugs (and missing quirk can be seen as such bug) should be fixed in the
kernel and not workaround by the users.
I don't see an actual proposal here to fix the process. Allowing
specific reset methods to be trivially tested is a step towards fixing
the process. Unfortunately we can't tell the difference between
someone setting a policy because they prefer a reset mechanism, are
testing a reset mechanism, or they're avoiding a broken reset mechanism.
We can't force participation if we've made it clear that the interface
should not be used long term for anything other than policy preference
and testing.
quoted
For policy preference, I already described how I've configured QEMU to
prefer a bus reset rather than a PM reset due to lack of specification
regarding the scope of a PM "soft reset". This interface would allow a
system policy to do that same thing.
I don't think anyone is suggesting this as a means to avoid quirks that
would resolve reset issues and create the best default general behavior.
This provides a mechanism to test various reset methods, and thereby
identify broken methods, and set a policy. Sure, that policy might be
to avoid a broken reset in the interim before it gets quirked and
there's potential for abuse there, but I think the benefits outweigh
the risks.
This interface is proposed as first class citizen in the general sysfs
layout. Of course, it will be seen as a way to bypass the kernel.
At least, put it under CONFIG_EXPERT option, so no distro will enable it
by default.
Of course we're proposing it to be accessible, it should also require
admin privileges to modify, sysfs has lots of such things. If it's
relegated to non-default accessibility, it won't be used for testing
and it won't be available for system policy and it's pointless.
quoted
quoted
And I don't see why simple line parser with loop iterator over strchr()
suddenly becomes complicated code.
Setting multiple bits in a bitmap is easy. How do you then go on to
allow the user to specify an ordering preference? If you have an
algorithm you'd like to propose that allows the user to manage the
ordering when enabling multiple methods without substantially
increasing the complexity, please share. IMO, a given device will
generally use one reset method and it seems sufficient to restrict user
preference to achieve all the use cases I've noted. Thanks,
Linked list + iterator will do the trick.
So you're suggesting to add potentially multiple dynamic allocations per
device and list locking and management for an unspecified use case for
an interface you seem to be opposed to anyway. It should be pretty
clear why the keep-it-simple approach was taken in this series. Thanks,
Alex
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-24 15:14:52
On Wed, Mar 24, 2021 at 08:37:43AM -0600, Alex Williamson wrote:
On Wed, 24 Mar 2021 12:03:00 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Mon, Mar 22, 2021 at 11:10:03AM -0600, Alex Williamson wrote:
quoted
On Sun, 21 Mar 2021 10:40:55 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Sat, Mar 20, 2021 at 08:59:42AM -0600, Alex Williamson wrote:
quoted
On Sat, 20 Mar 2021 11:10:08 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 10:23:13AM -0600, Alex Williamson wrote:
quoted
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
What does it mean "none"? Does it mean nothing supported? If yes, I think that
pci_warn() will be enough. At least for me, taint is usable during debug stages,
probably if device doesn't crash no one will look to see /proc/sys/kernel/tainted.
"none" as implemented in this patch, clearing the enabled function
reset methods.
It is far from intuitive, the empty string will be easier to understand,
because "none" means no reset at all.
"No reset at all" is what "none" achieves, the
pci_dev.reset_methods_enabled bitmap is cleared. We can use an empty
string, but I think we want a way to clear all enabled resets and a way
to return it to the default. I could see arguments for an empty string
serving either purpose, so this version proposed explicitly using
"none" and "default", as included in the ABI update.
I will stick with "default" only and leave "none" for something else.
Are you suggesting writing "default" restores the unmodified behavior
and writing an empty string clears all enabled reset methods?
quoted
quoted
quoted
quoted
quoted
quoted
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
I don't know enough to discuss the PCI details, but you raised good point.
This sysfs is user visible API that is presented as is from device point
of view. It can be easily run into problems if PCI/core doesn't work with
user's choice.
quoted
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
But this patch was presented as a way to overcome situations where
supported != working and user magically knows which reset type to set.
It's not magic, the new sysfs attributes expose which resets are
enabled and the order that they're used, the user can simply select the
next one. Being able to bypass a broken reset method is a helpful side
effect of getting to select a preferred reset method.
Magic in a sense that user has no idea what those resets mean, the
expectation is that he will blindly iterate till something works.
Which ought to actually be a safe thing to do. We should have quirks to
exclude resets that are known broken but still probe as present and I'd
be perfectly fine if we issue a warning if the user disables all resets
for a given device.
quoted
quoted
quoted
If you want to take this patch to be policy decision tool,
it will need to accept "reset_type1,reset_type2,..." sort of input,
so fallback will work natively.
I don't see that as a requirement. We have fall-through support in the
kernel, but for a given device we're really only ever going to make use
of one of those methods. If a user knows enough about a device to have
a preference, I think it can be singular. That also significantly
simplifies the interface and supporting code. Thanks,
I'm struggling to get requirements from this thread. You talked about
policy decision to overtake fallback mechanism, Amey wanted to avoid
quirks.
Do you have an example of such devices or we are talking about
theoretical case?
Look at any device that already has a reset quirk and the process it
took to get there. Those are more than just theoretical cases.
So let's fix the process. The long standing kernel policy is that kernel
bugs (and missing quirk can be seen as such bug) should be fixed in the
kernel and not workaround by the users.
I don't see an actual proposal here to fix the process. Allowing
specific reset methods to be trivially tested is a step towards fixing
the process. Unfortunately we can't tell the difference between
someone setting a policy because they prefer a reset mechanism, are
testing a reset mechanism, or they're avoiding a broken reset mechanism.
We can't force participation if we've made it clear that the interface
should not be used long term for anything other than policy preference
and testing.
Yes, and real testing/debugging almost always requires kernel rebuild.
Everything else is waste of time.
quoted
quoted
For policy preference, I already described how I've configured QEMU to
prefer a bus reset rather than a PM reset due to lack of specification
regarding the scope of a PM "soft reset". This interface would allow a
system policy to do that same thing.
I don't think anyone is suggesting this as a means to avoid quirks that
would resolve reset issues and create the best default general behavior.
This provides a mechanism to test various reset methods, and thereby
identify broken methods, and set a policy. Sure, that policy might be
to avoid a broken reset in the interim before it gets quirked and
there's potential for abuse there, but I think the benefits outweigh
the risks.
This interface is proposed as first class citizen in the general sysfs
layout. Of course, it will be seen as a way to bypass the kernel.
At least, put it under CONFIG_EXPERT option, so no distro will enable it
by default.
Of course we're proposing it to be accessible, it should also require
admin privileges to modify, sysfs has lots of such things. If it's
relegated to non-default accessibility, it won't be used for testing
and it won't be available for system policy and it's pointless.
We probably have difference in view of what testing is. I expect from
the users who experience issues with reset to do extra steps and one of
them is to require from them to compile their kernel.
The root permissions doesn't protect from anything, SO lovers will use
root without even thinking twice.
quoted
quoted
quoted
And I don't see why simple line parser with loop iterator over strchr()
suddenly becomes complicated code.
Setting multiple bits in a bitmap is easy. How do you then go on to
allow the user to specify an ordering preference? If you have an
algorithm you'd like to propose that allows the user to manage the
ordering when enabling multiple methods without substantially
increasing the complexity, please share. IMO, a given device will
generally use one reset method and it seems sufficient to restrict user
preference to achieve all the use cases I've noted. Thanks,
Linked list + iterator will do the trick.
So you're suggesting to add potentially multiple dynamic allocations per
device and list locking and management for an unspecified use case for
an interface you seem to be opposed to anyway. It should be pretty
clear why the keep-it-simple approach was taken in this series. Thanks,
I'm trying to help you with your use case of providing reset policy
mechanism, which can be without CONFIG_EXPERT. However if you want
to continue path of having specific reset type only, please ensure
that this is not taken to the "bypass kernel" direction.
Thanks
From: Alex Williamson <hidden> Date: 2021-03-24 17:18:18
On Wed, 24 Mar 2021 17:13:56 +0200
Leon Romanovsky [off-list ref] wrote:
On Wed, Mar 24, 2021 at 08:37:43AM -0600, Alex Williamson wrote:
quoted
On Wed, 24 Mar 2021 12:03:00 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Mon, Mar 22, 2021 at 11:10:03AM -0600, Alex Williamson wrote:
quoted
On Sun, 21 Mar 2021 10:40:55 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Sat, Mar 20, 2021 at 08:59:42AM -0600, Alex Williamson wrote:
quoted
On Sat, 20 Mar 2021 11:10:08 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 10:23:13AM -0600, Alex Williamson wrote:
quoted
What if we taint the kernel or pci_warn() for cases where either all
the reset methods are disabled, ie. 'echo none > reset_method', or any
time a device specific method is disabled?
What does it mean "none"? Does it mean nothing supported? If yes, I think that
pci_warn() will be enough. At least for me, taint is usable during debug stages,
probably if device doesn't crash no one will look to see /proc/sys/kernel/tainted.
"none" as implemented in this patch, clearing the enabled function
reset methods.
It is far from intuitive, the empty string will be easier to understand,
because "none" means no reset at all.
"No reset at all" is what "none" achieves, the
pci_dev.reset_methods_enabled bitmap is cleared. We can use an empty
string, but I think we want a way to clear all enabled resets and a way
to return it to the default. I could see arguments for an empty string
serving either purpose, so this version proposed explicitly using
"none" and "default", as included in the ABI update.
I will stick with "default" only and leave "none" for something else.
Are you suggesting writing "default" restores the unmodified behavior
and writing an empty string clears all enabled reset methods?
quoted
quoted
quoted
quoted
quoted
quoted
I'd almost go so far as to prevent disabling a device specific reset
altogether, but for example should a device specific reset that fixes
an aspect of FLR behavior prevent using a bus reset? I'd prefer in that
case if direct FLR were disabled via a device flag introduced with the
quirk and the remaining resets can still be selected by preference.
I don't know enough to discuss the PCI details, but you raised good point.
This sysfs is user visible API that is presented as is from device point
of view. It can be easily run into problems if PCI/core doesn't work with
user's choice.
quoted
Theoretically all the other reset methods work and are available, it's
only a policy decision which to use, right?
But this patch was presented as a way to overcome situations where
supported != working and user magically knows which reset type to set.
It's not magic, the new sysfs attributes expose which resets are
enabled and the order that they're used, the user can simply select the
next one. Being able to bypass a broken reset method is a helpful side
effect of getting to select a preferred reset method.
Magic in a sense that user has no idea what those resets mean, the
expectation is that he will blindly iterate till something works.
Which ought to actually be a safe thing to do. We should have quirks to
exclude resets that are known broken but still probe as present and I'd
be perfectly fine if we issue a warning if the user disables all resets
for a given device.
quoted
quoted
quoted
If you want to take this patch to be policy decision tool,
it will need to accept "reset_type1,reset_type2,..." sort of input,
so fallback will work natively.
I don't see that as a requirement. We have fall-through support in the
kernel, but for a given device we're really only ever going to make use
of one of those methods. If a user knows enough about a device to have
a preference, I think it can be singular. That also significantly
simplifies the interface and supporting code. Thanks,
I'm struggling to get requirements from this thread. You talked about
policy decision to overtake fallback mechanism, Amey wanted to avoid
quirks.
Do you have an example of such devices or we are talking about
theoretical case?
Look at any device that already has a reset quirk and the process it
took to get there. Those are more than just theoretical cases.
So let's fix the process. The long standing kernel policy is that kernel
bugs (and missing quirk can be seen as such bug) should be fixed in the
kernel and not workaround by the users.
I don't see an actual proposal here to fix the process. Allowing
specific reset methods to be trivially tested is a step towards fixing
the process. Unfortunately we can't tell the difference between
someone setting a policy because they prefer a reset mechanism, are
testing a reset mechanism, or they're avoiding a broken reset mechanism.
We can't force participation if we've made it clear that the interface
should not be used long term for anything other than policy preference
and testing.
Yes, and real testing/debugging almost always requires kernel rebuild.
Everything else is waste of time.
Sorry, this is nonsense. Allowing users to debug issues without a full
kernel rebuild is a good thing.
quoted
quoted
quoted
For policy preference, I already described how I've configured QEMU to
prefer a bus reset rather than a PM reset due to lack of specification
regarding the scope of a PM "soft reset". This interface would allow a
system policy to do that same thing.
I don't think anyone is suggesting this as a means to avoid quirks that
would resolve reset issues and create the best default general behavior.
This provides a mechanism to test various reset methods, and thereby
identify broken methods, and set a policy. Sure, that policy might be
to avoid a broken reset in the interim before it gets quirked and
there's potential for abuse there, but I think the benefits outweigh
the risks.
This interface is proposed as first class citizen in the general sysfs
layout. Of course, it will be seen as a way to bypass the kernel.
At least, put it under CONFIG_EXPERT option, so no distro will enable it
by default.
Of course we're proposing it to be accessible, it should also require
admin privileges to modify, sysfs has lots of such things. If it's
relegated to non-default accessibility, it won't be used for testing
and it won't be available for system policy and it's pointless.
We probably have difference in view of what testing is. I expect from
the users who experience issues with reset to do extra steps and one of
them is to require from them to compile their kernel.
I would define the ability to generate a CI test that can pick a
device, unbind it from its driver, and iterate reset methods as a
worthwhile improvement in testing.
The root permissions doesn't protect from anything, SO lovers will use
root without even thinking twice.
Yes, with great power comes great responsibility. Many admins ignore
this. That's far beyond the scope of this series.
quoted
quoted
quoted
quoted
And I don't see why simple line parser with loop iterator over strchr()
suddenly becomes complicated code.
Setting multiple bits in a bitmap is easy. How do you then go on to
allow the user to specify an ordering preference? If you have an
algorithm you'd like to propose that allows the user to manage the
ordering when enabling multiple methods without substantially
increasing the complexity, please share. IMO, a given device will
generally use one reset method and it seems sufficient to restrict user
preference to achieve all the use cases I've noted. Thanks,
Linked list + iterator will do the trick.
So you're suggesting to add potentially multiple dynamic allocations per
device and list locking and management for an unspecified use case for
an interface you seem to be opposed to anyway. It should be pretty
clear why the keep-it-simple approach was taken in this series. Thanks,
I'm trying to help you with your use case of providing reset policy
mechanism, which can be without CONFIG_EXPERT. However if you want
to continue path of having specific reset type only, please ensure
that this is not taken to the "bypass kernel" direction.
You've lost me, are you saying you'd be in favor of an interface that
allows an admin to specify an arbitrary list of reset methods because
that's somehow more in line with a policy choice than a userspace
workaround? This seems like unnecessary bloat because (a) it allows
the same bypass mechanism, and (b) a given device is only going to use
a single method anyway, so the functionality is unnecessary. Please
help me understand how this favors the policy use case. Thanks,
Alex
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-25 08:38:52
On Wed, Mar 24, 2021 at 11:17:29AM -0600, Alex Williamson wrote:
On Wed, 24 Mar 2021 17:13:56 +0200
Leon Romanovsky [off-list ref] wrote:
<...>
quoted
Yes, and real testing/debugging almost always requires kernel rebuild.
Everything else is waste of time.
Sorry, this is nonsense. Allowing users to debug issues without a full
kernel rebuild is a good thing.
It is far from debug, this interface doesn't give you any answers why
the reset didn't work, it just helps you to find the one that works.
Unless you believe that this information will be enough to understand
the root cause, you will need to ask from the user to perform extra
tests, maybe try some quirk. All of that requires from the users to
rebuild their kernel.
So no, it is not debug.
quoted
quoted
quoted
quoted
For policy preference, I already described how I've configured QEMU to
prefer a bus reset rather than a PM reset due to lack of specification
regarding the scope of a PM "soft reset". This interface would allow a
system policy to do that same thing.
I don't think anyone is suggesting this as a means to avoid quirks that
would resolve reset issues and create the best default general behavior.
This provides a mechanism to test various reset methods, and thereby
identify broken methods, and set a policy. Sure, that policy might be
to avoid a broken reset in the interim before it gets quirked and
there's potential for abuse there, but I think the benefits outweigh
the risks.
This interface is proposed as first class citizen in the general sysfs
layout. Of course, it will be seen as a way to bypass the kernel.
At least, put it under CONFIG_EXPERT option, so no distro will enable it
by default.
Of course we're proposing it to be accessible, it should also require
admin privileges to modify, sysfs has lots of such things. If it's
relegated to non-default accessibility, it won't be used for testing
and it won't be available for system policy and it's pointless.
We probably have difference in view of what testing is. I expect from
the users who experience issues with reset to do extra steps and one of
them is to require from them to compile their kernel.
I would define the ability to generate a CI test that can pick a
device, unbind it from its driver, and iterate reset methods as a
worthwhile improvement in testing.
Who is going to run this CI? At least all kernel CIs (external and
internal to HW vendors) that I'm familiar are building kernel themselves.
Distro kernel is too bloat to be really usable for CI.
quoted
The root permissions doesn't protect from anything, SO lovers will use
root without even thinking twice.
Yes, with great power comes great responsibility. Many admins ignore
this. That's far beyond the scope of this series.
<...>
quoted
I'm trying to help you with your use case of providing reset policy
mechanism, which can be without CONFIG_EXPERT. However if you want
to continue path of having specific reset type only, please ensure
that this is not taken to the "bypass kernel" direction.
You've lost me, are you saying you'd be in favor of an interface that
allows an admin to specify an arbitrary list of reset methods because
that's somehow more in line with a policy choice than a userspace
workaround? This seems like unnecessary bloat because (a) it allows
the same bypass mechanism, and (b) a given device is only going to use
a single method anyway, so the functionality is unnecessary. Please
help me understand how this favors the policy use case. Thanks,
The policy decision is global logic that is easier to grasp. At some
point of our discussion, you presented the case where PM reset is not
defined well and you prefer to do bus reset (something like that).
I expect that QEMU sets same reset policy for all devices at the same
time instead of trying per-device to guess which one works.
And yes, you will be able to bypass kernel, but at least this interface
will be broader than initial one that serves only SO and workarounds.
Thanks
From: Alex Williamson <hidden> Date: 2021-03-25 14:56:27
On Thu, 25 Mar 2021 10:37:54 +0200
Leon Romanovsky [off-list ref] wrote:
On Wed, Mar 24, 2021 at 11:17:29AM -0600, Alex Williamson wrote:
quoted
On Wed, 24 Mar 2021 17:13:56 +0200
Leon Romanovsky [off-list ref] wrote:
<...>
quoted
quoted
Yes, and real testing/debugging almost always requires kernel rebuild.
Everything else is waste of time.
Sorry, this is nonsense. Allowing users to debug issues without a full
kernel rebuild is a good thing.
It is far from debug, this interface doesn't give you any answers why
the reset didn't work, it just helps you to find the one that works.
Unless you believe that this information will be enough to understand
the root cause, you will need to ask from the user to perform extra
tests, maybe try some quirk. All of that requires from the users to
rebuild their kernel.
So no, it is not debug.
It allows a user to experiment to determine (a) my device doesn't work
in a given scenario with the default configuration, but (b) if I change
the reset to this other thing it does work. That is a step in
debugging.
It's absurd to think that a sysfs attribute could provide root cause,
but it might be enough for someone to further help that user. It would
be a useful clue for a bug report. Yes, reaching root cause might
involve building a kernel, but that doesn't invalidate that having a
step towards debugging in the base kernel might be a useful tool.
quoted
quoted
quoted
quoted
quoted
For policy preference, I already described how I've configured QEMU to
prefer a bus reset rather than a PM reset due to lack of specification
regarding the scope of a PM "soft reset". This interface would allow a
system policy to do that same thing.
I don't think anyone is suggesting this as a means to avoid quirks that
would resolve reset issues and create the best default general behavior.
This provides a mechanism to test various reset methods, and thereby
identify broken methods, and set a policy. Sure, that policy might be
to avoid a broken reset in the interim before it gets quirked and
there's potential for abuse there, but I think the benefits outweigh
the risks.
This interface is proposed as first class citizen in the general sysfs
layout. Of course, it will be seen as a way to bypass the kernel.
At least, put it under CONFIG_EXPERT option, so no distro will enable it
by default.
Of course we're proposing it to be accessible, it should also require
admin privileges to modify, sysfs has lots of such things. If it's
relegated to non-default accessibility, it won't be used for testing
and it won't be available for system policy and it's pointless.
We probably have difference in view of what testing is. I expect from
the users who experience issues with reset to do extra steps and one of
them is to require from them to compile their kernel.
I would define the ability to generate a CI test that can pick a
device, unbind it from its driver, and iterate reset methods as a
worthwhile improvement in testing.
Who is going to run this CI? At least all kernel CIs (external and
internal to HW vendors) that I'm familiar are building kernel themselves.
Distro kernel is too bloat to be really usable for CI.
At this point I'm suspicious you're trolling. A distro kernel CI
certainly uses the kernel they intend to ship and support in their
environment. You're concerned about a bloated kernel, but the proposal
here adds 2-bytes per device to track reset methods and a trivial array
in text memory, meanwhile you're proposing multiple per-device memory
allocations to enhance the feature you think is too bloated for CI.
quoted
quoted
The root permissions doesn't protect from anything, SO lovers will use
root without even thinking twice.
Yes, with great power comes great responsibility. Many admins ignore
this. That's far beyond the scope of this series.
<...>
quoted
quoted
I'm trying to help you with your use case of providing reset policy
mechanism, which can be without CONFIG_EXPERT. However if you want
to continue path of having specific reset type only, please ensure
that this is not taken to the "bypass kernel" direction.
You've lost me, are you saying you'd be in favor of an interface that
allows an admin to specify an arbitrary list of reset methods because
that's somehow more in line with a policy choice than a userspace
workaround? This seems like unnecessary bloat because (a) it allows
the same bypass mechanism, and (b) a given device is only going to use
a single method anyway, so the functionality is unnecessary. Please
help me understand how this favors the policy use case. Thanks,
The policy decision is global logic that is easier to grasp. At some
point of our discussion, you presented the case where PM reset is not
defined well and you prefer to do bus reset (something like that).
I expect that QEMU sets same reset policy for all devices at the same
time instead of trying per-device to guess which one works.
And yes, you will be able to bypass kernel, but at least this interface
will be broader than initial one that serves only SO and workarounds.
I still think allocating objects for a list and managing that list is
too bloated and complicated, but I agree that being able to have more
fine grained control could be useful. Is it necessary to be able to
re-order reset methods or might it still be better aligned to a policy
use case if we allow plus and minus operators? For example, a device
might list:
[pm] [bus]
Indicating that PM and bus reset are both available and enabled. The
user could do:
echo -pm > reset_methods
This would result in:
pm [bus]
Indicating that both PM and bus resets are available, but only bus reset
is enabled (note this is the identical result to "echo bus >" in the
current proposal). "echo +pm" or "echo default" could re-enable the PM
reset. Would something like that be satisfactory?
If we need to allow re-ording, we'd want to use a byte-array where each
byte indicates a type of reset and perhaps a non-zero value in the
array indicates the method is enabled and the value indicates priority.
For example writing "dev_spec,flr,bus" would parse to write 1 to the
byte associated with the device specific reset, 2 to flr, 3 to bus
reset, then we'd process low to high (or maybe starting at a high value
to count down to zero might be more simple). We could do that with
only adding less than a fixed 8-bytes per device and no dynamic
allocation. Thoughts? Thanks,
Alex
From: Leon Romanovsky <leon@kernel.org> Date: 2021-03-25 16:11:06
On Thu, Mar 25, 2021 at 08:55:04AM -0600, Alex Williamson wrote:
On Thu, 25 Mar 2021 10:37:54 +0200
Leon Romanovsky [off-list ref] wrote:
quoted
On Wed, Mar 24, 2021 at 11:17:29AM -0600, Alex Williamson wrote:
quoted
On Wed, 24 Mar 2021 17:13:56 +0200
Leon Romanovsky [off-list ref] wrote:
<...>
quoted
quoted
Yes, and real testing/debugging almost always requires kernel rebuild.
Everything else is waste of time.
Sorry, this is nonsense. Allowing users to debug issues without a full
kernel rebuild is a good thing.
It is far from debug, this interface doesn't give you any answers why
the reset didn't work, it just helps you to find the one that works.
Unless you believe that this information will be enough to understand
the root cause, you will need to ask from the user to perform extra
tests, maybe try some quirk. All of that requires from the users to
rebuild their kernel.
So no, it is not debug.
It allows a user to experiment to determine (a) my device doesn't work
in a given scenario with the default configuration, but (b) if I change
the reset to this other thing it does work. That is a step in
debugging.
It's absurd to think that a sysfs attribute could provide root cause,
but it might be enough for someone to further help that user. It would
be a useful clue for a bug report. Yes, reaching root cause might
involve building a kernel, but that doesn't invalidate that having a
step towards debugging in the base kernel might be a useful tool.
Let's agree to do not agree.
quoted
quoted
quoted
quoted
quoted
quoted
For policy preference, I already described how I've configured QEMU to
prefer a bus reset rather than a PM reset due to lack of specification
regarding the scope of a PM "soft reset". This interface would allow a
system policy to do that same thing.
I don't think anyone is suggesting this as a means to avoid quirks that
would resolve reset issues and create the best default general behavior.
This provides a mechanism to test various reset methods, and thereby
identify broken methods, and set a policy. Sure, that policy might be
to avoid a broken reset in the interim before it gets quirked and
there's potential for abuse there, but I think the benefits outweigh
the risks.
This interface is proposed as first class citizen in the general sysfs
layout. Of course, it will be seen as a way to bypass the kernel.
At least, put it under CONFIG_EXPERT option, so no distro will enable it
by default.
Of course we're proposing it to be accessible, it should also require
admin privileges to modify, sysfs has lots of such things. If it's
relegated to non-default accessibility, it won't be used for testing
and it won't be available for system policy and it's pointless.
We probably have difference in view of what testing is. I expect from
the users who experience issues with reset to do extra steps and one of
them is to require from them to compile their kernel.
I would define the ability to generate a CI test that can pick a
device, unbind it from its driver, and iterate reset methods as a
worthwhile improvement in testing.
Who is going to run this CI? At least all kernel CIs (external and
internal to HW vendors) that I'm familiar are building kernel themselves.
Distro kernel is too bloat to be really usable for CI.
At this point I'm suspicious you're trolling. A distro kernel CI
certainly uses the kernel they intend to ship and support in their
environment. You're concerned about a bloated kernel, but the proposal
here adds 2-bytes per device to track reset methods and a trivial array
in text memory, meanwhile you're proposing multiple per-device memory
allocations to enhance the feature you think is too bloated for CI.
I don't know why you decided to focus on memory footprint which is not
important at all during CI runs. The bloat is in Kconfig options that
are not needed. Those extra options add significant overhead during
builds and runs itself.
And not, I'm not trolling, but representing HW vendor that pushes its CI
and developers environment to the limit, by running full kernel builds with
less than 30 seconds and boot-to-test with less than 6 seconds for full
Fedora VM.
quoted
quoted
quoted
The root permissions doesn't protect from anything, SO lovers will use
root without even thinking twice.
Yes, with great power comes great responsibility. Many admins ignore
this. That's far beyond the scope of this series.
<...>
quoted
quoted
I'm trying to help you with your use case of providing reset policy
mechanism, which can be without CONFIG_EXPERT. However if you want
to continue path of having specific reset type only, please ensure
that this is not taken to the "bypass kernel" direction.
You've lost me, are you saying you'd be in favor of an interface that
allows an admin to specify an arbitrary list of reset methods because
that's somehow more in line with a policy choice than a userspace
workaround? This seems like unnecessary bloat because (a) it allows
the same bypass mechanism, and (b) a given device is only going to use
a single method anyway, so the functionality is unnecessary. Please
help me understand how this favors the policy use case. Thanks,
The policy decision is global logic that is easier to grasp. At some
point of our discussion, you presented the case where PM reset is not
defined well and you prefer to do bus reset (something like that).
I expect that QEMU sets same reset policy for all devices at the same
time instead of trying per-device to guess which one works.
And yes, you will be able to bypass kernel, but at least this interface
will be broader than initial one that serves only SO and workarounds.
I still think allocating objects for a list and managing that list is
too bloated and complicated, but I agree that being able to have more
fine grained control could be useful. Is it necessary to be able to
re-order reset methods or might it still be better aligned to a policy
use case if we allow plus and minus operators? For example, a device
might list:
[pm] [bus]
Indicating that PM and bus reset are both available and enabled. The
user could do:
echo -pm > reset_methods
This would result in:
pm [bus]
Indicating that both PM and bus resets are available, but only bus reset
is enabled (note this is the identical result to "echo bus >" in the
current proposal). "echo +pm" or "echo default" could re-enable the PM
reset. Would something like that be satisfactory?
Yes, I actually imagined simpler interface:
To set specific type:
echo pm > reset_methods
To set policy:
echo "pm,bus" > reset_methods
But your proposal is nicer.
If we need to allow re-ording, we'd want to use a byte-array where each
byte indicates a type of reset and perhaps a non-zero value in the
array indicates the method is enabled and the value indicates priority.
For example writing "dev_spec,flr,bus" would parse to write 1 to the
byte associated with the device specific reset, 2 to flr, 3 to bus
reset, then we'd process low to high (or maybe starting at a high value
to count down to zero might be more simple). We could do that with
only adding less than a fixed 8-bytes per device and no dynamic
allocation. Thoughts? Thanks,
Like I suggested, linked list will be easier and the reset will be
something like:
for_each_reset_type(device, type) {
switch (type) {
case PM:
ret = do_some_reset(device);
break;
case BUS:
.....
}
if (!ret || ret == -ENOMEM) <-- go to next type in linked list
return ret;
}