Re: [PATCH 2/10] cxgb3 - main source file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Stephen,

Thanks for the review. Please see my replies inline.

Stephen Hemminger wrote:

O
+ * If we have multiple receive queues per port serviced by NAPI we need one
+ * netdevice per queue as NAPI operates on netdevices.  We already have one
+ * netdevice, namely the one associated with the interface, so we use dummy
+ * ones for any additional queues.  Note that these netdevices exist purely
+ * so that NAPI has something to work with, they do not represent network
+ * ports and are not registered.
+ */
+static int init_dummy_netdevs(struct adapter *adap)
+{
+	int i, j, dummy_idx = 0;
+	struct net_device *nd;
+
+	for_each_port(adap, i) {
+		const struct port_info *pi = &adap->port[i];
+
+		for (j = 0; j < pi->nqsets - 1; j++) {
+			if (!adap->dummy_netdev[dummy_idx]) {
+				nd = alloc_netdev(0, "", ether_setup);
+				if (!nd)
+					goto free_all;
+
+				nd->priv = adap;
+				nd->weight = 64;
+				set_bit(__LINK_STATE_START, &nd->state);
+				adap->dummy_netdev[dummy_idx] = nd;
+			}
+			strcpy(adap->dummy_netdev[dummy_idx]->name,
+			       pi->dev->name);
+			dummy_idx++;
+		}
+	}
+	return 0;
+
+free_all:
+	while (--dummy_idx >= 0) {
+		free_netdev(adap->dummy_netdev[dummy_idx]);
+		adap->dummy_netdev[dummy_idx] = NULL;
+	}
+	return -ENOMEM;
+}


I understand this, but it seems more trouble than it is worth
and adds longterm maintance burden to NAPI and receive management code.

You would be better off doing your own scheduling in a tasklet.

This code is directly inspired from the backlog_dev mechanism in net/core/dev.c. NAPI provides fairness between adapters. Using our own scheduling would conflict with this.

+	case SIOCCHIOCTL:
+		return cxgb_extension_ioctl(dev, req->ifr_data);


Adding a chelsio specific ioctl value isn't going to get accepted.
You need to use SIOCDEVPRIVATE, and figure out how to do 32bit/64bit
ioctl compatibility for it.
SIOCCHIOCTL is defined as SIOCDEVPRIVATE in cxgb3_ioctl.h.
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void cxgb_netpoll(struct net_device *dev)
+{
+	unsigned long flags;
+	struct adapter *adapter = dev->priv;
+	struct sge_qset *qs = dev2qset(dev);
+
+	local_irq_save(flags);
+	t3_intr_handler(adapter, qs->rspq.polling) (adapter->pdev->irq,
+						    adapter);
+	local_irq_restore(flags);
+}
+#endif

IRQ's are always disabled when netpoll is called.
Okay, will fix.
+	for (i = 0; i < ai->nports; ++i) {
+		struct net_device *netdev;
+
+		netdev = alloc_etherdev(adapter ? 0 : sizeof(*adapter));
+		if (!netdev) {
+			err = -ENOMEM;
+			goto out_free_dev;
+		}
+
+		if (!adapter) {
+			adapter = netdev->priv;
+
+			adapter->pdev = pdev;
+			adapter->port[0].dev = netdev;	// so we don't leak it
+
+			adapter->regs = ioremap_nocache(mmio_start, mmio_len);
+			if (!adapter->regs) {
+				CH_ERR("%s: cannot map device registers\n",
+				       pci_name(pdev));
+				err = -ENOMEM;
+				goto out_free_dev;
+			}
+
+			adapter->name = pci_name(pdev);
+			adapter->msg_enable = dflt_msg_enable;
+			adapter->mmio_len = mmio_len;
+			INIT_LIST_HEAD(&adapter->adapter_list);
+			mutex_init(&adapter->mdio_lock);
+			spin_lock_init(&adapter->work_lock);
+			spin_lock_init(&adapter->stats_lock);
+
+			INIT_WORK(&adapter->ext_intr_handler_task,
+				  ext_intr_task, adapter);
+			INIT_WORK(&adapter->adap_check_task, t3_adap_check_task,
+				  adapter);
+
+			pci_set_drvdata(pdev, netdev);
+		}


It looks like you are repeating the same method of doing multi-port that you
did on cxgb2. It was wrong then, and it is wrong now:

DON'T
	use if_port to distinguish port. if_port is legacy field see IF_PORT_XXX
	allocate adapter as part of 1st interface
	set dev->priv differently on each port
	don't use array in adapter structure for each port
DO
	allocate adapter separately
	use netdev_priv() for per port data
	use port->adapter for adapter access
We're fixing it.

Cheers,
Divy

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux