Re: [net-next v2 1/3] bnge: Add infrastructure support for RoCE MPC channels

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch has no consumer, and its subject and body promise more
  than it does.

--

bnge: Add infrastructure support for RoCE MPC channels

This patch adds a bar1 pointer to struct bnge_auxr_dev. It also fills that
pointer in from bd->bar1 in bnge_set_auxr_dev_info(), next to the existing
bar0. The commit message says follow-on patches will use it to send RoCE
commands to the firmware over the MPC channels.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_auxr.c b/drivers/net/ethernet/broadcom/bnge/bnge_auxr.c
> index 0955b488b6fea..87ae40eab5926 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_auxr.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_auxr.c
> @@ -181,6 +181,7 @@ static void bnge_set_auxr_dev_info(struct bnge_auxr_dev *auxr_dev,
>  	auxr_dev->pf_port_id = bd->pf.port_id;
>  	auxr_dev->en_state = bd->state;
>  	auxr_dev->bar0 = bd->bar0;
> +	auxr_dev->bar1 = bd->bar1;
>  }
>  
>  void bnge_rdma_aux_device_add(struct bnge_dev *bd)
> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_auxr.h b/drivers/net/ethernet/broadcom/bnge/bnge_auxr.h
> index 6c5c15ef2b0a7..13cb9289f5b89 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_auxr.h
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_auxr.h
> @@ -46,6 +46,7 @@ struct bnge_auxr_dev {
>  	struct net_device	*net;
>  	struct pci_dev		*pdev;
>  	void __iomem		*bar0;
> +	void __iomem		*bar1;

[Severity: Low]
Does anything read auxr_dev->bar1? This commit has no user of the new
field. The later patches in the series ("bnge: Update HSI" and "bnge:
reserve TX/completion rings for the RoCE MPC channel") don't add one either.

drivers/infiniband/hw/bng_re/ dereferences neither ->bar0 nor ->bar1. It
still maps its own regions, in bng_re_map_cmdq_mbox():

	mbox->reg.bar_base = pci_resource_start(pdev, mbox->reg.bar_id);

and in bng_re_map_creq_db():

	creq_db->reg.bar_base = pci_resource_start(pdev, creq_db->reg.bar_id);

Could this assignment and the new field move into the bng_re patch that
first uses auxr_dev->bar1?

The subject and body also describe more than the patch does:

    bnge: Add infrastructure support for RoCE MPC channels

    Add the initial L2 driver plumbing for RoCE MPC (PRIMATE channel)
    support:

Nothing in the change is specific to MPC or PRIMATE. The body also has a
bullet list with only one item.

If the field stays in this patch, could the message say that it only
passes the existing L2 BAR1 doorbell mapping to the RoCE auxiliary device?

>  
>  	struct bnge_msix_info	msix_info[BNGE_MAX_ROCE_MSIX];
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006161256.1493672-1-siva.kallam%40broadcom.com