[PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Heiko Thiery]

When accessing the timecounter register on an i.MX8MQ the kernel hangs.
This is only the case when the interface is down. This can be reproduced
by reading with 'phc_ctrl eth0 get'.

Like described in the change in 91c0d987a9788dcc5fe26baafd73bf9242b68900
the igp clock is disabled when the interface is down and leads to a
system hang.

So we check if the ptp clock status before reading the timecounter
register.

Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
---
v2:
 - add mutex (thanks to Richard)

v3:
I did a mistake and did not test properly
 - add parenteses
 - fix the used variable

 drivers/net/ethernet/freescale/fec_ptp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 2e344aada4c6..1753807cbf97 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -377,9 +377,16 @@ static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 	u64 ns;
 	unsigned long flags;
 
+	mutex_lock(&adapter->ptp_clk_mutex);
+	/* Check the ptp clock */
+	if (!adapter->ptp_clk_on) {
+		mutex_unlock(&adapter->ptp_clk_mutex);
+		return -EINVAL;
+	}
 	spin_lock_irqsave(&adapter->tmreg_lock, flags);
 	ns = timecounter_read(&adapter->tc);
 	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
+	mutex_unlock(&adapter->ptp_clk_mutex);
 
 	*ts = ns_to_timespec64(ns);
 
-- 
2.30.0

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Heiko Thiery]

Hi all,

Am Do., 25. Feb. 2021 um 22:15 Uhr schrieb Heiko Thiery
<heiko.thiery@gmail.com>:
>
> When accessing the timecounter register on an i.MX8MQ the kernel hangs.
> This is only the case when the interface is down. This can be reproduced
> by reading with 'phc_ctrl eth0 get'.
>
> Like described in the change in 91c0d987a9788dcc5fe26baafd73bf9242b68900
> the igp clock is disabled when the interface is down and leads to a
> system hang.
>
> So we check if the ptp clock status before reading the timecounter
> register.
>
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>


Sorry for the noise. But just realized that I sent a v3 version of the
patch but forgot to update the subject line (still v2). Should I
resend it with the correct subject?

> ---
> v2:
>  - add mutex (thanks to Richard)
>
> v3:
> I did a mistake and did not test properly
>  - add parenteses
>  - fix the used variable
>
>  drivers/net/ethernet/freescale/fec_ptp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 2e344aada4c6..1753807cbf97 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -377,9 +377,16 @@ static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
>         u64 ns;
>         unsigned long flags;
>
> +       mutex_lock(&adapter->ptp_clk_mutex);
> +       /* Check the ptp clock */
> +       if (!adapter->ptp_clk_on) {
> +               mutex_unlock(&adapter->ptp_clk_mutex);
> +               return -EINVAL;
> +       }
>         spin_lock_irqsave(&adapter->tmreg_lock, flags);
>         ns = timecounter_read(&adapter->tc);
>         spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
> +       mutex_unlock(&adapter->ptp_clk_mutex);
>
>         *ts = ns_to_timespec64(ns);
>
> --
> 2.30.0
>

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Thu, Feb 25, 2021 at 10:15:16PM +0100, Heiko Thiery wrote:
> When accessing the timecounter register on an i.MX8MQ the kernel hangs.
> This is only the case when the interface is down. This can be reproduced
> by reading with 'phc_ctrl eth0 get'.
> 
> Like described in the change in 91c0d987a9788dcc5fe26baafd73bf9242b68900
> the igp clock is disabled when the interface is down and leads to a
> system hang.
> 
> So we check if the ptp clock status before reading the timecounter
> register.
> 
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> ---
> v2:
>  - add mutex (thanks to Richard)
> 
> v3:
> I did a mistake and did not test properly
>  - add parenteses
>  - fix the used variable

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

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Jakub Kicinski]

On Fri, 26 Feb 2021 07:23:31 -0800 Richard Cochran wrote:
> On Thu, Feb 25, 2021 at 10:15:16PM +0100, Heiko Thiery wrote:
> > When accessing the timecounter register on an i.MX8MQ the kernel hangs.
> > This is only the case when the interface is down. This can be reproduced
> > by reading with 'phc_ctrl eth0 get'.
> > 
> > Like described in the change in 91c0d987a9788dcc5fe26baafd73bf9242b68900
> > the igp clock is disabled when the interface is down and leads to a
> > system hang.
> > 
> > So we check if the ptp clock status before reading the timecounter
> > register.
> > 
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
> > v2:
> >  - add mutex (thanks to Richard)
> > 
> > v3:
> > I did a mistake and did not test properly
> >  - add parenteses
> >  - fix the used variable  

