[PATCH] New driver 3Com 3C359 Tokenring Velocity XL

[PATCH] New driver 3Com 3C359 Tokenring Velocity XL

[Mike Phillips]

All,

The driver for the 3c359 adapter has been tested in the wild for several
months now and ready for inclusion in the kernel.

The patch itself is fairly large due to the microcode required for the
adapter, so links provided rather than including in this email.

2.4.17:

plain: http://www.linuxtr.net/download/3com/3c359-2.4.17.patch
gzip: http://www.linuxtr.net/download/3com/3c359-2.4.17.patch.gz

2.5.5pre1

plain: http://www.linuxtr.net/download/3com/3c359-2.5.5pre1.patch
gzip: http://www.linuxtr.net/download/3com/3c359-2.5.5pre1.patch.gz

The only difference between the 2.4.17 and 2.5.5pre1 patch is the part
for the Configure.help / Config.help files.

Thanks to 3Com for providing the tech docs for the adapter and to all the
beta testers for the driver.

Thanks
Mike Phillips
Linux Token Ring Project
http://www.linuxtr.net

Re: [PATCH] New driver 3Com 3C359 Tokenring Velocity XL

[Jeff Garzik]

Thanks, applied to 2.4 and 2.5.  I removed the definition of
PCI_DEVICE_xxx from the top of 3c359.h...

Comments:
1) buggy use of PCI DMA API -- you should use memory returned from
pci_alloc_consistent, do not directly map memory created by
alloc_trdev() nor depend on the alignment returned by alloc_trdev()

2) support for ETHTOOL_GDRVINFO ioctl
3) support for ETHTOOL_[GS]MSGLVL ioctls... this implies that
'message_level' would only serve as a default for a value that can be
changed per-interface

4) ideally "\n" should not be in MODULE_DESCRIPTION

5) style: no need to cast to/from a void pointer, such as
	> struct xl_private *xl_priv = (struct xl_private *)dev->priv ;

6) hardcoded magic numbers for PKT_BUF_SZ limits (100 and 18000)

7) xl_probe duplicates error handling code... use the standard kernel
style of multiple goto targets, one for each needed stage of
cleanup-after-error:

	err_out3:
		pci_release_regions(pdev)
	err_out2:
		kfree(dev)
	err_out:
		return rc;

8) in  xl_hw_reset, you probably want to call yield [in 2.5] or
schedule_timeout

9) jiffies comparison bug: never directly compare jiffies, use
time_before() or time_after()

10) in xl_open, return the error value returned by request_irq, on error

11) same comment for xl_open as #7

12) xl_wait_misr_flags needs to call yield() or -something-, don't just
empty loop.  cpu_relax(), for example, if you cannot schedule...

Overall... good job, it's a readable, clean driver.

	Jeff




-- 
Jeff Garzik      | "Why is it that attractive girls like you
Building 1024    |  always seem to have a boyfriend?"
MandrakeSoft     | "Because I'm a nympho that owns a brewery?"
                 |             - BBC TV show "Coupling"

Re: [PATCH] New driver 3Com 3C359 Tokenring Velocity XL

[Mike Phillips]

Jeff: 

> Comments:
> 1) buggy use of PCI DMA API -- you should use memory returned from
> pci_alloc_consistent, do not directly map memory created by
> alloc_trdev() nor depend on the alignment returned by alloc_trdev()
 
The driver doesn't map any of the ->priv structure, the comment in the
code is a left over from when it did. The priv strcture just has
pointers to the memory areas (which are kmalloc'ed and mapped). 
I should probably change the allocations to pci_alloc_consistent 
from their current map_single as well. 

All other comments taken on board, will be included in the next
update.

> Overall... good job, it's a readable, clean driver.
> 
Thanks, these things do get a little easier each time you do one (esp.
once you've figured out which planet the hardware designers and tech
doc writers live on :)
-- 
Mike Phillips
Linux Token Ring Project
http://www.linuxtr.net
mailto: mikep@linuxtr.net