Message-Id: <a9b093c1-5774-f251-b9a8-c0ada3ad5e55@linux.vnet.ibm.com>
Date:   Sun, 18 Feb 2018 19:53:38 -0600
From:   Nathan Fontenot <nfont@...ux.vnet.ibm.com>
To:     netdev@...r.kernel.org
Cc:     jallen@...ux.vnet.ibm.com, tlfalcon@...ux.vnet.ibm.com
Subject: Re: [PATCH net-next3/5] ibmvnic: Free and re-allocate scrqs when
 tx/rx scrqs change

On 02/17/2018 05:32 PM, Nathan Fontenot wrote:
> When the driver resets it is possible that the number of tx/rx
> sub-crqs can change. This patch handles this so that the driver does
> not try to access non-existent sub-crqs.
> 
> Additionally, a parameter is added to release_sub_crqs() so that
> we know if the h_call to free the sub-crq needs to be made. In
> the reset path we have to do a reset of the main crq, which is
> a free followed by a register of the main crq. The free of main
> crq results in all of the sub crq's being free'ed. When updating
> sub-crq count in the reset path we do not want to h_free the
> sub-crqs, they are already free'ed.

This patch has a bug in that it does not use the correct queue count
when releasing the sub crqs when the driver is in the probed state.

A version 2 of the patch set will be sent.

-Nathan

