[PATCH 0/5] net: fec: fix external PTP PHY support

[PATCH 0/5] net: fec: fix external PTP PHY support

[Sergey Organov]

This is a collection of otherwise independent fixes that got developed
out of attempt to use DP83640 PTP PHY connected to built-in fec1 (that
has its own PTP support) of the iMX 6SX micro-controller.

The first patch in the series is actual fix for appeared problems,
while the rest are small style or behavior improvements made during
development of the fix.

NOTE: the patches are developed and tested on 4.9.146, and rebased on
top of recent 'master', where, besides visual inspection, I only
tested that they do compile.

NOTE: for more background, please refer here:

https://lore.kernel.org/netdev/87r1uqtybr.fsf@osv.gnss.ru/

Sergey Organov (5):
  net: fec: properly support external PTP PHY for hardware time stamping
  net: fec: enable to use PPS feature without time stamping
  net: fec: initialize clock with 0 rather than current kernel time
  net: fec: get rid of redundant code in fec_ptp_set()
  net: fec: replace snprintf() with strlcpy() in fec_ptp_init()

 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c | 19 +++++++++++++++----
 drivers/net/ethernet/freescale/fec_ptp.c  | 24 ++++++++++++++----------
 3 files changed, 30 insertions(+), 14 deletions(-)

-- 
2.10.0.1.g57b01a3

[PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Sergey Organov]

When external PTP-aware PHY is in use, it's that PHY that is to time
stamp network packets, and it's that PHY where configuration requests
of time stamping features are to be routed.

To achieve these goals:

1. Make sure we don't time stamp packets when external PTP PHY is in use

2. Make sure we redirect ioctl() related to time stamping of Ethernet
   packets to connected PTP PHY rather than handle them ourselves

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c | 18 ++++++++++++++----
 drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index a6cdd5b6..de9f46a 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -595,6 +595,7 @@ struct fec_enet_private {
 void fec_ptp_init(struct platform_device *pdev, int irq_idx);
 void fec_ptp_stop(struct platform_device *pdev);
 void fec_ptp_start_cyclecounter(struct net_device *ndev);
+void fec_ptp_disable_hwts(struct net_device *ndev);
 int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
 int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 2d0d313..995ea2e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1298,7 +1298,11 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
 			ndev->stats.tx_bytes += skb->len;
 		}
 
+		/* It could be external PHY that had set SKBTX_IN_PROGRESS, so
+		 * we still need to check it's we who are to time stamp
+		 */
 		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
+		    unlikely(fep->hwts_tx_en) &&
 			fep->bufdesc_ex) {
 			struct skb_shared_hwtstamps shhwtstamps;
 			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
@@ -2755,10 +2759,16 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
 		return -ENODEV;
 
 	if (fep->bufdesc_ex) {
-		if (cmd == SIOCSHWTSTAMP)
-			return fec_ptp_set(ndev, rq);
-		if (cmd == SIOCGHWTSTAMP)
-			return fec_ptp_get(ndev, rq);
+		bool use_fec_hwts = !phy_has_hwtstamp(phydev);
+
+		if (cmd == SIOCSHWTSTAMP) {
+			if (use_fec_hwts)
+				return fec_ptp_set(ndev, rq);
+			fec_ptp_disable_hwts(ndev);
+		} else if (cmd == SIOCGHWTSTAMP) {
+			if (use_fec_hwts)
+				return fec_ptp_get(ndev, rq);
+		}
 	}
 
 	return phy_mii_ioctl(phydev, rq, cmd);
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 945643c..f8a592c 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -452,6 +452,18 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
 	return -EOPNOTSUPP;
 }
 
+/**
+ * fec_ptp_disable_hwts - disable hardware time stamping
+ * @ndev: pointer to net_device
+ */
+void fec_ptp_disable_hwts(struct net_device *ndev)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	fep->hwts_tx_en = 0;
+	fep->hwts_rx_en = 0;
+}
+
 int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
-- 
2.10.0.1.g57b01a3

Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Vladimir Oltean]

Hi Sergey,

On Mon, Jul 06, 2020 at 05:26:12PM +0300, Sergey Organov wrote:
> When external PTP-aware PHY is in use, it's that PHY that is to time
> stamp network packets, and it's that PHY where configuration requests
> of time stamping features are to be routed.
> 
> To achieve these goals:
> 
> 1. Make sure we don't time stamp packets when external PTP PHY is in use
> 
> 2. Make sure we redirect ioctl() related to time stamping of Ethernet
>    packets to connected PTP PHY rather than handle them ourselves
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  1 +
>  drivers/net/ethernet/freescale/fec_main.c | 18 ++++++++++++++----
>  drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
>  3 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index a6cdd5b6..de9f46a 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -595,6 +595,7 @@ struct fec_enet_private {
>  void fec_ptp_init(struct platform_device *pdev, int irq_idx);
>  void fec_ptp_stop(struct platform_device *pdev);
>  void fec_ptp_start_cyclecounter(struct net_device *ndev);
> +void fec_ptp_disable_hwts(struct net_device *ndev);
>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
>  int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
>  
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 2d0d313..995ea2e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1298,7 +1298,11 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>  			ndev->stats.tx_bytes += skb->len;
>  		}
>  
> +		/* It could be external PHY that had set SKBTX_IN_PROGRESS, so
> +		 * we still need to check it's we who are to time stamp
> +		 */
>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
> +		    unlikely(fep->hwts_tx_en) &&

I think this could qualify as a pretty significant fix in its own right,
that should go to stable trees. Right now, this patch appears pretty
easy to overlook.

Is this the same situation as what is being described here for the
gianfar driver?

https://patchwork.ozlabs.org/project/netdev/patch/20191227004435.21692-2-olteanv@gmail.com/

If so, it is interesting because I thought we had agreed that it's only
DSA who suffers from the double-TX-timestamp design issue, not PHYTER.
Not to mention, interesting because FEC + a timestamping DSA switch such
as mv88e6xxx is not unheard of. Hmmm...

>  			fep->bufdesc_ex) {
>  			struct skb_shared_hwtstamps shhwtstamps;
>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
> @@ -2755,10 +2759,16 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
>  		return -ENODEV;
>  
>  	if (fep->bufdesc_ex) {
> -		if (cmd == SIOCSHWTSTAMP)
> -			return fec_ptp_set(ndev, rq);
> -		if (cmd == SIOCGHWTSTAMP)
> -			return fec_ptp_get(ndev, rq);
> +		bool use_fec_hwts = !phy_has_hwtstamp(phydev);
> +
> +		if (cmd == SIOCSHWTSTAMP) {
> +			if (use_fec_hwts)
> +				return fec_ptp_set(ndev, rq);
> +			fec_ptp_disable_hwts(ndev);
> +		} else if (cmd == SIOCGHWTSTAMP) {
> +			if (use_fec_hwts)
> +				return fec_ptp_get(ndev, rq);
> +		}
>  	}
>  
>  	return phy_mii_ioctl(phydev, rq, cmd);
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 945643c..f8a592c 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -452,6 +452,18 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
>  	return -EOPNOTSUPP;
>  }
>  
> +/**
> + * fec_ptp_disable_hwts - disable hardware time stamping
> + * @ndev: pointer to net_device
> + */
> +void fec_ptp_disable_hwts(struct net_device *ndev)
> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +
> +	fep->hwts_tx_en = 0;
> +	fep->hwts_rx_en = 0;
> +}
> +
>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -- 
> 2.10.0.1.g57b01a3
> 

Cheers,
-Vladimir

Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> Hi Sergey,
>
> On Mon, Jul 06, 2020 at 05:26:12PM +0300, Sergey Organov wrote:
>> When external PTP-aware PHY is in use, it's that PHY that is to time
>> stamp network packets, and it's that PHY where configuration requests
>> of time stamping features are to be routed.
>> 
>> To achieve these goals:
>> 
>> 1. Make sure we don't time stamp packets when external PTP PHY is in use
>> 
>> 2. Make sure we redirect ioctl() related to time stamping of Ethernet
>>    packets to connected PTP PHY rather than handle them ourselves

[...]

>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index 2d0d313..995ea2e 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1298,7 +1298,11 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>>  			ndev->stats.tx_bytes += skb->len;
>>  		}
>>  
>> +		/* It could be external PHY that had set SKBTX_IN_PROGRESS, so
>> +		 * we still need to check it's we who are to time stamp
>> +		 */
>>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
>> +		    unlikely(fep->hwts_tx_en) &&
>
> I think this could qualify as a pretty significant fix in its own right,
> that should go to stable trees. Right now, this patch appears pretty
> easy to overlook.
>
> Is this the same situation as what is being described here for the
> gianfar driver?
>
> https://patchwork.ozlabs.org/project/netdev/patch/20191227004435.21692-2-olteanv@gmail.com/

Yes, it sounds exactly like that!

However, I'd insist that the second part of the patch is as important.
Please refer to my original post for the description of nasty confusion
the second part of the patch fixes:

https://lore.kernel.org/netdev/87r1uqtybr.fsf@osv.gnss.ru/

Basically, you get PHY response when you ask for capabilities, but then
MAC executes ioctl() request for corresponding configuration!

[...]

Thanks,
-- Sergey

Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Vladimir Oltean]

On Mon, Jul 06, 2020 at 06:21:59PM +0300, Sergey Organov wrote:
> Vladimir Oltean <olteanv@gmail.com> writes:
> 
> > Hi Sergey,
> >
> > On Mon, Jul 06, 2020 at 05:26:12PM +0300, Sergey Organov wrote:
> >> When external PTP-aware PHY is in use, it's that PHY that is to time
> >> stamp network packets, and it's that PHY where configuration requests
> >> of time stamping features are to be routed.
> >> 
> >> To achieve these goals:
> >> 
> >> 1. Make sure we don't time stamp packets when external PTP PHY is in use
> >> 
> >> 2. Make sure we redirect ioctl() related to time stamping of Ethernet
> >>    packets to connected PTP PHY rather than handle them ourselves
> 
> [...]
> 
> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> >> index 2d0d313..995ea2e 100644
> >> --- a/drivers/net/ethernet/freescale/fec_main.c
> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> @@ -1298,7 +1298,11 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> >>  			ndev->stats.tx_bytes += skb->len;
> >>  		}
> >>  
> >> +		/* It could be external PHY that had set SKBTX_IN_PROGRESS, so
> >> +		 * we still need to check it's we who are to time stamp
> >> +		 */
> >>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
> >> +		    unlikely(fep->hwts_tx_en) &&
> >
> > I think this could qualify as a pretty significant fix in its own right,
> > that should go to stable trees. Right now, this patch appears pretty
> > easy to overlook.
> >
> > Is this the same situation as what is being described here for the
> > gianfar driver?
> >
> > https://patchwork.ozlabs.org/project/netdev/patch/20191227004435.21692-2-olteanv@gmail.com/
> 
> Yes, it sounds exactly like that!
> 

Cool. Join the club! You were lucky though, in your case it was pretty
evident where the problem might be, so you were already on your way even
though you didn't know exactly what was going on.

Towards the point that you brought up in that thread:

> Could somebody please help me implement (or point me to) proper fix to
> reliably use external PHY to timestamp network packets?

We do it like this:
- DSA: If there is a timestamping switch stacked on top of a
  timestamping Ethernet MAC, the switch hijacks the .ndo_do_ioctl of the
  host port, and you are supposed to use the PTP clock of the switch,
  through the .ndo_do_ioctl of its own (virtual) net devices. This
  approach works without changing any code in each individual Ethernet
  MAC driver.
- PHY: The Ethernet MAC driver needs to be kind enough to check whether
  the PHY supports hw timestamping, and pass this ioctl to that PHY
  while making sure it doesn't do anything stupid in the meanwhile, like
  also acting upon that timestamping request itself.

Both are finicky in their own ways. There is no real way for the user to
select which PHC they want to use. The assumption is that you'd always
want to use the outermost one, and that things in the kernel side always
collaborate towards that end.

> However, I'd insist that the second part of the patch is as important.
> Please refer to my original post for the description of nasty confusion
> the second part of the patch fixes:
> 
> https://lore.kernel.org/netdev/87r1uqtybr.fsf@osv.gnss.ru/
> 
> Basically, you get PHY response when you ask for capabilities, but then
> MAC executes ioctl() request for corresponding configuration!
> 
> [...]
> 

Yup, sure, _but_ my point is: PHY timestamping is not supposed to work
unless you do that phy_has_hwtstamp dance in .ndo_do_ioctl and pass it
to the PHY driver. Whereas, timestamping on a DSA switch is supposed to
just work. So, the double-TX-timestamp fix is common for both DSA and
PHY timestamping, and it should be a separate patch that goes to David's
"net" tree and has an according Fixes: tag for the stable people to pick
it up. Then, the PHY timestamping patch is technically a new feature,
because the driver wasn't looking at the PHY's ability to perform PTP
timestamping, and now it does. So that part is a patch for "net-next".

> Thanks,
> -- Sergey

Thank you,
-Vladimir

Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Mon, Jul 06, 2020 at 06:21:59PM +0300, Sergey Organov wrote:
>> Vladimir Oltean <olteanv@gmail.com> writes:
 
>> > Hi Sergey,
>> >
>> > On Mon, Jul 06, 2020 at 05:26:12PM +0300, Sergey Organov wrote:
>> >> When external PTP-aware PHY is in use, it's that PHY that is to time
>> >> stamp network packets, and it's that PHY where configuration requests
>> >> of time stamping features are to be routed.
>> >> 
>> >> To achieve these goals:
>> >> 
>> >> 1. Make sure we don't time stamp packets when external PTP PHY is in use
>> >> 
>> >> 2. Make sure we redirect ioctl() related to time stamping of Ethernet
>> >>    packets to connected PTP PHY rather than handle them ourselves
>> 
>> [...]
>> 
>> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> >> index 2d0d313..995ea2e 100644
>> >> --- a/drivers/net/ethernet/freescale/fec_main.c
>> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> >> @@ -1298,7 +1298,11 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>> >>  			ndev->stats.tx_bytes += skb->len;
>> >>  		}
>> >>  
>> >> +		/* It could be external PHY that had set SKBTX_IN_PROGRESS, so
>> >> +		 * we still need to check it's we who are to time stamp
>> >> +		 */
>> >>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
>> >> +		    unlikely(fep->hwts_tx_en) &&
>> >
>> > I think this could qualify as a pretty significant fix in its own right,
>> > that should go to stable trees. Right now, this patch appears pretty
>> > easy to overlook.
>> >
>> > Is this the same situation as what is being described here for the
>> > gianfar driver?
>> >
>> > https://patchwork.ozlabs.org/project/netdev/patch/20191227004435.21692-2-olteanv@gmail.com/
>> 
>> Yes, it sounds exactly like that!
>> 
>
> Cool. Join the club! You were lucky though, in your case it was pretty
> evident where the problem might be, so you were already on your way even
> though you didn't know exactly what was going on.
>
> Towards the point that you brought up in that thread:
>
>> Could somebody please help me implement (or point me to) proper fix to
>> reliably use external PHY to timestamp network packets?
>
> We do it like this:
> - DSA: If there is a timestamping switch stacked on top of a
>   timestamping Ethernet MAC, the switch hijacks the .ndo_do_ioctl of the
>   host port, and you are supposed to use the PTP clock of the switch,
>   through the .ndo_do_ioctl of its own (virtual) net devices. This
>   approach works without changing any code in each individual Ethernet
>   MAC driver.
> - PHY: The Ethernet MAC driver needs to be kind enough to check whether
>   the PHY supports hw timestamping, and pass this ioctl to that PHY
>   while making sure it doesn't do anything stupid in the meanwhile, like
>   also acting upon that timestamping request itself.
>
> Both are finicky in their own ways. There is no real way for the user to
> select which PHC they want to use. The assumption is that you'd always
> want to use the outermost one, and that things in the kernel side always
> collaborate towards that end.

Makes sense, -- thanks for clarification! Indeed, if somebody connected
that external thingy, chances are high it was made for a purpose.

>
>> However, I'd insist that the second part of the patch is as important.
>> Please refer to my original post for the description of nasty confusion
>> the second part of the patch fixes:
>> 
>> https://lore.kernel.org/netdev/87r1uqtybr.fsf@osv.gnss.ru/
>> 
>> Basically, you get PHY response when you ask for capabilities, but then
>> MAC executes ioctl() request for corresponding configuration!
>> 
>> [...]
>> 
>
> Yup, sure, _but_ my point is: PHY timestamping is not supposed to work
> unless you do that phy_has_hwtstamp dance in .ndo_do_ioctl and pass it
> to the PHY driver. Whereas, timestamping on a DSA switch is supposed to
> just work. So, the double-TX-timestamp fix is common for both DSA and
> PHY timestamping, and it should be a separate patch that goes to David's
> "net" tree and has an according Fixes: tag for the stable people to pick
> it up. Then, the PHY timestamping patch is technically a new feature,
> because the driver wasn't looking at the PHY's ability to perform PTP
> timestamping, and now it does. So that part is a patch for "net-next".

Ah, thanks, now it makes sense! I simply was not aware of the DSA
(whatever it is) you've mentioned above.

I'll then make these 2 changes separate in v2 indeed, though I'm not
aware about Fixes: tag and if I should do something about it. Any clues?

Thanks,
-- Sergey

Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Vladimir Oltean]

On Mon, Jul 06, 2020 at 09:33:30PM +0300, Sergey Organov wrote:
> Vladimir Oltean <olteanv@gmail.com> writes:
> 
> > On Mon, Jul 06, 2020 at 06:21:59PM +0300, Sergey Organov wrote:
> >> Vladimir Oltean <olteanv@gmail.com> writes:
>  
> >> > Hi Sergey,
> >> >
> >> > On Mon, Jul 06, 2020 at 05:26:12PM +0300, Sergey Organov wrote:
> >> >> When external PTP-aware PHY is in use, it's that PHY that is to time
> >> >> stamp network packets, and it's that PHY where configuration requests
> >> >> of time stamping features are to be routed.
> >> >> 
> >> >> To achieve these goals:
> >> >> 
> >> >> 1. Make sure we don't time stamp packets when external PTP PHY is in use
> >> >> 
> >> >> 2. Make sure we redirect ioctl() related to time stamping of Ethernet
> >> >>    packets to connected PTP PHY rather than handle them ourselves
> >> 
> >> [...]
> >> 
> >> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> >> >> index 2d0d313..995ea2e 100644
> >> >> --- a/drivers/net/ethernet/freescale/fec_main.c
> >> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> >> @@ -1298,7 +1298,11 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> >> >>  			ndev->stats.tx_bytes += skb->len;
> >> >>  		}
> >> >>  
> >> >> +		/* It could be external PHY that had set SKBTX_IN_PROGRESS, so
> >> >> +		 * we still need to check it's we who are to time stamp
> >> >> +		 */
> >> >>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
> >> >> +		    unlikely(fep->hwts_tx_en) &&
> >> >
> >> > I think this could qualify as a pretty significant fix in its own right,
> >> > that should go to stable trees. Right now, this patch appears pretty
> >> > easy to overlook.
> >> >
> >> > Is this the same situation as what is being described here for the
> >> > gianfar driver?
> >> >
> >> > https://patchwork.ozlabs.org/project/netdev/patch/20191227004435.21692-2-olteanv@gmail.com/
> >> 
> >> Yes, it sounds exactly like that!
> >> 
> >
> > Cool. Join the club! You were lucky though, in your case it was pretty
> > evident where the problem might be, so you were already on your way even
> > though you didn't know exactly what was going on.
> >
> > Towards the point that you brought up in that thread:
> >
> >> Could somebody please help me implement (or point me to) proper fix to
> >> reliably use external PHY to timestamp network packets?
> >
> > We do it like this:
> > - DSA: If there is a timestamping switch stacked on top of a
> >   timestamping Ethernet MAC, the switch hijacks the .ndo_do_ioctl of the
> >   host port, and you are supposed to use the PTP clock of the switch,
> >   through the .ndo_do_ioctl of its own (virtual) net devices. This
> >   approach works without changing any code in each individual Ethernet
> >   MAC driver.
> > - PHY: The Ethernet MAC driver needs to be kind enough to check whether
> >   the PHY supports hw timestamping, and pass this ioctl to that PHY
> >   while making sure it doesn't do anything stupid in the meanwhile, like
> >   also acting upon that timestamping request itself.
> >
> > Both are finicky in their own ways. There is no real way for the user to
> > select which PHC they want to use. The assumption is that you'd always
> > want to use the outermost one, and that things in the kernel side always
> > collaborate towards that end.
> 
> Makes sense, -- thanks for clarification! Indeed, if somebody connected
> that external thingy, chances are high it was made for a purpose.
> 
> >
> >> However, I'd insist that the second part of the patch is as important.
> >> Please refer to my original post for the description of nasty confusion
> >> the second part of the patch fixes:
> >> 
> >> https://lore.kernel.org/netdev/87r1uqtybr.fsf@osv.gnss.ru/
> >> 
> >> Basically, you get PHY response when you ask for capabilities, but then
> >> MAC executes ioctl() request for corresponding configuration!
> >> 
> >> [...]
> >> 
> >
> > Yup, sure, _but_ my point is: PHY timestamping is not supposed to work
> > unless you do that phy_has_hwtstamp dance in .ndo_do_ioctl and pass it
> > to the PHY driver. Whereas, timestamping on a DSA switch is supposed to
> > just work. So, the double-TX-timestamp fix is common for both DSA and
> > PHY timestamping, and it should be a separate patch that goes to David's
> > "net" tree and has an according Fixes: tag for the stable people to pick
> > it up. Then, the PHY timestamping patch is technically a new feature,
> > because the driver wasn't looking at the PHY's ability to perform PTP
> > timestamping, and now it does. So that part is a patch for "net-next".
> 
> Ah, thanks, now it makes sense! I simply was not aware of the DSA
> (whatever it is) you've mentioned above.
> 

