finer-grained locking in wan/lmc driver
Subsystems (2)
PATCH 1/1 finer-grained locking in wan/lmc driver
Hi Jeff,
Patch against 2.6-test6.
- lmc_ioctl was calling copy_{from|to}_user with the device lock
held, replace it with finer-grained locking;
- Remove check_version usage;
- If copy_from_user fails, returns -EFAULT instead of -ENOMEM;
I didn't acquire the device lock before calling lmc_proto_ioctl,
which will call syncppp ioctls, which will might call copy_{to|from}_user, but I think the locking shouldn't be done on that layer of the driver. (maybe on lmc_proto_main?)
I'm not sure I locked everywhere I was suppose to lock, please review.
If it looks ok, please consider applying,
Thanks.
Felipe
net/wan/lmc/lmc_main.c renamed from net/wan/lmc/lmc_main.c.orig +38 -29
@@ -132,7 +132,6 @@lmc_ctl_t ctl;int ret;u_int16_t regVal; -unsigned long flags;struct sppp *sp;
@@ -142,12 +141,6 @@lmc_trace (dev,"lmc_ioctl in" ); -/* -* Most functions mess with the structure -* Disable interrupts while we do the polling -*/ -spin_lock_irqsave (&sc->lmc_lock, flags); -switch (cmd) {/* * Return current driver state. Since we keep this up
@@ -173,7 +166,8 @@if (copy_from_user (&ctl, ifr->ifr_data,sizeof (lmc_ctl_t)))return -EFAULT ; - + +spin_lock_irq (&sc->lmc_lock); sc->lmc_media->set_status (sc, &ctl);if (ctl.crc_length != sc->ictl.crc_length) {
@@ -188,9 +182,11 @@ sp->pp_flags &= ~PP_KEEPALIVE ;/* Turn off */ else sp->pp_flags |= PP_KEEPALIVE; /* Turn on */ + +spin_unlock_irq (&sc->lmc_lock); ret =0 ; -break ; +break ;case LMCIOCIFTYPE :/*fold01*/ {
@@ -204,14 +200,14 @@if (copy_from_user (&new_type, ifr->ifr_data,sizeof (u_int16_t)))return -EFAULT ; - - +if (new_type == old_type) { - ret =0 ; + ret =0 ;break ;/* no change */ } - + +spin_lock_irq (&sc->lmc_lock);lmc_proto_close (sc);lmc_proto_detach (sc);
@@ -219,12 +215,14 @@// lmc_proto_init(sc); lmc_proto_attach (sc);lmc_proto_open (sc); +spin_unlock_irq (&sc->lmc_lock); ret =0 ;break ; }case LMCIOCGETXINFO :/*fold01*/ +spin_lock_irq (&sc->lmc_lock); sc->lmc_xinfo.Magic0 =0xBEEFCAFE ; sc->lmc_xinfo.PciCardType = sc->lmc_cardtype;
@@ -240,6 +238,7 @@ sc->lmc_xinfo.Magic1 =0xDEADBEEF ; +spin_unlock_irq (&sc->lmc_lock);if (copy_to_user (ifr->ifr_data, &sc->lmc_xinfo,sizeof (struct lmc_xinfo )))return -EFAULT ;
@@ -248,6 +247,7 @@break ;case LMCIOCGETLMCSTATS :/*fold01*/ +spin_lock_irq (&sc->lmc_lock);if (sc->lmc_cardtype ==LMC_CARDTYPE_T1 ){lmc_mii_writereg (sc,0 ,17 ,T1FRAMER_FERR_LSB ); sc->stats.framingBitErrorCount +=
@@ -271,6 +271,7 @@ sc->stats.severelyErroredFrameCount += regVal &T1FRAMER_SEF_MASK ; } +spin_unlock_irq (&sc->lmc_lock);if (copy_to_user (ifr->ifr_data, &sc->stats,sizeof (struct lmc_statistics )))
@@ -285,11 +286,13 @@break ; } +spin_lock_irq (&sc->lmc_lock);memset (&sc->stats,0 ,sizeof (struct lmc_statistics )); sc->stats.check =STATCHECK ; sc->stats.version_size = (DRIVER_VERSION <<16 ) +sizeof (struct lmc_statistics ); sc->stats.lmc_cardtype = sc->lmc_cardtype; +spin_unlock_irq (&sc->lmc_lock); ret =0 ;break ;
@@ -306,8 +309,11 @@if (copy_from_user (&ctl, ifr->ifr_data,sizeof (lmc_ctl_t)))return -EFAULT ; - sc->lmc_media->set_circuit_type (sc, ctl.circuit_type); + +spin_lock_irq (&sc->lmc_lock); + sc->lmc_media->set_circuit_type (sc, ctl.circuit_type); sc->ictl.circuit_type = ctl.circuit_type; +spin_unlock_irq (&sc->lmc_lock); ret =0 ;break ;
@@ -319,9 +325,11 @@ }/* Reset driver and bring back to current state */ +spin_lock_irq (&sc->lmc_lock);printk (" REG16 before reset +%04x\n" ,lmc_mii_readreg (sc,0 ,16 ));lmc_running_reset (dev);printk (" REG16 after reset +%04x\n" ,lmc_mii_readreg (sc,0 ,16 )); +spin_unlock_irq (&sc->lmc_lock);LMC_EVENT_LOG (LMC_EVENT_FORCEDRESET ,LMC_CSR_READ (sc, csr_status),lmc_mii_readreg (sc,0 ,16 ));
@@ -364,6 +372,8 @@case lmc_xilinx_reset:/*fold02*/ {u16 mii; + +spin_lock_irq (&sc->lmc_lock); mii =lmc_mii_readreg (sc,0 ,16 );/*
@@ -422,11 +432,8 @@lmc_led_off (sc,LMC_DS3_LED2 ); } } - - - ret =0x0 ; - +spin_unlock_irq (&sc->lmc_lock); }break ;
@@ -434,6 +441,7 @@ {u16 mii;int timeout =500000 ; +spin_lock_irq (&sc->lmc_lock); mii =lmc_mii_readreg (sc,0 ,16 );/*
@@ -476,12 +484,10 @@* stop driving Xilinx-related signals */ lmc_gpio_mkinput (sc,0xff ); - ret =0x0 ; - +spin_unlock_irq (&sc->lmc_lock);break ; - }case lmc_xilinx_load:/*fold02*/
@@ -505,12 +511,13 @@if (copy_from_user (data, xc.data, xc.len)) {kfree (data); - ret = -ENOMEM ; + ret = -EFAULT ;break ; }printk ("%s: Starting load of data Len: %d at 0x%p == 0x%p\n" , dev->name, xc.len, xc.data, data); +spin_lock_irq (&sc->lmc_lock);lmc_gpio_mkinput (sc,0xff );/*
@@ -609,8 +616,8 @@kfree (data); +spin_unlock_irq (&sc->lmc_lock); ret =0 ; -break ; }default :/*fold02*/
@@ -619,7 +626,9 @@ }netif_wake_queue (dev); - sc->lmc_txfull =0 ; +spin_lock_irq (&sc->lmc_lock); + sc->lmc_txfull =0 ; +spin_unlock_irq (&sc->lmc_lock); }break ;
@@ -629,8 +638,6 @@break ; } -spin_unlock_irqrestore (&sc->lmc_lock, flags);/*fold01*/ -lmc_trace (dev,"lmc_ioctl out" );return ret;
@@ -930,7 +937,10 @@* later on, no one else will take our card away from * us. */ -request_region (ioaddr,LMC_REG_RANGE , dev->name); +if (!request_region (ioaddr,LMC_REG_RANGE , dev->name)) { +printk (KERN_WARNING "io port %3lX is busy" , ioaddr); +return NULL ; + } sc->lmc_cardtype =LMC_CARDTYPE_UNKNOWN ; sc->lmc_timing =LMC_CTL_CLOCK_SOURCE_EXT ;
@@ -1060,8 +1070,7 @@* Fix the two variables * */ -if (!(check_region (pci_ioaddr,LMC_REG_RANGE )) && - (vendor ==CORRECT_VENDOR_ID ) && +if ((vendor ==CORRECT_VENDOR_ID ) && (device ==CORRECT_DEV_ID ) && ((subvendor ==PCI_VENDOR_LMC ) || (subdevice ==PCI_VENDOR_LMC ))){struct net_device *cur, *prev =NULL ;
conflicts with a just-applied patch... can you wait a few days, then resend against the latest -bk snapshot?
Quoted 1 line · Jeff Garzik
conflicts with a just-applied patch... can you wait a few days, then resend against the latest -bk snapshot?
Yep.
Felipe
Hi Jeff,
Quoted 1 line · Jeff Garzik
conflicts with a just-applied patch... can you wait a few days, then resend against the latest -bk snapshot?
Resend against 2.6.0-test7-bk9. Please apply,
Felipe
drivers/net/wan/lmc/lmc_main.c renamed from drivers/net/wan/lmc/lmc_main.c.orig +38 -29
@@ -132,7 +132,6 @@lmc_ctl_t ctl;int ret;u_int16_t regVal; -unsigned long flags;struct sppp *sp;
@@ -142,12 +141,6 @@lmc_trace (dev,"lmc_ioctl in" ); -/* -* Most functions mess with the structure -* Disable interrupts while we do the polling -*/ -spin_lock_irqsave (&sc->lmc_lock, flags); -switch (cmd) {/* * Return current driver state. Since we keep this up
@@ -173,7 +166,8 @@if (copy_from_user (&ctl, ifr->ifr_data,sizeof (lmc_ctl_t)))return -EFAULT ; - + +spin_lock_irq (&sc->lmc_lock); sc->lmc_media->set_status (sc, &ctl);if (ctl.crc_length != sc->ictl.crc_length) {
@@ -188,9 +182,11 @@ sp->pp_flags &= ~PP_KEEPALIVE ;/* Turn off */ else sp->pp_flags |= PP_KEEPALIVE; /* Turn on */ + +spin_unlock_irq (&sc->lmc_lock); ret =0 ; -break ; +break ;case LMCIOCIFTYPE :/*fold01*/ {
@@ -204,14 +200,14 @@if (copy_from_user (&new_type, ifr->ifr_data,sizeof (u_int16_t)))return -EFAULT ; - - +if (new_type == old_type) { - ret =0 ; + ret =0 ;break ;/* no change */ } - + +spin_lock_irq (&sc->lmc_lock);lmc_proto_close (sc);lmc_proto_detach (sc);
@@ -219,12 +215,14 @@// lmc_proto_init(sc); lmc_proto_attach (sc);lmc_proto_open (sc); +spin_unlock_irq (&sc->lmc_lock); ret =0 ;break ; }case LMCIOCGETXINFO :/*fold01*/ +spin_lock_irq (&sc->lmc_lock); sc->lmc_xinfo.Magic0 =0xBEEFCAFE ; sc->lmc_xinfo.PciCardType = sc->lmc_cardtype;
@@ -240,6 +238,7 @@ sc->lmc_xinfo.Magic1 =0xDEADBEEF ; +spin_unlock_irq (&sc->lmc_lock);if (copy_to_user (ifr->ifr_data, &sc->lmc_xinfo,sizeof (struct lmc_xinfo )))return -EFAULT ;
@@ -248,6 +247,7 @@break ;case LMCIOCGETLMCSTATS :/*fold01*/ +spin_lock_irq (&sc->lmc_lock);if (sc->lmc_cardtype ==LMC_CARDTYPE_T1 ){lmc_mii_writereg (sc,0 ,17 ,T1FRAMER_FERR_LSB ); sc->stats.framingBitErrorCount +=
@@ -271,6 +271,7 @@ sc->stats.severelyErroredFrameCount += regVal &T1FRAMER_SEF_MASK ; } +spin_unlock_irq (&sc->lmc_lock);if (copy_to_user (ifr->ifr_data, &sc->stats,sizeof (struct lmc_statistics )))
@@ -285,11 +286,13 @@break ; } +spin_lock_irq (&sc->lmc_lock);memset (&sc->stats,0 ,sizeof (struct lmc_statistics )); sc->stats.check =STATCHECK ; sc->stats.version_size = (DRIVER_VERSION <<16 ) +sizeof (struct lmc_statistics ); sc->stats.lmc_cardtype = sc->lmc_cardtype; +spin_unlock_irq (&sc->lmc_lock); ret =0 ;break ;
@@ -306,8 +309,11 @@if (copy_from_user (&ctl, ifr->ifr_data,sizeof (lmc_ctl_t)))return -EFAULT ; - sc->lmc_media->set_circuit_type (sc, ctl.circuit_type); + +spin_lock_irq (&sc->lmc_lock); + sc->lmc_media->set_circuit_type (sc, ctl.circuit_type); sc->ictl.circuit_type = ctl.circuit_type; +spin_unlock_irq (&sc->lmc_lock); ret =0 ;break ;
@@ -319,9 +325,11 @@ }/* Reset driver and bring back to current state */ +spin_lock_irq (&sc->lmc_lock);printk (" REG16 before reset +%04x\n" ,lmc_mii_readreg (sc,0 ,16 ));lmc_running_reset (dev);printk (" REG16 after reset +%04x\n" ,lmc_mii_readreg (sc,0 ,16 )); +spin_unlock_irq (&sc->lmc_lock);LMC_EVENT_LOG (LMC_EVENT_FORCEDRESET ,LMC_CSR_READ (sc, csr_status),lmc_mii_readreg (sc,0 ,16 ));
@@ -364,6 +372,8 @@case lmc_xilinx_reset:/*fold02*/ {u16 mii; + +spin_lock_irq (&sc->lmc_lock); mii =lmc_mii_readreg (sc,0 ,16 );/*
@@ -422,11 +432,8 @@lmc_led_off (sc,LMC_DS3_LED2 ); } } - - - ret =0x0 ; - +spin_unlock_irq (&sc->lmc_lock); }break ;
@@ -434,6 +441,7 @@ {u16 mii;int timeout =500000 ; +spin_lock_irq (&sc->lmc_lock); mii =lmc_mii_readreg (sc,0 ,16 );/*
@@ -476,12 +484,10 @@* stop driving Xilinx-related signals */ lmc_gpio_mkinput (sc,0xff ); - ret =0x0 ; - +spin_unlock_irq (&sc->lmc_lock);break ; - }case lmc_xilinx_load:/*fold02*/
@@ -505,12 +511,13 @@if (copy_from_user (data, xc.data, xc.len)) {kfree (data); - ret = -ENOMEM ; + ret = -EFAULT ;break ; }printk ("%s: Starting load of data Len: %d at 0x%p == 0x%p\n" , dev->name, xc.len, xc.data, data); +spin_lock_irq (&sc->lmc_lock);lmc_gpio_mkinput (sc,0xff );/*
@@ -609,8 +616,8 @@kfree (data); +spin_unlock_irq (&sc->lmc_lock); ret =0 ; -break ; }default :/*fold02*/
@@ -619,7 +626,9 @@ }netif_wake_queue (dev); - sc->lmc_txfull =0 ; +spin_lock_irq (&sc->lmc_lock); + sc->lmc_txfull =0 ; +spin_unlock_irq (&sc->lmc_lock); }break ;
@@ -629,8 +638,6 @@break ; } -spin_unlock_irqrestore (&sc->lmc_lock, flags);/*fold01*/ -lmc_trace (dev,"lmc_ioctl out" );return ret;
@@ -930,7 +937,10 @@* later on, no one else will take our card away from * us. */ -request_region (ioaddr,LMC_REG_RANGE , dev->name); +if (!request_region (ioaddr,LMC_REG_RANGE , dev->name)) { +printk (KERN_WARNING "io port %3lX is busy" , ioaddr); +return NULL ; + } sc->lmc_cardtype =LMC_CARDTYPE_UNKNOWN ; sc->lmc_timing =LMC_CTL_CLOCK_SOURCE_EXT ;
@@ -1060,8 +1070,7 @@* Fix the two variables * */ -if (!(check_region (pci_ioaddr,LMC_REG_RANGE )) && - (vendor ==CORRECT_VENDOR_ID ) && +if ((vendor ==CORRECT_VENDOR_ID ) && (device ==CORRECT_DEV_ID ) && ((subvendor ==PCI_VENDOR_LMC ) || (subdevice ==PCI_VENDOR_LMC ))){struct net_device *cur, *prev =NULL ;