Re: [PATCH v12 05/23] cxl: add function for type2 cxl regs setup

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

> 
> >> +	 */
> >> +	if (rc == -ENODEV)
> >> +		return 0;  
> > Hmm. I don't mind hugely but I'd expect the -ENODEV handler in the
> > clearly accelerator specific code that follows not here.
> >
> > That would require cxl_map_device_regs() to definitely not return
> > -ENODEV though which is a bit ugly so I guess this is ok.
> >
> > I'm not entirely convinced this helper makes sense though given
> > the 2 parts of the component regs are just done inline in
> > cxl_pci_accel_setup_regs() and if you did that then this
> > accelerator specific 'carry on anyway' would be in the function
> > with accel in the name.
> >
> > 	You'd need a
> > 	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, caps);
> > 	if (rc) {
> > 		if (rc != -ENODEV)
> > 			return rc;
> > 	} else {
> > 		rc = cxl_map_device_regs();
> > 		if (rc)
> > 			return rc;	
> > 	}	
> > though which is a little messy.  
> 
> 
> That messiness is the reason I added the other function keeping, I 
> think, the code clearer.
> 
> Note that other function is only used by accel code, but I can change 
> the name for making it more visible:
> 
> 
> cxl_pci_setup_memdev_regs  ---> cxl_accel_setup_memdev_regs
That works.

> 
> 
> >> +
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	return cxl_map_device_regs(&map, &cxlds->regs.device_regs);
> >> +}
> >> +
> >> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds,
> >> +			      unsigned long *caps)
> >> +{
> >> +	int rc;
> >> +
> >> +	rc = cxl_pci_setup_memdev_regs(pdev, cxlds, caps);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
> >> +				&cxlds->reg_map, caps);
> >> +	if (rc) {
> >> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
> >> +		return rc;
> >> +	}
> >> +
> >> +	if (!caps || !test_bit(CXL_CM_CAP_CAP_ID_RAS, caps))  
> > As before. Why not just mandate caps?  If someone really doesn't
> > care they can provide a bitmap and ignore it.  Seems like a simpler
> > interface to me.  
> 
> 
> Not sure what you meant here. This is not just about knowing by the 
> caller the capabilities but also mapping the related structures if present.

I meant the !caps variable not being NULL. Just mandate that there must be a caps
bitmap passed in always.  Caller can throw away the value if it doesn't want it.

> 
> The now returned caps is useful for dealing with mandatory vs optional 
> caps which the current code targeting Type3-only can not. In other 
> words, the core code can not know if a cap missing is an error or not.
Not that. Was a much more mundane point ;)

> 
> 
> >> +		return 0;
> >> +
> >> +	rc = cxl_map_component_regs(&cxlds->reg_map,
> >> +				    &cxlds->regs.component,
> >> +				    BIT(CXL_CM_CAP_CAP_ID_RAS));
> >> +	if (rc)
> >> +		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
> >> +
> >> +	return rc;
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, "CXL");