Skip to contentSkip to conversation

e1000_main.c: fix inline compile errors

PATCH In review Adrian Bunk 2 participants 5 messages Jul 14, 2004 – Jul 17, 2004 2 messages not in the archive Network drivers

lore
Share & tools
lore

Adrian Bunk

author
#0 lore raw

PATCH 1/1 e1000_main.c: fix inline compile errors

Trying to compile drivers/net/e1000/e1000_main.c in 2.6.8-rc1-mm1 with gcc 3.4 results in compile errors starting with the following:

<-- snip -->

...
CC drivers/net/e1000/e1000_main.o
drivers/net/e1000/e1000_main.c: In function `e1000_up':
drivers/net/e1000/e1000_main.c:136: sorry, unimplemented: inlining failed in call to 'e1000_irq_enable': function body not available drivers/net/e1000/e1000_main.c:274: sorry, unimplemented: called from here make[3]: *** [drivers/net/e1000/e1000_main.o] Error 1

<-- snip -->

The patch below moves some inlined functions before the place where they are called the first time.

An alternative approach would be to remove the inlines.

diffstat output:

 drivers/net/e1000/e1000_main.c |  137 ++++++++++++++++-----------------
 1 files changed, 69 insertions(+), 68 deletions(-)
Signed-off-by Adrian Bunk <[email protected]>
drivers/net/e1000/e1000_main.c renamed from drivers/net/e1000/e1000_main.c.old +69 -68
@@ -100,6 +100,9 @@
 
 MODULE_DEVICE_TABLE(pci, e1000_pci_tbl);
 
+static inline void e1000_irq_disable(struct e1000_adapter *adapter);
+static inline void e1000_irq_enable(struct e1000_adapter *adapter);
+
 int e1000_up(struct e1000_adapter *adapter);
 void e1000_down(struct e1000_adapter *adapter);
 void e1000_reset(struct e1000_adapter *adapter);

@@ -132,10 +135,11 @@
 static struct net_device_stats * e1000_get_stats(struct net_device *netdev);
 static int e1000_change_mtu(struct net_device *netdev, int new_mtu);
 static int e1000_set_mac(struct net_device *netdev, void *p);
-static inline void e1000_irq_disable(struct e1000_adapter *adapter);
-static inline void e1000_irq_enable(struct e1000_adapter *adapter);
 static irqreturn_t e1000_intr(int irq, void *data, struct pt_regs *regs);
 static boolean_t e1000_clean_tx_irq(struct e1000_adapter *adapter);
+static inline void e1000_rx_checksum(struct e1000_adapter *adapter,
+                                     struct e1000_rx_desc *rx_desc,
+                                     struct sk_buff *skb);
 #ifdef CONFIG_E1000_NAPI
 static int e1000_clean(struct net_device *netdev, int *budget);
 static boolean_t e1000_clean_rx_irq(struct e1000_adapter *adapter,

@@ -150,9 +154,6 @@
 void set_ethtool_ops(struct net_device *netdev);
 static void e1000_enter_82542_rst(struct e1000_adapter *adapter);
 static void e1000_leave_82542_rst(struct e1000_adapter *adapter);
-static inline void e1000_rx_checksum(struct e1000_adapter *adapter,
-                                     struct e1000_rx_desc *rx_desc,
-                                     struct sk_buff *skb);
 static void e1000_tx_timeout(struct net_device *dev);
 static void e1000_tx_timeout_task(struct net_device *dev);
 static void e1000_smartspeed(struct e1000_adapter *adapter);

@@ -248,6 +249,34 @@
 module_exit(e1000_exit_module);
 
 
+/**
+ * e1000_irq_disable - Mask off interrupt generation on the NIC
+ * @adapter: board private structure
+ **/
+
+static inline void
+e1000_irq_disable(struct e1000_adapter *adapter)
+{
+	atomic_inc(&adapter->irq_sem);
+	E1000_WRITE_REG(&adapter->hw, IMC, ~0);
+	E1000_WRITE_FLUSH(&adapter->hw);
+	synchronize_irq(adapter->pdev->irq);
+}
+
+/**
+ * e1000_irq_enable - Enable default interrupt generation settings
+ * @adapter: board private structure
+ **/
+
+static inline void
+e1000_irq_enable(struct e1000_adapter *adapter)
+{
+	if(atomic_dec_and_test(&adapter->irq_sem)) {
+		E1000_WRITE_REG(&adapter->hw, IMS, IMS_ENABLE_MASK);
+		E1000_WRITE_FLUSH(&adapter->hw);
+	}
+}
+
 int
 e1000_up(struct e1000_adapter *adapter)
 {

@@ -2055,34 +2084,6 @@
 }
 
 /**
- * e1000_irq_disable - Mask off interrupt generation on the NIC
- * @adapter: board private structure
- **/
-
-static inline void
-e1000_irq_disable(struct e1000_adapter *adapter)
-{
-	atomic_inc(&adapter->irq_sem);
-	E1000_WRITE_REG(&adapter->hw, IMC, ~0);
-	E1000_WRITE_FLUSH(&adapter->hw);
-	synchronize_irq(adapter->pdev->irq);
-}
-
-/**
- * e1000_irq_enable - Enable default interrupt generation settings
- * @adapter: board private structure
- **/
-
-static inline void
-e1000_irq_enable(struct e1000_adapter *adapter)
-{
-	if(atomic_dec_and_test(&adapter->irq_sem)) {
-		E1000_WRITE_REG(&adapter->hw, IMS, IMS_ENABLE_MASK);
-		E1000_WRITE_FLUSH(&adapter->hw);
-	}
-}
-
-/**
  * e1000_intr - Interrupt Handler
  * @irq: interrupt number
  * @data: pointer to a network interface device structure

@@ -2227,6 +2228,41 @@
 }
 
 /**
+ * e1000_rx_checksum - Receive Checksum Offload for 82543
+ * @adapter: board private structure
+ * @rx_desc: receive descriptor
+ * @sk_buff: socket buffer with received data
+ **/
+
+static inline void
+e1000_rx_checksum(struct e1000_adapter *adapter,
+                  struct e1000_rx_desc *rx_desc,
+                  struct sk_buff *skb)
+{
+	/* 82543 or newer only */
+	if((adapter->hw.mac_type < e1000_82543) ||
+	/* Ignore Checksum bit is set */
+	(rx_desc->status & E1000_RXD_STAT_IXSM) ||
+	/* TCP Checksum has not been calculated */
+	(!(rx_desc->status & E1000_RXD_STAT_TCPCS))) {
+		skb->ip_summed = CHECKSUM_NONE;
+		return;
+	}
+
+	/* At this point we know the hardware did the TCP checksum */
+	/* now look at the TCP checksum error bit */
+	if(rx_desc->errors & E1000_RXD_ERR_TCPE) {
+		/* let the stack verify checksum errors */
+		skb->ip_summed = CHECKSUM_NONE;
+		adapter->hw_csum_err++;
+	} else {
+	/* TCP checksum is good */
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+		adapter->hw_csum_good++;
+	}
+}
+
+/**
  * e1000_clean_rx_irq - Send received data up the network stack,
  * @adapter: board private structure
  **/

@@ -2580,41 +2616,6 @@
 	return E1000_SUCCESS;
 }
 
-/**
- * e1000_rx_checksum - Receive Checksum Offload for 82543
- * @adapter: board private structure
- * @rx_desc: receive descriptor
- * @sk_buff: socket buffer with received data
- **/
-
-static inline void
-e1000_rx_checksum(struct e1000_adapter *adapter,
-                  struct e1000_rx_desc *rx_desc,
-                  struct sk_buff *skb)
-{
-	/* 82543 or newer only */
-	if((adapter->hw.mac_type < e1000_82543) ||
-	/* Ignore Checksum bit is set */
-	(rx_desc->status & E1000_RXD_STAT_IXSM) ||
-	/* TCP Checksum has not been calculated */
-	(!(rx_desc->status & E1000_RXD_STAT_TCPCS))) {
-		skb->ip_summed = CHECKSUM_NONE;
-		return;
-	}
-
-	/* At this point we know the hardware did the TCP checksum */
-	/* now look at the TCP checksum error bit */
-	if(rx_desc->errors & E1000_RXD_ERR_TCPE) {
-		/* let the stack verify checksum errors */
-		skb->ip_summed = CHECKSUM_NONE;
-		adapter->hw_csum_err++;
-	} else {
-	/* TCP checksum is good */
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
-		adapter->hw_csum_good++;
-	}
-}
-
 void
 e1000_pci_set_mwi(struct e1000_hw *hw)
 {
#1 lore raw
Quoted 29 lines · Adrian Bunk

/**
+ * e1000_rx_checksum - Receive Checksum Offload for 82543
+ * @adapter: board private structure
+ * @rx_desc: receive descriptor
+ * @sk_buff: socket buffer with received data
+ **/

drivers/net/e1000/e1000_main.c renamed from drivers/net/e1000/e1000_main.c.old +23 -0
+
+static inline void
+e1000_rx_checksum(struct e1000_adapter *adapter,
+                  struct e1000_rx_desc *rx_desc,
+                  struct sk_buff *skb)
+{
+	/* 82543 or newer only */
+	if((adapter->hw.mac_type < e1000_82543) ||
+	/* Ignore Checksum bit is set */
+	(rx_desc->status & E1000_RXD_STAT_IXSM) ||
+	/* TCP Checksum has not been calculated */
+	(!(rx_desc->status & E1000_RXD_STAT_TCPCS))) {
+		skb->ip_summed = CHECKSUM_NONE;
+		return;
+	}
+
+	/* At this point we know the hardware did the TCP checksum */
+	/* now look at the TCP checksum error bit */
+	if(rx_desc->errors & E1000_RXD_ERR_TCPE) {
+		/* let the stack verify checksum errors */
+		skb->ip_summed = CHECKSUM_NONE;
+		adapter->hw_csum_err++;
+	} else {

drivers/net/e1000/e1000_main.c line 2268

Quoted 10 lines
drivers/net/e1000/e1000_main.c renamed from drivers/net/e1000/e1000_main.c.old +7 -0
+	/* TCP checksum is good */
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+		adapter->hw_csum_good++;
+	}
+}
+
+/**
  * e1000_clean_rx_irq - Send received data up the network stack,
  * @adapter: board private structure
  **/

As you go thru them, consider removing inline keyword for such large functions.

Quoted 29 lines
drivers/net/e1000/e1000_main.c renamed from drivers/net/e1000/e1000_main.c.old +0 -25
@@ -2580,41 +2616,6 @@
 	return E1000_SUCCESS;
 }
 
-/**
- * e1000_rx_checksum - Receive Checksum Offload for 82543
- * @adapter: board private structure
- * @rx_desc: receive descriptor
- * @sk_buff: socket buffer with received data
- **/
-
-static inline void
-e1000_rx_checksum(struct e1000_adapter *adapter,
-                  struct e1000_rx_desc *rx_desc,
-                  struct sk_buff *skb)
-{
-	/* 82543 or newer only */
-	if((adapter->hw.mac_type < e1000_82543) ||
-	/* Ignore Checksum bit is set */
-	(rx_desc->status & E1000_RXD_STAT_IXSM) ||
-	/* TCP Checksum has not been calculated */
-	(!(rx_desc->status & E1000_RXD_STAT_TCPCS))) {
-		skb->ip_summed = CHECKSUM_NONE;
-		return;
-	}
-
-	/* At this point we know the hardware did the TCP checksum */
-	/* now look at the TCP checksum error bit */
-	if(rx_desc->errors & E1000_RXD_ERR_TCPE) {

drivers/net/e1000/e1000_main.c line 2617

Quoted 10 lines
drivers/net/e1000/e1000_main.c renamed from drivers/net/e1000/e1000_main.c.old +0 -10
-		/* let the stack verify checksum errors */
-		skb->ip_summed = CHECKSUM_NONE;
-		adapter->hw_csum_err++;
-	} else {
-	/* TCP checksum is good */
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
-		adapter->hw_csum_good++;
-	}
-}
-

--
vda

author
#2 lore raw
Quoted 4 lines · Denis Vlasenko

...
As you go thru them, consider removing inline keyword for such large functions.
...

I did propose this as an alternative approach in the text that accopagnied the patch.

My main reason for not directly proposing to remove the inlines was the fact that all inline functions were either very small or called only once.

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw
Quoted 10 lines · Denis Vlasenko
Quoted 9 lines · Adrian Bunk
Quoted 4 lines · Denis Vlasenko

...
As you go thru them, consider removing inline keyword for such large functions.
...

I did propose this as an alternative approach in the text that accopagnied the patch.

Quoted 6 lines
Quoted 3 lines

My main reason for not directly proposing to remove the inlines was the fact that all inline functions were either very small or called only once.

I think that large inlines with one callee is overoptimization and should not be done.

Unless I'm mistaken, it's simply equivalent to putting the code of the function at the place where the only call of the function currently is?

Or is there an additional problem I miss?

Quoted 1 line

vda

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#6 lore raw
Quoted 19 lines · Denis Vlasenko
Quoted 18 lines · Adrian Bunk
Quoted 16 lines · Denis Vlasenko
Quoted 6 lines · Adrian Bunk
Quoted 4 lines · Denis Vlasenko

...
As you go thru them, consider removing inline keyword for such large functions.
...

Quoted 6 lines

I did propose this as an alternative approach in the text that accopagnied the patch.

My main reason for not directly proposing to remove the inlines was the fact that all inline functions were either very small or called only once.

I think that large inlines with one callee is overoptimization and should not be done.

Quoted 8 lines
Quoted 4 lines

Unless I'm mistaken, it's simply equivalent to putting the code of the function at the place where the only call of the function currently is?

Or is there an additional problem I miss?

Yes. New gcc do that automagically for statics. It'll never 'autoinline' function with multiple callers.
...

But the way e1000_main.c is ordered, gcc can't inline such a function (due to -fno-unit-at-a-time, even gcc 3.4 cannot).

Quoted 1 line

vda

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed