| Message ID | 20210412074330.9371-3-lijunp213@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ibmvnic: sysfs changes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 10 maintainers not CCed: tlfalcon@linux.ibm.com drt@linux.ibm.com paulus@samba.org sukadev@linux.ibm.com benh@kernel.crashing.org linuxppc-dev@lists.ozlabs.org mpe@ellerman.id.au ljp@linux.ibm.com davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 3 this patch: 3 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 81 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Mon, 12 Apr 2021 02:43:30 -0500 Lijun Pan wrote: > Add timeout and fatal reset sysfs entries so that both functions > can be triggered manually the tested. Otherwise, you have to run > the program for enough time and check both randomly generated > resets in the long long log. This looks more suitable for debugfs. But can't you use ethtool or devlink reset functionality somehow?
On Mon, Apr 12, 2021 at 1:23 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 12 Apr 2021 02:43:30 -0500 Lijun Pan wrote: > > Add timeout and fatal reset sysfs entries so that both functions > > can be triggered manually the tested. Otherwise, you have to run > > the program for enough time and check both randomly generated > > resets in the long long log. > > This looks more suitable for debugfs. > > But can't you use ethtool or devlink reset functionality somehow? ethtool and devlink reset seem better to be implemented by a FAILVOER reset for this driver. ethtool/devlink reset are not implemented in this driver, which will be a todo list for me. This timeout reset can be triggered by tx watchdog, .ndo_tx_timeout->ibmvnic_tx_timeout->ibmvnic_reset(adapter, VNIC_RESET_TIMEOUT); Do you know is there a way to trigger that ndo_tx_timeout from some user space tool? The FATAL reset is triggered by Firmware, quite specific for this driver. So in order to verify that, I put it in sysfs entry.
On Mon, 12 Apr 2021 15:26:00 -0500 Lijun Pan wrote: > On Mon, Apr 12, 2021 at 1:23 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 12 Apr 2021 02:43:30 -0500 Lijun Pan wrote: > > > Add timeout and fatal reset sysfs entries so that both functions > > > can be triggered manually the tested. Otherwise, you have to run > > > the program for enough time and check both randomly generated > > > resets in the long long log. > > > > This looks more suitable for debugfs. > > > > But can't you use ethtool or devlink reset functionality somehow? > > ethtool and devlink reset seem better to be implemented by a FAILVOER reset for > this driver. ethtool/devlink reset are not implemented in this driver, > which will be a todo list for me. ethtool isn't really much to implement, its basically a bunch of ops the driver implements. You can pick and choose which ones you implement. It'd be better to use ethtool or devlink, but I guess debugfs could be acceptable too. sysfs is a stable API, so it's definitely a no-go. > This timeout reset can be triggered by tx watchdog, > .ndo_tx_timeout->ibmvnic_tx_timeout->ibmvnic_reset(adapter, VNIC_RESET_TIMEOUT); > Do you know is there a way to trigger that ndo_tx_timeout from some > user space tool? > > The FATAL reset is triggered by Firmware, quite specific for this driver. > So in order to verify that, I put it in sysfs entry. Good question, I don't think we have a way to trigger the timeout in a generic way. My first instinct would be to use ethtool self test (ethtool_ops->self_test) to call the same function within the driver.
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index d44a7b5b8f67..b4d2c055a284 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -5329,6 +5329,8 @@ static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter, bool reset) return rc; } +static struct device_attribute dev_attr_timeout; +static struct device_attribute dev_attr_fatal; static struct device_attribute dev_attr_failover; static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id) @@ -5407,9 +5409,15 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id) netdev->min_mtu = adapter->min_mtu - ETH_HLEN; netdev->max_mtu = adapter->max_mtu - ETH_HLEN; + rc = device_create_file(&dev->dev, &dev_attr_timeout); + if (rc) + goto ibmvnic_dev_file_timeout_err; + rc = device_create_file(&dev->dev, &dev_attr_fatal); + if (rc) + goto ibmvnic_dev_file_fatal_err; rc = device_create_file(&dev->dev, &dev_attr_failover); if (rc) - goto ibmvnic_dev_file_err; + goto ibmvnic_dev_file_failover_err; netif_carrier_off(netdev); rc = register_netdev(netdev); @@ -5428,7 +5436,13 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id) ibmvnic_register_fail: device_remove_file(&dev->dev, &dev_attr_failover); -ibmvnic_dev_file_err: +ibmvnic_dev_file_failover_err: + device_remove_file(&dev->dev, &dev_attr_fatal); + +ibmvnic_dev_file_fatal_err: + device_remove_file(&dev->dev, &dev_attr_timeout); + +ibmvnic_dev_file_timeout_err: release_stats_token(adapter); ibmvnic_stats_fail: @@ -5481,11 +5495,43 @@ static void ibmvnic_remove(struct vio_dev *dev) rtnl_unlock(); mutex_destroy(&adapter->fw_lock); + device_remove_file(&dev->dev, &dev_attr_timeout); + device_remove_file(&dev->dev, &dev_attr_fatal); device_remove_file(&dev->dev, &dev_attr_failover); free_netdev(netdev); dev_set_drvdata(&dev->dev, NULL); } +static ssize_t timeout_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct net_device *netdev = dev_get_drvdata(dev); + struct ibmvnic_adapter *adapter = netdev_priv(netdev); + + if (!sysfs_streq(buf, "1")) + return -EINVAL; + + ibmvnic_reset(adapter, VNIC_RESET_TIMEOUT); + + return count; +} +static DEVICE_ATTR_WO(timeout); + +static ssize_t fatal_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct net_device *netdev = dev_get_drvdata(dev); + struct ibmvnic_adapter *adapter = netdev_priv(netdev); + + if (!sysfs_streq(buf, "1")) + return -EINVAL; + + ibmvnic_reset(adapter, VNIC_RESET_FATAL); + + return count; +} +static DEVICE_ATTR_WO(fatal); + static ssize_t failover_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) {
Add timeout and fatal reset sysfs entries so that both functions can be triggered manually the tested. Otherwise, you have to run the program for enough time and check both randomly generated resets in the long long log. Signed-off-by: Lijun Pan <lijunp213@gmail.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 50 ++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)