unbalanced exit path in Forerunner HE he_init_one()
Subsystems (6)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 unbalanced exit path in Forerunner HE he_init_one()
- pci_enable_device() isn't balanced on error path;
- result of atm_dev_register() is lost if he_dev allocation fails.
drivers/atm/he.c | 40 +++++++++++++++++++++++++++++----------- 1 files changed, 29 insertions(+), 11 deletions(-)
drivers/atm/he.c renamed from drivers/atm/he.c~drivers-atm-he-chas +29 -11
@@ -354,29 +354,36 @@ he_init_one(struct pci_dev *pci_dev, con {struct atm_dev *atm_dev;struct he_dev *he_dev; +int ret = -EIO ;printk (KERN_INFO "he: %s\n" , version);#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,3 ,43 ) -if (pci_enable_device (pci_dev))return -EIO ; +if (pci_enable_device (pci_dev)) + goto out;#endif if (pci_set_dma_mask (pci_dev,HE_DMA_MASK ) !=0 ) {printk (KERN_WARNING "he: no suitable dma available\n" ); -return -EIO ; + goto out_disable; } atm_dev =atm_dev_register (DEV_LABEL , &he_ops,-1 ,0 ); -if (!atm_dev)return -ENODEV ; +if (!atm_dev) { + ret = -ENODEV ; + goto out_disable; + }#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,4 ,3 )pci_set_drvdata (pci_dev, atm_dev);#else pci_dev->driver_data = atm_dev;#endif - he_dev = (struct he_dev *)kmalloc (sizeof (struct he_dev ), -GFP_KERNEL ); -if (!he_dev)return -ENOMEM ; + he_dev = (struct he_dev *)kmalloc (sizeof (*he_dev),GFP_KERNEL ); +if (!he_dev) { + ret = -ENOMEM ; + goto out_deregister; + }memset (he_dev,0 ,sizeof (struct he_dev )); he_dev->pci_dev = pci_dev;
@@ -385,16 +392,27 @@ he_init_one(struct pci_dev *pci_dev, conHE_DEV (atm_dev) = he_dev; he_dev->number = atm_dev->number;/* was devs */ if (he_start (atm_dev)) { -atm_dev_deregister (atm_dev); -he_stop (he_dev); -kfree (he_dev); -return -ENODEV ; + ret = -ENODEV ; + goto out_stop; } he_dev->next =NULL ;if (he_devs) he_dev->next = he_devs; he_devs = he_dev; -return 0 ; + ret =0 ; +out: +return ret; + +out_stop: +he_stop (he_dev); +kfree (he_dev); +out_deregister: +atm_dev_deregister (atm_dev); +out_disable: +#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,3 ,43 ) +pci_disable_device (pci_dev); +#endif + goto out; }static void __devexit
drivers/atm/he.c he_init_one(struct pci_dev *pci_dev, con · line 364
Quoted 5 lines · Francois Romieu
#if
LINUX_VERSION_CODE>KERNEL_VERSION(2,3,43)drivers/atm/he.c renamed from drivers/atm/he.c~drivers-atm-he-chas +2
-1-if (pci_enable_device (pci_dev))return -EIO ; +if (pci_enable_device (pci_dev)) + goto out;#endif
Btw, this driver badly wants a cleanup of the ifdef mess. Look at some network drivers on how to support older kenrels nicely wiþh not much more than a compat layer header instead of spreading ifdefs all over the place. Who reviewed this driver before inclusion?
Christoph Hellwig <[email protected]> :
[...]
Quoted 1 line
Btw, this driver badly wants a cleanup of the ifdef mess. Look
It may happen some day in the future.
[...]
Quoted 2 lines
ifdefs all over the place. Who reviewed this driver before inclusion?
Happy relaxed people :o)
--
Ueimor
From: Christoph Hellwig <[email protected]>
Date: Thu, 8 May 2003 06:06:40 +0100
Who reviewed this driver before inclusion?
This one goes all the way back to when Werner still maintained the ATM layer, and frankly back then we didn't give a crap about these issues as much as we do now.
Quoted 3 lines · David S. Miller
This one goes all the way back to when Werner still maintained the ATM layer, and frankly back then we didn't give a crap about these issues as much as we do now.
Actually, the HE driver is a more recent addition. But I have to admit that my own drivers have more #ifdefs than really necessary, too. Also, I saw my role more as that of an integrator than a reviewer, so I let people do whatever they liked in their drivers.
Ah, the good old times, when everything was better ;-)
- Werner
Signature · 3 lines
_________________________________________________________________________ / Werner Almesberger, Buenos Aires, Argentina [email protected] / /_http://www.almesberger.net/____________________________________________/
this patch should make the he code a bit more palatable. it drops any of the < 2.3 support left over the good 'ole days and puts all the compatibility code in a single place.
drivers/atm/he.c renamed from drivers/atm/he.c.001 +22 -188
@@ -96,16 +96,30 @@#define USE_TPD_POOL /* #undef CONFIG_ATM_HE_USE_SUNI */ -/* 2.2 kernel support */ +/* compatibility */ -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,43 ) -#define dev_kfree_skb_irq (skb) dev_kfree_skb(skb) -#define dev_kfree_skb_any (skb) dev_kfree_skb(skb) -#undef USE_TASKLET +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,5 ,69 ) +typedef void irqreturn_t ; +#define IRQ_NONE +#define IRQ_HANDLED +#define IRQ_RETVAL (x)#endif -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,2 ,18 ) -#define set_current_state (x) current->state = (x); +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,4 ,9 ) +#define __devexit_p (func) func +#endif + +#ifndef MODULE_LICENSE +#define MODULE_LICENSE (x) +#endif + +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,4 ,3 ) +#define pci_set_drvdata (pci_dev, data) (pci_dev)->driver_data = (data) +#define pci_get_drvdata (pci_dev) (pci_dev)->driver_data +#endif + +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,5 ,44 ) +#define pci_pool_create (a, b, c, d, e) pci_pool_create(a, b, c, d, e, SLAB_KERNEL)#endif #include "he.h"
@@ -135,11 +149,7 @@static int he_send (struct atm_vcc *vcc,struct sk_buff *skb);static int he_sg_send (struct atm_vcc *vcc,unsigned long start,unsigned long size);static int he_ioctl (struct atm_dev *dev,unsigned int cmd,void *arg); -#if LINUX_VERSION_CODE >=KERNEL_VERSION (2 ,5 ,69 )static irqreturn_t he_irq_handler (int irq,void *dev_id,struct pt_regs *regs); -#else -static void he_irq_handler (int irq,void *dev_id,struct pt_regs *regs); -#endif static void he_tasklet (unsigned long data);static int he_proc_read (struct atm_dev *dev,loff_t *pos,char *page);static int he_start (struct atm_dev *dev);
@@ -357,9 +367,7 @@printk (KERN_INFO "he: %s\n" , version); -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,3 ,43 )if (pci_enable_device (pci_dev))return -EIO ; -#endif if (pci_set_dma_mask (pci_dev,HE_DMA_MASK ) !=0 ) {printk (KERN_WARNING "he: no suitable dma available\n" );
@@ -368,11 +376,7 @@ atm_dev =atm_dev_register (DEV_LABEL , &he_ops,-1 ,0 );if (!atm_dev)return -ENODEV ; -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,4 ,3 )pci_set_drvdata (pci_dev, atm_dev); -#else - pci_dev->driver_data = atm_dev; -#endif he_dev = (struct he_dev *)kmalloc (sizeof (struct he_dev ),GFP_KERNEL );
@@ -403,11 +407,7 @@struct atm_dev *atm_dev;struct he_dev *he_dev; -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,4 ,3 ) atm_dev =pci_get_drvdata (pci_dev); -#else - atm_dev = pci_dev->driver_data; -#endif he_dev =HE_DEV (atm_dev);/* need to remove from he_devs */
@@ -416,11 +416,7 @@atm_dev_deregister (atm_dev);kfree (he_dev); -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,4 ,3 )pci_set_drvdata (pci_dev,NULL ); -#else - pci_dev->driver_data =NULL ; -#endif }
@@ -783,13 +779,8 @@ #ifdef USE_RBPS /* small buffer pool */ #ifdef USE_RBPS_POOL -#if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,44) - he_dev->rbps_pool = pci_pool_create("rbps", he_dev->pci_dev, - CONFIG_RBPS_BUFSIZE, 8, 0, SLAB_KERNEL); -#else he_dev->rbps_pool = pci_pool_create("rbps", he_dev->pci_dev, CONFIG_RBPS_BUFSIZE, 8, 0); -#endif if (he_dev->rbps_pool == NULL) { hprintk("unable to create rbps pages\n");
@@ -855,13 +846,8 @@/* large buffer pool */ #ifdef USE_RBPL_POOL -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,5 ,44 ) - he_dev->rbpl_pool =pci_pool_create ("rbpl" , he_dev->pci_dev, -CONFIG_RBPL_BUFSIZE ,8 ,0 ,SLAB_KERNEL ); -#else he_dev->rbpl_pool =pci_pool_create ("rbpl" , he_dev->pci_dev,CONFIG_RBPL_BUFSIZE ,8 ,0 ); -#endif if (he_dev->rbpl_pool ==NULL ) {hprintk ("unable to create rbpl pool\n" );
@@ -1053,11 +1039,7 @@ he_dev =HE_DEV (dev); pci_dev = he_dev->pci_dev; -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,3 ,3 ) he_dev->membase = pci_dev->resource[0 ].start; -#else - he_dev->membase = pci_dev->base_address[0 ] &PCI_BASE_ADDRESS_MEM_MASK ; -#endif HPRINTK ("membase = 0x%lx irq = %d.\n" , he_dev->membase, pci_dev->irq);/*
@@ -1519,13 +1501,8 @@he_init_tpdrq (he_dev);#ifdef USE_TPD_POOL -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,5 ,44 ) - he_dev->tpd_pool =pci_pool_create ("tpd" , he_dev->pci_dev, -sizeof (struct he_tpd ),TPD_ALIGNMENT ,0 ,SLAB_KERNEL ); -#else he_dev->tpd_pool =pci_pool_create ("tpd" , he_dev->pci_dev,sizeof (struct he_tpd ),TPD_ALIGNMENT ,0 ); -#endif if (he_dev->tpd_pool ==NULL ) {hprintk ("unable to create tpd pci_pool\n" );
@@ -1919,11 +1896,7 @@if (RBRQ_HBUF_ERR (he_dev->rbrq_head)) {hprintk ("HBUF_ERR! (cid 0x%x)\n" , cid); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,99 ) - ++vcc->stats->rx_drop; -#else atomic_inc (&vcc->stats->rx_drop); -#endif goto return_host_buffers; }
@@ -1958,27 +1931,12 @@RBRQ_LEN_ERR (he_dev->rbrq_head) ?"LEN_ERR" :"" , vcc->vpi, vcc->vci); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,99 ) - ++vcc->stats->rx_err; -#else atomic_inc (&vcc->stats->rx_err); -#endif goto return_host_buffers; } -#if LINUX_VERSION_CODE >=KERNEL_VERSION (2 ,3 ,15 ) skb =atm_alloc_charge (vcc, he_vcc->pdu_len + rx_skb_reserve,GFP_ATOMIC ); -#else -if (!atm_charge (vcc,atm_pdu2truesize (he_vcc->pdu_len + rx_skb_reserve))) - skb =NULL ; -else - { - skb =alloc_skb (he_vcc->pdu_len + rx_skb_reserve,GFP_ATOMIC ); -if (!skb)atm_return (vcc, -atm_pdu2truesize (he_vcc->pdu_len + rx_skb_reserve)); - } -#endif if (!skb) {HPRINTK ("charge failed (%d.%d)\n" , vcc->vpi, vcc->vci);
@@ -2035,11 +1993,7 @@#endif vcc->push (vcc, skb); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,99 ) - ++vcc->stats->rx; -#else atomic_inc (&vcc->stats->rx); -#endif return_host_buffers: ++pdus_assembled;
@@ -2355,11 +2309,7 @@#endif } -#if LINUX_VERSION_CODE >=KERNEL_VERSION (2 ,5 ,69 )static irqreturn_t -#else -static void -#endifhe_irq_handler (int irq,void *dev_id,struct pt_regs *regs) {unsigned long flags;
@@ -2367,11 +2317,7 @@int handled =0 ;if (he_dev ==NULL ) -#if LINUX_VERSION_CODE >=KERNEL_VERSION (2 ,5 ,69 )return IRQ_NONE ; -#else -return ; -#endif HE_SPIN_LOCK (he_dev, flags);
@@ -2406,11 +2352,7 @@#endif }HE_SPIN_UNLOCK (he_dev, flags); -#if LINUX_VERSION_CODE >=KERNEL_VERSION (2 ,5 ,69 )return IRQ_RETVAL (handled); -#else -return ; -#endif }
@@ -2454,11 +2396,7 @@ tpd->vcc->pop (tpd->vcc, tpd->skb); else dev_kfree_skb_any(tpd->skb); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,99 ) - ++tpd->vcc->stats->tx_err; -#else atomic_inc (&tpd->vcc->stats->tx_err); -#endif }#ifdef USE_TPD_POOL pci_pool_free (he_dev->tpd_pool, tpd,TPD_ADDR (tpd->status));
@@ -2509,11 +2447,7 @@HPRINTK ("open vcc %p %d.%d\n" , vcc, vpi, vci); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,1 ) - vcc->flags |=ATM_VF_ADDR ; -#else set_bit (ATM_VF_ADDR , &vcc->flags); -#endif cid =he_mkcid (he_dev, vpi, vci);
@@ -2528,13 +2462,8 @@ he_vcc->pdu_len =0 ; he_vcc->rc_index =-1 ; -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,1 ) -init_waitqueue (&he_vcc->rx_waitq); -init_waitqueue (&he_vcc->tx_waitq); -#else init_waitqueue_head (&he_vcc->rx_waitq);init_waitqueue_head (&he_vcc->tx_waitq); -#endif HE_VCC (vcc) = he_vcc;
@@ -2729,20 +2658,10 @@if (err) {if (he_vcc)kfree (he_vcc); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,1 ) - vcc->flags &= ~ATM_VF_ADDR ; -#else clear_bit (ATM_VF_ADDR , &vcc->flags); -#endif }else - { -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,1 ) - vcc->flags |=ATM_VF_READY ; -#else set_bit (ATM_VF_READY , &vcc->flags); -#endif - }return err; }
@@ -2751,11 +2670,7 @@ he_close(struct atm_vcc *vcc) {unsigned long flags; -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,3 ,1 )DECLARE_WAITQUEUE (wait, current); -#else -struct wait_queue wait = { current,NULL }; -#endif struct he_dev *he_dev =HE_DEV (vcc->dev);struct he_tpd *tpd;unsigned cid;
@@ -2765,11 +2680,7 @@HPRINTK ("close vcc %p %d.%d\n" , vcc, vcc->vpi, vcc->vci); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,1 ) - vcc->flags &= ~ATM_VF_READY ; -#else clear_bit (ATM_VF_READY , &vcc->flags); -#endif cid =he_mkcid (he_dev, vcc->vpi, vcc->vci);if (vcc->qos.rxtp.traffic_class !=ATM_NONE )
@@ -2929,11 +2840,7 @@kfree (he_vcc); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,1 ) - vcc->flags &= ~ATM_VF_ADDR ; -#else clear_bit (ATM_VF_ADDR , &vcc->flags); -#endif } static int
@@ -2969,11 +2876,7 @@ vcc->pop (vcc, skb); elsedev_kfree_skb_any (skb ); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,99 ) - ++vcc->stats->tx_err; -#else atomic_inc (&vcc->stats->tx_err); -#endif return -EINVAL ; }
@@ -2985,11 +2888,7 @@ vcc->pop (vcc, skb); elsedev_kfree_skb_any (skb ); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,99 ) - ++vcc->stats->tx_err; -#else atomic_inc (&vcc->stats->tx_err); -#endif return -EINVAL; }#endif
@@ -3002,11 +2901,7 @@ vcc->pop (vcc, skb); elsedev_kfree_skb_any (skb ); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,99 ) - ++vcc->stats->tx_err; -#else atomic_inc (&vcc->stats->tx_err); -#endif HE_SPIN_UNLOCK (he_dev, flags);return -ENOMEM ; }
@@ -3051,12 +2946,8 @@ vcc->pop (vcc, skb); elsedev_kfree_skb_any (skb ); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,99 ) - ++vcc->stats->tx_err; -#else atomic_inc (&vcc->stats->tx_err);HE_SPIN_UNLOCK (he_dev, flags); -#endif return -ENOMEM ; } tpd->status |=TPD_USERCELL ;
@@ -3086,11 +2977,7 @@__enqueue_tpd (he_dev, tpd, cid);HE_SPIN_UNLOCK (he_dev, flags); -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,99 ) - ++vcc->stats->tx; -#else atomic_inc (&vcc->stats->tx); -#endif return 0 ; }
@@ -3316,6 +3203,7 @@return (byte_read); } +MODULE_LICENSE ("GPL" );MODULE_AUTHOR ("chas williams <[email protected]>" );MODULE_DESCRIPTION ("ForeRunnerHE ATM Adapter driver" );MODULE_PARM (disable64,"h" );
@@ -3331,7 +3219,6 @@MODULE_PARM (sdh,"i" );MODULE_PARM_DESC (sdh,"use SDH framing (default 0)" ); -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,3 ,1 )static struct pci_device_id he_pci_tbl[]__devinitdata = { {PCI_VENDOR_ID_FORE ,PCI_DEVICE_ID_FORE_HE ,PCI_ANY_ID ,PCI_ANY_ID ,0 ,0 ,0 },
@@ -3341,11 +3228,7 @@static struct pci_driver he_driver = { .name ="he" , .probe = he_init_one, -#if LINUX_VERSION_CODE > KERNEL_VERSION(2,4,9) .remove =__devexit_p (he_remove_one), -#else - .remove = he_remove_one, -#endif .id_table = he_pci_tbl, };
@@ -3361,52 +3244,3 @@module_init (he_init);module_exit (he_cleanup); -#else -static int __init -he_init () -{ -if (!pci_present ()) -return -EIO ; - -#ifdef CONFIG_ATM_HE_USE_SUNI_MODULE -/* request_module("suni"); */ -#endif - - pci_dev =NULL ; -while ((pci_dev =pci_find_device (PCI_VENDOR_ID_FORE , -PCI_DEVICE_ID_FORE_HE , pci_dev)) !=NULL ) -if (he_init_one (pci_dev,NULL ) ==0 ) - ++ndevs; - -return (ndevs ?0 : -ENODEV ); -} - -static void __devexit -he_cleanup (void ) -{ -while (he_devs) - { -struct he_dev *next = he_devs->next; -he_stop (he_devs); -atm_dev_deregister (he_devs->atm_dev); -kfree (he_devs); - - he_devs = next; - } - -} - -int init_module (void ) -{ -return he_init (); -} - -void cleanup_module (void ) -{ -he_cleanup (); -} -#endif - -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,4 ,7 ) -MODULE_LICENSE ("GPL" ); -#endif
drivers/atm/he.h renamed from drivers/atm/he.h.001 +0 -5
@@ -371,13 +371,8 @@int rc_index; -#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,1 ) -struct wait_queue *rx_waitq; -struct wait_queue *tx_waitq; -#else wait_queue_head_t rx_waitq;wait_queue_head_t tx_waitq; -#endif };#define HE_VCC (vcc) ((struct he_vcc *)(vcc->dev_data))
David S. Miller
Applied From: chas williams <[email protected]>
Date: Thu, 08 May 2003 20:58:23 -0400
this patch should make the he code a bit more palatable. it drops any of the < 2.3 support left over the good 'ole days and puts all the compatibility code in a single place.
Applied, thanks.
Update:
- pci_enable_device() balanced on error path in he_init_one();
- return of atm_dev_register() isn't lost any more if he_dev allocation fails in he_init_one();
- pci_disable_device() added to he_disable_one();
drivers/atm/he.c | 43 ++++++++++++++++++++++++++++++------------- 1 files changed, 30 insertions(+), 13 deletions(-)
drivers/atm/he.c renamed from drivers/atm/he.c~drivers-atm-he-chas +30 -13
@@ -364,23 +364,29 @@ he_init_one(struct pci_dev *pci_dev, con {struct atm_dev *atm_dev;struct he_dev *he_dev; +int ret = -EIO ;printk (KERN_INFO "he: %s\n" , version); -if (pci_enable_device (pci_dev))return -EIO ; -if (pci_set_dma_mask (pci_dev,HE_DMA_MASK ) !=0 ) - { +if (pci_enable_device (pci_dev)) + goto out; +if (pci_set_dma_mask (pci_dev,HE_DMA_MASK ) !=0 ) {printk (KERN_WARNING "he: no suitable dma available\n" ); -return -EIO ; + goto out_disable; } atm_dev =atm_dev_register (DEV_LABEL , &he_ops,-1 ,0 ); -if (!atm_dev)return -ENODEV ; +if (!atm_dev) { + ret = -ENODEV ; + goto out_disable; + }pci_set_drvdata (pci_dev, atm_dev); - he_dev = (struct he_dev *)kmalloc (sizeof (struct he_dev ), -GFP_KERNEL ); -if (!he_dev)return -ENOMEM ; + he_dev = (struct he_dev *)kmalloc (sizeof (struct he_dev ),GFP_KERNEL ); +if (!he_dev) { + ret = -ENOMEM ; + goto out_deregister; + }memset (he_dev,0 ,sizeof (struct he_dev )); he_dev->pci_dev = pci_dev;
@@ -389,16 +395,26 @@ he_init_one(struct pci_dev *pci_dev, conHE_DEV (atm_dev) = he_dev; he_dev->number = atm_dev->number;/* was devs */ if (he_start (atm_dev)) { -atm_dev_deregister (atm_dev); -he_stop (he_dev); -kfree (he_dev); -return -ENODEV ; + ret = -ENODEV ; + goto out_stop; } he_dev->next =NULL ;if (he_devs) he_dev->next = he_devs; he_devs = he_dev; -return 0 ; + ret =0 ; +out: +return ret; + +out_stop: +he_stop (he_dev); +kfree (he_dev); +out_deregister: +pci_set_drvdata (pci_dev,NULL ); +atm_dev_deregister (atm_dev); +out_disable: +pci_disable_device (pci_dev); + goto out; }static void __devexit
@@ -417,6 +433,7 @@ he_remove_one (struct pci_dev *pci_dev)kfree (he_dev);pci_set_drvdata (pci_dev,NULL ); +pci_disable_device (pci_dev); }
Re: [ATM] [UPDATE] unbalanced exit path in Forerunner HE he_init_one() (and an iphase patch too!)
Quoted 4 lines
-
pci_enable_device()balanced on error path inhe_init_one();
- return ofatm_dev_register()isn't lost any more ifhe_devallocation fails inhe_init_one();
-pci_disable_device()added tohe_disable_one();
i hope you dont mind, but i really dislike the goto. one is plenty. attached is a cleanup for the iphase (i made a small addition -- i believe the MEMDUMP_XXXXXXREG cases are also guilty of dereferencing).
[ATM]: unbalanced exit path in Forerunner HE he_init_one(). thanks to
Francois Romieu <[email protected]>
drivers/atm/he.c renamed from drivers/atm/he.c.002 +24 -12
@@ -362,43 +362,54 @@static int __devinit he_init_one (struct pci_dev *pci_dev,const struct pci_device_id *pci_ent) { -struct atm_dev *atm_dev; -struct he_dev *he_dev; +struct atm_dev *atm_dev =NULL ; +struct he_dev *he_dev =NULL ; +int err =0 ;printk (KERN_INFO "he: %s\n" , version);if (pci_enable_device (pci_dev))return -EIO ; -if (pci_set_dma_mask (pci_dev,HE_DMA_MASK ) !=0 ) - { +if (pci_set_dma_mask (pci_dev,HE_DMA_MASK ) !=0 ) {printk (KERN_WARNING "he: no suitable dma available\n" ); -return -EIO ; + err = -EIO ; + goto init_one_failure; } atm_dev =atm_dev_register (DEV_LABEL , &he_ops,-1 ,0 ); -if (!atm_dev)return -ENODEV ; +if (!atm_dev) { + err = -ENODEV ; + goto init_one_failure; + }pci_set_drvdata (pci_dev, atm_dev); he_dev = (struct he_dev *)kmalloc (sizeof (struct he_dev ),GFP_KERNEL ); -if (!he_dev)return -ENOMEM ; +if (!he_dev) { + err = -ENOMEM ; + goto init_one_failure; + }memset (he_dev,0 ,sizeof (struct he_dev )); he_dev->pci_dev = pci_dev; he_dev->atm_dev = atm_dev; he_dev->atm_dev->dev_data = he_dev;HE_DEV (atm_dev) = he_dev; - he_dev->number = atm_dev->number;/* was devs */ + he_dev->number = atm_dev->number;if (he_start (atm_dev)) { -atm_dev_deregister (atm_dev);he_stop (he_dev); -kfree (he_dev); -return -ENODEV ; + err = -ENODEV ; + goto init_one_failure; } he_dev->next =NULL ;if (he_devs) he_dev->next = he_devs; he_devs = he_dev; -return 0 ; + +init_one_failure: +if (atm_dev)atm_dev_deregister (atm_dev); +if (he_dev)kfree (he_dev); +pci_disable_device (pci_dev); +return err; }static void __devexit
@@ -417,6 +428,7 @@kfree (he_dev);pci_set_drvdata (pci_dev,NULL ); +pci_disable_device (pci_dev); }
[ATM]: Sneak variant of "ioremap() return dereferencing". thanks to
Francois Romieu <[email protected]>
drivers/atm/iphase.c renamed from drivers/atm/iphase.c.002 +4 -4
@@ -2774,7 +2774,7 @@if (!capable (CAP_NET_ADMIN ))return -EPERM ; tmps = (u16 *)ia_cmds.buf;for (i=0 ; i<0x80 ; i+=2 , tmps++) -if (put_user (*(u16 *)(iadev->seg_reg+i), tmps))return -EFAULT ; +if (put_user ((u16)(readl (iadev->seg_reg+i) &0xffff ), tmps))return -EFAULT ; ia_cmds.status =0 ; ia_cmds.len =0x80 ;break ;
@@ -2782,7 +2782,7 @@if (!capable (CAP_NET_ADMIN ))return -EPERM ; tmps = (u16 *)ia_cmds.buf;for (i=0 ; i<0x80 ; i+=2 , tmps++) -if (put_user (*(u16 *)(iadev->reass_reg+i), tmps))return -EFAULT ; +if (put_user ((u16)(readl (iadev->reass_reg+i) &0xffff ), tmps))return -EFAULT ; ia_cmds.status =0 ; ia_cmds.len =0x80 ;break ;
@@ -2799,10 +2799,10 @@ rfL = ®s_local->rfredn;/* Copy real rfred registers into the local copy */ for (i=0 ; i<(sizeof (rfredn_t))/4 ; i++) - ((u_int *)rfL)[i] = ((u_int *)iadev->reass_reg)[i] &0xffff ; + ((u_int *)rfL)[i] =readl (iadev->reass_reg + i) &0xffff ;/* Copy real ffred registers into the local copy */ for (i=0 ; i<(sizeof (ffredn_t))/4 ; i++) - ((u_int *)ffL)[i] = ((u_int *)iadev->seg_reg)[i] &0xffff ; + ((u_int *)ffL)[i] =readl (iadev->seg_reg + i) &0xffff ;if (copy_to_user (ia_cmds.buf, regs_local,sizeof (ia_regs_t))) {kfree (regs_local);
Re: [ATM] [UPDATE] unbalanced exit path in Forerunner HE he_init_one() (and an iphase patch too!)
drivers/atm/he.c line 412
Quoted 6 lines · chas williams
drivers/atm/he.c renamed from drivers/atm/he.c.002 +6
-0+ +init_one_failure: +if (atm_dev)atm_dev_deregister (atm_dev); +if (he_dev)kfree (he_dev); +pci_disable_device (pci_dev); +return err;
kfree(NULL) if perfectly fine. Also please untangle all this if statements to two separate lines.
Re: [ATM] [UPDATE] unbalanced exit path in Forerunner HE he_init_one() (and an iphase patch too!)
Quoted 8 lines · In message <[email protected]>,Christoph Hellwig writes:
drivers/atm/he.c line 412
Quoted 5 lines
drivers/atm/he.c renamed from drivers/atm/he.c.002 +5
-0+init_one_failure: +if (atm_dev)atm_dev_deregister (atm_dev); +if (he_dev)kfree (he_dev); +pci_disable_device (pci_dev); +return err;kfree(NULL) if perfectly fine. Also please untangle all this if statements to two separate lines.
but its ok for usb drivers?
class/usb-midi.c: if ( u ) kfree(u); class/usblp.c: if (usblp->statusbuf) kfree(usblp->statusbuf); class/usblp.c: if (usblp->device_id_string) kfree(usblp->device_id_string);
image/mdc800.c:#define try_free_mem(A) if (A != 0) { kfree (A); A=0; }
Re: [ATM] [UPDATE] unbalanced exit path in Forerunner HE he_init_one() (and an iphase patch too!)
From: chas williams <[email protected]>
Date: Sat, 10 May 2003 09:52:49 -0400
Quoted 2 lines · In message <[email protected]>,Christoph Hellwig writes:
kfree(NULL) if perfectly fine. Also please untangle all this if statements to two separate lines.
but its ok for usb drivers?
Bad ugly code in one area of the kernel is not an excuse for it in other areas :-)
Really, putting a statement on the same line as the if test controlling it's execution is very unreadable. Most people's eyes expect to see:
if (test)
IF_CODE_BLOCK
Whether it be one or more lines. I cannot count how many locking bugs escaped for a long time because of utter shit like this:
if (test) return 0;
Re: [ATM] [UPDATE] unbalanced exit path in Forerunner HE he_init_one() (and an iphase patch too!)
Quoted 3 lines · In message <[email protected]>,"David S. Miller" writes:
but its ok for usb drivers?
Bad ugly code in one area of the kernel is not an excuse for it in other areas :-)
perhaps someone should mention this to them? i believe usb is under active development.
Quoted 4 lines
test controlling it's execution is very unreadable. Most people's eyes expect to see:
if (test)
IF_CODE_BLOCK
sorry, i am an old fortran programmer. i am always trying to put a continutation punch in field 6 on multiple lines. (btw, this doesnt mean i like goto.)
here are the two patches again:
[ATM]: unbalanced exit path in Forerunner HE he_init_one(). thanks to
Francois Romieu <[email protected]>
drivers/atm/he.c renamed from drivers/atm/he.c.002 +26 -12
@@ -362,43 +362,56 @@static int __devinit he_init_one (struct pci_dev *pci_dev,const struct pci_device_id *pci_ent) { -struct atm_dev *atm_dev; -struct he_dev *he_dev; +struct atm_dev *atm_dev =NULL ; +struct he_dev *he_dev =NULL ; +int err =0 ;printk (KERN_INFO "he: %s\n" , version);if (pci_enable_device (pci_dev))return -EIO ; -if (pci_set_dma_mask (pci_dev,HE_DMA_MASK ) !=0 ) - { +if (pci_set_dma_mask (pci_dev,HE_DMA_MASK ) !=0 ) {printk (KERN_WARNING "he: no suitable dma available\n" ); -return -EIO ; + err = -EIO ; + goto init_one_failure; } atm_dev =atm_dev_register (DEV_LABEL , &he_ops,-1 ,0 ); -if (!atm_dev)return -ENODEV ; +if (!atm_dev) { + err = -ENODEV ; + goto init_one_failure; + }pci_set_drvdata (pci_dev, atm_dev); he_dev = (struct he_dev *)kmalloc (sizeof (struct he_dev ),GFP_KERNEL ); -if (!he_dev)return -ENOMEM ; +if (!he_dev) { + err = -ENOMEM ; + goto init_one_failure; + }memset (he_dev,0 ,sizeof (struct he_dev )); he_dev->pci_dev = pci_dev; he_dev->atm_dev = atm_dev; he_dev->atm_dev->dev_data = he_dev;HE_DEV (atm_dev) = he_dev; - he_dev->number = atm_dev->number;/* was devs */ + he_dev->number = atm_dev->number;if (he_start (atm_dev)) { -atm_dev_deregister (atm_dev);he_stop (he_dev); -kfree (he_dev); -return -ENODEV ; + err = -ENODEV ; + goto init_one_failure; } he_dev->next =NULL ;if (he_devs) he_dev->next = he_devs; he_devs = he_dev; -return 0 ; + +init_one_failure: +if (atm_dev) +atm_dev_deregister (atm_dev); +if (he_dev) +kfree (he_dev); +pci_disable_device (pci_dev); +return err; }static void __devexit
@@ -417,6 +430,7 @@kfree (he_dev);pci_set_drvdata (pci_dev,NULL ); +pci_disable_device (pci_dev); }
[ATM]: [ATM]: Sneak variant of "ioremap() return dereferencing". thanks to
Francois Romieu <[email protected]>
drivers/atm/iphase.c renamed from drivers/atm/iphase.c.002 +4 -4
@@ -2774,7 +2774,7 @@if (!capable (CAP_NET_ADMIN ))return -EPERM ; tmps = (u16 *)ia_cmds.buf;for (i=0 ; i<0x80 ; i+=2 , tmps++) -if (put_user (*(u16 *)(iadev->seg_reg+i), tmps))return -EFAULT ; +if (put_user ((u16)(readl (iadev->seg_reg+i) &0xffff ), tmps))return -EFAULT ; ia_cmds.status =0 ; ia_cmds.len =0x80 ;break ;
@@ -2782,7 +2782,7 @@if (!capable (CAP_NET_ADMIN ))return -EPERM ; tmps = (u16 *)ia_cmds.buf;for (i=0 ; i<0x80 ; i+=2 , tmps++) -if (put_user (*(u16 *)(iadev->reass_reg+i), tmps))return -EFAULT ; +if (put_user ((u16)(readl (iadev->reass_reg+i) &0xffff ), tmps))return -EFAULT ; ia_cmds.status =0 ; ia_cmds.len =0x80 ;break ;
@@ -2799,10 +2799,10 @@ rfL = ®s_local->rfredn;/* Copy real rfred registers into the local copy */ for (i=0 ; i<(sizeof (rfredn_t))/4 ; i++) - ((u_int *)rfL)[i] = ((u_int *)iadev->reass_reg)[i] &0xffff ; + ((u_int *)rfL)[i] =readl (iadev->reass_reg + i) &0xffff ;/* Copy real ffred registers into the local copy */ for (i=0 ; i<(sizeof (ffredn_t))/4 ; i++) - ((u_int *)ffL)[i] = ((u_int *)iadev->seg_reg)[i] &0xffff ; + ((u_int *)ffL)[i] =readl (iadev->seg_reg + i) &0xffff ;if (copy_to_user (ia_cmds.buf, regs_local,sizeof (ia_regs_t))) {kfree (regs_local);
Re: [ATM] [UPDATE] unbalanced exit path in Forerunner HE he_init_one() (and an iphase patch too!)
From: chas williams <[email protected]>
Date: Sat, 10 May 2003 11:18:32 -0400
Quoted 3 lines · In message <[email protected]>,"David S. Miller" writes:
but its ok for usb drivers?
Bad ugly code in one area of the kernel is not an excuse for it in other areas :-)
perhaps someone should mention this to them? i believe usb is under active development.
We just did. :-)
And a coding style document lives in the tree in case you weren't aware of this. linux/Documentation/CodingStlye
[ATM]: unbalanced exit path in Forerunner HE he_init_one(). thanks to
Francois Romieu <[email protected]>
I'll apply this, thanks.
Re: [ATM] [UPDATE] unbalanced exit path in Forerunner HE he_init_one() (and an iphase patch too!)
In reply to chas williamsQuoted 4 lines · chas williams
class/usb-midi.c: if ( u ) kfree(u); class/usblp.c: if (usblp->statusbuf) kfree(usblp->statusbuf); class/usblp.c: if (usblp->device_id_string) kfree(usblp->device_id_string);image/mdc800.c:#define
try_free_mem(A)if (A != 0) { kfree (A); A=0; }
It's wrong there, too. usb is not exactly the right example if you look for code that matches the style guidelines..
Re: [ATM] [UPDATE] unbalanced exit path in Forerunner HE he_init_one() (and an iphase patch too!)
In reply to chas williamsQuoted 11 lines · chas williams
Quoted 8 lines · In message <[email protected]>,Christoph Hellwig writes:
drivers/atm/he.c line 412
Quoted 5 lines
drivers/atm/he.c renamed from drivers/atm/he.c.002 +5
-0+init_one_failure: +if (atm_dev)atm_dev_deregister (atm_dev); +if (he_dev)kfree (he_dev); +pci_disable_device (pci_dev); +return err;kfree(NULL) if perfectly fine. Also please untangle all this if statements to two separate lines.
but its ok for usb drivers?
Not at all. I'll gladly take patches to fix this crud up.
thanks,
greg k-h
PATCH 1/1 Crud-- (was: Re: [ATM] [UPDATE] unbalanced usw)
[Cc: list modified]
Greg KH <[email protected]> :
[...]
Quoted 1 line
Not at all. I'll gladly take patches to fix this crud up.
What about the following patch ?
usb/host/ehci-q.c | 0 drivers/usb/class/cdc-acm.c | 42 +++-- drivers/usb/class/usb-midi.c | 62 +++++-- drivers/usb/class/usblp.c | 28 ++- drivers/usb/core/hcd-pci.c | 3 drivers/usb/host/ohci-sa1111.c | 6 drivers/usb/input/hid-core.c | 32 ++- drivers/usb/input/hid-input.c | 3 drivers/usb/input/hid-lgff.c | 6 drivers/usb/input/hid-tmff.c | 3 drivers/usb/input/hiddev.c | 27 ++- drivers/usb/input/pid.c | 6 drivers/usb/input/usbkbd.c | 3 drivers/usb/media/ov511.c | 332 +++++++++++++++++++++++++---------------- drivers/usb/media/se401.c | 19 +- drivers/usb/media/vicam.c | 3 drivers/usb/misc/auerswald.c | 33 ++-- drivers/usb/net/catc.c | 12 - drivers/usb/net/kaweth.c | 3 drivers/usb/serial/belkin_sa.c | 7 drivers/usb/serial/console.c | 9 - drivers/usb/serial/cyberjack.c | 9 - drivers/usb/serial/io_ti.c | 3 drivers/usb/serial/ir-usb.c | 3 24 files changed, 417 insertions(+), 237 deletions(-)
drivers/usb/class/usb-midi.c renamed from drivers/usb/class/usb-midi.c~codingstyle-patrol-usb +41 -21
@@ -1355,8 +1355,10 @@ static struct usb_midi_device *parse_des next = p2 + p2[0 ]; length -= p2[0 ]; -if (p2[0 ] <2 )break ; -if (p2[1 ] !=USB_DT_CS_INTERFACE )break ; +if (p2[0 ] <2 ) +break ; + if (p2[1] != USB_DT_CS_INTERFACE)r +break ;if (p2[2 ] ==MIDI_IN_JACK && p2[0 ] >=6 ) { jack = p2[4 ];#ifdef HAVE_JACK_STRINGS
@@ -1366,7 +1368,8 @@ static struct usb_midi_device *parse_des jack, (p2[3] == EMBEDDED_JACK)?"EMBEDDED":"EXTERNAL" ); }else if ( p2[2 ] ==MIDI_OUT_JACK && p2[0 ] >=6 ) { pins = p2[5 ]; -if ( p2[0 ] < (6 +2 * pins) )continue ; +if ( p2[0 ] < (6 +2 * pins) ) +continue ; jack = p2[4 ];#ifdef HAVE_JACK_STRINGS jack2string[jack] = p2[5 +2 * pins];
@@ -1375,9 +1378,11 @@ static struct usb_midi_device *parse_des jack, (p2[3] == EMBEDDED_JACK)?"EMBEDDED":"EXTERNAL", pins ); }else if ( p2[2 ] ==ELEMENT_DESCRIPTOR && p2[0 ] >=10 ) { pins = p2[4 ]; -if ( p2[0 ] < (9 +2 * pins ) )continue ; +if ( p2[0 ] < (9 +2 * pins ) ) +continue ; nbytes = p2[8 +2 * pins ]; -if ( p2[0 ] < (10 +2 * pins + nbytes) )continue ; +if ( p2[0 ] < (10 +2 * pins + nbytes) ) +continue ; longBits =0L ;for ( offset =0 , shift =0 ; offset < nbytes && offset <8 ; offset ++, shift +=8 ) { longBits |= ((long )(p2[9 +2 * pins + offset])) << shift;
@@ -1408,7 +1413,8 @@ static struct usb_midi_device *parse_desif ( p2 && next && ( p2 > next ) ) p2 =0 ; -if ( p1[0 ] <9 || !p2 || p2[0 ] <4 )continue ; +if ( p1[0 ] <9 || !p2 || p2[0 ] <4 ) +continue ;if ( (p1[2 ] &0x80 ) ==0x80 ) {if ( iep <15 ) {
@@ -1417,7 +1423,8 @@ static struct usb_midi_device *parse_des pins =16 ; u->in[iep].endpoint = p1[2 ]; u->in[iep].cableId = (1 << pins ) -1 ; -if ( u->in[iep].cableId ) iep ++; +if ( u->in[iep].cableId ) + iep ++;if ( iep <15 ) { u->in[iep].endpoint =-1 ; u->in[iep].cableId =-1 ;
@@ -1430,7 +1437,8 @@ static struct usb_midi_device *parse_des pins =16 ; u->out[oep].endpoint = p1[2 ]; u->out[oep].cableId = (1 << pins ) -1 ; -if ( u->out[oep].cableId ) oep ++; +if ( u->out[oep].cableId ) + oep ++;if ( oep <15 ) { u->out[oep].endpoint =-1 ; u->out[oep].cableId =-1 ;
@@ -1446,7 +1454,8 @@ static struct usb_midi_device *parse_des next =find_descriptor (buffer, bufSize, p1,USB_DT_ENDPOINT , ifnum, altSetting ); -if ( p1[0 ] <7 )continue ; +if ( p1[0 ] <7 ) +continue ;if ( (p1[2 ] &0x80 ) ==0x80 ) {if ( iep <15 ) {
@@ -1455,7 +1464,8 @@ static struct usb_midi_device *parse_des pins =16 ; u->in[iep].endpoint = p1[2 ]; u->in[iep].cableId = (1 << pins ) -1 ; -if ( u->in[iep].cableId ) iep ++; +if ( u->in[iep].cableId ) + iep ++;if ( iep <15 ) { u->in[iep].endpoint =-1 ; u->in[iep].cableId =-1 ;
@@ -1468,7 +1478,8 @@ static struct usb_midi_device *parse_des pins =16 ; u->out[oep].endpoint = p1[2 ]; u->out[oep].cableId = (1 << pins ) -1 ; -if ( u->out[oep].cableId ) oep ++; +if ( u->out[oep].cableId ) + oep ++;if ( oep <15 ) { u->out[oep].endpoint =-1 ; u->out[oep].cableId =-1 ;
@@ -1486,7 +1497,7 @@ static struct usb_midi_device *parse_desreturn u; error_end: -if ( u )kfree (u); +kfree (u);return NULL ; }
@@ -1501,7 +1512,8 @@ static int on_bits( unsigned short v )int ret=0 ;for ( i=0 ; i<16 ; i++ ) { -if ( v & (1 <<i) ) ret++; +if ( v & (1 <<i) ) + ret++; }return ret;
@@ -1578,7 +1590,8 @@ static int alloc_usb_midi_device( structif ( alt <0 ) { alt =get_alt_setting ( d, u->interface ); } -if ( alt <0 ) {return -ENXIO ; } +if ( alt <0 ) +return -ENXIO ;/* Configure interface */ if (usb_set_interface ( d, u->interface, alt ) <0 ) {
@@ -1596,7 +1609,8 @@ static int alloc_usb_midi_device( struct && u->in[inEndpoints].cableId >= 0 ) { inDevs +=on_bits ((unsigned short )u->in[inEndpoints].cableId); mins[inEndpoints] =alloc_midi_in_endpoint ( d, u->in[inEndpoints].endpoint ); -if ( mins[inEndpoints] ==NULL ) { goto error_end; } +if ( mins[inEndpoints] ==NULL ) + goto error_end; inEndpoints++; }
@@ -1605,7 +1619,8 @@ static int alloc_usb_midi_device( struct && u->out[outEndpoints].cableId >= 0 ) { outDevs +=on_bits ((unsigned short )u->out[outEndpoints].cableId); mouts[outEndpoints] =alloc_midi_out_endpoint ( d, u->out[outEndpoints].endpoint ); -if ( mouts[outEndpoints] ==NULL ) { goto error_end; } +if ( mouts[outEndpoints] ==NULL ) + goto error_end; outEndpoints++; }
@@ -1707,7 +1722,8 @@ static int alloc_usb_midi_device( struct mout = mouts[outEndpoint]; mdevs[i] =allocMidiDev ( s, min, mout, inCableId, outCableId ); -if ( mdevs[i] ==NULL ) { goto error_end; } +if ( mdevs[i] ==NULL ) + goto error_end; }
@@ -1962,11 +1978,15 @@ static int detect_by_hand(struct usb_devreturn -EINVAL ; } -if ( ualt <0 ) { ualt =-1 ; } +if ( ualt <0 ) + ualt =-1 ; -if ( umin <0 || umin >15 ) { umin =0x01 |USB_DIR_IN ; } -if ( umout <0 || umout >15 ) { umout =0x01 ; } -if ( ucable <0 || ucable >15 ) { ucable =0 ; } +if ( umin <0 || umin >15 ) + umin =0x01 |USB_DIR_IN ; +if ( umout <0 || umout >15 ) + umout =0x01 ; +if ( ucable <0 || ucable >15 ) + ucable =0 ; u.deviceName =0 ;/* A flag for alloc_usb_midi_device to get device name from device. */
diff -puN drivers/usb/host/ehci-q.c~codingstyle-patrol-usb drivers/usb/host/ehci-q.c
drivers/usb/media/se401.c renamed from drivers/usb/media/se401.c~codingstyle-patrol-usb +9 -10
@@ -170,7 +170,8 @@ static int se401_read_proc(char *page, c len -= off;if (len < count) { *eof =1 ; -if (len <=0 )return 0 ; +if (len <=0 ) +return 0 ; }else len = count;
@@ -749,7 +750,8 @@ static inline void decode_JangGu_vlc (st } }else {if (vlc_cod==2 ) { -if (!bit) vlc_data=-(1 <<vlc_size)+1 ; +if (!bit) + vlc_data = -(1 <<vlc_size) +1 ; vlc_cod--; } vlc_size--;
@@ -1046,15 +1048,12 @@ static int se401_open(struct inode *inodif (se401->user)return -EBUSY ; - se401->user=1 ; - se401->fbuf=rvmalloc (se401->maxframesize *SE401_NUMFRAMES ); -if (!se401->fbuf) err=-ENOMEM ; - -if (0 != err) { - se401->user =0 ; - }else { + se401->fbuf =rvmalloc (se401->maxframesize *SE401_NUMFRAMES ); +if (se401->fbuf) file->private_data = dev; - } +else + err = -ENOMEM ; + se401->user = !err;return err; }
drivers/usb/serial/ir-usb.c renamed from drivers/usb/serial/ir-usb.c~codingstyle-patrol-usb +2 -1
@@ -461,7 +461,8 @@ static void ir_read_bulk_callback (struc* contains a busy indicator and baud rate change. * See section 5.4.1.2 of the USB IrDA spec. */ -if ((*data &0x0f ) >0 ) ir_baud = *data &0x0f ; +if ((*data &0x0f ) >0 ) + ir_baud = *data &0x0f ; usb_serial_debug_data ( __FILE__,
drivers/usb/serial/cyberjack.c renamed from drivers/usb/serial/cyberjack.c~codingstyle-patrol-usb +6 -3
@@ -284,7 +284,8 @@ static void cyberjack_read_int_callback(struct usb_serial *serial;unsigned char *data = urb->transfer_buffer; -if (port_paranoia_check (port, __FUNCTION__))return ; +if (port_paranoia_check (port, __FUNCTION__)) +return ;dbg ("%s - port %d" , __FUNCTION__, port->number);
@@ -293,7 +294,8 @@ static void cyberjack_read_int_callback(return ; serial = port->serial; -if (serial_paranoia_check (serial, __FUNCTION__))return ; +if (serial_paranoia_check (serial, __FUNCTION__)) +return ;usb_serial_debug_data (__FILE__, __FUNCTION__, urb->actual_length, data);
@@ -372,7 +374,8 @@ static void cyberjack_read_bulk_callback/* Reduce urbs to do by one. */ priv->rdtodo-=urb->actual_length;/* Just to be sure */ -if ( priv->rdtodo<0 ) priv->rdtodo=0 ; +if ( priv->rdtodo<0 ) + priv->rdtodo =0 ;dbg ("%s - rdtodo: %d" , __FUNCTION__, priv->rdtodo);
drivers/usb/net/kaweth.c renamed from drivers/usb/net/kaweth.c~codingstyle-patrol-usb +2 -1
@@ -811,7 +811,8 @@ static void kaweth_async_set_rx_mode(str {__u16 packet_filter_bitmap = kaweth->packet_filter_bitmap; kaweth->packet_filter_bitmap =0 ; -if (packet_filter_bitmap ==0 )return ; +if (packet_filter_bitmap ==0 ) +return ; {int result;
drivers/usb/media/ov511.c renamed from drivers/usb/media/ov511.c~codingstyle-patrol-usb +206 -126
651 lines not on this page · raw message
drivers/usb/media/vicam.c renamed from drivers/usb/media/vicam.c~codingstyle-patrol-usb +2 -1
@@ -1139,7 +1139,8 @@ vicam_create_proc_entry(struct vicam_cam cam->proc_dir =create_proc_entry (name,S_IFDIR , vicam_proc_root); -if ( !cam->proc_dir )return ;// We should probably return an error here +if ( !cam->proc_dir ) +return ;// FIXME: We should probably return an error here ent =create_proc_entry ("shutter" ,S_IFREG |S_IRUGO , cam->proc_dir);
drivers/usb/serial/belkin_sa.c renamed from drivers/usb/serial/belkin_sa.c~codingstyle-patrol-usb +5 -2
@@ -285,10 +285,13 @@ static void belkin_sa_read_int_callback goto exit; } -if (port_paranoia_check (port, __FUNCTION__))return ; +if (port_paranoia_check (port, __FUNCTION__)) +return ; serial = port->serial; -if (serial_paranoia_check (serial, __FUNCTION__))return ; + +if (serial_paranoia_check (serial, __FUNCTION__)) +return ;usb_serial_debug_data (__FILE__, __FUNCTION__, urb->actual_length, data);
drivers/usb/serial/io_ti.c renamed from drivers/usb/serial/io_ti.c~codingstyle-patrol-usb +2 -1
@@ -1553,7 +1553,8 @@ static __u8 MapLineStatus (__u8 ti_lsr)__u8 lsr =0 ;#define MAP_FLAG (flagUmp, flagUart) \ - if (ti_lsr & flagUmp) lsr |= flagUart; + if (ti_lsr & flagUmp) + lsr |= flagUart; MAP_FLAG(UMP_UART_LSR_OV_MASK, LSR_OVER_ERR) /* overrun */MAP_FLAG (UMP_UART_LSR_PE_MASK ,LSR_PAR_ERR )/* parity error */
drivers/usb/serial/console.c renamed from drivers/usb/serial/console.c~codingstyle-patrol-usb +6 -3
@@ -76,9 +76,12 @@ static int __init usb_console_setup(stru s = options;while (*s >='0' && *s <='9' ) s++; -if (*s) parity = *s++; -if (*s) bits = *s++ -'0' ; -if (*s) doflow = (*s++ =='r' ); +if (*s) + parity = *s++; +if (*s) + bits = *s++ -'0' ; +if (*s) + doflow = (*s++ =='r' ); }/* build a cflag setting */
drivers/usb/input/usbkbd.c renamed from drivers/usb/input/usbkbd.c~codingstyle-patrol-usb +2 -1
@@ -137,7 +137,8 @@ int usb_kbd_event(struct input_dev *dev, {struct usb_kbd *kbd = dev->private; -if (type !=EV_LED )return -1 ; +if (type !=EV_LED ) +return -1 ; kbd->newleds = (!!test_bit(LED_KANA, dev->led) << 3) | (!!test_bit(LED_COMPOSE, dev->led) << 3) |
drivers/usb/input/hid-input.c renamed from drivers/usb/input/hid-input.c~codingstyle-patrol-usb +2 -1
@@ -351,7 +351,8 @@ static void hidinput_configure_usage(str usage->code =find_next_zero_bit (bit, max +1 , usage->code); } -if (usage->code > max)return ; +if (usage->code > max) +return ;if (usage->type ==EV_ABS ) {int a = field->logical_minimum;
drivers/usb/input/hiddev.c renamed from drivers/usb/input/hiddev.c~codingstyle-patrol-usb +18 -9
@@ -96,16 +96,19 @@ hiddev_lookup_report(struct hid_device *case HID_REPORT_ID_FIRST : list = report_enum->report_list.next; -if (list == &report_enum->report_list)return NULL ; +if (list == &report_enum->report_list) +return NULL ; rinfo->report_id = ((struct hid_report *) list)->id;break ;case HID_REPORT_ID_NEXT : list = (struct list_head *) report_enum->report_id_hash[rinfo->report_id &HID_REPORT_ID_MASK ]; -if (list ==NULL )return NULL ; +if (list ==NULL ) +return NULL ; list = list->next; -if (list == &report_enum->report_list)return NULL ; +if (list == &report_enum->report_list) +return NULL ; rinfo->report_id = ((struct hid_report *) list)->id;break ;
@@ -311,7 +314,8 @@ static ssize_t hiddev_read(struct file * event_size = ((list->flags &HIDDEV_FLAG_UREF ) !=0 ) ?sizeof (struct hiddev_usage_ref ) :sizeof (struct hiddev_event ); -if (count < event_size)return 0 ; +if (count < event_size) +return 0 ;while (retval ==0 ) {if (list->head == list->tail) {
@@ -404,7 +408,8 @@ static int hiddev_ioctl(struct inode *instruct hid_field *field;int i; -if (!hiddev->exist)return -EIO ; +if (!hiddev->exist) +return -EIO ;switch (cmd) {
@@ -646,18 +651,22 @@ static int hiddev_ioctl(struct inode *inif (_IOC_NR (cmd) ==_IOC_NR (HIDIOCGNAME (0 ))) {int len; -if (!hid->name)return 0 ; +if (!hid->name) +return 0 ; len =strlen (hid->name) +1 ; -if (len >_IOC_SIZE (cmd)) len =_IOC_SIZE (cmd); +if (len >_IOC_SIZE (cmd)) + len =_IOC_SIZE (cmd);return copy_to_user ((char *) arg, hid->name, len) ? -EFAULT : len; }if (_IOC_NR (cmd) ==_IOC_NR (HIDIOCGPHYS (0 ))) {int len; -if (!hid->phys)return 0 ; +if (!hid->phys) +return 0 ; len =strlen (hid->phys) +1 ; -if (len >_IOC_SIZE (cmd)) len =_IOC_SIZE (cmd); +if (len >_IOC_SIZE (cmd)) + len =_IOC_SIZE (cmd);return copy_to_user ((char *) arg, hid->phys, len) ? -EFAULT : len; }
drivers/usb/input/hid-core.c renamed from drivers/usb/input/hid-core.c~codingstyle-patrol-usb +22 -10
@@ -61,7 +61,8 @@ static struct hid_report *hid_register_rreturn NULL ;memset (report,0 ,sizeof (struct hid_report )); -if (id !=0 ) report_enum->numbered =1 ; +if (id !=0 ) + report_enum->numbered =1 ; report->id = id; report->type = type;
@@ -539,11 +540,13 @@ static void hid_free_device(struct hid_dfor (j =0 ; j <256 ; j++) {struct hid_report *report = report_enum->report_id_hash[j]; -if (report)hid_free_report (report); +if (report) +hid_free_report (report); } } -if (device->rdesc)kfree (device->rdesc); +if (device->rdesc) +kfree (device->rdesc);kfree (device); }
@@ -741,7 +744,8 @@ static __inline__ __s32 snto32(__u32 valstatic __inline____u32 s32ton (__s32 value,unsigned n) {__s32 a = value >> (n -1 ); -if (a && a !=-1 )return value <0 ?1 << (n -1 ) : (1 << (n -1 )) -1 ; +if (a && a !=-1 ) +return value <0 ?1 << (n -1 ) : (1 << (n -1 )) -1 ;return value & ((1 << n) -1 ); }
@@ -769,7 +773,10 @@ static __inline__ void implement(__u8 *rstatic __inline__int search (__s32 *array,__s32 value,unsigned n) { -while (n--)if (*array++ == value)return 0 ; +while (n--) { +if (*array++ == value) +return 0 ; + }return -1 ; }
@@ -814,9 +821,11 @@ static void hid_input_field(struct hid_dif (HID_MAIN_ITEM_VARIABLE & field->flags) {if (field->flags &HID_MAIN_ITEM_RELATIVE ) { -if (!value[n])continue ; +if (!value[n]) +continue ; }else { -if (value[n] == field->value[n])continue ; +if (value[n] == field->value[n]) +continue ; }hid_process_event (hid, field, &field->usage[n], value[n], regs);continue ;
@@ -1558,9 +1567,12 @@ static struct hid_device *usb_hid_config fail: -if (hid->urbin)usb_free_urb (hid->urbin); -if (hid->urbout)usb_free_urb (hid->urbout); -if (hid->urbctrl)usb_free_urb (hid->urbctrl); +if (hid->urbin) +usb_free_urb (hid->urbin); +if (hid->urbout) +usb_free_urb (hid->urbout); +if (hid->urbctrl) +usb_free_urb (hid->urbctrl);hid_free_buffers (dev, hid);hid_free_device (hid);
drivers/usb/input/hid-lgff.c renamed from drivers/usb/input/hid-lgff.c~codingstyle-patrol-usb +4 -2
@@ -154,7 +154,8 @@ int hid_lgff_init(struct hid_device* hid } private =kmalloc (sizeof (struct lgff_device ),GFP_KERNEL ); -if (!private)return -1 ; +if (!private) +return -1 ;memset (private,0 ,sizeof (struct lgff_device )); hid->ff_private = private;
@@ -216,7 +217,8 @@ static struct hid_report* hid_lgff_duplistruct hid_report * ret; ret =kmalloc (sizeof (struct lgff_device ),GFP_KERNEL ); -if (!ret)return NULL ; +if (!ret) +return NULL ; *ret = *report; ret->field[0 ] =kmalloc (sizeof (struct hid_field ),GFP_KERNEL );
drivers/usb/input/pid.c renamed from drivers/usb/input/pid.c~codingstyle-patrol-usb +4 -2
@@ -117,7 +117,8 @@ static int hid_pid_erase(struct input_deunsigned wanted_report =HID_UP_PID |FF_PID_USAGE_BLOCK_FREE ;/* PID Block Free Report */ int ret; -if (!CHECK_OWNERSHIP (id, pid))return -EACCES ; +if (!CHECK_OWNERSHIP (id, pid)) +return -EACCES ;/* Find report */ ret =hid_find_report_by_usage (hid, wanted_report, &report,HID_OUTPUT_REPORT );
@@ -214,7 +215,8 @@ static int hid_pid_upload_effect(struct }else {/* We want to update an effect */ -if (!CHECK_OWNERSHIP (effect->id, pid_private))return -EACCES ; +if (!CHECK_OWNERSHIP (effect->id, pid_private)) +return -EACCES ;/* Parameter type cannot be updated */ if (effect->type != pid_private->effects[effect->id].effect.type)
drivers/usb/input/hid-tmff.c renamed from drivers/usb/input/hid-tmff.c~codingstyle-patrol-usb +2 -1
@@ -112,7 +112,8 @@ int hid_tmff_init(struct hid_device *hidstruct list_head *pos; private =kmalloc (sizeof (struct tmff_device ),GFP_KERNEL ); -if (!private)return -ENOMEM ; +if (!private) +return -ENOMEM ;memset (private,0 ,sizeof (struct tmff_device )); hid->ff_private = private;
drivers/usb/net/catc.c renamed from drivers/usb/net/catc.c~codingstyle-patrol-usb +8 -4
@@ -828,10 +828,14 @@ static int catc_probe(struct usb_interfaif ((!catc->ctrl_urb) || (!catc->tx_urb) || (!catc->rx_urb) || (!catc->irq_urb)) {err ("No free urbs available." ); -if (catc->ctrl_urb)usb_free_urb (catc->ctrl_urb); -if (catc->tx_urb)usb_free_urb (catc->tx_urb); -if (catc->rx_urb)usb_free_urb (catc->rx_urb); -if (catc->irq_urb)usb_free_urb (catc->irq_urb); +if (catc->ctrl_urb) +usb_free_urb (catc->ctrl_urb); +if (catc->tx_urb) +usb_free_urb (catc->tx_urb); +if (catc->rx_urb) +usb_free_urb (catc->rx_urb); +if (catc->irq_urb) +usb_free_urb (catc->irq_urb);kfree (netdev);kfree (catc);return -ENOMEM ;
drivers/usb/misc/auerswald.c renamed from drivers/usb/misc/auerswald.c~codingstyle-patrol-usb +22 -11
@@ -572,7 +572,8 @@ static int auerchain_setup (pauerchain_t/* fill the list of free elements */ for (;numElements; numElements--) { acep = (pauerchainelement_t )kmalloc (sizeof (auerchainelement_t),GFP_KERNEL ); -if (!acep) goto ac_fail; +if (!acep) + goto ac_fail;memset (acep,0 ,sizeof (auerchainelement_t));INIT_LIST_HEAD (&acep->list);list_add_tail (&acep->list, &acp->free_list);
@@ -780,16 +781,20 @@ static int auerbuf_setup (pauerbufctl_t/* fill the list of free elements */ for (;numElements; numElements--) { bep = (pauerbuf_t )kmalloc (sizeof (auerbuf_t),GFP_KERNEL ); -if (!bep) goto bl_fail; +if (!bep) + goto bl_fail;memset (bep,0 ,sizeof (auerbuf_t)); bep->list = bcp;INIT_LIST_HEAD (&bep->buff_list); bep->bufp = (char *)kmalloc (bufsize,GFP_KERNEL ); -if (!bep->bufp) goto bl_fail; +if (!bep->bufp) + goto bl_fail; bep->dr = (struct usb_ctrlrequest *)kmalloc (sizeof (struct usb_ctrlrequest ),GFP_KERNEL ); -if (!bep->dr) goto bl_fail; +if (!bep->dr) + goto bl_fail; bep->urbp =usb_alloc_urb (0 ,GFP_KERNEL ); -if (!bep->urbp) goto bl_fail; +if (!bep->urbp) + goto bl_fail;list_add_tail (&bep->buff_list, &bcp->free_buff_list); }return 0 ;
@@ -1242,7 +1247,8 @@ static void auerchar_ctrlread_dispatch (static void auerswald_delete (pauerswald_t cp) {dbg ("auerswald_delete" ); -if (cp ==NULL )return ; +if (cp ==NULL ) +return ;/* Wake up all processes waiting for a buffer */ wake_up (&cp->bufferwait);
@@ -1261,7 +1267,8 @@ static void auerswald_delete( pauerswaldstatic void auerchar_delete (pauerchar_t ccp) {dbg ("auerchar_delete" ); -if (ccp ==NULL )return ; +if (ccp ==NULL ) +return ;/* wake up pending synchronous reads */ ccp->removed =1 ;
@@ -1335,7 +1342,8 @@ static void auerswald_removeservice (paudbg ("auerswald_removeservice called" );/* check if we have a service allocated */ -if (scp->id ==AUH_UNASSIGNED )return ; +if (scp->id ==AUH_UNASSIGNED ) +return ;/* If there is a device: close the channel */ if (cp->usbdev) {
@@ -1494,7 +1502,8 @@ static int auerchar_ioctl (struct inode u =0 ;/* no data */ if (ccp->readbuf) {int restlen = ccp->readbuf->len - ccp->readoffset; -if (restlen >0 ) u =1 ; +if (restlen >0 ) + u =1 ; }if (!u) {if (!list_empty (&ccp->bufctl.rec_buff_list)) {
@@ -1787,7 +1796,8 @@ write_again: }/* protect against too big write requests */ -if (len > cp->maxControlLength) len = cp->maxControlLength; +if (len > cp->maxControlLength) + len = cp->maxControlLength;/* Fill the buffer */ if (copy_from_user ( bp->bufp+AUH_SIZE , buf, len)) {
@@ -2096,7 +2106,8 @@ static void auerswald_disconnect (struct/* Inform all waiting readers */ for ( u =0 ; u <AUH_TYPESIZE ; u++) {pauerscon_t scp = cp->services[u]; -if (scp) scp->disconnect ( scp); +if (scp) + scp->disconnect ( scp); } } }
drivers/usb/class/cdc-acm.c renamed from drivers/usb/class/cdc-acm.c~codingstyle-patrol-usb +28 -14
114 lines not on this page · raw message
drivers/usb/class/usblp.c renamed from drivers/usb/class/usblp.c~codingstyle-patrol-usb +18 -10
@@ -298,8 +298,10 @@ static int usblp_check_status(struct usb status = *usblp->statusbuf;if (~status &LP_PERRORP ) { newerr =3 ; -if (status &LP_POUTPA ) newerr =1 ; -if (~status &LP_PSELECD ) newerr =2 ; +if (status &LP_POUTPA ) + newerr =1 ; +if (~status &LP_PSELECD ) + newerr =2 ; }if (newerr != err)
@@ -926,8 +928,8 @@ abort:if (usblp->readbuf)usb_buffer_free (usblp->dev,USBLP_BUF_SIZE , usblp->readbuf, usblp->writeurb->transfer_dma); -if (usblp->statusbuf)kfree (usblp->statusbuf); -if (usblp->device_id_string)kfree (usblp->device_id_string); +kfree (usblp->statusbuf); +kfree (usblp->device_id_string);usb_free_urb (usblp->writeurb);usb_free_urb (usblp->readurb);kfree (usblp);
@@ -987,10 +989,12 @@ static int usblp_select_alts(struct usblcontinue ;if (!(epd->bEndpointAddress &USB_ENDPOINT_DIR_MASK )) { -if (!epwrite) epwrite=epd; +if (!epwrite) + epwrite = epd; }else { -if (!epread) epread=epd; +if (!epread) + epread = epd; } }
@@ -1020,9 +1024,12 @@ static int usblp_select_alts(struct usblreturn proto_bias;/* Ordering is important here. */ -if (usblp->protocol[2 ].alt_setting !=-1 )return 2 ; -if (usblp->protocol[1 ].alt_setting !=-1 )return 1 ; -if (usblp->protocol[3 ].alt_setting !=-1 )return 3 ; +if (usblp->protocol[2 ].alt_setting !=-1 ) +return 2 ; +if (usblp->protocol[1 ].alt_setting !=-1 ) +return 1 ; +if (usblp->protocol[3 ].alt_setting !=-1 ) +return 3 ;/* If nothing is available, then don't bind to this device. */ return -1 ;
@@ -1036,7 +1043,8 @@ static int usblp_set_protocol(struct usbreturn -EINVAL ; alts = usblp->protocol[protocol].alt_setting; -if (alts <0 )return -EINVAL ; +if (alts <0 ) +return -EINVAL ; r =usb_set_interface (usblp->dev, usblp->ifnum, alts);if (r <0 ) { err("can't set desired altsetting %d on interface %d",
drivers/usb/core/hcd-pci.c renamed from drivers/usb/core/hcd-pci.c~codingstyle-patrol-usb +2 -1
@@ -209,7 +209,8 @@ void usb_hcd_pci_remove (struct pci_devreturn ;dev_info (hcd->controller,"remove, state %x\n" , hcd->state); -if (in_interrupt ())BUG (); +if (in_interrupt ()) +BUG (); hub = hcd->self.root_hub; hcd->state =USB_STATE_QUIESCING ;
drivers/usb/host/ohci-sa1111.c renamed from drivers/usb/host/ohci-sa1111.c~codingstyle-patrol-usb +4 -2
@@ -209,7 +209,8 @@ int usb_hcd_sa1111_probe (const struct h err2:hcd_buffer_destroy (hcd); -if (hcd) driver->hcd_free (hcd); +if (hcd) + driver->hcd_free (hcd); err1:sa1111_stop_hc (dev);release_mem_region (dev->res.start, dev->res.end - dev->res.start +1 );
@@ -237,7 +238,8 @@ void usb_hcd_sa1111_remove (struct usb_hinfo ("remove: %s, state %x" , hcd->self.bus_name, hcd->state); -if (in_interrupt ())BUG (); +if (in_interrupt ()) +BUG (); hub = hcd->self.root_hub; hcd->state =USB_STATE_QUIESCING ;
_
Re: [USB] [PATCH] Crud-- (was: Re: [ATM] [UPDATE] unbalanced usw)
Francois Romieu <[email protected]> :
[...]
drivers/usb/class/usb-midi.c static struct usb_midi_device *parse_des · line 1360
Quoted 12 lines
diff -puN
drivers/usb/class/usb-midi.c~codingstyle-patrol-usbdrivers/usb/class/usb-midi.cdrivers/usb/class/usb-midi.c renamed from drivers/usb/class/usb-midi.c~codingstyle-patrol-usb +3
-2@@ -1355,8 +1355,10 @@ static struct usb_midi_device *parse_des next = p2 + p2[0 ]; length -= p2[0 ]; -if (p2[0 ] <2 )break ; -if (p2[1 ] !=USB_DT_CS_INTERFACE )break ; +if (p2[0 ] <2 ) +break ; +if (p2[1 ] !=USB_DT_CS_INTERFACE )r
!!!
drivers/usb/class/usb-midi.c static struct usb_midi_device *parse_des · line 1361
Quoted 1 line
drivers/usb/class/usb-midi.c renamed from drivers/usb/class/usb-midi.c~codingstyle-patrol-usb +1
-0+break ;
... and the winner of the "Bozo of the week award" is Francois Romieu for his new concept of compilation checking without .config update.
I'll send a link for an update once it at least compiles.
--
Ueimor
Re: [USB] [PATCH] Crud-- (was: Re: [ATM] [UPDATE] unbalanced usw)
Francois Romieu <[email protected]> :
[...]
Quoted 1 line
I'll send a link for an update once it at least compiles.
Update available at:
http://www.fr.zoreil.com/2.5.69/patches/codingstyle-patrol-usb.patch
This one really compiles.
--
Ueimor
Greg KH
AppliedRe: [USB] [PATCH] Crud-- (was: Re: [ATM] [UPDATE] unbalanced usw)
Quoted 8 lines · Francois Romieu
Francois Romieu <[email protected]> :
[...]Quoted 1 line
I'll send a link for an update once it at least compiles.
Update available at:
http://www.fr.zoreil.com/2.5.69/patches/codingstyle-patrol-usb.patchThis one really compiles.
Applied, thanks.
greg k-h
Re: [ATM] [UPDATE] unbalanced exit path in Forerunner HE he_init_one() (and an iphase patch too!)
In reply to chas williamschas williams <[email protected]> :
[...]
Quoted 1 line
i hope you dont mind, but i really dislike the goto. one is plenty.
I am not fond of the 'test before destruction' approach when the tests could have been avoided. Anyway you maintain the driver on a regular basis, you are free to choose the fix.
Quoted 2 lines
attached is a cleanup for the iphase (i made a small addition -- i believe the
MEMDUMP_XXXXXXREGcases are also guilty of dereferencing).
Yes.
--
Ueimor
Quoted 1 line · In message <[email protected]>,Francois Romieu…
-
pci_enable_device()isn't balanced on error path;
actually pci_disable_device() should probably go into he_stop() or add it to he_remove_one() and he_cleanup()
chas williams <[email protected]> :
[...]
Quoted 2 lines
actually
pci_disable_device()should probably go intohe_stop()or add it tohe_remove_one()andhe_cleanup()
Doh... This should be more accurate:
- pci_enable_device() balanced on error path in he_init_one();
- return of atm_dev_register() isn't lost any more if he_dev allocation fails in he_init_one();
- pci_disable_device() added to he_disable_one();
- he_cleanup() calls he_disable_one() instead of duplicating it;
- partial rework of code dedicated to kernel versions compatibility.
drivers/atm/he.c | 67 +++++++++++++++++++++++++++++++++++++------------------------------ 1 files changed, 37 insertions(+), 30 deletions(-)
drivers/atm/he.c renamed from drivers/atm/he.c~drivers-atm-he-chas +37 -30
@@ -98,9 +98,16 @@ void sn_delete_polled_interrupt(int irq)/* 2.2 kernel support */ +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,4 ,4 ) +#define pci_set_drvdata (pdev,p) ((pdev)->driver_data = p) +#define pci_get_drvdata (pdev) (pdev)->driver_data +#endif +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,3 ,43 )#define dev_kfree_skb_irq (skb) dev_kfree_skb(skb)#define dev_kfree_skb_any (skb) dev_kfree_skb(skb) +#define pci_disable_device (pdev) do {} while(0) +#define pci_enable_device (pdev) 0#undef USE_TASKLET#endif
@@ -354,29 +361,30 @@ he_init_one(struct pci_dev *pci_dev, con {struct atm_dev *atm_dev;struct he_dev *he_dev; +int ret = -EIO ;printk (KERN_INFO "he: %s\n" , version); -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,3 ,43 ) -if (pci_enable_device (pci_dev))return -EIO ; -#endif +if (pci_enable_device (pci_dev)) + goto out;if (pci_set_dma_mask (pci_dev,HE_DMA_MASK ) !=0 ) {printk (KERN_WARNING "he: no suitable dma available\n" ); -return -EIO ; + goto out_disable; } atm_dev =atm_dev_register (DEV_LABEL , &he_ops,-1 ,0 ); -if (!atm_dev)return -ENODEV ; -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,4 ,3 ) +if (!atm_dev) { + ret = -ENODEV ; + goto out_disable; + }pci_set_drvdata (pci_dev, atm_dev); -#else - pci_dev->driver_data = atm_dev; -#endif - he_dev = (struct he_dev *)kmalloc (sizeof (struct he_dev ), -GFP_KERNEL ); -if (!he_dev)return -ENOMEM ; + he_dev = (struct he_dev *)kmalloc (sizeof (*he_dev),GFP_KERNEL ); +if (!he_dev) { + ret = -ENOMEM ; + goto out_deregister; + }memset (he_dev,0 ,sizeof (struct he_dev )); he_dev->pci_dev = pci_dev;
@@ -385,16 +393,25 @@ he_init_one(struct pci_dev *pci_dev, conHE_DEV (atm_dev) = he_dev; he_dev->number = atm_dev->number;/* was devs */ if (he_start (atm_dev)) { -atm_dev_deregister (atm_dev); -he_stop (he_dev); -kfree (he_dev); -return -ENODEV ; + ret = -ENODEV ; + goto out_stop; } he_dev->next =NULL ;if (he_devs) he_dev->next = he_devs; he_devs = he_dev; -return 0 ; + ret =0 ; +out: +return ret; + +out_stop: +he_stop (he_dev); +kfree (he_dev); +out_deregister: +atm_dev_deregister (atm_dev); +out_disable: +pci_disable_device (pci_dev); + goto out; }static void __devexit
@@ -403,11 +420,7 @@ he_remove_one (struct pci_dev *pci_dev)struct atm_dev *atm_dev;struct he_dev *he_dev; -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,4 ,3 ) atm_dev =pci_get_drvdata (pci_dev); -#else - atm_dev = pci_dev->driver_data; -#endif he_dev =HE_DEV (atm_dev);/* need to remove from he_devs */
@@ -416,11 +429,8 @@ he_remove_one (struct pci_dev *pci_dev)atm_dev_deregister (atm_dev);kfree (he_dev); -#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,4 ,3 )pci_set_drvdata (pci_dev,NULL ); -#else - pci_dev->driver_data =NULL ; -#endif +pci_disable_device (pci_dev); }
@@ -3384,13 +3394,10 @@ he_init() static void __devexithe_cleanup (void ) { -while (he_devs) - { +while (he_devs) {struct he_dev *next = he_devs->next; -he_stop (he_devs); -atm_dev_deregister (he_devs->atm_dev); -kfree (he_devs); +he_remove_one (he_dev->pci_dev); he_devs = next; }