| Message ID | 151891036590.22863.17400577526600853168.stgit@ltcalpine2-lp14.aus.stglabs.ibm.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next3/5] ibmvnic: Free and re-allocate scrqs when tx/rx scrqs change | expand |
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@linux.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; >
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],
2354 do_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],
2376 do_h_free);
2377 }
2378
2379 kfree(adapter->rx_scrq);
2380 adapter->rx_scrq = NULL;
2381 }
2382 }
2383
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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;
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 <nfont@linux.vnet.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 69 ++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 26 deletions(-)