https://netdevconf.info/2.1/papers/distributed-switch-architecture.pdf

> I'll then make these 2 changes separate in v2 indeed, though I'm not
> aware about Fixes: tag and if I should do something about it. Any clues?
> 

Add these 2 lines to your .gitconfig file:

[pretty]
	fixes = Fixes: %h (\"%s\")

Then use $(git blame) to find the commit which introduced the bad
behavior. I was able to go down back to this commit, which I then tagged
as follows:

git show 6605b730c061f67c44113391e5af5125d0672e99 --pretty=fixes

Then you copy the first line of the generated output to the patch, right
above your Signed-off-by: tag. Like this:

Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware clock")

Note that the offending commit has been obscured, in the meantime, by
refactoring commit ff43da86c69d ("NET: FEC: dynamtic check DMA desc buff
type"). That doesn't mean that the Fixes: tag should point to the newest
commit touching the code though. In case where the refactoring is recent
though (not this case), Greg will send an email that backporting failed,
and you can send him a follow-up with a patch adjusted for each
individual stable tree where adjustments need to be made. You can also
ignore Greg's email, if you don't care about old stable trees.

In this particular case, the original offending commit and the one
obscuring it were included first in the following kernel tags:

$(git tag --contains 6605b730c061): v3.8
$(git tag --contains ff43da86c69d): v3.9

But, if you look at https://www.kernel.org/, the oldest stable tree
being actively maintained should be 3.16, so v3.8 vs v3.9 shouldn't make
any difference because nobody will try to apply your fix patch to a tree
older than 3.9 anyway.

When sending a bugfix patch, there are 2 options:

- You send the patch to the linux-stable mailing list directly. For
  networking fixes, however, David doesn't prefer this. See below.

- You send the patch to the netdev list (the same list where you sent
  this one), but with --subject-prefix "PATCH net" so that it gets
  applied to a different tree (this one:
  https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git as
  opposed to this one:
  https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git).
  The "net" tree is periodically merged into "net-next". Because your
  patch series will have to be split, there are 2 options: either you
  send your bugfix patches first, wait for them to be merged, and then
  for "net" to be merged into "net-next", or try somehow to make sure
  that the patches for "net" and for "net-next" can be applied in
  parallel without interfering and creating merge conflicts. I think you
  can do the latter.

Whatever you do, however, please be sure to copy Richard Cochran to
PTP-related patches, he tends to have a broader picture of the 1588 work
that is being done throughout the kernel, and can provide more feedback.

> Thanks,
> -- Sergey
> 

Thanks,
-Vladimir

Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Mon, Jul 06, 2020 at 09:33:30PM +0300, Sergey Organov wrote:

[...]

>
>> I'll then make these 2 changes separate in v2 indeed, though I'm not
>> aware about Fixes: tag and if I should do something about it. Any clues?
>> 
>
> Add these 2 lines to your .gitconfig file:
>
> [pretty]
> 	fixes = Fixes: %h (\"%s\")
>
> Then use $(git blame) to find the commit which introduced the bad
> behavior. I was able to go down back to this commit, which I then tagged
> as follows:
>
> git show 6605b730c061f67c44113391e5af5125d0672e99 --pretty=fixes
>
> Then you copy the first line of the generated output to the patch, right
> above your Signed-off-by: tag. Like this:
>
> Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware clock")
>
> Note that the offending commit has been obscured, in the meantime, by
> refactoring commit ff43da86c69d ("NET: FEC: dynamtic check DMA desc buff
> type"). That doesn't mean that the Fixes: tag should point to the newest
> commit touching the code though. In case where the refactoring is recent
> though (not this case), Greg will send an email that backporting failed,
> and you can send him a follow-up with a patch adjusted for each
> individual stable tree where adjustments need to be made. You can also
> ignore Greg's email, if you don't care about old stable trees.
>
> In this particular case, the original offending commit and the one
> obscuring it were included first in the following kernel tags:
>
> $(git tag --contains 6605b730c061): v3.8
> $(git tag --contains ff43da86c69d): v3.9
>
> But, if you look at https://www.kernel.org/, the oldest stable tree
> being actively maintained should be 3.16, so v3.8 vs v3.9 shouldn't make
> any difference because nobody will try to apply your fix patch to a tree
> older than 3.9 anyway.
>
> When sending a bugfix patch, there are 2 options:
>
> - You send the patch to the linux-stable mailing list directly. For
>   networking fixes, however, David doesn't prefer this. See below.
>
> - You send the patch to the netdev list (the same list where you sent
>   this one), but with --subject-prefix "PATCH net" so that it gets
>   applied to a different tree (this one:
>   https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git as
>   opposed to this one:
>   https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git).
>   The "net" tree is periodically merged into "net-next". Because your
>   patch series will have to be split, there are 2 options: either you
>   send your bugfix patches first, wait for them to be merged, and then
>   for "net" to be merged into "net-next", or try somehow to make sure
>   that the patches for "net" and for "net-next" can be applied in
>   parallel without interfering and creating merge conflicts. I think you
>   can do the latter.
>
> Whatever you do, however, please be sure to copy Richard Cochran to
> PTP-related patches, he tends to have a broader picture of the 1588 work
> that is being done throughout the kernel, and can provide more
> feedback.

Thanks a lot for thorough explanations and for finding the offensive
commit for me!

I'll then start with sending that separate patch as bug-fix with "PATCH net"
subject prefix, and then will re-send v2 of the series to net-next (with
just "PATCH v2") later, as soon as I collect all the feedback. I expect
no merge conflicts indeed.

Sounds like a plan!

Thanks again,
-- Sergey

Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Richard Cochran]

On Tue, Jul 07, 2020 at 10:04:37AM +0300, Vladimir Oltean wrote:
> > > We do it like this:
> > > - DSA: If there is a timestamping switch stacked on top of a
> > >   timestamping Ethernet MAC, the switch hijacks the .ndo_do_ioctl of the
> > >   host port, and you are supposed to use the PTP clock of the switch,
> > >   through the .ndo_do_ioctl of its own (virtual) net devices. This
> > >   approach works without changing any code in each individual Ethernet
> > >   MAC driver.
> > > - PHY: The Ethernet MAC driver needs to be kind enough to check whether
> > >   the PHY supports hw timestamping, and pass this ioctl to that PHY
> > >   while making sure it doesn't do anything stupid in the meanwhile, like
> > >   also acting upon that timestamping request itself.
> > >
> > > Both are finicky in their own ways. There is no real way for the user to
> > > select which PHC they want to use. The assumption is that you'd always
> > > want to use the outermost one, and that things in the kernel side always
> > > collaborate towards that end.

Vladimir, your explanations in this thread are valuable.  Please
consider converting them into a patch to expand

   Documentation/networking/timestamping.rst


Thanks,
Richard

Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Richard Cochran]

On Mon, Jul 06, 2020 at 09:33:30PM +0300, Sergey Organov wrote:
> Vladimir Oltean <olteanv@gmail.com> writes:
> 
> > On Mon, Jul 06, 2020 at 06:21:59PM +0300, Sergey Organov wrote:
> > Both are finicky in their own ways. There is no real way for the user to
> > select which PHC they want to use. The assumption is that you'd always
> > want to use the outermost one, and that things in the kernel side always
> > collaborate towards that end.

+1

In addition, for PHY time stamping you must enable the costly
CONFIG_NETWORK_PHY_TIMESTAMPING option at compile time, and so the
user most definitely wants the "outer" function.

> Makes sense, -- thanks for clarification! Indeed, if somebody connected
> that external thingy, chances are high it was made for a purpose.

Yes, exactly.

Thanks,
Richard

[PATCH 2/5] net: fec: enable to use PPS feature without time stamping

[Sergey Organov]

PPS feature could be useful even when hardware time stamping
of network packets is not in use, so remove offending check
for this condition from fec_ptp_enable_pps().

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index f8a592c..4a12086 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -103,11 +103,6 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
 	u64 ns;
 	val = 0;
 
-	if (!(fep->hwts_tx_en || fep->hwts_rx_en)) {
-		dev_err(&fep->pdev->dev, "No ptp stack is running\n");
-		return -EINVAL;
-	}
-
 	if (fep->pps_enable == enable)
 		return 0;
 
-- 
2.10.0.1.g57b01a3

RE: [EXT] [PATCH 2/5] net: fec: enable to use PPS feature without time stamping

[Andy Duan]

From: Sergey Organov <sorganov@gmail.com> Sent: Monday, July 6, 2020 10:26 PM
> PPS feature could be useful even when hardware time stamping of network
> packets is not in use, so remove offending check for this condition from
> fec_ptp_enable_pps().

If hardware time stamping of network packets is not in use, PPS is based on local
clock, what is the use case ?

> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index f8a592c..4a12086 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -103,11 +103,6 @@ static int fec_ptp_enable_pps(struct
> fec_enet_private *fep, uint enable)
>         u64 ns;
>         val = 0;
> 
> -       if (!(fep->hwts_tx_en || fep->hwts_rx_en)) {
> -               dev_err(&fep->pdev->dev, "No ptp stack is running\n");
> -               return -EINVAL;
> -       }
> -
>         if (fep->pps_enable == enable)
>                 return 0;
> 
> --
> 2.10.0.1.g57b01a3

Re: [EXT] [PATCH 2/5] net: fec: enable to use PPS feature without time stamping

[Sergey Organov]

Andy Duan <fugang.duan@nxp.com> writes:

> From: Sergey Organov <sorganov@gmail.com> Sent: Monday, July 6, 2020 10:26 PM
>> PPS feature could be useful even when hardware time stamping of network
>> packets is not in use, so remove offending check for this condition from
>> fec_ptp_enable_pps().
>
> If hardware time stamping of network packets is not in use, PPS is
> based on local
> clock, what is the use case ?

First, having special code to disable something that does no harm seems
to be wrong idea in general. In this particular case, if PPS is not
needed, it is still disabled by default, and one is still free not to
use it.

Then, as I'm not aware of a rule that renders PPS based on local clock
useless, I'm to assume it might be useful.

Finally, as an attempt to give direct answer to your question, suppose I
have external device that is capable to time stamp PPS against known
time scale (such as GPS system time) with high precision. Now I can get
nice estimations of local time drifts and feed, say, "chrony", with the
data to adjust my local clock accordingly.

Thanks,
-- Sergey

[PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Initializing with 0 makes it much easier to identify time stamps from
otherwise uninitialized clock.

Initialization of PTP clock with current kernel time makes little sense as
PTP time scale differs from UTC time scale that kernel time represents.
It only leads to confusion when no actual PTP initialization happens, as
these time scales differ in a small integer number of seconds (37 at the
time of writing.)

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 4a12086..e455343 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -264,7 +264,7 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
 	fep->cc.mult = FEC_CC_MULT;
 
 	/* reset the ns time counter */
-	timecounter_init(&fep->tc, &fep->cc, ktime_to_ns(ktime_get_real()));
+	timecounter_init(&fep->tc, &fep->cc, 0);
 
 	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 }
-- 
2.10.0.1.g57b01a3

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Vladimir Oltean]

Hi Sergey,

On Mon, Jul 06, 2020 at 05:26:14PM +0300, Sergey Organov wrote:
> Initializing with 0 makes it much easier to identify time stamps from
> otherwise uninitialized clock.
> 
> Initialization of PTP clock with current kernel time makes little sense as
> PTP time scale differs from UTC time scale that kernel time represents.
> It only leads to confusion when no actual PTP initialization happens, as
> these time scales differ in a small integer number of seconds (37 at the
> time of writing.)
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---

Reading your patch, I got reminded of my own attempt of making an
identical change to the ptp_qoriq driver:

https://www.spinics.net/lists/netdev/msg601625.html

Could we have some sort of kernel-wide convention, I wonder (even though
it might be too late for that)? After your patch, I can see equal
amounts of confusion of users expecting some boot-time output of
$(phc_ctl /dev/ptp0 get) as it used to be, and now getting something
else.

There's no correct answer, I'm afraid. Whatever the default value of the
clock may be, it's bound to be confusing for some reason, _if_ the
reason why you're investigating it in the first place is a driver bug.
Also, I don't really see how your change to use Jan 1st 1970 makes it
any less confusing.

>  drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 4a12086..e455343 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -264,7 +264,7 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
>  	fep->cc.mult = FEC_CC_MULT;
>  
>  	/* reset the ns time counter */
> -	timecounter_init(&fep->tc, &fep->cc, ktime_to_ns(ktime_get_real()));
> +	timecounter_init(&fep->tc, &fep->cc, 0);
>  
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>  }
> -- 
> 2.10.0.1.g57b01a3
> 

Thanks,
-Vladimir

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> Hi Sergey,
>
> On Mon, Jul 06, 2020 at 05:26:14PM +0300, Sergey Organov wrote:
>> Initializing with 0 makes it much easier to identify time stamps from
>> otherwise uninitialized clock.
>> 
>> Initialization of PTP clock with current kernel time makes little sense as
>> PTP time scale differs from UTC time scale that kernel time represents.
>> It only leads to confusion when no actual PTP initialization happens, as
>> these time scales differ in a small integer number of seconds (37 at the
>> time of writing.)
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>
> Reading your patch, I got reminded of my own attempt of making an
> identical change to the ptp_qoriq driver:
>
> https://www.spinics.net/lists/netdev/msg601625.html
>
> Could we have some sort of kernel-wide convention, I wonder (even though
> it might be too late for that)? After your patch, I can see equal
> amounts of confusion of users expecting some boot-time output of
> $(phc_ctl /dev/ptp0 get) as it used to be, and now getting something
> else.
>
> There's no correct answer, I'm afraid.

IMHO, the correct answer would be keep non-initialized clock at 0. No
ticking.

> Whatever the default value of the clock may be, it's bound to be
> confusing for some reason, _if_ the reason why you're investigating it
> in the first place is a driver bug. Also, I don't really see how your
> change to use Jan 1st 1970 makes it any less confusing.

When I print the clocks in application, I see seconds and milliseconds
part since epoch. With this patch seconds count from 0, that simply
match uptime. Easy to tell from any other (malfunctioning) clock.

Here is the description of confusion and improvement. I spent half a day
not realizing that I sometimes get timestamps from the wrong PTP clock.
Part of the problem is that kernel time at startup, when it is used for
initialization of the PTP clock, is in fact somewhat random, and it
could be off by a few seconds. Now, when in application I get time stamp
that is almost right, and then another one that is, say, 9 seconds off,
what should I think? Right, that I drive PTP clock wrongly.

Now, when one of those timestamps is almost 0, I see immediately I got
time from wrong PTP clock, rather than wrong time from correct PTP
clock.

Thanks,
-- Sergey

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Vladimir Oltean]

On Mon, Jul 06, 2020 at 09:24:24PM +0300, Sergey Organov wrote:
> Vladimir Oltean <olteanv@gmail.com> writes:
> 
> > Hi Sergey,
> >
> > On Mon, Jul 06, 2020 at 05:26:14PM +0300, Sergey Organov wrote:
> >> Initializing with 0 makes it much easier to identify time stamps from
> >> otherwise uninitialized clock.
> >> 
> >> Initialization of PTP clock with current kernel time makes little sense as
> >> PTP time scale differs from UTC time scale that kernel time represents.
> >> It only leads to confusion when no actual PTP initialization happens, as
> >> these time scales differ in a small integer number of seconds (37 at the
> >> time of writing.)
> >> 
> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >> ---
> >
> > Reading your patch, I got reminded of my own attempt of making an
> > identical change to the ptp_qoriq driver:
> >
> > https://www.spinics.net/lists/netdev/msg601625.html
> >
> > Could we have some sort of kernel-wide convention, I wonder (even though
> > it might be too late for that)? After your patch, I can see equal
> > amounts of confusion of users expecting some boot-time output of
> > $(phc_ctl /dev/ptp0 get) as it used to be, and now getting something
> > else.
> >
> > There's no correct answer, I'm afraid.
> 
> IMHO, the correct answer would be keep non-initialized clock at 0. No
> ticking.
> 

What do you mean 'no ticking', and what do you mean by 'non-initialized
clock' exactly? I don't know if the fec driver is special in any way, do
you mean that multiple runs of $(phc_ctl /dev/ptp0 get) from user space
all return 0? That is not at all what is to be expected, I think. The
PHC is always ticking. Its time is increasing. What would be that
initialization procedure that makes it tick, and who is doing it (and
when)?

> > Whatever the default value of the clock may be, it's bound to be
> > confusing for some reason, _if_ the reason why you're investigating it
> > in the first place is a driver bug. Also, I don't really see how your
> > change to use Jan 1st 1970 makes it any less confusing.
> 
> When I print the clocks in application, I see seconds and milliseconds
> part since epoch. With this patch seconds count from 0, that simply
> match uptime. Easy to tell from any other (malfunctioning) clock.
> 

It doesn't really match uptime (CLOCK_MONOTONIC). Instead, it is just
initialized with zero. If you have fec built as module and you insmod it
after a few days of uptime, it will not track CLOCK_MONOTONIC at all.

Not to say that there's anything wrong with initializing it with 0. It's
just that I don't see why it would be objectively better.

> Here is the description of confusion and improvement. I spent half a day
> not realizing that I sometimes get timestamps from the wrong PTP clock.

There is a suite of tests in tools/testing/selftests/ptp/ which is
useful in debugging problems like this.

Alternatively, you can write to each individual clock using $(phc_ctl
/dev/ptpN set 0) and check your timestamps again. If the timestamps
don't nudge, it's clear that the timestamps you're getting are not from
the PHC you've written to. Much simpler.

> Part of the problem is that kernel time at startup, when it is used for
> initialization of the PTP clock, is in fact somewhat random, and it
> could be off by a few seconds.

Yes, the kernel time at startup is exactly random (not traceable to any
clock reference). And so is the PHC.

> Now, when in application I get time stamp
> that is almost right, and then another one that is, say, 9 seconds off,
> what should I think? Right, that I drive PTP clock wrongly.
> 
> Now, when one of those timestamps is almost 0, I see immediately I got
> time from wrong PTP clock, rather than wrong time from correct PTP
> clock.
> 

There are 2 points to be made here:

1. There are simpler ways to debug your issue than to leave a patch in
   the kernel, like the "phc_ctl set 0" I mentioned above. This can be
   considered a debugging patch which is also going to have consequences
   for the other users of the driver, if applied. We need to consider
   whether the change in behavior is useful in general.

2. There are boards out there which don't have any battery-backed RTC,
   so CLOCK_REALTIME could be ticking in Jan 1970 already, and therefore
   the PHC would be initialized with a time in 1970. Or your GM might be
   configured to be ticking in Jan 1970 (there are some applications
   that only require the network to be synchronized, but not for the
   time to be traceable to TAI). How does your change make a difference
   to eliminate confusion there, when all of your clocks are going to be
   in 1970? It doesn't make a net difference. Bottom line, a clock
   initialized with 0 doesn't mean it's special in any way. You _could_
   make that change in your debugging environment, and it _could_ be
   useful to your debugging, but if it's not universally useful, I
   wouldn't try to patch the kernel with this change.

