lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 13 Feb 2024 16:57:12 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 Russell King <linux@...linux.org.uk>, <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>, Richard Cochran
 <richardcochran@...il.com>, <UNGLinuxDriver@...rochip.com>,
 <netdev@...r.kernel.org>, <linux-doc@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <thomas.petazzoni@...tlin.com>,
 Köry Maincent <kory.maincent@...tlin.com>
Subject: Re: [PATCH net-next 2/3] net: lan966x: Allow using PCH extension
 for PTP

Hello Horatiu,

On Tue, 13 Feb 2024 11:31:56 +0100
Horatiu Vultur <horatiu.vultur@...rochip.com> wrote:

> The 02/12/2024 18:33, Maxime Chevallier wrote:
> > [Some people who received this message don't often get email from maxime.chevallier@...tlin.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]  
> 
> Hi Maxime,
> 
> I have tried your patches on pcb8291, which is a lan966x without PHYs
> that support timestamping. And on this platform this patch breaks up the
> things. Because it should just do the timestamping the MAC in that case,
> but with this patch it doesn't get any time.
> The same issue can be reproduced on pcb8280 and then disable PHY
> timestamping, or change the lan8814 not to support HW timestamping.
> 
> Please see bellow the reason why.

You are entirely correct and I apparently messed-up my series as these
changes were implemented locally and somehow lost in the rebase. Indeed
this codes doesn't work at all... I'll resend that, thanks a lot for
the test and sorry !

> 
> > 
> > +/* Enable or disable PCH timestamp transmission. This uses the USGMII PCH
> > + * extensions to transmit the timestamps in the frame preamble.
> > + */
> > +static void lan966x_ptp_pch_configure(struct lan966x_port *port, bool *enable)
> > +{
> > +       struct phy_device *phydev = port->dev->phydev;
> > +       int ret;
> > +
> > +       if (!phydev)
> > +               *enable = false;
> > +
> > +       if (*enable) {
> > +               /* If we cannot enable inband PCH mode, we fallback to classic
> > +                * timestamping
> > +                */
> > +               if (phy_inband_ext_available(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP)) {
> > +                       ret = phy_inband_ext_enable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
> > +                       if (ret)
> > +                               *enable = false;
> > +               } else {
> > +                       *enable = false;
> > +               }
> > +       } else {
> > +               phy_inband_ext_disable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
> > +       }
> > +
> > +       lan_rmw(SYS_PCH_CFG_PCH_SUB_PORT_ID_SET(port->chip_port % 4) |
> > +               SYS_PCH_CFG_PCH_TX_MODE_SET(*enable) |
> > +               SYS_PCH_CFG_PCH_RX_MODE_SET(*enable),
> > +               SYS_PCH_CFG_PCH_SUB_PORT_ID |
> > +               SYS_PCH_CFG_PCH_TX_MODE |
> > +               SYS_PCH_CFG_PCH_RX_MODE,
> > +               port->lan966x, SYS_PCH_CFG(port->chip_port));
> > +}
> > +
> >  int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
> >                              struct kernel_hwtstamp_config *cfg,
> >                              struct netlink_ext_ack *extack)
> >  {
> >         struct lan966x *lan966x = port->lan966x;
> > +       bool timestamp_in_pch = false;
> >         struct lan966x_phc *phc;
> > 
> >         switch (cfg->tx_type) {
> > @@ -303,10 +339,18 @@ int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
> >                 return -ERANGE;
> >         }
> > 
> > +       if (cfg->source == HWTSTAMP_SOURCE_PHYLIB &&
> > +           cfg->tx_type == HWTSTAMP_TX_ON &&
> > +           port->config.portmode == PHY_INTERFACE_MODE_QUSGMII)
> > +               timestamp_in_pch = true;
> > +
> > +       lan966x_ptp_pch_configure(port, &timestamp_in_pch);
> > +
> >         /* Commit back the result & save it */
> >         mutex_lock(&lan966x->ptp_lock);
> >         phc = &lan966x->phc[LAN966X_PHC_PORT];
> >         phc->hwtstamp_config = *cfg;
> > +       phc->pch = timestamp_in_pch;  
> 
> Here we figure out if pch is enabled or not. If the cfg->source is not
> PHYLIB or the interface is not QUSGMII then timestamp_in_pch will stay
> false.
> 
> >         mutex_unlock(&lan966x->ptp_lock);
> > 
> >         return 0;
> > @@ -397,6 +441,7 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
> >         LAN966X_SKB_CB(skb)->jiffies = jiffies;
> > 
> >         lan966x->ptp_skbs++;
> > +  
> 
> I think this is just a small style change. So maybe it shouldn't be in
> here.
> 
> >         port->ts_id++;
> >         if (port->ts_id == LAN966X_MAX_PTP_ID)
> >                 port->ts_id = 0;
> > @@ -500,6 +545,27 @@ irqreturn_t lan966x_ptp_irq_handler(int irq, void *args)
> >                 /* Read RX timestamping to get the ID */
> >                 id = lan_rd(lan966x, PTP_TWOSTEP_STAMP);
> > 
> > +               /* If PCH is enabled, there is a "feature" that also the MAC
> > +                * will generate an interrupt for transmitted frames. This
> > +                * interrupt should be ignored, so clear the allocated resources
> > +                * and try to get the next timestamp. Maybe should clean the
> > +                * resources on the TX side?
> > +                */
> > +               if (phy_inband_ext_enabled(port->dev->phydev,
> > +                                          PHY_INBAND_EXT_PCH_TIMESTAMP)) {
> > +                       spin_lock(&lan966x->ptp_ts_id_lock);
> > +                       lan966x->ptp_skbs--;
> > +                       spin_unlock(&lan966x->ptp_ts_id_lock);
> > +
> > +                       dev_kfree_skb_any(skb_match);
> > +
> > +                       lan_rmw(PTP_TWOSTEP_CTRL_NXT_SET(1),
> > +                               PTP_TWOSTEP_CTRL_NXT,
> > +                               lan966x, PTP_TWOSTEP_CTRL);
> > +
> > +                       continue;
> > +               }
> > +
> >                 spin_lock_irqsave(&port->tx_skbs.lock, flags);
> >                 skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
> >                         if (LAN966X_SKB_CB(skb)->ts_id != id)
> > @@ -1088,19 +1154,27 @@ void lan966x_ptp_rxtstamp(struct lan966x *lan966x, struct sk_buff *skb,
> >         struct timespec64 ts;
> >         u64 full_ts_in_ns;
> > 
> > +       phc = &lan966x->phc[LAN966X_PHC_PORT];
> > +
> >         if (!lan966x->ptp ||
> > -           !lan966x->ports[src_port]->ptp_rx_cmd)
> > +           !lan966x->ports[src_port]->ptp_rx_cmd ||
> > +           !phc->pch)  
> 
> And here because phc->pch is false, it would just return.
> Meaning that it would never be able to get the time.
> I presume that this check should not be modified.

Dammit you are right and I had these modifications locally, but
apparently I messed my rebase and lost that... 

> 
> >                 return;
> > 
> > -       phc = &lan966x->phc[LAN966X_PHC_PORT];
> > -       lan966x_ptp_gettime64(&phc->info, &ts);
> > -
> > -       /* Drop the sub-ns precision */
> > -       timestamp = timestamp >> 2;
> > -       if (ts.tv_nsec < timestamp)
> > -               ts.tv_sec--;
> > -       ts.tv_nsec = timestamp;
> > -       full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
> > +       if (phc->pch) {
> > +               /* Drop the sub-ns precision */
> > +               timestamp = timestamp >> 2;
> > +               full_ts_in_ns = lower_32_bits(timestamp);
> > +       } else {
> > +               lan966x_ptp_gettime64(&phc->info, &ts);
> > +
> > +               /* Drop the sub-ns precision */
> > +               timestamp = timestamp >> 2;
> > +               if (ts.tv_nsec < timestamp)
> > +                       ts.tv_sec--;
> > +               ts.tv_nsec = timestamp;
> > +               full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
> > +       }  
>  
> 


Thanks for the review and analysis Horatiu, and sorry for this hiccup !

Maxime

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