RE: [PATCH net-next 2/2] rtase: Link queues to NAPI instances

[Justin Lai <justinlai0215@realtek.com>]

> On Tue, Jun 10, 2025 at 06:33:34PM +0800, Justin Lai wrote:
> > Link queues to NAPI instances with netif_queue_set_napi. This
> > information can be queried with the netdev-genl API.
> >
> > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/rtase/rtase.h    |  4 +++
> >  .../net/ethernet/realtek/rtase/rtase_main.c   | 33 +++++++++++++++++--
> >  2 files changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/rtase/rtase.h
> > b/drivers/net/ethernet/realtek/rtase/rtase.h
> > index 498cfe4d0cac..be98f4de46c4 100644
> > --- a/drivers/net/ethernet/realtek/rtase/rtase.h
> > +++ b/drivers/net/ethernet/realtek/rtase/rtase.h
> > @@ -39,6 +39,9 @@
> >  #define RTASE_FUNC_RXQ_NUM  1
> >  #define RTASE_INTERRUPT_NUM 1
> >
> > +#define RTASE_TX_RING 0
> > +#define RTASE_RX_RING 1
> > +
> >  #define RTASE_MITI_TIME_COUNT_MASK    GENMASK(3, 0)
> >  #define RTASE_MITI_TIME_UNIT_MASK     GENMASK(7, 4)
> >  #define RTASE_MITI_DEFAULT_TIME       128
> > @@ -288,6 +291,7 @@ struct rtase_ring {
> >       u32 cur_idx;
> >       u32 dirty_idx;
> >       u16 index;
> > +     u8 ring_type;
> 
> Two questions:
> 
> 1. why not use enum netdev_queue_type instead of making driver specific
> values that are the opposite of the existing enum values ?
> 
> If you used the existing enum, you could omit the if below (see below), as
> well?
> 
> 2. is "ring" in the name redundant? maybe just "type"? asking because below
> the code becomes "ring->ring_type" and maybe "ring->type" is better?
> 
> >       struct sk_buff *skbuff[RTASE_NUM_DESC];
> >       void *data_buf[RTASE_NUM_DESC];
> > diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > index a88af868da8c..ef3ada91d555 100644
> > --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > @@ -326,6 +326,7 @@ static void rtase_tx_desc_init(struct rtase_private
> *tp, u16 idx)
> >       ring->cur_idx = 0;
> >       ring->dirty_idx = 0;
> >       ring->index = idx;
> > +     ring->ring_type = RTASE_TX_RING;
> >       ring->alloc_fail = 0;
> >
> >       for (i = 0; i < RTASE_NUM_DESC; i++) { @@ -345,6 +346,10 @@
> > static void rtase_tx_desc_init(struct rtase_private *tp, u16 idx)
> >               ring->ivec = &tp->int_vector[0];
> >               list_add_tail(&ring->ring_entry,
> &tp->int_vector[0].ring_list);
> >       }
> > +
> > +     netif_queue_set_napi(tp->dev, ring->index,
> > +                          NETDEV_QUEUE_TYPE_TX,
> > +                          &ring->ivec->napi);
> >  }
> >
> >  static void rtase_map_to_asic(union rtase_rx_desc *desc, dma_addr_t
> > mapping, @@ -590,6 +595,7 @@ static void rtase_rx_desc_init(struct
> rtase_private *tp, u16 idx)
> >       ring->cur_idx = 0;
> >       ring->dirty_idx = 0;
> >       ring->index = idx;
> > +     ring->ring_type = RTASE_RX_RING;
> >       ring->alloc_fail = 0;
> >
> >       for (i = 0; i < RTASE_NUM_DESC; i++) @@ -597,6 +603,9 @@ static
> > void rtase_rx_desc_init(struct rtase_private *tp, u16 idx)
> >
> >       ring->ring_handler = rx_handler;
> >       ring->ivec = &tp->int_vector[idx];
> > +     netif_queue_set_napi(tp->dev, ring->index,
> > +                          NETDEV_QUEUE_TYPE_RX,
> > +                          &ring->ivec->napi);
> >       list_add_tail(&ring->ring_entry,
> > &tp->int_vector[idx].ring_list);  }
> >
> > @@ -1161,8 +1170,18 @@ static void rtase_down(struct net_device *dev)
> >               ivec = &tp->int_vector[i];
> >               napi_disable(&ivec->napi);
> >               list_for_each_entry_safe(ring, tmp, &ivec->ring_list,
> > -                                      ring_entry)
> > +                                      ring_entry) {
> > +                     if (ring->ring_type == RTASE_TX_RING)
> > +                             netif_queue_set_napi(tp->dev,
> ring->index,
> > +
> NETDEV_QUEUE_TYPE_TX,
> > +                                                  NULL);
> > +                     else
> > +                             netif_queue_set_napi(tp->dev,
> ring->index,
> > +
> NETDEV_QUEUE_TYPE_RX,
> > +                                                  NULL);
> > +
> 
> Using the existing enum would simplify this block?
> 
>   netif_queue_set_napi(tp->dev, ring->index, ring->type, NULL);
> 
> >                       list_del(&ring->ring_entry);
> > +             }
> >       }
> >
> >       netif_tx_disable(dev);
> > @@ -1518,8 +1537,18 @@ static void rtase_sw_reset(struct net_device
> *dev)
> >       for (i = 0; i < tp->int_nums; i++) {
> >               ivec = &tp->int_vector[i];
> >               list_for_each_entry_safe(ring, tmp, &ivec->ring_list,
> > -                                      ring_entry)
> > +                                      ring_entry) {
> > +                     if (ring->ring_type == RTASE_TX_RING)
> > +                             netif_queue_set_napi(tp->dev,
> ring->index,
> > +
> NETDEV_QUEUE_TYPE_TX,
> > +                                                  NULL);
> > +                     else
> > +                             netif_queue_set_napi(tp->dev,
> ring->index,
> > +
> NETDEV_QUEUE_TYPE_RX,
> > +                                                  NULL);
> > +
> 
> Same as above?

Hi Joe,

Thank you for your reply. I will use the enum netdev_queue_type to avoid
needing if statements to determine the ring type later. I will also rename
ring_type to type.

Thanks,
Justin