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>,
	Ben Widawsky <ben.widawsky@intel.com>,
	"kernel test robot" <lkp@intel.com>, <vishal.l.verma@intel.com>,
	<alison.schofield@intel.com>, <nvdimm@lists.linux.dev>,
	<ira.weiny@intel.com>
Subject: Re: [PATCH v3 17/28] cxl/mbox: Move mailbox and other non-PCI specific infrastructure to the core
Date: Thu, 2 Sep 2021 18:56:06 +0100	[thread overview]
Message-ID: <20210902185606.0000731b@Huawei.com> (raw)
In-Reply-To: <162982121720.1124374.4630115550776741892.stgit@dwillia2-desk3.amr.corp.intel.com>

On Tue, 24 Aug 2021 09:06:57 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Now that the internals of mailbox operations are abstracted from the PCI
> specifics a bulk of infrastructure can move to the core.
> 
> The CXL_PMEM driver intends to proxy LIBNVDIMM UAPI and driver requests
> to the equivalent functionality provided by the CXL hardware mailbox
> interface. In support of that intent move the mailbox implementation to
> a shared location for the CXL_PCI driver native IOCTL path and CXL_PMEM
> nvdimm command proxy path to share.
> 
> A unit test framework seeks to implement a unit test backend transport
> for mailbox commands to communicate mocked up payloads. It can reuse all
> of the mailbox infrastructure minus the PCI specifics, so that also gets
> moved to the core.
> 
> Finally with the mailbox infrastructure and ioctl handling being
> transport generic there is no longer any need to pass file
> file_operations to devm_cxl_add_memdev(). That allows all the ioctl
> boilerplate to move into the core for unit test reuse.
> 
> No functional change intended, just code movement.

This didn't give me the warm fuzzy feeling of a straight forward move patch.
A few oddities below, but more generally can you break this up a bit.
That "Finally" for examples feels like it could be done as a precursor to this.

This also has the updated docs thing I commented on in an earlier patch
that needs moving back to that patch.

Jonathan


> 
> Acked-by: Ben Widawsky <ben.widawsky@intel.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  Documentation/driver-api/cxl/memory-devices.rst |    3 
>  drivers/cxl/core/Makefile                       |    1 
>  drivers/cxl/core/bus.c                          |    4 
>  drivers/cxl/core/core.h                         |    8 
>  drivers/cxl/core/mbox.c                         |  834 ++++++++++++++++++++
>  drivers/cxl/core/memdev.c                       |   81 ++
>  drivers/cxl/cxlmem.h                            |   81 ++
>  drivers/cxl/pci.c                               |  941 -----------------------
>  8 files changed, 987 insertions(+), 966 deletions(-)
>  create mode 100644 drivers/cxl/core/mbox.c



>  
>  #endif /* __CXL_CORE_H__ */
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> new file mode 100644
> index 000000000000..706fe007c8d6
> --- /dev/null
> +++ b/drivers/cxl/core/mbox.c
> @@ -0,0 +1,834 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2020 Intel Corporation. All rights reserved. */
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/security.h>
> +#include <linux/debugfs.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>

Given stated aim is to have this pci free, I doubt this header is required!

> +#include <cxlmem.h>
> +#include <cxl.h>
> +
> +#include "core.h"
> +
> +static bool cxl_raw_allow_all;
> +
> +/**
> + * DOC: cxl mbox
> + *
> + * Core implementation of the CXL 2.0 Type-3 Memory Device Mailbox. The
> + * implementation is used by the cxl_pci driver to initialize the device
> + * and implement the cxl_mem.h IOCTL UAPI. It also implements the
> + * backend of the cxl_pmem_ctl() transport for LIBNVDIMM.
> + *

Trivial: No need for the last blank line.

> + */
> +
> +#define cxl_for_each_cmd(cmd)                                                  \
> +	for ((cmd) = &cxl_mem_commands[0];                                     \
> +	     ((cmd)-cxl_mem_commands) < ARRAY_SIZE(cxl_mem_commands); (cmd)++)

Spaces around the - 

> +
> +#define cxl_doorbell_busy(cxlm)                                                \
> +	(readl((cxlm)->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET) &                  \
> +	 CXLDEV_MBOX_CTRL_DOORBELL)

I think we now have two copies of this which isn't ideal.
Something gone wrong with moving this?

> +
> +/* CXL 2.0 - 8.2.8.4 */
> +#define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)

Also this which still seems to be in pci.c

...