On Fri, 26 Feb 2021 08:22:50 +0100 Heiko Thiery wrote:
> Sorry for the noise. But just realized that I sent a v3 version of the
> patch but forgot to update the subject line (still v2). Should I
> resend it with the correct subject?

No need, looks like patchwork caught the right version.

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

Applied, thanks!

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]

Hi Heiko,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[also build test ERROR on net-next/master ipvs/master linus/master v5.11 next-20210225]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heiko-Thiery/net-fec-ptp-avoid-register-access-when-ipg-clock-is-disabled/20210226-050218
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 6cf739131a15e4177e58a1b4f2bede9d5da78552
config: mips-randconfig-r013-20210225 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project a921aaf789912d981cbb2036bdc91ad7289e1523)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/a7258bb15d947bb808b6209012a56ae993ec6001
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heiko-Thiery/net-fec-ptp-avoid-register-access-when-ipg-clock-is-disabled/20210226-050218
        git checkout a7258bb15d947bb808b6209012a56ae993ec6001
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> drivers/net/ethernet/freescale/fec_ptp.c:380:14: error: use of undeclared identifier 'fep'
           mutex_lock(&fep->ptp_clk_mutex);
                       ^
   drivers/net/ethernet/freescale/fec_ptp.c:383:17: error: use of undeclared identifier 'fep'
                   mutex_unlock(&fep->ptp_clk_mutex);
                                 ^
>> drivers/net/ethernet/freescale/fec_ptp.c:384:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
                   return -EINVAL;
                   ^
   drivers/net/ethernet/freescale/fec_ptp.c:382:2: note: previous statement is here
           if (!adapter->ptp_clk_on)
           ^
   drivers/net/ethernet/freescale/fec_ptp.c:388:16: error: use of undeclared identifier 'fep'
           mutex_unlock(&fep->ptp_clk_mutex);
                         ^
   1 warning and 3 errors generated.


vim +/fep +380 drivers/net/ethernet/freescale/fec_ptp.c

   364	
   365	/**
   366	 * fec_ptp_gettime
   367	 * @ptp: the ptp clock structure
   368	 * @ts: timespec structure to hold the current time value
   369	 *
   370	 * read the timecounter and return the correct value on ns,
   371	 * after converting it into a struct timespec.
   372	 */
   373	static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
   374	{
   375		struct fec_enet_private *adapter =
   376		    container_of(ptp, struct fec_enet_private, ptp_caps);
   377		u64 ns;
   378		unsigned long flags;
   379	
 > 380		mutex_lock(&fep->ptp_clk_mutex);
   381		/* Check the ptp clock */
   382		if (!adapter->ptp_clk_on)
   383			mutex_unlock(&fep->ptp_clk_mutex);
 > 384			return -EINVAL;
   385		spin_lock_irqsave(&adapter->tmreg_lock, flags);
   386		ns = timecounter_read(&adapter->tc);
   387		spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
   388		mutex_unlock(&fep->ptp_clk_mutex);
   389	
   390		*ts = ns_to_timespec64(ns);
   391	
   392		return 0;
   393	}
   394	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30869 bytes --]

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3898 bytes --]

Hi Heiko,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[also build test ERROR on net-next/master ipvs/master linus/master v5.11 next-20210225]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heiko-Thiery/net-fec-ptp-avoid-register-access-when-ipg-clock-is-disabled/20210226-050218
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 6cf739131a15e4177e58a1b4f2bede9d5da78552
config: mips-randconfig-r013-20210225 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project a921aaf789912d981cbb2036bdc91ad7289e1523)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/a7258bb15d947bb808b6209012a56ae993ec6001
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heiko-Thiery/net-fec-ptp-avoid-register-access-when-ipg-clock-is-disabled/20210226-050218
        git checkout a7258bb15d947bb808b6209012a56ae993ec6001
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> drivers/net/ethernet/freescale/fec_ptp.c:380:14: error: use of undeclared identifier 'fep'
           mutex_lock(&fep->ptp_clk_mutex);
                       ^
   drivers/net/ethernet/freescale/fec_ptp.c:383:17: error: use of undeclared identifier 'fep'
                   mutex_unlock(&fep->ptp_clk_mutex);
                                 ^
