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
