Re: [PATCH 2/5] nvme: register ns_id attributes as default sysfs groups

2018-09-28 Thread Christoph Hellwig 
On Fri, Sep 28, 2018 at 08:17:20AM +0200, Hannes Reinecke wrote:
> We should be registering the ns_id attribute as default sysfs
> attribute groups, otherwise we have a race condition between
> the uevent and the attributes appearing in sysfs.

Looks good,

Reviewed-by: Christoph Hellwig

Re: [PATCH 2/5] nvme: register ns_id attributes as default sysfs groups

2018-09-28 Thread Keith Busch 
On Fri, Sep 28, 2018 at 08:17:20AM +0200, Hannes Reinecke wrote:
> We should be registering the ns_id attribute as default sysfs
> attribute groups, otherwise we have a race condition between
> the uevent and the attributes appearing in sysfs.
> 
> Suggested-by: Bart van Assche 
> Signed-off-by: Hannes Reinecke 

Thanks, looks great!

Reviewed-by: Keith Busch

[PATCH 2/5] nvme: register ns_id attributes as default sysfs groups

2018-09-27 Thread Hannes Reinecke 
We should be registering the ns_id attribute as default sysfs
attribute groups, otherwise we have a race condition between
the uevent and the attributes appearing in sysfs.

Suggested-by: Bart van Assche 
Signed-off-by: Hannes Reinecke 
---
 drivers/nvme/host/core.c  |  21 -
 drivers/nvme/host/lightnvm.c  | 105 ++
 drivers/nvme/host/multipath.c |  15 ++
 drivers/nvme/host/nvme.h  |  10 +---
 4 files changed, 59 insertions(+), 92 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0e824e8c8fd7..e0a9e1c5b30e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2734,6 +2734,14 @@ const struct attribute_group nvme_ns_id_attr_group = {
.is_visible = nvme_ns_id_attrs_are_visible,
 };
 
+const struct attribute_group *nvme_ns_id_attr_groups[] = {
+   &nvme_ns_id_attr_group,
+#ifdef CONFIG_NVM
+   &nvme_nvm_attr_group,
+#endif
+   NULL,
+};
+
 #define nvme_show_str_function(field)  
\
 static ssize_t  field##_show(struct device *dev,   
\
struct device_attribute *attr, char *buf)   
\
@@ -3099,14 +3107,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, 
unsigned nsid)
 
nvme_get_ctrl(ctrl);
 
-   device_add_disk(ctrl->device, ns->disk, NULL);
-   if (sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
-   &nvme_ns_id_attr_group))
-   pr_warn("%s: failed to create sysfs group for identification\n",
-   ns->disk->disk_name);
-   if (ns->ndev && nvme_nvm_register_sysfs(ns))
-   pr_warn("%s: failed to register lightnvm sysfs group for 
identification\n",
-   ns->disk->disk_name);
+   device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
 
nvme_mpath_add_disk(ns, id);
nvme_fault_inject_init(ns);
@@ -3132,10 +3133,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 
nvme_fault_inject_fini(ns);
if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
-   sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
-   &nvme_ns_id_attr_group);
-   if (ns->ndev)
-   nvme_nvm_unregister_sysfs(ns);
del_gendisk(ns->disk);
blk_cleanup_queue(ns->queue);
if (blk_get_integrity(ns->disk))
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 6fe5923c95d4..1e4f97538838 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -1190,10 +1190,29 @@ static NVM_DEV_ATTR_12_RO(multiplane_modes);
 static NVM_DEV_ATTR_12_RO(media_capabilities);
 static NVM_DEV_ATTR_12_RO(max_phys_secs);
 