>> drivers/net/ethernet/freescale/fec_ptp.c:384:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
                   return -EINVAL;
                   ^
   drivers/net/ethernet/freescale/fec_ptp.c:382:2: note: previous statement is here
           if (!adapter->ptp_clk_on)
           ^
   drivers/net/ethernet/freescale/fec_ptp.c:388:16: error: use of undeclared identifier 'fep'
           mutex_unlock(&fep->ptp_clk_mutex);
                         ^
   1 warning and 3 errors generated.


vim +/fep +380 drivers/net/ethernet/freescale/fec_ptp.c

   364	
   365	/**
   366	 * fec_ptp_gettime
   367	 * @ptp: the ptp clock structure
   368	 * @ts: timespec structure to hold the current time value
   369	 *
   370	 * read the timecounter and return the correct value on ns,
   371	 * after converting it into a struct timespec.
   372	 */
   373	static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
   374	{
   375		struct fec_enet_private *adapter =
   376		    container_of(ptp, struct fec_enet_private, ptp_caps);
   377		u64 ns;
   378		unsigned long flags;
   379	
 > 380		mutex_lock(&fep->ptp_clk_mutex);
   381		/* Check the ptp clock */
   382		if (!adapter->ptp_clk_on)
   383			mutex_unlock(&fep->ptp_clk_mutex);
 > 384			return -EINVAL;
   385		spin_lock_irqsave(&adapter->tmreg_lock, flags);
   386		ns = timecounter_read(&adapter->tc);
   387		spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
   388		mutex_unlock(&fep->ptp_clk_mutex);
   389	
   390		*ts = ns_to_timespec64(ns);
   391	
   392		return 0;
   393	}
   394	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30869 bytes --]

[PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Heiko Thiery]

When accessing the timecounter register on an i.MX8MQ the kernel hangs.
This is only the case when the interface is down. This can be reproduced
by reading with 'phc_ctrl eth0 get'.

Like described in the change in 91c0d987a9788dcc5fe26baafd73bf9242b68900
the igp clock is disabled when the interface is down and leads to a
system hang.

So we check if the ptp clock status before reading the timecounter
register.

Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
---
v2:
 - add mutex (thanks to Richard)

 drivers/net/ethernet/freescale/fec_ptp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 2e344aada4c6..22f5e800c2d7 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -377,9 +377,15 @@ static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 	u64 ns;
 	unsigned long flags;
 
+	mutex_lock(&fep->ptp_clk_mutex);
+	/* Check the ptp clock */
+	if (!adapter->ptp_clk_on)
+		mutex_unlock(&fep->ptp_clk_mutex);
+		return -EINVAL;
 	spin_lock_irqsave(&adapter->tmreg_lock, flags);
 	ns = timecounter_read(&adapter->tc);
 	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
+	mutex_unlock(&fep->ptp_clk_mutex);
 
 	*ts = ns_to_timespec64(ns);
 
-- 
2.30.0

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Data: Thursday, August 14, 2014 10:34 PM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Thu, Aug 14, 2014 at 09:26:45AM +0000, fugang.duan@freescale.com wrote:
>> When ethx disable, all clocks are disabled. For gettime(), your mean
>> that we check clock is on or not, If clock is disabled, return an error
>in .fec_ptp_gettime() ?
>
>Yes.
>
I will change it. Thanks! I will submit the next version.

Thanks for your review.

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Thu, Aug 14, 2014 at 09:26:45AM +0000, fugang.duan@freescale.com wrote:
> When ethx disable, all clocks are disabled. For gettime(), your mean that we check clock is on or not,
> If clock is disabled, return an error in .fec_ptp_gettime() ?

Yes.

> How did it in the delayed work ?

The work must run whenever the physical clock is runnning.

This is COMPLETELY INDEPENDENT from time stamping.

If your patch has anything with "fep->hwts_tx_en", then it is wrong.

> Thanks for your review, I am confused by your suggestion.

Just start and stop the work queue when you start and stop the
physical clock.

Is it clear now?

Thanks,
Richard

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Sent: Thursday, August 14, 2014 5:13 PM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Thu, Aug 14, 2014 at 09:02:07AM +0000, fugang.duan@freescale.com wrote:
>>
>> I know your means. Even if cancel the time_keep work, ptp timer still is
>running and continutious.
>> The time_keep work just to avoid timer overrun because FEC just support
>32bit counter.
>
>Right, so the time_keep must continue when the clock is running. It does
>not depend on whether time stamping is enabled.

