linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	linux-cxl@vger.kernel.org,
	Linux ACPI <linux-acpi@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-nvdimm <linux-nvdimm@lists.01.org>,
	Linux PCI <linux-pci@vger.kernel.org>,
	Bjorn Helgaas <helgaas@kernel.org>,
	Chris Browy <cbrowy@avery-design.com>,
	Christoph Hellwig <hch@infradead.org>,
	David Hildenbrand <david@redhat.com>,
	David Rientjes <rientjes@google.com>,
	Ira Weiny <ira.weiny@intel.com>, Jon Masters <jcm@jonmasters.org>,
	Rafael Wysocki <rafael.j.wysocki@intel.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	Vishal Verma <vishal.l.verma@intel.com>,
	"John Groves (jgroves)" <jgroves@micron.com>,
	"Kelley, Sean V" <sean.v.kelley@intel.com>
Subject: Re: [PATCH v2 2/8] cxl/mem: Find device capabilities
Date: Wed, 10 Feb 2021 11:54:29 -0800	[thread overview]
Message-ID: <CAPcyv4i4_6HLNpw7p-1PD9cePuMuPkvUfx0ROT8M0Y7ftxzYfg@mail.gmail.com> (raw)
In-Reply-To: <20210210185319.chharluce2ly4cne@intel.com>

On Wed, Feb 10, 2021 at 10:53 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
[..]
> > Christoph raised this in v1, and I agree with him that his would me more compact
> > and readable as
> >
> >       struct range pmem_range;
> >       struct range ram_range;
> >
> > The discussion seemed to get lost without getting resolved that I can see.
> >
>
> I had been waiting for Dan to chime in, since he authored it. I'll change it and
> he can yell if he cares.

No concerns from me.

