[PATCH net 1/2] net: enetc: set MAC RX FIFO to recommended value

[PATCH net 1/2] net: enetc: set MAC RX FIFO to recommended value

[Vladimir Oltean]

From: Alex Marginean <alexandru.marginean@nxp.com>

On LS1028A, the MAC RX FIFO defaults to the value 2, which is too high
and may lead to RX lock-up under traffic at a rate higher than 6 Gbps.
Set it to 1 instead, as recommended by the hardware design team and by
later versions of the ENETC block guide.

Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>
Reviewed-by: Jason Liu <jason.hui.liu@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 ++
 drivers/net/ethernet/freescale/enetc/enetc_pf.c | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index de0d20b0f489..00938f7960a4 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -234,6 +234,8 @@ enum enetc_bdr_type {TX, RX};
 #define ENETC_PM0_MAXFRM	0x8014
 #define ENETC_SET_TX_MTU(val)	((val) << 16)
 #define ENETC_SET_MAXFRM(val)	((val) & 0xffff)
+#define ENETC_PM0_RX_FIFO	0x801c
+#define ENETC_PM0_RX_FIFO_VAL	1
 
 #define ENETC_PM_IMDIO_BASE	0x8030
 
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index ca02f033bea2..224fc37a6757 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -490,6 +490,12 @@ static void enetc_configure_port_mac(struct enetc_hw *hw)
 
 	enetc_port_wr(hw, ENETC_PM1_CMD_CFG, ENETC_PM0_CMD_PHY_TX_EN |
 		      ENETC_PM0_CMD_TXP	| ENETC_PM0_PROMISC);
+
+	/* On LS1028A, the MAC RX FIFO defaults to 2, which is too high
+	 * and may lead to RX lock-up under traffic. Set it to 1 instead,
+	 * as recommended by the hardware team.
+	 */
+	enetc_port_wr(hw, ENETC_PM0_RX_FIFO, ENETC_PM0_RX_FIFO_VAL);
 }
 
 static void enetc_mac_config(struct enetc_hw *hw, phy_interface_t phy_mode)
-- 
2.25.1

[PATCH net 2/2] net: enetc: allow hardware timestamping on TX queues with tc-etf enabled

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The txtime is passed to the driver in skb->skb_mstamp_ns, which is
actually in a union with skb->tstamp (the place where software
timestamps are kept).

Since commit b50a5c70ffa4 ("net: allow simultaneous SW and HW transmit
timestamping"), __sock_recv_timestamp has some logic for making sure
that the two calls to skb_tstamp_tx:

skb_tx_timestamp(skb) # Software timestamp in the driver
-> skb_tstamp_tx(skb, NULL)

and

skb_tstamp_tx(skb, &shhwtstamps) # Hardware timestamp in the driver

will both do the right thing and in a race-free manner, meaning that
skb_tx_timestamp will deliver a cmsg with the software timestamp only,
and skb_tstamp_tx with a non-NULL hwtstamps argument will deliver a cmsg
with the hardware timestamp only.

Why are races even possible? Well, because although the software timestamp
skb->tstamp is private per skb, the hardware timestamp skb_hwtstamps(skb)
lives in skb_shinfo(skb), an area which is shared between skbs and their
clones. And skb_tstamp_tx works by cloning the packets when timestamping
them, therefore attempting to perform hardware timestamping on an skb's
clone will also change the hardware timestamp of the original skb. And
the original skb might have been yet again cloned for software
timestamping, at an earlier stage.

So the logic in __sock_recv_timestamp can't be as simple as saying
"does this skb have a hardware timestamp? if yes I'll send the hardware
timestamp to the socket, otherwise I'll send the software timestamp",
precisely because the hardware timestamp is shared.
Instead, it's quite the other way around: __sock_recv_timestamp says
"does this skb have a software timestamp? if yes, I'll send the software
timestamp, otherwise the hardware one". This works because the software
timestamp is not shared with clones.

But that means we have a problem when we attempt hardware timestamping
with skbs that don't have the skb->tstamp == 0. __sock_recv_timestamp
will say "oh, yeah, this must be some sort of odd clone" and will not
deliver the hardware timestamp to the socket. And this is exactly what
is happening when we have txtime enabled on the socket: as mentioned,
that is put in a union with skb->tstamp, so it is quite easy to mistake
it.

Do what other drivers do (intel igb/igc) and write zero to skb->tstamp
before taking the hardware timestamp. It's of no use to us now (we're
already on the TX confirmation path).

