Re: [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps to socket error queue

[Vladimir Oltean <olteanv@gmail.com>]

Hi Richard,

On Tue, 24 Dec 2019 at 21:05, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Tue, Dec 17, 2019 at 10:21:29PM +0000, Keller, Jacob E wrote:
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > > Behalf Of Vladimir Oltean
>
> > > There are many more drivers that are in principle broken with DSA PTP,
> > > since they don't even have the equivalent check for priv->hwts_tx_en.
>
> Please stop saying that.  It is not true.
>

Why isn't it true?
You mean to say that this code in cavium/liquidio/lio_main.c will ever
work with a PTP-cable DSA switch or PHY?

static netdev_tx_t liquidio_xmit(struct sk_buff *skb, struct net_device *netdev)
{
(...)
    if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
        skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
        cmdsetup.s.timestamp = 1;
    }

Or this one in cavium/octeon/octeon_mgmt.c?

    re.s.tstamp = ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) != 0);

Or this one in mscc/ocelot.c:

    /* Check if timestamping is needed */
    if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP) {
        info.rew_op = ocelot_port->ptp_cmd;
        if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
            info.rew_op |= (ocelot_port->ts_id  % 4) << 3;
    }

Or this one in mellanox/mlx5/core/en_tx.c:

            if (unlikely(skb_shinfo(skb)->tx_flags &
                     SKBTX_HW_TSTAMP)) {
                struct skb_shared_hwtstamps hwts = {};

                hwts.hwtstamp =
                    mlx5_timecounter_cyc2time(sq->clock,
                                  get_cqe_ts(cqe));
                skb_tstamp_tx(skb, &hwts);
            }

etc etc.

How will these drivers not transmit a second hw TX timestamp to the
stack, if they don't check whether TX timestamping is enabled for
their netdev?

Of course, at least that breakage is going to be much more binary and
obvious: PTP simply won't work at all for drivers stacked on top of
them until they are fixed.

> No fix is needed.  MAC drivers must set SKBTX_IN_PROGRESS and call
> skb_tstamp_tx() to deliver the transmit time stamp.  DSA drivers
> should call skb_complete_tx_timestamp() to deliver the transmit time
> stamp, and they should *not* set SKBTX_IN_PROGRESS.
>

Who says so, and why? How would it be better than fixing gianfar in
this case? How would it be better in avoiding compatibility with the
drivers mentioned above?

Does the TI PHYTER driver count?

commit e2e2f51dd0254fa0002bcd1c5fda180348163f09
Author: Stefan Sørensen <stefan.sorensen@spectralink.com>
Date:   Mon Feb 3 15:36:35 2014 +0100

    net:phy:dp83640: Declare that TX timestamping possible

    Set the SKBTX_IN_PROGRESS bit in tx_flags dp83640_txtstamp when doing
    tx timestamps as per Documentation/networking/timestamping.txt.

    Signed-off-by: Stefan Sørensen <stefan.sorensen@spectralink.com>
    Acked-by: Richard Cochran <richardcochran@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 547725fa8671..3f882eea6e1d 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1281,6 +1281,7 @@ static void dp83640_txtstamp(struct phy_device *phydev,
                }
                /* fall through */
        case HWTSTAMP_TX_ON:
+               skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
                skb_queue_tail(&dp83640->tx_queue, skb);
                schedule_work(&dp83640->ts_work);
                break;

I think this rule is somewhat made-up and arbitrary.

> Thanks,
> Richard

Thanks,
-Vladimir