Re: [PATCH net-next v4 09/14] ibmveth: Enable multi-queue RX receive path

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ibmveth: Enable multi-queue RX receive path

This patch turns on the multi-queue RX path: probe latches multi_queue and
num_rx_queues from the H_ILLAN_ATTRIBUTES MQ bit, subordinate queues are
registered with H_REG_LOGICAL_LAN_QUEUE, and open replenishes every queue
before setup_rx_interrupts() unmasks PHYP. A few questions below on the
new ordering, the CMO entitlement accounting and the probe unwind.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index cb93659fc0576..4ad7ced3c6083 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

> @@ -97,7 +97,17 @@ static struct ibmveth_stat ibmveth_stats[] = {
>  	{ "fw_enabled_ipv6_csum", IBMVETH_STAT_OFF(fw_ipv6_csum_support) },
>  	{ "tx_large_packets", IBMVETH_STAT_OFF(tx_large_packets) },
>  	{ "rx_large_packets", IBMVETH_STAT_OFF(rx_large_packets) },
> -	{ "fw_enabled_large_send", IBMVETH_STAT_OFF(fw_large_send_support) }
> +	{ "fw_enabled_large_send", IBMVETH_STAT_OFF(fw_large_send_support) },
> +	{ "hcall_reg_lan_queue", IBMVETH_STAT_OFF(hcall_stats.reg_lan_queue) },
> +	{ "hcall_reg_lan", IBMVETH_STAT_OFF(hcall_stats.reg_lan) },
> +	{ "hcall_add_bufs_queue",
> +	  IBMVETH_STAT_OFF(hcall_stats.add_bufs_queue) },
> +	{ "hcall_add_bufs", IBMVETH_STAT_OFF(hcall_stats.add_bufs) },
> +	{ "hcall_add_buf", IBMVETH_STAT_OFF(hcall_stats.add_buf) },
> +	{ "hcall_free_lan_queue",
> +	  IBMVETH_STAT_OFF(hcall_stats.free_lan_queue) },
> +	{ "hcall_free_lan", IBMVETH_STAT_OFF(hcall_stats.free_lan) },
> +	{ "hcall_send_lan", IBMVETH_STAT_OFF(hcall_stats.send_lan) },
>  };

[Severity: Low]
Should hcall_send_lan be exported through ethtool -S?  It is incremented
once per successfully transmitted packet on the ibmveth_send() success
path:

drivers/net/ethernet/ibm/ibmveth.c:ibmveth_send() {
	...
	adapter->hcall_stats.send_lan++;
	return 0;
}

That makes it the same value as the standard rtnl_link_stats64 tx_packets
counter reported by ndo_get_stats64 / ip -s link, and
Documentation/networking/statistics.rst asks that counters with a standard
uAPI not be duplicated as driver-private ethtool -S strings.  These
strings become uAPI once merged.

The commit message subject is "Enable multi-queue RX receive path" and
does not mention that eight new statistics strings are being added.  Could
the stats additions be split out or at least described?

> @@ -429,12 +439,64 @@ ibmveth_enable_irq(struct ibmveth_adapter *adapter, int queue_index)
>  	return ibmveth_toggle_irq(adapter, queue_index, true);
>  }
>  
> +/**
> + * ibmveth_dispose_subordinate_irq_mapping - Drop one subordinate virq mapping
> + * @adapter: ibmveth adapter structure
> + * @queue_idx: RX queue index (1..N)
> + *
> + * Subordinate queues get mappings from irq_create_mapping() during PHYP
> + * registration. Queue 0 uses netdev->irq from device tree and is left alone.
> + *
> + * Bound against IBMVETH_MAX_RX_QUEUES, not num_rx_queues: scale-down and
> + * scale-up fail paths dispose queues that are no longer in the published
> + * live set but still own a virq in queue_irq[]. The bulk helper still
> + * iterates only 1..num_rx_queues-1 for close/open-fail cleanup.

[Severity: Low]
This isn't a bug, but do the "scale-down and scale-up fail paths" exist at
this commit?  ibmveth_resize_rx_queues_incremental() and
ibmveth_setup_single_rx_interrupt() arrive in a later patch, and the only
caller here is the bulk helper bounded by num_rx_queues.

The same applies to the comment added above ibmveth_get_strings() in this
patch:

	/*
	 * Sum per-queue counters for rare ethtool reads. Do not write adapter
	 * globals on the hot path (ibmvnic-style); with qstats allocated for the
	 * adapter lifetime, these sums remain meaningful across ifdown/up.
	 */

