Linux-Doc Archive on lore.kernel.org
 help / color / Atom feed
From: Sergey Organov <sorganov@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: kuba@kernel.org, davem@davemloft.net, netdev@vger.kernel.org,
	richardcochran@gmail.com, linux-doc@vger.kernel.org
Subject: Re: [PATCH net-next 0/3] Document more PTP timestamping known quirks
Date: Sat, 18 Jul 2020 13:54:11 +0300
Message-ID: <878sfh2iwc.fsf@osv.gnss.ru> (raw)
In-Reply-To: <20200717215719.nhuaak2xu4fwebqp@skbuf> (Vladimir Oltean's message of "Sat, 18 Jul 2020 00:57:19 +0300")

Vladimir Oltean <olteanv@gmail.com> writes:

> On Sat, Jul 18, 2020 at 12:13:42AM +0300, Sergey Organov wrote:
>> Vladimir Oltean <olteanv@gmail.com> writes:
>> 
>> > I've tried to collect and summarize the conclusions of these discussions:
>> > https://patchwork.ozlabs.org/project/netdev/patch/20200711120842.2631-1-sorganov@gmail.com/
>> > https://patchwork.ozlabs.org/project/netdev/patch/20200710113611.3398-5-kurt@linutronix.de/
>> > which were a bit surprising to me. Make sure they are present in the
>> > documentation.
>> 
>> As one of participants of these discussions, I'm afraid I incline to
>> alternative approach to solving the issues current design has than the one
>> you advocate in these patch series.
>> 
>> I believe its upper-level that should enforce common policies like
>> handling hw time stamping at outermost capable device, not random MAC
>> driver out there.
>> 
>> I'd argue that it's then upper-level that should check PHY features, and
>> then do not bother MAC with ioctl() requests that MAC should not handle
>> in given configuration. This way, the checks for phy_has_hwtstamp()
>> won't be spread over multiple MAC drivers and will happily sit in the
>> upper-level ioctl() handler.
>> 
>> In other words, I mean that it's approach taken in ethtool that I tend
>> to consider being the right one.
>> 
>> Thanks,
>> -- Sergey
>
> Concretely speaking, what are you going to do for
> skb_defer_tx_timestamp() and skb_defer_rx_timestamp()? Not to mention
> subtle bugs like SKBTX_IN_PROGRESS.

I think that we have at least 2 problems here, and what I argue about
above addresses one of them, while you try to get solution for another
one.

> If you don't address those, it's pointless to move the
> phy_has_hwtstamp() check to net/core/dev_ioctl.c.

No, even though solving one problem could be considered pointless
without solving another, it doesn't mean that solving it is pointless. I
do hope you will solve another one.

I believe that logic in ethtool ioctl handling should be moved to clocks
subsystem ioctl handling, and then ethtool should simply forward
relevant calls to clocks subsystem. This will give us single
implementation point that defines which ioctls go to which clocks, and
single point where policy decisions are made, that, besides getting rid
of current inconsistencies, will allow for easier changes of policies in
the future.

That also could be the point that caches time stamping configuration and
gives it back to user space by ioctl request, freeing each driver from
implementing it, along with copying request structures to/from user
space that currently is done in every driver.

I believe such changes are valuable despite particular way the
SKBTX_IN_PROGRESS issue will be resolved.

> The only way I see to fix the bug is to introduce a new netdev flag,
> NETIF_F_PHY_HWTSTAMP or something like that. Then I'd grep for all
> occurrences of phy_has_hwtstamp() in the kernel (which currently amount
> to a whopping 2 users, 3 with your FEC "fix"), and declare this
> netdevice flag in their list of features. Then, phy_has_hwtstamp() and
> phy_has_tsinfo() and what not can be moved to generic places (or at
> least, I think they can), and those places could proceed to advertise
> and enable PHY timestamping only if the MAC declared itself ready. But,
> it is a bit strange to introduce a netdev flag just to fix a bug, I
> think.

To me this sounds like a plan.

In general (please don't take it as direct proposal to fix current
issues), the most flexible solution would be to allow for user space to
select which units will be time stamping (kernel clock being simply one
of them), and to deliver all the time stamps to the user space. This
will need clock IDs to be delivered along with time stamps (that is a
nice thing to have by itself, as I already mentioned elsewhere in
previous discussions.) For now it's just a raw idea, nevertheless to me
it sounds like a suitable goal for future design.

Thanks,
-- Sergey

  parent reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-17 16:10 Vladimir Oltean
2020-07-17 16:10 ` [PATCH net-next 1/3] docs: networking: timestamping: rename last section to "Known bugs" Vladimir Oltean
2020-07-17 22:05   ` Jacob Keller
2020-07-17 16:10 ` [PATCH net-next 2/3] docs: networking: timestamping: add one more known issue Vladimir Oltean
2020-07-17 23:08   ` Jacob Keller
2020-07-18 11:36     ` Vladimir Oltean
2020-07-17 16:10 ` [PATCH net-next 3/3] docs: networking: timestamping: add a set of frequently asked questions Vladimir Oltean
2020-07-17 23:12   ` Jacob Keller
2020-07-18 11:35     ` Vladimir Oltean
2020-07-20 18:54       ` Jacob Keller
2020-07-20 21:05         ` Vladimir Oltean
2020-07-20 21:45           ` Jacob Keller
2020-07-20 22:13             ` Vladimir Oltean
2020-07-21  0:21               ` Richard Cochran
2020-07-21 19:51                 ` Vladimir Oltean
2020-07-22  3:25                   ` Richard Cochran
2020-07-25 21:32                     ` Vladimir Oltean
2020-07-21 17:16               ` Jacob Keller
2020-07-21  0:15           ` Richard Cochran
2020-07-17 21:13 ` [PATCH net-next 0/3] Document more PTP timestamping known quirks Sergey Organov
2020-07-17 21:57   ` Vladimir Oltean
2020-07-17 23:13     ` Jacob Keller
2020-07-18 10:54     ` Sergey Organov [this message]
2020-07-18 11:30       ` Vladimir Oltean
2020-07-18 13:35         ` Sergey Organov
2020-07-18 11:21 ` Sergey Organov

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=878sfh2iwc.fsf@osv.gnss.ru \
    --to=sorganov@gmail.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.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

Linux-Doc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-doc/0 linux-doc/git/0.git

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

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-doc


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