nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>, kernel test robot <lkp@intel.com>,
	"Vishal L Verma" <vishal.l.verma@intel.com>,
	"Schofield, Alison" <alison.schofield@intel.com>,
	Linux NVDIMM <nvdimm@lists.linux.dev>,
	"Weiny, Ira" <ira.weiny@intel.com>,
	Ben Widawsky <ben.widawsky@intel.com>
Subject: Re: [PATCH v3 28/28] cxl/core: Split decoder setup into alloc + add
Date: Fri, 3 Sep 2021 19:01:33 +0100	[thread overview]
Message-ID: <20210903190133.000003e2@Huawei.com> (raw)
In-Reply-To: <CAPcyv4iaff7_YH1OG-yn4vnDbh-QF1DgLdGr8E4LT1bBBvX-yQ@mail.gmail.com>

On Fri, 3 Sep 2021 09:26:09 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Fri, Sep 3, 2021 at 6:34 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Tue, 24 Aug 2021 09:07:56 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >  
> > > The kbuild robot reports:
> > >
> > >     drivers/cxl/core/bus.c:516:1: warning: stack frame size (1032) exceeds
> > >     limit (1024) in function 'devm_cxl_add_decoder'
> > >
> > > It is also the case the devm_cxl_add_decoder() is unwieldy to use for
> > > all the different decoder types. Fix the stack usage by splitting the
> > > creation into alloc and add steps. This also allows for context
> > > specific construction before adding.
> > >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> >
> > Trivial comment inline - otherwise looks like a nice improvement.
> >
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> >  
> > > ---
> > >  drivers/cxl/acpi.c     |   74 ++++++++++++++++++++---------
> > >  drivers/cxl/core/bus.c |  124 +++++++++++++++---------------------------------
> > >  drivers/cxl/cxl.h      |   15 ++----
> > >  3 files changed, 95 insertions(+), 118 deletions(-)
> > >  
> >  
> > > @@ -268,6 +275,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > >       struct cxl_port *port;
> > >       struct cxl_dport *dport;
> > >       struct cxl_decoder *cxld;
> > > +     int single_port_map[1], rc;
> > >       struct cxl_walk_context ctx;
> > >       struct acpi_pci_root *pci_root;
> > >       struct cxl_port *root_port = arg;
> > > @@ -301,22 +309,42 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > >               return -ENODEV;
> > >       if (ctx.error)
> > >               return ctx.error;
> > > +     if (ctx.count > 1)
> > > +             return 0;
> > >
> > >       /* TODO: Scan CHBCR for HDM Decoder resources */
> > >
> > >       /*
> > > -      * In the single-port host-bridge case there are no HDM decoders
> > > -      * in the CHBCR and a 1:1 passthrough decode is implied.
> > > +      * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability
> > > +      * Structure) single ported host-bridges need not publish a decoder
> > > +      * capability when a passthrough decode can be assumed, i.e. all
> > > +      * transactions that the uport sees are claimed and passed to the single
> > > +      * dport. Default the range a 0-base 0-length until the first CXL region
> > > +      * is activated.
> > >        */  
> >
> > Is comment in right place or should it be up with the ctx.count > 1  
> 
> This comment is specifically about the implicit decoder, right beneath
> the comment, that is registered in the ctx.count == 1 case. Perhaps
> you were reacting to the spec reference which is generic, but later
> sentences make it clear this comment is about an exception noted in
> that spec reference?

More that the conditional is above that leads to us getting here. 
Probably thrown off by Diff having added the new block that follows this
after the removed block rather than before it.

I'm fine with how you have it here.

