Re: [PATCH 04/20] lightnvm: add minor version to generic geometry

[Javier González]

> On 22 Feb 2018, at 08.34, Matias Bjørling  wrote:
> 
> On 02/21/2018 10:26 AM, Javier González wrote:
>> Separate the version between major and minor on the generic geometry.
>> Also, add a "subversion" entry to sysfs to expose the minor version
>> without breaking user space.
>> Signed-off-by: Javier González 
>> ---
>>  drivers/lightnvm/core.c  |  4 ++--
>>  drivers/nvme/host/lightnvm.c | 25 -
>>  include/linux/lightnvm.h |  3 ++-
>>  3 files changed, 24 insertions(+), 8 deletions(-)
>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> index 42596afdf64c..dc9ec6baff45 100644
>> --- a/drivers/lightnvm/core.c
>> +++ b/drivers/lightnvm/core.c
>> @@ -897,8 +897,8 @@ static int nvm_init(struct nvm_dev *dev)
>>  goto err;
>>  }
>>  -   pr_debug("nvm: ver:%u nvm_vendor:%x\n",
>> -dev_geo->ver_id,
>> +pr_debug("nvm: ver:%u.%u nvm_vendor:%x\n",
>> +dev_geo->major_ver_id, dev_geo->minor_ver_id,
>>  dev_geo->c.vmnt);
>>  ret = nvm_core_init(dev);
>> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
>> index 598abba66f52..71b4ac57a668 100644
>> --- a/drivers/nvme/host/lightnvm.c
>> +++ b/drivers/nvme/host/lightnvm.c
>> @@ -296,7 +296,8 @@ static int nvme_nvm_setup_12(struct nvme_nvm_id12 *id,
>>  }
>>  /* 1.2 spec. only reports a single version id - unfold */
>> -dev_geo->ver_id = id->ver_id;
>> +dev_geo->major_ver_id = id->ver_id;
>> +dev_geo->minor_ver_id = 2;
>>  dev_geo->nr_chnls = src->num_ch;
>>  dev_geo->nr_luns = src->num_lun;
>> @@ -377,7 +378,14 @@ static void nvme_nvm_set_addr_20(struct nvm_addr_format 
>> *dst,
>>  static int nvme_nvm_setup_20(struct nvme_nvm_id20 *id,
>>   struct nvm_dev_geo *dev_geo)
>>  {
>> -dev_geo->ver_id = id->mjr;
>> +dev_geo->major_ver_id = id->mjr;
>> +dev_geo->minor_ver_id = id->mnr;
>> +
>> +if (!(dev_geo->major_ver_id == 2 && dev_geo->minor_ver_id == 0)) {
>> +pr_err("nvm: OCSSD version not supported (v%d.%d)\n",
>> +dev_geo->major_ver_id, dev_geo->minor_ver_id);
>> +return -EINVAL;
>> +}
>>  dev_geo->nr_chnls = le16_to_cpu(id->num_grp);
>>  dev_geo->nr_luns = le16_to_cpu(id->num_pu);
>> @@ -913,7 +921,11 @@ static ssize_t nvm_dev_attr_show(struct device *dev,
>>  attr = >attr;
>>  if (strcmp(attr->name, "version") == 0) {
>> -return scnprintf(page, PAGE_SIZE, "%u\n", dev_geo->ver_id);
>> +return scnprintf(page, PAGE_SIZE, "%u\n",
>> +dev_geo->major_ver_id);
>> +} else if (strcmp(attr->name, "subversion") == 0) {
>> +return scnprintf(page, PAGE_SIZE, "%u\n",
>> +dev_geo->minor_ver_id);
>>  } else if (strcmp(attr->name, "media_capabilities") == 0) {
>>  return scnprintf(page, PAGE_SIZE, "%u\n", dev_geo->c.mccap);
>>  } else if (strcmp(attr->name, "read_typ") == 0) {
>> @@ -1055,6 +1067,7 @@ static ssize_t nvm_dev_attr_show_20(struct device *dev,
>>/* general attributes */
>>  static NVM_DEV_ATTR_RO(version);
>> +static NVM_DEV_ATTR_RO(subversion);
>>  static NVM_DEV_ATTR_RO(media_capabilities);
>>static NVM_DEV_ATTR_RO(read_typ);
>> @@ -1085,6 +1098,7 @@ static NVM_DEV_ATTR_12_RO(max_phys_secs);
>>static struct attribute *nvm_dev_attrs_12[] = {
>>  _attr_version.attr,
>> +_attr_subversion.attr,
>>  _attr_media_capabilities.attr,
>>  _attr_vendor_opcode.attr,
>> @@ -1134,6 +1148,7 @@ static NVM_DEV_ATTR_20_RO(reset_max);
>>static struct attribute *nvm_dev_attrs_20[] = {
>>  _attr_version.attr,
>> +_attr_subversion.attr,
>>  _attr_media_capabilities.attr,
>>  _attr_groups.attr,
>> @@ -1167,7 +1182,7 @@ int nvme_nvm_register_sysfs(struct nvme_ns *ns)
>>  if (!ndev)
>>  return -EINVAL;
>>  -   switch (dev_geo->ver_id) {
>> +switch (dev_geo->major_ver_id) {
>>  case 1:
>>  return sysfs_create_group(_to_dev(ns->disk)->kobj,
>>  _dev_attr_group_12);
>> @@ -1184,7 +1199,7 @@ void nvme_nvm_unregister_sysfs(struct nvme_ns *ns)
>>  struct nvm_dev *ndev = ns->ndev;
>>  struct nvm_dev_geo *dev_geo = >dev_geo;
>>  -   switch (dev_geo->ver_id) {
>> +switch (dev_geo->major_ver_id) {
>>  case 1:
>>  sysfs_remove_group(_to_dev(ns->disk)->kobj,
>>  _dev_attr_group_12);
>> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
>> index 18e3751b1632..5af0b8837095 100644
>> --- a/include/linux/lightnvm.h
>> +++ b/include/linux/lightnvm.h
>> @@ -242,7 +242,8 @@ struct nvm_common_geo {
>>  /* Device identified geometry */
>>  struct nvm_dev_geo {
>>  /* device reported version */
>> -u8  ver_id;
>> 

Re: [PATCH 04/20] lightnvm: add minor version to generic geometry

[Javier González]

> On 22 Feb 2018, at 08.34, Matias Bjørling  wrote:
> 
> On 02/21/2018 10:26 AM, Javier González wrote:
>> Separate the version between major and minor on the generic geometry.
>> Also, add a "subversion" entry to sysfs to expose the minor version
>> without breaking user space.
>> Signed-off-by: Javier González 
>> ---
>>  drivers/lightnvm/core.c  |  4 ++--
>>  drivers/nvme/host/lightnvm.c | 25 -
>>  include/linux/lightnvm.h |  3 ++-
>>  3 files changed, 24 insertions(+), 8 deletions(-)
>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> index 42596afdf64c..dc9ec6baff45 100644
>> --- a/drivers/lightnvm/core.c
>> +++ b/drivers/lightnvm/core.c
>> @@ -897,8 +897,8 @@ static int nvm_init(struct nvm_dev *dev)
>>  goto err;
>>  }
>>  -   pr_debug("nvm: ver:%u nvm_vendor:%x\n",
>> -dev_geo->ver_id,
>> +pr_debug("nvm: ver:%u.%u nvm_vendor:%x\n",
>> +dev_geo->major_ver_id, dev_geo->minor_ver_id,
>>  dev_geo->c.vmnt);
>>  ret = nvm_core_init(dev);
>> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
>> index 598abba66f52..71b4ac57a668 100644
>> --- a/drivers/nvme/host/lightnvm.c
>> +++ b/drivers/nvme/host/lightnvm.c
>> @@ -296,7 +296,8 @@ static int nvme_nvm_setup_12(struct nvme_nvm_id12 *id,
>>  }
>>  /* 1.2 spec. only reports a single version id - unfold */
>> -dev_geo->ver_id = id->ver_id;
>> +dev_geo->major_ver_id = id->ver_id;
>> +dev_geo->minor_ver_id = 2;
>>  dev_geo->nr_chnls = src->num_ch;
>>  dev_geo->nr_luns = src->num_lun;
>> @@ -377,7 +378,14 @@ static void nvme_nvm_set_addr_20(struct nvm_addr_format 
>> *dst,
>>  static int nvme_nvm_setup_20(struct nvme_nvm_id20 *id,
>>   struct nvm_dev_geo *dev_geo)
>>  {
>> -dev_geo->ver_id = id->mjr;
>> +dev_geo->major_ver_id = id->mjr;
>> +dev_geo->minor_ver_id = id->mnr;
>> +
>> +if (!(dev_geo->major_ver_id == 2 && dev_geo->minor_ver_id == 0)) {
>> +pr_err("nvm: OCSSD version not supported (v%d.%d)\n",
>> +dev_geo->major_ver_id, dev_geo->minor_ver_id);
>> +return -EINVAL;
>> +}
>>  dev_geo->nr_chnls = le16_to_cpu(id->num_grp);
>>  dev_geo->nr_luns = le16_to_cpu(id->num_pu);
>> @@ -913,7 +921,11 @@ static ssize_t nvm_dev_attr_show(struct device *dev,
>>  attr = >attr;
>>  if (strcmp(attr->name, "version") == 0) {
>> -return scnprintf(page, PAGE_SIZE, "%u\n", dev_geo->ver_id);
>> +return scnprintf(page, PAGE_SIZE, "%u\n",
>> +dev_geo->major_ver_id);
>> +} else if (strcmp(attr->name, "subversion") == 0) {
>> +return scnprintf(page, PAGE_SIZE, "%u\n",
>> +dev_geo->minor_ver_id);
>>  } else if (strcmp(attr->name, "media_capabilities") == 0) {
>>  return scnprintf(page, PAGE_SIZE, "%u\n", dev_geo->c.mccap);
>>  } else if (strcmp(attr->name, "read_typ") == 0) {
>> @@ -1055,6 +1067,7 @@ static ssize_t nvm_dev_attr_show_20(struct device *dev,
>>/* general attributes */
>>  static NVM_DEV_ATTR_RO(version);
>> +static NVM_DEV_ATTR_RO(subversion);
>>  static NVM_DEV_ATTR_RO(media_capabilities);
>>static NVM_DEV_ATTR_RO(read_typ);
>> @@ -1085,6 +1098,7 @@ static NVM_DEV_ATTR_12_RO(max_phys_secs);
>>static struct attribute *nvm_dev_attrs_12[] = {
>>  _attr_version.attr,
>> +_attr_subversion.attr,
>>  _attr_media_capabilities.attr,
>>  _attr_vendor_opcode.attr,
>> @@ -1134,6 +1148,7 @@ static NVM_DEV_ATTR_20_RO(reset_max);
>>static struct attribute *nvm_dev_attrs_20[] = {
>>  _attr_version.attr,
>> +_attr_subversion.attr,
>>  _attr_media_capabilities.attr,
>>  _attr_groups.attr,
>> @@ -1167,7 +1182,7 @@ int nvme_nvm_register_sysfs(struct nvme_ns *ns)
>>  if (!ndev)
>>  return -EINVAL;
>>  -   switch (dev_geo->ver_id) {
>> +switch (dev_geo->major_ver_id) {
>>  case 1:
>>  return sysfs_create_group(_to_dev(ns->disk)->kobj,
>>  _dev_attr_group_12);
>> @@ -1184,7 +1199,7 @@ void nvme_nvm_unregister_sysfs(struct nvme_ns *ns)
>>  struct nvm_dev *ndev = ns->ndev;
>>  struct nvm_dev_geo *dev_geo = >dev_geo;
>>  -   switch (dev_geo->ver_id) {
>> +switch (dev_geo->major_ver_id) {
>>  case 1:
>>  sysfs_remove_group(_to_dev(ns->disk)->kobj,
>>  _dev_attr_group_12);
>> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
>> index 18e3751b1632..5af0b8837095 100644
>> --- a/include/linux/lightnvm.h
>> +++ b/include/linux/lightnvm.h
>> @@ -242,7 +242,8 @@ struct nvm_common_geo {
>>  /* Device identified geometry */
>>  struct nvm_dev_geo {
>>  /* device reported version */
>> -u8  ver_id;
>> +u8  major_ver_id;
>> +u8  

Re: [PATCH 04/20] lightnvm: add minor version to generic geometry

[Matias Bjørling]

On 02/21/2018 10:26 AM, Javier González wrote:

Separate the version between major and minor on the generic geometry.
Also, add a "subversion" entry to sysfs to expose the minor version
without breaking user space.

Signed-off-by: Javier González 
---
  drivers/lightnvm/core.c  |  4 ++--
  drivers/nvme/host/lightnvm.c | 25 -
  include/linux/lightnvm.h |  3 ++-
  3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 42596afdf64c..dc9ec6baff45 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -897,8 +897,8 @@ static int nvm_init(struct nvm_dev *dev)
goto err;
}
  
-	pr_debug("nvm: ver:%u nvm_vendor:%x\n",

-   dev_geo->ver_id,
+   pr_debug("nvm: ver:%u.%u nvm_vendor:%x\n",
+   dev_geo->major_ver_id, dev_geo->minor_ver_id,
dev_geo->c.vmnt);
  
  	ret = nvm_core_init(dev);

diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 598abba66f52..71b4ac57a668 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -296,7 +296,8 @@ static int nvme_nvm_setup_12(struct nvme_nvm_id12 *id,
}
  
  	/* 1.2 spec. only reports a single version id - unfold */

-   dev_geo->ver_id = id->ver_id;
+   dev_geo->major_ver_id = id->ver_id;
+   dev_geo->minor_ver_id = 2;
  
  	dev_geo->nr_chnls = src->num_ch;

dev_geo->nr_luns = src->num_lun;
@@ -377,7 +378,14 @@ static void nvme_nvm_set_addr_20(struct nvm_addr_format 
*dst,
  static int nvme_nvm_setup_20(struct nvme_nvm_id20 *id,
 struct nvm_dev_geo *dev_geo)
  {
-   dev_geo->ver_id = id->mjr;
+   dev_geo->major_ver_id = id->mjr;
+   dev_geo->minor_ver_id = id->mnr;
+
+   if (!(dev_geo->major_ver_id == 2 && dev_geo->minor_ver_id == 0)) {
+   pr_err("nvm: OCSSD version not supported (v%d.%d)\n",
+   dev_geo->major_ver_id, dev_geo->minor_ver_id);
+   return -EINVAL;
+   }
  
  	dev_geo->nr_chnls = le16_to_cpu(id->num_grp);

dev_geo->nr_luns = le16_to_cpu(id->num_pu);
@@ -913,7 +921,11 @@ static ssize_t nvm_dev_attr_show(struct device *dev,
attr = >attr;
  
  	if (strcmp(attr->name, "version") == 0) {

-   return scnprintf(page, PAGE_SIZE, "%u\n", dev_geo->ver_id);
+   return scnprintf(page, PAGE_SIZE, "%u\n",
+   dev_geo->major_ver_id);
+   } else if (strcmp(attr->name, "subversion") == 0) {
+   return scnprintf(page, PAGE_SIZE, "%u\n",
+   dev_geo->minor_ver_id);
} else if (strcmp(attr->name, "media_capabilities") == 0) {
return scnprintf(page, PAGE_SIZE, "%u\n", dev_geo->c.mccap);
} else if (strcmp(attr->name, "read_typ") == 0) {
@@ -1055,6 +1067,7 @@ static ssize_t nvm_dev_attr_show_20(struct device *dev,
  
  /* general attributes */

  static NVM_DEV_ATTR_RO(version);
+static NVM_DEV_ATTR_RO(subversion);
  static NVM_DEV_ATTR_RO(media_capabilities);
  
  static NVM_DEV_ATTR_RO(read_typ);

@@ -1085,6 +1098,7 @@ static NVM_DEV_ATTR_12_RO(max_phys_secs);
  
  static struct attribute *nvm_dev_attrs_12[] = {

_attr_version.attr,
+   _attr_subversion.attr,
_attr_media_capabilities.attr,
  
  	_attr_vendor_opcode.attr,

@@ -1134,6 +1148,7 @@ static NVM_DEV_ATTR_20_RO(reset_max);
  
  static struct attribute *nvm_dev_attrs_20[] = {

_attr_version.attr,
+   _attr_subversion.attr,
_attr_media_capabilities.attr,
  
  	_attr_groups.attr,

@@ -1167,7 +1182,7 @@ int nvme_nvm_register_sysfs(struct nvme_ns *ns)
if (!ndev)
return -EINVAL;
  
-	switch (dev_geo->ver_id) {

+   switch (dev_geo->major_ver_id) {
case 1:
return sysfs_create_group(_to_dev(ns->disk)->kobj,
_dev_attr_group_12);
@@ -1184,7 +1199,7 @@ void nvme_nvm_unregister_sysfs(struct nvme_ns *ns)
struct nvm_dev *ndev = ns->ndev;
struct nvm_dev_geo *dev_geo = >dev_geo;
  
-	switch (dev_geo->ver_id) {

+   switch (dev_geo->major_ver_id) {
case 1:
sysfs_remove_group(_to_dev(ns->disk)->kobj,
_dev_attr_group_12);
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 18e3751b1632..5af0b8837095 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -242,7 +242,8 @@ struct nvm_common_geo {
  /* Device identified geometry */
  struct nvm_dev_geo {
/* device reported version */
-   u8  ver_id;
+   u8  major_ver_id;
+   u8  minor_ver_id;
  
  	/* full device geometry */

u16 nr_chnls;



Looks fairly good to me. Did you consider putting in the major and minor 

Re: [PATCH 04/20] lightnvm: add minor version to generic geometry

[Matias Bjørling]

On 02/21/2018 10:26 AM, Javier González wrote:

Separate the version between major and minor on the generic geometry.
Also, add a "subversion" entry to sysfs to expose the minor version
without breaking user space.

Signed-off-by: Javier González 
---
  drivers/lightnvm/core.c  |  4 ++--
  drivers/nvme/host/lightnvm.c | 25 -
  include/linux/lightnvm.h |  3 ++-
  3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 42596afdf64c..dc9ec6baff45 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -897,8 +897,8 @@ static int nvm_init(struct nvm_dev *dev)
goto err;
}
  
-	pr_debug("nvm: ver:%u nvm_vendor:%x\n",

-   dev_geo->ver_id,
+   pr_debug("nvm: ver:%u.%u nvm_vendor:%x\n",
+   dev_geo->major_ver_id, dev_geo->minor_ver_id,
dev_geo->c.vmnt);
  
  	ret = nvm_core_init(dev);

diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 598abba66f52..71b4ac57a668 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -296,7 +296,8 @@ static int nvme_nvm_setup_12(struct nvme_nvm_id12 *id,
}
  
  	/* 1.2 spec. only reports a single version id - unfold */

-   dev_geo->ver_id = id->ver_id;
+   dev_geo->major_ver_id = id->ver_id;
+   dev_geo->minor_ver_id = 2;
  
  	dev_geo->nr_chnls = src->num_ch;

dev_geo->nr_luns = src->num_lun;
@@ -377,7 +378,14 @@ static void nvme_nvm_set_addr_20(struct nvm_addr_format 
*dst,
  static int nvme_nvm_setup_20(struct nvme_nvm_id20 *id,
 struct nvm_dev_geo *dev_geo)
  {
-   dev_geo->ver_id = id->mjr;
+   dev_geo->major_ver_id = id->mjr;
+   dev_geo->minor_ver_id = id->mnr;
+
+   if (!(dev_geo->major_ver_id == 2 && dev_geo->minor_ver_id == 0)) {
+   pr_err("nvm: OCSSD version not supported (v%d.%d)\n",
+   dev_geo->major_ver_id, dev_geo->minor_ver_id);
+   return -EINVAL;
+   }
  
  	dev_geo->nr_chnls = le16_to_cpu(id->num_grp);

dev_geo->nr_luns = le16_to_cpu(id->num_pu);
@@ -913,7 +921,11 @@ static ssize_t nvm_dev_attr_show(struct device *dev,
attr = >attr;
  
  	if (strcmp(attr->name, "version") == 0) {

-   return scnprintf(page, PAGE_SIZE, "%u\n", dev_geo->ver_id);
+   return scnprintf(page, PAGE_SIZE, "%u\n",
+   dev_geo->major_ver_id);
+   } else if (strcmp(attr->name, "subversion") == 0) {
+   return scnprintf(page, PAGE_SIZE, "%u\n",
+   dev_geo->minor_ver_id);
} else if (strcmp(attr->name, "media_capabilities") == 0) {
return scnprintf(page, PAGE_SIZE, "%u\n", dev_geo->c.mccap);
} else if (strcmp(attr->name, "read_typ") == 0) {
@@ -1055,6 +1067,7 @@ static ssize_t nvm_dev_attr_show_20(struct device *dev,
  
  /* general attributes */

  static NVM_DEV_ATTR_RO(version);
+static NVM_DEV_ATTR_RO(subversion);
  static NVM_DEV_ATTR_RO(media_capabilities);
  
  static NVM_DEV_ATTR_RO(read_typ);

@@ -1085,6 +1098,7 @@ static NVM_DEV_ATTR_12_RO(max_phys_secs);
  
  static struct attribute *nvm_dev_attrs_12[] = {

_attr_version.attr,
+   _attr_subversion.attr,
_attr_media_capabilities.attr,
  
  	_attr_vendor_opcode.attr,

@@ -1134,6 +1148,7 @@ static NVM_DEV_ATTR_20_RO(reset_max);
  
  static struct attribute *nvm_dev_attrs_20[] = {

_attr_version.attr,
+   _attr_subversion.attr,
_attr_media_capabilities.attr,
  
  	_attr_groups.attr,

@@ -1167,7 +1182,7 @@ int nvme_nvm_register_sysfs(struct nvme_ns *ns)
if (!ndev)
return -EINVAL;
  
-	switch (dev_geo->ver_id) {

+   switch (dev_geo->major_ver_id) {
case 1:
return sysfs_create_group(_to_dev(ns->disk)->kobj,
_dev_attr_group_12);
@@ -1184,7 +1199,7 @@ void nvme_nvm_unregister_sysfs(struct nvme_ns *ns)
struct nvm_dev *ndev = ns->ndev;
struct nvm_dev_geo *dev_geo = >dev_geo;
  
-	switch (dev_geo->ver_id) {

+   switch (dev_geo->major_ver_id) {
case 1:
sysfs_remove_group(_to_dev(ns->disk)->kobj,
_dev_attr_group_12);
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 18e3751b1632..5af0b8837095 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -242,7 +242,8 @@ struct nvm_common_geo {
  /* Device identified geometry */
  struct nvm_dev_geo {
/* device reported version */
-   u8  ver_id;
+   u8  major_ver_id;
+   u8  minor_ver_id;
  
  	/* full device geometry */

u16 nr_chnls;



Looks fairly good to me. Did you consider putting in the major and minor 
version id into the