[PATCH net-next 0/2] ibmvnic: Update firmware error reporting

[PATCH net-next 0/2] ibmvnic: Update firmware error reporting

[Thomas Falcon]

This patch set cleans out a lot of dead code from the ibmvnic driver
and adds some more. The error ID field of the descriptor is not filled
in by firmware, so do not print it and do not use it to query for
more detailed information. Remove the unused code written for this.
Finally, update the message to print a string explainng the error
cause instead of just the error code.

Thomas Falcon (2):
  ibmvnic: Remove code to request error information
  ibmvnic: Update firmware error reporting with cause string

 drivers/net/ethernet/ibm/ibmvnic.c | 168 ++++++-------------------------------
 drivers/net/ethernet/ibm/ibmvnic.h |  33 --------
 2 files changed, 26 insertions(+), 175 deletions(-)

-- 
2.12.3

[PATCH net-next 1/2] ibmvnic: Remove code to request error information

[Thomas Falcon]

When backing device firmware reports an error, it provides an
error ID, which is meant to be queried for more detailed error
information. Currently, however, an error ID is not provided by
the Virtual I/O server and there are not any plans to do so. For
now, it is always unfilled or zero, so request_error_information
will never be called.  Remove it.

Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 144 +------------------------------------
 drivers/net/ethernet/ibm/ibmvnic.h |  33 ---------
 2 files changed, 1 insertion(+), 176 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index ffe7acbeaa22..109e4a58efad 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -718,23 +718,6 @@ static int init_tx_pools(struct net_device *netdev)
 	return 0;
 }
 
-static void release_error_buffers(struct ibmvnic_adapter *adapter)
-{
-	struct device *dev = &adapter->vdev->dev;
-	struct ibmvnic_error_buff *error_buff, *tmp;
-	unsigned long flags;
-
-	spin_lock_irqsave(&adapter->error_list_lock, flags);
-	list_for_each_entry_safe(error_buff, tmp, &adapter->errors, list) {
-		list_del(&error_buff->list);
-		dma_unmap_single(dev, error_buff->dma, error_buff->len,
-				 DMA_FROM_DEVICE);
-		kfree(error_buff->buff);
-		kfree(error_buff);
-	}
-	spin_unlock_irqrestore(&adapter->error_list_lock, flags);
-}
-
 static void ibmvnic_napi_enable(struct ibmvnic_adapter *adapter)
 {
 	int i;
@@ -896,7 +879,6 @@ static void release_resources(struct ibmvnic_adapter *adapter)
 	release_tx_pools(adapter);
 	release_rx_pools(adapter);
 
-	release_error_buffers(adapter);
 	release_napi(adapter);
 	release_login_rsp_buffer(adapter);
 }
@@ -3843,133 +3825,16 @@ static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
 	ibmvnic_send_crq(adapter, &crq);
 }
 