Please note that, although my comments appear to be in disagreement with
your idea, they are in fact not at all. It's just that, if there's a a
particular answer to "what time to initialize a PHC with" that is more
favourable than the rest (even though the question itself is a bit
irrelevant overall), then that answer ought to be enforced kernel-wide,
I think.

> Thanks,
> -- Sergey

Cheers,
-Vladimir

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Mon, Jul 06, 2020 at 09:24:24PM +0300, Sergey Organov wrote:
>> Vladimir Oltean <olteanv@gmail.com> writes:
>> 
>> > Hi Sergey,
>> >
>> > On Mon, Jul 06, 2020 at 05:26:14PM +0300, Sergey Organov wrote:
>> >> Initializing with 0 makes it much easier to identify time stamps from
>> >> otherwise uninitialized clock.
>> >> 
>> >> Initialization of PTP clock with current kernel time makes little sense as
>> >> PTP time scale differs from UTC time scale that kernel time represents.
>> >> It only leads to confusion when no actual PTP initialization happens, as
>> >> these time scales differ in a small integer number of seconds (37 at the
>> >> time of writing.)
>> >> 
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> ---
>> >
>> > Reading your patch, I got reminded of my own attempt of making an
>> > identical change to the ptp_qoriq driver:
>> >
>> > https://www.spinics.net/lists/netdev/msg601625.html
>> >
>> > Could we have some sort of kernel-wide convention, I wonder (even though
>> > it might be too late for that)? After your patch, I can see equal
>> > amounts of confusion of users expecting some boot-time output of
>> > $(phc_ctl /dev/ptp0 get) as it used to be, and now getting something
>> > else.
>> >
>> > There's no correct answer, I'm afraid.
>> 
>> IMHO, the correct answer would be keep non-initialized clock at 0. No
>> ticking.
>> 
>
> What do you mean 'no ticking', and what do you mean by 'non-initialized
> clock' exactly? I don't know if the fec driver is special in any way, do
> you mean that multiple runs of $(phc_ctl /dev/ptp0 get) from user space
> all return 0? That is not at all what is to be expected, I think. The
> PHC is always ticking. Its time is increasing.

That's how it is right now. My point is that it likely shouldn't. Why is
it ticking when nobody needs it? Does it draw more power due to that?

> What would be that initialization procedure that makes it tick, and
> who is doing it (and when)?

The user space code that cares, obviously. Most probably some PTP stack
daemon. I'd say that any set clock time ioctl() should start the clock,
or yet another ioctl() that enables/disables the clock, whatever.

>
>> > Whatever the default value of the clock may be, it's bound to be
>> > confusing for some reason, _if_ the reason why you're investigating it
>> > in the first place is a driver bug. Also, I don't really see how your
>> > change to use Jan 1st 1970 makes it any less confusing.
>> 
>> When I print the clocks in application, I see seconds and milliseconds
>> part since epoch. With this patch seconds count from 0, that simply
>> match uptime. Easy to tell from any other (malfunctioning) clock.
>> 
>
> It doesn't really match uptime (CLOCK_MONOTONIC). Instead, it is just
> initialized with zero. If you have fec built as module and you insmod it
> after a few days of uptime, it will not track CLOCK_MONOTONIC at all.
>
> Not to say that there's anything wrong with initializing it with 0. It's
> just that I don't see why it would be objectively better.

Well, it would have been better for me in my particular quest to find
the problem, so it rather needs to be shown where initializing with
kernel time is objectively better.

Moreover, everything else being equal, 0 is always better, just because
of simplicity.

>
>> Here is the description of confusion and improvement. I spent half a day
>> not realizing that I sometimes get timestamps from the wrong PTP clock.
>
> There is a suite of tests in tools/testing/selftests/ptp/ which is
> useful in debugging problems like this.
>
> Alternatively, you can write to each individual clock using $(phc_ctl
> /dev/ptpN set 0) and check your timestamps again. If the timestamps
> don't nudge, it's clear that the timestamps you're getting are not from
> the PHC you've written to. Much simpler.

Maybe. Once you do figure there is another clock in the system and/or
that that clock is offending. In my case /that/ was the hard part, not
changing that offending clock, once found, to whatever.

>
>> Part of the problem is that kernel time at startup, when it is used for
>> initialization of the PTP clock, is in fact somewhat random, and it
>> could be off by a few seconds.
>
> Yes, the kernel time at startup is exactly random (not traceable to any
> clock reference). And so is the PHC.
>
>> Now, when in application I get time stamp
>> that is almost right, and then another one that is, say, 9 seconds off,
>> what should I think? Right, that I drive PTP clock wrongly.
>> 
>> Now, when one of those timestamps is almost 0, I see immediately I got
>> time from wrong PTP clock, rather than wrong time from correct PTP
>> clock.
>> 
>
> There are 2 points to be made here:
>
> 1. There are simpler ways to debug your issue than to leave a patch in
>    the kernel, like the "phc_ctl set 0" I mentioned above. This can be
>    considered a debugging patch which is also going to have consequences
>    for the other users of the driver, if applied. We need to consider
>    whether the change in behavior is useful in general.

This does not apply to my particular case as I explained above, and then
ease with debug is just a nice side-effect of code simplification.

>
> 2. There are boards out there which don't have any battery-backed RTC,
>    so CLOCK_REALTIME could be ticking in Jan 1970 already, and therefore
>    the PHC would be initialized with a time in 1970. Or your GM might be
>    configured to be ticking in Jan 1970 (there are some applications
>    that only require the network to be synchronized, but not for the
>    time to be traceable to TAI). How does your change make a difference
>    to eliminate confusion there, when all of your clocks are going to be
>    in 1970? It doesn't make a net difference. Bottom line, a clock
>    initialized with 0 doesn't mean it's special in any way. You _could_
>    make that change in your debugging environment, and it _could_ be
>    useful to your debugging, but if it's not universally useful, I
>    wouldn't try to patch the kernel with this change.

If there is nothing special about any value, 0 is the value to choose,
because of simplicity. Once again, I only explained debugging advantages
because you've asked about it. It's just a nice side-effect, as it
often happens to be when one keeps things as simple as possible.

> Please note that, although my comments appear to be in disagreement with
> your idea, they are in fact not at all. It's just that, if there's a a
> particular answer to "what time to initialize a PHC with" that is more
> favourable than the rest (even though the question itself is a bit
> irrelevant overall), then that answer ought to be enforced kernel-wide,
> I think.

As everybody, I believe in a set of generic programming principles that
are not to be violated lightly. KISS is one of the principles I believe,
and trying to be clever with no apparent reason is one way of violating
it.

Overall, here is my argument: 0 is simpler than kernel time, so how is
it useful to initialize PTP with kernel time that is as wrong as a value
for PTP time as 0?

Thanks,
-- Sergey.

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Vladimir Oltean]

On Tue, Jul 07, 2020 at 07:07:08PM +0300, Sergey Organov wrote:
> Vladimir Oltean <olteanv@gmail.com> writes:
> >
> > What do you mean 'no ticking', and what do you mean by 'non-initialized
> > clock' exactly? I don't know if the fec driver is special in any way, do
> > you mean that multiple runs of $(phc_ctl /dev/ptp0 get) from user space
> > all return 0? That is not at all what is to be expected, I think. The
> > PHC is always ticking. Its time is increasing.
> 
> That's how it is right now. My point is that it likely shouldn't. Why is
> it ticking when nobody needs it? Does it draw more power due to that?
> 
> > What would be that initialization procedure that makes it tick, and
> > who is doing it (and when)?
> 
> The user space code that cares, obviously. Most probably some PTP stack
> daemon. I'd say that any set clock time ioctl() should start the clock,
> or yet another ioctl() that enables/disables the clock, whatever.
> 

That ioctl doesn't exist, at least not in PTP land. This also addresses
your previous point.

> >
> >> > Whatever the default value of the clock may be, it's bound to be
> >> > confusing for some reason, _if_ the reason why you're investigating it
> >> > in the first place is a driver bug. Also, I don't really see how your
> >> > change to use Jan 1st 1970 makes it any less confusing.
> >> 
> >> When I print the clocks in application, I see seconds and milliseconds
> >> part since epoch. With this patch seconds count from 0, that simply
> >> match uptime. Easy to tell from any other (malfunctioning) clock.
> >> 
> >
> > It doesn't really match uptime (CLOCK_MONOTONIC). Instead, it is just
> > initialized with zero. If you have fec built as module and you insmod it
> > after a few days of uptime, it will not track CLOCK_MONOTONIC at all.
> >
> > Not to say that there's anything wrong with initializing it with 0. It's
> > just that I don't see why it would be objectively better.
> 
> Well, it would have been better for me in my particular quest to find
> the problem, so it rather needs to be shown where initializing with
> kernel time is objectively better.
> 
> Moreover, everything else being equal, 0 is always better, just because
> of simplicity.
> 
> >
> >> Here is the description of confusion and improvement. I spent half a day
> >> not realizing that I sometimes get timestamps from the wrong PTP clock.
> >
> > There is a suite of tests in tools/testing/selftests/ptp/ which is
> > useful in debugging problems like this.
> >
> > Alternatively, you can write to each individual clock using $(phc_ctl
> > /dev/ptpN set 0) and check your timestamps again. If the timestamps
> > don't nudge, it's clear that the timestamps you're getting are not from
> > the PHC you've written to. Much simpler.
> 
> Maybe. Once you do figure there is another clock in the system and/or
> that that clock is offending. In my case /that/ was the hard part, not
> changing that offending clock, once found, to whatever.
> 

And my point was that you could have been in a different situation, when
all of your clocks could have been ticking in 1970, so this wouldn't
have been a distiguishing point. So this argument is poor. Using
phc_ctl, or scripts around that, is much more dynamic.

> >
> >> Part of the problem is that kernel time at startup, when it is used for
> >> initialization of the PTP clock, is in fact somewhat random, and it
> >> could be off by a few seconds.
> >
> > Yes, the kernel time at startup is exactly random (not traceable to any
> > clock reference). And so is the PHC.
> >
> >> Now, when in application I get time stamp
> >> that is almost right, and then another one that is, say, 9 seconds off,
> >> what should I think? Right, that I drive PTP clock wrongly.
> >> 
> >> Now, when one of those timestamps is almost 0, I see immediately I got
> >> time from wrong PTP clock, rather than wrong time from correct PTP
> >> clock.
> >> 
> >
> > There are 2 points to be made here:
> >
> > 1. There are simpler ways to debug your issue than to leave a patch in
> >    the kernel, like the "phc_ctl set 0" I mentioned above. This can be
> >    considered a debugging patch which is also going to have consequences
> >    for the other users of the driver, if applied. We need to consider
> >    whether the change in behavior is useful in general.
> 
> This does not apply to my particular case as I explained above, and then
> ease with debug is just a nice side-effect of code simplification.
> 
> >
> > 2. There are boards out there which don't have any battery-backed RTC,
> >    so CLOCK_REALTIME could be ticking in Jan 1970 already, and therefore
> >    the PHC would be initialized with a time in 1970. Or your GM might be
> >    configured to be ticking in Jan 1970 (there are some applications
> >    that only require the network to be synchronized, but not for the
> >    time to be traceable to TAI). How does your change make a difference
> >    to eliminate confusion there, when all of your clocks are going to be
> >    in 1970? It doesn't make a net difference. Bottom line, a clock
> >    initialized with 0 doesn't mean it's special in any way. You _could_
> >    make that change in your debugging environment, and it _could_ be
> >    useful to your debugging, but if it's not universally useful, I
> >    wouldn't try to patch the kernel with this change.
> 
> If there is nothing special about any value, 0 is the value to choose,
> because of simplicity. Once again, I only explained debugging advantages
> because you've asked about it. It's just a nice side-effect, as it
> often happens to be when one keeps things as simple as possible.
> 
> > Please note that, although my comments appear to be in disagreement with
> > your idea, they are in fact not at all. It's just that, if there's a a
> > particular answer to "what time to initialize a PHC with" that is more
> > favourable than the rest (even though the question itself is a bit
> > irrelevant overall), then that answer ought to be enforced kernel-wide,
> > I think.
> 
> As everybody, I believe in a set of generic programming principles that
> are not to be violated lightly. KISS is one of the principles I believe,
> and trying to be clever with no apparent reason is one way of violating
> it.
> 
> Overall, here is my argument: 0 is simpler than kernel time, so how is
> it useful to initialize PTP with kernel time that is as wrong as a value
> for PTP time as 0?
> 

And overall, my argument is: you are making a user-visible change, for
basically no strong reason, other than the fact that you like zero
better. You're trying to reduce confusion, not increase it, right?

I agree with the basic fact that zero is a simpler and more consistent
value to initialize a PHC with, than the system time. As I've already
shown to you, I even attempted to make a similar change to the ptp_qoriq
driver which was rejected. So I hoped that you could bring some better
arguments than "I believe 0 is simpler". Since no value is right, no
value is wrong either, so why make a change in the first place? The only
value in _changing_ to zero would be if all drivers were changed to use
it consistently, IMO.

But I will stop here and let the PTP maintainer make a choice. I only
intervened because I knew what the default answer was going to be.

> Thanks,
> -- Sergey.

Thanks,
-Vladimir

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Tue, Jul 07, 2020 at 07:07:08PM +0300, Sergey Organov wrote:
>> Vladimir Oltean <olteanv@gmail.com> writes:
>> >
>> > What do you mean 'no ticking', and what do you mean by 'non-initialized
>> > clock' exactly? I don't know if the fec driver is special in any way, do
>> > you mean that multiple runs of $(phc_ctl /dev/ptp0 get) from user space
>> > all return 0? That is not at all what is to be expected, I think. The
>> > PHC is always ticking. Its time is increasing.
>> 
>> That's how it is right now. My point is that it likely shouldn't. Why is
>> it ticking when nobody needs it? Does it draw more power due to that?
>> 
>> > What would be that initialization procedure that makes it tick, and
>> > who is doing it (and when)?
>> 
>> The user space code that cares, obviously. Most probably some PTP stack
>> daemon. I'd say that any set clock time ioctl() should start the clock,
>> or yet another ioctl() that enables/disables the clock, whatever.
>> 
>
> That ioctl doesn't exist, at least not in PTP land. This also addresses
> your previous point.

struct timespec ts;
...
clock_settime(clkid, &ts)

That's the starting point of my own code, and I bet it's there in PTP
for Linux, as well as in PTPD, as I fail to see how it could possibly
work without it.

>
>> >
>> >> > Whatever the default value of the clock may be, it's bound to be
>> >> > confusing for some reason, _if_ the reason why you're investigating it
>> >> > in the first place is a driver bug. Also, I don't really see how your
>> >> > change to use Jan 1st 1970 makes it any less confusing.
>> >> 
>> >> When I print the clocks in application, I see seconds and milliseconds
>> >> part since epoch. With this patch seconds count from 0, that simply
>> >> match uptime. Easy to tell from any other (malfunctioning) clock.
>> >> 
>> >
>> > It doesn't really match uptime (CLOCK_MONOTONIC). Instead, it is just
>> > initialized with zero. If you have fec built as module and you insmod it
>> > after a few days of uptime, it will not track CLOCK_MONOTONIC at all.
>> >
>> > Not to say that there's anything wrong with initializing it with 0. It's
>> > just that I don't see why it would be objectively better.
>> 
>> Well, it would have been better for me in my particular quest to find
>> the problem, so it rather needs to be shown where initializing with
>> kernel time is objectively better.
>> 
>> Moreover, everything else being equal, 0 is always better, just because
>> of simplicity.
>> 
>> >
>> >> Here is the description of confusion and improvement. I spent half a day
>> >> not realizing that I sometimes get timestamps from the wrong PTP clock.
>> >
>> > There is a suite of tests in tools/testing/selftests/ptp/ which is
>> > useful in debugging problems like this.
>> >
>> > Alternatively, you can write to each individual clock using $(phc_ctl
>> > /dev/ptpN set 0) and check your timestamps again. If the timestamps
>> > don't nudge, it's clear that the timestamps you're getting are not from
>> > the PHC you've written to. Much simpler.
>> 
>> Maybe. Once you do figure there is another clock in the system and/or
>> that that clock is offending. In my case /that/ was the hard part, not
>> changing that offending clock, once found, to whatever.
>> 
>
> And my point was that you could have been in a different situation, when
> all of your clocks could have been ticking in 1970, so this wouldn't
> have been a distiguishing point. So this argument is poor. Using
> phc_ctl, or scripts around that, is much more dynamic.
>
>> >
>> >> Part of the problem is that kernel time at startup, when it is used for
>> >> initialization of the PTP clock, is in fact somewhat random, and it
>> >> could be off by a few seconds.
>> >
>> > Yes, the kernel time at startup is exactly random (not traceable to any
>> > clock reference). And so is the PHC.
>> >
>> >> Now, when in application I get time stamp
>> >> that is almost right, and then another one that is, say, 9 seconds off,
>> >> what should I think? Right, that I drive PTP clock wrongly.
>> >> 
>> >> Now, when one of those timestamps is almost 0, I see immediately I got
>> >> time from wrong PTP clock, rather than wrong time from correct PTP
>> >> clock.
>> >> 
>> >
>> > There are 2 points to be made here:
>> >
>> > 1. There are simpler ways to debug your issue than to leave a patch in
>> >    the kernel, like the "phc_ctl set 0" I mentioned above. This can be
>> >    considered a debugging patch which is also going to have consequences
>> >    for the other users of the driver, if applied. We need to consider
>> >    whether the change in behavior is useful in general.
>> 
>> This does not apply to my particular case as I explained above, and then
>> ease with debug is just a nice side-effect of code simplification.
>> 
>> >
>> > 2. There are boards out there which don't have any battery-backed RTC,
>> >    so CLOCK_REALTIME could be ticking in Jan 1970 already, and therefore
>> >    the PHC would be initialized with a time in 1970. Or your GM might be
>> >    configured to be ticking in Jan 1970 (there are some applications
>> >    that only require the network to be synchronized, but not for the
>> >    time to be traceable to TAI). How does your change make a difference
>> >    to eliminate confusion there, when all of your clocks are going to be
>> >    in 1970? It doesn't make a net difference. Bottom line, a clock
>> >    initialized with 0 doesn't mean it's special in any way. You _could_
>> >    make that change in your debugging environment, and it _could_ be
>> >    useful to your debugging, but if it's not universally useful, I
>> >    wouldn't try to patch the kernel with this change.
>> 
>> If there is nothing special about any value, 0 is the value to choose,
>> because of simplicity. Once again, I only explained debugging advantages
>> because you've asked about it. It's just a nice side-effect, as it
>> often happens to be when one keeps things as simple as possible.
>> 
>> > Please note that, although my comments appear to be in disagreement with
>> > your idea, they are in fact not at all. It's just that, if there's a a
>> > particular answer to "what time to initialize a PHC with" that is more
>> > favourable than the rest (even though the question itself is a bit
>> > irrelevant overall), then that answer ought to be enforced kernel-wide,
>> > I think.
>> 
>> As everybody, I believe in a set of generic programming principles that
>> are not to be violated lightly. KISS is one of the principles I believe,
>> and trying to be clever with no apparent reason is one way of violating
>> it.
>> 
>> Overall, here is my argument: 0 is simpler than kernel time, so how is
>> it useful to initialize PTP with kernel time that is as wrong as a value
>> for PTP time as 0?
>> 
>
> And overall, my argument is: you are making a user-visible change, for
> basically no strong reason, other than the fact that you like zero
> better. You're trying to reduce confusion, not increase it, right?

No, not to increase, and I believe there is no way to increase it by
using the value that at least some of the drivers already use.

> I agree with the basic fact that zero is a simpler and more consistent
> value to initialize a PHC with, than the system time. As I've already
> shown to you, I even attempted to make a similar change to the ptp_qoriq
> driver which was rejected. So I hoped that you could bring some better
> arguments than "I believe 0 is simpler". Since no value is right, no
> value is wrong either, so why make a change in the first place? The only
> value in _changing_ to zero would be if all drivers were changed to use
> it consistently, IMO.
>
> But I will stop here and let the PTP maintainer make a choice. I only
> intervened because I knew what the default answer was going to be.

