Hello Jeff;
Many of the problems that you quickly found are in the original driver. I am
happy to fix many of the minor problems for you, but I am reluctant to make
major changes, since someone else is the maintainer of this code. I do not
want to step on anyone's toes. I just made the barest minimum changes to
allow the driver work on the ARM architecture without breaking i86 (I have
two different PCI boards which I used to test the driver on i86). If we do
not hear from tsbogend@alpha.franken.de in next few weeks, then I will
assume that he has fallen off of the face of the earth and I will be happy
to make the more major changes you suggested. I agree that the changes need
to be done.
Thank you for your help and prompt response.
>* why are calls to pci_dma_sync_single called with "if (0)" ? Are you
>sure this patch should be applied to the kernel?
>
>+ if (0) pci_dma_sync_single(lp->pci_dev, lp->rx_dma_addr[i],
>rx_skbuff->len, PCI_DMA_FROMDEVICE);
I was not certain, if I need to call pci_dma_sync_single, therefore I used
the calls and then if (0)ed them to see if they were really necessary. It
turns out that they were not needed and I just forgot to remove the lines.
>* pcnet32_tbl[] seems to duplicate needlessly information found in
>pcnet32_pci_tbl[]
It does duplicate information. I was uncertain how VLbus would handle the
changes so I left this alone.
>* MODULE_DEVICE_TABLE not present in code
The original driver is this way.
>* do not use check-region. instead, use request-region and test its
>return value for NULL (== region unavailable).
The original driver is this way. I just cut and pasted.
>* use "return -ENODEV" instead of "return ENODEV". The '-' style is
>preferred since typical error tests throughout the code look like>
>
> if (rc < 0) { /* we got an error */ }
The original driver was this way. Fixed in nine (9) places.
>* two errors in the following code. (1) "if (0)", (2) you don't need to
>print out a message, the PCI subsystem does that for you
The original driver was this way for item (2). Fixed item (1).
>* consider turning "switch (chip-version)" into a lookup table.
I agree it is a good idea, but more than a minor change.
>* You don't need to allocate and align your private structure manually.
>init_etherdev does this for you. Simply pass the -true- size of your
>private structure to init_ethernet, and it will allocate and align the
>space for you. All you need to do is take the value of dev->priv:
>
> dev = init_etherdev (NULL, sizeof (struct my_private_struct));
> if (!dev) return -ENOMEM;
> my_priv = dev->priv;
> ...
This is part of the original driver. From the documentation in the original
driver, it is claimed that the struct must be aligned on a sixteen (16) BYTE
boundary. I will have to look at the manufactures data sheet to verify this
and I would like to verify that init_etherdev aligns to a sixteen (16) BYTE
boundary, before making such a change.
>* all calls to printk() should have their formats contain a KERN_xxx
>prefix:
>
> printk(KERN_WARNING "warrior is about to die\n");
>
The original driver was this way. Fixed.
>* ioctl() needs to be protected by hardware spinlock
The original driver was this way.
>* why don't you implement a PCI driver cleanup in the remove() method?
>By not doing so, you are preventing any chance of hotplug with this
>driver. Plus, since pci_unregister_driver calls remove() once for each
>PCI driver, it should simplify the code a bit -- you don't have to
>manually loop in cleanup_module or similar. Also note you can use
>pdev->driver_data to store information (like dev or dev->priv) which is
>generally kept in a linked list. For example of drivers which do this,
>while supporting non-PCI buses like EISA or VLB, check out the updated
>3c59x.c.
I agree, the original driver was this way. If the maintainer does not appear
then I will commit to making this change and all the other changes.
>* check the return value of pci_module_init!
Fixed.
Best Regards,
--George
George France, france@crl.dec.com
Cambridge Research Laboratory, Compaq Computer Corporation
One Kendall Square, Building 700 MS: CRL
Cambridge, MA 02139 USA
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun May 07 2000 - 21:00:11 EST