[PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Clay McClure]

CPTS_MOD merely implies PTP_1588_CLOCK; it is possible to build cpts
without PTP clock support. In that case, ptp_clock_register() returns
NULL and we should not WARN_ON(cpts->clock) when downing the interface.
The ptp_*() functions are stubbed without PTP_1588_CLOCK, so it's safe
to pass them a null pointer.

Signed-off-by: Clay McClure <clay@daemons.net>
---
 drivers/net/ethernet/ti/cpts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index fd214f8730a9..daf4505f4a70 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -646,7 +646,7 @@ EXPORT_SYMBOL_GPL(cpts_register);
 
 void cpts_unregister(struct cpts *cpts)
 {
-	if (WARN_ON(!cpts->clock))
+	if (IS_REACHABLE(PTP_1588_CLOCK) && WARN_ON(!cpts->clock))
 		return;
 
 	ptp_clock_unregister(cpts->clock);
-- 
2.20.1

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Grygorii Strashko]

On 16/04/2020 11:56, Clay McClure wrote:
> CPTS_MOD merely implies PTP_1588_CLOCK; it is possible to build cpts
> without PTP clock support. In that case, ptp_clock_register() returns
> NULL and we should not WARN_ON(cpts->clock) when downing the interface.
> The ptp_*() functions are stubbed without PTP_1588_CLOCK, so it's safe
> to pass them a null pointer.

Could you explain the purpose of the exercise (Enabling CPTS with PTP_1588_CLOCK disabled), pls?

> 
> Signed-off-by: Clay McClure <clay@daemons.net>
> ---
>   drivers/net/ethernet/ti/cpts.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
> index fd214f8730a9..daf4505f4a70 100644
> --- a/drivers/net/ethernet/ti/cpts.c
> +++ b/drivers/net/ethernet/ti/cpts.c
> @@ -646,7 +646,7 @@ EXPORT_SYMBOL_GPL(cpts_register);
>   
>   void cpts_unregister(struct cpts *cpts)
>   {
> -	if (WARN_ON(!cpts->clock))
> +	if (IS_REACHABLE(PTP_1588_CLOCK) && WARN_ON(!cpts->clock))
>   		return;
>   
>   	ptp_clock_unregister(cpts->clock);
> 

-- 
Best regards,
grygorii

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Clay McClure]

On Thu, Apr 16, 2020 at 02:11:45PM +0300, Grygorii Strashko wrote:

Grygorii,

> > CPTS_MOD merely implies PTP_1588_CLOCK; it is possible to build cpts
> > without PTP clock support. In that case, ptp_clock_register() returns
> > NULL and we should not WARN_ON(cpts->clock) when downing the interface.
> > The ptp_*() functions are stubbed without PTP_1588_CLOCK, so it's safe
> > to pass them a null pointer.
> 
> Could you explain the purpose of the exercise (Enabling CPTS with
> PTP_1588_CLOCK disabled), pls?

Hardware timestamping with a free-running PHC _almost_ works without
PTP_1588_CLOCK, but since PHC rollover is handled by the PTP kworker
in this driver the timestamps end up not being monotonic.

And of course the moment you want to syntonize/synchronize the PHC with
another clock (say, CLOCK_REALTIME), you'll need a PTP clock device. So
you're right, there's not much point in building CPTS_MOD without
PTP_1588_CLOCK.

Given that, I wonder why all the Ethernet drivers seem to just `imply`
PTP_1588_CLOCK, rather than `depends on` it?

In any case, I was surprised to get a warning during `ifdown` but not
during `ifup`. What do you think of this change, which prints an error
like this during `ifup` if PTP_1588_CLOCK is not enabled:

[    6.192707] 000: cpsw 4a100000.ethernet: error registering cpts device

--- 
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 10ad706dda53..70b15039cd37 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -462,8 +462,8 @@ int cpts_register(struct cpts *cpts)
        timecounter_init(&cpts->tc, &cpts->cc, ktime_get_real_ns());
 
        cpts->clock = ptp_clock_register(&cpts->info, cpts->dev);
-       if (IS_ERR(cpts->clock)) {
-               err = PTR_ERR(cpts->clock);
+       if (IS_ERR_OR_NULL(cpts->clock)) {
+               err = cpts->clock ? PTR_ERR(cpts->clock) : -EOPNOTSUPP;
                cpts->clock = NULL;
                goto err_ptp;
        }

-- 
Clay

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Arnd Bergmann]

