| Message ID | 1345715813-20757-12-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Aug 23, 2012 at 02:56:51AM -0700, Jeff Kirsher wrote: > From: Matthew Vick <matthew.vick@intel.com> > > Where possible, move PTP-related functions into igb_ptp.c and update the > names of functions and variables to match the established coding style > in the files and specify that they are PTP-specific. Function renaming as an end in itself? Sounds like churn to me. Also, I disagree with some of the displacements. > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c > index 6adb0f7..d1a120e 100644 > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c > @@ -2280,21 +2280,8 @@ static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data) > } > } > > -static int igb_ethtool_begin(struct net_device *netdev) > -{ > - struct igb_adapter *adapter = netdev_priv(netdev); > - pm_runtime_get_sync(&adapter->pdev->dev); > - return 0; > -} > - > -static void igb_ethtool_complete(struct net_device *netdev) > -{ > - struct igb_adapter *adapter = netdev_priv(netdev); > - pm_runtime_put(&adapter->pdev->dev); > -} > - Why have you moved this block? How are these "PTP-related"? > #ifdef CONFIG_IGB_PTP > -static int igb_ethtool_get_ts_info(struct net_device *dev, > +static int igb_get_ts_info(struct net_device *dev, I like the old name better. > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c ... > -#ifdef CONFIG_IGB_PTP > -/** > - * igb_tx_hwtstamp - utility function which checks for TX time stamp > - * @q_vector: pointer to q_vector containing needed info > - * @buffer: pointer to igb_tx_buffer structure > - * > - * If we were asked to do hardware stamping and such a time stamp is > - * available, then it must have been for this skb here because we only > - * allow only one such packet into the queue. > - */ > -static void igb_tx_hwtstamp(struct igb_q_vector *q_vector, > - struct igb_tx_buffer *buffer_info) > -{ > - struct igb_adapter *adapter = q_vector->adapter; > - struct e1000_hw *hw = &adapter->hw; > - struct skb_shared_hwtstamps shhwtstamps; > - u64 regval; > - > - /* if skb does not support hw timestamp or TX stamp not valid exit */ > - if (likely(!(buffer_info->tx_flags & IGB_TX_FLAGS_TSTAMP)) || > - !(rd32(E1000_TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID)) > - return; > - > - regval = rd32(E1000_TXSTMPL); > - regval |= (u64)rd32(E1000_TXSTMPH) << 32; > - > - igb_systim_to_hwtstamp(adapter, &shhwtstamps, regval); > - skb_tstamp_tx(buffer_info->skb, &shhwtstamps); > -} > -#endif /* CONFIG_IGB_PTP */ > - Here you have taken a static local function and made it into a global function. This can have performance impacts. > /** > * igb_clean_tx_irq - Reclaim resources after transmit completes > * @q_vector: pointer to q_vector containing needed info > @@ -5827,7 +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector) > > #ifdef CONFIG_IGB_PTP > /* retrieve hardware timestamp */ > - igb_tx_hwtstamp(q_vector, tx_buffer); > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer); This name stinks, too. You know that you can have time stamping all by itself, right? It is logically separate from the ptp clock stuff. This patch doesn't really improve the driver at all, IMHO. 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
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Thursday, August 23, 2012 4:16 AM > To: Kirsher, Jeffrey T > Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org; > gospo@redhat.com; sassmann@redhat.com > Subject: Re: [net-next 11/13] igb: Update PTP function names/variables > and locations. > > On Thu, Aug 23, 2012 at 02:56:51AM -0700, Jeff Kirsher wrote: > > From: Matthew Vick <matthew.vick@intel.com> > > > > Where possible, move PTP-related functions into igb_ptp.c and update > > the names of functions and variables to match the established coding > > style in the files and specify that they are PTP-specific. > > Function renaming as an end in itself? Sounds like churn to me. Also, I > disagree with some of the displacements. The goal was to make it clear what was required for the PTP flow and what wasn't. Again, ixgbe was my reference in this. Personally, I disagree with the old naming scheme. > > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c > > b/drivers/net/ethernet/intel/igb/igb_ethtool.c > > index 6adb0f7..d1a120e 100644 > > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c > > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c > > @@ -2280,21 +2280,8 @@ static void igb_get_strings(struct net_device > *netdev, u32 stringset, u8 *data) > > } > > } > > > > -static int igb_ethtool_begin(struct net_device *netdev) -{ > > - struct igb_adapter *adapter = netdev_priv(netdev); > > - pm_runtime_get_sync(&adapter->pdev->dev); > > - return 0; > > -} > > - > > -static void igb_ethtool_complete(struct net_device *netdev) -{ > > - struct igb_adapter *adapter = netdev_priv(netdev); > > - pm_runtime_put(&adapter->pdev->dev); > > -} > > - > > Why have you moved this block? How are these "PTP-related"? This is just git's diff being silly. :) Rather than move igb_get_ts_info up, git thought I moved those other functions down. I wanted igb_get_ts_info to be with the other get functions. > > #ifdef CONFIG_IGB_PTP > > -static int igb_ethtool_get_ts_info(struct net_device *dev, > > +static int igb_get_ts_info(struct net_device *dev, > > I like the old name better. The old name is out of the coding style of igb. Every other function is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and igb_ethtool_complete. Are you suggesting we add _ethtool to every ethtool function in igb? > > -#ifdef CONFIG_IGB_PTP > > -/** > > - * igb_tx_hwtstamp - utility function which checks for TX time stamp > > - * @q_vector: pointer to q_vector containing needed info > > - * @buffer: pointer to igb_tx_buffer structure > > - * > > - * If we were asked to do hardware stamping and such a time stamp is > > - * available, then it must have been for this skb here because we > > only > > - * allow only one such packet into the queue. > > - */ > > -static void igb_tx_hwtstamp(struct igb_q_vector *q_vector, > > - struct igb_tx_buffer *buffer_info) > > -{ > > - struct igb_adapter *adapter = q_vector->adapter; > > - struct e1000_hw *hw = &adapter->hw; > > - struct skb_shared_hwtstamps shhwtstamps; > > - u64 regval; > > - > > - /* if skb does not support hw timestamp or TX stamp not valid > exit */ > > - if (likely(!(buffer_info->tx_flags & IGB_TX_FLAGS_TSTAMP)) || > > - !(rd32(E1000_TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID)) > > - return; > > - > > - regval = rd32(E1000_TXSTMPL); > > - regval |= (u64)rd32(E1000_TXSTMPH) << 32; > > - > > - igb_systim_to_hwtstamp(adapter, &shhwtstamps, regval); > > - skb_tstamp_tx(buffer_info->skb, &shhwtstamps); > > -} > > -#endif /* CONFIG_IGB_PTP */ > > - > > Here you have taken a static local function and made it into a global > function. This can have performance impacts. Which, this function calls igb_systim_to_hwtstamp anyway, which is global. Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't even be called in clean_tx_irq, FWIW. > > /** > > * igb_clean_tx_irq - Reclaim resources after transmit completes > > * @q_vector: pointer to q_vector containing needed info @@ -5827,7 > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector > *q_vector) > > > > #ifdef CONFIG_IGB_PTP > > /* retrieve hardware timestamp */ > > - igb_tx_hwtstamp(q_vector, tx_buffer); > > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer); > > This name stinks, too. You know that you can have time stamping all by > itself, right? It is logically separate from the ptp clock stuff. > > This patch doesn't really improve the driver at all, IMHO. > > Thanks, > Richard Yes, I'm aware. But, as it stands today, we don't use it for anything else. If the function is feature specific, then we should be calling it out as such. I'm sorry you feel like this patch doesn't improve the driver. The goal is code cleanup and consistency, both of which I consider to be driver improvements and is why I made the patches. Cheers, Matthew -- 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 Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote: > > > #ifdef CONFIG_IGB_PTP > > > -static int igb_ethtool_get_ts_info(struct net_device *dev, > > > +static int igb_get_ts_info(struct net_device *dev, > > > > I like the old name better. > > The old name is out of the coding style of igb. Every other function is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and igb_ethtool_complete. Are you suggesting we add _ethtool to every ethtool function in igb? No, just leave the names alone, and keep the functions where they are. It is just churn. One of the most useful ways to understand code (at least for me) is to use git blame. It tells you when code was added, what the reason was, and how the change looks in context. By moving and renaming willy nilly, you are obscuring this valuable information. > > > -#ifdef CONFIG_IGB_PTP > > > -/** > > > - * igb_tx_hwtstamp - utility function which checks for TX time stamp > > > - * @q_vector: pointer to q_vector containing needed info > > > - * @buffer: pointer to igb_tx_buffer structure > > > - * > > > - * If we were asked to do hardware stamping and such a time stamp is > > > - * available, then it must have been for this skb here because we > > > only > > > - * allow only one such packet into the queue. > > > - */ > > > -static void igb_tx_hwtstamp(struct igb_q_vector *q_vector, > > > - struct igb_tx_buffer *buffer_info) > > > -{ > > > - struct igb_adapter *adapter = q_vector->adapter; > > > - struct e1000_hw *hw = &adapter->hw; > > > - struct skb_shared_hwtstamps shhwtstamps; > > > - u64 regval; > > > - > > > - /* if skb does not support hw timestamp or TX stamp not valid > > exit */ > > > - if (likely(!(buffer_info->tx_flags & IGB_TX_FLAGS_TSTAMP)) || > > > - !(rd32(E1000_TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID)) > > > - return; > > > - > > > - regval = rd32(E1000_TXSTMPL); > > > - regval |= (u64)rd32(E1000_TXSTMPH) << 32; > > > - > > > - igb_systim_to_hwtstamp(adapter, &shhwtstamps, regval); > > > - skb_tstamp_tx(buffer_info->skb, &shhwtstamps); > > > -} > > > -#endif /* CONFIG_IGB_PTP */ > > > - > > > > Here you have taken a static local function and made it into a global > > function. This can have performance impacts. > > Which, this function calls igb_systim_to_hwtstamp anyway, which is > global. So how does calling two global functions in series improve performance? > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't > even be called in clean_tx_irq, FWIW. If this is part of some larger plan, then it would help to see that plan. > > > /** > > > * igb_clean_tx_irq - Reclaim resources after transmit completes > > > * @q_vector: pointer to q_vector containing needed info @@ -5827,7 > > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector > > *q_vector) > > > > > > #ifdef CONFIG_IGB_PTP > > > /* retrieve hardware timestamp */ > > > - igb_tx_hwtstamp(q_vector, tx_buffer); > > > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer); > > > > This name stinks, too. You know that you can have time stamping all by > > itself, right? It is logically separate from the ptp clock stuff. > > > > This patch doesn't really improve the driver at all, IMHO. > > > > Thanks, > > Richard > > Yes, I'm aware. But, as it stands today, we don't use it for anything else. If the function is feature specific, then we should be calling it out as such. Right now the time stamping is being equated with the clock functions, but it really should be decoupled. The 82580 can time stamp every received packet, which can be interesting for performance monitoring, even without PTP (and adding *that* would be a useful change). > I'm sorry you feel like this patch doesn't improve the driver. The goal is code cleanup and consistency, both of which I consider to be driver improvements and is why I made the patches. But the code wasn't dirty in the first place. It doesn't need this "cleaning." This series undoes the inline-able functions for no good reason. As far as ixgbe goes, this driver came first, so you might as well be making *that* driver consistent with this one. 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
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] > On Behalf Of Richard Cochran > Sent: Thursday, August 23, 2012 10:54 AM > To: Vick, Matthew > Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org; > gospo@redhat.com; sassmann@redhat.com > Subject: Re: [net-next 11/13] igb: Update PTP function names/variables and > locations. > > On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote: > > > > > #ifdef CONFIG_IGB_PTP > > > > -static int igb_ethtool_get_ts_info(struct net_device *dev, > > > > +static int igb_get_ts_info(struct net_device *dev, > > > > > > I like the old name better. > > > > The old name is out of the coding style of igb. Every other function is > igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and > igb_ethtool_complete. Are you suggesting we add _ethtool to every ethtool > function in igb? > > No, just leave the names alone, and keep the functions where they are. It > is just churn. > > One of the most useful ways to understand code (at least for me) is to use > git blame. It tells you when code was added, what the reason was, and how > the change looks in context. By moving and renaming willy nilly, you are > obscuring this valuable information. > > > > > -#ifdef CONFIG_IGB_PTP > > > > -/** > > > > - * igb_tx_hwtstamp - utility function which checks for TX time > > > > stamp > > > > - * @q_vector: pointer to q_vector containing needed info > > > > - * @buffer: pointer to igb_tx_buffer structure > > > > - * > > > > - * If we were asked to do hardware stamping and such a time stamp > > > > is > > > > - * available, then it must have been for this skb here because we > > > > only > > > > - * allow only one such packet into the queue. > > > > - */ > > > > -static void igb_tx_hwtstamp(struct igb_q_vector *q_vector, > > > > - struct igb_tx_buffer *buffer_info) > > > > -{ > > > > - struct igb_adapter *adapter = q_vector->adapter; > > > > - struct e1000_hw *hw = &adapter->hw; > > > > - struct skb_shared_hwtstamps shhwtstamps; > > > > - u64 regval; > > > > - > > > > - /* if skb does not support hw timestamp or TX stamp not valid > > > exit */ > > > > - if (likely(!(buffer_info->tx_flags & IGB_TX_FLAGS_TSTAMP)) || > > > > - !(rd32(E1000_TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID)) > > > > - return; > > > > - > > > > - regval = rd32(E1000_TXSTMPL); > > > > - regval |= (u64)rd32(E1000_TXSTMPH) << 32; > > > > - > > > > - igb_systim_to_hwtstamp(adapter, &shhwtstamps, regval); > > > > - skb_tstamp_tx(buffer_info->skb, &shhwtstamps); > > > > -} > > > > -#endif /* CONFIG_IGB_PTP */ > > > > - > > > > > > Here you have taken a static local function and made it into a > > > global function. This can have performance impacts. > > > > Which, this function calls igb_systim_to_hwtstamp anyway, which is > > global. > > So how does calling two global functions in series improve performance? > > > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't > > even be called in clean_tx_irq, FWIW. > > If this is part of some larger plan, then it would help to see that plan. > > > > > /** > > > > * igb_clean_tx_irq - Reclaim resources after transmit completes > > > > * @q_vector: pointer to q_vector containing needed info @@ > > > > -5827,7 > > > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector > > > *q_vector) > > > > > > > > #ifdef CONFIG_IGB_PTP > > > > /* retrieve hardware timestamp */ > > > > - igb_tx_hwtstamp(q_vector, tx_buffer); > > > > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer); > > > > > > This name stinks, too. You know that you can have time stamping all > > > by itself, right? It is logically separate from the ptp clock stuff. > > > > > > This patch doesn't really improve the driver at all, IMHO. > > > > > > Thanks, > > > Richard > > > > Yes, I'm aware. But, as it stands today, we don't use it for anything > else. If the function is feature specific, then we should be calling it > out as such. > > Right now the time stamping is being equated with the clock functions, but > it really should be decoupled. The 82580 can time stamp every received > packet, which can be interesting for performance monitoring, even without > PTP (and adding *that* would be a useful change). > The timestamp all does not really work with the ptp clock features gone, because you don't have the clock. You can't equate the time values of the packets when the clock isn't synched to something meaningful. Yes that does not require PTP adjustment functions, but it does require the SYSTIME setup and some method to get the clock correct, which currently is only done in the PTP init sequence. Timestamp all packets also can cause a performance hit when used with certain workloads. > > I'm sorry you feel like this patch doesn't improve the driver. The goal > is code cleanup and consistency, both of which I consider to be driver > improvements and is why I made the patches. > > But the code wasn't dirty in the first place. It doesn't need this > "cleaning." This series undoes the inline-able functions for no good > reason. As far as ixgbe goes, this driver came first, so you might as well > be making *that* driver consistent with this one. ixgbe hardware is (currently) even more closely synched with PTP for the register bits so it does make some sense for ixgbe to remain the way it is. Right now the igb features are partially synched (even before this change) in odd ways. The time values returned when PHC information is disabled are basically only useful for comparing between themselves, not with any meaningful clock on the device. > > 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 -- 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 Thu, Aug 23, 2012 at 06:00:37PM +0000, Keller, Jacob E wrote: > > > > Right now the time stamping is being equated with the clock functions, but > > it really should be decoupled. The 82580 can time stamp every received > > packet, which can be interesting for performance monitoring, even without > > PTP (and adding *that* would be a useful change). > > > The timestamp all does not really work with the ptp clock features > gone, because you don't have the clock. You can't equate the time > values of the packets when the clock isn't synched to something > meaningful. Yes that does not require PTP adjustment functions, but > it does require the SYSTIME setup and some method to get the clock > correct, which currently is only done in the PTP init sequence. Relative, high resolution time stamps can be interesting all by themselves. That is why wireshark has a whole menu of timing choices including relative since start, inter-packet, and so on. > Timestamp all packets also can cause a performance hit when used with certain workloads. Only when enabled. > ixgbe hardware is (currently) even more closely synched with PTP for the register bits so it does make some sense for ixgbe to remain the way it is. Right now the igb features are partially synched (even before this change) in odd ways. The time values returned when PHC information is disabled are basically only useful for comparing between themselves, not with any meaningful clock on the device. Yes, I agree that igb is a bit oddly synced WRT clock and time stamping. I would welcome a change to let it have HW time stamping as an independent feature. 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
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Thursday, August 23, 2012 10:54 AM > To: Vick, Matthew > Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org; > gospo@redhat.com; sassmann@redhat.com > Subject: Re: [net-next 11/13] igb: Update PTP function names/variables > and locations. > > On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote: > > > > > #ifdef CONFIG_IGB_PTP > > > > -static int igb_ethtool_get_ts_info(struct net_device *dev, > > > > +static int igb_get_ts_info(struct net_device *dev, > > > > > > I like the old name better. > > > > The old name is out of the coding style of igb. Every other function > is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and > igb_ethtool_complete. Are you suggesting we add _ethtool to every > ethtool function in igb? > > No, just leave the names alone, and keep the functions where they are. > It is just churn. > > One of the most useful ways to understand code (at least for me) is to > use git blame. It tells you when code was added, what the reason was, > and how the change looks in context. By moving and renaming willy > nilly, you are obscuring this valuable information. Repairing is not churn. I think it's better to make the code clearer than require developers use git blame to figure out who named a function in a silly way. I agree, it kind of stinks to lose that history, but I'd rather not skip making the right change because of git blame concerns. > > [...] > > > > > Which, this function calls igb_systim_to_hwtstamp anyway, which is > > global. > > So how does calling two global functions in series improve performance? It isn't calling two global functions. It's calling a global function that calls a static function. > > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't > > even be called in clean_tx_irq, FWIW. > > If this is part of some larger plan, then it would help to see that > plan. Definitely fair enough. I will work with Jeff to try and push the whole series next time (at the very least, I'd like to re-spin the series for the ethtool query patch). > > > > /** > > > > * igb_clean_tx_irq - Reclaim resources after transmit completes > > > > * @q_vector: pointer to q_vector containing needed info @@ > > > > -5827,7 > > > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector > > > *q_vector) > > > > > > > > #ifdef CONFIG_IGB_PTP > > > > /* retrieve hardware timestamp */ > > > > - igb_tx_hwtstamp(q_vector, tx_buffer); > > > > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer); > > > > > > This name stinks, too. You know that you can have time stamping all > > > by itself, right? It is logically separate from the ptp clock > stuff. > > > > > > This patch doesn't really improve the driver at all, IMHO. > > > > > > Thanks, > > > Richard > > > > Yes, I'm aware. But, as it stands today, we don't use it for anything > else. If the function is feature specific, then we should be calling it > out as such. > > Right now the time stamping is being equated with the clock functions, > but it really should be decoupled. The 82580 can time stamp every > received packet, which can be interesting for performance monitoring, > even without PTP (and adding *that* would be a useful change). As Jake said, there's no support for hardware timestamping without PTP today. As such, if the function is only useful for that feature, then I think it's less ambiguous to leave it the way I've coded it today. If we de-couple hardware timestamping and clock synchronization, which I wouldn't be opposed to, then the function name can reflect becoming more generic. As it stands, I'd rather not call something generic when it isn't. I think it's misleading, personally. > > I'm sorry you feel like this patch doesn't improve the driver. The > goal is code cleanup and consistency, both of which I consider to be > driver improvements and is why I made the patches. > > But the code wasn't dirty in the first place. It doesn't need this > "cleaning." This series undoes the inline-able functions for no good > reason. As far as ixgbe goes, this driver came first, so you might as > well be making *that* driver consistent with this one. > > Thanks, > Richard Actually, yes, the code *was* dirty, because it breaks the coding style of the rest of the driver and isn't as self-documenting as it could be, and the inline-able functions you claim I'm destroying called global functions anyway. Also, just because igb came first doesn't mean we should ignore porting better or clearer solutions from later drivers. If something is better or clearer, I think we should use be using it. Cheers, Matthew -- 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
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Thursday, August 23, 2012 11:11 AM > To: Keller, Jacob E > Cc: Vick, Matthew; Kirsher, Jeffrey T; davem@davemloft.net; > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com > Subject: Re: [net-next 11/13] igb: Update PTP function names/variables > and locations. > > [...] > > Relative, high resolution time stamps can be interesting all by > themselves. That is why wireshark has a whole menu of timing choices > including relative since start, inter-packet, and so on. > > [...] > > Yes, I agree that igb is a bit oddly synced WRT clock and time > stamping. I would welcome a change to let it have HW time stamping as > an independent feature. > > Thanks, > Richard Isn't this discussion creeping outside the scope of this patch? My aim here with this patch is to help tidy up PTP code as it is today. As it is today, igb has no support for hardware timestamping without PTP. If you or someone else writes a follow-on patch to add hardware timestamping without PTP, then they can move and rename the functions accordingly (and I'd personally really appreciate it if they did rename it, since a generic function should be named like a generic function and a PTP function should be named like a PTP function). Cheers, Matthew -- 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
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] > On Behalf Of Vick, Matthew > Sent: Thursday, August 23, 2012 11:35 AM > To: Richard Cochran > Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org; > gospo@redhat.com; sassmann@redhat.com > Subject: RE: [net-next 11/13] igb: Update PTP function names/variables and > locations. > > > -----Original Message----- > > From: Richard Cochran [mailto:richardcochran@gmail.com] > > Sent: Thursday, August 23, 2012 10:54 AM > > To: Vick, Matthew > > Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org; > > gospo@redhat.com; sassmann@redhat.com > > Subject: Re: [net-next 11/13] igb: Update PTP function names/variables > > and locations. > > > > On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote: > > > > > > > #ifdef CONFIG_IGB_PTP > > > > > -static int igb_ethtool_get_ts_info(struct net_device *dev, > > > > > +static int igb_get_ts_info(struct net_device *dev, > > > > > > > > I like the old name better. > > > > > > The old name is out of the coding style of igb. Every other function > > is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and > > igb_ethtool_complete. Are you suggesting we add _ethtool to every > > ethtool function in igb? > > > > No, just leave the names alone, and keep the functions where they are. > > It is just churn. > > > > One of the most useful ways to understand code (at least for me) is to > > use git blame. It tells you when code was added, what the reason was, > > and how the change looks in context. By moving and renaming willy > > nilly, you are obscuring this valuable information. > > Repairing is not churn. I think it's better to make the code clearer than > require developers use git blame to figure out who named a function in a > silly way. I agree, it kind of stinks to lose that history, but I'd rather > not skip making the right change because of git blame concerns. The history isn't lost, it is just obscured. I agree if the change is necessary it should be done. I think we all disagree on what is necessary. I would prefer function names to match. Richard has a point regarding the time stamping all packets, however again the ioctl turning that on tends to be quite PTP centric already. I think that value isn't necessarily added due to the current coupling of the features. It is partially coupled by the hardware anyways. Effectively with the PTP framework disabled you have a half useful feature that is enabled by a strange ioctl that has a lot of PTP specific names, and doesn't always do what you want. I am not sure how much work would be required to get to a state where the separation of function makes sense. - Jake > > > > > [...] > > > > > > > > Which, this function calls igb_systim_to_hwtstamp anyway, which is > > > global. > > > > So how does calling two global functions in series improve performance? > > It isn't calling two global functions. It's calling a global function that > calls a static function. > > > > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't > > > even be called in clean_tx_irq, FWIW. > > > > If this is part of some larger plan, then it would help to see that > > plan. > > Definitely fair enough. I will work with Jeff to try and push the whole > series next time (at the very least, I'd like to re-spin the series for > the ethtool query patch). > > > > > > /** > > > > > * igb_clean_tx_irq - Reclaim resources after transmit completes > > > > > * @q_vector: pointer to q_vector containing needed info @@ > > > > > -5827,7 > > > > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector > > > > *q_vector) > > > > > > > > > > #ifdef CONFIG_IGB_PTP > > > > > /* retrieve hardware timestamp */ > > > > > - igb_tx_hwtstamp(q_vector, tx_buffer); > > > > > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer); > > > > > > > > This name stinks, too. You know that you can have time stamping > > > > all by itself, right? It is logically separate from the ptp clock > > stuff. > > > > > > > > This patch doesn't really improve the driver at all, IMHO. > > > > > > > > Thanks, > > > > Richard > > > > > > Yes, I'm aware. But, as it stands today, we don't use it for > > > anything > > else. If the function is feature specific, then we should be calling > > it out as such. > > > > Right now the time stamping is being equated with the clock functions, > > but it really should be decoupled. The 82580 can time stamp every > > received packet, which can be interesting for performance monitoring, > > even without PTP (and adding *that* would be a useful change). > > As Jake said, there's no support for hardware timestamping without PTP > today. As such, if the function is only useful for that feature, then I > think it's less ambiguous to leave it the way I've coded it today. If we > de-couple hardware timestamping and clock synchronization, which I > wouldn't be opposed to, then the function name can reflect becoming more > generic. As it stands, I'd rather not call something generic when it > isn't. I think it's misleading, personally. > > > > I'm sorry you feel like this patch doesn't improve the driver. The > > goal is code cleanup and consistency, both of which I consider to be > > driver improvements and is why I made the patches. > > > > But the code wasn't dirty in the first place. It doesn't need this > > "cleaning." This series undoes the inline-able functions for no good > > reason. As far as ixgbe goes, this driver came first, so you might as > > well be making *that* driver consistent with this one. > > > > Thanks, > > Richard > > Actually, yes, the code *was* dirty, because it breaks the coding style of > the rest of the driver and isn't as self-documenting as it could be, and > the inline-able functions you claim I'm destroying called global functions > anyway. Also, just because igb came first doesn't mean we should ignore > porting better or clearer solutions from later drivers. If something is > better or clearer, I think we should use be using it. > > Cheers, > Matthew > -- > 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 -- 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
> -----Original Message----- > From: Keller, Jacob E > Sent: Thursday, August 23, 2012 11:48 AM > To: Vick, Matthew; Richard Cochran > Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org; > gospo@redhat.com; sassmann@redhat.com > Subject: RE: [net-next 11/13] igb: Update PTP function names/variables > and locations. > > [...] > > The history isn't lost, it is just obscured. I agree if the change is > necessary it should be done. I think we all disagree on what is > necessary. I would prefer function names to match. Richard has a point > regarding the time stamping all packets, however again the ioctl > turning that on tends to be quite PTP centric already. I think that > value isn't necessarily added due to the current coupling of the > features. It is partially coupled by the hardware anyways. > > Effectively with the PTP framework disabled you have a half useful > feature that is enabled by a strange ioctl that has a lot of PTP > specific names, and doesn't always do what you want. > > I am not sure how much work would be required to get to a state where > the separation of function makes sense. > > - Jake Fair point about the history. Since these functions are tightly coupled with PTP, it makes sense to call them that for now. Long-term, I completely agree with Richard--once the function is independent of PTP, we should give it a generic name. Cheers, Matthew -- 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 Thu, 2012-08-23 at 19:53 +0200, Richard Cochran wrote: > On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote: > > > > > #ifdef CONFIG_IGB_PTP > > > > -static int igb_ethtool_get_ts_info(struct net_device *dev, > > > > +static int igb_get_ts_info(struct net_device *dev, > > > > > > I like the old name better. > > > > The old name is out of the coding style of igb. Every other > > function is igb_get_* or igb_set_*, with the exception of > > igb_ethtool_begin and igb_ethtool_complete. > No, just leave the names alone, and keep the functions where they > are. It is just churn. > > One of the most useful ways to understand code (at least for me) is to > use git blame. It tells you when code was added, what the reason was, > and how the change looks in context. By moving and renaming willy > nilly, you are obscuring this valuable information. [] > > I'm sorry you feel like this patch doesn't improve the driver. > > The goal is code cleanup and consistency, both of which I > > consider to be driver improvements and is why I made the patches. > > But the code wasn't dirty in the first place. It doesn't need this > "cleaning." This series undoes the inline-able functions for no good > reason. As far as ixgbe goes, this driver came first, so you might as > well be making *that* driver consistent with this one. I disagree with Richard. Improving code clarity and consistency isn't churn. Old code isn't necessarily the best code, nor should necessarily old code be a required guide for new code. The most valuable form of code is the current one, not any antecedent version. People that need to wade through old crud to blame someone still can. -- 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 Thu, Aug 23, 2012 at 08:02:25PM -0700, Joe Perches wrote: > Improving code clarity and consistency isn't churn. This patch series moves code around for no good reason. That is, by definition, churn. > The most valuable form of code is the current one, > not any antecedent version. You really haven't noticed all the code review and rebasing that goes on around here in order to improve the commit history, have you? 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
On Thu, Aug 23, 2012 at 06:44:24PM +0000, Vick, Matthew wrote: > > Isn't this discussion creeping outside the scope of this patch? Yes, it is. I am suggesting that the driver could use improvement, but not by renaming functions to suit your or someone else's taste. Instead, you could work on the time stamping feature. (And if, along the way, you end up renaming the functions that you change, then I won't complain ;) 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
On Fri, 2012-08-24 at 08:32 +0200, Richard Cochran wrote: > On Thu, Aug 23, 2012 at 08:02:25PM -0700, Joe Perches wrote: > > > Improving code clarity and consistency isn't churn. > > This patch series moves code around for no good reason. That is, by > definition, churn. For your definition of good. > > The most valuable form of code is the current one, > > not any antecedent version. > > You really haven't noticed all the code review and rebasing that goes > on around here in order to improve the commit history, have you? That'd be incorrect too. -- 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: Joe Perches <joe@perches.com> Date: Fri, 24 Aug 2012 02:22:34 -0700 > On Fri, 2012-08-24 at 08:32 +0200, Richard Cochran wrote: >> On Thu, Aug 23, 2012 at 08:02:25PM -0700, Joe Perches wrote: >> >> > Improving code clarity and consistency isn't churn. >> >> This patch series moves code around for no good reason. That is, by >> definition, churn. > > For your definition of good. I think the people doing all of the actual development and maintainence of the code get to decide how to define good. And in this case that's basically the Intel NIC development team, not you. Moving functions around is a very valuable and useful cleanup quite often. So you can count me in on their definition of "good" as well. -- 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
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Thursday, August 23, 2012 11:56 PM > To: Vick, Matthew > Cc: Keller, Jacob E; Kirsher, Jeffrey T; davem@davemloft.net; > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com > Subject: Re: [net-next 11/13] igb: Update PTP function names/variables > and locations. > > On Thu, Aug 23, 2012 at 06:44:24PM +0000, Vick, Matthew wrote: > > > > Isn't this discussion creeping outside the scope of this patch? > > Yes, it is. I am suggesting that the driver could use improvement, but > not by renaming functions to suit your or someone else's taste. > Instead, you could work on the time stamping feature. > > (And if, along the way, you end up renaming the functions that you > change, then I won't complain ;) > > Thanks, > Richard Looking over the discussions and by trying to reach consensus, I think what I will do is re-spin patch 3 that Jeff sent (Correct PTP support query from ethtool.) to V2 with your feedback and work with Jeff to send all of my patchset up together. This first group of patches is groundwork for the last two. How does this sound? Cheers, Matthew -- 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, 2012-08-24 at 11:12 -0400, David Miller wrote: > From: Joe Perches <joe@perches.com> > Date: Fri, 24 Aug 2012 02:22:34 -0700 > > On Fri, 2012-08-24 at 08:32 +0200, Richard Cochran wrote: > >> On Thu, Aug 23, 2012 at 08:02:25PM -0700, Joe Perches wrote: > >> > Improving code clarity and consistency isn't churn. > >> This patch series moves code around for no good reason. That is, by > >> definition, churn. > > > > For your definition of good. > > I think the people doing all of the actual development and > maintainence of the code get to decide how to define good. And in > this case that's basically the Intel NIC development team, not you. Just for clarity, you do mean Richard's view and not mine. I agree with these proposed changes. > Moving functions around is a very valuable and useful cleanup quite > often. > > So you can count me in on their definition of "good" as well. -- 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, Aug 24, 2012 at 03:52:30PM +0000, Vick, Matthew wrote: > > Looking over the discussions and by trying to reach consensus, I think what I will do is re-spin patch 3 that Jeff sent (Correct PTP support query from ethtool.) to V2 with your feedback and work with Jeff to send all of my patchset up together. This first group of patches is groundwork for the last two. How does this sound? Sound good to me. BTW, if you have some bug fixes coming (like ifup/ifdown), please put them *before* any other renaming/refactoring. In that way, the bug fix can do directly to 3.5 stable, too. 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
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Friday, August 24, 2012 10:49 PM > To: Vick, Matthew > Cc: Keller, Jacob E; Kirsher, Jeffrey T; davem@davemloft.net; > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com > Subject: Re: [net-next 11/13] igb: Update PTP function names/variables > and locations. > > On Fri, Aug 24, 2012 at 03:52:30PM +0000, Vick, Matthew wrote: > > > > Looking over the discussions and by trying to reach consensus, I > think what I will do is re-spin patch 3 that Jeff sent (Correct PTP > support query from ethtool.) to V2 with your feedback and work with > Jeff to send all of my patchset up together. This first group of > patches is groundwork for the last two. How does this sound? > > Sound good to me. > > BTW, if you have some bug fixes coming (like ifup/ifdown), please put > them *before* any other renaming/refactoring. In that way, the bug fix > can do directly to 3.5 stable, too. > > Thanks, > Richard Unfortunately, the way I've built the series makes it difficult to put those fixes before the re-factoring. The need for the design change drove the re-factor to give me the foundation. I'll make a note to myself to come up with something to apply to stable. Cheers, Matthew -- 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
> > Unfortunately, the way I've built the series makes it difficult to put those fixes before the re-factoring. The need for the design change drove the re-factor to give me the foundation. I'll make a note to myself to come up with something to apply to stable. Hm, I didn't see anything here that looks like bug fix. Did I miss something? What causes the ifup/ifdown weirdness, anyhow? (It seems hard to believe that it has something to do with the names or locations of the various functions.) 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
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Monday, August 27, 2012 9:44 AM > To: Vick, Matthew > Cc: Keller, Jacob E; Kirsher, Jeffrey T; davem@davemloft.net; > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com > Subject: Re: [net-next 11/13] igb: Update PTP function names/variables > and locations. > > > > > Unfortunately, the way I've built the series makes it difficult to > put those fixes before the re-factoring. The need for the design change > drove the re-factor to give me the foundation. I'll make a note to > myself to come up with something to apply to stable. > > Hm, I didn't see anything here that looks like bug fix. Did I miss > something? What causes the ifup/ifdown weirdness, anyhow? > > (It seems hard to believe that it has something to do with the names or > locations of the various functions.) > > Thanks, > Richard Sorry, I wasn't very clear--I mean the bug fix I have for ifup/ifdown is in the next few patches with the implementation change I'm doing, since the code I have in place to fix it applies only to the new implementation. I'll have to make a separate patch for stable to address it there. If the ifup/ifdown weirdness happens because we never re-enable timestamping in the hardware following reset and the device goes through a reset during an ifdown/ifup cycle. It's a pretty straightforward fix, so it shouldn't be much effort to spin up something for stable. Matthew -- 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/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h index a3b5b90..7973469 100644 --- a/drivers/net/ethernet/intel/igb/igb.h +++ b/drivers/net/ethernet/intel/igb/igb.h @@ -344,7 +344,6 @@ struct igb_adapter { /* OS defined structs */ struct pci_dev *pdev; - struct hwtstamp_config hwtstamp_config; spinlock_t stats64_lock; struct rtnl_link_stats64 stats64; @@ -380,8 +379,8 @@ struct igb_adapter { #ifdef CONFIG_IGB_PTP struct ptp_clock *ptp_clock; - struct ptp_clock_info caps; - struct delayed_work overflow_work; + struct ptp_clock_info ptp_caps; + struct delayed_work ptp_overflow_work; spinlock_t tmreg_lock; struct cyclecounter cc; struct timecounter tc; @@ -440,10 +439,14 @@ extern void igb_power_up_link(struct igb_adapter *); extern void igb_set_fw_version(struct igb_adapter *); #ifdef CONFIG_IGB_PTP extern void igb_ptp_init(struct igb_adapter *adapter); -extern void igb_ptp_remove(struct igb_adapter *adapter); -extern void igb_systim_to_hwtstamp(struct igb_adapter *adapter, - struct skb_shared_hwtstamps *hwtstamps, - u64 systim); +extern void igb_ptp_stop(struct igb_adapter *adapter); +extern void igb_ptp_tx_hwtstamp(struct igb_q_vector *q_vector, + struct igb_tx_buffer *buffer_info); +extern void igb_ptp_rx_hwtstamp(struct igb_q_vector *q_vector, + union e1000_adv_rx_desc *rx_desc, + struct sk_buff *skb); +extern int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, + struct ifreq *ifr, int cmd); #endif /* CONFIG_IGB_PTP */ static inline s32 igb_reset_phy(struct e1000_hw *hw) diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c index 6adb0f7..d1a120e 100644 --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c @@ -2280,21 +2280,8 @@ static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data) } } -static int igb_ethtool_begin(struct net_device *netdev) -{ - struct igb_adapter *adapter = netdev_priv(netdev); - pm_runtime_get_sync(&adapter->pdev->dev); - return 0; -} - -static void igb_ethtool_complete(struct net_device *netdev) -{ - struct igb_adapter *adapter = netdev_priv(netdev); - pm_runtime_put(&adapter->pdev->dev); -} - #ifdef CONFIG_IGB_PTP -static int igb_ethtool_get_ts_info(struct net_device *dev, +static int igb_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info) { struct igb_adapter *adapter = netdev_priv(dev); @@ -2325,6 +2312,19 @@ static int igb_ethtool_get_ts_info(struct net_device *dev, } #endif /* CONFIG_IGB_PTP */ +static int igb_ethtool_begin(struct net_device *netdev) +{ + struct igb_adapter *adapter = netdev_priv(netdev); + pm_runtime_get_sync(&adapter->pdev->dev); + return 0; +} + +static void igb_ethtool_complete(struct net_device *netdev) +{ + struct igb_adapter *adapter = netdev_priv(netdev); + pm_runtime_put(&adapter->pdev->dev); +} + static const struct ethtool_ops igb_ethtool_ops = { .get_settings = igb_get_settings, .set_settings = igb_set_settings, @@ -2351,11 +2351,11 @@ static const struct ethtool_ops igb_ethtool_ops = { .get_ethtool_stats = igb_get_ethtool_stats, .get_coalesce = igb_get_coalesce, .set_coalesce = igb_set_coalesce, - .begin = igb_ethtool_begin, - .complete = igb_ethtool_complete, #ifdef CONFIG_IGB_PTP - .get_ts_info = igb_ethtool_get_ts_info, + .get_ts_info = igb_get_ts_info, #endif /* CONFIG_IGB_PTP */ + .begin = igb_ethtool_begin, + .complete = igb_ethtool_complete, }; void igb_set_ethtool_ops(struct net_device *netdev) diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index 03477d7..6e39f0c 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -2260,7 +2260,7 @@ static void __devexit igb_remove(struct pci_dev *pdev) pm_runtime_get_noresume(&pdev->dev); #ifdef CONFIG_IGB_PTP - igb_ptp_remove(adapter); + igb_ptp_stop(adapter); #endif /* CONFIG_IGB_PTP */ /* @@ -5750,37 +5750,6 @@ static int igb_poll(struct napi_struct *napi, int budget) return 0; } -#ifdef CONFIG_IGB_PTP -/** - * igb_tx_hwtstamp - utility function which checks for TX time stamp - * @q_vector: pointer to q_vector containing needed info - * @buffer: pointer to igb_tx_buffer structure - * - * If we were asked to do hardware stamping and such a time stamp is - * available, then it must have been for this skb here because we only - * allow only one such packet into the queue. - */ -static void igb_tx_hwtstamp(struct igb_q_vector *q_vector, - struct igb_tx_buffer *buffer_info) -{ - struct igb_adapter *adapter = q_vector->adapter; - struct e1000_hw *hw = &adapter->hw; - struct skb_shared_hwtstamps shhwtstamps; - u64 regval; - - /* if skb does not support hw timestamp or TX stamp not valid exit */ - if (likely(!(buffer_info->tx_flags & IGB_TX_FLAGS_TSTAMP)) || - !(rd32(E1000_TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID)) - return; - - regval = rd32(E1000_TXSTMPL); - regval |= (u64)rd32(E1000_TXSTMPH) << 32; - - igb_systim_to_hwtstamp(adapter, &shhwtstamps, regval); - skb_tstamp_tx(buffer_info->skb, &shhwtstamps); -} -#endif /* CONFIG_IGB_PTP */ - /** * igb_clean_tx_irq - Reclaim resources after transmit completes * @q_vector: pointer to q_vector containing needed info @@ -5827,7 +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector) #ifdef CONFIG_IGB_PTP /* retrieve hardware timestamp */ - igb_tx_hwtstamp(q_vector, tx_buffer); + igb_ptp_tx_hwtstamp(q_vector, tx_buffer); #endif /* CONFIG_IGB_PTP */ /* free the skb */ @@ -6001,47 +5970,6 @@ static inline void igb_rx_hash(struct igb_ring *ring, skb->rxhash = le32_to_cpu(rx_desc->wb.lower.hi_dword.rss); } -#ifdef CONFIG_IGB_PTP -static void igb_rx_hwtstamp(struct igb_q_vector *q_vector, - union e1000_adv_rx_desc *rx_desc, - struct sk_buff *skb) -{ - struct igb_adapter *adapter = q_vector->adapter; - struct e1000_hw *hw = &adapter->hw; - u64 regval; - - if (!igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP | - E1000_RXDADV_STAT_TS)) - return; - - /* - * If this bit is set, then the RX registers contain the time stamp. No - * other packet will be time stamped until we read these registers, so - * read the registers to make them available again. Because only one - * packet can be time stamped at a time, we know that the register - * values must belong to this one here and therefore we don't need to - * compare any of the additional attributes stored for it. - * - * If nothing went wrong, then it should have a shared tx_flags that we - * can turn into a skb_shared_hwtstamps. - */ - if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) { - u32 *stamp = (u32 *)skb->data; - regval = le32_to_cpu(*(stamp + 2)); - regval |= (u64)le32_to_cpu(*(stamp + 3)) << 32; - skb_pull(skb, IGB_TS_HDR_LEN); - } else { - if(!(rd32(E1000_TSYNCRXCTL) & E1000_TSYNCRXCTL_VALID)) - return; - - regval = rd32(E1000_RXSTMPL); - regval |= (u64)rd32(E1000_RXSTMPH) << 32; - } - - igb_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), regval); -} -#endif /* CONFIG_IGB_PTP */ - static void igb_rx_vlan(struct igb_ring *ring, union e1000_adv_rx_desc *rx_desc, struct sk_buff *skb) @@ -6153,7 +6081,7 @@ static bool igb_clean_rx_irq(struct igb_q_vector *q_vector, int budget) } #ifdef CONFIG_IGB_PTP - igb_rx_hwtstamp(q_vector, rx_desc, skb); + igb_ptp_rx_hwtstamp(q_vector, rx_desc, skb); #endif /* CONFIG_IGB_PTP */ igb_rx_hash(rx_ring, rx_desc, skb); igb_rx_checksum(rx_ring, rx_desc, skb); @@ -6347,183 +6275,6 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) return 0; } -#ifdef CONFIG_IGB_PTP -/** - * igb_hwtstamp_ioctl - control hardware time stamping - * @netdev: - * @ifreq: - * @cmd: - * - * Outgoing time stamping can be enabled and disabled. Play nice and - * disable it when requested, although it shouldn't case any overhead - * when no packet needs it. At most one packet in the queue may be - * marked for time stamping, otherwise it would be impossible to tell - * for sure to which packet the hardware time stamp belongs. - * - * Incoming time stamping has to be configured via the hardware - * filters. Not all combinations are supported, in particular event - * type has to be specified. Matching the kind of event packet is - * not supported, with the exception of "all V2 events regardless of - * level 2 or 4". - * - **/ -static int igb_hwtstamp_ioctl(struct net_device *netdev, - struct ifreq *ifr, int cmd) -{ - struct igb_adapter *adapter = netdev_priv(netdev); - struct e1000_hw *hw = &adapter->hw; - struct hwtstamp_config config; - u32 tsync_tx_ctl = E1000_TSYNCTXCTL_ENABLED; - u32 tsync_rx_ctl = E1000_TSYNCRXCTL_ENABLED; - u32 tsync_rx_cfg = 0; - bool is_l4 = false; - bool is_l2 = false; - u32 regval; - - if (copy_from_user(&config, ifr->ifr_data, sizeof(config))) - return -EFAULT; - - /* reserved for future extensions */ - if (config.flags) - return -EINVAL; - - switch (config.tx_type) { - case HWTSTAMP_TX_OFF: - tsync_tx_ctl = 0; - case HWTSTAMP_TX_ON: - break; - default: - return -ERANGE; - } - - switch (config.rx_filter) { - case HWTSTAMP_FILTER_NONE: - tsync_rx_ctl = 0; - break; - case HWTSTAMP_FILTER_PTP_V1_L4_EVENT: - case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: - case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: - case HWTSTAMP_FILTER_ALL: - /* - * register TSYNCRXCFG must be set, therefore it is not - * possible to time stamp both Sync and Delay_Req messages - * => fall back to time stamping all packets - */ - tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_ALL; - config.rx_filter = HWTSTAMP_FILTER_ALL; - break; - case HWTSTAMP_FILTER_PTP_V1_L4_SYNC: - tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L4_V1; - tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE; - is_l4 = true; - break; - case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ: - tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L4_V1; - tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE; - is_l4 = true; - break; - case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: - case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: - tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L2_L4_V2; - tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE; - is_l2 = true; - is_l4 = true; - config.rx_filter = HWTSTAMP_FILTER_SOME; - break; - case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: - case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: - tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L2_L4_V2; - tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE; - is_l2 = true; - is_l4 = true; - config.rx_filter = HWTSTAMP_FILTER_SOME; - break; - case HWTSTAMP_FILTER_PTP_V2_EVENT: - case HWTSTAMP_FILTER_PTP_V2_SYNC: - case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: - tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_EVENT_V2; - config.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; - is_l2 = true; - is_l4 = true; - break; - default: - return -ERANGE; - } - - if (hw->mac.type == e1000_82575) { - if (tsync_rx_ctl | tsync_tx_ctl) - return -EINVAL; - return 0; - } - - /* - * Per-packet timestamping only works if all packets are - * timestamped, so enable timestamping in all packets as - * long as one rx filter was configured. - */ - if ((hw->mac.type >= e1000_82580) && tsync_rx_ctl) { - tsync_rx_ctl = E1000_TSYNCRXCTL_ENABLED; - tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_ALL; - } - - /* enable/disable TX */ - regval = rd32(E1000_TSYNCTXCTL); - regval &= ~E1000_TSYNCTXCTL_ENABLED; - regval |= tsync_tx_ctl; - wr32(E1000_TSYNCTXCTL, regval); - - /* enable/disable RX */ - regval = rd32(E1000_TSYNCRXCTL); - regval &= ~(E1000_TSYNCRXCTL_ENABLED | E1000_TSYNCRXCTL_TYPE_MASK); - regval |= tsync_rx_ctl; - wr32(E1000_TSYNCRXCTL, regval); - - /* define which PTP packets are time stamped */ - wr32(E1000_TSYNCRXCFG, tsync_rx_cfg); - - /* define ethertype filter for timestamped packets */ - if (is_l2) - wr32(E1000_ETQF(3), - (E1000_ETQF_FILTER_ENABLE | /* enable filter */ - E1000_ETQF_1588 | /* enable timestamping */ - ETH_P_1588)); /* 1588 eth protocol type */ - else - wr32(E1000_ETQF(3), 0); - -#define PTP_PORT 319 - /* L4 Queue Filter[3]: filter by destination port and protocol */ - if (is_l4) { - u32 ftqf = (IPPROTO_UDP /* UDP */ - | E1000_FTQF_VF_BP /* VF not compared */ - | E1000_FTQF_1588_TIME_STAMP /* Enable Timestamping */ - | E1000_FTQF_MASK); /* mask all inputs */ - ftqf &= ~E1000_FTQF_MASK_PROTO_BP; /* enable protocol check */ - - wr32(E1000_IMIR(3), htons(PTP_PORT)); - wr32(E1000_IMIREXT(3), - (E1000_IMIREXT_SIZE_BP | E1000_IMIREXT_CTRL_BP)); - if (hw->mac.type == e1000_82576) { - /* enable source port check */ - wr32(E1000_SPQF(3), htons(PTP_PORT)); - ftqf &= ~E1000_FTQF_MASK_SOURCE_PORT_BP; - } - wr32(E1000_FTQF(3), ftqf); - } else { - wr32(E1000_FTQF(3), E1000_FTQF_MASK); - } - wrfl(); - - adapter->hwtstamp_config = config; - - /* clear TX/RX time stamp registers, just to be sure */ - regval = rd32(E1000_TXSTMPH); - regval = rd32(E1000_RXSTMPH); - - return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ? - -EFAULT : 0; -} -#endif /* CONFIG_IGB_PTP */ - /** * igb_ioctl - * @netdev: @@ -6539,7 +6290,7 @@ static int igb_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) return igb_mii_ioctl(netdev, ifr, cmd); #ifdef CONFIG_IGB_PTP case SIOCSHWTSTAMP: - return igb_hwtstamp_ioctl(netdev, ifr, cmd); + return igb_ptp_hwtstamp_ioctl(netdev, ifr, cmd); #endif /* CONFIG_IGB_PTP */ default: return -EOPNOTSUPP; diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c index c846ea9..34e0d69 100644 --- a/drivers/net/ethernet/intel/igb/igb_ptp.c +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c @@ -69,22 +69,22 @@ * 2^40 * 10^-9 / 60 = 18.3 minutes. */ -#define IGB_OVERFLOW_PERIOD (HZ * 60 * 9) -#define INCPERIOD_82576 (1 << E1000_TIMINCA_16NS_SHIFT) -#define INCVALUE_82576_MASK ((1 << E1000_TIMINCA_16NS_SHIFT) - 1) -#define INCVALUE_82576 (16 << IGB_82576_TSYNC_SHIFT) -#define IGB_NBITS_82580 40 +#define IGB_SYSTIM_OVERFLOW_PERIOD (HZ * 60 * 9) +#define INCPERIOD_82576 (1 << E1000_TIMINCA_16NS_SHIFT) +#define INCVALUE_82576_MASK ((1 << E1000_TIMINCA_16NS_SHIFT) - 1) +#define INCVALUE_82576 (16 << IGB_82576_TSYNC_SHIFT) +#define IGB_NBITS_82580 40 /* * SYSTIM read access for the 82576 */ -static cycle_t igb_82576_systim_read(const struct cyclecounter *cc) +static cycle_t igb_ptp_read_82576(const struct cyclecounter *cc) { - u64 val; - u32 lo, hi; struct igb_adapter *igb = container_of(cc, struct igb_adapter, cc); struct e1000_hw *hw = &igb->hw; + u64 val; + u32 lo, hi; lo = rd32(E1000_SYSTIML); hi = rd32(E1000_SYSTIMH); @@ -99,12 +99,12 @@ static cycle_t igb_82576_systim_read(const struct cyclecounter *cc) * SYSTIM read access for the 82580 */ -static cycle_t igb_82580_systim_read(const struct cyclecounter *cc) +static cycle_t igb_ptp_read_82580(const struct cyclecounter *cc) { - u64 val; - u32 lo, hi, jk; struct igb_adapter *igb = container_of(cc, struct igb_adapter, cc); struct e1000_hw *hw = &igb->hw; + u64 val; + u32 lo, hi, jk; /* * The timestamp latches on lowest register read. For the 82580 @@ -121,17 +121,63 @@ static cycle_t igb_82580_systim_read(const struct cyclecounter *cc) return val; } +/** + * igb_ptp_systim_to_hwtstamp - convert system time value to hw timestamp + * @adapter: board private structure + * @hwtstamps: timestamp structure to update + * @systim: unsigned 64bit system time value. + * + * We need to convert the system time value stored in the RX/TXSTMP registers + * into a hwtstamp which can be used by the upper level timestamping functions. + * + * The 'tmreg_lock' spinlock is used to protect the consistency of the + * system time value. This is needed because reading the 64 bit time + * value involves reading two (or three) 32 bit registers. The first + * read latches the value. Ditto for writing. + * + * In addition, here have extended the system time with an overflow + * counter in software. + **/ +static void igb_ptp_systim_to_hwtstamp(struct igb_adapter *adapter, + struct skb_shared_hwtstamps *hwtstamps, + u64 systim) +{ + unsigned long flags; + u64 ns; + + switch (adapter->hw.mac.type) { + case e1000_i210: + case e1000_i211: + case e1000_i350: + case e1000_82580: + case e1000_82576: + break; + default: + return; + } + + spin_lock_irqsave(&adapter->tmreg_lock, flags); + + ns = timecounter_cyc2time(&adapter->tc, systim); + + spin_unlock_irqrestore(&adapter->tmreg_lock, flags); + + memset(hwtstamps, 0, sizeof(*hwtstamps)); + hwtstamps->hwtstamp = ns_to_ktime(ns); +} + /* * PTP clock operations */ -static int ptp_82576_adjfreq(struct ptp_clock_info *ptp, s32 ppb) +static int igb_ptp_adjfreq_82576(struct ptp_clock_info *ptp, s32 ppb) { + struct igb_adapter *igb = container_of(ptp, struct igb_adapter, + ptp_caps); + struct e1000_hw *hw = &igb->hw; + int neg_adj = 0; u64 rate; u32 incvalue; - int neg_adj = 0; - struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps); - struct e1000_hw *hw = &igb->hw; if (ppb < 0) { neg_adj = 1; @@ -153,13 +199,14 @@ static int ptp_82576_adjfreq(struct ptp_clock_info *ptp, s32 ppb) return 0; } -static int ptp_82580_adjfreq(struct ptp_clock_info *ptp, s32 ppb) +static int igb_ptp_adjfreq_82580(struct ptp_clock_info *ptp, s32 ppb) { + struct igb_adapter *igb = container_of(ptp, struct igb_adapter, + ptp_caps); + struct e1000_hw *hw = &igb->hw; + int neg_adj = 0; u64 rate; u32 inca; - int neg_adj = 0; - struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps); - struct e1000_hw *hw = &igb->hw; if (ppb < 0) { neg_adj = 1; @@ -178,11 +225,12 @@ static int ptp_82580_adjfreq(struct ptp_clock_info *ptp, s32 ppb) return 0; } -static int igb_adjtime(struct ptp_clock_info *ptp, s64 delta) +static int igb_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta) { - s64 now; + struct igb_adapter *igb = container_of(ptp, struct igb_adapter, + ptp_caps); unsigned long flags; - struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps); + s64 now; spin_lock_irqsave(&igb->tmreg_lock, flags); @@ -195,12 +243,13 @@ static int igb_adjtime(struct ptp_clock_info *ptp, s64 delta) return 0; } -static int igb_gettime(struct ptp_clock_info *ptp, struct timespec *ts) +static int igb_ptp_gettime(struct ptp_clock_info *ptp, struct timespec *ts) { + struct igb_adapter *igb = container_of(ptp, struct igb_adapter, + ptp_caps); + unsigned long flags; u64 ns; u32 remainder; - unsigned long flags; - struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps); spin_lock_irqsave(&igb->tmreg_lock, flags); @@ -214,11 +263,13 @@ static int igb_gettime(struct ptp_clock_info *ptp, struct timespec *ts) return 0; } -static int igb_settime(struct ptp_clock_info *ptp, const struct timespec *ts) +static int igb_ptp_settime(struct ptp_clock_info *ptp, + const struct timespec *ts) { - u64 ns; + struct igb_adapter *igb = container_of(ptp, struct igb_adapter, + ptp_caps); unsigned long flags; - struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps); + u64 ns; ns = ts->tv_sec * 1000000000ULL; ns += ts->tv_nsec; @@ -232,29 +283,265 @@ static int igb_settime(struct ptp_clock_info *ptp, const struct timespec *ts) return 0; } -static int ptp_82576_enable(struct ptp_clock_info *ptp, - struct ptp_clock_request *rq, int on) +static int igb_ptp_enable(struct ptp_clock_info *ptp, + struct ptp_clock_request *rq, int on) { return -EOPNOTSUPP; } -static int ptp_82580_enable(struct ptp_clock_info *ptp, - struct ptp_clock_request *rq, int on) +static void igb_ptp_overflow_check(struct work_struct *work) { - return -EOPNOTSUPP; + struct igb_adapter *igb = + container_of(work, struct igb_adapter, ptp_overflow_work.work); + struct timespec ts; + + igb_ptp_gettime(&igb->ptp_caps, &ts); + + pr_debug("igb overflow check at %ld.%09lu\n", ts.tv_sec, ts.tv_nsec); + + schedule_delayed_work(&igb->ptp_overflow_work, + IGB_SYSTIM_OVERFLOW_PERIOD); } -static void igb_overflow_check(struct work_struct *work) +/** + * igb_ptp_tx_hwtstamp - utility function which checks for TX time stamp + * @q_vector: pointer to q_vector containing needed info + * @buffer: pointer to igb_tx_buffer structure + * + * If we were asked to do hardware stamping and such a time stamp is + * available, then it must have been for this skb here because we only + * allow only one such packet into the queue. + */ +void igb_ptp_tx_hwtstamp(struct igb_q_vector *q_vector, + struct igb_tx_buffer *buffer_info) { - struct timespec ts; - struct igb_adapter *igb = - container_of(work, struct igb_adapter, overflow_work.work); + struct igb_adapter *adapter = q_vector->adapter; + struct e1000_hw *hw = &adapter->hw; + struct skb_shared_hwtstamps shhwtstamps; + u64 regval; - igb_gettime(&igb->caps, &ts); + /* if skb does not support hw timestamp or TX stamp not valid exit */ + if (likely(!(buffer_info->tx_flags & IGB_TX_FLAGS_TSTAMP)) || + !(rd32(E1000_TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID)) + return; - pr_debug("igb overflow check at %ld.%09lu\n", ts.tv_sec, ts.tv_nsec); + regval = rd32(E1000_TXSTMPL); + regval |= (u64)rd32(E1000_TXSTMPH) << 32; - schedule_delayed_work(&igb->overflow_work, IGB_OVERFLOW_PERIOD); + igb_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval); + skb_tstamp_tx(buffer_info->skb, &shhwtstamps); +} + +void igb_ptp_rx_hwtstamp(struct igb_q_vector *q_vector, + union e1000_adv_rx_desc *rx_desc, + struct sk_buff *skb) +{ + struct igb_adapter *adapter = q_vector->adapter; + struct e1000_hw *hw = &adapter->hw; + u64 regval; + + if (!igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP | + E1000_RXDADV_STAT_TS)) + return; + + /* + * If this bit is set, then the RX registers contain the time stamp. No + * other packet will be time stamped until we read these registers, so + * read the registers to make them available again. Because only one + * packet can be time stamped at a time, we know that the register + * values must belong to this one here and therefore we don't need to + * compare any of the additional attributes stored for it. + * + * If nothing went wrong, then it should have a shared tx_flags that we + * can turn into a skb_shared_hwtstamps. + */ + if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) { + u32 *stamp = (u32 *)skb->data; + regval = le32_to_cpu(*(stamp + 2)); + regval |= (u64)le32_to_cpu(*(stamp + 3)) << 32; + skb_pull(skb, IGB_TS_HDR_LEN); + } else { + if (!(rd32(E1000_TSYNCRXCTL) & E1000_TSYNCRXCTL_VALID)) + return; + + regval = rd32(E1000_RXSTMPL); + regval |= (u64)rd32(E1000_RXSTMPH) << 32; + } + + igb_ptp_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), regval); +} + +/** + * igb_ptp_hwtstamp_ioctl - control hardware time stamping + * @netdev: + * @ifreq: + * @cmd: + * + * Outgoing time stamping can be enabled and disabled. Play nice and + * disable it when requested, although it shouldn't case any overhead + * when no packet needs it. At most one packet in the queue may be + * marked for time stamping, otherwise it would be impossible to tell + * for sure to which packet the hardware time stamp belongs. + * + * Incoming time stamping has to be configured via the hardware + * filters. Not all combinations are supported, in particular event + * type has to be specified. Matching the kind of event packet is + * not supported, with the exception of "all V2 events regardless of + * level 2 or 4". + * + **/ +int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, + struct ifreq *ifr, int cmd) +{ + struct igb_adapter *adapter = netdev_priv(netdev); + struct e1000_hw *hw = &adapter->hw; + struct hwtstamp_config config; + u32 tsync_tx_ctl = E1000_TSYNCTXCTL_ENABLED; + u32 tsync_rx_ctl = E1000_TSYNCRXCTL_ENABLED; + u32 tsync_rx_cfg = 0; + bool is_l4 = false; + bool is_l2 = false; + u32 regval; + + if (copy_from_user(&config, ifr->ifr_data, sizeof(config))) + return -EFAULT; + + /* reserved for future extensions */ + if (config.flags) + return -EINVAL; + + switch (config.tx_type) { + case HWTSTAMP_TX_OFF: + tsync_tx_ctl = 0; + case HWTSTAMP_TX_ON: + break; + default: + return -ERANGE; + } + + switch (config.rx_filter) { + case HWTSTAMP_FILTER_NONE: + tsync_rx_ctl = 0; + break; + case HWTSTAMP_FILTER_PTP_V1_L4_EVENT: + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: + case HWTSTAMP_FILTER_ALL: + /* + * register TSYNCRXCFG must be set, therefore it is not + * possible to time stamp both Sync and Delay_Req messages + * => fall back to time stamping all packets + */ + tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_ALL; + config.rx_filter = HWTSTAMP_FILTER_ALL; + break; + case HWTSTAMP_FILTER_PTP_V1_L4_SYNC: + tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L4_V1; + tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE; + is_l4 = true; + break; + case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ: + tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L4_V1; + tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE; + is_l4 = true; + break; + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: + tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L2_L4_V2; + tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE; + is_l2 = true; + is_l4 = true; + config.rx_filter = HWTSTAMP_FILTER_SOME; + break; + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: + tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L2_L4_V2; + tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE; + is_l2 = true; + is_l4 = true; + config.rx_filter = HWTSTAMP_FILTER_SOME; + break; + case HWTSTAMP_FILTER_PTP_V2_EVENT: + case HWTSTAMP_FILTER_PTP_V2_SYNC: + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: + tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_EVENT_V2; + config.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; + is_l2 = true; + is_l4 = true; + break; + default: + return -ERANGE; + } + + if (hw->mac.type == e1000_82575) { + if (tsync_rx_ctl | tsync_tx_ctl) + return -EINVAL; + return 0; + } + + /* + * Per-packet timestamping only works if all packets are + * timestamped, so enable timestamping in all packets as + * long as one rx filter was configured. + */ + if ((hw->mac.type >= e1000_82580) && tsync_rx_ctl) { + tsync_rx_ctl = E1000_TSYNCRXCTL_ENABLED; + tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_ALL; + } + + /* enable/disable TX */ + regval = rd32(E1000_TSYNCTXCTL); + regval &= ~E1000_TSYNCTXCTL_ENABLED; + regval |= tsync_tx_ctl; + wr32(E1000_TSYNCTXCTL, regval); + + /* enable/disable RX */ + regval = rd32(E1000_TSYNCRXCTL); + regval &= ~(E1000_TSYNCRXCTL_ENABLED | E1000_TSYNCRXCTL_TYPE_MASK); + regval |= tsync_rx_ctl; + wr32(E1000_TSYNCRXCTL, regval); + + /* define which PTP packets are time stamped */ + wr32(E1000_TSYNCRXCFG, tsync_rx_cfg); + + /* define ethertype filter for timestamped packets */ + if (is_l2) + wr32(E1000_ETQF(3), + (E1000_ETQF_FILTER_ENABLE | /* enable filter */ + E1000_ETQF_1588 | /* enable timestamping */ + ETH_P_1588)); /* 1588 eth protocol type */ + else + wr32(E1000_ETQF(3), 0); + +#define PTP_PORT 319 + /* L4 Queue Filter[3]: filter by destination port and protocol */ + if (is_l4) { + u32 ftqf = (IPPROTO_UDP /* UDP */ + | E1000_FTQF_VF_BP /* VF not compared */ + | E1000_FTQF_1588_TIME_STAMP /* Enable Timestamping */ + | E1000_FTQF_MASK); /* mask all inputs */ + ftqf &= ~E1000_FTQF_MASK_PROTO_BP; /* enable protocol check */ + + wr32(E1000_IMIR(3), htons(PTP_PORT)); + wr32(E1000_IMIREXT(3), + (E1000_IMIREXT_SIZE_BP | E1000_IMIREXT_CTRL_BP)); + if (hw->mac.type == e1000_82576) { + /* enable source port check */ + wr32(E1000_SPQF(3), htons(PTP_PORT)); + ftqf &= ~E1000_FTQF_MASK_SOURCE_PORT_BP; + } + wr32(E1000_FTQF(3), ftqf); + } else { + wr32(E1000_FTQF(3), E1000_FTQF_MASK); + } + wrfl(); + + /* clear TX/RX time stamp registers, just to be sure */ + regval = rd32(E1000_TXSTMPH); + regval = rd32(E1000_RXSTMPH); + + return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ? + -EFAULT : 0; } void igb_ptp_init(struct igb_adapter *adapter) @@ -266,39 +553,39 @@ void igb_ptp_init(struct igb_adapter *adapter) case e1000_i211: case e1000_i350: case e1000_82580: - adapter->caps.owner = THIS_MODULE; - strcpy(adapter->caps.name, "igb-82580"); - adapter->caps.max_adj = 62499999; - adapter->caps.n_ext_ts = 0; - adapter->caps.pps = 0; - adapter->caps.adjfreq = ptp_82580_adjfreq; - adapter->caps.adjtime = igb_adjtime; - adapter->caps.gettime = igb_gettime; - adapter->caps.settime = igb_settime; - adapter->caps.enable = ptp_82580_enable; - adapter->cc.read = igb_82580_systim_read; - adapter->cc.mask = CLOCKSOURCE_MASK(IGB_NBITS_82580); - adapter->cc.mult = 1; - adapter->cc.shift = 0; + adapter->ptp_caps.owner = THIS_MODULE; + strcpy(adapter->ptp_caps.name, "igb-82580"); + adapter->ptp_caps.max_adj = 62499999; + adapter->ptp_caps.n_ext_ts = 0; + adapter->ptp_caps.pps = 0; + adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580; + adapter->ptp_caps.adjtime = igb_ptp_adjtime; + adapter->ptp_caps.gettime = igb_ptp_gettime; + adapter->ptp_caps.settime = igb_ptp_settime; + adapter->ptp_caps.enable = igb_ptp_enable; + adapter->cc.read = igb_ptp_read_82580; + adapter->cc.mask = CLOCKSOURCE_MASK(IGB_NBITS_82580); + adapter->cc.mult = 1; + adapter->cc.shift = 0; /* Enable the timer functions by clearing bit 31. */ wr32(E1000_TSAUXC, 0x0); break; case e1000_82576: - adapter->caps.owner = THIS_MODULE; - strcpy(adapter->caps.name, "igb-82576"); - adapter->caps.max_adj = 1000000000; - adapter->caps.n_ext_ts = 0; - adapter->caps.pps = 0; - adapter->caps.adjfreq = ptp_82576_adjfreq; - adapter->caps.adjtime = igb_adjtime; - adapter->caps.gettime = igb_gettime; - adapter->caps.settime = igb_settime; - adapter->caps.enable = ptp_82576_enable; - adapter->cc.read = igb_82576_systim_read; - adapter->cc.mask = CLOCKSOURCE_MASK(64); - adapter->cc.mult = 1; - adapter->cc.shift = IGB_82576_TSYNC_SHIFT; + adapter->ptp_caps.owner = THIS_MODULE; + strcpy(adapter->ptp_caps.name, "igb-82576"); + adapter->ptp_caps.max_adj = 1000000000; + adapter->ptp_caps.n_ext_ts = 0; + adapter->ptp_caps.pps = 0; + adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82576; + adapter->ptp_caps.adjtime = igb_ptp_adjtime; + adapter->ptp_caps.gettime = igb_ptp_gettime; + adapter->ptp_caps.settime = igb_ptp_settime; + adapter->ptp_caps.enable = igb_ptp_enable; + adapter->cc.read = igb_ptp_read_82576; + adapter->cc.mask = CLOCKSOURCE_MASK(64); + adapter->cc.mult = 1; + adapter->cc.shift = IGB_82576_TSYNC_SHIFT; /* Dial the nominal frequency. */ wr32(E1000_TIMINCA, INCPERIOD_82576 | INCVALUE_82576); break; @@ -313,13 +600,14 @@ void igb_ptp_init(struct igb_adapter *adapter) timecounter_init(&adapter->tc, &adapter->cc, ktime_to_ns(ktime_get_real())); - INIT_DELAYED_WORK(&adapter->overflow_work, igb_overflow_check); + INIT_DELAYED_WORK(&adapter->ptp_overflow_work, igb_ptp_overflow_check); spin_lock_init(&adapter->tmreg_lock); - schedule_delayed_work(&adapter->overflow_work, IGB_OVERFLOW_PERIOD); + schedule_delayed_work(&adapter->ptp_overflow_work, + IGB_SYSTIM_OVERFLOW_PERIOD); - adapter->ptp_clock = ptp_clock_register(&adapter->caps); + adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps); if (IS_ERR(adapter->ptp_clock)) { adapter->ptp_clock = NULL; dev_err(&adapter->pdev->dev, "ptp_clock_register failed\n"); @@ -328,7 +616,13 @@ void igb_ptp_init(struct igb_adapter *adapter) adapter->netdev->name); } -void igb_ptp_remove(struct igb_adapter *adapter) +/** + * igb_ptp_stop - Disable PTP device and stop the overflow check. + * @adapter: Board private structure. + * + * This function stops the PTP support and cancels the delayed work. + **/ +void igb_ptp_stop(struct igb_adapter *adapter) { switch (adapter->hw.mac.type) { case e1000_i211: @@ -336,7 +630,7 @@ void igb_ptp_remove(struct igb_adapter *adapter) case e1000_i350: case e1000_82580: case e1000_82576: - cancel_delayed_work_sync(&adapter->overflow_work); + cancel_delayed_work_sync(&adapter->ptp_overflow_work); break; default: return; @@ -348,48 +642,3 @@ void igb_ptp_remove(struct igb_adapter *adapter) adapter->netdev->name); } } - -/** - * igb_systim_to_hwtstamp - convert system time value to hw timestamp - * @adapter: board private structure - * @hwtstamps: timestamp structure to update - * @systim: unsigned 64bit system time value. - * - * We need to convert the system time value stored in the RX/TXSTMP registers - * into a hwtstamp which can be used by the upper level timestamping functions. - * - * The 'tmreg_lock' spinlock is used to protect the consistency of the - * system time value. This is needed because reading the 64 bit time - * value involves reading two (or three) 32 bit registers. The first - * read latches the value. Ditto for writing. - * - * In addition, here have extended the system time with an overflow - * counter in software. - **/ -void igb_systim_to_hwtstamp(struct igb_adapter *adapter, - struct skb_shared_hwtstamps *hwtstamps, - u64 systim) -{ - u64 ns; - unsigned long flags; - - switch (adapter->hw.mac.type) { - case e1000_i210: - case e1000_i211: - case e1000_i350: - case e1000_82580: - case e1000_82576: - break; - default: - return; - } - - spin_lock_irqsave(&adapter->tmreg_lock, flags); - - ns = timecounter_cyc2time(&adapter->tc, systim); - - spin_unlock_irqrestore(&adapter->tmreg_lock, flags); - - memset(hwtstamps, 0, sizeof(*hwtstamps)); - hwtstamps->hwtstamp = ns_to_ktime(ns); -}