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 C68A8C05027 for ; Fri, 10 Feb 2023 18:10:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3405828000B; Fri, 10 Feb 2023 13:10:06 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2F0E1280003; Fri, 10 Feb 2023 13:10:06 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1B88028000B; Fri, 10 Feb 2023 13:10:06 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 09025280003 for ; Fri, 10 Feb 2023 13:10:06 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id B699AAB899 for ; Fri, 10 Feb 2023 18:10:05 +0000 (UTC) X-FDA: 80452171170.21.D326B04 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by imf28.hostedemail.com (Postfix) with ESMTP id 6CAA8C0017 for ; Fri, 10 Feb 2023 18:10:03 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=none; spf=pass (imf28.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=1676052603; 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=SFgAhOZv2hLV615st9NyhF9+nVz/WxoUW/u4YbKgMAA=; b=nrPc6mqADtda0cqrEk3rksYdxYRnQ9KEymVLRn1KZKiwGuiE+stLeGXeYvaO/fgQL7FQ3S Euglq5dcDoYi/ktcIkNCiw5Xnmg9YjgJ0x64RckVKQN+knQf7tOzD76+D+ue5NOzvby53N p8UPgCbpHZ6P88vwplrFxQ5qd0+mvoE= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=none; spf=pass (imf28.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=1676052603; a=rsa-sha256; cv=none; b=p3WYoaY77+2e/E4g5NQEhPRC4r/MlBsisC2YXSSeMJMW/QO2byxhMXgxtqCSKGyO6Hx908 bOjJKZzCSQSAqF+G46Gfqz569yqUeICJgwn7xtygUmubdKNGvaAVADYO4jM1m5+AhtE15c 823KOdq5VpdR3L+cBQV/rGZJau4+4iE= Received: from lhrpeml500005.china.huawei.com (unknown [172.18.147.200]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4PD1qy4DPdz67xhs; Sat, 11 Feb 2023 02:05:50 +0800 (CST) Received: from localhost (10.81.210.211) 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; Fri, 10 Feb 2023 18:10:00 +0000 Date: Fri, 10 Feb 2023 18:09:58 +0000 From: Jonathan Cameron To: Dan Williams CC: , Fan Ni , , , , Subject: Re: [PATCH v2 13/20] cxl/region: Add region autodiscovery Message-ID: <20230210180958.00002e5a@Huawei.com> In-Reply-To: <167601999958.1924368.9366954455835735048.stgit@dwillia2-xfh.jf.intel.com> References: <167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf.intel.com> <167601999958.1924368.9366954455835735048.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.81.210.211] X-ClientProxiedBy: lhrpeml100004.china.huawei.com (7.191.162.219) To lhrpeml500005.china.huawei.com (7.191.163.240) X-CFilter-Loop: Reflected X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: 5o1qax1zudwandby8779okpswa8h5eak X-Rspamd-Queue-Id: 6CAA8C0017 X-HE-Tag: 1676052603-884624 X-HE-Meta: U2FsdGVkX1/lBWhKBNuMNiUEDo1vaGHLVfdkzJq9iXjwQ8yusxYDOXji02vG5ffAjTSO5xsy3FCBD3lvtdIghp8vzY+9D64wuDM8TlcyZb9VtvvzPWwCAWwpf1SF4AP3vcwlgRnKor7PS3H/pTQN1SF1ZGTmpd53MdYRXjJmabzS5umfoS4icz/0wH6LbFaMjhshKfHFFoL4E9iT/2LF82m51GhPxg99/tPjQIc2yH4pZpGUKlg+B/hM/tAxD96hvMcwOd50ZMjqdpljmqnLYe/klnmpN2LB7hXM1oFhIVh2YxH/7/9JkJ6m0QhGne70m4Eyd0kyq0Jw5ewLmWFhecD/4Mf/f1yPocotG03J5u/nIGOZN4EcEBNPqe3bzfh16niE1i92mHPUQ1Wbr7Gp8c2YcRE/RMy/mAegEXnnVhHbogBHulrm/8vxPNlrmskGNypGGjPJDNQmsMQOaoJ26WV1UofEnAZsuofvQmlgO401SYajABxhuNO0HKsTKSh7XeAyFcQbM7TnCVLqO8RImTH3lzVJn4lcnvMkFxcFpuyrEOmu4lgPNbaQh2cSEG/KdaTSZPh0/AsCFBrCe0lkmpdcd9THaSykJo0IP38SO/dA1+IoUHF/WrptxGQPzTcZfWpcs8Eh49zB/hVNoembtcTuzZ5UJtD3dmbTR7zK/Uoqur08IiBy56dfRXLBYS38/HsqnxgWrWEG6fpsZT0L2cSZYSP1zRj0xlYMMC9P+K+WovEBVfmDShCxYp7R9Amd5rsyT7GSUd8rfiMoOkeMWZzNM4fbB3yuCuxPWD6w8SCJ2/hJObwe27yCrCYVNEgJoPtUolBNYWfjOGHSsXro4FH80HNkWwpH2Sck3rpA43XIadxBls0gHa+lqGCSCTodc/7ODRtXrviPGFlUPBJ099mS/ucymvlItI7Q1+HRTqcp8SNPthpkcNScNQCy7B+PDPPjMHKTV4GeXEZbqdq Rnj674zr TNdjhxw1fQbF3h+Zq9twc9Ag0oymJnQcLe659Nr6GsW2t7lOXUjwHzINmOXT4eZ4HG8qNaZRi0/a/LqjCVJPTZAIj/pxZqQg/EyHX1OOyuCbfLnevGcTbCVu/yHOKRnoL/s+2BU6ovz4JPtUgzdKu0yeJdH2iTni3PwiNk0U6x03a4pUDAWm4b2obWCrkHuSBHQosWV1t9FmKq63yrXQzUMTexZSbC0KONz5Fd89hEG3IuF1jYCfofD8faUyuX/Pk7iP/lMxUA5kIulEr2osIfrP9e9kjIpFGuYMujlcmPaWEb6V9AddgCofPsYR5QzwudWmoR1VtwTrJbA56nbri55LTKCGKWqIqiWjI 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 Fri, 10 Feb 2023 01:06:39 -0800 Dan Williams wrote: > Region autodiscovery is an asynchronous state machine advanced by > cxl_port_probe(). After the decoders on an endpoint port are enumerated > they are scanned for actively enabled instances. Each active decoder is > flagged for auto-assembly CXL_DECODER_F_AUTO and attached to a region. > If a region does not already exist for the address range setting of the > decoder one is created. That creation process may race with other > decoders of the same region being discovered since cxl_port_probe() is > asynchronous. A new 'struct cxl_root_decoder' lock, @range_lock, is > introduced to mitigate that race. > > Once all decoders have arrived, "p->nr_targets == p->interleave_ways", > they are sorted by their relative decode position. The sort algorithm > involves finding the point in the cxl_port topology where one leg of the > decode leads to deviceA and the other deviceB. At that point in the > topology the target order in the 'struct cxl_switch_decoder' indicates > the relative position of those endpoint decoders in the region. > > >From that point the region goes through the same setup and validation Why the >? > steps as user-created regions, but instead of programming the decoders > it validates that driver would have written the same values to the > decoders as were already present. > > Tested-by: Fan Ni > Link: https://lore.kernel.org/r/167564540972.847146.17096178433176097831.stgit@dwillia2-xfh.jf.intel.com > Signed-off-by: Dan Williams A few trivial things inline and this being complex code I'm not as confident about it as the rest of the series but with that in mind and the fact I didn't find anything that looked broken... Reviewed-by: Jonathan Cameron ... > + > +static int cxl_region_sort_targets(struct cxl_region *cxlr) > +{ > + struct cxl_region_params *p = &cxlr->params; > + int i, rc = 0; > + > + sort(p->targets, p->nr_targets, sizeof(p->targets[0]), cmp_decode_pos, > + NULL); > + > + for (i = 0; i < p->nr_targets; i++) { > + struct cxl_endpoint_decoder *cxled = p->targets[i]; > + > + if (cxled->pos < 0) > + rc = -ENXIO; If it makes sense to carry on after pos < 0 I'd like to see a comment here on why. If not, nicer to have a separate dev_dbg() for failed case nad direct return here. > + cxled->pos = i; > + } > + > + dev_dbg(&cxlr->dev, "region sort %s\n", rc ? "failed" : "successful"); > + return rc; > +} > + > + > +int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled) > +{ > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > + struct range *hpa = &cxled->cxld.hpa_range; > + struct cxl_decoder *cxld = &cxled->cxld; > + struct cxl_root_decoder *cxlrd; > + struct cxl_region_params *p; > + struct cxl_region *cxlr; > + bool attach = false; > + struct device *dev; > + int rc; > + > + dev = device_find_child(&root->dev, &cxld->hpa_range, > + match_decoder_by_range); > + if (!dev) { > + dev_err(cxlmd->dev.parent, > + "%s:%s no CXL window for range %#llx:%#llx\n", > + dev_name(&cxlmd->dev), dev_name(&cxld->dev), > + cxld->hpa_range.start, cxld->hpa_range.end); > + return -ENXIO; > + } > + > + cxlrd = to_cxl_root_decoder(dev); > + > + /* > + * Ensure that if multiple threads race to construct_region() for @hpa > + * one does the construction and the others add to that. > + */ > + mutex_lock(&cxlrd->range_lock); > + dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa, > + match_region_by_range); > + if (!dev) > + cxlr = construct_region(cxlrd, cxled); > + else > + cxlr = to_cxl_region(dev); > + mutex_unlock(&cxlrd->range_lock); > + > + if (IS_ERR(cxlr)) { > + rc = PTR_ERR(cxlr); > + goto out; > + } > + > + attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE); > + > + down_read(&cxl_region_rwsem); > + p = &cxlr->params; > + attach = p->state == CXL_CONFIG_COMMIT; > + up_read(&cxl_region_rwsem); > + > + if (attach) { > + int rc = device_attach(&cxlr->dev); Shadowing int rc isn't great for readability. Just call it rc2 or something :) Or given you don't make use of the value... /* * If device_attach() fails the range may still be active via * the platform-firmware memory map, otherwise the driver for * regions is local to this file, so driver matching can't fail + * and hence device_attach() cannot return 1. //very much not obvious otherwise to anyone who isn't far too familiar with device_attach() */ if (device_attach(&cxlr->dev) < 0) dev_err() > + > + /* > + * If device_attach() fails the range may still be active via > + * the platform-firmware memory map, otherwise the driver for > + * regions is local to this file, so driver matching can't fail. > + */ > + if (rc < 0) > + dev_err(&cxlr->dev, "failed to enable, range: %pr\n", > + p->res); > + } > + > + put_device(&cxlr->dev); > +out: > + put_device(&cxlrd->cxlsd.cxld.dev); Moderately horrible. Maybe just keep an extra local variable around for the first use of struct device *dev? or maybe add a put_cxl_root_decoder() helper? There are lots of other deep structure access like this I guess, so I don't mind if you just leave this as yet another one. > + return rc; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, CXL); ... > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > index a8d46a67b45e..d88518836c2d 100644 > --- a/drivers/cxl/port.c > +++ b/drivers/cxl/port.c > @@ -30,6 +30,34 @@ static void schedule_detach(void *cxlmd) > schedule_cxl_memdev_detach(cxlmd); > } > > +static int discover_region(struct device *dev, void *root) > +{ > + struct cxl_endpoint_decoder *cxled; > + int rc; > + > + if (!is_endpoint_decoder(dev)) > + return 0; > + > + cxled = to_cxl_endpoint_decoder(dev); > + if ((cxled->cxld.flags & CXL_DECODER_F_ENABLE) == 0) > + return 0; > + > + if (cxled->state != CXL_DECODER_STATE_AUTO) > + return 0; > + > + /* > + * Region enumeration is opportunistic, if this add-event fails, > + * continue to the next endpoint decoder. > + */ > + rc = cxl_add_to_region(root, cxled); > + if (rc) > + dev_dbg(dev, "failed to add to region: %#llx-%#llx\n", > + cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end); > + > + return 0; > +} > + > + Two blank lines? > static int cxl_switch_port_probe(struct cxl_port *port) > {