| 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
| ||
|
Message-ID: <20240213165712.27e0443a@device-28.home>
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, ×tamp_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