Re: [PATCH net-next 1/5] sfc: add new helper macros to iterate channels by type

["Íñigo Huguet" <ihuguet@redhat.com>]

On Wed, May 11, 2022 at 9:19 AM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> Hi 婉igo,
>
> On Tue, May 10, 2022 at 10:44:39AM +0200, 婉igo Huguet wrote:
> > Sometimes in the driver it's needed to iterate a subset of the channels
> > depending on whether it is an rx, tx or xdp channel. Now it's done
> > iterating over all channels and checking if it's of the desired type,
> > leading to too much nested and a bit complex to understand code.
> >
> > Add new iterator macros to allow iterating only over a single type of
> > channel.
>
> We have similar code we'll be upstreaming soon, once we've managed to
> split off Siena. The crucial part of that seems to have been done
> today.
>
> > Signed-off-by: 婉igo Huguet <ihuguet@redhat.com>
> > ---
> >  drivers/net/ethernet/sfc/net_driver.h | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
> > index 318db906a154..7f665ba6a082 100644
> > --- a/drivers/net/ethernet/sfc/net_driver.h
> > +++ b/drivers/net/ethernet/sfc/net_driver.h
> > @@ -1501,6 +1501,27 @@ efx_get_channel(struct efx_nic *efx, unsigned index)
> >            _channel = (_channel->channel + 1 < (_efx)->n_channels) ?  \
> >                    (_efx)->channel[_channel->channel + 1] : NULL)
> >
> > +#define efx_for_each_rx_channel(_channel, _efx)                                  \
> > +     for (_channel = (_efx)->channel[0];                                 \
> > +          _channel;                                                      \
> > +          _channel = (_channel->channel + 1 < (_efx)->n_rx_channels) ?   \
> > +                  (_efx)->channel[_channel->channel + 1] : NULL)
> > +#define efx_for_each_tx_channel(_channel, _efx)                                  \
> > +     for (_channel = (_efx)->channel[efx->tx_channel_offset];            \
> > +          _channel;                                                      \
> > +          _channel = (_channel->channel + 1 <                            \
> > +                  (_efx)->tx_channel_offset + (_efx)->n_tx_channels) ?   \
> > +                  (_efx)->channel[_channel->channel + 1] : NULL)
>
> We've chosen a different naming conventions here, and we're also removing
> the channel array.
> Also not every channel has RX queues and not every channel has TX queues.
>
> Sounds like it's time we have another call.

I saw you were already upstreaming the siena split, probably it had
been a good idea to wait for it to be merged before sending this.

I'm going to be on PTO the rest of the week and the next one, maybe we
can talk when I'm back, and hopefully you will have made more
progress. Then I can resubmit this series adapted to the new state of
the code, if it's still useful.

> Martin
>
> > +#define efx_for_each_xdp_channel(_channel, _efx)                         \
> > +     for (_channel = ((_efx)->n_xdp_channels > 0) ?                      \
> > +                  (_efx)->channel[efx->xdp_channel_offset] : NULL;       \
> > +          _channel;                                                      \
> > +          _channel = (_channel->channel + 1 <                            \
> > +                  (_efx)->xdp_channel_offset + (_efx)->n_xdp_channels) ? \
> > +                  (_efx)->channel[_channel->channel + 1] : NULL)
> > +
> >  /* Iterate over all used channels in reverse */
> >  #define efx_for_each_channel_rev(_channel, _efx)                     \
> >       for (_channel = (_efx)->channel[(_efx)->n_channels - 1];        \
> > --
> > 2.34.1
>


-- 
Íñigo Huguet