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>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	<nvdimm@lists.linux.dev>, <ben.widawsky@intel.com>,
	<vishal.l.verma@intel.com>, <alison.schofield@intel.com>,
	<ira.weiny@intel.com>
Subject: Re: [PATCH 10/23] libnvdimm/labels: Add uuid helpers
Date: Wed, 11 Aug 2021 19:13:19 +0100	[thread overview]
Message-ID: <20210811191319.00006d64@Huawei.com> (raw)
In-Reply-To: <162854812073.1980150.8157116233571368158.stgit@dwillia2-desk3.amr.corp.intel.com>

On Mon, 9 Aug 2021 15:28:40 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for CXL labels that move the uuid to a different offset
> in the label, add nsl_{ref,get,validate}_uuid(). These helpers use the
> proper uuid_t type. That type definition predated the libnvdimm
> subsystem, so now is as a good a time as any to convert all the uuid
> handling in the subsystem to uuid_t to match the helpers.
> 
> As for the whitespace changes, all new code is clang-format compliant.
> 
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

There are a few interesting corners where you have cleaned out a pointless
copy before validating uuids. Perhaps call that out as a change in here
as it isn't as simple as just replacing like with like?
Perhaps I'm missing some reason that was needed in the code before this
patch.

All LGTM.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/nvdimm/btt.c            |   11 +++--
>  drivers/nvdimm/btt.h            |    4 +-
>  drivers/nvdimm/btt_devs.c       |   12 +++---
>  drivers/nvdimm/core.c           |   40 ++-----------------
>  drivers/nvdimm/label.c          |   34 +++++++---------
>  drivers/nvdimm/label.h          |    3 -
>  drivers/nvdimm/namespace_devs.c |   83 ++++++++++++++++++++-------------------
>  drivers/nvdimm/nd-core.h        |    5 +-
>  drivers/nvdimm/nd.h             |   37 ++++++++++++++++-
>  drivers/nvdimm/pfn_devs.c       |    2 -
>  include/linux/nd.h              |    4 +-
>  11 files changed, 115 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index 92dec4952297..1cdfbadb7408 100644

