On Tue, Dec 07, 2021 at 04:31:08PM +0100, Clément Léger wrote:
> Le Tue, 7 Dec 2021 15:23:48 +0000,
> Vladimir Oltean <vladimir.oltean@xxxxxxx> a écrit :
>
> > On Tue, Dec 07, 2021 at 04:16:24PM +0100, Clément Léger wrote:
> > > Le Tue, 7 Dec 2021 13:52:01 +0000,
> > > Vladimir Oltean <vladimir.oltean@xxxxxxx> a écrit :
> > >
> > > > On Tue, Dec 07, 2021 at 10:08:53AM +0100, Clément Léger wrote:
> > > > > Ethernet frames can be extracted or injected autonomously to or from
> > > > > the device’s DDR3/DDR3L memory and/or PCIe memory space. Linked list
> > > > > data structures in memory are used for injecting or extracting Ethernet
> > > > > frames. The FDMA generates interrupts when frame extraction or
> > > > > injection is done and when the linked lists need updating.
> > > > >
> > > > > The FDMA is shared between all the ethernet ports of the switch and
> > > > > uses a linked list of descriptors (DCB) to inject and extract packets.
> > > > > Before adding descriptors, the FDMA channels must be stopped. It would
> > > > > be inefficient to do that each time a descriptor would be added so the
> > > > > channels are restarted only once they stopped.
> > > > >
> > > > > Both channels uses ring-like structure to feed the DCBs to the FDMA.
> > > > > head and tail are never touched by hardware and are completely handled
> > > > > by the driver. On top of that, page recycling has been added and is
> > > > > mostly taken from gianfar driver.
> > > > >
> > > > > Co-developed-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> > > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> > > > > Signed-off-by: Clément Léger <clement.leger@xxxxxxxxxxx>
> > > > > ---
> > > >
> > > > > +static void ocelot_fdma_send_skb(struct ocelot *ocelot,
> > > > > + struct ocelot_fdma *fdma, struct sk_buff *skb)
> > > > > +{
> > > > > + struct ocelot_fdma_tx_ring *tx_ring = &fdma->tx_ring;
> > > > > + struct ocelot_fdma_tx_buf *tx_buf;
> > > > > + struct ocelot_fdma_dcb *dcb;
> > > > > + dma_addr_t dma;
> > > > > + u16 next_idx;
> > > > > +
> > > > > + dcb = &tx_ring->dcbs[tx_ring->next_to_use];
> > > > > + tx_buf = &tx_ring->bufs[tx_ring->next_to_use];
> > > > > + if (!ocelot_fdma_tx_dcb_set_skb(ocelot, tx_buf, dcb, skb)) {
> > > > > + dev_kfree_skb_any(skb);
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + next_idx = ocelot_fdma_idx_next(tx_ring->next_to_use,
> > > > > + OCELOT_FDMA_TX_RING_SIZE);
> > > > > + /* If the FDMA TX chan is empty, then enqueue the DCB directly */
> > > > > + if (ocelot_fdma_tx_ring_empty(fdma)) {
> > > > > + dma = ocelot_fdma_idx_dma(tx_ring->dcbs_dma, tx_ring->next_to_use);
> > > > > + ocelot_fdma_activate_chan(ocelot, dma, MSCC_FDMA_INJ_CHAN);
> > > > > + } else {
> > > > > + /* Chain the DCBs */
> > > > > + dcb->llp = ocelot_fdma_idx_dma(tx_ring->dcbs_dma, next_idx);
> > > > > + }
> > > > > + skb_tx_timestamp(skb);
> > > > > +
> > > > > + tx_ring->next_to_use = next_idx;
> > > >
> > > > You've decided against moving these before ocelot_fdma_activate_chan?
> > > > The skb may be freed by ocelot_fdma_tx_cleanup() before
> > > > skb_tx_timestamp() has a chance to run, is this not true?
> > >
> > > Since tx_ring->next_to_use is updated after calling skb_tx_timestamp,
> > > fdma_tx_cleanup will not free it. However, I'm not sure if the
> > > timestamping should be done before being sent by the hardware (ie, does
> > > the timestamping function modifies the SKB inplace). If not, then the
> > > current code is ok. By looking at ocelot_port_inject_frame, the
> > > timestamping is done after sending the frame.
> >
> > It looks like we may need Richard for an expert opinon.
> > Documentation/networking/timestamping.rst only says:
> >
> > | Driver should call skb_tx_timestamp() as close to passing sk_buff to hardware
> > | as possible.
> >
> > not whether it must be done before or it can be done after too;
> > but my intuition says that is also needs to be strictly _before_ the
> > hardware xmit, otherwise it also races with the hardware TX timestamping
> > path and that may lead to issues of its own (the logic whether to
> > deliver a software and/or a hardware timestamp to the socket is not
> > trivial at all).
>
> Ok, I will move it before sending since it since it is cleaner anyway.
> And probably submit a fix for the register-based injection path later.
Let me give you a stronger argument. If PHY timestamping is in use, it
hooks into skb_tx_timestamp(). If the PHY needs to arm whatever register
or clone the skb or whatever, this must be done strictly before sending
the packet, otherwise the PHY driver may miss the TX timestamping
interrupt for the packet that just went by.
