The 11/07/2022 17:06, Alexander Lobakin wrote:
Hi Olek,
>
> From: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> Date: Sun, 6 Nov 2022 22:11:52 +0100
>
> > The function lan966x_fdma_rx_get_frame was unmapping the frame from
> > device and check also if the frame was received on a valid port. And
> > only after that it tried to generate the skb.
> > Move this check in a different function, in preparation for xdp
> > support. Such that xdp to be added here and the
> > lan966x_fdma_rx_get_frame to be used only when giving the skb to upper
> > layers.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> > ---
> > .../ethernet/microchip/lan966x/lan966x_fdma.c | 85 +++++++++++++------
> > .../ethernet/microchip/lan966x/lan966x_main.h | 9 ++
> > 2 files changed, 69 insertions(+), 25 deletions(-)
>
> [...]
>
> > -static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
> > +static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
> > {
> > struct lan966x *lan966x = rx->lan966x;
> > - u64 src_port, timestamp;
> > struct lan966x_db *db;
> > - struct sk_buff *skb;
> > struct page *page;
> >
> > - /* Get the received frame and unmap it */
> > db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
> > page = rx->page[rx->dcb_index][rx->db_index];
> > + if (unlikely(!page))
> > + return FDMA_ERROR;
> >
> > dma_sync_single_for_cpu(lan966x->dev, (dma_addr_t)db->dataptr,
> > FDMA_DCB_STATUS_BLOCKL(db->status),
> > DMA_FROM_DEVICE);
> >
> > + dma_unmap_single_attrs(lan966x->dev, (dma_addr_t)db->dataptr,
> > + PAGE_SIZE << rx->page_order, DMA_FROM_DEVICE,
> > + DMA_ATTR_SKIP_CPU_SYNC);
> > +
> > + lan966x_ifh_get_src_port(page_address(page), src_port);
> > + if (WARN_ON(*src_port >= lan966x->num_phys_ports))
> > + return FDMA_ERROR;
> > +
> > + return FDMA_PASS;
>
> How about making this function return s64, which would be "src_port
> or negative error", and dropping the second argument @src_port (the
> example of calling it below)?
That was also my first thought.
But the thing is, I am also adding FDMA_DROP in the next patch of this
series(3/4). And I am planning to add also FDMA_TX and FDMA_REDIRECT in
a next patch series.
Should they(FDMA_DROP, FDMA_TX, FDMA_REDIRECT) also be some negative
numbers? And then have something like you proposed belowed:
---
src_port = lan966x_fdma_rx_check_frame(rx);
if (unlikely(src_port < 0)) {
switch(src_port) {
case FDMA_ERROR:
...
goto allocate_new
case FDMA_DROP:
...
continue;
case FDMA_TX:
case FDMA_REDIRECT:
}
}
---
>
> > +}
> > +
> > +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
> > + u64 src_port)
> > +{
>
> [...]
>
> > - skb = lan966x_fdma_rx_get_frame(rx);
> > + counter++;
> >
> > - rx->page[rx->dcb_index][rx->db_index] = NULL;
> > - rx->dcb_index++;
> > - rx->dcb_index &= FDMA_DCB_MAX - 1;
> > + switch (lan966x_fdma_rx_check_frame(rx, &src_port)) {
> > + case FDMA_PASS:
> > + break;
> > + case FDMA_ERROR:
> > + lan966x_fdma_rx_free_page(rx);
> > + lan966x_fdma_rx_advance_dcb(rx);
> > + goto allocate_new;
> > + }
>
> So, here you could do (if you want to keep the current flow)::
>
> src_port = lan966x_fdma_rx_check_frame(rx);
> switch (src_port) {
> case 0 .. S64_MAX: // for example
> break;
> case FDMA_ERROR: // must be < 0
> lan_966x_fdma_rx_free_page(rx);
> ...
> }
>
> But given that the error path is very unlikely and cold, I would
> prefer if-else over switch case:
>
> src_port = lan966x_fdma_rx_check_frame(rx);
> if (unlikely(src_port < 0)) {
> lan_966x_fdma_rx_free_page(rx);
> ...
> goto allocate_new;
> }
>
> >
> > + skb = lan966x_fdma_rx_get_frame(rx, src_port);
> > + lan966x_fdma_rx_advance_dcb(rx);
> > if (!skb)
> > - break;
> > + goto allocate_new;
> >
> > napi_gro_receive(&lan966x->napi, skb);
> > - counter++;
> > }
> >
> > +allocate_new:
> > /* Allocate new pages and map them */
> > while (dcb_reload != rx->dcb_index) {
> > db = &rx->dcbs[dcb_reload].db[rx->db_index];
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index 4ec33999e4df6..464fb5e4a8ff6 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > @@ -100,6 +100,15 @@ enum macaccess_entry_type {
> > ENTRYTYPE_MACV6,
> > };
> >
> > +/* FDMA return action codes for checking if the frame is valid
> > + * FDMA_PASS, frame is valid and can be used
> > + * FDMA_ERROR, something went wrong, stop getting more frames
> > + */
> > +enum lan966x_fdma_action {
> > + FDMA_PASS = 0,
> > + FDMA_ERROR,
> > +};
> > +
> > struct lan966x_port;
> >
> > struct lan966x_db {
> > --
> > 2.38.0
>
> Thanks,
> Olek
--
/Horatiu
