From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Ben Widawsky <ben.widawsky@intel.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: Thu, 11 Feb 2021 10:01:52 +0000 [thread overview]
Message-ID: <20210211100152.00000667@Huawei.com> (raw)
In-Reply-To: <CAPcyv4i4_6HLNpw7p-1PD9cePuMuPkvUfx0ROT8M0Y7ftxzYfg@mail.gmail.com>
On Wed, 10 Feb 2021 11:54:29 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> > > ...
> > >
> > > > +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.
I think I'd drop it for now - feels like a topic that needs more discussion.
Also, dumping this data to the kernel log isn't exactly elegant - particularly
if we dump a lot more of it. Perhaps tracepoints?
>
> [..]
> > > > 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.
I guess it may depend on timing of this. Personally I think 5.12 may be too aggressive.
As long as Bjorn can take a DVSEC deduplication as an immutable branch then perhaps
during 5.13 this tree can sit on top of that.
Jonathan
next prev parent reply other threads:[~2021-02-11 10:06 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
2021-02-11 10:01 ` Jonathan Cameron [this message]
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=20210211100152.00000667@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=ben.widawsky@intel.com \
--cc=cbrowy@avery-design.com \
--cc=dan.j.williams@intel.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 \
/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).