-static struct attribute *nvm_dev_attrs_12[] = {
+/* 2.0 values */
+static NVM_DEV_ATTR_20_RO(groups);
+static NVM_DEV_ATTR_20_RO(punits);
+static NVM_DEV_ATTR_20_RO(chunks);
+static NVM_DEV_ATTR_20_RO(clba);
+static NVM_DEV_ATTR_20_RO(ws_min);
+static NVM_DEV_ATTR_20_RO(ws_opt);
+static NVM_DEV_ATTR_20_RO(maxoc);
+static NVM_DEV_ATTR_20_RO(maxocpu);
+static NVM_DEV_ATTR_20_RO(mw_cunits);
+static NVM_DEV_ATTR_20_RO(write_typ);
+static NVM_DEV_ATTR_20_RO(write_max);
+static NVM_DEV_ATTR_20_RO(reset_typ);
+static NVM_DEV_ATTR_20_RO(reset_max);
+
+static struct attribute *nvm_dev_attrs[] = {
+   /* version agnostic attrs */
&dev_attr_version.attr,
&dev_attr_capabilities.attr,
+   &dev_attr_read_typ.attr,
+   &dev_attr_read_max.attr,
 
+   /* 1.2 attrs */
&dev_attr_vendor_opcode.attr,
&dev_attr_device_mode.attr,
&dev_attr_media_manager.attr,
@@ -1208,8 +1227,6 @@ static struct attribute *nvm_dev_attrs_12[] = {
&dev_attr_page_size.attr,
&dev_attr_hw_sector_size.attr,
&dev_attr_oob_sector_size.attr,
-   &dev_attr_read_typ.attr,
-   &dev_attr_read_max.attr,
&dev_attr_prog_typ.attr,
&dev_attr_prog_max.attr,
&dev_attr_erase_typ.attr,
@@ -1218,33 +1235,7 @@ static struct attribute *nvm_dev_attrs_12[] = {
&dev_attr_media_capabilities.attr,
&dev_attr_max_phys_secs.attr,
 
-   NULL,
-};
-
-static const struct attribute_group nvm_dev_attr_group_12 = {
-   .name   = "lightnvm",
-   .attrs  = nvm_dev_attrs_12,
-};
-
-/* 2.0 values */
-static NVM_DEV_ATTR_20_RO(groups);
-static NVM_DEV_ATTR_20_RO(punits);
-static NVM_DEV_ATTR_20_RO(chunks);
-static NVM_DEV_ATTR_20_RO(clba);
-static NVM_DEV_ATTR_20_RO(ws_min);
-static NVM_DEV_ATTR_20_RO(ws_opt);
-static NVM_DEV_ATTR_20_RO(maxoc);
-static NVM_DEV_ATTR_20_RO(maxocpu);
-static NVM_DEV_ATTR_20_RO(mw_cunits);
-static NVM_DEV_ATTR_20_RO(write_typ);
-static NVM_DEV_ATTR_20_RO(write_max);
-static NVM_DEV_ATTR_20_RO(reset_typ);
-static NVM_DEV_ATTR_20_RO(reset_max);
-
-static struct attribute *nvm_dev_attrs_20[] = {
-

Re: [PATCH 2/5] nvme: register ns_id attributes as default sysfs groups

2018-09-06 Thread Hannes Reinecke 
On 09/05/2018 03:45 PM, Christoph Hellwig wrote:
> On Wed, Sep 05, 2018 at 03:32:03PM +0200, Hannes Reinecke wrote:
>> On 09/05/2018 03:18 PM, Christoph Hellwig wrote:
>>> On Wed, Sep 05, 2018 at 09:00:50AM +0200, Hannes Reinecke wrote:
 We should be registering the ns_id attribute as default sysfs
 attribute groups, otherwise we have a race condition between
 the uevent and the attributes appearing in sysfs.
>>>
>>> Please give Bart credit for his work, as the lightnvm bits are almost
>>> bigger than the rest.
>>>
>> Okay, will be doing so.
>>
 +static umode_t nvm_dev_attrs_visible(struct kobject *kobj,
 +   struct attribute *attr, int index)
  {
 +  struct device *dev = container_of(kobj, struct device, kobj);
 +  struct gendisk *disk = dev_to_disk(dev);
 +  struct nvme_ns *ns = disk->private_data;
struct nvm_dev *ndev = ns->ndev;
 +  struct device_attribute *dev_attr =
 +  container_of(attr, typeof(*dev_attr), attr);
  
 +  if (dev_attr->show == nvm_dev_attr_show)
 +  return attr->mode;
  
 +  switch (ndev ? ndev->geo.major_ver_id : 0) {
>>>
>>> How could ndev be zero here?
>>>
>> For 'normal' NVMe devices (ie non-lightnvm). As we now register all
>> sysfs attributes (including the lightnvm ones) per default we'll need to
>> blank them out for non-lightnvm devices.
> 
> But then we need to exit early at the beginning of the function,
> as we should not register attributes using nvm_dev_attr_show (or
> anything else for that matter) either.
> 
Okay, will be fixing it up.

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/5] nvme: register ns_id attributes as default sysfs groups

2018-09-05 Thread Christoph Hellwig 
On Wed, Sep 05, 2018 at 03:32:03PM +0200, Hannes Reinecke wrote:
> On 09/05/2018 03:18 PM, Christoph Hellwig wrote:
> > On Wed, Sep 05, 2018 at 09:00:50AM +0200, Hannes Reinecke wrote:
> >> We should be registering the ns_id attribute as default sysfs
> >> attribute groups, otherwise we have a race condition between
> >> the uevent and the attributes appearing in sysfs.
> > 
> > Please give Bart credit for his work, as the lightnvm bits are almost
> > bigger than the rest.
> > 
> Okay, will be doing so.
> 
> >> +static umode_t nvm_dev_attrs_visible(struct kobject *kobj,
> >> +   struct attribute *attr, int index)
> >>  {
> >> +  struct device *dev = container_of(kobj, struct device, kobj);
> >> +  struct gendisk *disk = dev_to_disk(dev);
> >> +  struct nvme_ns *ns = disk->private_data;
> >>struct nvm_dev *ndev = ns->ndev;
> >> +  struct device_attribute *dev_attr =
> >> +  container_of(attr, typeof(*dev_attr), attr);
> >>  
> >> +  if (dev_attr->show == nvm_dev_attr_show)
> >> +  return attr->mode;
> >>  
> >> +  switch (ndev ? ndev->geo.major_ver_id : 0) {
> > 
> > How could ndev be zero here?
> > 
> For 'normal' NVMe devices (ie non-lightnvm). As we now register all
> sysfs attributes (including the lightnvm ones) per default we'll need to
> blank them out for non-lightnvm devices.

But then we need to exit early at the beginning of the function,
as we should not register attributes using nvm_dev_attr_show (or
anything else for that matter) either.

Re: [PATCH 2/5] nvme: register ns_id attributes as default sysfs groups

2018-09-05 Thread Hannes Reinecke 
On 09/05/2018 03:18 PM, Christoph Hellwig wrote:
> On Wed, Sep 05, 2018 at 09:00:50AM +0200, Hannes Reinecke wrote:
>> We should be registering the ns_id attribute as default sysfs
>> attribute groups, otherwise we have a race condition between
>> the uevent and the attributes appearing in sysfs.
> 
> Please give Bart credit for his work, as the lightnvm bits are almost
> bigger than the rest.
> 
Okay, will be doing so.

>> +static umode_t nvm_dev_attrs_visible(struct kobject *kobj,
>> + struct attribute *attr, int index)
>>  {
>> +struct device *dev = container_of(kobj, struct device, kobj);
>> +struct gendisk *disk = dev_to_disk(dev);
>> +struct nvme_ns *ns = disk->private_data;
>>  struct nvm_dev *ndev = ns->ndev;
>> +struct device_attribute *dev_attr =
>> +container_of(attr, typeof(*dev_attr), attr);
>>  
>> +if (dev_attr->show == nvm_dev_attr_show)
>> +return attr->mode;
>>  
>> +switch (ndev ? ndev->geo.major_ver_id : 0) {
> 
> How could ndev be zero here?
> 
For 'normal' NVMe devices (ie non-lightnvm). As we now register all
sysfs attributes (including the lightnvm ones) per default we'll need to
blank them out for non-lightnvm devices.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/5] nvme: register ns_id attributes as default sysfs groups

2018-09-05 Thread Christoph Hellwig 
On Wed, Sep 05, 2018 at 09:00:50AM +0200, Hannes Reinecke wrote:
> We should be registering the ns_id attribute as default sysfs
> attribute groups, otherwise we have a race condition between
> the uevent and the attributes appearing in sysfs.

Please give Bart credit for his work, as the lightnvm bits are almost
bigger than the rest.

> +static umode_t nvm_dev_attrs_visible(struct kobject *kobj,
> +  struct attribute *attr, int index)
>  {
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct gendisk *disk = dev_to_disk(dev);
> + struct nvme_ns *ns = disk->private_data;
>   struct nvm_dev *ndev = ns->ndev;
> + struct device_attribute *dev_attr =
> + container_of(attr, typeof(*dev_attr), attr);
>  
> + if (dev_attr->show == nvm_dev_attr_show)
> + return attr->mode;
>  
> + switch (ndev ? ndev->geo.major_ver_id : 0) {

How could ndev be zero here?

[PATCH 2/5] nvme: register ns_id attributes as default sysfs groups

2018-09-05 Thread Hannes Reinecke 
We should be registering the ns_id attribute as default sysfs
attribute groups, otherwise we have a race condition between
the uevent and the attributes appearing in sysfs.

Signed-off-by: Hannes Reinecke 
---
 drivers/nvme/host/core.c  |  21 -
 drivers/nvme/host/lightnvm.c  | 106 +-
 drivers/nvme/host/multipath.c |  15 ++
 drivers/nvme/host/nvme.h  |  10 +---
 4 files changed, 58 insertions(+), 94 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0e824e8c8fd7..e0a9e1c5b30e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2734,6 +2734,14 @@ const struct attribute_group nvme_ns_id_attr_group = {
.is_visible = nvme_ns_id_attrs_are_visible,
 };
 
+const struct attribute_group *nvme_ns_id_attr_groups[] = {
+   &nvme_ns_id_attr_group,
+#ifdef CONFIG_NVM
+   &nvme_nvm_attr_group,
+#endif
+   NULL,
+};
+
 #define nvme_show_str_function(field)  
\
 static ssize_t  field##_show(struct device *dev,   
\
struct device_attribute *attr, char *buf)   
\
@@ -3099,14 +3107,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, 
unsigned nsid)
 
nvme_get_ctrl(ctrl);
 
-   device_add_disk(ctrl->device, ns->disk, NULL);
-   if (sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
-   &nvme_ns_id_attr_group))
-   pr_warn("%s: failed to create sysfs group for identification\n",
-   ns->disk->disk_name);
-   if (ns->ndev && nvme_nvm_register_sysfs(ns))
-   pr_warn("%s: failed to register lightnvm sysfs group for 
identification\n",
-   ns->disk->disk_name);
+   device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
 
nvme_mpath_add_disk(ns, id);
nvme_fault_inject_init(ns);
@@ -3132,10 +3133,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 
nvme_fault_inject_fini(ns);
if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
-   sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
-   &nvme_ns_id_attr_group);
-   if (ns->ndev)
-   nvme_nvm_unregister_sysfs(ns);
del_gendisk(ns->disk);
blk_cleanup_queue(ns->queue);
if (blk_get_integrity(ns->disk))
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 6fe5923c95d4..941ce5fc48f5 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -1190,10 +1190,29 @@ static NVM_DEV_ATTR_12_RO(multiplane_modes);
 static NVM_DEV_ATTR_12_RO(media_capabilities);
 static NVM_DEV_ATTR_12_RO(max_phys_secs);
 
-static struct attribute *nvm_dev_attrs_12[] = {
+/* 2.0 values */
+static NVM_DEV_ATTR_20_RO(groups);
+static NVM_DEV_ATTR_20_RO(punits);
+static NVM_DEV_ATTR_20_RO(chunks);
+static NVM_DEV_ATTR_20_RO(clba);
+static NVM_DEV_ATTR_20_RO(ws_min);
+static NVM_DEV_ATTR_20_RO(ws_opt);
+static NVM_DEV_ATTR_20_RO(maxoc);
+static NVM_DEV_ATTR_20_RO(maxocpu);
+static NVM_DEV_ATTR_20_RO(mw_cunits);
+static NVM_DEV_ATTR_20_RO(write_typ);
+static NVM_DEV_ATTR_20_RO(write_max);
+static NVM_DEV_ATTR_20_RO(reset_typ);
+static NVM_DEV_ATTR_20_RO(reset_max);
+
+static struct attribute *nvm_dev_attrs[] = {
+   /* version agnostic attrs */
&dev_attr_version.attr,
&dev_attr_capabilities.attr,
+   &dev_attr_read_typ.attr,
+   &dev_attr_read_max.attr,
 
+   /* 1.2 attrs */
&dev_attr_vendor_opcode.attr,
&dev_attr_device_mode.attr,
&dev_attr_media_manager.attr,
@@ -1208,8 +1227,6 @@ static struct attribute *nvm_dev_attrs_12[] = {
&dev_attr_page_size.attr,
&dev_attr_hw_sector_size.attr,
&dev_attr_oob_sector_size.attr,
-   &dev_attr_read_typ.attr,
-   &dev_attr_read_max.attr,
&dev_attr_prog_typ.attr,
&dev_attr_prog_max.attr,
&dev_attr_erase_typ.attr,
@@ -1218,33 +1235,7 @@ static struct attribute *nvm_dev_attrs_12[] = {
&dev_attr_media_capabilities.attr,
&dev_attr_max_phys_secs.attr,
 
-   NULL,
-};
-
-static const struct attribute_group nvm_dev_attr_group_12 = {
-   .name   = "lightnvm",
-   .attrs  = nvm_dev_attrs_12,
-};
-
-/* 2.0 values */
-static NVM_DEV_ATTR_20_RO(groups);
-static NVM_DEV_ATTR_20_RO(punits);
-static NVM_DEV_ATTR_20_RO(chunks);
-static NVM_DEV_ATTR_20_RO(clba);
-static NVM_DEV_ATTR_20_RO(ws_min);
-static NVM_DEV_ATTR_20_RO(ws_opt);
-static NVM_DEV_ATTR_20_RO(maxoc);
-static NVM_DEV_ATTR_20_RO(maxocpu);
-static NVM_DEV_ATTR_20_RO(mw_cunits);
-static NVM_DEV_ATTR_20_RO(write_typ);
-static NVM_DEV_ATTR_20_RO(write_max);
-static NVM_DEV_ATTR_20_RO(reset_typ);
-static NVM_DEV_ATTR_20_RO(reset_max);
-
-static struct attribute *nvm_dev_attrs_20[] = {
-   &dev_attr_version.attr,
-