Thanks, so will I, as I won't care much either way, just wanted to make
life of somebody who might travel my path less painful. That person is
not who is already fluent in all the different opportunities to debug
PTP clocks, for sure.

Thanks,
-- Sergey

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Vladimir Oltean]

On Tue, Jul 07, 2020 at 08:09:07PM +0300, Sergey Organov wrote:
> Vladimir Oltean <olteanv@gmail.com> writes:
> 
> > On Tue, Jul 07, 2020 at 07:07:08PM +0300, Sergey Organov wrote:
> >> Vladimir Oltean <olteanv@gmail.com> writes:
> >> >
> >> > What do you mean 'no ticking', and what do you mean by 'non-initialized
> >> > clock' exactly? I don't know if the fec driver is special in any way, do
> >> > you mean that multiple runs of $(phc_ctl /dev/ptp0 get) from user space
> >> > all return 0? That is not at all what is to be expected, I think. The
> >> > PHC is always ticking. Its time is increasing.
> >> 
> >> That's how it is right now. My point is that it likely shouldn't. Why is
> >> it ticking when nobody needs it? Does it draw more power due to that?
> >> 
> >> > What would be that initialization procedure that makes it tick, and
> >> > who is doing it (and when)?
> >> 
> >> The user space code that cares, obviously. Most probably some PTP stack
> >> daemon. I'd say that any set clock time ioctl() should start the clock,
> >> or yet another ioctl() that enables/disables the clock, whatever.
> >> 
> >
> > That ioctl doesn't exist, at least not in PTP land. This also addresses
> > your previous point.
> 
> struct timespec ts;
> ...
> clock_settime(clkid, &ts)
> 
> That's the starting point of my own code, and I bet it's there in PTP
> for Linux, as well as in PTPD, as I fail to see how it could possibly
> work without it.
> 

This won't stop it from ticking, which is what we were talking about,
will it?

Thanks,
-Vladimir

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Tue, Jul 07, 2020 at 08:09:07PM +0300, Sergey Organov wrote:
>> Vladimir Oltean <olteanv@gmail.com> writes:
>> 
>> > On Tue, Jul 07, 2020 at 07:07:08PM +0300, Sergey Organov wrote:
>> >> Vladimir Oltean <olteanv@gmail.com> writes:
>> >> >
>> >> > What do you mean 'no ticking', and what do you mean by 'non-initialized
>> >> > clock' exactly? I don't know if the fec driver is special in any way, do
>> >> > you mean that multiple runs of $(phc_ctl /dev/ptp0 get) from user space
>> >> > all return 0? That is not at all what is to be expected, I think. The
>> >> > PHC is always ticking. Its time is increasing.
>> >> 
>> >> That's how it is right now. My point is that it likely shouldn't. Why is
>> >> it ticking when nobody needs it? Does it draw more power due to that?
>> >> 
>> >> > What would be that initialization procedure that makes it tick, and
>> >> > who is doing it (and when)?
>> >> 
>> >> The user space code that cares, obviously. Most probably some PTP stack
>> >> daemon. I'd say that any set clock time ioctl() should start the clock,
>> >> or yet another ioctl() that enables/disables the clock, whatever.
>> >> 
>> >
>> > That ioctl doesn't exist, at least not in PTP land. This also addresses
>> > your previous point.
>> 
>> struct timespec ts;
>> ...
>> clock_settime(clkid, &ts)
>> 
>> That's the starting point of my own code, and I bet it's there in PTP
>> for Linux, as well as in PTPD, as I fail to see how it could possibly
>> work without it.
>> 
>
> This won't stop it from ticking, which is what we were talking about,
> will it?

It won't. Supposedly it'd force clock (that doesn't tick by default and
stays at 0) to start ticking.

If the ability to stop the clock is in fact useful (I don't immediately
see how it could), and how to do it if it is, is a separate issue. I'd
then expect clock_stop(clkid), or clock_settime(clkid, NULL) to do the
job.

Thanks,
-- Sergey

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Richard Cochran]

On Tue, Jul 07, 2020 at 08:56:41PM +0300, Sergey Organov wrote:
> It won't. Supposedly it'd force clock (that doesn't tick by default and
> stays at 0) to start ticking.

No existing clockid_t has this behavior.  Consider CLOCK_REALTIME or
CLOCK_MONOTONIC.
 
The PHC must act the same as the other POSIX clocks.

Thanks,
Richard

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Richard Cochran <richardcochran@gmail.com> writes:

> On Tue, Jul 07, 2020 at 08:56:41PM +0300, Sergey Organov wrote:
>> It won't. Supposedly it'd force clock (that doesn't tick by default and
>> stays at 0) to start ticking.
>
> No existing clockid_t has this behavior.  Consider CLOCK_REALTIME or
> CLOCK_MONOTONIC.
>  
> The PHC must act the same as the other POSIX clocks.

Yeah, that's a good argument!

Thanks,
-- Sergey

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Richard Cochran]

On Tue, Jul 07, 2020 at 07:43:29PM +0300, Vladimir Oltean wrote:
> And overall, my argument is: you are making a user-visible change, for
> basically no strong reason, other than the fact that you like zero
> better. You're trying to reduce confusion, not increase it, right?

;^)
 
> I agree with the basic fact that zero is a simpler and more consistent
> value to initialize a PHC with, than the system time. As I've already
> shown to you, I even attempted to make a similar change to the ptp_qoriq
> driver which was rejected. So I hoped that you could bring some better
> arguments than "I believe 0 is simpler". Since no value is right, no
> value is wrong either, so why make a change in the first place? The only
> value in _changing_ to zero would be if all drivers were changed to use
> it consistently, IMO.

Right.

I would not appose making all PHCs start with zero.  If you feel
strongly about starting all PHCs at zero, please prepare a patch set
and get the ACKs of the appropriate driver maintainers.

(The effort seems pointless to me because the user needs to consult
the synchronization flags in any case.)

Thanks,
Richard

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Richard Cochran]

On Mon, Jul 06, 2020 at 06:27:21PM +0300, Vladimir Oltean wrote:
> There's no correct answer, I'm afraid. Whatever the default value of the
> clock may be, it's bound to be confusing for some reason, _if_ the
> reason why you're investigating it in the first place is a driver bug.
> Also, I don't really see how your change to use Jan 1st 1970 makes it
> any less confusing.

+1

For a PHC, the user of the clock must check the PTP stack's
synchronization flags via the management interface to know the status
of the time signal.

Thanks,
Richard

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Richard Cochran <richardcochran@gmail.com> writes:

> On Mon, Jul 06, 2020 at 06:27:21PM +0300, Vladimir Oltean wrote:
>> There's no correct answer, I'm afraid. Whatever the default value of the
>> clock may be, it's bound to be confusing for some reason, _if_ the
>> reason why you're investigating it in the first place is a driver bug.
>> Also, I don't really see how your change to use Jan 1st 1970 makes it
>> any less confusing.
>
> +1
>
> For a PHC, the user of the clock must check the PTP stack's
> synchronization flags via the management interface to know the status
> of the time signal.

Sorry, but I'm looking at it from the POV of PTP stack, so I don't have
another one to check.

If PTP clock itself had some means of checking for its quality (like
last time is was synchronized, skew and offset estimations at that time,
etc.), then it'd render its initial value mostly unimportant indeed.

But even then, the original problem was that Ethernet packets were time
stamped with the wrong (different) PTP clock, and there is no any flags
or clock ID in the time stamp of Ethernet packet, so any flags attached
to PTP clock are still useless.

Thanks,
-- Sergey

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Richard Cochran <richardcochran@gmail.com> writes:

> On Mon, Jul 06, 2020 at 06:27:21PM +0300, Vladimir Oltean wrote:
>> There's no correct answer, I'm afraid. Whatever the default value of the
>> clock may be, it's bound to be confusing for some reason, _if_ the
>> reason why you're investigating it in the first place is a driver bug.
>> Also, I don't really see how your change to use Jan 1st 1970 makes it
>> any less confusing.
>
> +1
>
> For a PHC, the user of the clock must check the PTP stack's
> synchronization flags via the management interface to know the status
> of the time signal.

Actually, as I just realized, the right solution for my original problem
would rather be adding PTP clock ID that time stamped Ethernet packet to
the Ethernet hardware time stamp (see my previous reply as well).

Thanks,
-- Sergey

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Richard Cochran]

On Wed, Jul 08, 2020 at 03:37:29PM +0300, Sergey Organov wrote:
> Richard Cochran <richardcochran@gmail.com> writes:
> 
> > On Mon, Jul 06, 2020 at 06:27:21PM +0300, Vladimir Oltean wrote:
> >> There's no correct answer, I'm afraid. Whatever the default value of the
> >> clock may be, it's bound to be confusing for some reason, _if_ the
> >> reason why you're investigating it in the first place is a driver bug.
> >> Also, I don't really see how your change to use Jan 1st 1970 makes it
> >> any less confusing.
> >
> > +1
> >
> > For a PHC, the user of the clock must check the PTP stack's
> > synchronization flags via the management interface to know the status
> > of the time signal.
> 
> Actually, as I just realized, the right solution for my original problem
> would rather be adding PTP clock ID that time stamped Ethernet packet to
> the Ethernet hardware time stamp (see my previous reply as well).

I think you misunderstood my point.  I wasn't commenting on the
"stacked" MAC/PHY/DSA time stamp issue in the kernel.

I am talking about the question of whether to initialize the PHC time
to zero (decades off from TAI) or ktime (likely about 37 seconds off
from TAI).  It does not really matter, because the user must not guess
whether the time is valid based on the value.  Instead, the user
should query the relevant PTP data sets in a "live" online manner.

For example, to tell whether a PHC is synchronized to anything at all,
you need to check PORT_DATA_SET.portState and probably also
CURRENT_DATA_SET.offsetFromMaster depending on your needs.

In addition, if you care about global time, you need to check:

TIME_PROPERTIES_DATA_SET
  currentUtcOffset
  currentUtcOffsetValid
  ptpTimescale
  timeTraceable
  frequencyTraceable

Thanks,
Richard

Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Richard Cochran <richardcochran@gmail.com> writes:

> On Wed, Jul 08, 2020 at 03:37:29PM +0300, Sergey Organov wrote:
>> Richard Cochran <richardcochran@gmail.com> writes:
>> 
>> > On Mon, Jul 06, 2020 at 06:27:21PM +0300, Vladimir Oltean wrote:
>> >> There's no correct answer, I'm afraid. Whatever the default value of the
>> >> clock may be, it's bound to be confusing for some reason, _if_ the
>> >> reason why you're investigating it in the first place is a driver bug.
>> >> Also, I don't really see how your change to use Jan 1st 1970 makes it
>> >> any less confusing.
>> >
>> > +1
>> >
>> > For a PHC, the user of the clock must check the PTP stack's
>> > synchronization flags via the management interface to know the status
>> > of the time signal.
>> 
>> Actually, as I just realized, the right solution for my original problem
>> would rather be adding PTP clock ID that time stamped Ethernet packet to
>> the Ethernet hardware time stamp (see my previous reply as well).
>
> I think you misunderstood my point.  I wasn't commenting on the
> "stacked" MAC/PHY/DSA time stamp issue in the kernel.

I think I did. It's rather that initialization value of PHP clock has
consequences in MAC/PHY/DSA, and there is no way to check any flags
/there/. And it's exactly the place where I needed the info, see
background explanation below.

I understand what your point is, but I try to explain that it's
irrelevant for my particular use-case.

>
> I am talking about the question of whether to initialize the PHC time
> to zero (decades off from TAI) or ktime (likely about 37 seconds off
> from TAI).  It does not really matter, because the user must not guess
> whether the time is valid based on the value.  Instead, the user
> should query the relevant PTP data sets in a "live" online manner.

I'm implementing PTP master clock, and there are no PTP data sets (yet)
in my case, as it's me who will eventually create them.

>
> For example, to tell whether a PHC is synchronized to anything at all,
> you need to check PORT_DATA_SET.portState and probably also
> CURRENT_DATA_SET.offsetFromMaster depending on your needs.

These are fields of PTP stack software that is not running in my case.

>
> In addition, if you care about global time, you need to check:
>
> TIME_PROPERTIES_DATA_SET
>   currentUtcOffset
>   currentUtcOffsetValid
>   ptpTimescale
>   timeTraceable
>   frequencyTraceable

I'm going to become PTP master clock, I already have very precise
estimations of PTP time, I know UTC offset, all this independently from
any PTP stack. Moreover, I've already synchronized PHY clock to this
time scale with +-5ns accuracy, and then I suddenly get somewhat wrong
hardware time stamps for Ethernet packets that I send/receive. You see?

Setting initial value of PTP clock to 0 would allow me to figure what
happens (time stamp coming from /different/ PTP clock) half a day
earlier. Not a big deal, I agree, yet I wanted to help a little bit
somebody who might happen to run into a similar trouble in the future.

Thanks,
-- Sergey

[PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()

[Sergey Organov]

Code of the form "if(x) x = 0" replaced with "x = 0".

Code of the form "if(x == a) x = a" removed.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index e455343..4152cae 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
 
 	switch (config.rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
-		if (fep->hwts_rx_en)
-			fep->hwts_rx_en = 0;
-		config.rx_filter = HWTSTAMP_FILTER_NONE;
+		fep->hwts_rx_en = 0;
 		break;
 
 	default:
-- 
2.10.0.1.g57b01a3

RE: [EXT] [PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()

[Andy Duan]

From: Sergey Organov <sorganov@gmail.com> Sent: Monday, July 6, 2020 10:26 PM
> Code of the form "if(x) x = 0" replaced with "x = 0".
> 
> Code of the form "if(x == a) x = a" removed.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index e455343..4152cae 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq
> *ifr)
> 
>         switch (config.rx_filter) {
>         case HWTSTAMP_FILTER_NONE:
> -               if (fep->hwts_rx_en)
> -                       fep->hwts_rx_en = 0;
> -               config.rx_filter = HWTSTAMP_FILTER_NONE;
The line should keep according your commit log.

> +               fep->hwts_rx_en = 0;
>                 break;
> 
>         default:
> --
> 2.10.0.1.g57b01a3

Re: [EXT] [PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()

[Sergey Organov]

Andy Duan <fugang.duan@nxp.com> writes:

> From: Sergey Organov <sorganov@gmail.com> Sent: Monday, July 6, 2020 10:26 PM
>> Code of the form "if(x) x = 0" replaced with "x = 0".
>> 
>> Code of the form "if(x == a) x = a" removed.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
>> b/drivers/net/ethernet/freescale/fec_ptp.c
>> index e455343..4152cae 100644
>> --- a/drivers/net/ethernet/freescale/fec_ptp.c
>> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
>> @@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq
>> *ifr)
>> 
>>         switch (config.rx_filter) {
>>         case HWTSTAMP_FILTER_NONE:
>> -               if (fep->hwts_rx_en)
>> -                       fep->hwts_rx_en = 0;
>> -               config.rx_filter = HWTSTAMP_FILTER_NONE;
> The line should keep according your commit log.

You mean I should fix commit log like this:

Code of the form "switch(x) case a: x = a; break" removed.

?

I'll do if it's cleaner that way.

Thanks,
-- Sergey


>
>> +               fep->hwts_rx_en = 0;
>>                 break;
>> 
>>         default:
>> --
>> 2.10.0.1.g57b01a3

RE: [EXT] [PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()

[Andy Duan]

From: Sergey Organov <sorganov@gmail.com> Sent: Tuesday, July 7, 2020 10:43 PM
> Andy Duan <fugang.duan@nxp.com> writes:
> 
> > From: Sergey Organov <sorganov@gmail.com> Sent: Monday, July 6, 2020
> 10:26 PM
> >> Code of the form "if(x) x = 0" replaced with "x = 0".
> >>
> >> Code of the form "if(x == a) x = a" removed.
> >>
> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >> ---
> >>  drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> >> b/drivers/net/ethernet/freescale/fec_ptp.c
> >> index e455343..4152cae 100644
> >> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> >> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> >> @@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev, struct
> ifreq
> >> *ifr)
> >>
> >>         switch (config.rx_filter) {
> >>         case HWTSTAMP_FILTER_NONE:
> >> -               if (fep->hwts_rx_en)
> >> -                       fep->hwts_rx_en = 0;
> >> -               config.rx_filter = HWTSTAMP_FILTER_NONE;
> > The line should keep according your commit log.
> 
> You mean I should fix commit log like this:
> 
> Code of the form "switch(x) case a: x = a; break" removed.
> 
> ?
Like this:

case HWTSTAMP_FILTER_NONE:
	fep->hwts_rx_en = 0;
	config.rx_filter = HWTSTAMP_FILTER_NONE;
	break;
> 
> I'll do if it's cleaner that way.
> 
> Thanks,
> -- Sergey
> 
> 
> >
> >> +               fep->hwts_rx_en = 0;
> >>                 break;
> >>
> >>         default:
> >> --
> >> 2.10.0.1.g57b01a3

Re: [EXT] [PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()

[Sergey Organov]

Andy Duan <fugang.duan@nxp.com> writes:

> From: Sergey Organov <sorganov@gmail.com> Sent: Tuesday, July 7, 2020 10:43 PM
>> Andy Duan <fugang.duan@nxp.com> writes:
>> 
>> > From: Sergey Organov <sorganov@gmail.com> Sent: Monday, July 6, 2020
>> 10:26 PM
>> >> Code of the form "if(x) x = 0" replaced with "x = 0".
>> >>
>> >> Code of the form "if(x == a) x = a" removed.
>> >>
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> ---
>> >>  drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
>> >>  1 file changed, 1 insertion(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
>> >> b/drivers/net/ethernet/freescale/fec_ptp.c
>> >> index e455343..4152cae 100644
>> >> --- a/drivers/net/ethernet/freescale/fec_ptp.c
>> >> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
>> >> @@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev, struct
>> ifreq
>> >> *ifr)
>> >>
>> >>         switch (config.rx_filter) {
>> >>         case HWTSTAMP_FILTER_NONE:
>> >> -               if (fep->hwts_rx_en)
>> >> -                       fep->hwts_rx_en = 0;
>> >> -               config.rx_filter = HWTSTAMP_FILTER_NONE;
>> > The line should keep according your commit log.
>> 
>> You mean I should fix commit log like this:
>> 
>> Code of the form "switch(x) case a: x = a; break" removed.
>> 
>> ?
> Like this:
>
> case HWTSTAMP_FILTER_NONE:
> 	fep->hwts_rx_en = 0;
> 	config.rx_filter = HWTSTAMP_FILTER_NONE;

This last line is redundant, as it's part of the switch that reads:

switch (config.rx_filter) {
 case HWTSTAMP_FILTER_NONE: config.rx_filter = HWTSTAMP_FILTER_NONE;

that effectively reduces to:

if(x == a) x = a;

that is a no-op (provided 'x' is a usual memory reference),
and that is exactly what I've described in the commit log.

What do I miss?

Thanks,
-- Sergey

RE: [EXT] [PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()

[Andy Duan]

From: Sergey Organov <sorganov@gmail.com> Sent: Wednesday, July 8, 2020 4:49 PM
> Andy Duan <fugang.duan@nxp.com> writes:
> 
> > From: Sergey Organov <sorganov@gmail.com> Sent: Tuesday, July 7, 2020
> > 10:43 PM
> >> Andy Duan <fugang.duan@nxp.com> writes:
> >>
> >> > From: Sergey Organov <sorganov@gmail.com> Sent: Monday, July 6,
> >> > 2020
> >> 10:26 PM
> >> >> Code of the form "if(x) x = 0" replaced with "x = 0".
> >> >>
> >> >> Code of the form "if(x == a) x = a" removed.
> >> >>
> >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >> >> ---
> >> >>  drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
> >> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> >> >> b/drivers/net/ethernet/freescale/fec_ptp.c
> >> >> index e455343..4152cae 100644
> >> >> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> >> >> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> >> >> @@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev,
> >> >> struct
> >> ifreq
> >> >> *ifr)
> >> >>
> >> >>         switch (config.rx_filter) {
> >> >>         case HWTSTAMP_FILTER_NONE:
> >> >> -               if (fep->hwts_rx_en)
> >> >> -                       fep->hwts_rx_en = 0;
> >> >> -               config.rx_filter = HWTSTAMP_FILTER_NONE;
 
The original patch seems fine. Thanks!

For the patch: Acked-by: Fugang Duan <fugang.duan@nxp.com>

Re: [EXT] [PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()

[Sergey Organov]

Andy Duan <fugang.duan@nxp.com> writes:

> From: Sergey Organov <sorganov@gmail.com> Sent: Wednesday, July 8, 2020 4:49 PM
>> Andy Duan <fugang.duan@nxp.com> writes:
>> 
>> > From: Sergey Organov <sorganov@gmail.com> Sent: Tuesday, July 7, 2020
>> > 10:43 PM
>> >> Andy Duan <fugang.duan@nxp.com> writes:
>> >>
>> >> > From: Sergey Organov <sorganov@gmail.com> Sent: Monday, July 6,
>> >> > 2020
>> >> 10:26 PM
>> >> >> Code of the form "if(x) x = 0" replaced with "x = 0".
>> >> >>
>> >> >> Code of the form "if(x == a) x = a" removed.
>> >> >>
>> >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> >> ---
>> >> >>  drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
>> >> >>  1 file changed, 1 insertion(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
>> >> >> b/drivers/net/ethernet/freescale/fec_ptp.c
>> >> >> index e455343..4152cae 100644
>> >> >> --- a/drivers/net/ethernet/freescale/fec_ptp.c
>> >> >> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
>> >> >> @@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev,
>> >> >> struct
>> >> ifreq
>> >> >> *ifr)
>> >> >>
>> >> >>         switch (config.rx_filter) {
>> >> >>         case HWTSTAMP_FILTER_NONE:
>> >> >> -               if (fep->hwts_rx_en)
>> >> >> -                       fep->hwts_rx_en = 0;
>> >> >> -               config.rx_filter = HWTSTAMP_FILTER_NONE;
>  
> The original patch seems fine. Thanks!
>
> For the patch: Acked-by: Fugang Duan <fugang.duan@nxp.com>

OK, thanks for reviewing!

-- Sergey

[PATCH 5/5] net: fec: replace snprintf() with strlcpy() in fec_ptp_init()

[Sergey Organov]

No need to use snprintf() on a constant string, nor using magic
constant in the fixed code was a good idea.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 4152cae..a0c1f44 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -582,7 +582,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 	int ret;
 
 	fep->ptp_caps.owner = THIS_MODULE;
-	snprintf(fep->ptp_caps.name, 16, "fec ptp");
+	strlcpy(fep->ptp_caps.name, "fec ptp", sizeof(fep->ptp_caps.name));
 
 	fep->ptp_caps.max_adj = 250000000;
 	fep->ptp_caps.n_alarm = 0;
-- 
2.10.0.1.g57b01a3

[PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Fix support for external PTP-aware devices such as DSA or PTP PHY:

Make sure we never time stamp tx packets when hardware time stamping
is disabled.

Check for PTP PHY being in use and then pass ioctls related to time
stamping of Ethernet packets to the PTP PHY rather than handle them
ourselves. In addition, disable our own hardware time stamping in this
case.

Fixes: 6605b73 ("FEC: Add time stamping code and a PTP hardware clock")
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---

v2:
  - Extracted from larger patch series
  - Description/comments updated according to discussions
  - Added Fixes: tag

 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++------
 drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index d8d76da..832a217 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -590,6 +590,7 @@ struct fec_enet_private {
 void fec_ptp_init(struct platform_device *pdev, int irq_idx);
 void fec_ptp_stop(struct platform_device *pdev);
 void fec_ptp_start_cyclecounter(struct net_device *ndev);
+void fec_ptp_disable_hwts(struct net_device *ndev);
 int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
 int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 3982285..cc7fbfc 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1294,8 +1294,13 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
 			ndev->stats.tx_bytes += skb->len;
 		}
 
-		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
-			fep->bufdesc_ex) {
+		/* NOTE: SKBTX_IN_PROGRESS being set does not imply it's we who
+		 * are to time stamp the packet, so we still need to check time
+		 * stamping enabled flag.
+		 */
+		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
+			     fep->hwts_tx_en) &&
+		    fep->bufdesc_ex) {
 			struct skb_shared_hwtstamps shhwtstamps;
 			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
 
@@ -2723,10 +2728,16 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
 		return -ENODEV;
 
 	if (fep->bufdesc_ex) {
-		if (cmd == SIOCSHWTSTAMP)
-			return fec_ptp_set(ndev, rq);
-		if (cmd == SIOCGHWTSTAMP)
-			return fec_ptp_get(ndev, rq);
+		bool use_fec_hwts = !phy_has_hwtstamp(phydev);
+
+		if (cmd == SIOCSHWTSTAMP) {
+			if (use_fec_hwts)
+				return fec_ptp_set(ndev, rq);
+			fec_ptp_disable_hwts(ndev);
+		} else if (cmd == SIOCGHWTSTAMP) {
+			if (use_fec_hwts)
+				return fec_ptp_get(ndev, rq);
+		}
 	}
 
 	return phy_mii_ioctl(phydev, rq, cmd);
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 945643c..f8a592c 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -452,6 +452,18 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
 	return -EOPNOTSUPP;
 }
 
+/**
+ * fec_ptp_disable_hwts - disable hardware time stamping
+ * @ndev: pointer to net_device
+ */
+void fec_ptp_disable_hwts(struct net_device *ndev)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	fep->hwts_tx_en = 0;
+	fep->hwts_rx_en = 0;
+}
+
 int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
-- 
2.10.0.1.g57b01a3

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Vladimir Oltean]

Hi Sergey,

On Sat, Jul 11, 2020 at 03:08:42PM +0300, Sergey Organov wrote:
> Fix support for external PTP-aware devices such as DSA or PTP PHY:
> 
> Make sure we never time stamp tx packets when hardware time stamping
> is disabled.
> 
> Check for PTP PHY being in use and then pass ioctls related to time
> stamping of Ethernet packets to the PTP PHY rather than handle them
> ourselves. In addition, disable our own hardware time stamping in this
> case.
> 
> Fixes: 6605b73 ("FEC: Add time stamping code and a PTP hardware clock")

Please use a 12-character sha1sum. Try to use the "pretty" format
specifier I gave you in the original thread, it saves you from counting,
and also from people complaining once it gets merged:

https://www.google.com/search?q=stephen+rothwell+%22fixes+tag+needs+some+work%22

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> 
> v2:
>   - Extracted from larger patch series
>   - Description/comments updated according to discussions
>   - Added Fixes: tag
> 
>  drivers/net/ethernet/freescale/fec.h      |  1 +
>  drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++------
>  drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
>  3 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index d8d76da..832a217 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -590,6 +590,7 @@ struct fec_enet_private {
>  void fec_ptp_init(struct platform_device *pdev, int irq_idx);
>  void fec_ptp_stop(struct platform_device *pdev);
>  void fec_ptp_start_cyclecounter(struct net_device *ndev);
> +void fec_ptp_disable_hwts(struct net_device *ndev);
>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
>  int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
>  
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 3982285..cc7fbfc 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1294,8 +1294,13 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>  			ndev->stats.tx_bytes += skb->len;
>  		}
>  
> -		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
> -			fep->bufdesc_ex) {
> +		/* NOTE: SKBTX_IN_PROGRESS being set does not imply it's we who
> +		 * are to time stamp the packet, so we still need to check time
> +		 * stamping enabled flag.
> +		 */
> +		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
> +			     fep->hwts_tx_en) &&
> +		    fep->bufdesc_ex) {
>  			struct skb_shared_hwtstamps shhwtstamps;
>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
>  
> @@ -2723,10 +2728,16 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
>  		return -ENODEV;
>  
>  	if (fep->bufdesc_ex) {
> -		if (cmd == SIOCSHWTSTAMP)
> -			return fec_ptp_set(ndev, rq);
> -		if (cmd == SIOCGHWTSTAMP)
> -			return fec_ptp_get(ndev, rq);
> +		bool use_fec_hwts = !phy_has_hwtstamp(phydev);

I thought we were in agreement that FEC does not support PHY
timestamping at this point, and this patch would only be fixing DSA
switches (even though PHYs would need this fixed too, when support is
added for them)? I would definitely not introduce support (and
incomplete, at that) for a new feature in a bugfix patch.

But it looks like we aren't.

> +
> +		if (cmd == SIOCSHWTSTAMP) {
> +			if (use_fec_hwts)
> +				return fec_ptp_set(ndev, rq);
> +			fec_ptp_disable_hwts(ndev);
> +		} else if (cmd == SIOCGHWTSTAMP) {
> +			if (use_fec_hwts)
> +				return fec_ptp_get(ndev, rq);
> +		}
>  	}
>  
>  	return phy_mii_ioctl(phydev, rq, cmd);
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 945643c..f8a592c 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -452,6 +452,18 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
>  	return -EOPNOTSUPP;
>  }
>  
> +/**
> + * fec_ptp_disable_hwts - disable hardware time stamping
> + * @ndev: pointer to net_device
> + */
> +void fec_ptp_disable_hwts(struct net_device *ndev)

This is not really needed, is it?
- PHY ability of hwtstamping does not change across the runtime of the
  kernel (or do you have a "special" one where it does?)
- The initial values for hwts_tx_en and hwts_rx_en are already 0
- There is no code path for which it is possible for hwts_tx_en or
  hwts_rx_en to have been non-zero prior to this call making them zero.

It is just "to be sure", in a very non-necessary way.

But nonetheless, it shouldn't be present in this patch either way, due
to the fact that one patch should have one topic only, and the topic of
this patch should be solving a clearly defined bug.

> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +
> +	fep->hwts_tx_en = 0;
> +	fep->hwts_rx_en = 0;
> +}
> +
>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -- 
> 2.10.0.1.g57b01a3
> 

Thanks,
-Vladimir

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> Hi Sergey,
>
> On Sat, Jul 11, 2020 at 03:08:42PM +0300, Sergey Organov wrote:
>> Fix support for external PTP-aware devices such as DSA or PTP PHY:
>> 
>> Make sure we never time stamp tx packets when hardware time stamping
>> is disabled.
>> 
>> Check for PTP PHY being in use and then pass ioctls related to time
>> stamping of Ethernet packets to the PTP PHY rather than handle them
>> ourselves. In addition, disable our own hardware time stamping in this
>> case.
>> 
>> Fixes: 6605b73 ("FEC: Add time stamping code and a PTP hardware clock")
>
> Please use a 12-character sha1sum. Try to use the "pretty" format
> specifier I gave you in the original thread, it saves you from
> counting,

I did as you suggested:

[pretty]
        fixes = Fixes: %h (\"%s\")
[alias]
	fixes = show --no-patch --pretty='Fixes: %h (\"%s\")'

And that's what it gave me. Dunno, maybe its Git version that is
responsible?

I now tried to find a way to specify the number of digits in the
abbreviated hash in the format, but failed. There is likely some global
setting for minimum number of digits, but I'm yet to find it. Any idea?

> and also from people complaining once it gets merged:
>
> https://www.google.com/search?q=stephen+rothwell+%22fixes+tag+needs+some+work%22
>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> 
>> v2:
>>   - Extracted from larger patch series
>>   - Description/comments updated according to discussions
>>   - Added Fixes: tag
>> 
>>  drivers/net/ethernet/freescale/fec.h      |  1 +
>>  drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++------
>>  drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
>>  3 files changed, 30 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
>> index d8d76da..832a217 100644
>> --- a/drivers/net/ethernet/freescale/fec.h
>> +++ b/drivers/net/ethernet/freescale/fec.h
>> @@ -590,6 +590,7 @@ struct fec_enet_private {
>>  void fec_ptp_init(struct platform_device *pdev, int irq_idx);
>>  void fec_ptp_stop(struct platform_device *pdev);
>>  void fec_ptp_start_cyclecounter(struct net_device *ndev);
>> +void fec_ptp_disable_hwts(struct net_device *ndev);
>>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
>>  int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
>>  
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index 3982285..cc7fbfc 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1294,8 +1294,13 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>>  			ndev->stats.tx_bytes += skb->len;
>>  		}
>>  
>> -		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
>> -			fep->bufdesc_ex) {
>> +		/* NOTE: SKBTX_IN_PROGRESS being set does not imply it's we who
>> +		 * are to time stamp the packet, so we still need to check time
>> +		 * stamping enabled flag.
>> +		 */
>> +		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
>> +			     fep->hwts_tx_en) &&
>> +		    fep->bufdesc_ex) {
>>  			struct skb_shared_hwtstamps shhwtstamps;
>>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
>>  
>> @@ -2723,10 +2728,16 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
>>  		return -ENODEV;
>>  
>>  	if (fep->bufdesc_ex) {
>> -		if (cmd == SIOCSHWTSTAMP)
>> -			return fec_ptp_set(ndev, rq);
>> -		if (cmd == SIOCGHWTSTAMP)
>> -			return fec_ptp_get(ndev, rq);
>> +		bool use_fec_hwts = !phy_has_hwtstamp(phydev);
>
> I thought we were in agreement that FEC does not support PHY
> timestamping at this point, and this patch would only be fixing DSA
> switches (even though PHYs would need this fixed too, when support is
> added for them)? I would definitely not introduce support (and
> incomplete, at that) for a new feature in a bugfix patch.
>
> But it looks like we aren't.

We were indeed, and, honestly, I did prepare the split version of the
changes. But then I felt uneasy describing these commits, as I realized
that I fix single source file and single original commit by adding
proper support for a single feature that is described in your (single)
recent document, but with 2 separate commits, each of which solves only
half of the problem. I felt I need to somehow explain why could somebody
want half a fix, and didn't know how, so I've merged them back into
single commit.

In case you insist they are to be separate, I do keep the split version
in my git tree, but to finish it that way, I'd like to clarify a few
details:

1. Should it be patch series with 2 commits, or 2 entirely separate
patches?

2. If patch series, which change should go first? Here please notice
that ioctl() change makes no sense without SKBTX fix unconditionally,
while SKBTX fix makes no sense without ioctl() fix for PTP PHY users
only.

3. If entirely separate patches, should I somehow refer to SKBTX patch in
ioctl() one (and/or vice versa), to make it explicit they are
(inter)dependent? 

4. How/if should I explain why anybody would benefit from applying
SKBTX patch, yet be in trouble applying ioctl() one? 

>
>> +
>> +		if (cmd == SIOCSHWTSTAMP) {
>> +			if (use_fec_hwts)
>> +				return fec_ptp_set(ndev, rq);
>> +			fec_ptp_disable_hwts(ndev);
>> +		} else if (cmd == SIOCGHWTSTAMP) {
>> +			if (use_fec_hwts)
>> +				return fec_ptp_get(ndev, rq);
>> +		}
>>  	}
>>  
>>  	return phy_mii_ioctl(phydev, rq, cmd);
>> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
>> index 945643c..f8a592c 100644
>> --- a/drivers/net/ethernet/freescale/fec_ptp.c
>> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
>> @@ -452,6 +452,18 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
>>  	return -EOPNOTSUPP;
>>  }
>>  
>> +/**
>> + * fec_ptp_disable_hwts - disable hardware time stamping
>> + * @ndev: pointer to net_device
>> + */
>> +void fec_ptp_disable_hwts(struct net_device *ndev)
>
> This is not really needed, is it?
> - PHY ability of hwtstamping does not change across the runtime of the
>   kernel (or do you have a "special" one where it does?)
> - The initial values for hwts_tx_en and hwts_rx_en are already 0
> - There is no code path for which it is possible for hwts_tx_en or
>   hwts_rx_en to have been non-zero prior to this call making them
>   zero.

If everybody agree it is not needed, I'm fine getting it out of the
patch, but please consider my worries below.

I'm afraid your third statement might happen to be not exactly true.
It's due to this same path the hwts_tx_en could end-up being set, as we
have if() on phy_has_hwtstamp(phydev), so the path can set hwts_xx_en if
this code gets somehow run when phydev->phy is set to 0, or attached PHY
is not yet has PTP property.

I'm not sure it can happen, but essential thing here is that I have no
evidence it can't, and another place to ensure hwts_xx_en fields are
cleared would be at the time of attachment of PTP-aware PHY, by check
and clear and that time, yet even here it'd rely on PTP-awareness being
already established at the moment.

The second variant is harder for me to figure, yet is less reliable, so,
overall, I preferred to keep the proposed solution that I believe should
work no matter what, and let somebody who is more fluid in the code-base
to get responsibility to remove it. For example, I didn't want to even
start to consider how all this behaves on cable connect/disconnect, if
up/down, or over hibernation.

>
> It is just "to be sure", in a very non-necessary way.

It is "to be sure", without "just", as I tried to explain above. If it
were my own code, I'd ask for an evidence that this part is not needed,
before getting rid of this safety belt.

>
> But nonetheless, it shouldn't be present in this patch either way, due
> to the fact that one patch should have one topic only, and the topic of
> this patch should be solving a clearly defined bug.

I actually don't care either way, but to be picky, the answer depends on
particular definition of the bug, and the bug I have chased, and its
definition I've used, even in the original series, requires simultaneous
fixes in 2 places of the code.

You have yet another bug in mind that part of my original patch happens
to solve, yes, but that, by itself, doesn't necessarily mean the patch
should be split. Nevertheless, I honestly tried to split it, according
to our agreement, but failed, see above, and I still willing to try
again, provided somebody actually needs it.

Thanks,
-- Sergey

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Andrew Lunn]

> I did as you suggested:
> 
> [pretty]
>         fixes = Fixes: %h (\"%s\")
> [alias]
> 	fixes = show --no-patch --pretty='Fixes: %h (\"%s\")'
> 
> And that's what it gave me. Dunno, maybe its Git version that is
> responsible?

[core]
        abbrev = 12

	Andrew

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Vladimir Oltean]

On Sun, Jul 12, 2020 at 05:16:56PM +0300, Sergey Organov wrote:
> Vladimir Oltean <olteanv@gmail.com> writes:
> 
> > Hi Sergey,
> >
> > On Sat, Jul 11, 2020 at 03:08:42PM +0300, Sergey Organov wrote:
> >> Fix support for external PTP-aware devices such as DSA or PTP PHY:
> >> 
> >> Make sure we never time stamp tx packets when hardware time stamping
> >> is disabled.
> >> 
> >> Check for PTP PHY being in use and then pass ioctls related to time
> >> stamping of Ethernet packets to the PTP PHY rather than handle them
> >> ourselves. In addition, disable our own hardware time stamping in this
> >> case.
> >> 
> >> Fixes: 6605b73 ("FEC: Add time stamping code and a PTP hardware clock")
> >
> > Please use a 12-character sha1sum. Try to use the "pretty" format
> > specifier I gave you in the original thread, it saves you from
> > counting,
> 
> I did as you suggested:
> 
> [pretty]
>         fixes = Fixes: %h (\"%s\")
> [alias]
> 	fixes = show --no-patch --pretty='Fixes: %h (\"%s\")'
> 
> And that's what it gave me. Dunno, maybe its Git version that is
> responsible?
> 
> I now tried to find a way to specify the number of digits in the
> abbreviated hash in the format, but failed. There is likely some global
> setting for minimum number of digits, but I'm yet to find it. Any idea?
> 

Sorry, my fault. I gave you only partial settings. Use this:

[core]
	abbrev = 12
[pretty]
	fixes = Fixes: %h (\"%s\")

> > and also from people complaining once it gets merged:
> >
> > https://www.google.com/search?q=stephen+rothwell+%22fixes+tag+needs+some+work%22
> >
> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >> ---
> >> 
> >> v2:
> >>   - Extracted from larger patch series
> >>   - Description/comments updated according to discussions
> >>   - Added Fixes: tag
> >> 
> >>  drivers/net/ethernet/freescale/fec.h      |  1 +
> >>  drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++------
> >>  drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
> >>  3 files changed, 30 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> >> index d8d76da..832a217 100644
> >> --- a/drivers/net/ethernet/freescale/fec.h
> >> +++ b/drivers/net/ethernet/freescale/fec.h
> >> @@ -590,6 +590,7 @@ struct fec_enet_private {
> >>  void fec_ptp_init(struct platform_device *pdev, int irq_idx);
> >>  void fec_ptp_stop(struct platform_device *pdev);
> >>  void fec_ptp_start_cyclecounter(struct net_device *ndev);
> >> +void fec_ptp_disable_hwts(struct net_device *ndev);
> >>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
> >>  int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
> >>  
> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> >> index 3982285..cc7fbfc 100644
> >> --- a/drivers/net/ethernet/freescale/fec_main.c
> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> @@ -1294,8 +1294,13 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> >>  			ndev->stats.tx_bytes += skb->len;
> >>  		}
> >>  
> >> -		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
> >> -			fep->bufdesc_ex) {
> >> +		/* NOTE: SKBTX_IN_PROGRESS being set does not imply it's we who
> >> +		 * are to time stamp the packet, so we still need to check time
> >> +		 * stamping enabled flag.
> >> +		 */
> >> +		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
> >> +			     fep->hwts_tx_en) &&
> >> +		    fep->bufdesc_ex) {
> >>  			struct skb_shared_hwtstamps shhwtstamps;
> >>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
> >>  
> >> @@ -2723,10 +2728,16 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
> >>  		return -ENODEV;
> >>  
> >>  	if (fep->bufdesc_ex) {
> >> -		if (cmd == SIOCSHWTSTAMP)
> >> -			return fec_ptp_set(ndev, rq);
> >> -		if (cmd == SIOCGHWTSTAMP)
> >> -			return fec_ptp_get(ndev, rq);
> >> +		bool use_fec_hwts = !phy_has_hwtstamp(phydev);
> >
> > I thought we were in agreement that FEC does not support PHY
> > timestamping at this point, and this patch would only be fixing DSA
> > switches (even though PHYs would need this fixed too, when support is
> > added for them)? I would definitely not introduce support (and
> > incomplete, at that) for a new feature in a bugfix patch.
> >
> > But it looks like we aren't.
> 
> We were indeed, and, honestly, I did prepare the split version of the
> changes. But then I felt uneasy describing these commits, as I realized
> that I fix single source file and single original commit by adding
> proper support for a single feature that is described in your (single)
> recent document, but with 2 separate commits, each of which solves only
> half of the problem. I felt I need to somehow explain why could somebody
> want half a fix, and didn't know how, so I've merged them back into
> single commit.
> 

Right now there are 2 mainline DSA timestamping drivers that could be
paired with the FEC driver: mv88e6xxx and sja1105 (there is a third one,
felix, which is an embedded L2 switch, so its DSA master is known and
fixed, and it's not FEC). In practice, there are boards out there that
use FEC in conjunction with both these DSA switch families.

As far as I understand. the reason why SKBTX_IN_PROGRESS exists is for
skb_tx_timestamp() to only provide a software timestamp if the hardware
timestamping isn't going to. So hardware timestamping logic must signal
its intention. With SO_TIMESTAMPING, this should not be strictly
necessary, as this UAPI supports multiple sources of timestamping
(including software and hardware together), but I think
SKBTX_IN_PROGRESS predates this UAPI and timestamping should continue to
work with older socket options.

Now, out of the 2 mainline DSA drivers, 1 of them isn't setting
SKBTX_IN_PROGRESS, and that is mv88e6xxx. So mv88e6xxx isn't triggerring
this bug. I'm not sure why it isn't setting the flag. It might very well
be that the author of the patch had a board with a FEC DSA master, and
setting this flag made bad things happen, so he just left it unset.
Doesn't really matter.
But sja1105 is setting the flag:

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/sja1105/sja1105_ptp.c#n890

So, at the very least, you are fixing PTP on DSA setups with FEC as
master and sja1105 as switch. Boards like that do exist.

> In case you insist they are to be separate, I do keep the split version
> in my git tree, but to finish it that way, I'd like to clarify a few
> details:
> 
> 1. Should it be patch series with 2 commits, or 2 entirely separate
> patches?
> 

Entirely separate.

> 2. If patch series, which change should go first? Here please notice
> that ioctl() change makes no sense without SKBTX fix unconditionally,
> while SKBTX fix makes no sense without ioctl() fix for PTP PHY users
> only.
> 

Please look at the SKBTX fix from the perspective of code that currently
exists in mainline, and not from the perspective of what you have in
mind.

> 3. If entirely separate patches, should I somehow refer to SKBTX patch in
> ioctl() one (and/or vice versa), to make it explicit they are
> (inter)dependent? 
> 

Nope. The PHY timestamping support will go to David's net-next, this
common PHY/DSA bugfix to net, and they'll meet sooner rather than later.

> 4. How/if should I explain why anybody would benefit from applying
> SKBTX patch, yet be in trouble applying ioctl() one? 
> 

Could you please rephrase? I don't understand the part about being in
trouble for applying the ioctl patch.

Thanks,
-Vladimir

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Sun, Jul 12, 2020 at 05:16:56PM +0300, Sergey Organov wrote:
>> Vladimir Oltean <olteanv@gmail.com> writes:
>> 
>> > Hi Sergey,
>> >
>> > On Sat, Jul 11, 2020 at 03:08:42PM +0300, Sergey Organov wrote:
>> >> Fix support for external PTP-aware devices such as DSA or PTP PHY:
>> >> 
>> >> Make sure we never time stamp tx packets when hardware time stamping
>> >> is disabled.
>> >> 
>> >> Check for PTP PHY being in use and then pass ioctls related to time
>> >> stamping of Ethernet packets to the PTP PHY rather than handle them
>> >> ourselves. In addition, disable our own hardware time stamping in this
>> >> case.
>> >> 
>> >> Fixes: 6605b73 ("FEC: Add time stamping code and a PTP hardware clock")
>> >
>> > Please use a 12-character sha1sum. Try to use the "pretty" format
>> > specifier I gave you in the original thread, it saves you from
>> > counting,
>> 
>> I did as you suggested:
>> 
>> [pretty]
>>         fixes = Fixes: %h (\"%s\")
>> [alias]
>> 	fixes = show --no-patch --pretty='Fixes: %h (\"%s\")'
>> 
>> And that's what it gave me. Dunno, maybe its Git version that is
>> responsible?
>> 
>> I now tried to find a way to specify the number of digits in the
>> abbreviated hash in the format, but failed. There is likely some global
>> setting for minimum number of digits, but I'm yet to find it. Any idea?
>> 
>
> Sorry, my fault. I gave you only partial settings. Use this:
>
> [core]
> 	abbrev = 12

Thanks, Vladimir and Andrew!

>
>> > and also from people complaining once it gets merged:
>> >
>> > https://www.google.com/search?q=stephen+rothwell+%22fixes+tag+needs+some+work%22
>> >
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> ---
>> >> 
>> >> v2:
>> >>   - Extracted from larger patch series
>> >>   - Description/comments updated according to discussions
>> >>   - Added Fixes: tag
>> >> 
>> >>  drivers/net/ethernet/freescale/fec.h      |  1 +
>> >>  drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++------
>> >>  drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
>> >>  3 files changed, 30 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
>> >> index d8d76da..832a217 100644
>> >> --- a/drivers/net/ethernet/freescale/fec.h
>> >> +++ b/drivers/net/ethernet/freescale/fec.h
>> >> @@ -590,6 +590,7 @@ struct fec_enet_private {
>> >>  void fec_ptp_init(struct platform_device *pdev, int irq_idx);
>> >>  void fec_ptp_stop(struct platform_device *pdev);
>> >>  void fec_ptp_start_cyclecounter(struct net_device *ndev);
>> >> +void fec_ptp_disable_hwts(struct net_device *ndev);
>> >>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
>> >>  int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
>> >>  
>> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> >> index 3982285..cc7fbfc 100644
>> >> --- a/drivers/net/ethernet/freescale/fec_main.c
>> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> >> @@ -1294,8 +1294,13 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>> >>  			ndev->stats.tx_bytes += skb->len;
>> >>  		}
>> >>  
>> >> -		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
>> >> -			fep->bufdesc_ex) {
>> >> +		/* NOTE: SKBTX_IN_PROGRESS being set does not imply it's we who
>> >> +		 * are to time stamp the packet, so we still need to check time
>> >> +		 * stamping enabled flag.
>> >> +		 */
>> >> +		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
>> >> +			     fep->hwts_tx_en) &&
>> >> +		    fep->bufdesc_ex) {
>> >>  			struct skb_shared_hwtstamps shhwtstamps;
>> >>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
>> >>  

[...]

> As far as I understand. the reason why SKBTX_IN_PROGRESS exists is for
> skb_tx_timestamp() to only provide a software timestamp if the hardware
> timestamping isn't going to. So hardware timestamping logic must signal
> its intention. With SO_TIMESTAMPING, this should not be strictly
> necessary, as this UAPI supports multiple sources of timestamping
> (including software and hardware together),

As a side note, I tried, but didn't find a way to get 2 timestamps,
software and hardware, for the same packet. It looks like once upon a
time it was indeed supported by:

SOF_TIMESTAMPING_SYS_HARDWARE: This option is deprecated and ignored.

> but I think
> SKBTX_IN_PROGRESS predates this UAPI and timestamping should continue to
> work with older socket options.

<rant>The UAPI to all this is rather messy, starting with ugly tricks to
convert PTP file descriptors to clock IDs, followed by strange ways to
figure correct PTP clock for given Ethernet interface, followed by
entirely different methods of getting time stamping capabilities and
configuring them, and so forth.</rant>

>
> Now, out of the 2 mainline DSA drivers, 1 of them isn't setting
> SKBTX_IN_PROGRESS, and that is mv88e6xxx. So mv88e6xxx isn't triggerring
> this bug. I'm not sure why it isn't setting the flag. It might very well
> be that the author of the patch had a board with a FEC DSA master, and
> setting this flag made bad things happen, so he just left it unset.
> Doesn't really matter.
> But sja1105 is setting the flag:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/sja1105/sja1105_ptp.c#n890
>
> So, at the very least, you are fixing PTP on DSA setups with FEC as
> master and sja1105 as switch. Boards like that do exist.

I'll do as you suggest, but I want to say that I didn't question your
claim that my proposed changes may fix some existing PTP/DSA setup.

What I still find doubtful is that this fact necessarily means that the
part of the patch that fixes some other bug must be submitted
separately. If it were the rule, almost no patch that needs to fix 2
separate places would be accepted, as there might be some bug somewhere
that could be fixed by 1 change, no?

In this particular case, you just happen to identify such a bug
immediately, but I, as patch submitter, should not be expected to check
all the kernel for other possible bugs that my changes may happen to
fix, no?

It seems like I misunderstand something very basic and that bothers me.

>
>> In case you insist they are to be separate, I do keep the split version
>> in my git tree, but to finish it that way, I'd like to clarify a few
>> details:
>> 
>> 1. Should it be patch series with 2 commits, or 2 entirely separate
>> patches?
>> 
>
> Entirely separate.

OK, will do a separate patch, as you suggest.

[...]

>
>> 3. If entirely separate patches, should I somehow refer to SKBTX patch in
>> ioctl() one (and/or vice versa), to make it explicit they are
>> (inter)dependent? 
>> 
>
> Nope. The PHY timestamping support will go to David's net-next, this
> common PHY/DSA bugfix to net, and they'll meet sooner rather than
> later.

I'll do as you suggest, separating the patches, yet I fail to see why
PHY /time stamping bug fix/ should go to another tree than PHY/DSA /time
stamping bug fix/? What's the essential difference? Could you please
clarify?

Thanks,
-- Sergey

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Vladimir Oltean]

On Sun, Jul 12, 2020 at 08:29:46PM +0300, Sergey Organov wrote:
> Vladimir Oltean <olteanv@gmail.com> writes:
> 
> > As far as I understand. the reason why SKBTX_IN_PROGRESS exists is for
> > skb_tx_timestamp() to only provide a software timestamp if the hardware
> > timestamping isn't going to. So hardware timestamping logic must signal
> > its intention. With SO_TIMESTAMPING, this should not be strictly
> > necessary, as this UAPI supports multiple sources of timestamping
> > (including software and hardware together),
> 
> As a side note, I tried, but didn't find a way to get 2 timestamps,
> software and hardware, for the same packet. It looks like once upon a
> time it was indeed supported by:
> 
> SOF_TIMESTAMPING_SYS_HARDWARE: This option is deprecated and ignored.
> 

With SO_TIMESTAMPING, it is supported through
SOF_TIMESTAMPING_OPT_TX_SWHW.
With APIs pre-SO_TIMESTAMPING (such as SO_TIMESTAMPNS), my understanding
is that it is not supported. One timestamp would overwrite the other. So
this needs to be avoided somehow, and this is how SKBTX_IN_PROGRESS came
to be.

> > but I think
> > SKBTX_IN_PROGRESS predates this UAPI and timestamping should continue to
> > work with older socket options.
> 
> <rant>The UAPI to all this is rather messy, starting with ugly tricks to
> convert PTP file descriptors to clock IDs, followed by strange ways to
> figure correct PTP clock for given Ethernet interface, followed by
> entirely different methods of getting time stamping capabilities and
> configuring them, and so forth.</rant>
> 

Yes, but I don't think this really matters in any way here.

> >
> > Now, out of the 2 mainline DSA drivers, 1 of them isn't setting
> > SKBTX_IN_PROGRESS, and that is mv88e6xxx. So mv88e6xxx isn't triggerring
> > this bug. I'm not sure why it isn't setting the flag. It might very well
> > be that the author of the patch had a board with a FEC DSA master, and
> > setting this flag made bad things happen, so he just left it unset.
> > Doesn't really matter.
> > But sja1105 is setting the flag:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/sja1105/sja1105_ptp.c#n890
> >
> > So, at the very least, you are fixing PTP on DSA setups with FEC as
> > master and sja1105 as switch. Boards like that do exist.
> 
> I'll do as you suggest, but I want to say that I didn't question your
> claim that my proposed changes may fix some existing PTP/DSA setup.
> 
> What I still find doubtful is that this fact necessarily means that the
> part of the patch that fixes some other bug must be submitted
> separately. If it were the rule, almost no patch that needs to fix 2
> separate places would be accepted, as there might be some bug somewhere
> that could be fixed by 1 change, no?
> 

So, I am asking you to flag this as a separate bugfix for DSA
timestamping for multiple reasons:

- because you are not "fixing PHY timestamping", more on that separately
- because I am looking at this problem from the perspective of a user
  who has a problem with DSA timestamping (aka code that already exists,
  and that is already claimed to work). They're going to be searching
  through the git log for potentially relevant changes, and even if they
  might notice a patch that "adds support for PHY timestamping in FEC"*,
  it might not register as something relevant to them, and skip it.
  Just as you don't care about DSA, neither does that hypothetical user
  care about PHY timestamping.



According to its design principles, DSA TX timestamping is supposed to
"just work". The FEC driver should have been DSA-ready from day one,
however it wasn't. In fact, subtle requirements from the MAC driver,
such as not indulging in solipsism (like gianfar, fec and probably many
more other drivers, in more subtle ways **), were unobvious enough to
the designers of DSA timestamping that they didn't, probably, even see
this coming.

Look, if the argument you're trying to make is that you should be using
this tag instead:

Fixes: 90af1059c52c ("net: dsa: forward timestamping callbacks to switch drivers")

since what gianfar/fec/etc were doing was not illegal as part of the
rules "back then", then I won't oppose that.



And according to the design principles of PHY TX timestamping, that is
not supposed to "just work" unless there is MAC driver collaboration.
That collaboration is _obviously_ not there for FEC, you don't even need
to open one kernel source code file to see that, just run a simple
command from user space, see below.

> In this particular case, you just happen to identify such a bug
> immediately, but I, as patch submitter, should not be expected to check
> all the kernel for other possible bugs that my changes may happen to
> fix, no?
> 

Well, ideally you would, it gives reviewers and readers confidence that
you understand the changes you are making. Some time in the future, when
you're no longer going to be reachable for questions, the commit still
needs to speak for itself.

> It seems like I misunderstand something very basic and that bothers me.
> 

Yes, but I can't seem to pinpoint what that is...
I _think_ the problem seems to be that you're not differentiating your
point of view from the mainline kernel's point of view.

> >
> >> In case you insist they are to be separate, I do keep the split version
> >> in my git tree, but to finish it that way, I'd like to clarify a few
> >> details:
> >> 
> >> 1. Should it be patch series with 2 commits, or 2 entirely separate
> >> patches?
> >> 
> >
> > Entirely separate.
> 
> OK, will do a separate patch, as you suggest.
> 
> [...]
> 
> >
> >> 3. If entirely separate patches, should I somehow refer to SKBTX patch in
> >> ioctl() one (and/or vice versa), to make it explicit they are
> >> (inter)dependent? 
> >> 
> >
> > Nope. The PHY timestamping support will go to David's net-next, this
> > common PHY/DSA bugfix to net, and they'll meet sooner rather than
> > later.
> 
> I'll do as you suggest, separating the patches, yet I fail to see why
> PHY /time stamping bug fix/ should go to another tree than PHY/DSA /time
> stamping bug fix/? What's the essential difference? Could you please
> clarify?
> 

The essential difference is that for PHY timestamping, there is no
feature that is claimed to work which doesn't work.

If you run "ethtool -T eth0" on a FEC network interface, it will always
report its own PHC, and never the PHC of a PHY. So, you cannot claim
that you are fixing PHY timestamping, since PHY timestamping is not
advertised. That's not what a bug fix is, at least not around here, with
its associated backporting efforts.

The only way you could have claimed that this was fixing PHY
timestamping was if "ethtool -T eth0" was reporting a PHY PHC, however
timestamps were not coming from the PHY.
From the perspective of the mainline kernel, that can never happen.
From your perspective as a developer, in your private work tree, where
_you_ added the necessary wiring for PHY timestamping, I fully
understand that this is exactly what happened _to_you_.
I am not saying that PHY timestamping doesn't need this issue fixed. It
does, and if it weren't for DSA, it would have simply been a "new
feature", and it would have been ok to have everything in the same patch.

The fact that you figured out so quickly what's going on just means
you're very smart. It took me 2 months to find out the same bug coming
from the DSA side of things.

> Thanks,
> -- Sergey

* That was my first complaint about your patch. You've changed that
  since, and your new commit is odd because it's now adding a new
  feature in a bug fix, therefore for a totally different reason.

** After I fixed gianfar, I did grep for other drivers that might be
   suffering from the same issue. The FEC driver did *not* strike me as
   obviously broken, and I was *specifically* searching for this bug
   pattern.

Thanks,
-Vladimir

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Sun, Jul 12, 2020 at 08:29:46PM +0300, Sergey Organov wrote:
>> Vladimir Oltean <olteanv@gmail.com> writes:
>> 
>> > As far as I understand. the reason why SKBTX_IN_PROGRESS exists is for
>> > skb_tx_timestamp() to only provide a software timestamp if the hardware
>> > timestamping isn't going to. So hardware timestamping logic must signal
>> > its intention. With SO_TIMESTAMPING, this should not be strictly
>> > necessary, as this UAPI supports multiple sources of timestamping
>> > (including software and hardware together),
>> 
>> As a side note, I tried, but didn't find a way to get 2 timestamps,
>> software and hardware, for the same packet. It looks like once upon a
>> time it was indeed supported by:
>> 
>> SOF_TIMESTAMPING_SYS_HARDWARE: This option is deprecated and ignored.
>> 
>
> With SO_TIMESTAMPING, it is supported through
> SOF_TIMESTAMPING_OPT_TX_SWHW.

This one I overlooked, -- thanks for pointing!

> With APIs pre-SO_TIMESTAMPING (such as SO_TIMESTAMPNS), my understanding
> is that it is not supported. One timestamp would overwrite the other. So
> this needs to be avoided somehow, and this is how SKBTX_IN_PROGRESS came
> to be.
>
>
>> > but I think
>> > SKBTX_IN_PROGRESS predates this UAPI and timestamping should continue to
>> > work with older socket options.
>> 
>> <rant>The UAPI to all this is rather messy, starting with ugly tricks to
>> convert PTP file descriptors to clock IDs, followed by strange ways to
>> figure correct PTP clock for given Ethernet interface, followed by
>> entirely different methods of getting time stamping capabilities and
>> configuring them, and so forth.</rant>
>> 
>
> Yes, but I don't think this really matters in any way here.

Surprisingly it does, as you've got a wrong premise that ethtool doesn't
support external PTP PHY on FEC. And I think it's due to complexities of
the current implementation, where ethtool has entirely separate path of
doing things, with zero help from the FEC driver, see below.

[...]

>> I'll do as you suggest, but I want to say that I didn't question your
>> claim that my proposed changes may fix some existing PTP/DSA setup.
>> 
>> What I still find doubtful is that this fact necessarily means that the
>> part of the patch that fixes some other bug must be submitted
>> separately. If it were the rule, almost no patch that needs to fix 2
>> separate places would be accepted, as there might be some bug somewhere
>> that could be fixed by 1 change, no?
>> 
>
> So, I am asking you to flag this as a separate bugfix for DSA
> timestamping for multiple reasons:

OK, you want it, I'll do it, however:

>
> - because you are not "fixing PHY timestamping", more on that
> separately

I believe I do, more on that below.

> - because I am looking at this problem from the perspective of a user
>   who has a problem with DSA timestamping (aka code that already exists,
>   and that is already claimed to work). They're going to be searching
>   through the git log for potentially relevant changes, and even if they
>   might notice a patch that "adds support for PHY timestamping in FEC"*,
>   it might not register as something relevant to them, and skip it.
>   Just as you don't care about DSA, neither does that hypothetical user
>   care about PHY timestamping.

I don't think it's fair. I believe I do care about both, please re-read
the subject and description of the patch:

"net: fec: fix hardware time stamping by external devices"

that sure must be relevant for any external device, be it PHY or DSA,
and then:

"Fix support for external PTP-aware devices such as DSA or PTP PHY:"

And while formally I do add "support for external PTP PHY in FEC", as
you mention, I still consider it a bug-fix, as ethtool already correctly
supports this, see below for more background.

[...]

>> > Nope. The PHY timestamping support will go to David's net-next, this
>> > common PHY/DSA bugfix to net, and they'll meet sooner rather than
>> > later.
>> 
>> I'll do as you suggest, separating the patches, yet I fail to see why
>> PHY /time stamping bug fix/ should go to another tree than PHY/DSA /time
>> stamping bug fix/? What's the essential difference? Could you please
>> clarify?
>> 
>
> The essential difference is that for PHY timestamping, there is no
> feature that is claimed to work which doesn't work.

I believe this is a feature that is supposed to work, yet it doesn't,
see below.

>
> If you run "ethtool -T eth0" on a FEC network interface, it will always
> report its own PHC, and never the PHC of a PHY. So, you cannot claim
> that you are fixing PHY timestamping, since PHY timestamping is not
> advertised. That's not what a bug fix is, at least not around here, with
> its associated backporting efforts.

You can't actually try it as you don't have the hardware, right? As for
me, rather than running exactly ethtool, I do corresponding ioctl() in
my program, and the kernel does report features of my external PTP PHY,
not of internal one of the FEC, without my patches!

> The only way you could have claimed that this was fixing PHY
> timestamping was if "ethtool -T eth0" was reporting a PHY PHC, however
> timestamps were not coming from the PHY.

That's /exactly/ the case! Moreover, my original work is on 4.9.146
kernel, so ethtool works correctly at least since then. Here is quote from
my original question that I already gave reference to:

<quote>
Almost everything works fine out of the box, except hardware
timestamping. The problems are that I apparently get timestamps from fec
built-in PTP instead of external PHY, and that

  ioctl(fd, SIOCSHWTSTAMP, &ifr)

ends up being executed by fec1 built-in PTP code instead of being
forwarded to the external PHY, and that this happens despite the call to

   info.cmd = ETHTOOL_GET_TS_INFO;                                                                             
   ioctl(fd, SIOCETHTOOL, &ifr);                                                                     

returning phc_index = 1 that corresponds to external PHY, and reports
features of the external PHY, leading to major inconsistency as seen
from user-space.
</quote>

You see? This is exactly the case where I could claim fixing PHY time
stamping even according to your own expertise!

> From the perspective of the mainline kernel, that can never happen.

Yet in happened to me, and in some way because of the UAPI deficiencies
I've mentioned, as ethtool has entirely separate code path, that happens
to be correct for a long time already.

> From your perspective as a developer, in your private work tree, where
> _you_ added the necessary wiring for PHY timestamping, I fully
> understand that this is exactly what happened _to_you_.
> I am not saying that PHY timestamping doesn't need this issue fixed. It
> does, and if it weren't for DSA, it would have simply been a "new
> feature", and it would have been ok to have everything in the same
> patch.

Except that it's not a "new feature", but a bug-fix of an existing one,
as I see it.

>
> The fact that you figured out so quickly what's going on just means
> you're very smart. It took me 2 months to find out the same bug coming
> from the DSA side of things.

Thanks, but I'd rather attribute this to the fact that in my case there
was the code that worked correctly, so I was lucky to get support point
to rely upon.

Thanks,
-- Sergey

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Vladimir Oltean]

On Mon, Jul 13, 2020 at 01:32:09AM +0300, Sergey Organov wrote:
> >
> > If you run "ethtool -T eth0" on a FEC network interface, it will always
> > report its own PHC, and never the PHC of a PHY. So, you cannot claim
> > that you are fixing PHY timestamping, since PHY timestamping is not
> > advertised. That's not what a bug fix is, at least not around here, with
> > its associated backporting efforts.
>
> You can't actually try it as you don't have the hardware, right? As for
> me, rather than running exactly ethtool, I do corresponding ioctl() in
> my program, and the kernel does report features of my external PTP PHY,
> not of internal one of the FEC, without my patches!
>
> > The only way you could have claimed that this was fixing PHY
> > timestamping was if "ethtool -T eth0" was reporting a PHY PHC, however
> > timestamps were not coming from the PHY.
>
> That's /exactly/ the case! Moreover, my original work is on 4.9.146
> kernel, so ethtool works correctly at least since then. Here is quote from
> my original question that I already gave reference to:
>
> <quote>
> Almost everything works fine out of the box, except hardware
> timestamping. The problems are that I apparently get timestamps from fec
> built-in PTP instead of external PHY, and that
>
>   ioctl(fd, SIOCSHWTSTAMP, &ifr)
>
> ends up being executed by fec1 built-in PTP code instead of being
> forwarded to the external PHY, and that this happens despite the call to
>
>    info.cmd = ETHTOOL_GET_TS_INFO;
>    ioctl(fd, SIOCETHTOOL, &ifr);
>
> returning phc_index = 1 that corresponds to external PHY, and reports
> features of the external PHY, leading to major inconsistency as seen
> from user-space.
> </quote>
>
> You see? This is exactly the case where I could claim fixing PHY time
> stamping even according to your own expertise!
>
> > From the perspective of the mainline kernel, that can never happen.
>
> Yet in happened to me, and in some way because of the UAPI deficiencies
> I've mentioned, as ethtool has entirely separate code path, that happens
> to be correct for a long time already.
>

Yup, you are right:

int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
{
	const struct ethtool_ops *ops = dev->ethtool_ops;
	struct phy_device *phydev = dev->phydev;

	memset(info, 0, sizeof(*info));
	info->cmd = ETHTOOL_GET_TS_INFO;

	if (phy_has_tsinfo(phydev))
		return phy_ts_info(phydev, info);
	if (ops->get_ts_info)
		return ops->get_ts_info(dev, info);

	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
				SOF_TIMESTAMPING_SOFTWARE;
	info->phc_index = -1;

	return 0;
}

Very bad design choice indeed...
Given the fact that the PHY timestamping needs massaging from MAC driver
for plenty of other reasons, now of all things, ethtool just decided
it's not going to consult the MAC driver about the PHC it intends to
expose to user space, and just say "here's the PHY, deal with it". This
is a structural bug, I would say.

> > From your perspective as a developer, in your private work tree, where
> > _you_ added the necessary wiring for PHY timestamping, I fully
> > understand that this is exactly what happened _to_you_.
> > I am not saying that PHY timestamping doesn't need this issue fixed. It
> > does, and if it weren't for DSA, it would have simply been a "new
> > feature", and it would have been ok to have everything in the same
> > patch.
>
> Except that it's not a "new feature", but a bug-fix of an existing one,
> as I see it.
>

See above. It's clear that the intention of the PHY timestamping support
is for MAC drivers to opt-in, otherwise some mechanism would have been
devised such that not every single one of them would need to check for
phy_has_hwtstamp() in .ndo_do_ioctl(). That simply doesn't scale. Also,
it seems that automatically calling phy_ts_info from
__ethtool_get_ts_info is not coherent with that intention.

I need to think more about this. Anyway, if your aim is to "reduce
confusion" for others walking in your foot steps, I think this is much
worthier of your time: avoiding the inconsistent situation where the MAC
driver is obviously not ready for PHY timestamping, however not all
parts of the kernel are in agreement with that, and tell the user
something else.

Thanks,
-Vladimir

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Mon, Jul 13, 2020 at 01:32:09AM +0300, Sergey Organov wrote:

[...]

>> > From the perspective of the mainline kernel, that can never happen.
>>
>> Yet in happened to me, and in some way because of the UAPI deficiencies
>> I've mentioned, as ethtool has entirely separate code path, that happens
>> to be correct for a long time already.
>>
>
> Yup, you are right:
>

[...]

> Very bad design choice indeed...
> Given the fact that the PHY timestamping needs massaging from MAC driver
> for plenty of other reasons, now of all things, ethtool just decided
> it's not going to consult the MAC driver about the PHC it intends to
> expose to user space, and just say "here's the PHY, deal with it". This
> is a structural bug, I would say.
>
>> > From your perspective as a developer, in your private work tree, where
>> > _you_ added the necessary wiring for PHY timestamping, I fully
>> > understand that this is exactly what happened _to_you_.
>> > I am not saying that PHY timestamping doesn't need this issue fixed. It
>> > does, and if it weren't for DSA, it would have simply been a "new
>> > feature", and it would have been ok to have everything in the same
>> > patch.
>>
>> Except that it's not a "new feature", but a bug-fix of an existing one,
>> as I see it.
>>
>
> See above. It's clear that the intention of the PHY timestamping support
> is for MAC drivers to opt-in, otherwise some mechanism would have been
> devised such that not every single one of them would need to check for
> phy_has_hwtstamp() in .ndo_do_ioctl(). That simply doesn't scale. Also,
> it seems that automatically calling phy_ts_info from
> __ethtool_get_ts_info is not coherent with that intention.
>
> I need to think more about this. Anyway, if your aim is to "reduce
> confusion" for others walking in your foot steps, I think this is much
> worthier of your time: avoiding the inconsistent situation where the MAC
> driver is obviously not ready for PHY timestamping, however not all
> parts of the kernel are in agreement with that, and tell the user
> something else.

You see, I have a problem on kernel 4.9.146. After I apply this patch,
the problem goes away, at least for FEC/PHY combo that I care about, and
chances are high that for DSA as well, according to your own expertise.
Why should I care what is or is not ready for what to get a bug-fix
patch into the kernel? Why should I guess some vague "intentions" or
spend my time elsewhere?

Also please notice that if, as you suggest, I will propose only half of
the patch that will fix DSA only, then I will create confusion for
FEC/PHY users that will have no way to figure they need another part of
the fix to get their setup to work.

Could we please finally agree that, as what I suggest is indeed a simple
bug-fix, we could safely let it into the kernel?

Thanks,
-- Sergey

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Vladimir Oltean]

On Tue, Jul 14, 2020 at 03:39:04PM +0300, Sergey Organov wrote:
> Vladimir Oltean <olteanv@gmail.com> writes:
> 
> > On Mon, Jul 13, 2020 at 01:32:09AM +0300, Sergey Organov wrote:
> 
> [...]
> 
> >> > From the perspective of the mainline kernel, that can never happen.
> >>
> >> Yet in happened to me, and in some way because of the UAPI deficiencies
> >> I've mentioned, as ethtool has entirely separate code path, that happens
> >> to be correct for a long time already.
> >>
> >
> > Yup, you are right:
> >
> 
> [...]
> 
> > Very bad design choice indeed...
> > Given the fact that the PHY timestamping needs massaging from MAC driver
> > for plenty of other reasons, now of all things, ethtool just decided
> > it's not going to consult the MAC driver about the PHC it intends to
> > expose to user space, and just say "here's the PHY, deal with it". This
> > is a structural bug, I would say.
> >
> >> > From your perspective as a developer, in your private work tree, where
> >> > _you_ added the necessary wiring for PHY timestamping, I fully
> >> > understand that this is exactly what happened _to_you_.
> >> > I am not saying that PHY timestamping doesn't need this issue fixed. It
> >> > does, and if it weren't for DSA, it would have simply been a "new
> >> > feature", and it would have been ok to have everything in the same
> >> > patch.
> >>
> >> Except that it's not a "new feature", but a bug-fix of an existing one,
> >> as I see it.
> >>
> >
> > See above. It's clear that the intention of the PHY timestamping support
> > is for MAC drivers to opt-in, otherwise some mechanism would have been
> > devised such that not every single one of them would need to check for
> > phy_has_hwtstamp() in .ndo_do_ioctl(). That simply doesn't scale. Also,
> > it seems that automatically calling phy_ts_info from
> > __ethtool_get_ts_info is not coherent with that intention.
> >
> > I need to think more about this. Anyway, if your aim is to "reduce
> > confusion" for others walking in your foot steps, I think this is much
> > worthier of your time: avoiding the inconsistent situation where the MAC
> > driver is obviously not ready for PHY timestamping, however not all
> > parts of the kernel are in agreement with that, and tell the user
> > something else.
> 
> You see, I have a problem on kernel 4.9.146. After I apply this patch,
> the problem goes away, at least for FEC/PHY combo that I care about, and
> chances are high that for DSA as well, according to your own expertise.
> Why should I care what is or is not ready for what to get a bug-fix
> patch into the kernel? Why should I guess some vague "intentions" or
> spend my time elsewhere?
> 
> Also please notice that if, as you suggest, I will propose only half of
> the patch that will fix DSA only, then I will create confusion for
> FEC/PHY users that will have no way to figure they need another part of
> the fix to get their setup to work.
> 
> Could we please finally agree that, as what I suggest is indeed a simple
> bug-fix, we could safely let it into the kernel?
> 
> Thanks,
> -- Sergey

I cannot contradict you, you have all the arguments on your side. The
person who added support for "ethtool -T" in commit c8f3a8c31069
("ethtool: Introduce a method for getting time stamping capabilities.")
made a fundamental mistake in that they exposed broken functionality to
the user, in case CONFIG_NETWORK_PHY_TIMESTAMPING is enabled and the MAC
driver doesn't fulfill the requirements, be they skb_tx_timestamp(),
phy_has_hwtstamp() and what not. So, therefore, any patch that is adding
PHY timestamping compatibility in a MAC driver can rightfully claim that
it is fixing a bug, a sloppy design. Fair enough.

The only reason why I mentioned about spending your time on useful
things is because in your previous series you seemed to be concerned
about that. In retrospect, I believe you agree with me that your
confusion would have been significantly lower if the output of "ethtool
-T" was in harmony with the actual source of hardware timestamps.
Now that we discussed it through and I did see your point, I just
suggested what I believe to be the fundamental issue here, don't shoot
the messenger. Of course you are free to spend your time however you
want to.

Acked-by: Vladimir Oltean <olteanv@gmail.com>

Thanks,
-Vladimir

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Tue, Jul 14, 2020 at 03:39:04PM +0300, Sergey Organov wrote:
>> Vladimir Oltean <olteanv@gmail.com> writes:
>> 
>> > On Mon, Jul 13, 2020 at 01:32:09AM +0300, Sergey Organov wrote:
>> 
>> [...]
>> 
>> >> > From the perspective of the mainline kernel, that can never happen.
>> >>
>> >> Yet in happened to me, and in some way because of the UAPI
>> >> deficiencies
>> >> I've mentioned, as ethtool has entirely separate code path, that
>> >> happens
>> >> to be correct for a long time already.
>> >>
>> >
>> > Yup, you are right:
>> >
>> 
>> [...]
>> 
>> > Very bad design choice indeed...
>> > Given the fact that the PHY timestamping needs massaging from MAC
>> > driver
>> > for plenty of other reasons, now of all things, ethtool just decided
>> > it's not going to consult the MAC driver about the PHC it intends to
>> > expose to user space, and just say "here's the PHY, deal with it". This
>> > is a structural bug, I would say.
>> >
>> >> > From your perspective as a developer, in your private work
>> >> > tree, where
>> >> > _you_ added the necessary wiring for PHY timestamping, I fully
>> >> > understand that this is exactly what happened _to_you_.
>> >> > I am not saying that PHY timestamping doesn't need this issue
>> >> > fixed. It
>> >> > does, and if it weren't for DSA, it would have simply been a "new
>> >> > feature", and it would have been ok to have everything in the same
>> >> > patch.
>> >>
>> >> Except that it's not a "new feature", but a bug-fix of an
>> >> existing one,
>> >> as I see it.
>> >>
>> >
>> > See above. It's clear that the intention of the PHY timestamping
>> > support
>> > is for MAC drivers to opt-in, otherwise some mechanism would have been
>> > devised such that not every single one of them would need to check for
>> > phy_has_hwtstamp() in .ndo_do_ioctl(). That simply doesn't scale. Also,
>> > it seems that automatically calling phy_ts_info from
>> > __ethtool_get_ts_info is not coherent with that intention.
>> >
>> > I need to think more about this. Anyway, if your aim is to "reduce
>> > confusion" for others walking in your foot steps, I think this is much
>> > worthier of your time: avoiding the inconsistent situation where
>> > the MAC
>> > driver is obviously not ready for PHY timestamping, however not all
>> > parts of the kernel are in agreement with that, and tell the user
>> > something else.
>> 
>> You see, I have a problem on kernel 4.9.146. After I apply this patch,
>> the problem goes away, at least for FEC/PHY combo that I care about, and
>> chances are high that for DSA as well, according to your own expertise.
>> Why should I care what is or is not ready for what to get a bug-fix
>> patch into the kernel? Why should I guess some vague "intentions" or
>> spend my time elsewhere?
>> 
>> Also please notice that if, as you suggest, I will propose only half of
>> the patch that will fix DSA only, then I will create confusion for
>> FEC/PHY users that will have no way to figure they need another part of
>> the fix to get their setup to work.
>> 
>> Could we please finally agree that, as what I suggest is indeed a simple
>> bug-fix, we could safely let it into the kernel?
>> 
>> Thanks,
>> -- Sergey
>
> I cannot contradict you, you have all the arguments on your side. The
> person who added support for "ethtool -T" in commit c8f3a8c31069
> ("ethtool: Introduce a method for getting time stamping capabilities.")
> made a fundamental mistake in that they exposed broken functionality to
> the user, in case CONFIG_NETWORK_PHY_TIMESTAMPING is enabled and the MAC
> driver doesn't fulfill the requirements, be they skb_tx_timestamp(),
> phy_has_hwtstamp() and what not. So, therefore, any patch that is adding
> PHY timestamping compatibility in a MAC driver can rightfully claim that
> it is fixing a bug, a sloppy design. Fair enough.

OK, thanks!

>
> The only reason why I mentioned about spending your time on useful
> things is because in your previous series you seemed to be concerned
> about that. In retrospect, I believe you agree with me that your
> confusion would have been significantly lower if the output of "ethtool
> -T" was in harmony with the actual source of hardware timestamps.
> Now that we discussed it through and I did see your point, I just
> suggested what I believe to be the fundamental issue here, don't shoot
> the messenger. Of course you are free to spend your time however you
> want to.

I do care about these things indeed, it's only that right now what I
care most is to get the fixes into the kernel.

Then we can think without hurry about how all this could be improved.

> Acked-by: Vladimir Oltean <olteanv@gmail.com>

Thanks for reviewing, and again for helpful and beneficial discussion!

-- Sergey

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Vladimir Oltean]

On Tue, Jul 14, 2020 at 05:23:24PM +0300, Vladimir Oltean wrote:
> On Tue, Jul 14, 2020 at 03:39:04PM +0300, Sergey Organov wrote:
> > Vladimir Oltean <olteanv@gmail.com> writes:
> > 
> > > On Mon, Jul 13, 2020 at 01:32:09AM +0300, Sergey Organov wrote:
> > 
> > [...]
> > 
> > >> > From the perspective of the mainline kernel, that can never happen.
> > >>
> > >> Yet in happened to me, and in some way because of the UAPI deficiencies
> > >> I've mentioned, as ethtool has entirely separate code path, that happens
> > >> to be correct for a long time already.
> > >>
> > >
> > > Yup, you are right:
> > >
> > 
> > [...]
> > 
> > > Very bad design choice indeed...
> > > Given the fact that the PHY timestamping needs massaging from MAC driver
> > > for plenty of other reasons, now of all things, ethtool just decided
> > > it's not going to consult the MAC driver about the PHC it intends to
> > > expose to user space, and just say "here's the PHY, deal with it". This
> > > is a structural bug, I would say.
> > >
> > >> > From your perspective as a developer, in your private work tree, where
> > >> > _you_ added the necessary wiring for PHY timestamping, I fully
> > >> > understand that this is exactly what happened _to_you_.
> > >> > I am not saying that PHY timestamping doesn't need this issue fixed. It
> > >> > does, and if it weren't for DSA, it would have simply been a "new
> > >> > feature", and it would have been ok to have everything in the same
> > >> > patch.
> > >>
> > >> Except that it's not a "new feature", but a bug-fix of an existing one,
> > >> as I see it.
> > >>
> > >
> > > See above. It's clear that the intention of the PHY timestamping support
> > > is for MAC drivers to opt-in, otherwise some mechanism would have been
> > > devised such that not every single one of them would need to check for
> > > phy_has_hwtstamp() in .ndo_do_ioctl(). That simply doesn't scale. Also,
> > > it seems that automatically calling phy_ts_info from
> > > __ethtool_get_ts_info is not coherent with that intention.
> > >
> > > I need to think more about this. Anyway, if your aim is to "reduce
> > > confusion" for others walking in your foot steps, I think this is much
> > > worthier of your time: avoiding the inconsistent situation where the MAC
> > > driver is obviously not ready for PHY timestamping, however not all
> > > parts of the kernel are in agreement with that, and tell the user
> > > something else.
> > 
> > You see, I have a problem on kernel 4.9.146. After I apply this patch,
> > the problem goes away, at least for FEC/PHY combo that I care about, and
> > chances are high that for DSA as well, according to your own expertise.
> > Why should I care what is or is not ready for what to get a bug-fix
> > patch into the kernel? Why should I guess some vague "intentions" or
> > spend my time elsewhere?
> > 
> > Also please notice that if, as you suggest, I will propose only half of
> > the patch that will fix DSA only, then I will create confusion for
> > FEC/PHY users that will have no way to figure they need another part of
> > the fix to get their setup to work.
> > 
> > Could we please finally agree that, as what I suggest is indeed a simple
> > bug-fix, we could safely let it into the kernel?
> > 
> > Thanks,
> > -- Sergey
> 
> I cannot contradict you, you have all the arguments on your side. The
> person who added support for "ethtool -T" in commit c8f3a8c31069
> ("ethtool: Introduce a method for getting time stamping capabilities.")
> made a fundamental mistake in that they exposed broken functionality to
> the user, in case CONFIG_NETWORK_PHY_TIMESTAMPING is enabled and the MAC
> driver doesn't fulfill the requirements, be they skb_tx_timestamp(),
> phy_has_hwtstamp() and what not. So, therefore, any patch that is adding
> PHY timestamping compatibility in a MAC driver can rightfully claim that
> it is fixing a bug, a sloppy design. Fair enough.
> 
> The only reason why I mentioned about spending your time on useful
> things is because in your previous series you seemed to be concerned
> about that. In retrospect, I believe you agree with me that your
> confusion would have been significantly lower if the output of "ethtool
> -T" was in harmony with the actual source of hardware timestamps.
> Now that we discussed it through and I did see your point, I just
> suggested what I believe to be the fundamental issue here, don't shoot
> the messenger. Of course you are free to spend your time however you
> want to.
> 
> Acked-by: Vladimir Oltean <olteanv@gmail.com>
> 
> Thanks,
> -Vladimir

Of course, it would be good if you sent a new version with the sha1sum
of the Fixes: tag having the right length, otherwise people will
complain.

Thanks,
-Vladimir

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Vladimir Oltean <olteanv@gmail.com> writes:


[...]

>> Acked-by: Vladimir Oltean <olteanv@gmail.com>
>> 
>> Thanks,
>> -Vladimir
>
> Of course, it would be good if you sent a new version with the sha1sum
> of the Fixes: tag having the right length, otherwise people will
> complain.

Ah, thanks for reminding! I entirely forgot about it due to this lengthy
discussion. Will do!

Thanks,
-- Sergey

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Richard Cochran]

On Sat, Jul 11, 2020 at 03:08:42PM +0300, Sergey Organov wrote:
> Fix support for external PTP-aware devices such as DSA or PTP PHY:
> 
> Make sure we never time stamp tx packets when hardware time stamping
> is disabled.
> 
> Check for PTP PHY being in use and then pass ioctls related to time
> stamping of Ethernet packets to the PTP PHY rather than handle them
> ourselves. In addition, disable our own hardware time stamping in this
> case.
> 
> Fixes: 6605b73 ("FEC: Add time stamping code and a PTP hardware clock")
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> 
> v2:
>   - Extracted from larger patch series
>   - Description/comments updated according to discussions
>   - Added Fixes: tag

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Richard Cochran <richardcochran@gmail.com> writes:

> On Sat, Jul 11, 2020 at 03:08:42PM +0300, Sergey Organov wrote:
>> Fix support for external PTP-aware devices such as DSA or PTP PHY:
>> 
>> Make sure we never time stamp tx packets when hardware time stamping
>> is disabled.
>> 
>> Check for PTP PHY being in use and then pass ioctls related to time
>> stamping of Ethernet packets to the PTP PHY rather than handle them
>> ourselves. In addition, disable our own hardware time stamping in this
>> case.
>> 
>> Fixes: 6605b73 ("FEC: Add time stamping code and a PTP hardware clock")
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> 
>> v2:
>>   - Extracted from larger patch series
>>   - Description/comments updated according to discussions
>>   - Added Fixes: tag
>
> Acked-by: Richard Cochran <richardcochran@gmail.com>

Thanks for reviewing!

-- Sergey

[PATCH v3 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Fix support for external PTP-aware devices such as DSA or PTP PHY:

Make sure we never time stamp tx packets when hardware time stamping
is disabled.

Check for PTP PHY being in use and then pass ioctls related to time
stamping of Ethernet packets to the PTP PHY rather than handle them
ourselves. In addition, disable our own hardware time stamping in this
case.

Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware clock")
Signed-off-by: Sergey Organov <sorganov@gmail.com>
Acked-by: Richard Cochran <richardcochran@gmail.com>
Acked-by: Vladimir Oltean <olteanv@gmail.com>
---

v3:
  - Fixed SHA1 length of Fixes: tag
  - Added Acked-by: tags

v2:
  - Extracted from larger patch series
  - Description/comments updated according to discussions
  - Added Fixes: tag

 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++------
 drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index d8d76da..832a217 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -590,6 +590,7 @@ struct fec_enet_private {
 void fec_ptp_init(struct platform_device *pdev, int irq_idx);
 void fec_ptp_stop(struct platform_device *pdev);
 void fec_ptp_start_cyclecounter(struct net_device *ndev);
+void fec_ptp_disable_hwts(struct net_device *ndev);
 int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
 int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 3982285..cc7fbfc 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1294,8 +1294,13 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
 			ndev->stats.tx_bytes += skb->len;
 		}
 
-		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
-			fep->bufdesc_ex) {
+		/* NOTE: SKBTX_IN_PROGRESS being set does not imply it's we who
+		 * are to time stamp the packet, so we still need to check time
+		 * stamping enabled flag.
+		 */
+		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
+			     fep->hwts_tx_en) &&
+		    fep->bufdesc_ex) {
 			struct skb_shared_hwtstamps shhwtstamps;
 			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
 
@@ -2723,10 +2728,16 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
 		return -ENODEV;
 
 	if (fep->bufdesc_ex) {
-		if (cmd == SIOCSHWTSTAMP)
-			return fec_ptp_set(ndev, rq);
-		if (cmd == SIOCGHWTSTAMP)
-			return fec_ptp_get(ndev, rq);
+		bool use_fec_hwts = !phy_has_hwtstamp(phydev);
+
+		if (cmd == SIOCSHWTSTAMP) {
+			if (use_fec_hwts)
+				return fec_ptp_set(ndev, rq);
+			fec_ptp_disable_hwts(ndev);
+		} else if (cmd == SIOCGHWTSTAMP) {
+			if (use_fec_hwts)
+				return fec_ptp_get(ndev, rq);
+		}
 	}
 
 	return phy_mii_ioctl(phydev, rq, cmd);
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 945643c..f8a592c 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -452,6 +452,18 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
 	return -EOPNOTSUPP;
 }
 
