Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
@ 2021-02-20  6:56 Heiko Thiery
  2021-02-23  3:00 ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Heiko Thiery @ 2021-02-20  6:56 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Jakub Kicinski, David S . Miller, Fugang Duan, 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>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 3 +++
 1 file changed, 3 insertions(+)

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
  2021-02-20  6:56 [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled Heiko Thiery
@ 2021-02-23  3:00 ` Jakub Kicinski
  2021-02-23  8:00   ` Heiko Thiery
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2021-02-23  3:00 UTC (permalink / raw)
  To: Heiko Thiery
  Cc: linux-kernel, netdev, David S . Miller, Fugang Duan, Richard Cochran

On Sat, 20 Feb 2021 07:56:55 +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>

Please widen the CC list, you should CC Richard on PTP patches.

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 2e344aada4c6..c9882083da02 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -377,6 +377,9 @@ static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
>  	u64 ns;
>  	unsigned long flags;
>  
> +	/* Check the ptp clock */

Comment is rather redundant. Drop it or say _when_ ptp_clk_on may not
be true.

> +	if (!adapter->ptp_clk_on)
> +		return -EINVAL;

Why is the PTP interface registered when it can't be accessed?

Perhaps the driver should unregister the PTP clock when it's brought
down?

>  	spin_lock_irqsave(&adapter->tmreg_lock, flags);
>  	ns = timecounter_read(&adapter->tc);
>  	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
  2021-02-23  3:00 ` Jakub Kicinski
@ 2021-02-23  8:00   ` Heiko Thiery
  2021-02-23 14:27     ` Richard Cochran
  0 siblings, 1 reply; 9+ messages in thread
From: Heiko Thiery @ 2021-02-23  8:00 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: linux-kernel, netdev, David S . Miller, Fugang Duan, Richard Cochran

HI Jakub,

Am Di., 23. Feb. 2021 um 04:00 Uhr schrieb Jakub Kicinski <kuba@kernel.org>:
>
> On Sat, 20 Feb 2021 07:56:55 +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>
>
> Please widen the CC list, you should CC Richard on PTP patches.
>
> > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> > index 2e344aada4c6..c9882083da02 100644
> > --- a/drivers/net/ethernet/freescale/fec_ptp.c
> > +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> > @@ -377,6 +377,9 @@ static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
> >       u64 ns;
> >       unsigned long flags;
> >
> > +     /* Check the ptp clock */
>
> Comment is rather redundant. Drop it or say _when_ ptp_clk_on may not
> be true.

I just used the same comment as the one in the fec_ptp_settime() function.

>
> > +     if (!adapter->ptp_clk_on)
> > +             return -EINVAL;
>
> Why is the PTP interface registered when it can't be accessed?
>
> Perhaps the driver should unregister the PTP clock when it's brought
> down?

Good question, but I do not know what happens e.g. with linuxptp when
the device that was opened before will be gone.

Maybe Richard can give a hint.

>
> >       spin_lock_irqsave(&adapter->tmreg_lock, flags);
> >       ns = timecounter_read(&adapter->tc);
> >       spin_unlock_irqrestore(&adapter->tmreg_lock, flags);

-- 
Heiko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
  2021-02-23  8:00   ` Heiko Thiery
@ 2021-02-23 14:27     ` Richard Cochran
  2021-02-23 15:04       ` Heiko Thiery
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Cochran @ 2021-02-23 14:27 UTC (permalink / raw)
  To: Heiko Thiery
  Cc: Jakub Kicinski, linux-kernel, netdev, David S . Miller, Fugang Duan

On Tue, Feb 23, 2021 at 09:00:32AM +0100, Heiko Thiery wrote:
> HI Jakub,
> 
> Am Di., 23. Feb. 2021 um 04:00 Uhr schrieb Jakub Kicinski <kuba@kernel.org>:
> > Why is the PTP interface registered when it can't be accessed?
> >
> > Perhaps the driver should unregister the PTP clock when it's brought
> > down?

I don't see any reason why a clock should stop ticking just because
the interface is down.  This is a poor driver design, but sadly it
gets copied and even defended.

> Good question, but I do not know what happens e.g. with linuxptp when
> the device that was opened before will be gone.

If a network interface goes down, ptp4l will notice via rtnl and close
the interface.  Then it re-opens the sockets on rtnl up.  However, the
file descriptor representing the dynamic posix clock stays opened.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
  2021-02-23 14:27     ` Richard Cochran
@ 2021-02-23 15:04       ` Heiko Thiery
  2021-02-23 16:11         ` Richard Cochran
  0 siblings, 1 reply; 9+ messages in thread
From: Heiko Thiery @ 2021-02-23 15:04 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Jakub Kicinski, linux-kernel, netdev, David S . Miller, Fugang Duan

Hi Richard,

Am Di., 23. Feb. 2021 um 15:27 Uhr schrieb Richard Cochran
<richardcochran@gmail.com>:
>
> On Tue, Feb 23, 2021 at 09:00:32AM +0100, Heiko Thiery wrote:
> > HI Jakub,
> >
> > Am Di., 23. Feb. 2021 um 04:00 Uhr schrieb Jakub Kicinski <kuba@kernel.org>:
> > > Why is the PTP interface registered when it can't be accessed?
> > >
> > > Perhaps the driver should unregister the PTP clock when it's brought
> > > down?
>
> I don't see any reason why a clock should stop ticking just because
> the interface is down.  This is a poor driver design, but sadly it
> gets copied and even defended.

It is not only the PHC clock that stops. Rather, it is the entire
ethernet building block in the SOC that is disabled, including the
PHC.

> > Good question, but I do not know what happens e.g. with linuxptp when
> > the device that was opened before will be gone.
>
> If a network interface goes down, ptp4l will notice via rtnl and close
> the interface.  Then it re-opens the sockets on rtnl up.  However, the
> file descriptor representing the dynamic posix clock stays opened.

Thanks,

-- 
Heiko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
  2021-02-23 15:04       ` Heiko Thiery
@ 2021-02-23 16:11         ` Richard Cochran
  2021-02-25 13:49           ` Heiko Thiery
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Cochran @ 2021-02-23 16:11 UTC (permalink / raw)
  To: Heiko Thiery
  Cc: Jakub Kicinski, linux-kernel, netdev, David S . Miller, Fugang Duan

On Tue, Feb 23, 2021 at 04:04:16PM +0100, Heiko Thiery wrote:
> It is not only the PHC clock that stops. Rather, it is the entire
> ethernet building block in the SOC that is disabled, including the
> PHC.

Sure, but why does the driver do that?

Thanks,
Richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
  2021-02-23 16:11         ` Richard Cochran
@ 2021-02-25 13:49           ` Heiko Thiery
  2021-02-25 14:05             ` Heiko Thiery
  0 siblings, 1 reply; 9+ messages in thread
From: Heiko Thiery @ 2021-02-25 13:49 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Jakub Kicinski, linux-kernel, netdev, David S . Miller, Fugang Duan

Hi Richard,

Am Di., 23. Feb. 2021 um 17:11 Uhr schrieb Richard Cochran
<richardcochran@gmail.com>:
>
> On Tue, Feb 23, 2021 at 04:04:16PM +0100, Heiko Thiery wrote:
> > It is not only the PHC clock that stops. Rather, it is the entire
> > ethernet building block in the SOC that is disabled, including the
> > PHC.
>
> Sure, but why does the driver do that?

That is a good question. I tried to understand the clock
infrastructure of the imx8 but it looks quite complicated. I cannot
find the point where all the stuff is disabled.

-- 
Heiko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
  2021-02-25 13:49           ` Heiko Thiery
@ 2021-02-25 14:05             ` Heiko Thiery
  2021-02-25 17:14               ` Richard Cochran
  0 siblings, 1 reply; 9+ messages in thread
From: Heiko Thiery @ 2021-02-25 14:05 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Jakub Kicinski, linux-kernel, netdev, David S . Miller, Fugang Duan

Hi Richard,

Am Do., 25. Feb. 2021 um 14:49 Uhr schrieb Heiko Thiery
<heiko.thiery@gmail.com>:
>
> Hi Richard,
>
> Am Di., 23. Feb. 2021 um 17:11 Uhr schrieb Richard Cochran
> <richardcochran@gmail.com>:
> >
> > On Tue, Feb 23, 2021 at 04:04:16PM +0100, Heiko Thiery wrote:
> > > It is not only the PHC clock that stops. Rather, it is the entire
> > > ethernet building block in the SOC that is disabled, including the
> > > PHC.
> >
> > Sure, but why does the driver do that?
>
> That is a good question. I tried to understand the clock
> infrastructure of the imx8 but it looks quite complicated. I cannot
> find the point where all the stuff is disabled.

But the explanation why it is currently disabled that way can be found
in the commit 91c0d987a9788dcc5fe26baafd73bf9242b68900.

-- 
Heiko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
  2021-02-25 14:05             ` Heiko Thiery
@ 2021-02-25 17:14               ` Richard Cochran
  0 siblings, 0 replies; 9+ messages in thread
From: Richard Cochran @ 2021-02-25 17:14 UTC (permalink / raw)
  To: Heiko Thiery
  Cc: Jakub Kicinski, linux-kernel, netdev, David S . Miller, Fugang Duan

On Thu, Feb 25, 2021 at 03:05:32PM +0100, Heiko Thiery wrote:

> But the explanation why it is currently disabled that way can be found
> in the commit 91c0d987a9788dcc5fe26baafd73bf9242b68900.

Okay, without re-factoring the entire driver, I agree that the gettime
lock up aught to be fixed in a similar way.  I missed the original
patch, but the diff fragment in this thread doesn't appear to take the
mutex as it should.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-20  6:56 [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled Heiko Thiery
2021-02-23  3:00 ` Jakub Kicinski
2021-02-23  8:00   ` Heiko Thiery
2021-02-23 14:27     ` Richard Cochran
2021-02-23 15:04       ` Heiko Thiery
2021-02-23 16:11         ` Richard Cochran
2021-02-25 13:49           ` Heiko Thiery
2021-02-25 14:05             ` Heiko Thiery
2021-02-25 17:14               ` Richard Cochran

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git