On Mon, Apr 20, 2020 at 11:38 AM Clay McClure <clay@daemons.net> wrote:
> On Thu, Apr 16, 2020 at 02:11:45PM +0300, Grygorii Strashko wrote:
>
> > > CPTS_MOD merely implies PTP_1588_CLOCK; it is possible to build cpts
> > > without PTP clock support. In that case, ptp_clock_register() returns
> > > NULL and we should not WARN_ON(cpts->clock) when downing the interface.
> > > The ptp_*() functions are stubbed without PTP_1588_CLOCK, so it's safe
> > > to pass them a null pointer.
> >
> > Could you explain the purpose of the exercise (Enabling CPTS with
> > PTP_1588_CLOCK disabled), pls?
>
> Hardware timestamping with a free-running PHC _almost_ works without
> PTP_1588_CLOCK, but since PHC rollover is handled by the PTP kworker
> in this driver the timestamps end up not being monotonic.
>
> And of course the moment you want to syntonize/synchronize the PHC with
> another clock (say, CLOCK_REALTIME), you'll need a PTP clock device. So
> you're right, there's not much point in building CPTS_MOD without
> PTP_1588_CLOCK.
>
> Given that, I wonder why all the Ethernet drivers seem to just `imply`
> PTP_1588_CLOCK, rather than `depends on` it?

I suspect we should move all of them back. This was an early user
of 'imply', but the meaning of that keyword has now changed
in the latest Kconfig.

> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
> index 10ad706dda53..70b15039cd37 100644
> --- a/drivers/net/ethernet/ti/cpts.c
> +++ b/drivers/net/ethernet/ti/cpts.c
> @@ -462,8 +462,8 @@ int cpts_register(struct cpts *cpts)
>         timecounter_init(&cpts->tc, &cpts->cc, ktime_get_real_ns());
>
>         cpts->clock = ptp_clock_register(&cpts->info, cpts->dev);
> -       if (IS_ERR(cpts->clock)) {
> -               err = PTR_ERR(cpts->clock);
> +       if (IS_ERR_OR_NULL(cpts->clock)) {
> +               err = cpts->clock ? PTR_ERR(cpts->clock) : -EOPNOTSUPP;
>                 cpts->clock = NULL;
>                 goto err_ptp;

Something else is wrong if you need IS_ERR_OR_NULL(). Any
kernel interface should either return an negative error code when
something goes wrong, or should return NULL for all errors, but
not mix the two.

        Arnd

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Richard Cochran]

On Mon, Apr 20, 2020 at 04:38:32PM +0200, Arnd Bergmann wrote:
> 
> I suspect we should move all of them back. This was an early user
> of 'imply', but the meaning of that keyword has now changed
> in the latest Kconfig.

Can you please explain the justification for changing the meaning?

It was a big PITA for me to support this in the first place, and now
we are back to square one?

> Something else is wrong if you need IS_ERR_OR_NULL(). Any
> kernel interface should either return an negative error code when
> something goes wrong, or should return NULL for all errors, but
> not mix the two.

On the contrary, this is exactly what the whole "imply" thing
demanded.

d1cbfd771ce82 (Nicolas Pitre       2016-11-11 172) #if IS_REACHABLE(CONFIG_PTP_1588_CLOCK)
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 173) 
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 174) /**
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 175)  * ptp_clock_register() - register a PTP hardware clock driver
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 176)  *
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 177)  * @info:   Structure describing the new clock.
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 178)  * @parent: Pointer to the parent device of the new clock.
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 179)  *
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 180)  * Returns a valid pointer on success or PTR_ERR on failure.  If PHC
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 181)  * support is missing at the configuration level, this function
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 182)  * returns NULL, and drivers are expected to gracefully handle that
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 183)  * case separately.
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 184)  */
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 185) 
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 186) extern struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
d1cbfd771ce82 (Nicolas Pitre       2016-11-11 187) 					    struct device *parent);

Thanks,
Richard

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Arnd Bergmann]

On Mon, Apr 20, 2020 at 7:00 PM Richard Cochran
<richardcochran@gmail.com> wrote:
>
> On Mon, Apr 20, 2020 at 04:38:32PM +0200, Arnd Bergmann wrote:
> >
> > I suspect we should move all of them back. This was an early user
> > of 'imply', but the meaning of that keyword has now changed
> > in the latest Kconfig.
>
> Can you please explain the justification for changing the meaning?
>
> It was a big PITA for me to support this in the first place, and now
> we are back to square one?

I don't understand it either. Apparently it didn't always do what users
expected, though the new definition seems less useful to me, as it
only changes the default.

> > Something else is wrong if you need IS_ERR_OR_NULL(). Any
> > kernel interface should either return an negative error code when
> > something goes wrong, or should return NULL for all errors, but
> > not mix the two.
>
> On the contrary, this is exactly what the whole "imply" thing
> demanded.
>
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 172) #if IS_REACHABLE(CONFIG_PTP_1588_CLOCK)
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 173)
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 174) /**
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 175)  * ptp_clock_register() - register a PTP hardware clock driver
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 176)  *
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 177)  * @info:   Structure describing the new clock.
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 178)  * @parent: Pointer to the parent device of the new clock.
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 179)  *
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 180)  * Returns a valid pointer on success or PTR_ERR on failure.  If PHC
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 181)  * support is missing at the configuration level, this function
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 182)  * returns NULL, and drivers are expected to gracefully handle that
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 183)  * case separately.
> d1cbfd771ce82 (Nicolas Pitre       2016-11-11 184)  */