ibmveth_get_strings() only memcpy()s string tables here, and no
rx_qstats/tx_qstats members exist in struct ibmveth_adapter until the
following patch.  Could both comments move to the patches that add the
code they describe, so the intermediate commits read correctly?

> +	for (i = 0; i < num; i++)
> +		napi_enable(&adapter->napi[i]);
> +
> +	if (adapter->multi_queue && num > 1) {
> +		for (i = 0; i < num; i++) {
> +			rc = ibmveth_enable_irq(adapter, i);
> +			if (rc) {
> +				netdev_err(netdev,
> +					   "Failed to enable IRQ for queue %d, rc=%d\n",
> +					   i, rc);
> +				while (--i >= 0)
> +					ibmveth_disable_irq(adapter, i);
> +				rc = -EIO;
> +				goto err_disable_napi;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  
> +err_disable_napi:
> +	/* PHYP unmask was rolled back above; disable NAPI before free_irq */
> +	for (i = 0; i < num; i++)
> +		napi_disable(&adapter->napi[i]);
> +	for (i = 0; i < num; i++) {
> +		if (adapter->queue_irq[i])
> +			free_irq(adapter->queue_irq[i], &adapter->napi[i]);
> +	}
> +	goto err_dispose_mappings;

[Severity: Low]
Can the rollback loop above be undone by an in-flight poll?  Queues 0..i-1
were already unmasked, so a poll may be running when
ibmveth_enable_irq() fails for queue i:

ibmveth_poll() {
	...
	if (!napi_complete_done(napi, frames_processed))
		goto out;
	...
	lpar_rc = ibmveth_enable_irq(adapter, queue_index);
}

There is no synchronize_irq() between the while (--i >= 0)
ibmveth_disable_irq() loop and napi_disable(), so a poll that completes
after the mask re-enables PHYP delivery.  napi_disable() waits for the
poll but does not undo that re-enable, and the following free_irq() plus
ibmveth_dispose_subordinate_irq_mappings() then leave the hypervisor
source enabled for a hwirq whose Linux mapping is gone.

The scale-down path added later does disable_irq() followed by
synchronize_irq() for exactly this reason.  Should this path do the same?

[ ... ]

> @@ -834,9 +914,15 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
>   */
>  static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter)
>  {
> -	__be64 *p = adapter->buffer_list_addr[0] + 4096 - 8;
> +	int i;
>  
> -	adapter->rx_no_buffer = be64_to_cpup(p);
> +	adapter->rx_no_buffer = 0;
> +	for (i = 0; i < adapter->num_rx_queues; i++) {
> +		__be64 *p = adapter->buffer_list_addr[i] + 4096 - 8;
> +		u64 drops = be64_to_cpup(p);
> +
> +		adapter->rx_no_buffer += drops;
> +	}
>  }

[Severity: High]
Is this cross-queue aggregation safe against the RX queue resize path?
The only lock held here is the caller's own per-queue lock:

ibmveth_replenish_task(adapter, queue_index) {
	spin_lock_irqsave(&rxq->replenish_lock, flags);
	...
	ibmveth_update_rx_no_buffer(adapter);
	spin_unlock_irqrestore(&rxq->replenish_lock, flags);
}

That lock does not cover other queues' buffer_list_addr[] entries, nor
num_rx_queues.  The ethtool -L scale-down path added later in the series
publishes the new count and then frees the pages:

	adapter->num_rx_queues = new_count;

	for (i = new_count; i < old_count; i++) {
		...
		ibmveth_free_single_rx_queue(adapter, i);   /* free_page() + NULL */
	}

If a surviving queue's poll already loaded the older, larger
num_rx_queues, can this loop dereference NULL + 4096 - 8, or read a page
that was just freed, from softirq context?  A READ_ONCE() of the count
plus a NULL check, or serializing the aggregation against the resize
path, would make this explicit.

Separately, two concurrent polls on different queues both execute
adapter->rx_no_buffer = 0 followed by += drops on the same field.  Can
that lose increments and expose a transient 0 through
rx_no_buffer / ethtool -S?

> @@ -847,8 +933,12 @@ static void ibmveth_replenish_task(struct ibmveth_adapter *adapter,
>  	unsigned long flags;
>  	int i;
>  
> -	if (queue_index >= adapter->num_rx_queues)
> +	if (queue_index >= adapter->num_rx_queues) {
> +		netdev_dbg(adapter->netdev,
> +			   "Skipping replenish for freed queue %d (num_queues=%d)\n",
> +			   queue_index, adapter->num_rx_queues);
>  		return;
> +	}
>  
>  	adapter->replenish_task_cycles++;

[Severity: Low]
With several RX queues now polling concurrently, do the adapter-wide plain
u64 counters need serialization?  replenish_task_cycles is incremented
before the per-queue lock is taken, and replenish_no_mem,
replenish_add_buff_success/failure and the new hcall_stats fields are
touched under different per-queue locks, so the lockset intersection for
these adapter-global fields is empty.  hcall_stats.send_lan is also
incremented from ibmveth_start_xmit() on up to 16 TX queues in parallel.
The visible effect is lost increments in the ethtool -S values.

[ ... ]

> @@ -1284,6 +1374,137 @@ static int ibmveth_register_logical_lan(struct ibmveth_adapter *adapter,
> +	lpar_rc = h_reg_logical_lan_queue(adapter->vdev->unit_address,
> +					  adapter->buffer_list_dma[queue_index],
> +					  rxq_desc.desc, &handle, &hwirq);
> +	adapter->hcall_stats.reg_lan_queue++;

[Severity: Low]
Should this hcall retry on a busy return?  Every other long-running hcall
in the driver loops:

	do {
		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));

including h_free_logical_lan_queue() in the IRQ-mapping failure path a few
lines below this call, and h_send_logical_lan() in ibmveth_send().  As
written, a transient H_BUSY / H_LONG_BUSY_ORDER_* here becomes -EIO from
ibmveth_register_single_rx_queue() and a failed ifup.

> +	/*
> +	 * H_FUNCTION means firmware rejected this subordinate register
> +	 * (MQ unsupported). That is a hard open failure: do not clear
> +	 * multi_queue or claim single-queue fallback. Keep a specific
> +	 * log, then the generic failure lines below (no early return).
> +	 */
> +	if (lpar_rc == H_FUNCTION)
> +		netdev_err(adapter->netdev,
> +			   "h_reg_logical_lan_queue H_FUNCTION for queue %d (firmware MQ unsupported)\n",
> +			   queue_index);

[Severity: High]
Can the adapter recover if firmware stops honouring the MQ register after
probe?  multi_queue and num_rx_queues are latched once in ibmveth_probe()
from the H_ILLAN_ATTRIBUTES bit and are never re-read or reduced, so a
H_FUNCTION here fails ibmveth_open() and every subsequent open the same
way.

The reset work item only closes and reopens, it never re-reads the
attributes:

ibmveth_reset() {
	rtnl_lock();
	dev_close(adapter->netdev);
	dev_open(adapter->netdev, NULL);
	rtnl_unlock();
}

ethtool -L ethX rx 1 while down does not restore RX either: multi_queue
stays 1, so rx_buffers_per_hcall remains IBMVETH_MAX_RX_QUEUE and
ibmveth_add_logical_lan_buffers() keeps calling
h_add_logical_lan_buffers_queue(), whose H_FUNCTION path has no fallback
when multi_queue is set.

The driver already assumes PHYP capabilities can disappear across LPM:

	/*
	 * Live Partition Migration may drop multi-
	 * buffer support. Fall back to single-buffer
	 * on the next replenish; ...
	 */

Would clearing multi_queue and falling back to one queue on H_FUNCTION be
preferable to leaving the interface permanently unable to open?

> +static int
> +ibmveth_register_single_rx_queue(struct ibmveth_adapter *adapter,
> +				 int queue_idx, u64 mac_address)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +	union ibmveth_buf_desc rxq_desc;
> +	long lpar_rc;
> +
> +	(void)mac_address;

[Severity: Low]
This isn't a bug, but is the mac_address parameter worth keeping?
H_REG_LOGICAL_LAN_QUEUE has no MAC operand and
ibmveth_register_logical_lan_queue() has no MAC parameter, so callers
compute ether_addr_to_u64(netdev->dev_addr) only to feed a parameter that
is voided here.  The (void)x; suppression cast is also not needed under
the kernel's warning set.

[ ... ]

> @@ -1356,9 +1579,31 @@ ibmveth_register_rx_queues(struct ibmveth_adapter *adapter, u64 mac_address)
> +	for (i = 1; i < adapter->num_rx_queues; i++) {
> +		rc = ibmveth_register_single_rx_queue(adapter, i, mac_address);
> +		if (rc)
> +			goto err_unregister;
> +	}
> +
>  	netdev_dbg(netdev,
> -		   "registered 1 RX queue with hypervisor (single-queue mode)\n");
> +		   "registered %d RX queues with hypervisor (multi-queue mode)\n",
> +		   adapter->num_rx_queues);
> +
>  	return 0;
> +
> +err_unregister:
> +	ibmveth_dispose_subordinate_irq_mappings(adapter);
> +	ibmveth_free_all_queues(adapter);
> +	return rc;
>  }

> @@ -1396,12 +1641,29 @@ static int ibmveth_open(struct net_device *netdev)
> +	/*
> +	 * MQ: post buffers before setup_rx_interrupts() unmasks PHYP
> +	 * (avoids drops if traffic arrives during open; PHYP allows
> +	 * either order). Single-queue keeps the classic kick: setup
> +	 * (no unmask) then schedule_rx_queue() so the first poll
> +	 * replenishes and enables.
> +	 */
> +	if (adapter->multi_queue && adapter->num_rx_queues > 1) {
> +		for (i = 0; i < adapter->num_rx_queues; i++) {
> +			netdev_dbg(netdev,
> +				   "initial replenish cycle for queue %d\n", i);
> +			ibmveth_replenish_task(adapter, i);
> +		}
> +	}
> +
>  	rc = ibmveth_setup_rx_interrupts(adapter);

[Severity: Medium]
Is PHYP delivery for subordinate queues guaranteed to be masked in this
window?  Queue 0 is explicitly masked before registration:

ibmveth_register_rx_queues() {
	adapter->queue_irq[0] = netdev->irq;
	rc = ibmveth_disable_irq(adapter, 0);
	...
}

but queues 1..N get no equivalent H_VIOCTL/H_DISABLE_VIO_INTERRUPT after
H_REG_LOGICAL_LAN_QUEUE plus irq_create_mapping().  Buffers are now
posted for every queue here, before any handler exists, and
ibmveth_setup_rx_interrupts() was also reordered so request_irq() for all
queues completes before any napi_enable():

	for (i = 0; i < num; i++) {
		...
		rc = request_irq(adapter->queue_irq[i], ibmveth_interrupt, ...);
	}

	for (i = 0; i < num; i++)
		napi_enable(&adapter->napi[i]);

If an interrupt arrives in that window, ibmveth_schedule_rx_queue() finds
NAPI still disabled:

	if (napi_schedule_prep(napi)) {
		lpar_rc = ibmveth_disable_irq(adapter, qindex);
		WARN_ON(lpar_rc != H_SUCCESS);
		__napi_schedule(napi);
	}

so the handler returns IRQ_HANDLED without masking the hypervisor source
and without scheduling a poll.  That is the same storm window described in
ibmveth_cleanup_rx_interrupts() ("Mask PHYP before napi_disable so
ibmveth_interrupt cannot return IRQ_HANDLED without masking").  The
err_free_irqs path has the same gap: it frees handlers and disposes
subordinate mappings without masking already-registered subordinate
queues.

The later scale-up path uses register -> request_irq -> publish ->
replenish -> napi_enable -> enable_irq and documents why.  Would enabling
NAPI before request_irq (as the baseline did), or explicitly masking each
subordinate queue right after registration, close this window?

[ ... ]

> @@ -2373,18 +2632,25 @@ static unsigned long ibmveth_get_desired_dma(struct vio_dev *vdev)
>  	/* add size of mapped tx buffers */
>  	ret += IOMMU_PAGE_ALIGN(IBMVETH_MAX_TX_BUF_SIZE, tbl);
>  
> -	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
> -		/* add the size of the active receive buffers */
> -		if (adapter->rx_buff_pool[0][i].active)
> -			ret +=
> -			    adapter->rx_buff_pool[0][i].size *
> -			    IOMMU_PAGE_ALIGN(adapter->rx_buff_pool[0][i].
> -					     buff_size, tbl);
> -		rxqentries += adapter->rx_buff_pool[0][i].size;
> -	}
> -	/* add the size of the receive queue entries */
> -	ret += IOMMU_PAGE_ALIGN(
> -		rxqentries * sizeof(struct ibmveth_rx_q_entry), tbl);
> +	for (q = 0; q < adapter->num_rx_queues; q++) {
> +		int rxqentries = 1;

[Severity: Medium]
Does the buffer-list term still need scaling here?  The unchanged line
above this loop counts it once:

	ret = IBMVETH_BUFF_LIST_SIZE + IBMVETH_FILT_LIST_SIZE;

while multi-queue allocates and dma_map_single()s a distinct 4096-byte
buffer-list page per queue, which ibmveth_cleanup_rx_resources() unmaps
per index.  That leaves the desired value short by (num_rx_queues - 1)
buffer-list pages.

Also, is anything updating the CMO entitlement for the new default queue
count?  The VIO core samples it before the driver's probe runs:

arch/powerpc/platforms/pseries/vio.c:vio_cmo_bus_probe() {
	viodev->cmo.desired =
		IOMMU_PAGE_ALIGN(viodrv->get_desired_dma(viodev), tbl);
}

with dev_get_drvdata() still NULL, so the single-queue sized
IBMVETH_IO_ENTITLEMENT_DEFAULT is used for an adapter that will open with
up to 8 RX queues, and neither ibmveth_probe() nor ibmveth_open() calls
vio_cmo_set_dev_desired().  On a CMO partition, can this make
dma_map_single()/dma_alloc_coherent() fail during open on adapters that
worked in single-queue mode?

> +
> +		for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
> +			/* add the size of the active receive buffers */
> +			struct ibmveth_buff_pool *bpool =
> +				&adapter->rx_buff_pool[q][i];

[Severity: Medium]
Is rx_buff_pool[q][i] populated for q >= 1 when this runs?  Pool metadata
for queues 1..N is only filled in by ibmveth_alloc_buffer_pools(), which
runs from ndo_open; ibmveth_probe() initializes only rx_buff_pool[0][i],
and the private area is zeroed, so before the first open these entries
have size = active = buff_size = 0.

ibmveth_change_mtu() pushes the result straight into firmware accounting:

	vio_cmo_set_dev_desired(viodev, ibmveth_get_desired_dma(viodev));

Can an MTU change while the interface is down therefore lower the
entitlement to roughly the single-queue value, right before an open that
allocates buffers for up to 8 queues?

> @@ -2449,9 +2715,18 @@ static const struct net_device_ops ibmveth_netdev_ops = {
>  #endif
>  };
>  
> +static void ibmveth_put_pool_kobjs(struct ibmveth_adapter *adapter,
> +				   int pools_ready)
> +{
> +	int i;
> +
> +	for (i = 0; i < pools_ready; i++)
> +		kobject_put(&adapter->rx_buff_pool[0][i].kobj);
> +}
> +

[ ... ]

> @@ -2531,16 +2810,27 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>  		netdev->features |= NETIF_F_FRAGLIST;
>  	}
>  
> -	/* Initialize queue count - always 1 for now */
> -	adapter->multi_queue = 0;
> -	adapter->num_rx_queues = IBMVETH_DEFAULT_RX_QUEUES;
> +	if (ret == H_SUCCESS &&
> +	    (ret_attr & IBMVETH_ILLAN_RX_MULTI_QUEUE_SUPPORT)) {
> +		adapter->multi_queue = 1;
> +		adapter->num_rx_queues = min(num_online_cpus(),
> +					     IBMVETH_DEFAULT_QUEUES);
> +		netdev_dbg(netdev, "RX multi queue mode enabled: %d queues\n",
> +			   adapter->num_rx_queues);
> +	} else {
> +		adapter->multi_queue = 0;
> +		adapter->num_rx_queues = IBMVETH_DEFAULT_RX_QUEUES;
> +	}

[Severity: Medium]
Now that num_rx_queues can be greater than one here, does
ibmveth_resume() need updating too?  It still kicks queue 0 only:

ibmveth_resume() {
	...
	ibmveth_schedule_rx_queue(adapter, 0);
	return 0;
}

The kick exists because the poll routine is what replenishes buffers and
re-arms the PHYP interrupt via napi_complete_done() ->
ibmveth_enable_irq().  After a suspend/resume or partition migration,
queues 1..N-1 would then have no buffers posted and no interrupt
re-armed until an ifdown/ifup.

> @@ -2558,15 +2848,24 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>  
>  	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>  		struct kobject *kobj = &adapter->rx_buff_pool[0][i].kobj;
> -		int error;
>  
>  		ibmveth_init_buffer_pool(&adapter->rx_buff_pool[0][i], i,
>  					 pool_count[i], pool_size[i],
>  					 pool_active[i]);
> -		error = kobject_init_and_add(kobj, &ktype_veth_pool,
> -					     &dev->dev.kobj, "pool%d", i);
> -		if (!error)
> -			kobject_uevent(kobj, KOBJ_ADD);
> +		rc = kobject_init_and_add(kobj, &ktype_veth_pool,
> +					  &dev->dev.kobj, "pool%d", i);
> +		if (rc) {
> +			dev_err(&dev->dev,
> +				"failed to create pool%d kobject: %d\n", i, rc);
> +			/* init_and_add takes a ref even on failure */
> +			kobject_put(kobj);
> +			ibmveth_put_pool_kobjs(adapter, pools_ready);
> +			free_netdev(netdev);
> +			return rc;
> +		}
> +
> +		pools_ready++;
> +		kobject_uevent(kobj, KOBJ_ADD);
>  	}

