On Tue, Jun 27, 2017 at 6:24 PM, Vishal Verma <[email protected]> wrote:
> The UEFI 2.7 specification defines an updated BTT metadata format,
> bumping the revision to 2.0. Add support for the new format, while
> retaining compatibility for the old 1.1 format.
>
> Cc: Toshi Kani <[email protected]>
> Cc: Linda Knippers <[email protected]>
> Cc: Dan Williams <[email protected]>
> Signed-off-by: Vishal Verma <[email protected]>
> ---
>
> v3:
> - Add athe actual BTT2 guid from the UEFI spec, and plumb a new
> claim class for it (Dan)
Looks correct, but some code organization comments below:
[..]
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index f05d9b0..a11c189 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1427,14 +1427,43 @@ static DEVICE_ATTR_RO(holder);
>
> static ssize_t __holder_class_store(struct device *dev, const char *buf)
> {
> + struct nd_region *nd_region = to_nd_region(dev->parent);
> + struct nd_mapping *nd_mapping = &nd_region->mapping[0];
> struct nd_namespace_common *ndns = to_ndns(dev);
> + int use_btt_legacy = 0;
> +
> + /* for legacy/label-less namespaces, just use BTT1 */
> + if (nd_mapping != NULL && nd_mapping->nvdimm != NULL) {
By definition if we have an nd_mapping then nd_mapping->nvdimm is
non-null, so you can go straight to calling to_ndd(). However, I think
you want to loop through all nd_mappings in the region to make sure
they all agree about the version and if they disagree we should fail
the write. That loop will use nd_region->ndr_mappings to catch the no
mappings case.
> + struct nd_namespace_index *nsindex;
> + struct nvdimm_drvdata *ndd;
> +
> + ndd = to_ndd(nd_mapping);
> + nsindex = to_namespace_index(ndd, ndd->ns_current);
> + if (nsindex == NULL) {
> + ;
> + /*
> + * Labels haven't been initialized yet, and when they
> + * will, they will be of the 1.2 format, so we can
> + * assume BTT2.0
> + */
> + } else {
> + /* check if existing labels are v1.1 */
> + if (__le16_to_cpu(nsindex->major) == 1
> + && __le16_to_cpu(nsindex->minor) == 1)
> + use_btt_legacy = 1;
> + }
> + } else
> + use_btt_legacy = 1;
This whole hunk above really wants to be its own helper function...
>
> if (dev->driver || ndns->claim)
> return -EBUSY;
>
> - if (strcmp(buf, "btt") == 0 || strcmp(buf, "btt\n") == 0)
> - ndns->claim_class = NVDIMM_CCLASS_BTT;
> - else if (strcmp(buf, "pfn") == 0 || strcmp(buf, "pfn\n") == 0)
> + if (strcmp(buf, "btt") == 0 || strcmp(buf, "btt\n") == 0) {
> + if (use_btt_legacy)
> + ndns->claim_class = NVDIMM_CCLASS_BTT;
> + else
> + ndns->claim_class = NVDIMM_CCLASS_BTT2;
...and then here we can do:
ndns->claim_class = btt_claim_class(ndns);
That way any reader who doesn't care about the gory details of the
default btt_claim_class() can just skip over it.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
