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 X-Spam-Level: X-Spam-Status: No, score=-13.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1A630C433F5 for ; Tue, 21 Sep 2021 21:59:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F13F361159 for ; Tue, 21 Sep 2021 21:59:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235814AbhIUWA3 (ORCPT ); Tue, 21 Sep 2021 18:00:29 -0400 Received: from mga17.intel.com ([192.55.52.151]:42961 "EHLO mga17.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235828AbhIUWA3 (ORCPT ); Tue, 21 Sep 2021 18:00:29 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10114"; a="203628698" X-IronPort-AV: E=Sophos;i="5.85,311,1624345200"; d="scan'208";a="203628698" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Sep 2021 14:59:00 -0700 X-IronPort-AV: E=Sophos;i="5.85,311,1624345200"; d="scan'208";a="474238807" Received: from ksankar-mobl2.amr.corp.intel.com (HELO intel.com) ([10.252.132.1]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Sep 2021 14:59:00 -0700 Date: Tue, 21 Sep 2021 14:58:58 -0700 From: Ben Widawsky To: Dan Williams Cc: linux-cxl@vger.kernel.org, Alison Schofield , Ira Weiny , Jonathan Cameron , Vishal Verma Subject: Re: [PATCH v2] cxl/core/bus: Document and tighten up decoder APIs Message-ID: <20210921215858.33ll23o52dtpihu7@intel.com> References: <20210915155946.308339-1-ben.widawsky@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-cxl@vger.kernel.org On 21-09-21 14:41:20, Dan Williams wrote: > On Wed, Sep 15, 2021 at 9:00 AM Ben Widawsky wrote: > > > > Since the code to add decoders for switches and endpoints is on the > > horizon, document the recently added interfaces that will be consumed by > > them. > > > > Part of the original version of this patch was subsumed by > > f5786a5aedfc ("cxl/core: Split decoder setup into alloc + add") > > It's dangerous to quote 'pending' commits. The stable commit id for this is now: > > 48667f676189 ("cxl/core: Split decoder setup into alloc + add") Yes, sorry. At the time I thought pending was going to become next but that didn't happen and thus the danger. > > ...I would have just fixed this up on applying, but some notes below. > > This overall feels like a patch that should wait to go in with first > introduction of endpoint decoders. It's fine with me to wait for intro of endpoint decoders. I probably should have requested you just take the kdoc stuff with your last series, but not worth it now. > > > > > Signed-off-by: Ben Widawsky > > --- > > v2: > > - Dropped removal of host from cxl_decoder_add (Ben) > > - Change nr_targets to unsigned int (Jonathan) > > - Move description of 0 special case to param kdoc (Dan, Ben) > > - Reword kdocs to be more accurate (Jonathan, Ben) > > - Add back debug message to decoder_populate_targets (Ben) > > --- > > > > drivers/cxl/core/bus.c | 34 ++++++++++++++++++++++++++++++++-- > > drivers/cxl/cxl.h | 2 ++ > > 2 files changed, 34 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > > index 396252749477..d242afe08402 100644 > > --- a/drivers/cxl/core/bus.c > > +++ b/drivers/cxl/core/bus.c > > @@ -474,6 +474,7 @@ static int decoder_populate_targets(struct cxl_decoder *cxld, > > rc = -ENXIO; > > goto out_unlock; > > } > > + dev_dbg(&cxld->dev, "%s: target: %d\n", dev_name(dport->dport), i); > > Seems like this deserves its own patch since it's not related to $subject. I don't remember why I put this in this patch.... > > > cxld->target[i] = dport; > > } > > > > @@ -483,13 +484,28 @@ static int decoder_populate_targets(struct cxl_decoder *cxld, > > return rc; > > } > > > > -struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > +/** > > + * cxl_decoder_alloc - Allocate a new CXL decoder > > + * @port: owning port of this decoder > > + * @nr_targets: downstream targets accessible by this decoder. All upstream > > + * ports and root ports must have at least 1 target. Endpoint > > + * devices will have 0 targets. Callers wishing to register an > > + * endpoint device should specify 0. > > + * > > + * A port should contain one or more decoders. Each of those decoders enable > > + * some address space for CXL.mem utilization. A decoder is expected to be > > + * configured by the caller before registering. > > + * > > + * Return: A new cxl decoder to be registered by cxl_decoder_add() > > + */ > > +struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, > > + unsigned int nr_targets) > > { > > struct cxl_decoder *cxld; > > struct device *dev; > > int rc = 0; > > > > - if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1) > > + if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets == 0) > > This looks broken as the above text says that 0 is now an allowed > value. Shouldn't this be: > > - if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1) > + if (nr_targets > CXL_DECODER_MAX_INTERLEAVE) > > ...? > Yes. This got messed up. I actually originally intended this change as part of the endpoint decoder series. I'd rather drop this and just change to unsigned int. > > return ERR_PTR(-EINVAL); > > > > cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL); > > @@ -521,6 +537,20 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > } > > EXPORT_SYMBOL_GPL(cxl_decoder_alloc); > > > > +/** > > + * cxl_decoder_add - Add a decoder with targets > > + * @cxld: The cxl decoder allocated by cxl_decoder_alloc() > > + * @target_map: A list of downstream ports that this decoder can direct memory > > + * traffic to. These numbers should correspond with the port number > > + * in the PCIe Link Capabilities structure. > > + * > > + * Certain types of decoders may not have any targets. The main example of this > > + * is an endpoint device. A more awkward example is a hostbridge whose root > > + * ports get hot added (technically possible, though unlikely). > > I don't think this is possible, certainly not worth calling out unless > you want to prompt someone to go audit the PCI side for proper > operation in this case. > > The endpoint note is enough. I ran it by Chet as to whether or not it's technically possible, which it is according to him. I'm fine to drop it though. > > > + * > > + * Return: Negative error code if the decoder wasn't properly configured; else > > + * returns 0. > > + */ > > int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map) > > { > > struct cxl_port *port; > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 7d6b011dd963..e632cc8da091 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -289,6 +289,8 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id, > > struct cxl_decoder *to_cxl_decoder(struct device *dev); > > bool is_root_decoder(struct device *dev); > > struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets); > > +struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, > > + unsigned int nr_targets); > > Why are there now 2 different cxl_decoder_alloc() function signatures? This was unintentional. My apologies for the sloppy patch.