The key here is "gracefully". The second patch from Clay just turns NULL into
 -EOPNOTSUPP and treats the compile-time condition into a runtime error.
I don't see a point in allowing the driver to compile if it just always returns
an error. The two callers then go on to print a message for any error
and just keep going.

      Arnd

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Richard Cochran]

On Mon, Apr 20, 2020 at 08:57:05PM +0200, Arnd Bergmann wrote:
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 172) #if IS_REACHABLE(CONFIG_PTP_1588_CLOCK)
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 173)
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 174) /**
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 175)  * ptp_clock_register() - register a PTP hardware clock driver
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 176)  *
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 177)  * @info:   Structure describing the new clock.
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 178)  * @parent: Pointer to the parent device of the new clock.
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 179)  *
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 180)  * Returns a valid pointer on success or PTR_ERR on failure.  If PHC
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 181)  * support is missing at the configuration level, this function
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 182)  * returns NULL, and drivers are expected to gracefully handle that
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 183)  * case separately.
> > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 184)  */
> 
> The key here is "gracefully". The second patch from Clay just turns NULL into
>  -EOPNOTSUPP and treats the compile-time condition into a runtime error.

You are talking about the cpts driver, no?

I'm worried about ptp_clock_register(), because it does return NULL if
IS_REACHABLE(CONFIG_PTP_1588_CLOCK), and this is the "correct"
behavior ever since November 2016.

If somebody wants to change that stub to return EOPNOTSUPP, then fine,
but please have them audit the callers and submit a patch series.

Thanks,
Richard

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Arnd Bergmann]

On Mon, Apr 20, 2020 at 11:18 PM Richard Cochran
<richardcochran@gmail.com> wrote:
> On Mon, Apr 20, 2020 at 08:57:05PM +0200, Arnd Bergmann wrote:
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 172) #if IS_REACHABLE(CONFIG_PTP_1588_CLOCK)
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 173)
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 174) /**
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 175)  * ptp_clock_register() - register a PTP hardware clock driver
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 176)  *
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 177)  * @info:   Structure describing the new clock.
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 178)  * @parent: Pointer to the parent device of the new clock.
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 179)  *
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 180)  * Returns a valid pointer on success or PTR_ERR on failure.  If PHC
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 181)  * support is missing at the configuration level, this function
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 182)  * returns NULL, and drivers are expected to gracefully handle that
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 183)  * case separately.
> > > d1cbfd771ce82 (Nicolas Pitre       2016-11-11 184)  */
> >
> > The key here is "gracefully". The second patch from Clay just turns NULL into
> >  -EOPNOTSUPP and treats the compile-time condition into a runtime error.
>
> You are talking about the cpts driver, no?
>
> I'm worried about ptp_clock_register(), because it does return NULL if
> IS_REACHABLE(CONFIG_PTP_1588_CLOCK), and this is the "correct"
> behavior ever since November 2016.
>
> If somebody wants to change that stub to return EOPNOTSUPP, then fine,
> but please have them audit the callers and submit a patch series.

It's not great, but we have other interfaces like this that can return NULL for
success when the subsystem is disabled. The problem is when there is
a mismatch between the caller treating NULL as failure when it is meant to
be "successful lack of object returned".

       Arnd

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Richard Cochran]

On Mon, Apr 20, 2020 at 11:21:20PM +0200, Arnd Bergmann wrote:
> It's not great, but we have other interfaces like this that can return NULL for
> success when the subsystem is disabled. The problem is when there is
> a mismatch between the caller treating NULL as failure when it is meant to
> be "successful lack of object returned".

Yeah, that should be fixed.

To be clear, do you all see a need to change the stubbed version of
ptp_clock_register() or not?

Thanks,
Richard

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Arnd Bergmann]

On Mon, Apr 20, 2020 at 11:34 PM Richard Cochran
<richardcochran@gmail.com> wrote:
>
> On Mon, Apr 20, 2020 at 11:21:20PM +0200, Arnd Bergmann wrote:
> > It's not great, but we have other interfaces like this that can return NULL for
> > success when the subsystem is disabled. The problem is when there is
> > a mismatch between the caller treating NULL as failure when it is meant to
> > be "successful lack of object returned".
>
> Yeah, that should be fixed.
>
> To be clear, do you all see a need to change the stubbed version of
> ptp_clock_register() or not?

