| Message ID | 20200915182229.69529-3-olteanv@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Bugfixes in Microsemi Ocelot switch driver | expand |
On 15/09/2020 21:22:24+0300, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The ocelot_port->ts_id is used to: > - populate skb->cb[0] for matching the TX timestamp in the PTP IRQ with > an skb. > - 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. 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. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/ethernet/mscc/ocelot.c | 13 ++++++++++++- > drivers/net/ethernet/mscc/ocelot_net.c | 6 ++---- > include/soc/mscc/ocelot.h | 1 + > net/dsa/tag_ocelot.c | 11 +++++++---- > 4 files changed, 22 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c > index 5abb7d2b0a9e..8caf3afd464d 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; > @@ -1443,6 +1448,12 @@ int ocelot_init(struct ocelot *ocelot) > if (!ocelot->stats_queue) > return -ENOMEM; > > + for (port = 0; port < ocelot->num_phys_ports; port++) { > + struct ocelot_port *ocelot_port = ocelot->ports[port]; > + At this point, ocelot_port is NULL because ocelot_init is called before the port structures are allocated in mscc_ocelot_probe > + spin_lock_init(&ocelot_port->ts_id_lock); > + } > + > INIT_LIST_HEAD(&ocelot->multicast); > ocelot_mact_init(ocelot); > ocelot_vlan_init(ocelot); > 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 >
On Wed, Sep 16, 2020 at 01:12:04PM +0200, Alexandre Belloni wrote: > > + for (port = 0; port < ocelot->num_phys_ports; port++) { > > + struct ocelot_port *ocelot_port = ocelot->ports[port]; > > + > > At this point, ocelot_port is NULL because ocelot_init is called before > the port structures are allocated in mscc_ocelot_probe Yikes, you're right, thanks for testing! -Vladimir
From: Vladimir Oltean <olteanv@gmail.com> Date: Tue, 15 Sep 2020 21:22:24 +0300 > This is a problem because, at least theoretically, another timestampable > skb might use the same ocelot_port->ts_id before that is incremented. So > the logic of using and incrementing the timestamp id should be atomic > per port. Have you actually observed this race in practice? All transmit calls are serialized by the netdev transmit spinlock. Let's not add locking if it is not actually necessary.
On Wed, Sep 16, 2020 at 05:19:26PM -0700, David Miller wrote: > From: Vladimir Oltean <olteanv@gmail.com> > Date: Tue, 15 Sep 2020 21:22:24 +0300 > > > This is a problem because, at least theoretically, another timestampable > > skb might use the same ocelot_port->ts_id before that is incremented. So > > the logic of using and incrementing the timestamp id should be atomic > > per port. > > Have you actually observed this race in practice? > > All transmit calls are serialized by the netdev transmit spinlock. > > Let's not add locking if it is not actually necessary. It's a bit more complicated. This code is also used from DSA, and DSA now declares NETIF_F_LLTX.
From: Vladimir Oltean <olteanv@gmail.com> Date: Fri, 18 Sep 2020 02:43:40 +0300 > On Wed, Sep 16, 2020 at 05:19:26PM -0700, David Miller wrote: >> From: Vladimir Oltean <olteanv@gmail.com> >> Date: Tue, 15 Sep 2020 21:22:24 +0300 >> >> > This is a problem because, at least theoretically, another timestampable >> > skb might use the same ocelot_port->ts_id before that is incremented. So >> > the logic of using and incrementing the timestamp id should be atomic >> > per port. >> >> Have you actually observed this race in practice? >> >> All transmit calls are serialized by the netdev transmit spinlock. >> >> Let's not add locking if it is not actually necessary. > > It's a bit more complicated. > This code is also used from DSA, and DSA now declares NETIF_F_LLTX. Please document that in the commit log message, thank you.
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c index 5abb7d2b0a9e..8caf3afd464d 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; @@ -1443,6 +1448,12 @@ int ocelot_init(struct ocelot *ocelot) if (!ocelot->stats_queue) return -ENOMEM; + for (port = 0; port < ocelot->num_phys_ports; port++) { + struct ocelot_port *ocelot_port = ocelot->ports[port]; + + spin_lock_init(&ocelot_port->ts_id_lock); + } + INIT_LIST_HEAD(&ocelot->multicast); ocelot_mact_init(ocelot); ocelot_vlan_init(ocelot); 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); }