Re: [PATCH net-next v2 2/4] net: lan966x: Split function lan966x_fdma_rx_get_frame

[Horatiu Vultur <horatiu.vultur@microchip.com>]

The 11/07/2022 17:06, Alexander Lobakin wrote:

Hi Olek,

> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> 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@microchip.com>
> > ---
> >  .../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