Re: [PATCH V13 4/9] cxl/pci: Create PCI DOE mailbox's for memory devices

[Ira Weiny <ira.weiny@intel.com>]

On Wed, Jul 13, 2022 at 08:30:18PM -0700, Dan Williams wrote:
> ira.weiny@ wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 

[snip]

> > +
> > +static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
> > +{
> > +	struct device *dev = cxlds->dev;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	u16 off = 0;
> > +
> > +	/*
> > +	 * Mailbox creation is best effort.  Higher layers must determine if
> > +	 * the lack of a mailbox for their protocol is a device failure or not.
> > +	 */
> > +	pci_doe_for_each_off(pdev, off) {
> > +		struct pci_doe_mb *doe_mb;
> > +
> > +		doe_mb = pcim_doe_create_mb(pdev, off);
> > +		if (IS_ERR(doe_mb)) {
> > +			pci_err(pdev,
> > +				"Failed to create MB object for MB @ %x\n",
> > +				off);
> 
> oh, the rest of cxl_pci driver is using dev_err() and dev_dbg(), not a
> big deal, can be a follow-on cleanup to make it consistent.

Changed since I'm reving anyway.

> 
> > +			continue;
> > +		}
> > +
> > +		if (xa_insert(&cxlds->doe_mbs, off, doe_mb, GFP_KERNEL)) {
> 
> xarray all the things! Nice.
> 
> > +			pci_err(pdev,
> > +				"xa_insert failed to insert MB @ %x\n",
> > +				off);
> > +			continue;
> 
> Lets make these error messages more actionable:
> 
> "Failed to setup DOE, CDAT data may not be available"

I really wanted this to be generic rather than mention something like CDAT
here.

But I see in the other patch you mention skipping the creation of all the
mailboxes and just grab the CDAT one.

Let me resolve that first.

> 
> > +		}
> > +
> > +		pci_dbg(pdev, "Created DOE mailbox @%x\n", off);
> > +	}
> > +}
> > +
> >  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  {
> >  	struct cxl_register_map map;
> > @@ -408,6 +446,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	if (IS_ERR(cxlds))
> >  		return PTR_ERR(cxlds);
> >  
> > +	xa_init(&cxlds->doe_mbs);
> > +	if (devm_add_action(&pdev->dev, cxl_pci_destroy_doe, &cxlds->doe_mbs))
> > +		return -ENOMEM;
> > +
> 
> This belongs inside devm_cxl_pci_create_doe().

Done.

Ira

> 
> >  	cxlds->serial = pci_get_dsn(pdev);
> >  	cxlds->cxl_dvsec = pci_find_dvsec_capability(
> >  		pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
> > @@ -434,6 +476,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  
> >  	cxlds->component_reg_phys = cxl_regmap_to_base(pdev, &map);
> >  
> > +	devm_cxl_pci_create_doe(cxlds);
> > +
> >  	rc = cxl_pci_setup_mailbox(cxlds);
> >  	if (rc)
> >  		return rc;
> > -- 
> > 2.35.3
> > 
> 
>