+/**
+ * fec_ptp_disable_hwts - disable hardware time stamping
+ * @ndev: pointer to net_device
+ */
+void fec_ptp_disable_hwts(struct net_device *ndev)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	fep->hwts_tx_en = 0;
+	fep->hwts_rx_en = 0;
+}
+
 int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
-- 
2.10.0.1.g57b01a3

Re: [PATCH v3 net] net: fec: fix hardware time stamping by external devices

[Jakub Kicinski]

On Tue, 14 Jul 2020 19:28:02 +0300 Sergey Organov wrote:
> Fix support for external PTP-aware devices such as DSA or PTP PHY:
> 
> Make sure we never time stamp tx packets when hardware time stamping
> is disabled.
> 
> Check for PTP PHY being in use and then pass ioctls related to time
> stamping of Ethernet packets to the PTP PHY rather than handle them
> ourselves. In addition, disable our own hardware time stamping in this
> case.
> 
> Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware clock")
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> Acked-by: Richard Cochran <richardcochran@gmail.com>
> Acked-by: Vladimir Oltean <olteanv@gmail.com>
> ---
> 
> v3:
>   - Fixed SHA1 length of Fixes: tag
>   - Added Acked-by: tags
> 
> v2:
>   - Extracted from larger patch series
>   - Description/comments updated according to discussions
>   - Added Fixes: tag

