Re: [PATCH 6/8] cxl/pci: Prepare for mapping RAS Capability Structure

[Dan Williams <dan.j.williams@intel.com>]

On Thu, Mar 17, 2022 at 10:32 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 22-03-15 21:14:14, Dan Williams wrote:
> > The RAS Capabilitiy Structure is a CXL Component register capability
> > block. Unlike the HDM Decoder Capability, it will be referenced by the
> > cxl_pci driver in response to PCIe AER events. Due to this it is no
> > longer the case that cxl_map_component_regs() can assume that it should
> > map all component registers. Plumb a bitmask of capability ids to map
> > through cxl_map_component_regs().
> >
> > For symmetry cxl_probe_device_regs() is updated to populate @id in
> > 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a
> > need to map a subset of the device registers per caller.
>
> This seems weird to me. You spent the first 4 or so patches consolidating the
> mapping into a nice loop only to break out an ID to do individual mappings
> again. Are you sure this is such a win over having discrete mapping functions?

The loop is still there. This allows cxl_port and cxl_pci to share all
the same logic save for a bitmap to select the block. You're angling
for a:

cxl_map_hdm_regs(&port->dev, regs, &map);

...? Internally that cxl_map_hdm_regs() should be sharing code with
cxl_map_ras_regs(), so as far as I can see "discrete mapping
functions" is just asking for the below, and I'd rather skip the extra
wrapper:

int cxl_map_hdm_regs(struct pci_dev *pdev,
                           struct cxl_component_regs *regs,
                           struct cxl_register_map *map)
{
    return cxl_map_component_regs(&port->dev, regs, &map,
BIT(CXL_CM_CAP_CAP_ID_HDM));
}