No, if the NULL return is only meant to mean "nothing wrong, keep going
wihtout an object", that's fine with me. It does occasionally confuse driver
writers (as seen here), so it's not a great interface, but there is no general
solution to make it better.

     Arnd

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Grygorii Strashko]

Hi

On 21/04/2020 00:42, Arnd Bergmann wrote:
> On Mon, Apr 20, 2020 at 11:34 PM Richard Cochran
> <richardcochran@gmail.com> wrote:
>>
>> On Mon, Apr 20, 2020 at 11:21:20PM +0200, Arnd Bergmann wrote:
>>> It's not great, but we have other interfaces like this that can return NULL for
>>> success when the subsystem is disabled. The problem is when there is
>>> a mismatch between the caller treating NULL as failure when it is meant to
>>> be "successful lack of object returned".
>>
>> Yeah, that should be fixed.
>>
>> To be clear, do you all see a need to change the stubbed version of
>> ptp_clock_register() or not?
> 
> No, if the NULL return is only meant to mean "nothing wrong, keep going
> wihtout an object", that's fine with me. It does occasionally confuse driver
> writers (as seen here), so it's not a great interface, but there is no general
> solution to make it better.

As per commit
commit d1cbfd771ce8297fa11e89f315392de6056a2181
Author: Nicolas Pitre <nicolas.pitre@linaro.org>
Date:   Fri Nov 11 00:10:07 2016 -0500

     ptp_clock: Allow for it to be optional
     
     In order to break the hard dependency between the PTP clock subsystem and
     ethernet drivers capable of being clock providers, this patch provides
     simple PTP stub functions to allow linkage of those drivers into the
     kernel even when the PTP subsystem is configured out. Drivers must be
     ready to accept NULL from ptp_clock_register() in that case.
     
     And to make it possible for PTP to be configured out, the select statement
     in those driver's Kconfig menu entries is converted to the new "imply"
     statement. This way the PTP subsystem may have Kconfig dependencies of
     its own, such as POSIX_TIMERS, without having to make those ethernet
     drivers unavailable if POSIX timers are cconfigured out. And when support
     for POSIX timers is selected again then the default config option for PTP
     clock support will automatically be adjusted accordingly.


the idea of using "imply" is to keep networking enabled even if PTP is configured out
and this exactly what happens with cpts driver.
Another question is that CPTS completely nonfunctional in this case and it was never
expected that somebody will even try to use/run such configuration (except for random build purposes).


-- 
Best regards,
grygorii

Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK

[Clay McClure]

On Wed, Apr 22, 2020 at 02:16:11PM +0300, Grygorii Strashko wrote:
> 
> On 21/04/2020 00:42, Arnd Bergmann wrote:
> >
> > On Mon, Apr 20, 2020 at 11:34 PM Richard Cochran
> > >
> > > To be clear, do you all see a need to change the stubbed version of
> > > ptp_clock_register() or not?
> > 
> > No, if the NULL return is only meant to mean "nothing wrong, keep going
> > wihtout an object", that's fine with me. It does occasionally confuse driver
> > writers (as seen here), so it's not a great interface, but there is no general
> > solution to make it better.

That's why in my first patch I condition the WARN_ON() on PTP_1588_CLOCK,
since without that the null pointer here is not an error:

	void cpts_unregister(struct cpts *cpts)                                         
	{                                                                               
		if (WARN_ON(!cpts->clock))                                              
			return;                                                         

Grygorii's question (paraphrasing: "why would you ever do that?")
prompted my second patch, making the broken configuration obvious by
emitting an error during `ifup`, instead of just a warning during
`ifdown`.

But I think Grygorii is on to something here:

> Another question is that CPTS completely nonfunctional in this case and
> it was never expected that somebody will even try to use/run such
> configuration (except for random build purposes).

So, let's not allow it. In my view, commit d1cbfd771ce8 ("ptp_clock:
Allow for it to be optional") went a bit too far, and converted even
clearly PTP-specific modules from `select` to `imply` PTP_1588_CLOCK,
which is what made this broken configuration possible. I suggest
reverting that change, just for the PTP-specific modules under
drivers/net/ethernet.

I audited all drivers that call `ptp_clock_register()`; it looks like
these should `select` (instead of merely `imply`) PTP_1588_CLOCK:

    NET_DSA_MV88E6XXX_PTP
    NET_DSA_SJA1105_PTP
    MACB_USE_HWSTAMP
    CAVIUM_PTP
    TI_CPTS_MOD
    PTP_1588_CLOCK_IXP46X

Note how they all reference PTP or timestamping in their name, which is
a clue that they depend on PTP_1588_CLOCK.

I have a patch for this, but first, a procedural question: does mailing
list etiquette dictate that I reply to this thread with the new patch,
or does it begin a new thread?

-- 
Clay