[PATCH net-next 0/2] ibmvnic: sysfs changes

[PATCH net-next 0/2] ibmvnic: sysfs changes

[Lijun Pan]

Patch 1 improves the failover sysfs entry.
Patch 2 adds sysfs entry for timeout and fatal resets.

Lijun Pan (2):
  ibmvnic: improve failover sysfs entry
  ibmvnic: add sysfs entry for timeout and fatal reset

 drivers/net/ethernet/ibm/ibmvnic.c | 56 +++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 5 deletions(-)

-- 
2.23.0

[PATCH net-next 1/2] ibmvnic: improve failover sysfs entry

[Lijun Pan]

The current implementation replies on H_IOCTL call to issue a
H_SESSION_ERR_DETECTED command to let the hypervisor to send a failover
signal. However, it may not work while the vnic is already in error
state, e.g., "ibmvnic 30000003 env3: rx buffer returned with rc 6".
Add a last resort, that is to schedule a failover reset via CRQ command.

Signed-off-by: Lijun Pan <lijunp213@gmail.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index ee9bf18c597f..d44a7b5b8f67 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -5512,14 +5512,14 @@ static ssize_t failover_store(struct device *dev, struct device_attribute *attr,
 	rc = plpar_hcall_norets(H_VIOCTL, adapter->vdev->unit_address,
 				H_SESSION_ERR_DETECTED, session_token, 0, 0);
 	if (rc) {
-		netdev_err(netdev, "Client initiated failover failed, rc %ld\n",
+		netdev_err(netdev,
+			   "H_VIOCTL initiated failover failed, rc %ld, trying to send CRQ_CMD, the last resort\n",
 			   rc);
-		return -EINVAL;
+		ibmvnic_reset(adapter, VNIC_RESET_FAILOVER);
 	}
 
 	return count;
 }
-
 static DEVICE_ATTR_WO(failover);
 
 static unsigned long ibmvnic_get_desired_dma(struct vio_dev *vdev)
-- 
2.23.0

[PATCH net-next 2/2] ibmvnic: add sysfs entry for timeout and fatal reset

[Lijun Pan]

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(-)

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)
 {
-- 
2.23.0

Re: [PATCH net-next 2/2] ibmvnic: add sysfs entry for timeout and fatal reset

[Jakub Kicinski]

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?

Re: [PATCH net-next 2/2] ibmvnic: add sysfs entry for timeout and fatal reset

[Lijun Pan]

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.

Re: [PATCH net-next 2/2] ibmvnic: add sysfs entry for timeout and fatal reset

[Jakub Kicinski]

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.