[Severity: Medium]
Is it safe to free_netdev() immediately after ibmveth_put_pool_kobjs()?
The kobjects are embedded in netdev_priv(netdev), and ktype_veth_pool has
no release callback:

static struct kobj_type ktype_veth_pool = {
	.release        = NULL,
	...
};

With CONFIG_DEBUG_KOBJECT_RELEASE=y the final put defers cleanup using a
work item stored inside the kobject itself:

lib/kobject.c:kobject_release() {
#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	unsigned long delay = HZ + HZ * get_random_u32_below(4);
	...
	INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
	schedule_delayed_work(&kobj->release, delay);
#else
	kobject_cleanup(kobj);
#endif
}

free_netdev() then releases that memory with no window, so
kobject_delayed_cleanup() would read kobj->state_in_sysfs, kobj->parent
and kobj->name from freed storage.  The same shape applies to the two
other new put_pool_kobjs() call sites below.

> @@ -2574,6 +2873,7 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>  	if (rc) {
>  		netdev_dbg(netdev, "failed to set number of tx queues rc=%d\n",
>  			   rc);
> +		ibmveth_put_pool_kobjs(adapter, pools_ready);
>  		free_netdev(netdev);
>  		return rc;
>  	}
> @@ -2590,6 +2890,7 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>  
>  	if (rc) {
>  		netdev_dbg(netdev, "failed to register netdev rc=%d\n", rc);
> +		ibmveth_put_pool_kobjs(adapter, pools_ready);
>  		free_netdev(netdev);
>  		return rc;
>  	}

[ ... ]