| Message ID | 34a4527448b3a36c88d0c1c3cb9ad8661e693d7a.1307717675.git.richard.cochran@omicron.at |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 10 Jun 2011 17:06:59 +0200 Richard Cochran <richardcochran@gmail.com> wrote: > This commit adds a variation on netif_rx() designed to allow non-NAPI > Ethernet MAC drivers to support hardware time stamping in PHY devices. > Adapting a given driver requires two small changes, namely replacing > netif_rx() with netif_rx_defer() and adding a call to skb_tx_timestamp() > in the transmission path. > > Signed-off-by: Richard Cochran <richard.cochran@omicron.at> > --- > include/linux/netdevice.h | 21 +++++++++++++++++++++ > 1 files changed, 21 insertions(+), 0 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index ca333e7..9a56505 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -2066,6 +2066,27 @@ extern void dev_kfree_skb_any(struct sk_buff *skb); > #define HAVE_NETIF_RX 1 > extern int netif_rx(struct sk_buff *skb); > extern int netif_rx_ni(struct sk_buff *skb); > + > +/** > + * netif_rx_defer() - post buffer to the network code > + * @skb: buffer to post > + * > + * This function receives a packet from a device driver and queues it > + * for the upper (protocol) levels to process. All non-NAPI Ethernet > + * MAC drivers should use this instead of netif_rx() since this method > + * allows hardware timestamping to occur within the PHY. > + * > + * return values: > + * NET_RX_SUCCESS (no congestion) > + * NET_RX_DROP (packet was dropped) > + */ > +static inline int netif_rx_defer(struct sk_buff *skb) > +{ > + if (skb_defer_rx_timestamp(skb)) > + return NET_RX_SUCCESS; > + return netif_rx(skb); > +} Obvious question why not just put this in netif_rx. -- 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
On Fri, Jun 10, 2011 at 08:20:56AM -0700, Stephen Hemminger wrote: > On Fri, 10 Jun 2011 17:06:59 +0200 > Richard Cochran <richardcochran@gmail.com> wrote: > > > +static inline int netif_rx_defer(struct sk_buff *skb) > > +{ > > + if (skb_defer_rx_timestamp(skb)) > > + return NET_RX_SUCCESS; > > + return netif_rx(skb); > > +} > > Obvious question why not just put this in netif_rx. Well, if a packet gets defered, then that means that the PHY driver has decided to hold the packet until it obtains the time stamp from the PHY hardware. Then, the driver delivers the packet using netif_rx. So, we need to have two methods to deliver a frame, one with and one without the hook, otherwise you get packets going round in circles. Take a look at the one PHY driver using this (so far), on line 1017 of drivers/net/phy/dp83640.c, to see how it works. Thanks, Richard PS I did consider at renaming netif_rx to __netif_rx and then implementing netif_rx as shown above, but I found many, many callers of netif_rx which are not drivers, so I worry that bad side effects would appear from such a change. -- 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
On Fri, 10 Jun 2011 19:27:47 +0200 Richard Cochran <richardcochran@gmail.com> wrote: > On Fri, Jun 10, 2011 at 08:20:56AM -0700, Stephen Hemminger wrote: > > On Fri, 10 Jun 2011 17:06:59 +0200 > > Richard Cochran <richardcochran@gmail.com> wrote: > > > > > +static inline int netif_rx_defer(struct sk_buff *skb) > > > +{ > > > + if (skb_defer_rx_timestamp(skb)) > > > + return NET_RX_SUCCESS; > > > + return netif_rx(skb); > > > +} > > > > Obvious question why not just put this in netif_rx. > > Well, if a packet gets defered, then that means that the PHY driver > has decided to hold the packet until it obtains the time stamp from > the PHY hardware. Then, the driver delivers the packet using netif_rx. > > So, we need to have two methods to deliver a frame, one with and one > without the hook, otherwise you get packets going round in circles. > > Take a look at the one PHY driver using this (so far), on line 1017 of > drivers/net/phy/dp83640.c, to see how it works. > > Thanks, > Richard > > PS I did consider at renaming netif_rx to __netif_rx and then > implementing netif_rx as shown above, but I found many, many callers > of netif_rx which are not drivers, so I worry that bad side effects > would appear from such a change. Why not use a timestamp present flag like the receive hashing code already does. -- 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
From: Stephen Hemminger <shemminger@vyatta.com> Date: Fri, 10 Jun 2011 11:19:24 -0700 > On Fri, 10 Jun 2011 19:27:47 +0200 > Richard Cochran <richardcochran@gmail.com> wrote: > >> On Fri, Jun 10, 2011 at 08:20:56AM -0700, Stephen Hemminger wrote: >> > On Fri, 10 Jun 2011 17:06:59 +0200 >> > Richard Cochran <richardcochran@gmail.com> wrote: >> > >> > > +static inline int netif_rx_defer(struct sk_buff *skb) >> > > +{ >> > > + if (skb_defer_rx_timestamp(skb)) >> > > + return NET_RX_SUCCESS; >> > > + return netif_rx(skb); >> > > +} >> > >> > Obvious question why not just put this in netif_rx. >> >> Well, if a packet gets defered, then that means that the PHY driver >> has decided to hold the packet until it obtains the time stamp from >> the PHY hardware. Then, the driver delivers the packet using netif_rx. >> >> So, we need to have two methods to deliver a frame, one with and one >> without the hook, otherwise you get packets going round in circles. >> >> Take a look at the one PHY driver using this (so far), on line 1017 of >> drivers/net/phy/dp83640.c, to see how it works. >> >> Thanks, >> Richard >> >> PS I did consider at renaming netif_rx to __netif_rx and then >> implementing netif_rx as shown above, but I found many, many callers >> of netif_rx which are not drivers, so I worry that bad side effects >> would appear from such a change. > > Why not use a timestamp present flag like the receive hashing code > already does. I agree with Stephen that we should be decreasing the number of driver interfaces not increasing them. Also, it makes no sense to add this for obsolete RX processing such that netif_rx() is. If drivers want to add fancy features like this timestamping stuff, they better move on to NAPI, GRO, etc. first. Putting support for new features into deprecating things like netif_rx() makes no sense at all. -- 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
On Sat, Jun 11, 2011 at 04:10:25PM -0700, David Miller wrote: > > Also, it makes no sense to add this for obsolete RX processing such > that netif_rx() is. > > If drivers want to add fancy features like this timestamping stuff, > they better move on to NAPI, GRO, etc. first. Putting support for > new features into deprecating things like netif_rx() makes no > sense at all. Okay, I see your point. I won't bother trying to improve the "academy of ancient drivers," and I'll repost without the netif_rx wrapper. However, I do want to support the coldfire fec driver, since Freescale is selling two coldfire development boards with the dp83640 phy. But I don't think it makes sense to try and upgrade the fec driver to napi, when a simple "if !skb_defer_rx_timestamp" will do there. Thanks, Richard -- 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 --git a/include/linux/netdevice.h b/include/linux/netdevice.h index ca333e7..9a56505 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2066,6 +2066,27 @@ extern void dev_kfree_skb_any(struct sk_buff *skb); #define HAVE_NETIF_RX 1 extern int netif_rx(struct sk_buff *skb); extern int netif_rx_ni(struct sk_buff *skb); + +/** + * netif_rx_defer() - post buffer to the network code + * @skb: buffer to post + * + * This function receives a packet from a device driver and queues it + * for the upper (protocol) levels to process. All non-NAPI Ethernet + * MAC drivers should use this instead of netif_rx() since this method + * allows hardware timestamping to occur within the PHY. + * + * return values: + * NET_RX_SUCCESS (no congestion) + * NET_RX_DROP (packet was dropped) + */ +static inline int netif_rx_defer(struct sk_buff *skb) +{ + if (skb_defer_rx_timestamp(skb)) + return NET_RX_SUCCESS; + return netif_rx(skb); +} + #define HAVE_NETIF_RECEIVE_SKB 1 extern int netif_receive_skb(struct sk_buff *skb); extern gro_result_t dev_gro_receive(struct napi_struct *napi,
This commit adds a variation on netif_rx() designed to allow non-NAPI Ethernet MAC drivers to support hardware time stamping in PHY devices. Adapting a given driver requires two small changes, namely replacing netif_rx() with netif_rx_defer() and adding a call to skb_tx_timestamp() in the transmission path. Signed-off-by: Richard Cochran <richard.cochran@omicron.at> --- include/linux/netdevice.h | 21 +++++++++++++++++++++ 1 files changed, 21 insertions(+), 0 deletions(-)