Fixes: 0d08c9ec7d6e ("enetc: add support time specific departure base on the qos etf")
Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 30d7d4e83900..09471329f3a3 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -344,6 +344,12 @@ static void enetc_tstamp_tx(struct sk_buff *skb, u64 tstamp)
 	if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) {
 		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
 		shhwtstamps.hwtstamp = ns_to_ktime(tstamp);
+		/* Ensure skb_mstamp_ns, which might have been populated with
+		 * the txtime, is not mistaken for a software timestamp,
+		 * because this will prevent the dispatch of our hardware
+		 * timestamp to the socket.
+		 */
+		skb->tstamp = ktime_set(0, 0);
 		skb_tstamp_tx(skb, &shhwtstamps);
 	}
 }
-- 
2.25.1

Re: [PATCH net 2/2] net: enetc: allow hardware timestamping on TX queues with tc-etf enabled

[Vinicius Costa Gomes]

Vladimir Oltean <olteanv@gmail.com> writes:

> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> The txtime is passed to the driver in skb->skb_mstamp_ns, which is
> actually in a union with skb->tstamp (the place where software
> timestamps are kept).
>
> Since commit b50a5c70ffa4 ("net: allow simultaneous SW and HW transmit
> timestamping"), __sock_recv_timestamp has some logic for making sure
> that the two calls to skb_tstamp_tx:
>
> skb_tx_timestamp(skb) # Software timestamp in the driver
> -> skb_tstamp_tx(skb, NULL)
>
> and
>
> skb_tstamp_tx(skb, &shhwtstamps) # Hardware timestamp in the driver
>
> will both do the right thing and in a race-free manner, meaning that
> skb_tx_timestamp will deliver a cmsg with the software timestamp only,
> and skb_tstamp_tx with a non-NULL hwtstamps argument will deliver a cmsg
> with the hardware timestamp only.
>
> Why are races even possible? Well, because although the software timestamp
> skb->tstamp is private per skb, the hardware timestamp skb_hwtstamps(skb)
> lives in skb_shinfo(skb), an area which is shared between skbs and their
> clones. And skb_tstamp_tx works by cloning the packets when timestamping
> them, therefore attempting to perform hardware timestamping on an skb's
> clone will also change the hardware timestamp of the original skb. And
> the original skb might have been yet again cloned for software
> timestamping, at an earlier stage.
>
> So the logic in __sock_recv_timestamp can't be as simple as saying
> "does this skb have a hardware timestamp? if yes I'll send the hardware
> timestamp to the socket, otherwise I'll send the software timestamp",
> precisely because the hardware timestamp is shared.
> Instead, it's quite the other way around: __sock_recv_timestamp says
> "does this skb have a software timestamp? if yes, I'll send the software
> timestamp, otherwise the hardware one". This works because the software
> timestamp is not shared with clones.
>
> But that means we have a problem when we attempt hardware timestamping
> with skbs that don't have the skb->tstamp == 0. __sock_recv_timestamp
> will say "oh, yeah, this must be some sort of odd clone" and will not
> deliver the hardware timestamp to the socket. And this is exactly what
> is happening when we have txtime enabled on the socket: as mentioned,
> that is put in a union with skb->tstamp, so it is quite easy to mistake
> it.

Great write up :-)

I know first person the time/effort that went on debugging this.

What do you think of introducing a helper, something like:

skb_txtime_consumed()

to make it even clearer what's going on.

>
> Do what other drivers do (intel igb/igc) and write zero to skb->tstamp
> before taking the hardware timestamp. It's of no use to us now (we're
> already on the TX confirmation path).
>
> Fixes: 0d08c9ec7d6e ("enetc: add support time specific departure base on the qos etf")
> Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

Anyway,

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>


Cheers,
-- 
Vinicius

Re: [PATCH net 1/2] net: enetc: set MAC RX FIFO to recommended value

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Sun,  7 Mar 2021 15:23:38 +0200 you wrote:
> From: Alex Marginean <alexandru.marginean@nxp.com>
> 
> On LS1028A, the MAC RX FIFO defaults to the value 2, which is too high
> and may lead to RX lock-up under traffic at a rate higher than 6 Gbps.
> Set it to 1 instead, as recommended by the hardware design team and by
> later versions of the ENETC block guide.
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: enetc: set MAC RX FIFO to recommended value
    https://git.kernel.org/netdev/net/c/1b2395dfff5b
  - [net,2/2] net: enetc: allow hardware timestamping on TX queues with tc-etf enabled
    https://git.kernel.org/netdev/net/c/29d98f54a4fe

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