> 
> Signed-off-by: Nathan Fontenot <nfont@...ux.vnet.ibm.com>
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c |   69 ++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 9cfbb20b5ac1..a3079d5c072c 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -90,7 +90,7 @@ MODULE_VERSION(IBMVNIC_DRIVER_VERSION);
> 
>  static int ibmvnic_version = IBMVNIC_INITIAL_VERSION;
>  static int ibmvnic_remove(struct vio_dev *);
> -static void release_sub_crqs(struct ibmvnic_adapter *);
> +static void release_sub_crqs(struct ibmvnic_adapter *, int);
>  static int ibmvnic_reset_crq(struct ibmvnic_adapter *);
>  static int ibmvnic_send_crq_init(struct ibmvnic_adapter *);
>  static int ibmvnic_reenable_crq_queue(struct ibmvnic_adapter *);
> @@ -740,7 +740,7 @@ static int ibmvnic_login(struct net_device *netdev)
>  	do {
>  		if (adapter->renegotiate) {
>  			adapter->renegotiate = false;
> -			release_sub_crqs(adapter);
> +			release_sub_crqs(adapter, 1);
> 
>  			reinit_completion(&adapter->init_done);
>  			send_cap_queries(adapter);
> @@ -1602,7 +1602,7 @@ static int do_reset(struct ibmvnic_adapter *adapter,
>  	if (adapter->reset_reason == VNIC_RESET_CHANGE_PARAM ||
>  	    adapter->wait_for_reset) {
>  		release_resources(adapter);
> -		release_sub_crqs(adapter);
> +		release_sub_crqs(adapter, 1);
>  		release_crq_queue(adapter);
>  	}
> 
> @@ -2241,24 +2241,27 @@ static int reset_sub_crq_queues(struct ibmvnic_adapter *adapter)
>  }
> 
>  static void release_sub_crq_queue(struct ibmvnic_adapter *adapter,
> -				  struct ibmvnic_sub_crq_queue *scrq)
> +				  struct ibmvnic_sub_crq_queue *scrq,
> +				  int do_h_free)
>  {
>  	struct device *dev = &adapter->vdev->dev;
>  	long rc;
> 
>  	netdev_dbg(adapter->netdev, "Releasing sub-CRQ\n");
> 
> -	/* Close the sub-crqs */
> -	do {
> -		rc = plpar_hcall_norets(H_FREE_SUB_CRQ,
> -					adapter->vdev->unit_address,
> -					scrq->crq_num);
> -	} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
> +	if (do_h_free) {
> +		/* Close the sub-crqs */
> +		do {
> +			rc = plpar_hcall_norets(H_FREE_SUB_CRQ,
> +						adapter->vdev->unit_address,
> +						scrq->crq_num);
> +		} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
> 
> -	if (rc) {
> -		netdev_err(adapter->netdev,
> -			   "Failed to release sub-CRQ %16lx, rc = %ld\n",
> -			   scrq->crq_num, rc);
> +		if (rc) {
> +			netdev_err(adapter->netdev,
> +				   "Failed to release sub-CRQ %16lx, rc = %ld\n",
> +				   scrq->crq_num, rc);
> +		}
>  	}
> 
>  	dma_unmap_single(dev, scrq->msg_token, 4 * PAGE_SIZE,
> @@ -2326,12 +2329,12 @@ static struct ibmvnic_sub_crq_queue *init_sub_crq_queue(struct ibmvnic_adapter
>  	return NULL;
>  }
> 
> -static void release_sub_crqs(struct ibmvnic_adapter *adapter)
> +static void release_sub_crqs(struct ibmvnic_adapter *adapter, int do_h_free)
>  {
>  	int i;
> 
>  	if (adapter->tx_scrq) {
> -		for (i = 0; i < adapter->req_tx_queues; i++) {
> +		for (i = 0; i < adapter->num_active_tx_scrqs; i++) {
>  			if (!adapter->tx_scrq[i])
>  				continue;
> 
> @@ -2344,7 +2347,8 @@ static void release_sub_crqs(struct ibmvnic_adapter *adapter)
>  				adapter->tx_scrq[i]->irq = 0;
>  			}
> 
> -			release_sub_crq_queue(adapter, adapter->tx_scrq[i]);
> +			release_sub_crq_queue(adapter, adapter->tx_scrq[i],
> +					      do_h_free);
>  		}
> 
>  		kfree(adapter->tx_scrq);
> @@ -2352,7 +2356,7 @@ static void release_sub_crqs(struct ibmvnic_adapter *adapter)
>  	}
> 
>  	if (adapter->rx_scrq) {
> -		for (i = 0; i < adapter->req_rx_queues; i++) {
> +		for (i = 0; i < adapter->num_active_rx_scrqs; i++) {
>  			if (!adapter->rx_scrq[i])
>  				continue;
> 
> @@ -2365,7 +2369,8 @@ static void release_sub_crqs(struct ibmvnic_adapter *adapter)
>  				adapter->rx_scrq[i]->irq = 0;
>  			}
> 
> -			release_sub_crq_queue(adapter, adapter->rx_scrq[i]);
> +			release_sub_crq_queue(adapter, adapter->rx_scrq[i],
> +					      do_h_free);
>  		}
> 
>  		kfree(adapter->rx_scrq);
> @@ -2572,7 +2577,7 @@ static int init_sub_crq_irqs(struct ibmvnic_adapter *adapter)
>  		free_irq(adapter->tx_scrq[j]->irq, adapter->tx_scrq[j]);
>  		irq_dispose_mapping(adapter->rx_scrq[j]->irq);
>  	}
> -	release_sub_crqs(adapter);
> +	release_sub_crqs(adapter, 1);
>  	return rc;
>  }
> 
> @@ -2654,7 +2659,7 @@ static int init_sub_crqs(struct ibmvnic_adapter *adapter)
>  	adapter->tx_scrq = NULL;
>  tx_failed:
>  	for (i = 0; i < registered_queues; i++)
> -		release_sub_crq_queue(adapter, allqueues[i]);
> +		release_sub_crq_queue(adapter, allqueues[i], 1);
>  	kfree(allqueues);
>  	return -1;
>  }
> @@ -4279,6 +4284,7 @@ static int ibmvnic_init(struct ibmvnic_adapter *adapter)
>  {
>  	struct device *dev = &adapter->vdev->dev;
>  	unsigned long timeout = msecs_to_jiffies(30000);
> +	u64 old_num_rx_queues, old_num_tx_queues;
>  	int rc;
> 
>  	if (adapter->resetting && !adapter->wait_for_reset) {
> @@ -4296,6 +4302,9 @@ static int ibmvnic_init(struct ibmvnic_adapter *adapter)
> 
>  	adapter->from_passive_init = false;
> 
> +	old_num_rx_queues = adapter->req_rx_queues;
> +	old_num_tx_queues = adapter->req_tx_queues;
> +
>  	init_completion(&adapter->init_done);
>  	adapter->init_done_rc = 0;
>  	ibmvnic_send_crq_init(adapter);
> @@ -4315,10 +4324,18 @@ static int ibmvnic_init(struct ibmvnic_adapter *adapter)
>  		return -1;
>  	}
> 
> -	if (adapter->resetting && !adapter->wait_for_reset)
> -		rc = reset_sub_crq_queues(adapter);
> -	else
> +	if (adapter->resetting && !adapter->wait_for_reset) {
> +		if (adapter->req_rx_queues != old_num_rx_queues ||
> +		    adapter->req_tx_queues != old_num_tx_queues) {
> +			release_sub_crqs(adapter, 0);
> +			rc = init_sub_crqs(adapter);
> +		} else {
> +			rc = reset_sub_crq_queues(adapter);
> +		}
> +	} else {
>  		rc = init_sub_crqs(adapter);
> +	}
> +
>  	if (rc) {
>  		dev_err(dev, "Initialization of sub crqs failed\n");
>  		release_crq_queue(adapter);
> @@ -4418,7 +4435,7 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
>  	device_remove_file(&dev->dev, &dev_attr_failover);
> 
>  ibmvnic_init_fail:
> -	release_sub_crqs(adapter);
> +	release_sub_crqs(adapter, 1);
>  	release_crq_queue(adapter);
>  	free_netdev(netdev);
> 
> @@ -4435,7 +4452,7 @@ static int ibmvnic_remove(struct vio_dev *dev)
>  	mutex_lock(&adapter->reset_lock);
> 
>  	release_resources(adapter);
> -	release_sub_crqs(adapter);
> +	release_sub_crqs(adapter, 1);
>  	release_crq_queue(adapter);
> 
>  	adapter->state = VNIC_REMOVED;
> 

Powered by blists - more mailing lists