Re: [PATCH net-next3/5] ibmvnic: Free and re-allocate scrqs when tx/rx scrqs change

[kbuild test robot]

Hi Nathan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.16-rc2 next-20180216]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Nathan-Fontenot/ibmvnic-Free-and-re-allocate-scrqs-when-tx-rx-scrqs-change/20180218-203503
config: powerpc-allmodconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   drivers/net//ethernet/ibm/ibmvnic.c: In function 'release_sub_crqs':
>> drivers/net//ethernet/ibm/ibmvnic.c:2340:28: error: 'struct ibmvnic_adapter' 
>> has no member named 'num_active_tx_scrqs'; did you mean 
>> 'num_active_tx_pools'?
  for (i = 0; i < adapter->num_active_tx_scrqs; i++) {
   ^~~
   num_active_tx_pools
>> drivers/net//ethernet/ibm/ibmvnic.c:2362:28: error: 'struct ibmvnic_adapter' 
>> has no member named 'num_active_rx_scrqs'; did you mean 
>> 'num_active_rx_pools'?
  for (i = 0; i < adapter->num_active_rx_scrqs; i++) {
   ^~~
   num_active_rx_pools

vim +2340 drivers/net//ethernet/ibm/ibmvnic.c

  2334  
  2335  static void release_sub_crqs(struct ibmvnic_adapter *adapter, int 
do_h_free)
  2336  {
  2337  int i;
  2338  
  2339  if (adapter->tx_scrq) {
> 2340  for (i = 0; i < adapter->num_active_tx_scrqs; i++) {
  2341  if (!adapter->tx_scrq[i])
  2342  continue;
  2343  
  2344  netdev_dbg(adapter->netdev, "Releasing 
tx_scrq[%d]\n",
  2345 i);
  2346  if (adapter->tx_scrq[i]->irq) {
  2347  free_irq(adapter->tx_scrq[i]->irq,
  2348   adapter->tx_scrq[i]);
  2349  
irq_dispose_mapping(adapter->tx_scrq[i]->irq);
  2350  adapter->tx_scrq[i]->irq = 0;
  2351  }
  2352  
  2353  release_sub_crq_queue(adapter, 
adapter->tx_scrq[i],
  2354do_h_free);
  2355  }
  2356  
  2357  kfree(adapter->tx_scrq);
  2358  adapter->tx_scrq = NULL;
  2359  }
  2360  
  2361  if (adapter->rx_scrq) {
> 2362  for (i = 0; i < adapter->num_active_rx_scrqs; i++) {
  2363  if (!adapter->rx_scrq[i])
  2364  continue;
  2365  
  2366  netdev_dbg(adapter->netdev, "Releasing 
rx_scrq[%d]\n",
  2367 i);
  2368  if (adapter->rx_scrq[i]->irq) {
  2369  free_irq(adapter->rx_scrq[i]->irq,
  2370   adapter->rx_scrq[i]);
  2371  
irq_dispose_mapping(adapter->rx_scrq[i]->irq);
  2372  adapter->rx_scrq[i]->irq = 0;
  2373  }
  2374  
  2375  release_sub_crq_queue(adapter, 
adapter->rx_scrq[i],
  2376do_h_free);
  2377  }
  2378  
  2379  kfree(adapter->rx_scrq);
  2380  adapter->rx_scrq = NULL;
  2381  }
  2382  }
  2383  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH net-next3/5] ibmvnic: Free and re-allocate scrqs when tx/rx scrqs change

[Nathan Fontenot]

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 
> ---
>  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(>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 = >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]);
> + 

[PATCH net-next3/5] ibmvnic: Free and re-allocate scrqs when tx/rx scrqs change

[Nathan Fontenot]

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.

Signed-off-by: Nathan Fontenot 
---
 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(>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 = >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 <