RE: [EXTERNAL] Re: [v2,net-next,1/6] net: mana: Create separate EQs for each vPort

[Long Li]

> Subject: [EXTERNAL] Re: [v2,net-next,1/6] net: mana: Create separate EQs for
> each vPort
> 
> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fnetdev-
> ai.bots.linux.dev%2Fai-
> local.html&data=05%7C02%7Clongli%40microsoft.com%7C070979f149574e63
> e15a08de7b9aa19e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6
> 39084101208945710%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnR
> ydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3
> D%3D%7C0%7C%7C%7C&sdata=Cs%2BZwNguYa8FGbpb4H0naVDhDzhjzBvDU7
> 5aZekerzc%3D&reserved=0
> ---
> net: mana: Create separate EQs for each vPort
> 
> To prepare for assigning vPorts to dedicated MSI-X vectors, this commit
> removes EQ sharing among vPorts and creates dedicated EQs for each vPort.
> The EQ definition is moved from struct mana_context to struct
> mana_port_context and related support functions are updated.
> 
> > diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> > --- a/include/net/mana/mana.h
> > +++ b/include/net/mana/mana.h
> 
> [ ... ]
> 
> > @@ -499,6 +497,9 @@ struct mana_port_context {
> >
> >  	u8 mac_addr[ETH_ALEN];
> >
> > +	struct mana_eq *eqs;
> > +	struct dentry *mana_eqs_debugfs;
> > +
> >  	enum TRI_STATE rss_state;
> >
> >  	mana_handle_t default_rxobj;
> 
> [ ... ]
> 
> > diff --git a/drivers/infiniband/hw/mana/qp.c
> > b/drivers/infiniband/hw/mana/qp.c
> > --- a/drivers/infiniband/hw/mana/qp.c
> > +++ b/drivers/infiniband/hw/mana/qp.c
> > @@ -188,7 +188,7 @@ struct mana_obj_spec cq_spec = {};
> >  		cq_spec.gdma_region = cq->queue.gdma_region;
> >  		cq_spec.queue_size = cq->cqe * COMP_ENTRY_SIZE;
> >  		cq_spec.modr_ctx_id = 0;
> > -		eq = &mpc->ac->eqs[cq->comp_vector];
> > +		eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];
>                            ^^^^^^^
> >  		cq_spec.attached_eq = eq->eq->id;
> 
> Can mpc->eqs be NULL here? Looking at the initialization paths:
> 
> Before this patch, mpc->ac->eqs was allocated during mana_probe() and was
> always non-NULL after driver initialization.
> 
> After this patch, mpc->eqs is only allocated in two places:
> 1. mana_alloc_queues() - called when the Ethernet interface is brought up 2.
> mana_ib_cfg_vport() - called when RDMA RAW QPs configure the vport
> 
> However, mana_ib_create_qp_rss() doesn't call mana_ib_cfg_vport(). If an RSS
> QP is created via RDMA verbs before the Ethernet port is brought up and
> before any RAW QP has configured the vport, mpc->eqs would still be NULL.
> 
> Computing &mpc->eqs[cq->comp_vector % mpc->num_queues] from a NULL
> pointer produces an invalid address, and the subsequent dereference eq->eq-
> >id would crash the kernel.
> 
> Should there be a NULL check here, or should mana_ib_create_qp_rss() ensure
> EQs are created before reaching this point?

I'm fixing it in v3.

Thank you,
Long