> @@ -1050,7 +1050,6 @@ static int __blk_label_update(struct nd_region *nd_region,
>  	unsigned long *free, *victim_map = NULL;
>  	struct resource *res, **old_res_list;
>  	struct nd_label_id label_id;
> -	u8 uuid[NSLABEL_UUID_LEN];
>  	int min_dpa_idx = 0;
>  	LIST_HEAD(list);
>  	u32 nslot, slot;
> @@ -1088,8 +1087,7 @@ static int __blk_label_update(struct nd_region *nd_region,
>  		/* mark unused labels for garbage collection */
>  		for_each_clear_bit_le(slot, free, nslot) {
>  			nd_label = to_label(ndd, slot);
> -			memcpy(uuid, nd_label->uuid, NSLABEL_UUID_LEN);
> -			if (memcmp(uuid, nsblk->uuid, NSLABEL_UUID_LEN) != 0)
> +			if (!nsl_validate_uuid(ndd, nd_label, nsblk->uuid))
>  				continue;

The original code here was 'unusual'. I'm not sure why it couldn't always be
validated in place. 

>  			res = to_resource(ndd, nd_label);
>  			if (res && is_old_resource(res, old_res_list,
> @@ -1158,7 +1156,7 @@ static int __blk_label_update(struct nd_region *nd_region,
>  
>  		nd_label = to_label(ndd, slot);
>  		memset(nd_label, 0, sizeof_namespace_label(ndd));
> -		memcpy(nd_label->uuid, nsblk->uuid, NSLABEL_UUID_LEN);

> +		nsl_set_uuid(ndd, nd_label, nsblk->uuid);
>  		nsl_set_name(ndd, nd_label, nsblk->alt_name);
>  		nsl_set_flags(ndd, nd_label, NSLABEL_FLAG_LOCAL);
>  
> @@ -1206,8 +1204,7 @@ static int __blk_label_update(struct nd_region *nd_region,
>  		if (!nd_label)
>  			continue;
>  		nlabel++;
> -		memcpy(uuid, nd_label->uuid, NSLABEL_UUID_LEN);
> -		if (memcmp(uuid, nsblk->uuid, NSLABEL_UUID_LEN) != 0)
> +		if (!nsl_validate_uuid(ndd, nd_label, nsblk->uuid))
>  			continue;
>  		nlabel--;
>  		list_move(&label_ent->list, &list);
> @@ -1237,8 +1234,7 @@ static int __blk_label_update(struct nd_region *nd_region,
>  	}
>  	for_each_clear_bit_le(slot, free, nslot) {
>  		nd_label = to_label(ndd, slot);
> -		memcpy(uuid, nd_label->uuid, NSLABEL_UUID_LEN);
> -		if (memcmp(uuid, nsblk->uuid, NSLABEL_UUID_LEN) != 0)
> +		if (!nsl_validate_uuid(ndd, nd_label, nsblk->uuid))
>  			continue;
>  		res = to_resource(ndd, nd_label);
>  		res->flags &= ~DPA_RESOURCE_ADJUSTED;
> @@ -1318,12 +1314,11 @@ static int init_labels(struct nd_mapping *nd_mapping, int num_labels)
>  	return max(num_labels, old_num_labels);
>  }

  parent reply	other threads:[~2021-08-11 18:13 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-09 22:27 [PATCH 00/23] cxl_test: Enable CXL Topology and UAPI regression tests Dan Williams
2021-08-09 22:27 ` [PATCH 01/23] libnvdimm/labels: Introduce getters for namespace label fields Dan Williams
2021-08-10 20:48   ` Ben Widawsky
2021-08-10 21:58     ` Dan Williams
2021-08-11 18:44   ` Jonathan Cameron
2021-08-09 22:27 ` [PATCH 02/23] libnvdimm/labels: Add isetcookie validation helper Dan Williams
2021-08-11 18:44   ` Jonathan Cameron
2021-08-09 22:28 ` [PATCH 03/23] libnvdimm/labels: Introduce label setter helpers Dan Williams
2021-08-11 17:27   ` Jonathan Cameron
2021-08-11 17:42     ` Dan Williams
2021-08-09 22:28 ` [PATCH 04/23] libnvdimm/labels: Add a checksum calculation helper Dan Williams
2021-08-11 18:44   ` Jonathan Cameron
2021-08-09 22:28 ` [PATCH 05/23] libnvdimm/labels: Add blk isetcookie set / validation helpers Dan Williams
2021-08-11 18:45   ` Jonathan Cameron
2021-08-09 22:28 ` [PATCH 06/23] libnvdimm/labels: Add blk special cases for nlabel and position helpers Dan Williams
2021-08-11 18:45   ` Jonathan Cameron
2021-08-09 22:28 ` [PATCH 07/23] libnvdimm/labels: Add type-guid helpers Dan Williams
2021-08-11 18:46   ` Jonathan Cameron
2021-08-09 22:28 ` [PATCH 08/23] libnvdimm/labels: Add claim class helpers Dan Williams
2021-08-11 18:46   ` Jonathan Cameron
2021-08-09 22:28 ` [PATCH 09/23] libnvdimm/labels: Add address-abstraction uuid definitions Dan Williams
2021-08-11 18:49   ` Jonathan Cameron
2021-08-11 22:47     ` Dan Williams
2021-08-09 22:28 ` [PATCH 10/23] libnvdimm/labels: Add uuid helpers Dan Williams
2021-08-11  8:05   ` Andy Shevchenko
2021-08-11 16:59     ` Andy Shevchenko
2021-08-11 17:11       ` Dan Williams
2021-08-11 19:18         ` Andy Shevchenko
2021-08-11 19:26           ` Dan Williams
2021-08-12 22:34           ` Dan Williams
2021-08-13 10:14             ` Andy Shevchenko
2021-08-14  7:35               ` Christoph Hellwig
2021-08-11 18:13   ` Jonathan Cameron [this message]
2021-08-12 21:17     ` Dan Williams
2021-08-09 22:28 ` [PATCH 11/23] libnvdimm/labels: Introduce CXL labels Dan Williams
2021-08-11 18:41   ` Jonathan Cameron
2021-08-11 23:01     ` Dan Williams
2021-08-09 22:28 ` [PATCH 12/23] cxl/pci: Make 'struct cxl_mem' device type generic Dan Williams
2021-08-09 22:28 ` [PATCH 13/23] cxl/mbox: Introduce the mbox_send operation Dan Williams
2021-08-09 22:29 ` [PATCH 14/23] cxl/mbox: Move mailbox and other non-PCI specific infrastructure to the core Dan Williams
2021-08-11  6:11   ` [PATCH v2 " Dan Williams
2021-08-09 22:29 ` [PATCH 15/23] cxl/pci: Use module_pci_driver Dan Williams
2021-08-09 22:29 ` [PATCH 16/23] cxl/mbox: Convert 'enabled_cmds' to DECLARE_BITMAP Dan Williams
2021-08-09 22:29 ` [PATCH 17/23] cxl/mbox: Add exclusive kernel command support Dan Williams
2021-08-10 21:34   ` Ben Widawsky
2021-08-10 21:52     ` Dan Williams
2021-08-10 22:06       ` Ben Widawsky
2021-08-11  1:22         ` Dan Williams
2021-08-11  2:14           ` Dan Williams
2021-08-09 22:29 ` [PATCH 18/23] cxl/pmem: Translate NVDIMM label commands to CXL label commands Dan Williams
2021-08-09 22:29 ` [PATCH 19/23] cxl/pmem: Add support for multiple nvdimm-bridge objects Dan Williams
2021-08-09 22:29 ` [PATCH 20/23] tools/testing/cxl: Introduce a mocked-up CXL port hierarchy Dan Williams
2021-08-10 21:57   ` Ben Widawsky
2021-08-10 22:40     ` Dan Williams
2021-08-11 15:18       ` Ben Widawsky
     [not found]       ` <xp0k4.l2r85dw1p7do@intel.com>
2021-08-11 21:03         ` Dan Williams
2021-08-09 22:29 ` [PATCH 21/23] cxl/bus: Populate the target list at decoder create Dan Williams
2021-08-09 22:29 ` [PATCH 22/23] cxl/mbox: Move command definitions to common location Dan Williams
2021-08-09 22:29 ` [PATCH 23/23] tools/testing/cxl: Introduce a mock memory device + driver Dan Williams
2021-08-10 22:10 ` [PATCH 00/23] cxl_test: Enable CXL Topology and UAPI regression tests Ben Widawsky
2021-08-10 22:58   ` 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=20210811191319.00006d64@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=alison.schofield@intel.com \
    --cc=andriy.shevchenko@linux.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=nvdimm@lists.linux.dev \
    --cc=vishal.l.verma@intel.com \
    --subject='Re: [PATCH 10/23] libnvdimm/labels: Add uuid helpers' \
    /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).