Re: [PATCH net-next v4 08/15] idpf: refactor idpf to use libie_pci APIs

[Larysa Zaremba <larysa.zaremba@intel.com>]

There is one nice-to-have change that came out of Sashiko's review:

commit 5238b5986775c0c76881ed594a7b09216ee20107
Author: Larysa Zaremba <larysa.zaremba@intel.com>
Date:   Mon Jul 13 17:07:10 2026 +0200

    fixup! idpf: refactor idpf to use libie_pci APIs

diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
index 5a191644b28e..184d30c12abb 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_main.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
@@ -265,8 +265,9 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

        err = idpf_dev_init(adapter, ent);
        if (err) {
-               dev_err(&pdev->dev, "Unexpected dev ID 0x%x in idpf probe\n",
-                       ent->device);
+               dev_err(&pdev->dev,
+                       "Failed to initialize device (ID 0x%x): %d\n",
+                       ent->device, err);
                goto err_free;
        }


Everything else:

> commit 461758d19f17656a1ed2f88e0867b4610ec38b43
> Author: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> idpf: refactor idpf to use libie_pci APIs
> This patch converts the idpf driver over to the shared libie_pci init
> and MMIO helpers, routes mailbox and register accesses through
> libie_pci_get_mmio_addr(), and adds a struct libie_ctlq_ctx to the
> adapter for the mmio_info list. struct idpf_hw is kept around because
> it still houses the control queues.
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_controlq.h b/drivers/net/ethernet/intel/idpf/idpf_controlq.h
> > index de4ece40c2ff..acf595e9265f 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_controlq.h
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_controlq.h
> > @@ -109,8 +109,6 @@ struct idpf_mmio_reg {
> >   * Align to ctlq_hw_info
> >   */
> >  struct idpf_hw {
> > -	struct idpf_mmio_reg mbx;
> > -	struct idpf_mmio_reg rstat;
> >  	/* Array of remaining LAN BAR regions */
> >  	int num_lan_regs;
> >  	struct idpf_mmio_reg *lan_regs;
> After this hunk, do num_lan_regs, lan_regs, struct idpf_mmio_reg, and
> IDPF_MMIO_MAP_FALLBACK_MAX_REMAINING still have any users? A grep of
> the tree shows only their own declarations in idpf_controlq.h with no
> readers or writers after this commit.
> The same question applies to the mailbox helper macros in
> drivers/net/ethernet/intel/idpf/idpf_mem.h, which are left in place
> but still reference the just-removed mbx field:
> #define idpf_mbx_wr32(a, reg, value)    writel((value), ((a)->mbx.vaddr + (reg)))
> #define idpf_mbx_rd32(a, reg)           readl((a)->mbx.vaddr + (reg))
> #define idpf_mbx_wr64(a, reg, value)    writeq((value), ((a)->mbx.vaddr + (reg)))
> #define idpf_mbx_rd64(a, reg)           readq((a)->mbx.vaddr + (reg))
> Would it make sense to drop these together with the field removal
> above so that any future accidental use fails at review rather than
> silently reintroducing dead paths?
> 
struct idpf_hw and those macros are dropped completely in the next patch
> 
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_idc.c b/drivers/net/ethernet/intel/idpf/idpf_idc.c
> > index b7d6b08fc89e..0a7edb783758 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_idc.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_idc.c
> > @@ -416,9 +416,12 @@ idpf_idc_init_msix_data(struct idpf_adapter *adapter)
> >  int idpf_idc_init_aux_core_dev(struct idpf_adapter *adapter,
> >  			       enum iidc_function_type ftype)
> >  {
> > +	struct libie_mmio_info *mmio = &adapter->ctlq_ctx.mmio_info;
> >  	struct iidc_rdma_core_dev_info *cdev_info;
> >  	struct iidc_rdma_priv_dev_info *privd;
> > -	int err, i;
> > +	struct libie_pci_mmio_region *mr;
> > +	size_t num_mem_regions;
> > +	int err, i = 0;
> >  
> >  	adapter->cdev_info = kzalloc_obj(*cdev_info);
> >  	if (!adapter->cdev_info)
> > @@ -436,22 +439,37 @@ int idpf_idc_init_aux_core_dev(struct idpf_adapter *adapter,
> >  	cdev_info->rdma_protocol = IIDC_RDMA_PROTOCOL_ROCEV2;
> >  	privd->ftype = ftype;
> >  
> > +	num_mem_regions = list_count_nodes(&mmio->mmio_list);
> > +	if (num_mem_regions <= IDPF_MMIO_REG_NUM_STATIC) {
> > +		err = -EINVAL;
> > +		goto err_plug_aux_dev;
> > +	}
> > +
> > +	num_mem_regions -= IDPF_MMIO_REG_NUM_STATIC;
> >  	privd->mapped_mem_regions =
> >  		kzalloc_objs(struct iidc_rdma_lan_mapped_mem_region,
> > -			     adapter->hw.num_lan_regs);
> > +			     num_mem_regions);
> >  	if (!privd->mapped_mem_regions) {
> >  		err = -ENOMEM;
> >  		goto err_plug_aux_dev;
> >  	}
> >  
> > -	privd->num_memory_regions = cpu_to_le16(adapter->hw.num_lan_regs);
> > -	for (i = 0; i < adapter->hw.num_lan_regs; i++) {
> > -		privd->mapped_mem_regions[i].region_addr =
> > -			adapter->hw.lan_regs[i].vaddr;
> > -		privd->mapped_mem_regions[i].size =
> > -			cpu_to_le64(adapter->hw.lan_regs[i].addr_len);
> > -		privd->mapped_mem_regions[i].start_offset =
> > -			cpu_to_le64(adapter->hw.lan_regs[i].addr_start);
> > +	privd->num_memory_regions = cpu_to_le16(num_mem_regions);
> > +	list_for_each_entry(mr, &mmio->mmio_list, list) {
> > +		struct resource *static_regs = adapter->dev_ops.static_reg_info;
> > +		bool is_static = false;
> > +
> > +		for (uint j = 0; j < IDPF_MMIO_REG_NUM_STATIC; j++)
> > +			if (mr->offset == static_regs[j].start)
> > +				is_static = true;
> > +
> > +		if (is_static)
> > +			continue;
> > +
> > +		privd->mapped_mem_regions[i].region_addr = mr->addr;
> > +		privd->mapped_mem_regions[i].size = cpu_to_le64(mr->size);
> > +		privd->mapped_mem_regions[i++].start_offset =
> > +						cpu_to_le64(mr->offset);
> >  	}
> Is a semantic change to what is reported to the IIDC RDMA aux device
> intended here?
> Previously idpf_calc_remaining_mmio_regs() always reported
> IDPF_MMIO_MAP_FALLBACK_MAX_REMAINING (3) regions, including any that
> happened to have addr_len == 0, and firmware-supplied regions with
> size == 0 were forwarded verbatim.
> After this patch:
> - idpf_map_remaining_mmio_regs() skips zero-sized regions via the
>   !size || short-circuit before libie_pci_map_mmio_region().
> - idpf_cfg_lan_memory_regions() skips zero-sized firmware regions via
>   if (len && !libie_pci_map_mmio_region(mmio, offset, len)).
> - idpf_idc_init_aux_core_dev() now fails outright with -EINVAL when
>   list_count_nodes(&mmio->mmio_list) <= IDPF_MMIO_REG_NUM_STATIC.
> If a valid platform has, for example, the mailbox at BAR offset 0
> and rstat abutting the end of BAR0, are all three fallback regions
> zero-sized, and would that make the RDMA aux init unconditionally
> fail on that configuration? Would it be worth calling this ABI
> change out in the commit message?
> Separately, is the count/write-index invariant in this loop enforced
> anywhere?
> The code computes:
>     num_mem_regions = list_count_nodes(&mmio->mmio_list);
>     ...
>     num_mem_regions -= IDPF_MMIO_REG_NUM_STATIC;
>     privd->mapped_mem_regions =
>         kzalloc_objs(struct iidc_rdma_lan_mapped_mem_region,
>                      num_mem_regions);
>     ...
>     privd->num_memory_regions = cpu_to_le16(num_mem_regions);
>     list_for_each_entry(mr, &mmio->mmio_list, list) {
>         ...
>         privd->mapped_mem_regions[i++].start_offset =
>                                         cpu_to_le64(mr->offset);
>     }
> The loop increments i for every non-static entry, but there is no
> i < num_mem_regions guard. It relies on exactly
> IDPF_MMIO_REG_NUM_STATIC entries in the list having mr->offset equal
> to a static_reg_info[j].start. If, for any reason, fewer than
> IDPF_MMIO_REG_NUM_STATIC entries match (a future coalescing in
> libie_pci_map_mmio_region(), or a firmware-supplied region whose
> offset happens to coincide with a static offset without being added
> as a duplicate), does the loop overrun the mapped_mem_regions
> allocation?
> Would an explicit if (i >= num_mem_regions) break; inside the loop
> make this invariant local and enforceable?
> 
Static regions are *static*, we can absolutely rely on at least
static regions to be valid
> 
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
> > index ab3c409e587b..93b11fb1609f 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_main.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
> > @@ -246,32 +265,21 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	adapter->req_rx_splitq = true;
> >  
> >  	adapter->pdev = pdev;
> > -	err = pcim_enable_device(pdev);
> > -	if (err)
> > -		goto err_free;
> >  
> > -	err = pcim_request_region(pdev, 0, pci_name(pdev));
> > +	err = idpf_dev_init(adapter, ent);
> >  	if (err) {
> > -		pci_err(pdev, "pcim_request_region failed %pe\n", ERR_PTR(err));
> > -
> > +		dev_err(&pdev->dev, "Unexpected dev ID 0x%x in idpf probe\n",
> > +			ent->device);
> >  		goto err_free;
> >  	}
> >  
> > -	err = pci_enable_ptm(pdev);
> > -	if (err)
> > -		pci_dbg(pdev, "PCIe PTM is not supported by PCIe bus/controller\n");
> > -
> > -	/* set up for high or low dma */
> > -	err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> > +	err = idpf_cfg_device(adapter);
> >  	if (err) {
> > -		pci_err(pdev, "DMA configuration failed: %pe\n", ERR_PTR(err));
> > -
> > +		pci_err(pdev, "Failed to configure device specific resources: %pe\n",
> > +			ERR_PTR(err));
> >  		goto err_free;
> >  	}
> Does the "Unexpected dev ID" message still accurately describe every
> failure mode of idpf_dev_init() after this change?
> idpf_dev_init() now calls libie_pci_init_dev() first, which wraps
> pcim_enable_device(), pcim_request_region(), and
> dma_set_mask_and_coherent(). Failures in any of those (for example
> -EIO or -ENOMEM from BAR request, or DMA mask failures) will now be
> reported as "Unexpected dev ID 0x%x in idpf probe" instead of the
> prior specific messages ("pcim_request_region failed %pe", "DMA
> configuration failed: %pe"). Would it be worth propagating err via
> %pe alongside the device ID, or letting libie_pci_init_dev() log its
> own reason?
> 

Valid, look at diff at the start.

> 
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > index a3c17f0e14f3..357ff3e86b2e 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > @@ -3445,35 +3450,30 @@ int idpf_vc_core_init(struct idpf_adapter *adapter)
> >  	}
> >  
> >  	if (idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS, VIRTCHNL2_CAP_LAN_MEMORY_REGIONS)) {
> > -		err = idpf_send_get_lan_memory_regions(adapter);
> > +		err = idpf_cfg_lan_memory_regions(adapter);
> >  		if (err) {
> > -			dev_err(&adapter->pdev->dev, "Failed to get LAN memory regions: %d\n",
> > +			dev_err(&adapter->pdev->dev, "Failed to configure LAN memory regions: %d\n",
> >  				err);
> >  			return -EINVAL;
> >  		}
> >  	} else {
> >  		/* Fallback to mapping the remaining regions of the entire BAR */
> > -		err = idpf_calc_remaining_mmio_regs(adapter);
> > +		err = idpf_map_remaining_mmio_regs(adapter);
> >  		if (err) {
> > -			dev_err(&adapter->pdev->dev, "Failed to allocate BAR0 region(s): %d\n",
> > +			dev_err(&adapter->pdev->dev, "Failed to configure BAR0 region(s): %d\n",
> >  				err);
> > -			return -ENOMEM;
> > +			return err;
> >  		}
> >  	}
> Is the mmio_info->mmio_list traversed while it may be concurrently
> mutated?
> libie_pci_map_mmio_region() calls list_add_tail() and
> libie_pci_unmap_fltr_regs()/libie_pci_unmap_all_mmio_regions() call
> list_del() + kvfree(), while __libie_pci_get_mmio_addr() traverses
> the list with plain list_for_each_entry() (no lock, no RCU).
> In idpf_init_hard_reset() the mbx_task workqueue is kicked before
> idpf_vc_core_init() runs the memory-region setup:
>     queue_delayed_work(adapter->mbx_wq, &adapter->mbx_task, 0);
>     ...
>     err = idpf_vc_core_init(adapter);
> And idpf_vc_core_init() then reaches idpf_cfg_lan_memory_regions()
> (which calls libie_pci_map_mmio_region() in a loop) or its error
> path decfg_regions: which calls idpf_decfg_lan_memory_regions() ->
> libie_pci_unmap_fltr_regs().
> Meanwhile mbx_task -> idpf_ctlq_post_rx_buffs()/idpf_ctlq_send() ends
> up in libie_pci_get_mmio_addr(&hw->back->ctlq_ctx.mmio_info,
> cq->reg.tail), which walks the same mmio_list.
> Under current code the mailbox entry is the first one added at probe
> time and is never removed until unmap-all, so iteration terminates
> before touching the tail being mutated. Is that invariant intended
> to be relied on going forward? Would an explicit lock or a comment
> documenting the read/write ordering constraint help protect against
> a future addition (for example a PTP or ITR register lookup from a
> work item) that would need to walk past the mailbox entry?
> 
This is fine, we can rely on the mailbox register to be valid
while mailbox communication is going. MBX is a static region,
so it is expected to be unmapped only once we are shutting down for good
>