Arjan,
Thanks for the review. Please see my replies inline.
Arjan van de Ven wrote:
+/*
+ * Interrupt handler for asynchronous events used with MSI-X.
+ */
+static irqreturn_t t3_async_intr_handler(int irq, void *cookie)
+{
+ t3_slow_intr_handler(cookie);
+ return IRQ_HANDLED;
+}
this looks very wrong; why is t3_slow_intr_handler a void rather than
returning IRQ_HANDLED etc? And why wrap around it ?
t3_slow_intr_handler() processes non-data events such as board errors.
In line interupt and MSI mode, the intr handler deals with both data
and non-data events and calls t3_slow_intr_handler for the latter.
In MSI-X mode, t3_async_intr_handler() is registered to deal with these
non-data interrupts exclusively.
+
+static ssize_t attr_show(struct class_device *cd, char *buf,
+ ssize_t(*format) (struct adapter *, char *))
+{
+ ssize_t len;
+ struct adapter *adap = to_net_dev(cd)->priv;
+
+ /* Synchronize with ioctls that may shut down the device */
+ rtnl_lock();
+ len = (*format) (adap, buf);
+ rtnl_unlock();
+ return len;
+}
I'm usually kind of nervous with drivers taking the rtnl_lock; to me
that sounds like a layering violation.. why shouldn't your attributes
etc live in the net layer instead?
These attributes are really device specific.
The net layer does not support device specific attributes.
+#ifdef ETHTOOL_GPERMADDR
+ .get_perm_addr = ethtool_op_get_perm_addr
+#endif
what is this ifdef for?
it will be removed.
+static int cxgb_extension_ioctl(struct net_device *dev, void __user *useraddr)
+{
+ int ret;
+ u32 cmd;
+ struct adapter *adapter = dev->priv;
+
+ if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
+ return -EFAULT;
+
+ switch (cmd) {
+ case CHELSIO_SETREG:{
what are these for ?
They are used to parameter the HW:
register access, configuration of queue sets, on board memory
configuration,
firmware load, etc ...
+
+ /*
+ * Can't use pci_request_regions() here because some kernels want to
+ * request the MSI-X BAR in pci_enable_msix.
are these "some kernels" actual current mainline kernels?
Will fix both comment and related code.
+ if (!pci_set_dma_mask(pdev, DMA_64BIT_MASK)) {
+ pci_using_dac = 1;
+ err = pci_set_consistent_dma_mask(pdev, DMA_64BIT_MASK);
+ if (err) {
+ dev_err(&pdev->dev, "unable to obtain 64-bit DMA for "
+ "coherent allocations\n");
+ goto out_release_regions;
this looks wrong; if you can't get 64 bit coherent allocs but can get 32
bit ones.. why error out ?
This is how most of the existing drivers behave.
Cheers,
Divy
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