So, your mean we need to start the time_keep early like the orignal method, on the contrary,
The patch like below is not reasonable:

@@ -289,9 +289,11 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
        switch (config.tx_type) {
        case HWTSTAMP_TX_OFF:
                fep->hwts_tx_en = 0;
+               cancel_delayed_work_sync(&fep->time_keep);
                break;
        case HWTSTAMP_TX_ON:
                fep->hwts_tx_en = 1;
+               schedule_delayed_work(&fep->time_keep, HZ);
                break;
        default:

>
>> And, you said "return an error", I don't know your mean.
>> In addition, when clock is disabled, FEC ptp counter don't run any more.
>
>You can return an error from your 'gettime' method.
>
In the driver:
Gettime()->fec_ptp_gettime()->timecounter_read()->fec_ptp_read()
Period delayed work: fec_time_keep()->timecounter_read()->fec_ptp_read()

When ethx disable, all clocks are disabled. For gettime(), your mean that we check clock is on or not,
If clock is disabled, return an error in .fec_ptp_gettime() ?
How did it in the delayed work ?

Thanks for your review, I am confused by your suggestion.

Thanks,
Andy

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Thu, Aug 14, 2014 at 09:02:07AM +0000, fugang.duan@freescale.com wrote:
> 
> I know your means. Even if cancel the time_keep work, ptp timer still is running and continutious.
> The time_keep work just to avoid timer overrun because FEC just support 32bit counter.

Right, so the time_keep must continue when the clock is running. It
does not depend on whether time stamping is enabled.

> And, you said "return an error", I don't know your mean.
> In addition, when clock is disabled, FEC ptp counter don't run any more.

You can return an error from your 'gettime' method.

Thanks,
Richard

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Sent: Thursday, August 14, 2014 4:43 PM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Thu, Aug 14, 2014 at 08:24:39AM +0000, fugang.duan@freescale.com wrote:
>> 1. HW ptp is one feature of FEC IP, when the ethx interface is closed,
>ptp slave cannot sync with master,
>>    So the ptp timer is not accurate, of couse there have discontinuities
>in ptp clock.
>
>No, it just becomes a free running clock.
>
>This is no different than the Linux system clock. It keeps running, even
>if ntpd is not.
>
>> 2. When the ethx net interface is closed, FEC IP clocks are disabled,
>access ptp counter register causes
>>    Hw hang.
>
>Well, then, you need to return an error, don't you?

I know your means. Even if cancel the time_keep work, ptp timer still is running and continutious.
The time_keep work just to avoid timer overrun because FEC just support 32bit counter.

fec_time_keep()
{
...
ns = timecounter_read(&fep->tc);
...
}

static cycle_t fec_ptp_read(const struct cyclecounter *cc)
{
...
        tempval = readl(fep->hwp + FEC_ATIME_CTRL);
        tempval |= FEC_T_CTRL_CAPTURE;
        writel(tempval, fep->hwp + FEC_ATIME_CTRL);

        return readl(fep->hwp + FEC_ATIME);
...
}

And, you said "return an error", I don't know your mean.
In addition, when clock is disabled, FEC ptp counter don't run any more.

>
>> 3. PTP is not separate HW IP, which is one hw feature of FEC IP, when
>the net interface is closed, user
>>    Call clock_gettime() is not reasonable.
>
>Yes, it is. See #1.
>
>Thanks,
>Richard

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Thu, Aug 14, 2014 at 08:24:39AM +0000, fugang.duan@freescale.com wrote:
> 1. HW ptp is one feature of FEC IP, when the ethx interface is closed, ptp slave cannot sync with master,
>    So the ptp timer is not accurate, of couse there have discontinuities in ptp clock.

No, it just becomes a free running clock.

This is no different than the Linux system clock. It keeps running,
even if ntpd is not.

> 2. When the ethx net interface is closed, FEC IP clocks are disabled, access ptp counter register causes
>    Hw hang.

Well, then, you need to return an error, don't you?

> 3. PTP is not separate HW IP, which is one hw feature of FEC IP, when the net interface is closed, user
>    Call clock_gettime() is not reasonable.

Yes, it is. See #1.

Thanks,
Richard

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Sent: Thursday, August 14, 2014 4:11 PM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Thu, Aug 14, 2014 at 01:28:28AM +0000, fugang.duan@freescale.com wrote:
>> If the ethx close, ptp stack cannot run normally, stack stop send sync
>> message with master,
>
>But this has no effect on whether transmit time stamps are enabled.
>
>The timer work must run whenever the (physical) clock is running.
>Otherwise you will have discontinuities in your PTP clock.
>The kernel must always behave correctly, regardless of what user space
>does.
>
>When the user calls clock_gettime(), the result must always be consistent.
>This has nothing to do with time stamping at all.
>
I have some points against your views:
1. HW ptp is one feature of FEC IP, when the ethx interface is closed, ptp slave cannot sync with master,
   So the ptp timer is not accurate, of couse there have discontinuities in ptp clock.
2. When the ethx net interface is closed, FEC IP clocks are disabled, access ptp counter register causes
   Hw hang.
3. PTP is not separate HW IP, which is one hw feature of FEC IP, when the net interface is closed, user
   Call clock_gettime() is not reasonable.

Thanks,
Andy

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Thu, Aug 14, 2014 at 01:28:28AM +0000, fugang.duan@freescale.com wrote:
> If the ethx close, ptp stack cannot run normally, stack stop send sync message with master,

But this has no effect on whether transmit time stamps are enabled.

The timer work must run whenever the (physical) clock is
running. Otherwise you will have discontinuities in your PTP clock.
The kernel must always behave correctly, regardless of what user space
does.

When the user calls clock_gettime(), the result must always be
consistent. This has nothing to do with time stamping at all.

Thanks,
Richard

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Data: Thursday, August 14, 2014 5:20 AM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Wed, Aug 13, 2014 at 12:55:05PM +0800, Fugang Duan wrote:
>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 66fe1f6..8befa2c 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -2198,6 +2198,8 @@ fec_enet_open(struct net_device *ndev)
>>  	napi_enable(&fep->napi);
>>  	phy_start(fep->phy_dev);
>>  	netif_start_queue(ndev);
>> +	if (fep->hwts_tx_en)
>> +		schedule_delayed_work(&fep->time_keep, HZ);
>>  	return 0;
>>  }
>>
>> @@ -2206,6 +2208,9 @@ fec_enet_close(struct net_device *ndev)  {
>>  	struct fec_enet_private *fep = netdev_priv(ndev);
>>
>> +	if (fep->hwts_tx_en)
>> +		cancel_delayed_work_sync(&fep->time_keep);
>
>You make the clock logic depend on whether time stamping is enabled or not.
>Why do you do that?
>
>The clock should keep ticking, even if time stamping is disabled.
>
If the ethx close, ptp stack cannot run normally, stack stop send sync message with master,
So we also cancel the delayed work.

In addition, ethx interface close also gate ipg clock, if there have register access hw will hang.
So it is necessary to cancel the delayed work.

>Thanks,
>Richard

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Wed, Aug 13, 2014 at 12:55:05PM +0800, Fugang Duan wrote:

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 66fe1f6..8befa2c 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2198,6 +2198,8 @@ fec_enet_open(struct net_device *ndev)
>  	napi_enable(&fep->napi);
>  	phy_start(fep->phy_dev);
>  	netif_start_queue(ndev);
> +	if (fep->hwts_tx_en)
> +		schedule_delayed_work(&fep->time_keep, HZ);
>  	return 0;
>  }
>  
> @@ -2206,6 +2208,9 @@ fec_enet_close(struct net_device *ndev)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  
> +	if (fep->hwts_tx_en)
> +		cancel_delayed_work_sync(&fep->time_keep);

