On 18/09/2020 04:07:24+0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.olt...@nxp.com>
>
> The ocelot_port->ts_id is used to:
> (a) populate skb->cb[0] for matching the TX timestamp in the PTP IRQ
> with an skb.
> (b) populate the REW_OP from the injection header of the ongoing skb.
> Only then is ocelot_port->ts_id incremented.
>
> This is a problem because, at least theoretically, another timestampable
> skb might use the same ocelot_port->ts_id before that is incremented.
> Normally all transmit calls are serialized by the netdev transmit
> spinlock, but in this case, ocelot_port_add_txtstamp_skb() is also
> called by DSA, which has started declaring the NETIF_F_LLTX feature
> since commit 2b86cb829976 ("net: dsa: declare lockless TX feature for
> slave ports"). So the logic of using and incrementing the timestamp id
> should be atomic per port.
>
> The solution is to use the global ocelot_port->ts_id only while
> protected by the associated ocelot_port->ts_id_lock. That's where we
> populate skb->cb[0]. Note that for ocelot, ocelot_port_add_txtstamp_skb
> is called for the actual skb, but for felix, it is called for the skb's
> clone. That is something which will also be changed in the future.
>
> Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
> Reviewed-by: Horatiu Vultur <horatiu.vul...@microchip.com>
Tested-by: Alexandre Belloni <alexandre.bell...@bootlin.com>
Reviewed-by: Alexandre Belloni <alexandre.bell...@bootlin.com>
> ---
> Changes in v2:
> Added an extra explanation about NETIF_F_LLTX in commit message.
>
> drivers/net/ethernet/mscc/ocelot.c | 8 +++++++-
> drivers/net/ethernet/mscc/ocelot_net.c | 6 ++----
> include/soc/mscc/ocelot.h | 1 +
> net/dsa/tag_ocelot.c | 11 +++++++----
> 4 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot.c
> b/drivers/net/ethernet/mscc/ocelot.c
> index 5abb7d2b0a9e..83eb7c325061 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -421,10 +421,15 @@ int ocelot_port_add_txtstamp_skb(struct ocelot_port
> *ocelot_port,
>
> if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&
> ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> + spin_lock(&ocelot_port->ts_id_lock);
> +
> shinfo->tx_flags |= SKBTX_IN_PROGRESS;
> /* Store timestamp ID in cb[0] of sk_buff */
> - skb->cb[0] = ocelot_port->ts_id % 4;
> + skb->cb[0] = ocelot_port->ts_id;
> + ocelot_port->ts_id = (ocelot_port->ts_id + 1) % 4;
> skb_queue_tail(&ocelot_port->tx_skbs, skb);
> +
> + spin_unlock(&ocelot_port->ts_id_lock);
> return 0;
> }
> return -ENODATA;
> @@ -1300,6 +1305,7 @@ void ocelot_init_port(struct ocelot *ocelot, int port)
> struct ocelot_port *ocelot_port = ocelot->ports[port];
>
> skb_queue_head_init(&ocelot_port->tx_skbs);
> + spin_lock_init(&ocelot_port->ts_id_lock);
>
> /* Basic L2 initialization */
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c
> b/drivers/net/ethernet/mscc/ocelot_net.c
> index cacabc23215a..8490e42e9e2d 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -349,10 +349,8 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct
> net_device *dev)
>
> 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;
> - ocelot_port->ts_id++;
> - }
> + if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
> + info.rew_op |= skb->cb[0] << 3;
> }
>
> ocelot_gen_ifh(ifh, &info);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index da369b12005f..4521dd602ddc 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -566,6 +566,7 @@ struct ocelot_port {
> u8 ptp_cmd;
> struct sk_buff_head tx_skbs;
> u8 ts_id;
> + spinlock_t ts_id_lock;
>
> phy_interface_t phy_mode;
>
> diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
> index 42f327c06dca..b4fc05cafaa6 100644
> --- a/net/dsa/tag_ocelot.c
> +++ b/net/dsa/tag_ocelot.c
> @@ -160,11 +160,14 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
> packing(injection, &qos_class, 19, 17, OCELOT_TAG_LEN, PACK, 0);
>
> if (ocelot->ptp && (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
> + struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
> +
> rew_op = ocelot_port->ptp_cmd;
> - if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> - rew_op |= (ocelot_port->ts_id % 4) << 3;
> - ocelot_port->ts_id++;
> - }
> + /* Retrieve timestamp ID populated inside skb->cb[0] of the
> + * clone by ocelot_port_add_txtstamp_skb
> + */
> + if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
> + rew_op |= clone->cb[0] << 3;
>
> packing(injection, &rew_op, 125, 117, OCELOT_TAG_LEN, PACK, 0);
> }
> --
> 2.25.1
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
