netdevice: 82596: Convert netdev->priv to netdev_priv
Subsystems (2)
PATCH 1/1 netdevice: 82596: Convert netdev->priv to netdev_priv
netdev->priv points to the memory which is allocated by alloc_etherdev(). It should not change to netdev->mem_start later.
After fixing it, mem_start allocation is useless.
I removed mem_start allocation.
Signed-off-by Wang Chen <[email protected]>
--- drivers/net/82596.c | 59 ++++++++++++-------------------------------------- 1 files changed, 14 insertions(+), 45 deletions(-)
drivers/net/82596.c +14 -45
@@ -457,7 +457,7 @@ static inline int wait_cfg(struct net_device *dev, struct i596_cmd *cmd, int delstatic void i596_display_data (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);struct i596_cmd *cmd;struct i596_rfd *rfd;struct i596_rbd *rbd;
@@ -527,7 +527,7 @@ static irqreturn_t i596_error(int irq, void *dev_id)static inline void init_rx_bufs (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int i;struct i596_rfd *rfd;struct i596_rbd *rbd;
@@ -578,7 +578,7 @@ static inline void init_rx_bufs(struct net_device *dev)static inline void remove_rx_bufs (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);struct i596_rbd *rbd;int i;
@@ -592,7 +592,7 @@ static inline void remove_rx_bufs(struct net_device *dev)static void rebuild_rx_bufs (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int i;/* Ensure rx frame/buffer descriptors are tidy */
@@ -611,7 +611,7 @@ static void rebuild_rx_bufs(struct net_device *dev)static int init_i596_mem (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);#if !defined(ENABLE_MVME16x_NET) && !defined(ENABLE_BVME6000_NET ) || defined(ENABLE_APRICOT )short ioaddr = dev->base_addr;#endif
@@ -764,7 +764,7 @@ failed:static inline int i596_rx (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);struct i596_rfd *rfd;struct i596_rbd *rbd;int frames =0 ;
@@ -958,7 +958,7 @@ static void i596_reset(struct net_device *dev, struct i596_private *lp,static void i596_add_cmd (struct net_device *dev,struct i596_cmd *cmd) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int ioaddr = dev->base_addr;unsigned long flags;
@@ -1028,7 +1028,7 @@ static int i596_open(struct net_device *dev)static void i596_tx_timeout (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int ioaddr = dev->base_addr;/* Transmitter timeout, serious problems. */
@@ -1057,7 +1057,7 @@ static void i596_tx_timeout (struct net_device *dev)static int i596_start_xmit (struct sk_buff *skb,struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);struct tx_cmd *tx_cmd;struct i596_tbd *tbd;short length = skb->len;
@@ -1216,12 +1216,6 @@ struct net_device * __init i82596_probe(int unit) goto out; found: - dev->mem_start = (int )__get_free_pages (GFP_ATOMIC ,0 ); -if (!dev->mem_start) { - err = -ENOMEM ; - goto out1; - } -DEB (DEB_PROBE ,printk (KERN_INFO "%s: 82596 at %#3lx," , dev->name, dev->base_addr));for (i =0 ; i <6 ; i++)
@@ -1239,20 +1233,13 @@ found: dev->tx_timeout = i596_tx_timeout; dev->watchdog_timeo =TX_TIMEOUT ; - dev->priv = (void *)(dev->mem_start); - - lp = dev->priv; + lp =netdev_priv (dev);DEB (DEB_INIT ,printk (KERN_DEBUG "%s: lp at 0x%08lx (%zd bytes), " "lp->scb at 0x%08lx\n" , dev->name, (unsigned long )lp,sizeof (struct i596_private ), (unsigned long )&lp->scb));memset ((void *) lp,0 ,sizeof (struct i596_private )); -#ifdef __mc68000__ -cache_push (virt_to_phys ((void *)(dev->mem_start)),4096 ); -cache_clear (virt_to_phys ((void *)(dev->mem_start)),4096 ); -kernel_set_cachemode ((void *)(dev->mem_start),4096 ,IOMAP_NOCACHE_SER ); -#endif lp->scb.command =0 ; lp->scb.cmd =I596_NULL ; lp->scb.rfd =I596_NULL ;
@@ -1260,17 +1247,8 @@ found: err =register_netdev (dev);if (err) - goto out2; + goto out1;return dev; -out2: -#ifdef __mc68000__ -/* XXX This assumes default cache mode to be IOMAP_FULL_CACHING, -* XXX which may be invalid (CONFIG_060_WRITETHROUGH) -*/ -kernel_set_cachemode ((void *)(dev->mem_start),4096 , -IOMAP_FULL_CACHING ); -#endif -free_page ((u32)(dev->mem_start)); out1:#ifdef ENABLE_APRICOT release_region (dev->base_addr,I596_TOTAL_SIZE );
@@ -1302,7 +1280,7 @@ static irqreturn_t i596_interrupt(int irq, void *dev_id) } ioaddr = dev->base_addr; - lp = dev->priv; + lp =netdev_priv (dev);spin_lock (&lp->lock);
@@ -1445,7 +1423,7 @@ static irqreturn_t i596_interrupt(int irq, void *dev_id)static int i596_close (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);unsigned long flags;netif_stop_queue (dev);
@@ -1495,7 +1473,7 @@ static int i596_close(struct net_device *dev)static void set_multicast_list (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int config =0 , cnt; DEB(DEB_MULTI,printk(KERN_DEBUG "%s: set multicast list, %d entries, promisc %s, allmulti %s\n",
@@ -1581,15 +1559,6 @@ int __init init_module(void) void __exit cleanup_module(void) {unregister_netdev (dev_82596); -#ifdef __mc68000__ -/* XXX This assumes default cache mode to be IOMAP_FULL_CACHING, -* XXX which may be invalid (CONFIG_060_WRITETHROUGH) -*/ - -kernel_set_cachemode ((void *)(dev_82596->mem_start),4096 , -IOMAP_FULL_CACHING ); -#endif -free_page ((u32)(dev_82596->mem_start));#ifdef ENABLE_APRICOT /* If we don't do this, we can't re-insmod it later. */ release_region (dev_82596->base_addr,I596_TOTAL_SIZE );
Signature · 1 line
1.5.3.4
David Miller
maintainerFrom: Wang Chen <[email protected]>
Date: Mon, 17 Nov 2008 14:04:13 +0800
drivers/net/82596.c found: · line 1255
Quoted 5 lines
drivers/net/82596.c +0
-5-#ifdef __mc68000__ -cache_push (virt_to_phys ((void *)(dev->mem_start)),4096 ); -cache_clear (virt_to_phys ((void *)(dev->mem_start)),4096 ); -kernel_set_cachemode ((void *)(dev->mem_start),4096 ,IOMAP_NOCACHE_SER ); -#endif
Do you have any idea what this code is doing that you are removing?
It's changing the cache mode of the memory behind ->mem_start because this "lp" struct is directly accessed by the hardware, and without these operations the CPU and the device won't see consistent values in the structure.
Therefore your change breaks the driver.
David Miller said the following on 2008-11-17 14:36:
Quoted 11 lines
From: Wang Chen <[email protected]>
Date: Mon, 17 Nov 2008 14:04:13 +0800drivers/net/82596.c found: · line 1255
Quoted 5 lines
drivers/net/82596.c +0
-5-#ifdef __mc68000__ -cache_push (virt_to_phys ((void *)(dev->mem_start)),4096 ); -cache_clear (virt_to_phys ((void *)(dev->mem_start)),4096 ); -kernel_set_cachemode ((void *)(dev->mem_start),4096 ,IOMAP_NOCACHE_SER ); -#endif Do you have any idea what this code is doing that you are removing?
Quoted 7 lines
It's changing the cache mode of the memory behind ->mem_start because this "lp" struct is directly accessed by the hardware, and without these operations the CPU and the device won't see consistent values in the structure.
Therefore your change breaks the driver.
Yes. I was wrong.
Will send a patch again.
Thank you for stopping my fool code.
PATCH v2 1/1 netdevice: 82596: Convert netdev->priv to netdev_priv
In reply to David MillerThe memory, which pointed by netdev->priv, can be allocated by alloc_etherdev(). We don't need to use __get_free_pages() to get memory.
Signed-off-by Wang Chen <[email protected]>
--- drivers/net/82596.c | 53 +++++++++++++++++++++++--------------------------- 1 files changed, 24 insertions(+), 29 deletions(-)
drivers/net/82596.c +24 -29
@@ -457,7 +457,7 @@ static inline int wait_cfg(struct net_device *dev, struct i596_cmd *cmd, int delstatic void i596_display_data (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);struct i596_cmd *cmd;struct i596_rfd *rfd;struct i596_rbd *rbd;
@@ -527,7 +527,7 @@ static irqreturn_t i596_error(int irq, void *dev_id)static inline void init_rx_bufs (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int i;struct i596_rfd *rfd;struct i596_rbd *rbd;
@@ -578,7 +578,7 @@ static inline void init_rx_bufs(struct net_device *dev)static inline void remove_rx_bufs (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);struct i596_rbd *rbd;int i;
@@ -592,7 +592,7 @@ static inline void remove_rx_bufs(struct net_device *dev)static void rebuild_rx_bufs (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int i;/* Ensure rx frame/buffer descriptors are tidy */
@@ -611,7 +611,7 @@ static void rebuild_rx_bufs(struct net_device *dev)static int init_i596_mem (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);#if !defined(ENABLE_MVME16x_NET) && !defined(ENABLE_BVME6000_NET ) || defined(ENABLE_APRICOT )short ioaddr = dev->base_addr;#endif
@@ -764,7 +764,7 @@ failed:static inline int i596_rx (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);struct i596_rfd *rfd;struct i596_rbd *rbd;int frames =0 ;
@@ -958,7 +958,7 @@ static void i596_reset(struct net_device *dev, struct i596_private *lp,static void i596_add_cmd (struct net_device *dev,struct i596_cmd *cmd) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int ioaddr = dev->base_addr;unsigned long flags;
@@ -1028,7 +1028,7 @@ static int i596_open(struct net_device *dev)static void i596_tx_timeout (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int ioaddr = dev->base_addr;/* Transmitter timeout, serious problems. */
@@ -1057,7 +1057,7 @@ static void i596_tx_timeout (struct net_device *dev)static int i596_start_xmit (struct sk_buff *skb,struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);struct tx_cmd *tx_cmd;struct i596_tbd *tbd;short length = skb->len;
@@ -1135,7 +1135,7 @@ struct net_device * __init i82596_probe(int unit)return ERR_PTR (-ENODEV ); probed++; - dev =alloc_etherdev (0 ); + dev =alloc_etherdev (sizeof (struct i596_private ));if (!dev)return ERR_PTR (-ENOMEM );
@@ -1216,12 +1216,6 @@ struct net_device * __init i82596_probe(int unit) goto out; found: - dev->mem_start = (int )__get_free_pages (GFP_ATOMIC ,0 ); -if (!dev->mem_start) { - err = -ENOMEM ; - goto out1; - } -DEB (DEB_PROBE ,printk (KERN_INFO "%s: 82596 at %#3lx," , dev->name, dev->base_addr));for (i =0 ; i <6 ; i++)
@@ -1239,9 +1233,7 @@ found: dev->tx_timeout = i596_tx_timeout; dev->watchdog_timeo =TX_TIMEOUT ; - dev->priv = (void *)(dev->mem_start); - - lp = dev->priv; + lp =netdev_priv (dev); DEB(DEB_INIT,printk(KERN_DEBUG "%s: lp at 0x%08lx (%zd bytes), " "lp->scb at 0x%08lx\n", dev->name, (unsigned long)lp,
@@ -1249,9 +1241,12 @@ found:memset ((void *) lp,0 ,sizeof (struct i596_private ));#ifdef __mc68000__ -cache_push (virt_to_phys ((void *)(dev->mem_start)),4096 ); -cache_clear (virt_to_phys ((void *)(dev->mem_start)),4096 ); -kernel_set_cachemode ((void *)(dev->mem_start),4096 ,IOMAP_NOCACHE_SER ); +cache_push (virt_to_phys ((void *)netdev_priv (dev)), +sizeof (struct i596_private )); +cache_clear (virt_to_phys ((void *)netdev_priv (dev)), +sizeof (struct i596_private )); +kernel_set_cachemode ((void *)netdev_priv (dev), +sizeof (struct i596_private ),IOMAP_NOCACHE_SER );#endif lp->scb.command =0 ; lp->scb.cmd =I596_NULL ;
@@ -1267,10 +1262,10 @@ out2:/* XXX This assumes default cache mode to be IOMAP_FULL_CACHING, * XXX which may be invalid (CONFIG_060_WRITETHROUGH) */ -kernel_set_cachemode ((void *)(dev->mem_start),4096 , +kernel_set_cachemode ((void *)netdev_priv (dev), +sizeof (struct i596_private ),IOMAP_FULL_CACHING );#endif -free_page ((u32)(dev->mem_start)); out1:#ifdef ENABLE_APRICOT release_region (dev->base_addr,I596_TOTAL_SIZE );
@@ -1302,7 +1297,7 @@ static irqreturn_t i596_interrupt(int irq, void *dev_id) } ioaddr = dev->base_addr; - lp = dev->priv; + lp =netdev_priv (dev);spin_lock (&lp->lock);
@@ -1445,7 +1440,7 @@ static irqreturn_t i596_interrupt(int irq, void *dev_id)static int i596_close (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);unsigned long flags;netif_stop_queue (dev);
@@ -1495,7 +1490,7 @@ static int i596_close(struct net_device *dev)static void set_multicast_list (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp =netdev_priv (dev);int config =0 , cnt; DEB(DEB_MULTI,printk(KERN_DEBUG "%s: set multicast list, %d entries, promisc %s, allmulti %s\n",
@@ -1586,10 +1581,10 @@ void __exit cleanup_module(void)* XXX which may be invalid (CONFIG_060_WRITETHROUGH) */ -kernel_set_cachemode ((void *)(dev_82596->mem_start),4096 , +kernel_set_cachemode ((void *)netdev_priv (dev_82596), +sizeof (struct i596_private ),IOMAP_FULL_CACHING );#endif -free_page ((u32)(dev_82596->mem_start));#ifdef ENABLE_APRICOT /* If we don't do this, we can't re-insmod it later. */ release_region (dev_82596->base_addr,I596_TOTAL_SIZE );
Signature · 1 line
1.5.3.4
David Miller
maintainerFrom: Wang Chen <[email protected]>
Date: Mon, 17 Nov 2008 17:24:38 +0800
Quoted 4 lines
The memory, which pointed by
netdev->priv, can be allocated byalloc_etherdev(). We don't need to use__get_free_pages()to get memory.Signed-off-by Wang Chen <[email protected]>
I don't think this is right either.
There might be device imposed alignment constraints, for example.
And a whole page is also being allocated so that this cache mode change doesn't have an effect on unrelated kernel memory. That could kill performance if some critical other data structure sits in the same page and now has it's cacheability disabled.
David Miller said the following on 2008-11-17 17:29:
Quoted 8 lines
From: Wang Chen <[email protected]>
Date: Mon, 17 Nov 2008 17:24:38 +0800Quoted 4 lines
The memory, which pointed by
netdev->priv, can be allocated byalloc_etherdev(). We don't need to use__get_free_pages()to get memory.Signed-off-by Wang Chen <[email protected]>
Quoted 9 lines
I don't think this is right either.
There might be device imposed alignment constraints, for example.
And a whole page is also being allocated so that this cache mode change doesn't have an effect on unrelated kernel memory. That could kill performance if some critical other data structure sits in the same page and now has it's cacheability disabled.
On this situation, I think that keeping mem_start and using ml_priv is the only way to remove directly reference of netdev->priv.
I will send v3 tomorrow.
PATCH v3 1/1 netdevice: 82596: Convert netdev->priv to netdev_priv
1. When alloc_etherdev(), no memory be allocated to netdev->priv.
2. And it's need to get a whole page for priv.
For these reasons, use netdev->ml_priv to point to the page is the best method to convert directly reference of netdev->priv.
Signed-off-by Wang Chen <[email protected]>
--- drivers/net/82596.c | 28 ++++++++++++++-------------- 1 files changed, 14 insertions(+), 14 deletions(-)
drivers/net/82596.c +14 -14
@@ -457,7 +457,7 @@ static inline int wait_cfg(struct net_device *dev, struct i596_cmd *cmd, int delstatic void i596_display_data (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;struct i596_cmd *cmd;struct i596_rfd *rfd;struct i596_rbd *rbd;
@@ -527,7 +527,7 @@ static irqreturn_t i596_error(int irq, void *dev_id)static inline void init_rx_bufs (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;int i;struct i596_rfd *rfd;struct i596_rbd *rbd;
@@ -578,7 +578,7 @@ static inline void init_rx_bufs(struct net_device *dev)static inline void remove_rx_bufs (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;struct i596_rbd *rbd;int i;
@@ -592,7 +592,7 @@ static inline void remove_rx_bufs(struct net_device *dev)static void rebuild_rx_bufs (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;int i;/* Ensure rx frame/buffer descriptors are tidy */
@@ -611,7 +611,7 @@ static void rebuild_rx_bufs(struct net_device *dev)static int init_i596_mem (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;#if !defined(ENABLE_MVME16x_NET) && !defined(ENABLE_BVME6000_NET ) || defined(ENABLE_APRICOT )short ioaddr = dev->base_addr;#endif
@@ -764,7 +764,7 @@ failed:static inline int i596_rx (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;struct i596_rfd *rfd;struct i596_rbd *rbd;int frames =0 ;
@@ -958,7 +958,7 @@ static void i596_reset(struct net_device *dev, struct i596_private *lp,static void i596_add_cmd (struct net_device *dev,struct i596_cmd *cmd) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;int ioaddr = dev->base_addr;unsigned long flags;
@@ -1028,7 +1028,7 @@ static int i596_open(struct net_device *dev)static void i596_tx_timeout (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;int ioaddr = dev->base_addr;/* Transmitter timeout, serious problems. */
@@ -1057,7 +1057,7 @@ static void i596_tx_timeout (struct net_device *dev)static int i596_start_xmit (struct sk_buff *skb,struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;struct tx_cmd *tx_cmd;struct i596_tbd *tbd;short length = skb->len;
@@ -1239,9 +1239,9 @@ found: dev->tx_timeout = i596_tx_timeout; dev->watchdog_timeo =TX_TIMEOUT ; - dev->priv = (void *)(dev->mem_start); + dev->ml_priv = (void *)(dev->mem_start); - lp = dev->priv; + lp = dev->ml_priv; DEB(DEB_INIT,printk(KERN_DEBUG "%s: lp at 0x%08lx (%zd bytes), " "lp->scb at 0x%08lx\n", dev->name, (unsigned long)lp,
@@ -1302,7 +1302,7 @@ static irqreturn_t i596_interrupt(int irq, void *dev_id) } ioaddr = dev->base_addr; - lp = dev->priv; + lp = dev->ml_priv;spin_lock (&lp->lock);
@@ -1445,7 +1445,7 @@ static irqreturn_t i596_interrupt(int irq, void *dev_id)static int i596_close (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;unsigned long flags;netif_stop_queue (dev);
@@ -1495,7 +1495,7 @@ static int i596_close(struct net_device *dev)static void set_multicast_list (struct net_device *dev) { -struct i596_private *lp = dev->priv; +struct i596_private *lp = dev->ml_priv;int config =0 , cnt; DEB(DEB_MULTI,printk(KERN_DEBUG "%s: set multicast list, %d entries, promisc %s, allmulti %s\n",
Signature · 1 line
1.5.3.4
From: Wang Chen <[email protected]>
Date: Tue, 18 Nov 2008 11:57:25 +0800
Quoted 7 lines
1. When
alloc_etherdev(), no memory be allocated tonetdev->priv.
2. And it's need to get a whole page for priv.For these reasons, use
netdev->ml_privto point to the page is the best method to convert directly reference ofnetdev->priv.Signed-off-by Wang Chen <[email protected]>
Applied, thanks for all the hard work on this one ;-)