Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Jul 2022 11:50:43 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dan Williams <dan.j.williams@intel.com>
CC:     Bjorn Helgaas <bhelgaas@google.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        "Lukas Wunner" <lukas@wunner.de>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
        <linux-pci@vger.kernel.org>
Subject: Re: [PATCH V13 4/9] cxl/pci: Create PCI DOE mailbox's for memory
 devices
Message-ID: <YtBlg+epvGE+bQBe@iweiny-desk3>
References: <20220705154932.2141021-1-ira.weiny@intel.com>
 <20220705154932.2141021-5-ira.weiny@intel.com>
 <62cf8dca27760_1643dc29444@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <62cf8dca27760_1643dc29444@dwillia2-xfh.jf.intel.com.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?CEj3vNH7xdv9ZHoISWkP1uLInyWcfK5iUqnOBeSKk3lXOtzRzPVgdIQoGZNC?=
 =?us-ascii?Q?/vazH1O31pKaqBBcVL5RW1/v1zS7VDEUuj918EBOoXR1D4sbkfDich3NSFr3?=
 =?us-ascii?Q?8xtuYUhEz4xpjMBOCfzHFZ8LeskBJ/HHsdUQS/KIqFb2lovsubbnRoR81Dip?=
 =?us-ascii?Q?mu2BOv+zyczcxBOBYt8LTRxATeMTeMEJ7XJjI1ANGn4UQ1Ch4S2ywRklEHGw?=
 =?us-ascii?Q?LsrSzOLzt7MbSFs30a+o+JDUHYPHa9PyEOO4WTciYSxiYWLiI1sw+YWbi+dv?=
 =?us-ascii?Q?th02VjL0gmct/TOzR2yMFswzKsAGTy1Lq4d80BWOzeJcdVSKP8Mw3CiOBfrn?=
 =?us-ascii?Q?72B+dxJxKkakn8/kjVu99nZvVM1xsWAKJJXfEsnCiPFNYRa/kK/MRmJJuzpg?=
 =?us-ascii?Q?qeycHlNUgND2//5cJzXMhxVh3PCUuE4Ngzfx5ANAe7wAl97a7auHyAYKlV1q?=
 =?us-ascii?Q?CLofoCF3eX9nfzRoNuv6M68i3/RTmPTW/PmDKJ05iuiDHyiyZmvUWH7zNXlI?=
 =?us-ascii?Q?n148Pxe5MSo4guQBDvqSwHiC/sgf775LOojjH6k18GLppBrIsKNWO77f9vSC?=
 =?us-ascii?Q?Dc/tiOr5amIVWCHUU47KsUx2QTAytpOBVCPEBjgD921f4HrxjRQmmtCd66hv?=
 =?us-ascii?Q?i2IG/QCWAoO6+M3P6743/pBCqzY8V1HoM6uaXcP/Sdul4Y7pW0SXY0glI3K6?=
 =?us-ascii?Q?HdIICvR0pZLK6H7HG/ITHBkUcLu/KvukWCsOjA4t1QU0IKSZ7TtL5qJgJsBd?=
 =?us-ascii?Q?A7EvjH8Bl3rZaMG7oLTlkhxpaZg6YUgfgqG+dvfpwSgrddj4G7A1+nMvFKLF?=
 =?us-ascii?Q?VczqZ+1xJr1MYJAEqoBtEMbFC0B0xUrV+Ys+ucsxDqyHYX/TBzND9NR/vf3t?=
 =?us-ascii?Q?UDWJ1hr8SEFtNpSns1ndP7r2AbcQsoDfgdvi/5ROlfakKgwZqrWKwd/qKsE/?=
 =?us-ascii?Q?ePR+rE29ht92skFQR0U+PonCE/EIfN3qf5/2H8Z5HpUG0z0a4uNisqsylsUT?=
 =?us-ascii?Q?gp3jLzK6NNeAdpzW1wTMgDgQzAEX87/JzckUlRVq0Bh7yAjozPsZueaa0wcJ?=
 =?us-ascii?Q?q0k1TWYN92pfF+PDk8O11BpicnT93Ih1FzreM5GFYppFnosou86UHbi44lEo?=
 =?us-ascii?Q?aXtXoijkL8p8ImWFXWDuaRA5q4VxoD7he1rWiMz6xUjDd86KITl9ZCFHxPAO?=
 =?us-ascii?Q?3tmlHhkv6C/90NYhMyS+ITHwA0VGq0DMB79jrm/6H01enH5ODvzNztu4CTQx?=
 =?us-ascii?Q?FIKwO/QyDWMjuBFrf+JN+EPIMvKQtUE3WwXJkE46dwW2P0/3TEvxUXbUCNPd?=
 =?us-ascii?Q?p/LaJV01GOUQJDKBCoRXSr8n3lyuctVZK/VTXsimMNtDuARHvEoygB0LctTT?=
 =?us-ascii?Q?JBTtavlP1lcPeNDRaMsIyEhWK2hpzU8G9CJkkrXH9GNPw2emUjaCzpmUJEtP?=
 =?us-ascii?Q?ldWOf2gW1xtnFv7fclSS+8HB/qzIjyqxs9HFO+8hKti5jIrRLywBG5YVlu7l?=
 =?us-ascii?Q?a1M1HGMhOzQRPHfQlgtoItHEYqZHIICrqoJGdT4q5jN5wxtjN6tpaY79Op/G?=
 =?us-ascii?Q?6B352HZOqPmLH2J9kcHHRMrYyyBz/s0R/o5OA4ZK8uVhtZhsde/dFS7JbqgI?=
 =?us-ascii?Q?q/Gdg7z5dbR0arjiU1KgDRNOY2Blxg2y5SzCjLuzgQNR?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d8b697be-65e6-44b0-c3e4-08da65c9c59c
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6311.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jul 2022 18:50:50.5500
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pRmRuusTUVjpE2Pd/y6+lh7iJ9O2JT2DOHhjaXdne+1DQgk9F44GHZwaVPdnNa7FydYZvqTCPByVAa9VDyPtQg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB3581
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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
> > 
> 
>