J

  reply	other threads:[~2021-09-03 18:01 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 16:05 [PATCH v3 00/28] cxl_test: Enable CXL Topology and UAPI regression tests Dan Williams
2021-08-24 16:05 ` [PATCH v3 01/28] libnvdimm/labels: Introduce getters for namespace label fields Dan Williams
2021-08-24 16:05 ` [PATCH v3 02/28] libnvdimm/labels: Add isetcookie validation helper Dan Williams
2021-08-24 16:05 ` [PATCH v3 03/28] libnvdimm/labels: Introduce label setter helpers Dan Williams
2021-08-24 16:05 ` [PATCH v3 04/28] libnvdimm/labels: Add a checksum calculation helper Dan Williams
2021-08-24 16:05 ` [PATCH v3 05/28] libnvdimm/labels: Add blk isetcookie set / validation helpers Dan Williams
2021-08-24 16:05 ` [PATCH v3 06/28] libnvdimm/labels: Add blk special cases for nlabel and position helpers Dan Williams
2021-08-24 16:06 ` [PATCH v3 07/28] libnvdimm/labels: Add type-guid helpers Dan Williams
2021-08-24 16:06 ` [PATCH v3 08/28] libnvdimm/labels: Add claim class helpers Dan Williams
2021-08-24 16:06 ` [PATCH v3 09/28] libnvdimm/labels: Add address-abstraction uuid definitions Dan Williams
2021-08-24 16:06 ` [PATCH v3 10/28] libnvdimm/labels: Add uuid helpers Dan Williams
2021-08-24 16:06 ` [PATCH v3 11/28] libnvdimm/label: Add a helper for nlabel validation Dan Williams
2021-09-02 16:37   ` Jonathan Cameron
2021-08-24 16:06 ` [PATCH v3 12/28] libnvdimm/labels: Introduce the concept of multi-range namespace labels Dan Williams
2021-09-02 16:43   ` Jonathan Cameron
2021-08-24 16:06 ` [PATCH v3 13/28] libnvdimm/label: Define CXL region labels Dan Williams
2021-09-02 16:36   ` Jonathan Cameron
2021-09-02 16:41     ` Jonathan Cameron
2021-09-03  3:58       ` Dan Williams
2021-08-24 16:06 ` [PATCH v3 14/28] libnvdimm/labels: Introduce CXL labels Dan Williams
2021-09-03 17:00   ` Dan Williams
2021-08-24 16:06 ` [PATCH v3 15/28] cxl/pci: Make 'struct cxl_mem' device type generic Dan Williams
2021-09-02 16:55   ` Jonathan Cameron
2021-09-02 17:34     ` Dan Williams
2021-08-24 16:06 ` [PATCH v3 16/28] cxl/mbox: Introduce the mbox_send operation Dan Williams
2021-09-02 17:07   ` Jonathan Cameron
2021-08-24 16:06 ` [PATCH v3 17/28] cxl/mbox: Move mailbox and other non-PCI specific infrastructure to the core Dan Williams
2021-09-02 17:56   ` Jonathan Cameron
2021-09-02 18:56     ` Dan Williams
2021-08-24 16:07 ` [PATCH v3 18/28] cxl/pci: Use module_pci_driver Dan Williams
2021-09-02 17:58   ` Jonathan Cameron
2021-08-24 16:07 ` [PATCH v3 19/28] cxl/mbox: Convert 'enabled_cmds' to DECLARE_BITMAP Dan Williams
2021-09-02 17:59   ` Jonathan Cameron
2021-08-24 16:07 ` [PATCH v3 20/28] cxl/mbox: Add exclusive kernel command support Dan Williams
2021-09-02 18:09   ` Jonathan Cameron
2021-09-03 20:47     ` Dan Williams
2021-08-24 16:07 ` [PATCH v3 21/28] cxl/pmem: Translate NVDIMM label commands to CXL label commands Dan Williams
2021-09-02 18:22   ` Jonathan Cameron
2021-09-03 21:09     ` Dan Williams
2021-08-24 16:07 ` [PATCH v3 22/28] cxl/pmem: Add support for multiple nvdimm-bridge objects Dan Williams
2021-09-03 11:15   ` Jonathan Cameron
2021-08-24 16:07 ` [PATCH v3 23/28] cxl/acpi: Do not add DSDT disabled ACPI0016 host bridge ports Dan Williams
2021-09-02 18:30   ` Jonathan Cameron
2021-09-03 17:51     ` Dan Williams
2021-08-24 16:07 ` [PATCH v3 24/28] tools/testing/cxl: Introduce a mocked-up CXL port hierarchy Dan Williams
2021-09-03 12:52   ` Jonathan Cameron
2021-09-03 21:49     ` Dan Williams
2021-09-06  8:32       ` Jonathan Cameron
2021-09-07 15:57         ` Dan Williams
2021-08-24 16:07 ` [PATCH v3 25/28] cxl/bus: Populate the target list at decoder create Dan Williams
2021-09-03 12:59   ` Jonathan Cameron
2021-09-03 22:43     ` Dan Williams
2021-09-06  8:52       ` Jonathan Cameron
2021-08-24 16:07 ` [PATCH v3 26/28] cxl/mbox: Move command definitions to common location Dan Williams
2021-09-03 13:04   ` Jonathan Cameron
2021-08-24 16:07 ` [PATCH v3 27/28] tools/testing/cxl: Introduce a mock memory device + driver Dan Williams
2021-09-03 13:21   ` Jonathan Cameron
2021-09-03 23:33     ` Dan Williams
2021-09-06  8:57       ` Jonathan Cameron
2021-08-24 16:07 ` [PATCH v3 28/28] cxl/core: Split decoder setup into alloc + add Dan Williams
2021-09-03 13:33   ` Jonathan Cameron
2021-09-03 16:26     ` Dan Williams
2021-09-03 18:01       ` Jonathan Cameron [this message]
2021-09-04  0:27         ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210903190133.000003e2@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=alison.schofield@intel.com \
    --cc=ben.widawsky@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=ira.weiny@intel.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=lkp@intel.com \
    --cc=nvdimm@lists.linux.dev \
    --cc=vishal.l.verma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).