Re: [PATCH net-next v5 4/4] net: ocelot: add FDMA support

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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@nxp.com> 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@bootlin.com>
> > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > > ---  
> > 
> > > +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).