archive mirror
 help / color / mirror / Atom feed
From: Clay McClure <>
To: Grygorii Strashko <>
Cc: Arnd Bergmann <>,
	Richard Cochran <>,
	"David S. Miller" <>,
	Sekhar Nori <>, Networking <>,
	"" <>
Subject: Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK
Date: Sat, 25 Apr 2020 19:41:48 -0700	[thread overview]
Message-ID: <20200426024148.GA7518@arctic-shiba-lx> (raw)
In-Reply-To: <>

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))                                              

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

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

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


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?


      reply	other threads:[~2020-04-26  2:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-16  8:56 Clay McClure
2020-04-16 11:11 ` Grygorii Strashko
2020-04-20  9:36   ` Clay McClure
2020-04-20 14:38     ` Arnd Bergmann
2020-04-20 17:00       ` Richard Cochran
2020-04-20 18:57         ` Arnd Bergmann
2020-04-20 21:18           ` Richard Cochran
2020-04-20 21:21             ` Arnd Bergmann
2020-04-20 21:34               ` Richard Cochran
2020-04-20 21:42                 ` Arnd Bergmann
2020-04-22 11:16                   ` Grygorii Strashko
2020-04-26  2:41                     ` Clay McClure [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200426024148.GA7518@arctic-shiba-lx \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).