[ Trimmed To: list ]
Jeff Garzik wrote:
>
> comments:
Thanks, Jeff. Much appreciated.
> becker has new stuff posted at http://www.scyld.com/ check out the new
> code there, and also update links in your docs/code...
Links updated. That site appears to link back to nasa.gov for the
vortex driver source.
> Just increase the version to LK1.1.3, no need for AKPM added on to
> version. Or if you are gonna actively maintain the driver like I do
> with 8139too/tulip, ie. where the driver diverges from donald's quite a
> bit, then you might as well use your own version scheme.
LK1.1.3 it is. I like versioning things in "kernel time", so this
would be 3c59x.c-2.3.99-pre6-5-2. Whatever.
> Do you even need #warning near the top, why not just the #error line?
True. Multiple #errors are allowed.
> bug: you reverted a change which moved 'version' to the file of the
> file, removing '__initdata' marker in the process. (this needs to be
> below linux/init.h...) and it should be __devinitdata not __initdata.
Done.
> can vortex_info_tbl be marked __devinitdata? (make sure to check, often
> it cannot be marked __*initdata due to usage after xxx_init_one has
> ended)
Yes, with some juggling it can and it now is. I removed the 'chip_idx'
from the device private structure, so now only vortex_probe1 knows the
offset of this device in the info table. This will prevent accidental
references to released memory creeping in in the future.
> any feedback on whether or not EISA works? (not a showstopped of
> course...)
Unfortunately, no. I had five or six people respond with testing but
they were all 575, 905B and 905C users. Alan has a 590 "somewhere"
which he'll be testing the 2.2 patch with, but I need to keep trolling
computer junkyards...
> return error value from pci_enable_device
Tick.
> suggestion: using __setup() create a setup function which allows people
> to pass options into the driver using "3c59x=....", if using the third
> option of ether= bugs you :)
Sleeping dogs. I'll document the current usage next time.
> can you make ram_split[] const as well?
Yes.
> bug: when freeing a netdev, you should call unregister_netdevice(dev)
> then kfree(dev). vortex_probe1's 'free_dev' label doesn't call
> unregister_netdevice(dev). Make sure vortex_remove_one also calls
> unregister_netdevice.
Tick.
> bug: there is at least one place in the diff which reverts Dave
> Miller's PCI DMA changes. Basically grep for 'virt_to_bus' and
> similar. These should NEVER occur in the source code, you should be
> referencing the handles returned from pci_alloc_xxx: rx_ring_dma, etc.
> example bad change:
> - outl(vp->rx_ring_dma, ioaddr + UpListPtr);
> + outl(virt_to_bus(&vp->rx_ring[0]), ioaddr + UpListPtr);
Mangled merge. Hopefully sorted out now.
> it might be better to use a semaphore or something other than a spinlock
> to protect ioctl and get-stats.
The locking is not only to protect ioctl and get_stats from each other,
but to protect them from start_xmit, the ISR, the media selection timer
routine, tx_timeout, etc.
> what is the purpose of vp->open? it seems redundant to netif_xxx bits,
> and possibly racy
It is to help the suspend() and resume() methods keep track of the
open/close state of the interface.
Yes, it may duplicate the use of IFF_UP.
The whole power management side of things is sick at this time. The
suspend() and resume() methods don't seem to be getting called, 'cardctl
suspend' is irreversible and the wake-on-LAN stuff kills the 905 NICs
completely. There are similar problems with the existing 3c575_cb, so
it may indicate problems outside the driver, or an interface misfit.
Some of the problems I am seeing line up with the ones David Ford
reported today.
So I have left all this code in place, having simply disabled the part
which was causing trouble. I'll be continuing to work this and the
outstanding problem reports from the various mailing lists.
> xxx_start_xmit should not call netif_stop_queue at the beginning of the
> routine, but instead should probably look like this:
>
> give packet to hardware
> if (no more TX room)
> netif_stop_queue ()
OK, I think I got this change right. There is a netif_start_queue() in
vortex_start_xmit() which I believe to be redundant, but it's cheap and
I can't test it...
There is a netif_wake_queue() in vortex_interrupt() which I don't like.
Left it there with a FIXME pending hardware availability.
And I am not convinced that vortex_tx_timeout actually does the right
thing for vortex cards. It's functionally the same as Don's, and I
guess he has a 590...
> i wouldn't include wake-on-lan stuff in the code unless you've tested
> it...
See above.
> in vortex_remove_one, if dev==NULL you should probably call BUG()
Added a printk(KERN_ERR...
> i would ditch the comment in drivers/net/pcmcia/Config.in saying
> "3c575_cb has been moved"
Gone.
Thanks again. The new patch and changelog are at
http://www.uow.edu.au/~andrewm/linux/#3c59x-2.3
Also, you may care to cast an eye across my proposed 2.2 patch:
http://www.uow.edu.au/~andrewm/linux/#3c59x-2.2
This patch fixes some nasty, rarely-occurring races. This is with Alan
at present. SPARC and 3c590 testing desperately needed.
-- -akpm-- 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 Apr 30 2000 - 21:00:09 EST