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

[Alexander Lobakin]

From: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
Date: Mon, 7 Nov 2022 22:24:15 +0100

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

Hey,

> 
> > 
> > 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.

[...]

> > > +     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.

Yeah, I was reviewing the patches one by one and found out you're
adding more return values later :S

> 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:
>         }

It's okay to make them negative, but I wouldn't place them under
`unlikely`. It could be something like:

	src_port = lan966x_fdma_rx_check_frame(rx);
	if (unlikely(src_port == FDMA_ERROR))
		goto allocate_new;

	switch (src_port) {
	case 0 ... S64_MAX:
		// do PASS;
		break;
	case FDMA_TX:
		// do TX;
		break;
	case FDMA_REDIRECT:
	// and so on
	}

where

enum {
	FDMA_ERROR = -1, // only this one is "unlikely"
	FDMA_TX = -2,
	...
};

It's all just personal taste, so up to you :) Making
rx_check_frame() writing src_port to a pointer is fine as well.

> }
> ---
> 
> > 
> > > +}
> > > +
> > > +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
> > > +                                              u64 src_port)
> > > +{

[...]

> > > --
> > > 2.38.0
> > 
> > Thanks,
> > Olek
> 
> -- 
> /Horatiu

Thanks,
Olek