You make the clock logic depend on whether time stamping is enabled or
not. Why do you do that?

The clock should keep ticking, even if time stamping is disabled.

Thanks,
Richard

[PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Fugang Duan]

The current kernel hang on i.MX6SX with rootfs mount from MMC.
The root cause is ptp rise up period timer to access enet register
even if ipg clock is disabled.

FEC ptp driver start one period timer to read 1588 counter register in the
ptp init function that is called after FEC driver is probed.

To save power, after FEC probe finish, FEC driver disable all clocks including
ipg clock that is needed for register access.

i.MX5x, i.MX6q/dl/sl FEC register access don't cause system hang when ipg clock
is disabled, just return zero value. But for i.MX6sx SOC, it cause system hang.

To avoid the issue, we need to check ptp stack running status. If ptp4l run up,
and then schdule one delayed work to read ptp counter register. If ptp4l stack
is down, cancel the period delayed work. For ethx interface open/close, fec
suspend/resume situations, schdule/cancel the delayed work.

Signed-off-by: Fugang Duan <B38611@freescale.com>
---
 drivers/net/ethernet/freescale/fec.h      |    2 +-
 drivers/net/ethernet/freescale/fec_main.c |   12 +++++++++++-
 drivers/net/ethernet/freescale/fec_ptp.c  |   15 +++++++--------
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index bd53caf..03fad35 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -334,7 +334,7 @@ struct fec_enet_private {
 	u32 cycle_speed;
 	int hwts_rx_en;
 	int hwts_tx_en;
-	struct timer_list time_keep;
+	struct delayed_work time_keep;
 	struct regulator *reg_phy;
 };
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 66fe1f6..8befa2c 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2198,6 +2198,8 @@ fec_enet_open(struct net_device *ndev)
 	napi_enable(&fep->napi);
 	phy_start(fep->phy_dev);
 	netif_start_queue(ndev);
