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

["Clément Léger" <clement.leger@bootlin.com>]

Le Tue, 7 Dec 2021 15:23:48 +0000,
Vladimir Oltean <vladimir.oltean@nxp.com> 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@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).

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.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com