FWIW in the networking subsystem we like the changelog to be part of the
commit.

Applied, and added to the stable queue, thanks!

Re: [PATCH v3 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 14 Jul 2020 19:28:02 +0300 Sergey Organov wrote:
>> Fix support for external PTP-aware devices such as DSA or PTP PHY:
>>
>> Make sure we never time stamp tx packets when hardware time stamping
>> is disabled.
>>
>> Check for PTP PHY being in use and then pass ioctls related to time
>> stamping of Ethernet packets to the PTP PHY rather than handle them
>> ourselves. In addition, disable our own hardware time stamping in this
>> case.
>>
>> Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware
>> clock")
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> Acked-by: Richard Cochran <richardcochran@gmail.com>
>> Acked-by: Vladimir Oltean <olteanv@gmail.com>
>> ---
>>
>> v3:
>>   - Fixed SHA1 length of Fixes: tag
>>   - Added Acked-by: tags
>>
>> v2:
>>   - Extracted from larger patch series
>>   - Description/comments updated according to discussions
>>   - Added Fixes: tag
>
> FWIW in the networking subsystem we like the changelog to be part of the
> commit.

Thanks, Jakub, I took a notice for myself!

>
> Applied, and added to the stable queue, thanks!

Thanks, and I've also got a no-brainer patch that lets this bug fix
compile as-is with older kernels, where there were no phy_has_hwtstamp()
function. Dunno how to properly handle this. Here is the patch (on
top of v4.9.146), just in case:

--- >8 ---

commit eee1f92bbc83ad59c83935a21f635e088cf7aa02
Author: Sergey Organov <sorganov@gmail.com>
Date:   Tue Jun 30 17:12:16 2020 +0300

    phy: add phy_has_hwtstamp() for compatibility with newer kernels

    Signed-off-by: Sergey Organov <sorganov@gmail.com>

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 867110c9d707..aa01ed4e8e1f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -595,6 +595,15 @@ struct phy_driver {
 #define PHY_ANY_ID "MATCH ANY PHY"
 #define PHY_ANY_UID 0xffffffff
 
+/**
+ * phy_has_hwtstamp - Tests whether a PHY supports time stamp configuration.
+ * @phydev: the phy_device struct
+ */
+static inline bool phy_has_hwtstamp(struct phy_device *phydev)
+{
+       return phydev && phydev->drv && phydev->drv->hwtstamp;
+}
+
 /* A Structure for boards to register fixups with the PHY Lib */
 struct phy_fixup {
        struct list_head list;


-- Sergey

Re: [PATCH v3 net] net: fec: fix hardware time stamping by external devices

[Jakub Kicinski]

On Thu, 16 Jul 2020 23:38:13 +0300 Sergey Organov wrote:
> > Applied, and added to the stable queue, thanks!  
> 
> Thanks, and I've also got a no-brainer patch that lets this bug fix
> compile as-is with older kernels, where there were no phy_has_hwtstamp()
> function. Dunno how to properly handle this. Here is the patch (on
> top of v4.9.146), just in case:

I see, I'll only add it to 5.7. By default we backport net fixes to
the two most recent releases, anyway. Could you send a patch that will 
work on 4.4 or 4.9 - 5.4 to Greg yourself once this hits Linus's tree
in a week or two?

Re: [PATCH v3 net] net: fec: fix hardware time stamping by external devices

[Sergey Organov]

Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 16 Jul 2020 23:38:13 +0300 Sergey Organov wrote:
>> > Applied, and added to the stable queue, thanks!  
>> 
>> Thanks, and I've also got a no-brainer patch that lets this bug fix
>> compile as-is with older kernels, where there were no phy_has_hwtstamp()
>> function. Dunno how to properly handle this. Here is the patch (on
>> top of v4.9.146), just in case:
>
> I see, I'll only add it to 5.7. By default we backport net fixes to
> the two most recent releases, anyway. Could you send a patch that will 
> work on 4.4 or 4.9 - 5.4 to Greg yourself once this hits Linus's tree
> in a week or two?

Sure. Hopefully I get it right that I'll need to send it to Greg as a
backport of this one to older kernel trees.

Thanks,
-- Sergey

[PATCH net-next v2 0/4] net: fec: a few improvements

[Sergey Organov]

This is a collection of simple improvements that reduce and/or
simplify code. They got developed out of attempt to use DP83640 PTP
PHY connected to built-in FEC (that has its own PTP support) of the
iMX 6SX micro-controller. The primary bug-fix was now submitted
separately, and this is the rest of the changes.

NOTE: the patches are developed and tested on 4.9.146, and rebased on
top of recent 'net-next/master', where, besides visual inspection, I
only tested that they do compile.

Sergey Organov (4):
  net: fec: enable to use PPS feature without time stamping
  net: fec: initialize clock with 0 rather than current kernel time
  net: fec: get rid of redundant code in fec_ptp_set()
  net: fec: replace snprintf() with strlcpy() in fec_ptp_init()

v2:
  - bug-fix patch from original series submitted separately
  - added Acked-by: where applicable
  
 drivers/net/ethernet/freescale/fec_ptp.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

-- 
2.10.0.1.g57b01a3

[PATCH net-next v2 1/4] net: fec: enable to use PPS feature without time stamping

[Sergey Organov]

PPS feature could be useful even when hardware time stamping
of network packets is not in use, so remove offending check
for this condition from fec_ptp_enable_pps().

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 945643c02615..fda306b3e21f 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -103,11 +103,6 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
 	u64 ns;
 	val = 0;
 
-	if (!(fep->hwts_tx_en || fep->hwts_rx_en)) {
-		dev_err(&fep->pdev->dev, "No ptp stack is running\n");
-		return -EINVAL;
-	}
-
 	if (fep->pps_enable == enable)
 		return 0;
 
-- 
2.10.0.1.g57b01a3

[PATCH net-next v2 2/4] net: fec: initialize clock with 0 rather than current kernel time

[Sergey Organov]

Initializing with 0 makes it much easier to identify time stamps from
otherwise uninitialized clock.

Initialization of PTP clock with current kernel time makes little sense as
PTP time scale differs from UTC time scale that kernel time represents.
It only leads to confusion when no actual PTP initialization happens, as
these time scales differ in a small integer number of seconds (37 at the
time of writing.)

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index fda306b3e21f..4bec7e66a39b 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -264,7 +264,7 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
 	fep->cc.mult = FEC_CC_MULT;
 
 	/* reset the ns time counter */
-	timecounter_init(&fep->tc, &fep->cc, ktime_to_ns(ktime_get_real()));
+	timecounter_init(&fep->tc, &fep->cc, 0);
 
 	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 }
-- 
2.10.0.1.g57b01a3

[PATCH net-next v2 3/4] net: fec: get rid of redundant code in fec_ptp_set()

[Sergey Organov]

Code of the form "if(x) x = 0" replaced with "x = 0".

Code of the form "if(x == a) x = a" removed.

Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 4bec7e66a39b..0c8c56bdd7ac 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -473,9 +473,7 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
 
 	switch (config.rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
-		if (fep->hwts_rx_en)
-			fep->hwts_rx_en = 0;
-		config.rx_filter = HWTSTAMP_FILTER_NONE;
+		fep->hwts_rx_en = 0;
 		break;
 
 	default:
-- 
2.10.0.1.g57b01a3

[PATCH net-next v2 4/4] net: fec: replace snprintf() with strlcpy() in fec_ptp_init()

[Sergey Organov]

No need to use snprintf() on a constant string, nor using magic
constant in the fixed code was a good idea.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 0c8c56bdd7ac..93a86553147c 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -570,7 +570,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 	int ret;
 
 	fep->ptp_caps.owner = THIS_MODULE;
-	snprintf(fep->ptp_caps.name, 16, "fec ptp");
+	strlcpy(fep->ptp_caps.name, "fec ptp", sizeof(fep->ptp_caps.name));
 
 	fep->ptp_caps.max_adj = 250000000;
 	fep->ptp_caps.n_alarm = 0;
-- 
2.10.0.1.g57b01a3

RE: [EXT] [PATCH net-next v2 0/4] net: fec: a few improvements

[Andy Duan]

From: Sergey Organov <sorganov@gmail.com> Sent: Wednesday, July 15, 2020 11:43 PM
> This is a collection of simple improvements that reduce and/or simplify code.
> They got developed out of attempt to use DP83640 PTP PHY connected to
> built-in FEC (that has its own PTP support) of the iMX 6SX micro-controller.
> The primary bug-fix was now submitted separately, and this is the rest of the
> changes.
> 
> NOTE: the patches are developed and tested on 4.9.146, and rebased on top
> of recent 'net-next/master', where, besides visual inspection, I only tested
> that they do compile.
> 
> Sergey Organov (4):
>   net: fec: enable to use PPS feature without time stamping
>   net: fec: initialize clock with 0 rather than current kernel time
>   net: fec: get rid of redundant code in fec_ptp_set()
>   net: fec: replace snprintf() with strlcpy() in fec_ptp_init()
> 
> v2:
>   - bug-fix patch from original series submitted separately
>   - added Acked-by: where applicable
> 
>  drivers/net/ethernet/freescale/fec_ptp.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 

For the version: Acked-by: Fugang Duan <fugang.duan@nxp.com>
Thanks!

Re: [EXT] [PATCH net-next v2 0/4] net: fec: a few improvements

[Jakub Kicinski]

On Thu, 16 Jul 2020 03:00:56 +0000 Andy Duan wrote:
> From: Sergey Organov <sorganov@gmail.com> Sent: Wednesday, July 15, 2020 11:43 PM
> > This is a collection of simple improvements that reduce and/or simplify code.
> > They got developed out of attempt to use DP83640 PTP PHY connected to
> > built-in FEC (that has its own PTP support) of the iMX 6SX micro-controller.
> > The primary bug-fix was now submitted separately, and this is the rest of the
> > changes.
> > 
> > NOTE: the patches are developed and tested on 4.9.146, and rebased on top
> > of recent 'net-next/master', where, besides visual inspection, I only tested
> > that they do compile.
> > 
> > Sergey Organov (4):
> >   net: fec: enable to use PPS feature without time stamping
> >   net: fec: initialize clock with 0 rather than current kernel time
> >   net: fec: get rid of redundant code in fec_ptp_set()
> >   net: fec: replace snprintf() with strlcpy() in fec_ptp_init()

Applied, thanks!

> For the version: Acked-by: Fugang Duan <fugang.duan@nxp.com>

Thanks! In the future please make sure to have the tag as a separate
line, patchwork is not clever enough to pick it up if it doesn't start
at the start of the line :(