RE: [PATCH net 2/2] net: enetc: allow hardware timestamping on TX queues with tc-etf enabled

[Po Liu]

Ok to me.

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: 2021年3月7日 21:24
> To: David S . Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
> netdev@vger.kernel.org; Po Liu <po.liu@nxp.com>
> Cc: Alexandru Marginean <alexandru.marginean@nxp.com>; Claudiu Manoil
> <claudiu.manoil@nxp.com>; Michael Walle <michael@walle.cc>; Vladimir
> Oltean <vladimir.oltean@nxp.com>; Vinicius Costa Gomes
> <vinicius.gomes@intel.com>
> Subject: [PATCH net 2/2] net: enetc: allow hardware timestamping on TX
> queues with tc-etf enabled
> 
> Caution: EXT Email
> 
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The txtime is passed to the driver in skb->skb_mstamp_ns, which is actually in a
> union with skb->tstamp (the place where software timestamps are kept).
> 
> Since commit b50a5c70ffa4 ("net: allow simultaneous SW and HW transmit
> timestamping"), __sock_recv_timestamp has some logic for making sure that
> the two calls to skb_tstamp_tx:
> 
> skb_tx_timestamp(skb) # Software timestamp in the driver
> -> skb_tstamp_tx(skb, NULL)
> 
> and
> 
> skb_tstamp_tx(skb, &shhwtstamps) # Hardware timestamp in the driver
> 
> will both do the right thing and in a race-free manner, meaning that
> skb_tx_timestamp will deliver a cmsg with the software timestamp only, and
> skb_tstamp_tx with a non-NULL hwtstamps argument will deliver a cmsg with
> the hardware timestamp only.
> 
> Why are races even possible? Well, because although the software timestamp
> skb->tstamp is private per skb, the hardware timestamp
> skb->skb_hwtstamps(skb)
> lives in skb_shinfo(skb), an area which is shared between skbs and their clones.
> And skb_tstamp_tx works by cloning the packets when timestamping them,
> therefore attempting to perform hardware timestamping on an skb's clone will
> also change the hardware timestamp of the original skb. And the original skb
> might have been yet again cloned for software timestamping, at an earlier stage.
> 
> So the logic in __sock_recv_timestamp can't be as simple as saying "does this
> skb have a hardware timestamp? if yes I'll send the hardware timestamp to the
> socket, otherwise I'll send the software timestamp", precisely because the
> hardware timestamp is shared.
> Instead, it's quite the other way around: __sock_recv_timestamp says "does this
> skb have a software timestamp? if yes, I'll send the software timestamp,
> otherwise the hardware one". This works because the software timestamp is not
> shared with clones.
> 
> But that means we have a problem when we attempt hardware timestamping
> with skbs that don't have the skb->tstamp == 0. __sock_recv_timestamp will say
> "oh, yeah, this must be some sort of odd clone" and will not deliver the
> hardware timestamp to the socket. And this is exactly what is happening when
> we have txtime enabled on the socket: as mentioned, that is put in a union with
> skb->tstamp, so it is quite easy to mistake it.
> 
> Do what other drivers do (intel igb/igc) and write zero to skb->tstamp before
> taking the hardware timestamp. It's of no use to us now (we're already on the
> TX confirmation path).
> 
> Fixes: 0d08c9ec7d6e ("enetc: add support time specific departure base on the
> qos etf")
> Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 30d7d4e83900..09471329f3a3 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -344,6 +344,12 @@ static void enetc_tstamp_tx(struct sk_buff *skb, u64
> tstamp)
>         if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) {
>                 memset(&shhwtstamps, 0, sizeof(shhwtstamps));
>                 shhwtstamps.hwtstamp = ns_to_ktime(tstamp);
> +               /* Ensure skb_mstamp_ns, which might have been populated with
> +                * the txtime, is not mistaken for a software timestamp,
> +                * because this will prevent the dispatch of our hardware
> +                * timestamp to the socket.
> +                */
> +               skb->tstamp = ktime_set(0, 0);
>                 skb_tstamp_tx(skb, &shhwtstamps);
>         }
>  }
> --
> 2.25.1

Reviewed-by: Po Liu <po.liu@nxp.com>

Br,
Po Liu