+	if (fep->hwts_tx_en)
+		schedule_delayed_work(&fep->time_keep, HZ);
 	return 0;
 }
 
@@ -2206,6 +2208,9 @@ fec_enet_close(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
+	if (fep->hwts_tx_en)
+		cancel_delayed_work_sync(&fep->time_keep);
+
 	phy_stop(fep->phy_dev);
 
 	if (netif_device_present(ndev)) {
@@ -2682,10 +2687,11 @@ fec_drv_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
+	if (fep->hwts_tx_en)
+		cancel_delayed_work_sync(&fep->time_keep);
 	cancel_work_sync(&fep->tx_timeout_work);
 	unregister_netdev(ndev);
 	fec_enet_mii_remove(fep);
-	del_timer_sync(&fep->time_keep);
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);
 	if (fep->ptp_clock)
@@ -2703,6 +2709,8 @@ static int __maybe_unused fec_suspend(struct device *dev)
 
 	rtnl_lock();
 	if (netif_running(ndev)) {
+		if (fep->hwts_tx_en)
+			cancel_delayed_work_sync(&fep->time_keep);
 		phy_stop(fep->phy_dev);
 		napi_disable(&fep->napi);
 		netif_tx_lock_bh(ndev);
@@ -2746,6 +2754,8 @@ static int __maybe_unused fec_resume(struct device *dev)
 		netif_tx_unlock_bh(ndev);
 		napi_enable(&fep->napi);
 		phy_start(fep->phy_dev);
+		if (fep->hwts_tx_en)
+			schedule_delayed_work(&fep->time_keep, HZ);
 	}
 	rtnl_unlock();
 
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 82386b2..281f4f8 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -289,9 +289,11 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
 	switch (config.tx_type) {
 	case HWTSTAMP_TX_OFF:
 		fep->hwts_tx_en = 0;
+		cancel_delayed_work_sync(&fep->time_keep);
 		break;
 	case HWTSTAMP_TX_ON:
 		fep->hwts_tx_en = 1;
+		schedule_delayed_work(&fep->time_keep, HZ);
 		break;
 	default:
 		return -ERANGE;
@@ -338,9 +340,10 @@ int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr)
  * fec_time_keep - call timecounter_read every second to avoid timer overrun
  *                 because ENET just support 32bit counter, will timeout in 4s
  */
-static void fec_time_keep(unsigned long _data)
+static void fec_time_keep(struct work_struct *work)
 {
-	struct fec_enet_private *fep = (struct fec_enet_private *)_data;
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct fec_enet_private *fep = container_of(dwork, struct fec_enet_private, time_keep);
 	u64 ns;
 	unsigned long flags;
 
@@ -348,7 +351,7 @@ static void fec_time_keep(unsigned long _data)
 	ns = timecounter_read(&fep->tc);
 	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 
-	mod_timer(&fep->time_keep, jiffies + HZ);
+	schedule_delayed_work(&fep->time_keep, HZ);
 }
 
 /**
@@ -386,11 +389,7 @@ void fec_ptp_init(struct platform_device *pdev)
 
 	fec_ptp_start_cyclecounter(ndev);
 
-	init_timer(&fep->time_keep);
-	fep->time_keep.data = (unsigned long)fep;
-	fep->time_keep.function = fec_time_keep;
-	fep->time_keep.expires = jiffies + HZ;
-	add_timer(&fep->time_keep);
+	INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);
 
 	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
 	if (IS_ERR(fep->ptp_clock)) {
-- 
1.7.8