network driver updates
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): Pine.LNX.3.96.1010214020707.28011E-100000@mandrakesoft.mandrakesoft.com
PATCH 1/1 network driver updates
I found 2 bugs in several network drivers:
* dev->mem_start: NULL means "not command line configuration" 0xffffffff means "default".
several drivers only check for NULL, not for 0xffffffff.
* incorrect bounds checks for phy_idx: 2 entries in the structure, but up to 4 are initialized.
* something is wrong in the vortex initialization: I don't have such a card, but the driver didn't return an error message on insmod. I'm not sure if
my fix is correct.
Patch attached, against 2.4.1-ac12.
--
Manfred
drivers/net/3c59x.c +6 -3
@@ -967,7 +967,7 @@ pdev->driver_data = dev;/* The lower four bits are the media type. */ -if (dev->mem_start) { +if (dev->mem_start && dev->mem_start !=0xffffffff ) {/* * AKPM: ewww.. The 'options' param is passed in as the third arg to the * LILO 'ether=' argument for non-modular use
@@ -2661,9 +2661,12 @@ rc =pci_module_init (&vortex_driver);if (rc <0 ) { - rc =vortex_eisa_init (); -if (rc >0 ) +int rc2; + rc2 =vortex_eisa_init (); +if (rc2 >0 ) { vortex_have_eisa =1 ; + rc =0 ; + } }else { vortex_have_pci =1 ; }
drivers/net/eepro100.c +1 -1
@@ -643,7 +643,7 @@return -1 ; } -if (dev->mem_start >0 ) +if (dev->mem_start && dev->mem_start !=0xffffffff ) option = dev->mem_start;else if (card_idx >=0 && options[card_idx] >=0 ) option = options[card_idx];
drivers/net/epic100.c +1 -1
@@ -412,7 +412,7 @@ ep->rx_ring = (struct epic_rx_desc *)ring_space; ep->rx_ring_dma = ring_dma; -if (dev->mem_start) { +if (dev->mem_start && dev->mem_start !=0xffffffff ) { option = dev->mem_start; duplex = (dev->mem_start &16 ) ?1 :0 ; }else if (card_idx >=0 && card_idx <MAX_UNITS ) {
drivers/net/hamachi.c +4 -3
@@ -477,6 +477,7 @@ };#define PRIV_ALIGN 15/* Required alignment mask */ +#define MII_CNT 4struct hamachi_private {/* Descriptor rings first for alignment. Tx requires a second descriptor for status. */
@@ -503,7 +504,7 @@/* MII transceiver section. */ int mii_cnt;/* MII device addresses. */ u16 advertising;/* NWay media advertisement */ -unsigned char phys[2 ];/* MII device addresses. */ +unsigned char phys[MII_CNT ];/* MII device addresses, only first one used. */ u_int32_t rx_int_var, tx_int_var;/* interrupt control variables */ u_int32_t option;/* Hold on to a copy of the options */ u_int8_t pad[16 ];/* Used for 32-byte alignment */
@@ -621,7 +622,7 @@/* Check for options being passed in */ option = card_idx <MAX_UNITS ? options[card_idx] :0 ; -if (dev->mem_start) +if (dev->mem_start && dev->mem_start !=0xffffffff ) option = dev->mem_start;/* If the bus size is misidentified, do the following. */
@@ -705,7 +706,7 @@if (chip_tbl[hmp->chip_id].flags & CanHaveMII) {int phy, phy_idx =0 ; -for (phy =0 ; phy <32 && phy_idx <4 ; phy++) { +for (phy =0 ; phy <32 && phy_idx <MII_CNT ; phy++) {int mii_status =mdio_read (ioaddr, phy,1 );if (mii_status !=0xffff && mii_status !=0x0000 ) {
drivers/net/natsemi.c +1 -1
@@ -446,7 +446,7 @@ np->iosize = iosize;spin_lock_init (&np->lock); -if (dev->mem_start) +if (dev->mem_start && dev->mem_start !=0xffffffff ) option = dev->mem_start;/* The lower four bits are the media type. */
drivers/net/starfire.c +4 -3
@@ -329,6 +329,7 @@dma_addr_t mapping; }; +#define MII_CNT 4struct netdev_private {/* Descriptor rings first for alignment. */ struct starfire_rx_desc *rx_ring;
@@ -365,7 +366,7 @@/* MII transceiver section. */ int mii_cnt;/* MII device addresses. */ u16 advertising;/* NWay media advertisement */ -unsigned char phys[2 ];/* MII device addresses. */ +unsigned char phys[MII_CNT ];/* MII device addresses, only first one used. */ };static int mdio_read (struct net_device *dev,int phy_id,int location);
@@ -467,7 +468,7 @@ np->pci_dev = pdev; drv_flags = netdrv_tbl[chip_idx].drv_flags; -if (dev->mem_start) +if (dev->mem_start && dev->mem_start !=0xffffffff ) option = dev->mem_start;/* The lower four bits are the media type. */
@@ -499,7 +500,7 @@if (drv_flags & CanHaveMII) {int phy, phy_idx =0 ; -for (phy =0 ; phy <32 && phy_idx <4 ; phy++) { +for (phy =0 ; phy <32 && phy_idx <MII_CNT ; phy++) {int mii_status =mdio_read (dev, phy,1 );if (mii_status !=0xffff && mii_status !=0x0000 ) { np->phys[phy_idx++] = phy;
drivers/net/sundance.c +4 -3
@@ -314,6 +314,7 @@#define PRIV_ALIGN 15/* Required alignment mask */ /* Use __attribute__((aligned (L1_CACHE_BYTES))) to maintain alignment within the structure. */ +#define MII_CNT 4struct netdev_private {/* Descriptor rings first for alignment. */ struct netdev_desc rx_ring[RX_RING_SIZE ];
@@ -346,7 +347,7 @@/* MII transceiver section. */ int mii_cnt;/* MII device addresses. */ u16 advertising;/* NWay media advertisement */ -unsigned char phys[2 ];/* MII device addresses. */ +unsigned char phys[MII_CNT ];/* MII device addresses, only first one used. */ };/* The station address location in the EEPROM. */
@@ -425,7 +426,7 @@ np->drv_flags = pci_id_tbl[chip_idx].drv_flags;spin_lock_init (&np->lock); -if (dev->mem_start) +if (dev->mem_start && dev->mem_start !=0xffffffff ) option = dev->mem_start;/* The lower four bits are the media type. */
@@ -458,7 +459,7 @@if (1 ) {int phy, phy_idx =0 ; np->phys[0 ] =1 ;/* Default setting */ -for (phy =0 ; phy <32 && phy_idx <4 ; phy++) { +for (phy =0 ; phy <32 && phy_idx <MII_CNT ; phy++) {int mii_status =mdio_read (dev, phy,1 );if (mii_status !=0xffff && mii_status !=0x0000 ) { np->phys[phy_idx++] = phy;
drivers/net/tulip/tulip_core.c +3 -1
@@ -1294,9 +1294,11 @@if (mtu[board_idx] >0 ) dev->mtu = mtu[board_idx]; } -if (dev->mem_start) +if (dev->mem_start && dev->mem_start !=0xffffffff ) tp->default_port = dev->mem_start;if (tp->default_port) { +printk ("%s: forced to media type %s (%d).\n" , + dev->name, medianame[tp->default_port], tp->default_port); tp->medialock =1 ;if (tulip_media_cap[tp->default_port] & MediaAlwaysFD) tp->full_duplex =1 ;
drivers/net/via-rhine.c +1 -1
@@ -571,7 +571,7 @@ np->drv_flags = via_rhine_chip_info[chip_id].drv_flags; np->pdev = pdev; -if (dev->mem_start) +if (dev->mem_start && dev->mem_start !=0xffffffff ) option = dev->mem_start;/* The lower four bits are the media type. */
drivers/net/winbond-840.c +4 -3
@@ -313,6 +313,7 @@ };#define PRIV_ALIGN 15/* Required alignment mask */ +#define MII_CNT 4struct netdev_private {struct w840_rx_desc *rx_ring;dma_addr_t rx_addr[RX_RING_SIZE ];
@@ -344,7 +345,7 @@/* MII transceiver section. */ int mii_cnt;/* MII device addresses. */ u16 advertising;/* NWay media advertisement */ -unsigned char phys[2 ];/* MII device addresses. */ +unsigned char phys[MII_CNT ];/* MII device addresses, but only the first is used */ };static int eeprom_read (long ioaddr,int location);
@@ -436,7 +437,7 @@ pdev->driver_data = dev; -if (dev->mem_start) +if (dev->mem_start && dev->mem_start !=0xffffffff ) option = dev->mem_start;/* The lower four bits are the media type. */
@@ -465,7 +466,7 @@if (np->drv_flags & CanHaveMII) {int phy, phy_idx =0 ; -for (phy =1 ; phy <32 && phy_idx <4 ; phy++) { +for (phy =1 ; phy <32 && phy_idx <MII_CNT ; phy++) {int mii_status =mdio_read (dev, phy,1 );if (mii_status !=0xffff && mii_status !=0x0000 ) { np->phys[phy_idx++] = phy;
drivers/net/yellowfin.c +4 -3
@@ -328,6 +328,7 @@ IntrEarlyRx=0x100, IntrWakeup=0x200, };#define PRIV_ALIGN 31/* Required alignment mask */ +#define MII_CNT 4struct yellowfin_private {/* Descriptor rings first for alignment. Tx requires a second descriptor for status. */
@@ -357,7 +358,7 @@/* MII transceiver section. */ int mii_cnt;/* MII device addresses. */ u16 advertising;/* NWay media advertisement */ -unsigned char phys[2 ];/* MII device addresses. */ +unsigned char phys[MII_CNT ];/* MII device addresses, only first one used */ spinlock_t lock; };
@@ -453,7 +454,7 @@ np->chip_id = chip_idx; np->drv_flags = drv_flags; -if (dev->mem_start) +if (dev->mem_start && dev->mem_start !=0xffffffff ) option = dev->mem_start;/* The lower four bits are the media type. */
@@ -485,7 +486,7 @@if (np->drv_flags & HasMII) {int phy, phy_idx =0 ; -for (phy =0 ; phy <32 && phy_idx <4 ; phy++) { +for (phy =0 ; phy <32 && phy_idx <MII_CNT ; phy++) {int mii_status =mdio_read (ioaddr, phy,1 );if (mii_status !=0xffff && mii_status !=0x0000 ) { np->phys[phy_idx++] = phy;
Quoted 3 lines · Manfred Spraul
*
dev->mem_start: NULL means "not command line configuration"0xffffffffmeans "default".
several drivers only check for NULL, not for0xffffffff.
netdev->mem_start is unsigned long... Should the test be for ~0 instead? The value 0xFFFFFFFF seems wrong for 64-bit machines.
Jeff
Quoted 10 lines · Manfred Spraul
* something is wrong in the vortex initialization: I don't have such a card, but the driver didn't return an error message on insmod. I'm not sure if
my fix is correct.drivers/net/3c59x.c +0
-1@@ -2661,9 +2661,12 @@ rc =pci_module_init (&vortex_driver);if (rc <0 ) { - rc =vortex_eisa_init ();
drivers/net/3c59x.c line 2672
Quoted 10 lines
drivers/net/3c59x.c +5
-1-if (rc >0 ) +int rc2; + rc2 =vortex_eisa_init (); +if (rc2 >0 ) { vortex_have_eisa =1 ; + rc =0 ; + } }else { vortex_have_pci =1 ; }
IMHO vortex should be trying to initialize EISA regardless of the results of the PCI probe... Andrew?
Jeff
Em Wed, Feb 14, 2001 at 05:54:34AM -0600, Jeff Garzik escreveu:
Quoted 7 lines · Manfred Spraul
Quoted 3 lines
*
dev->mem_start: NULL means "not command line configuration"0xffffffffmeans "default".
several drivers only check for NULL, not for0xffffffff.
netdev->mem_startis unsigned long... Should the test be for ~0 instead? The value0xFFFFFFFFseems wrong for 64-bit machines.
I've added this to the Janitor's TODO List.
http://bazar.conectiva.com.br/~acme/TODO
- Arnaldo
Quoted 6 lines · Manfred Spraul
I found 2 bugs in several network drivers:
*
dev->mem_start: NULL means "not command line configuration"0xffffffffmeans "default".
several drivers only check for NULL, not for0xffffffff.
I think that's worth another "ewww...", don't you?
Quoted 3 lines
* something is wrong in the vortex initialization: I don't have such a card, but the driver didn't return an error message on insmod. I'm not sure if my fix is correct.
That was intentional - dhinds suggested that if the hardware isn't present the driver should float about in memory anyway. That is the current behaviour of 3c575_cb.c I forget the rationale for this. Perhaps David can remind me?
There are a fair few pending 3c59x updates wrt Linus' tree - small stuff. Most are in the zerocopy patch, and there's a delta on top of that here.
Quoted 4 lines · Jeff Garzik
IMHO vortex should be trying to initialize EISA regardless of the results of the PCI probe... Andrew?
I guess so. Testing is hard - EISA vorticies are hen's teeth. I've heard from only two people who have such hardware in a year.
I've made the below change. Pending enlightenment from DavidH and some testing from my sole EISA Vortex person, I'll flush this stuff out to DavidM. Thanks.
net/3c59x.c +11 -13
@@ -866,8 +866,7 @@ } rc =vortex_probe1 (NULL , ioaddr,inw (ioaddr +0xC88 ) >>12 , -EISA_TBL_OFFSET , - vortex_cards_found); +EISA_TBL_OFFSET , vortex_cards_found);if (rc ==0 ) vortex_cards_found++;else
@@ -1005,7 +1004,7 @@ pdev->driver_data = dev;/* The lower four bits are the media type. */ -if (dev->mem_start) { +if (dev->mem_start && dev->mem_start != ~0UL ) {/* * AKPM: ewww.. The 'options' param is passed in as the third arg to the * LILO 'ether=' argument for non-modular use
@@ -2794,18 +2793,17 @@static int __init vortex_init (void ) { -int rc; - - rc =pci_module_init (&vortex_driver); -if (rc <0 ) { - rc =vortex_eisa_init (); -if (rc >0 ) - vortex_have_eisa =1 ; - }else { +int pci_rc, eisa_rc; + + pci_rc =pci_module_init (&vortex_driver); + eisa_rc =vortex_eisa_init (); + +if (pci_rc ==0 ) vortex_have_pci =1 ; - } +if (eisa_rc >0 ) + vortex_have_eisa =1 ; -return rc; +return (vortex_have_pci + vortex_have_eisa) ?0 : -ENODEV ; }
Quoted 7 lines · Andrew Morton
Quoted 4 lines
* something is wrong in the vortex initialization: I don't have such a card, but the driver didn't return an error message on insmod. I'm not sure if my fix is correct.
That was intentional - dhinds suggested that if the hardware isn't present the driver should float about in memory anyway.
Say the driver is linked into the kernel. Hot plug drivers should not all complain about not finding their hardware.
-- Dave
Quoted 11 lines · David Hinds
Quoted 7 lines · Andrew Morton
Quoted 4 lines
* something is wrong in the vortex initialization: I don't have such a card, but the driver didn't return an error message on insmod. I'm not sure if my fix is correct.
That was intentional - dhinds suggested that if the hardware isn't present the driver should float about in memory anyway.
Say the driver is linked into the kernel. Hot plug drivers should not all complain about not finding their hardware.
Yes; that is the whole reason why pci_register_driver does not error out when it finds zero matching devices.
Jeff
Quoted 4 lines · David Hinds
Say the driver is linked into the kernel. Hot plug drivers should not all complain about not finding their hardware.
That's handled by pci_module_init(), check <linux/pci.h>:
if CONFIG_HOTPLUG is enabled, then pci_module_init() never returns with -ENODEV.
Which means that eisa cards will never be probed in a hotplug enabled kernel.
And loading the current 3c59x.c into a non CONFIG_HOTPLUG non EISA kernel results in a disconnected driver:
it's _not_ registered as a pci driver, pci_module_init() calls pci_unregister_driver().
What about the attached patch?
drivers/net/3c59x.c +6 -3
@@ -2661,13 +2661,16 @@ rc =pci_module_init (&vortex_driver);if (rc <0 ) { - rc =vortex_eisa_init (); -if (rc >0 ) - vortex_have_eisa =1 ; +if (rc != -ENODEV ) +return rc; }else { vortex_have_pci =1 ; } +if (vortex_eisa_init () >0 ) { + vortex_have_eisa =1 ; + rc =0 ; + }return rc; }
Quoted 11 lines · Manfred Spraul
Quoted 4 lines · David Hinds
Say the driver is linked into the kernel. Hot plug drivers should not all complain about not finding their hardware.
That's handled by
pci_module_init(), check <linux/pci.h>:
ifCONFIG_HOTPLUGis enabled, thenpci_module_init()never returns with-ENODEV.
Quoted 7 lines
Which means that eisa cards will never be probed in a hotplug enabled kernel.
And loading the current
3c59x.cinto a nonCONFIG_HOTPLUGnon EISA kernel results in a disconnected driver:
it's _not_ registered as a pci driver,pci_module_init()callspci_unregister_driver().
That's what we want to happen, isn't it? If it's not a hotplug kernel and the hardware isn't there at boot time, don't register the PCI driver. But still scan for EISA devices. That's what the patch I sent yesterday does. Here it is:
static int __init vortex_init (void)
{
int pci_rc, eisa_rc;
pci_rc = pci_module_init(&vortex_driver);
eisa_rc = vortex_eisa_init();
if (pci_rc == 0)
vortex_have_pci = 1;
if (eisa_rc > 0)
vortex_have_eisa = 1;
return (vortex_have_pci + vortex_have_eisa) ? 0 : -ENODEV;
}Really, vortex_have_pci should be calledvortex_have_pci_or_hotplug_and_dont_have_pci.
Look. When the driver's init_module() method is called there are four combinations which must be catered for (ignoring EISA):
CONFIG_HOTPLUG=n, MODULE=false
If the hardware isn't there, don't register the pci_driver. It can't _do_ anything.
CONFIG_HOTPLUG=n, MODULE=true
If the hardware isn't there, barf. modprobe will remove the driver again.
CONFIG_HOTPLUG=y, MODULE=false
If the hardware isn't there, register the pci driver, because the hardware may appear later
CONFIG_HOTPLUG=y, MODULE=true
If the hardware isn't there, barf. modprobe will remove the driver. Later, when the hardware is inserted, another modprobe will succeed.
This is what yesterday's patch implements.
Now, the thing I don't understand about David's design is the final one. What 3c575_cb does is:
CONFIG_HOTPLUG=y, MODULE=true
If the hardware isn't there, register the driver and hang around.
Why?
BTW: How come CONFIG_PCMCIA requires CONFIG_HOTPLUG? Doesn't it make sense to be able to have glued-in Cardbus devices?
-
Quoted 9 lines · Andrew Morton
Now, the thing I don't understand about David's design is the final one. What 3c575_cb does is:
CONFIG_HOTPLUG=y, MODULE=true
If the hardware isn't there, register the driver and hang around.Why?
Merely that I was trying to disassociate the concepts of module loading and device probing, and I thought it was most consistent to then allow people to load modules whenever they want, whether or not a device was present.
Quoted 2 lines
BTW: How come
CONFIG_PCMCIArequiresCONFIG_HOTPLUG? Doesn't it make sense to be able to have glued-in Cardbus devices?
I suppose it makes sense but I don't know if it is something worth the trouble of supporting.
-- Dave
Quoted 10 lines · David Hinds
Quoted 8 lines · Andrew Morton
Now, the thing I don't understand about David's design is the final one. What 3c575_cb does is:
CONFIG_HOTPLUG=y, MODULE=true
If the hardware isn't there, register the driver and hang around.
Quoted 6 lines
Quoted 1 line
Why?
Merely that I was trying to disassociate the concepts of module loading and device probing, and I thought it was most consistent to then allow people to load modules whenever they want, whether or not a device was present.
Fair enough. Thanks.
Another scenario may be where (say) a network driver is modprobed from a hotpluggable disk controller. You want to be able to load the netdriver, pop out the disk controller and then insert the network card. Or vice-versa - load a parallel port driver across NFS then pull the network card and push the parallel port card. Could use local disks for this, of course.
hmm..