lkml.org 
[lkml]   [2006]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/10] cxgb3 - main source file
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-12-05 09:33    [W:0.069 / U:0.684 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site