>
> > > +
> > > +   struct {
> > > +           struct range range;
> > > +   } ram;
> >
> > > +};
> > > +
> > > +#endif /* __CXL_H__ */
> > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > > index 99a6571508df..0a868a15badc 100644
> > > --- a/drivers/cxl/mem.c
> > > +++ b/drivers/cxl/mem.c
> >
> >
> > ...
> >
> > > +static void cxl_mem_mbox_timeout(struct cxl_mem *cxlm,
> > > +                            struct mbox_cmd *mbox_cmd)
> > > +{
> > > +   struct device *dev = &cxlm->pdev->dev;
> > > +
> > > +   dev_dbg(dev, "Mailbox command (opcode: %#x size: %zub) timed out\n",
> > > +           mbox_cmd->opcode, mbox_cmd->size_in);
> > > +
> > > +   if (IS_ENABLED(CONFIG_CXL_MEM_INSECURE_DEBUG)) {
> >
> > Hmm.  Whilst I can see the advantage of this for debug, I'm not sure we want
> > it upstream even under a rather evil looking CONFIG variable.
> >
> > Is there a bigger lock we can use to avoid chance of accidental enablement?
>
> Any suggestions? I'm told this functionality was extremely valuable for NVDIMM,
> though I haven't personally experienced it.

Yeah, there was no problem with the identical mechanism in LIBNVDIMM
land. However, I notice that the useful feature for LIBNVDIMM is the
option to dump all payloads. This one only fires on timeouts which is
less useful. So I'd say fix it to dump all payloads on the argument
that the safety mechanism was proven with the LIBNVDIMM precedent, or
delete it altogether to maintain v5.12 momentum. Payload dumping can
be added later.

[..]
> > > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > > index e709ae8235e7..6267ca9ae683 100644
> > > --- a/include/uapi/linux/pci_regs.h
> > > +++ b/include/uapi/linux/pci_regs.h
> > > @@ -1080,6 +1080,7 @@
> > >
> > >  /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
> > >  #define PCI_DVSEC_HEADER1          0x4 /* Designated Vendor-Specific Header1 */
> > > +#define PCI_DVSEC_HEADER1_LENGTH_MASK      0xFFF00000
> >
> > Seems sensible to add the revision mask as well.
> > The vendor id currently read using a word read rather than dword, but perhaps
> > neater to add that as well for completeness?
> >
> > Having said that, given Bjorn's comment on clashes and the fact he'd rather see
> > this stuff defined in drivers and combined later (see review patch 1 and follow
> > the link) perhaps this series should not touch this header at all.
>
> I'm fine to move it back.

Yeah, we're playing tennis now between Bjorn's and Christoph's
comments, but I like Bjorn's suggestion of "deduplicate post merge"
given the bloom of DVSEC infrastructure landing at the same time.

  reply	other threads:[~2021-02-10 19:55 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-10  0:02 [PATCH v2 0/8] CXL 2.0 Support Ben Widawsky
2021-02-10  0:02 ` [PATCH v2 1/8] cxl/mem: Introduce a driver for CXL-2.0-Type-3 endpoints Ben Widawsky
2021-02-10 16:17   ` Jonathan Cameron
2021-02-10 17:12     ` Ben Widawsky
2021-02-10 17:23       ` Jonathan Cameron
2021-02-10  0:02 ` [PATCH v2 2/8] cxl/mem: Find device capabilities Ben Widawsky
2021-02-10 13:32   ` Jonathan Cameron
2021-02-10 15:07     ` Jonathan Cameron
2021-02-10 16:55       ` Ben Widawsky
2021-02-10 17:30         ` Jonathan Cameron
2021-02-10 18:16         ` Ben Widawsky
2021-02-11  9:55           ` Jonathan Cameron
2021-02-11 15:55             ` Ben Widawsky
2021-02-12 13:27               ` Jonathan Cameron
2021-02-12 15:54                 ` Ben Widawsky
2021-02-11 18:27             ` Ben Widawsky
2021-02-12 13:23               ` Jonathan Cameron
2021-02-10 19:32     ` Ben Widawsky
2021-02-10 17:41   ` Jonathan Cameron
2021-02-10 18:53     ` Ben Widawsky
2021-02-10 19:54       ` Dan Williams [this message]
2021-02-11 10:01         ` Jonathan Cameron
2021-02-11 16:04           ` Ben Widawsky
2021-02-10  0:02 ` [PATCH v2 3/8] cxl/mem: Register CXL memX devices Ben Widawsky
2021-02-10 18:17   ` Jonathan Cameron
2021-02-11 10:17     ` Jonathan Cameron
2021-02-11 20:40       ` Dan Williams
2021-02-12 13:33         ` Jonathan Cameron
2021-02-10  0:02 ` [PATCH v2 4/8] cxl/mem: Add basic IOCTL interface Ben Widawsky
2021-02-10 18:45   ` Jonathan Cameron
2021-02-10 20:22     ` Ben Widawsky
2021-02-11  4:40     ` Dan Williams
2021-02-11 10:06       ` Jonathan Cameron
2021-02-11 16:54         ` Ben Widawsky
2021-02-14 16:30   ` Al Viro
2021-02-14 23:14     ` Ben Widawsky
2021-02-14 23:50       ` Al Viro
2021-02-14 23:57         ` Al Viro
2021-02-10  0:02 ` [PATCH v2 5/8] cxl/mem: Add a "RAW" send command Ben Widawsky
2021-02-10 15:26   ` Ariel.Sibley
2021-02-10 16:49     ` Ben Widawsky
2021-02-10 18:03       ` Ariel.Sibley
2021-02-10 18:11         ` Ben Widawsky
2021-02-10 18:46           ` Ariel.Sibley
2021-02-10 19:12             ` Ben Widawsky
2021-02-11 16:43     ` Dan Williams
2021-02-11 11:19   ` Jonathan Cameron
2021-02-11 16:01     ` Ben Widawsky
2021-02-12 13:40       ` Jonathan Cameron
2021-02-10  0:02 ` [PATCH v2 6/8] cxl/mem: Enable commands via CEL Ben Widawsky
2021-02-11 12:02   ` Jonathan Cameron
2021-02-11 17:45     ` Ben Widawsky
2021-02-11 20:34       ` Dan Williams
2021-02-16 13:43     ` Bartosz Golaszewski
2021-02-10  0:02 ` [PATCH v2 7/8] cxl/mem: Add set of informational commands Ben Widawsky
2021-02-11 12:07   ` Jonathan Cameron
2021-02-10  0:02 ` [PATCH v2 8/8] MAINTAINERS: Add maintainers of the CXL driver Ben Widawsky

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=CAPcyv4i4_6HLNpw7p-1PD9cePuMuPkvUfx0ROT8M0Y7ftxzYfg@mail.gmail.com \
    --to=dan.j.williams@intel.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=ben.widawsky@intel.com \
    --cc=cbrowy@avery-design.com \
    --cc=david@redhat.com \
    --cc=hch@infradead.org \
    --cc=helgaas@kernel.org \
    --cc=ira.weiny@intel.com \
    --cc=jcm@jonmasters.org \
    --cc=jgroves@micron.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=rdunlap@infradead.org \
    --cc=rientjes@google.com \
    --cc=sean.v.kelley@intel.com \
    --cc=vishal.l.verma@intel.com \
    --subject='Re: [PATCH v2 2/8] cxl/mem: Find device capabilities' \
    /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

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