> +/**
> + * cxl_mem_get_partition_info - Get partition info
> + * @cxlm: The device to act on
> + * @active_volatile_bytes: returned active volatile capacity; in bytes
> + * @active_persistent_bytes: returned active persistent capacity; in bytes
> + * @next_volatile_bytes: return next volatile capacity; in bytes
> + * @next_persistent_bytes: return next persistent capacity; in bytes
> + *
> + * Retrieve the current partition info for the device specified.  The active
> + * values are the current capacity in bytes.  If not 0, the 'next' values are

No problem with the updated comment, but shouldn't be in this patch without
at least being called out.

> + * the pending values, in bytes, which take affect on next cold reset.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL @8.2.9.5.2.1 Get Partition Info
> + */
> +static int cxl_mem_get_partition_info(struct cxl_mem *cxlm)
> +{
> +	struct cxl_mbox_get_partition_info {
> +		__le64 active_volatile_cap;
> +		__le64 active_persistent_cap;
> +		__le64 next_volatile_cap;
> +		__le64 next_persistent_cap;
> +	} __packed pi;
> +	int rc;
> +
> +	rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
> +				   NULL, 0, &pi, sizeof(pi));
> +
> +	if (rc)
> +		return rc;
> +
> +	cxlm->active_volatile_bytes =
> +		le64_to_cpu(pi.active_volatile_cap) * CXL_CAPACITY_MULTIPLIER;
> +	cxlm->active_persistent_bytes =
> +		le64_to_cpu(pi.active_persistent_cap) * CXL_CAPACITY_MULTIPLIER;
> +	cxlm->next_volatile_bytes =
> +		le64_to_cpu(pi.next_volatile_cap) * CXL_CAPACITY_MULTIPLIER;
> +	cxlm->next_persistent_bytes =
> +		le64_to_cpu(pi.next_volatile_cap) * CXL_CAPACITY_MULTIPLIER;

I'd have kept this bit of cleanup separate. For a code move patch I don't want
to have to spot the places where things weren't just a move. Same in other places.

Not a bit thing though so if you don't want to pull these out separately then
I don't mind that much.

> +
> +	return 0;
> +}

> +
> +struct cxl_mem *cxl_mem_create(struct device *dev)

The parameter change from struct pci_dev * is a bit more than I'd
expect in a code move patch. I would have done that in a precursor if possible.

> +{
> +	struct cxl_mem *cxlm;
> +
> +	cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL);
> +	if (!cxlm)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_init(&cxlm->mbox_mutex);
> +	cxlm->dev = dev;
> +	cxlm->enabled_cmds =
> +		devm_kmalloc_array(dev, BITS_TO_LONGS(cxl_cmd_count),
> +				   sizeof(unsigned long),
> +				   GFP_KERNEL | __GFP_ZERO);
> +	if (!cxlm->enabled_cmds)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return cxlm;
> +}
> +EXPORT_SYMBOL_GPL(cxl_mem_create);
> +

...


> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index a56d8f26a157..b7122ded3a04 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -2,6 +2,7 @@
>  /* Copyright(c) 2020-2021 Intel Corporation. */
>  #ifndef __CXL_MEM_H__
>  #define __CXL_MEM_H__
> +#include <uapi/linux/cxl_mem.h>
>  #include <linux/cdev.h>
>  #include "cxl.h"
>  
> @@ -28,21 +29,6 @@
>  	(FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) !=                       \
>  	 CXLMDEV_RESET_NEEDED_NOT)
>  

...

>  /**
> - * struct mbox_cmd - A command to be submitted to hardware.
> + * struct cxl_mbox_cmd - A command to be submitted to hardware.

Ah. Here it is ;)  Move to earlier patch.

>   * @opcode: (input) The command set and command submitted to hardware.
>   * @payload_in: (input) Pointer to the input payload.
>   * @payload_out: (output) Pointer to the output payload. Must be allocated by
> @@ -147,4 +132,62 @@ struct cxl_mem {
>  
>  	int (*mbox_send)(struct cxl_mem *cxlm, struct cxl_mbox_cmd *cmd);
>  };
>  #endif /* __CXL_MEM_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index a211b35af4be..b8075b941a3a 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1,17 +1,12 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
> -#include <uapi/linux/cxl_mem.h>
> -#include <linux/security.h>
> -#include <linux/debugfs.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/module.h>
>  #include <linux/sizes.h>
>  #include <linux/mutex.h>
>  #include <linux/list.h>
> -#include <linux/cdev.h>
> -#include <linux/idr.h>

Why was this here in the first place?
If it's unrelated, then separate patch ideally.


>  #include <linux/pci.h>
>  #include <linux/io.h>
> -#include <linux/io-64-nonatomic-lo-hi.h>
>  #include "cxlmem.h"
>  #include "pci.h"
>  #include "cxl.h"
> @@ -38,162 +33,6 @@

...

  reply	other threads:[~2021-09-02 17:56 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 [this message]
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
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=20210902185606.0000731b@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).