-static void handle_error_info_rsp(union ibmvnic_crq *crq,
-				  struct ibmvnic_adapter *adapter)
-{
-	struct device *dev = &adapter->vdev->dev;
-	struct ibmvnic_error_buff *error_buff, *tmp;
-	unsigned long flags;
-	bool found = false;
-	int i;
-
-	if (!crq->request_error_rsp.rc.code) {
-		dev_info(dev, "Request Error Rsp returned with rc=%x\n",
-			 crq->request_error_rsp.rc.code);
-		return;
-	}
-
-	spin_lock_irqsave(&adapter->error_list_lock, flags);
-	list_for_each_entry_safe(error_buff, tmp, &adapter->errors, list)
-		if (error_buff->error_id == crq->request_error_rsp.error_id) {
-			found = true;
-			list_del(&error_buff->list);
-			break;
-		}
-	spin_unlock_irqrestore(&adapter->error_list_lock, flags);
-
-	if (!found) {
-		dev_err(dev, "Couldn't find error id %x\n",
-			be32_to_cpu(crq->request_error_rsp.error_id));
-		return;
-	}
-
-	dev_err(dev, "Detailed info for error id %x:",
-		be32_to_cpu(crq->request_error_rsp.error_id));
-
-	for (i = 0; i < error_buff->len; i++) {
-		pr_cont("%02x", (int)error_buff->buff[i]);
-		if (i % 8 == 7)
-			pr_cont(" ");
-	}
-	pr_cont("\n");
-
-	dma_unmap_single(dev, error_buff->dma, error_buff->len,
-			 DMA_FROM_DEVICE);
-	kfree(error_buff->buff);
-	kfree(error_buff);
-}
-
-static void request_error_information(struct ibmvnic_adapter *adapter,
-				      union ibmvnic_crq *err_crq)
-{
-	struct device *dev = &adapter->vdev->dev;
-	struct net_device *netdev = adapter->netdev;
-	struct ibmvnic_error_buff *error_buff;
-	unsigned long timeout = msecs_to_jiffies(30000);
-	union ibmvnic_crq crq;
-	unsigned long flags;
-	int rc, detail_len;
-
-	error_buff = kmalloc(sizeof(*error_buff), GFP_ATOMIC);
-	if (!error_buff)
-		return;
-
-	detail_len = be32_to_cpu(err_crq->error_indication.detail_error_sz);
-	error_buff->buff = kmalloc(detail_len, GFP_ATOMIC);
-	if (!error_buff->buff) {
-		kfree(error_buff);
-		return;
-	}
-
-	error_buff->dma = dma_map_single(dev, error_buff->buff, detail_len,
-					 DMA_FROM_DEVICE);
-	if (dma_mapping_error(dev, error_buff->dma)) {
-		netdev_err(netdev, "Couldn't map error buffer\n");
-		kfree(error_buff->buff);
-		kfree(error_buff);
-		return;
-	}
-
-	error_buff->len = detail_len;
-	error_buff->error_id = err_crq->error_indication.error_id;
-
-	spin_lock_irqsave(&adapter->error_list_lock, flags);
-	list_add_tail(&error_buff->list, &adapter->errors);
-	spin_unlock_irqrestore(&adapter->error_list_lock, flags);
-
-	memset(&crq, 0, sizeof(crq));
-	crq.request_error_info.first = IBMVNIC_CRQ_CMD;
-	crq.request_error_info.cmd = REQUEST_ERROR_INFO;
-	crq.request_error_info.ioba = cpu_to_be32(error_buff->dma);
-	crq.request_error_info.len = cpu_to_be32(detail_len);
-	crq.request_error_info.error_id = err_crq->error_indication.error_id;
-
-	rc = ibmvnic_send_crq(adapter, &crq);
-	if (rc) {
-		netdev_err(netdev, "failed to request error information\n");
-		goto err_info_fail;
-	}
-
-	if (!wait_for_completion_timeout(&adapter->init_done, timeout)) {
-		netdev_err(netdev, "timeout waiting for error information\n");
-		goto err_info_fail;
-	}
-
-	return;
-
-err_info_fail:
-	spin_lock_irqsave(&adapter->error_list_lock, flags);
-	list_del(&error_buff->list);
-	spin_unlock_irqrestore(&adapter->error_list_lock, flags);
-
-	kfree(error_buff->buff);
-	kfree(error_buff);
-}
-
 static void handle_error_indication(union ibmvnic_crq *crq,
 				    struct ibmvnic_adapter *adapter)
 {
 	struct device *dev = &adapter->vdev->dev;
 
-	dev_err(dev, "Firmware reports %serror id %x, cause %d\n",
+	dev_err(dev, "Firmware reports %serror, cause %d\n",
 		crq->error_indication.flags
 			& IBMVNIC_FATAL_ERROR ? "FATAL " : "",
-		be32_to_cpu(crq->error_indication.error_id),
 		be16_to_cpu(crq->error_indication.error_cause));
 
-	if (be32_to_cpu(crq->error_indication.error_id))
-		request_error_information(adapter, crq);
-
 	if (crq->error_indication.flags & IBMVNIC_FATAL_ERROR)
 		ibmvnic_reset(adapter, VNIC_RESET_FATAL);
 	else
@@ -4468,10 +4333,6 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
 		netdev_dbg(netdev, "Got Error Indication\n");
 		handle_error_indication(crq, adapter);
 		break;
-	case REQUEST_ERROR_RSP:
-		netdev_dbg(netdev, "Got Error Detail Response\n");
-		handle_error_info_rsp(crq, adapter);
-		break;
 	case REQUEST_STATISTICS_RSP:
 		netdev_dbg(netdev, "Got Statistics Response\n");
 		complete(&adapter->stats_done);
@@ -4830,9 +4691,6 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 
 	spin_lock_init(&adapter->stats_lock);
 
-	INIT_LIST_HEAD(&adapter->errors);
-	spin_lock_init(&adapter->error_list_lock);
-
 	INIT_WORK(&adapter->ibmvnic_reset, __ibmvnic_reset);
 	INIT_LIST_HEAD(&adapter->rwi_list);
 	mutex_init(&adapter->reset_lock);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index f9fb780102ac..f06eec145ca6 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -512,24 +512,6 @@ struct ibmvnic_error_indication {
 	u8 reserved2[2];
 } __packed __aligned(8);
 
-struct ibmvnic_request_error_info {
-	u8 first;
-	u8 cmd;
-	u8 reserved[2];
-	__be32 ioba;
-	__be32 len;
-	__be32 error_id;
-} __packed __aligned(8);
-
-struct ibmvnic_request_error_rsp {
-	u8 first;
-	u8 cmd;
-	u8 reserved[2];
-	__be32 error_id;
-	__be32 len;
-	struct ibmvnic_rc rc;
-} __packed __aligned(8);
-
 struct ibmvnic_link_state_indication {
 	u8 first;
 	u8 cmd;
@@ -709,8 +691,6 @@ union ibmvnic_crq {
 	struct ibmvnic_request_debug_stats request_debug_stats;
 	struct ibmvnic_request_debug_stats request_debug_stats_rsp;
 	struct ibmvnic_error_indication error_indication;
-	struct ibmvnic_request_error_info request_error_info;
-	struct ibmvnic_request_error_rsp request_error_rsp;
 	struct ibmvnic_link_state_indication link_state_indication;
 	struct ibmvnic_change_mac_addr change_mac_addr;
 	struct ibmvnic_change_mac_addr change_mac_addr_rsp;
@@ -809,8 +789,6 @@ enum ibmvnic_commands {
 	SET_PHYS_PARMS = 0x07,
 	SET_PHYS_PARMS_RSP = 0x87,
 	ERROR_INDICATION = 0x08,
-	REQUEST_ERROR_INFO = 0x09,
-	REQUEST_ERROR_RSP = 0x89,
 	LOGICAL_LINK_STATE = 0x0C,
 	LOGICAL_LINK_STATE_RSP = 0x8C,
 	REQUEST_STATISTICS = 0x0D,
@@ -945,14 +923,6 @@ struct ibmvnic_rx_pool {
 	struct ibmvnic_long_term_buff long_term_buff;
 };
 
-struct ibmvnic_error_buff {
-	char *buff;
-	dma_addr_t dma;
-	int len;
-	struct list_head list;
-	__be32 error_id;
-};
-
 struct ibmvnic_vpd {
 	unsigned char *buff;
 	dma_addr_t dma_addr;
@@ -1047,9 +1017,6 @@ struct ibmvnic_adapter {
 	struct completion init_done;
 	int init_done_rc;
 
-	struct list_head errors;
-	spinlock_t error_list_lock;
-
 	struct completion fw_done;
 	int fw_done_rc;
 
-- 
2.12.3

[PATCH net-next 2/2] ibmvnic: Update firmware error reporting with cause string

[Thomas Falcon]

Print a string instead of the error code. Since there is a
possibility that the driver can recover, classify it as a
warning instead of an error.

Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 109e4a58efad..dafdd4ade705 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -3825,15 +3825,41 @@ static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
 	ibmvnic_send_crq(adapter, &crq);
 }
 
+static const char *ibmvnic_fw_err_cause(u16 cause)
+{
+	switch (cause) {
+	case ADAPTER_PROBLEM:
+		return "adapter problem";
+	case BUS_PROBLEM:
+		return "bus problem";
+	case FW_PROBLEM:
+		return "firmware problem";
+	case DD_PROBLEM:
+		return "device driver problem";
+	case EEH_RECOVERY:
+		return "EEH recovery";
+	case FW_UPDATED:
+		return "firmware updated";
+	case LOW_MEMORY:
+		return "low Memory";
+	default:
+		return "unknown";
+	}
+}
+
 static void handle_error_indication(union ibmvnic_crq *crq,
 				    struct ibmvnic_adapter *adapter)
 {
 	struct device *dev = &adapter->vdev->dev;
+	u16 cause;
+
+	cause = be16_to_cpu(crq->error_indication.error_cause);
 
-	dev_err(dev, "Firmware reports %serror, cause %d\n",
-		crq->error_indication.flags
-			& IBMVNIC_FATAL_ERROR ? "FATAL " : "",
-		be16_to_cpu(crq->error_indication.error_cause));
+	dev_warn_ratelimited(dev,
+			     "Firmware reports %serror, cause: %s. Starting recovery...\n",
+			     crq->error_indication.flags
+				& IBMVNIC_FATAL_ERROR ? "FATAL " : "",
+			     ibmvnic_fw_err_cause(cause));
 
 	if (crq->error_indication.flags & IBMVNIC_FATAL_ERROR)
 		ibmvnic_reset(adapter, VNIC_RESET_FATAL);
-- 
2.12.3

Re: [PATCH net-next 2/2] ibmvnic: Update firmware error reporting with cause string

[Nathan Fontenot]

On 08/06/2018 09:39 PM, Thomas Falcon wrote:
> Print a string instead of the error code. Since there is a
> possibility that the driver can recover, classify it as a
> warning instead of an error.
> 
> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
> ---
>   drivers/net/ethernet/ibm/ibmvnic.c | 34 ++++++++++++++++++++++++++++++----
>   1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 109e4a58efad..dafdd4ade705 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -3825,15 +3825,41 @@ static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
>   	ibmvnic_send_crq(adapter, &crq);
>   }
> 
> +static const char *ibmvnic_fw_err_cause(u16 cause)
> +{
> +	switch (cause) {
> +	case ADAPTER_PROBLEM:
> +		return "adapter problem";
> +	case BUS_PROBLEM:
> +		return "bus problem";
> +	case FW_PROBLEM:
> +		return "firmware problem";
> +	case DD_PROBLEM:
> +		return "device driver problem";
> +	case EEH_RECOVERY:
> +		return "EEH recovery";
> +	case FW_UPDATED:
> +		return "firmware updated";
> +	case LOW_MEMORY:
> +		return "low Memory";
> +	default:
> +		return "unknown";
> +	}
> +}
> +
>   static void handle_error_indication(union ibmvnic_crq *crq,
>   				    struct ibmvnic_adapter *adapter)
>   {
>   	struct device *dev = &adapter->vdev->dev;
> +	u16 cause;
> +
> +	cause = be16_to_cpu(crq->error_indication.error_cause);
> 
> -	dev_err(dev, "Firmware reports %serror, cause %d\n",
> -		crq->error_indication.flags
> -			& IBMVNIC_FATAL_ERROR ? "FATAL " : "",
> -		be16_to_cpu(crq->error_indication.error_cause));
> +	dev_warn_ratelimited(dev,
> +			     "Firmware reports %serror, cause: %s. Starting recovery...\n",

                                                ^^^^^^^

You're going to want a space between after the %s here.

-Nathan

> +			     crq->error_indication.flags
> +				& IBMVNIC_FATAL_ERROR ? "FATAL " : "",
> +			     ibmvnic_fw_err_cause(cause));
> 
>   	if (crq->error_indication.flags & IBMVNIC_FATAL_ERROR)
>   		ibmvnic_reset(adapter, VNIC_RESET_FATAL);
> 

Re: [PATCH net-next 2/2] ibmvnic: Update firmware error reporting with cause string

[Thomas Falcon]

On 08/06/2018 10:48 PM, Nathan Fontenot wrote:
>
> On 08/06/2018 09:39 PM, Thomas Falcon wrote:
>> Print a string instead of the error code. Since there is a
>> possibility that the driver can recover, classify it as a
>> warning instead of an error.
>>
>> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
>> ---
>>   drivers/net/ethernet/ibm/ibmvnic.c | 34 
>> ++++++++++++++++++++++++++++++----
>>   1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c 
>> b/drivers/net/ethernet/ibm/ibmvnic.c
>> index 109e4a58efad..dafdd4ade705 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -3825,15 +3825,41 @@ static void 
>> handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
>>       ibmvnic_send_crq(adapter, &crq);
>>   }
>>
>> +static const char *ibmvnic_fw_err_cause(u16 cause)
>> +{
>> +    switch (cause) {
>> +    case ADAPTER_PROBLEM:
>> +        return "adapter problem";
>> +    case BUS_PROBLEM:
>> +        return "bus problem";
>> +    case FW_PROBLEM:
>> +        return "firmware problem";
>> +    case DD_PROBLEM:
>> +        return "device driver problem";
>> +    case EEH_RECOVERY:
>> +        return "EEH recovery";
>> +    case FW_UPDATED:
>> +        return "firmware updated";
>> +    case LOW_MEMORY:
>> +        return "low Memory";
>> +    default:
>> +        return "unknown";
>> +    }
>> +}
>> +
>>   static void handle_error_indication(union ibmvnic_crq *crq,
>>                       struct ibmvnic_adapter *adapter)
>>   {
>>       struct device *dev = &adapter->vdev->dev;
>> +    u16 cause;
>> +
>> +    cause = be16_to_cpu(crq->error_indication.error_cause);
>>
>> -    dev_err(dev, "Firmware reports %serror, cause %d\n",
>> -        crq->error_indication.flags
>> -            & IBMVNIC_FATAL_ERROR ? "FATAL " : "",
>> -        be16_to_cpu(crq->error_indication.error_cause));
>> +    dev_warn_ratelimited(dev,
>> +                 "Firmware reports %serror, cause: %s. Starting 
>> recovery...\n",
>
>                                                ^^^^^^^
>
> You're going to want a space between after the %s here.
>
> -Nathan
>

It does look odd at first glance, but there is a space after "FATAL" below.

Thanks,
Tom

>> + crq->error_indication.flags
>> +                & IBMVNIC_FATAL_ERROR ? "FATAL " : "",
>> +                 ibmvnic_fw_err_cause(cause));
>>
>>       if (crq->error_indication.flags & IBMVNIC_FATAL_ERROR)
>>           ibmvnic_reset(adapter, VNIC_RESET_FATAL);
>>

Re: [PATCH net-next 0/2] ibmvnic: Update firmware error reporting

[David Miller]

From: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
Date: Mon,  6 Aug 2018 21:39:57 -0500

> This patch set cleans out a lot of dead code from the ibmvnic driver
> and adds some more. The error ID field of the descriptor is not filled
> in by firmware, so do not print it and do not use it to query for
> more detailed information. Remove the unused code written for this.
> Finally, update the message to print a string explainng the error
> cause instead of just the error code.

Series applied, thanks Thomas.