Re: [PATCH v6 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

2 messages, 2 authors, 2021-04-15 · open the first message on its own page

Re: [PATCH v6 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-04-15 17:44:28

On Wed, 14 Apr 2021 22:45:19 -0700 Dexuan Cui wrote:
+	buf = dma_alloc_coherent(gmi->dev, length, &dma_handle,
+				 GFP_KERNEL | __GFP_ZERO);
No need for GFP_ZERO, dma_alloc_coherent() zeroes the memory these days.
+static int mana_gd_register_irq(struct gdma_queue *queue,
+				const struct gdma_queue_spec *spec)
...
+	struct gdma_irq_context *gic;
+
+	struct gdma_context *gc;
Why the empty line?
+	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
+	if (!queue)
+		return -ENOMEM;
+
+	gmi = &queue->mem_info;
+	err = mana_gd_alloc_memory(gc, spec->queue_size, gmi);
+	if (err)
+		return err;
Leaks the memory from 'queue'?

Same code in mana_gd_create_mana_eq(), ...wq_cq(), etc.
+int mana_do_attach(struct net_device *ndev, enum mana_attach_caller caller)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+	struct gdma_dev *gd = apc->ac->gdma_dev;
+	u32 max_txq, max_rxq, max_queues;
+	int port_idx = apc->port_idx;
+	u32 num_indirect_entries;
+	int err;
+
+	if (caller == MANA_OPEN)
+		goto start_open;
+
+	err = mana_init_port_context(apc);
+	if (err)
+		return err;
+
+	err = mana_query_vport_cfg(apc, port_idx, &max_txq, &max_rxq,
+				   &num_indirect_entries);
+	if (err) {
+		netdev_err(ndev, "Failed to query info for vPort 0\n");
+		goto reset_apc;
+	}
+
+	max_queues = min_t(u32, max_txq, max_rxq);
+	if (apc->max_queues > max_queues)
+		apc->max_queues = max_queues;
+
+	if (apc->num_queues > apc->max_queues)
+		apc->num_queues = apc->max_queues;
+
+	memcpy(ndev->dev_addr, apc->mac_addr, ETH_ALEN);
+
+	if (caller == MANA_PROBE)
+		return 0;
+
+start_open:
Why keep this as a single function, there is no overlap between what's
done for OPEN and PROBE, it seems.

Similarly detach should probably be split into clearly distinct parts.
+	err = mana_create_eq(apc);
+	if (err)
+		goto reset_apc;
+
+	err = mana_create_vport(apc, ndev);
+	if (err)
+		goto destroy_eq;
+
+	err = netif_set_real_num_tx_queues(ndev, apc->num_queues);
+	if (err)
+		goto destroy_vport;
+
+	err = mana_add_rx_queues(apc, ndev);
+	if (err)
+		goto destroy_vport;
+
+	apc->rss_state = apc->num_queues > 1 ? TRI_STATE_TRUE : TRI_STATE_FALSE;
+
+	err = netif_set_real_num_rx_queues(ndev, apc->num_queues);
+	if (err)
+		goto destroy_vport;
+
+	mana_rss_table_init(apc);
+
+	err = mana_config_rss(apc, TRI_STATE_TRUE, true, true);
+	if (err)
+		goto destroy_vport;
+
+	return 0;
+
+destroy_vport:
+	mana_destroy_vport(apc);
+destroy_eq:
+	mana_destroy_eq(gd->gdma_context, apc);
+reset_apc:
+	if (caller == MANA_OPEN)
+		return err;
+	kfree(apc->rxqs);
+	apc->rxqs = NULL;
+	return err;
+}
+
+int mana_attach(struct net_device *ndev)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+	int err;
+
+	ASSERT_RTNL();
+
+	err = mana_do_attach(ndev, MANA_ATTACH);
+	if (err)
+		return err;
+
+	netif_device_attach(ndev);
+
+	apc->port_is_up = apc->port_st_save;
+
+	/* Ensure port state updated before txq state */
+	smp_wmb();
+
+	if (apc->port_is_up) {
+		netif_carrier_on(ndev);
+		netif_tx_wake_all_queues(ndev);
+	}
+
+	return 0;
+}

RE: [PATCH v6 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

From: Dexuan Cui <decui@microsoft.com>
Date: 2021-04-15 18:14:23

From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, April 15, 2021 10:44 AM
 ...
On Wed, 14 Apr 2021 22:45:19 -0700 Dexuan Cui wrote:
quoted
+	buf = dma_alloc_coherent(gmi->dev, length, &dma_handle,
+				 GFP_KERNEL | __GFP_ZERO);
No need for GFP_ZERO, dma_alloc_coherent() zeroes the memory these days.
Yes, indeed. Will remove __GFP_ZERO.
quoted
+static int mana_gd_register_irq(struct gdma_queue *queue,
+				const struct gdma_queue_spec *spec)
...
+	struct gdma_irq_context *gic;
+
+	struct gdma_context *gc;
Why the empty line?
No good reason. Will remove this line. I'll check the whole patch
for similar issues.
quoted
+	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
+	if (!queue)
+		return -ENOMEM;
+
+	gmi = &queue->mem_info;
+	err = mana_gd_alloc_memory(gc, spec->queue_size, gmi);
+	if (err)
+		return err;
Leaks the memory from 'queue'?
Sorry. This should be a bug I introduced when moving arouond some code.
Same code in mana_gd_create_mana_eq(), ...wq_cq(), etc.
Will fix all of them, and check for the code similar issues.
quoted
+int mana_do_attach(struct net_device *ndev, enum mana_attach_caller
caller)
quoted
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+	struct gdma_dev *gd = apc->ac->gdma_dev;
+	u32 max_txq, max_rxq, max_queues;
+	int port_idx = apc->port_idx;
+	u32 num_indirect_entries;
+	int err;
+
+	if (caller == MANA_OPEN)
+		goto start_open;
+
+	err = mana_init_port_context(apc);
+	if (err)
+		return err;
+
+	err = mana_query_vport_cfg(apc, port_idx, &max_txq, &max_rxq,
+				   &num_indirect_entries);
+	if (err) {
+		netdev_err(ndev, "Failed to query info for vPort 0\n");
+		goto reset_apc;
+	}
+
+	max_queues = min_t(u32, max_txq, max_rxq);
+	if (apc->max_queues > max_queues)
+		apc->max_queues = max_queues;
+
+	if (apc->num_queues > apc->max_queues)
+		apc->num_queues = apc->max_queues;
+
+	memcpy(ndev->dev_addr, apc->mac_addr, ETH_ALEN);
+
+	if (caller == MANA_PROBE)
+		return 0;
+
+start_open:
Why keep this as a single function, there is no overlap between what's
done for OPEN and PROBE, it seems.

Similarly detach should probably be split into clearly distinct parts.
Will improve the code. Thanks for the suggestion!

Thanks,
Dexuan
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help