diff mbox

[01/19] netdev: change transmit to limited range type

Message ID 20090901055128.466779045@vyatta.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

stephen hemminger Sept. 1, 2009, 5:50 a.m. UTC
The transmit function should only return one of three possible values,
some drivers got confused and returned errno's or other values.
This changes the definition so that this can be caught at compile time.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
The patch will produce warnings until drivers are converted.


---
 include/linux/netdevice.h |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Eric Dumazet Sept. 1, 2009, 7:39 a.m. UTC | #1
Stephen Hemminger a écrit :
> The transmit function should only return one of three possible values,
> some drivers got confused and returned errno's or other values.
> This changes the definition so that this can be caught at compile time.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
> The patch will produce warnings until drivers are converted.
> 
> 
> ---
>  include/linux/netdevice.h |   22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> --- a/include/linux/netdevice.h	2009-08-31 16:47:58.703591891 -0700
> +++ b/include/linux/netdevice.h	2009-08-31 22:24:53.101472780 -0700
> @@ -79,17 +79,19 @@ struct wireless_dev;
>  #define net_xmit_eval(e)	((e) == NET_XMIT_CN? 0 : (e))
>  #define net_xmit_errno(e)	((e) != NET_XMIT_CN ? -ENOBUFS : 0)
>  
> +/* Driver transmit return codes */
> +enum netdev_tx {
> +	NETDEV_TX_OK = 0,	/* driver took care of packet */
> +	NETDEV_TX_BUSY,		/* driver tx path was busy*/
> +	NETDEV_TX_LOCKED = -1,	/* driver tx lock was already taken */
> +};
> +typedef enum netdev_tx netdev_tx_t;
> +
>  #endif
>  
>  #define MAX_ADDR_LEN	32		/* Largest hardware address length */
>  
> -/* Driver transmit return codes */
> -#define NETDEV_TX_OK 0		/* driver took care of packet */
> -#define NETDEV_TX_BUSY 1	/* driver tx path was busy*/
> -#define NETDEV_TX_LOCKED -1	/* driver tx lock was already taken */
> -
>  #ifdef  __KERNEL__
> -
>  /*
>   *	Compute the worst case header length according to the protocols
>   *	used.
> @@ -507,9 +509,11 @@ struct netdev_queue {
>   *     This function is called when network device transistions to the down
>   *     state.
>   *
> - * int (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev);
> + * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
> + *                               struct net_device *dev);
>   *	Called when a packet needs to be transmitted.
> - *	Must return NETDEV_TX_OK , NETDEV_TX_BUSY, or NETDEV_TX_LOCKED,
> + *	Must return NETDEV_TX_OK , NETDEV_TX_BUSY.
> + *        (can also return NETDEV_TX_LOCKED iff NETIF_F_LLTX)
>   *	Required can not be NULL.
>   *
>   * u16 (*ndo_select_queue)(struct net_device *dev, struct sk_buff *skb);
> @@ -580,7 +584,7 @@ struct net_device_ops {
>  	void			(*ndo_uninit)(struct net_device *dev);
>  	int			(*ndo_open)(struct net_device *dev);
>  	int			(*ndo_stop)(struct net_device *dev);
> -	int			(*ndo_start_xmit) (struct sk_buff *skb,
> +	netdev_tx_t		(*ndo_start_xmit) (struct sk_buff *skb,
>  						   struct net_device *dev);
>  	u16			(*ndo_select_queue)(struct net_device *dev,
>  						    struct sk_buff *skb);
> 

I was wondering if we could add a third parameter to 
ndo_start_xmit() (and pass the struct netdev_queue *txq)
in a smooth way, but could not find a solution.

Unconverted drivers would not care of this third param; but this
should be provided by callers (like dev_hard_start_xmit())

Most multiqueue drivers have to recompute it, and even monoqueue drivers
have to call netif_stop_queue(dev)/netif_queue_stopped(dev), all
using netdev_get_tx_queue(dev, 0) in the end...

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Sept. 1, 2009, 8:18 a.m. UTC | #2
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 01 Sep 2009 09:39:47 +0200

> I was wondering if we could add a third parameter to 
> ndo_start_xmit() (and pass the struct netdev_queue *txq)
> in a smooth way, but could not find a solution.

Don't think that others didn't think about this too :-)

> Unconverted drivers would not care of this third param; but this
> should be provided by callers (like dev_hard_start_xmit())
>
> Most multiqueue drivers have to recompute it, and even monoqueue drivers
> have to call netif_stop_queue(dev)/netif_queue_stopped(dev), all
> using netdev_get_tx_queue(dev, 0) in the end...

At least that is a constant structure offset reference, and thus
computed at compile time.

Anyways, I like Stephen's netdev_tx_t changes and I'm build testing
them now in my net-next-2.6
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- a/include/linux/netdevice.h	2009-08-31 16:47:58.703591891 -0700
+++ b/include/linux/netdevice.h	2009-08-31 22:24:53.101472780 -0700
@@ -79,17 +79,19 @@  struct wireless_dev;
 #define net_xmit_eval(e)	((e) == NET_XMIT_CN? 0 : (e))
 #define net_xmit_errno(e)	((e) != NET_XMIT_CN ? -ENOBUFS : 0)
 
+/* Driver transmit return codes */
+enum netdev_tx {
+	NETDEV_TX_OK = 0,	/* driver took care of packet */
+	NETDEV_TX_BUSY,		/* driver tx path was busy*/
+	NETDEV_TX_LOCKED = -1,	/* driver tx lock was already taken */
+};
+typedef enum netdev_tx netdev_tx_t;
+
 #endif
 
 #define MAX_ADDR_LEN	32		/* Largest hardware address length */
 
-/* Driver transmit return codes */
-#define NETDEV_TX_OK 0		/* driver took care of packet */
-#define NETDEV_TX_BUSY 1	/* driver tx path was busy*/
-#define NETDEV_TX_LOCKED -1	/* driver tx lock was already taken */
-
 #ifdef  __KERNEL__
-
 /*
  *	Compute the worst case header length according to the protocols
  *	used.
@@ -507,9 +509,11 @@  struct netdev_queue {
  *     This function is called when network device transistions to the down
  *     state.
  *
- * int (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev);
+ * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
+ *                               struct net_device *dev);
  *	Called when a packet needs to be transmitted.
- *	Must return NETDEV_TX_OK , NETDEV_TX_BUSY, or NETDEV_TX_LOCKED,
+ *	Must return NETDEV_TX_OK , NETDEV_TX_BUSY.
+ *        (can also return NETDEV_TX_LOCKED iff NETIF_F_LLTX)
  *	Required can not be NULL.
  *
  * u16 (*ndo_select_queue)(struct net_device *dev, struct sk_buff *skb);
@@ -580,7 +584,7 @@  struct net_device_ops {
 	void			(*ndo_uninit)(struct net_device *dev);
 	int			(*ndo_open)(struct net_device *dev);
 	int			(*ndo_stop)(struct net_device *dev);
-	int			(*ndo_start_xmit) (struct sk_buff *skb,
+	netdev_tx_t		(*ndo_start_xmit) (struct sk_buff *skb,
 						   struct net_device *dev);
 	u16			(*ndo_select_queue)(struct net_device *dev,
 						    struct sk_buff *skb);