From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 395D8C6379F for ; Wed, 8 Feb 2023 12:30:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BA17C6B0073; Wed, 8 Feb 2023 07:30:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B51386B0074; Wed, 8 Feb 2023 07:30:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A19316B0075; Wed, 8 Feb 2023 07:30:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 8F6046B0073 for ; Wed, 8 Feb 2023 07:30:40 -0500 (EST) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 68C9C1A0D9C for ; Wed, 8 Feb 2023 12:30:40 +0000 (UTC) X-FDA: 80444058240.08.BD7516D Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by imf04.hostedemail.com (Postfix) with ESMTP id CE8614002B for ; Wed, 8 Feb 2023 12:30:36 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=none; spf=pass (imf04.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1675859437; a=rsa-sha256; cv=none; b=TMueeGH/HvV/EffEA/k5JZ2zemNeuhbiWdyzUFbdRmXrSscxLhPSYbF88vAaH4TKSQlxQl J2VwYAhfOUr3m9+CnVQHjFydlT4T7E4eJORf5F1Komk8z29gFROQ8Ubga/5030WU+8KqNa 5oaqUvThBNjBhKLY2WHSGPpsGVq15rE= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=none; spf=pass (imf04.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1675859437; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=qsHoT5lJ8e8w7xx77UK92va33PfT7E2MwTebwxSQ9MQ=; b=XjQ1dan9iFgIDeri689ryH0A/cy231AuoBQqOkyH7D74XFg/gmPzVQS0vNvlYNJ4LSfWlg Sx68bVy19y0OuHkidZHym69Z6qxaWDI9E46va7DfJjk6oqyODBVXxdfGoLduRyECtwIx1c kR2XXkTgYffEj/0YHwHtom8b6734N1A= Received: from lhrpeml500005.china.huawei.com (unknown [172.18.147.206]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4PBfSP3JP0z6801v; Wed, 8 Feb 2023 20:29:09 +0800 (CST) Received: from localhost (10.202.227.76) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.17; Wed, 8 Feb 2023 12:30:32 +0000 Date: Wed, 8 Feb 2023 12:30:31 +0000 From: Jonathan Cameron To: Dan Williams CC: , , , Subject: Re: [PATCH 07/18] cxl/region: Move region-position validation to a helper Message-ID: <20230208123031.00006990@Huawei.com> In-Reply-To: <167564538779.847146.8356062886811511706.stgit@dwillia2-xfh.jf.intel.com> References: <167564534874.847146.5222419648551436750.stgit@dwillia2-xfh.jf.intel.com> <167564538779.847146.8356062886811511706.stgit@dwillia2-xfh.jf.intel.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.227.76] X-ClientProxiedBy: lhrpeml500001.china.huawei.com (7.191.163.213) To lhrpeml500005.china.huawei.com (7.191.163.240) X-CFilter-Loop: Reflected X-Rspam-User: X-Rspamd-Queue-Id: CE8614002B X-Rspamd-Server: rspam01 X-Stat-Signature: xmz4jbcms1zc98cat8pqk68b8rzmi6ie X-HE-Tag: 1675859436-366763 X-HE-Meta: U2FsdGVkX19Wu0cyW8sjlBb/eQCv4MFEe0O6uPmqjyrSgNu4P59nZAuSSquWkrDH+dzxByDWGamG2VVPcCAFrxoV3ip08kueR1wWjqtscFmgeQYflf39WAkITaytoxUzobQ27u2gyp3CR6VUnqC09sFg0LvWhUoj6h+noUHIOJqeHhoLp/9hMkJkQ2T0Xs50+FISYLaFV5oiYsEmn1kdo2nhULWPm3AxS2NtAXr3nHtN93Q0F+/mseZ48Iwh1pCzq4WD65S+x7KzlYGkEuLxA954iuBjoXBws+1YXsTl8sbwvjvSoeX65jUZQeJDbAlVemJla3ePtrscsxnpCyQBY9ifAtPZgoTe73g8gjDWyswRXJr9x1nOIBPY9F9QEQ/4VK4ewgIA913lv6wD2UrA6hnO2i0UJKPXTTlvjPdjn8Jpz5zA72sBbXO/Hj8GT5GJuSB9nurRTSqXIz9S0AKX5uLSn+KLUEHfoLZqf8gj32jPYiMcYc7SLvhiSeRnDFm5Kx2yYrdeGal43jss8TEQpaw3WU5YdXO/w1ILvMBk0GW5DOt/u1vrdceB5n1zGSon6MQtOWyg7EzhHI0kb3oCpPGp3zYYGAPjTyEhuq7j60dalADqgsqGzdPLqS+8zAnxWhLK1N8CSgvKD8GA0E9U6b6gQi5ENobVnjiJ8xVEyuhD/1vFfkWJbftlyyHZNTBSsQ/wPZUaRK35SBBF5DySGc2Mwuw0X4Z6KEONUpKonKipcc+X/Gb1Q9GHpGW6TLVasGzr9lEcdwFmXdC3zgYWg6Feb8D9gsyLhK0a4lN0nVjlAHEHziLBB2kLYqRO51hsFrF6KKsHAuw9Yvo2fWtd0KvmjurwxtY/8YHdgdm7/gPiQbWJ2IQa6D6ZV6ha9Wpt/yWDUn79ogu9DGEOWiCihGkql1JXuJxJdDQXcb5/cfOEFvVcxMstlqef1x1zzJvzLZuAkCj25vcPJD/peQb EcqKR8Ea zTXmICdrGEleryWS5csUU6cgw1aDGvkRdQbN1eH2HthPcHbo4gA6WHCOpBzr8K6TE22PpuTZWGMtBZl7p+qVOJ3zjVygIulwhuy0Htv1u1w4W4g1qPrrn+7sf7Kn8sDfLYb3GaX0xP+HPoOzDEigHtZ2n3iEfgXkeBajHGDJ7yeJNZEYnBchDCUzCjtwL9n/X/uRAZ2A51YzVHrX0B5TZre289eTLn4hoByZENBFPfWhr0B3F0nJW+oiWWcIiYf9N2ncHlf0diURETWZNlX/fFEoyEA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sun, 05 Feb 2023 17:03:07 -0800 Dan Williams wrote: > In preparation for region autodiscovery, that needs all devices > discovered before their relative position in the region can be > determined, consolidate all position dependent validation in a helper. > > Recall that in the on-demand region creation flow the end-user picks the > position of a given endpoint decoder in a region. In the autodiscovery > case the position of an endpoint decoder can only be determined after > all other endpoint decoders that claim to decode the region's address > range have been enumerated and attached. So, in the autodiscovery case > endpoint decoders may be attached before their relative position is > known. Once all decoders arrive, then positions can be determined and > validated with cxl_region_validate_position() the same as user initiated > on-demand creation. > > Signed-off-by: Dan Williams Hi Dan, A few comments inline, but mostly reflect the original code rather than the refactoring you have done in this patch. Jonathan > +static int cxl_region_attach(struct cxl_region *cxlr, > + struct cxl_endpoint_decoder *cxled, int pos) > +{ > + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent); > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > + struct cxl_region_params *p = &cxlr->params; > + struct cxl_port *ep_port, *root_port; > + struct cxl_dport *dport; > + int rc = -ENXIO; > + > + if (cxled->mode != cxlr->mode) { > + dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > + dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); > + return -EINVAL; > + } > + > + if (cxled->mode == CXL_DECODER_DEAD) { > + dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev)); > + return -ENODEV; > + } > + > + /* all full of members, or interleave config not established? */ > + if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) { > + dev_dbg(&cxlr->dev, "region already active\n"); > + return -EBUSY; > + } else if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) { > + dev_dbg(&cxlr->dev, "interleave config missing\n"); > + return -ENXIO; > + } > + > ep_port = cxled_to_port(cxled); > root_port = cxlrd_to_port(cxlrd); > dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge); > @@ -1284,13 +1327,6 @@ static int cxl_region_attach(struct cxl_region *cxlr, > return -ENXIO; > } > > - if (cxlrd->calc_hb(cxlrd, pos) != dport) { > - dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n", > - dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), > - dev_name(&cxlrd->cxlsd.cxld.dev)); > - return -ENXIO; > - } > - In an ideal world, this would have been nice as two patches. One that reorders the various checks so that they are in the order after you have factored things out (easy to review for correctness) then one that factored it out. > if (cxled->cxld.target_type != cxlr->type) { > dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n", > dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), > @@ -1314,12 +1350,13 @@ static int cxl_region_attach(struct cxl_region *cxlr, > return -EINVAL; > } > > - for (iter = ep_port; !is_cxl_root(iter); > - iter = to_cxl_port(iter->dev.parent)) { > - rc = cxl_port_attach_region(iter, cxlr, cxled, pos); > - if (rc) > - goto err; > - } > + rc = cxl_region_validate_position(cxlr, cxled, pos); > + if (rc) > + return rc; > + > + rc = cxl_region_attach_position(cxlr, cxlrd, cxled, dport, pos); > + if (rc) > + return rc; > > p->targets[pos] = cxled; > cxled->pos = pos; More something about original code than the refactoring... I'm not keen on the side effects that aren't unwound in the error paths. p->targets[pos] and cxled->pos are left set. Probably never matters but not elegant or as easy to reason about as it would be if they were cleared in error cases. In particular there is a check on whether p->targets[pos] is set that will result in a dev_dbg even though setting it up actually failed. > @@ -1343,10 +1380,6 @@ static int cxl_region_attach(struct cxl_region *cxlr, > > err_decrement: > p->nr_targets--; > -err: > - for (iter = ep_port; !is_cxl_root(iter); > - iter = to_cxl_port(iter->dev.parent)) > - cxl_port_detach_region(iter, cxlr, cxled); > return rc; > } > >