From: Clay McClure <clay@daemons.net>
To: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
Richard Cochran <richardcochran@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
Sekhar Nori <nsekhar@ti.com>, Networking <netdev@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
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: <c04458ed-29ee-1797-3a11-7f3f560553e6@ti.com>
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
prev parent 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 [PATCH] net: cpts: Condition WARN_ON on PTP_1588_CLOCK 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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* 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 \
--to=clay@daemons.net \
--cc=arnd@arndb.de \
--cc=davem@davemloft.net \
--cc=grygorii.strashko@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nsekhar@ti.com \
--cc=richardcochran@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).