[PATCHv2 1/3] nvme: send uevent on connection up

[PATCHv2 1/3] nvme: send uevent on connection up

[Martin Belanger]

From: Martin Belanger <martin.belanger@dell.com>

When connectivity with a controller is lost, the driver will keep
trying to reconnect once every 10 sec. When connection is restored,
user-space apps need to be informed so that they can take proper
action. For example, TP8010 introduces the DIM PDU, which is used to
register with a discovery controller (DC). The DIM PDU is sent from
user-space.  The DIM PDU must be sent every time a connection is
established with a DC. Therefore, the kernel must tell user-space apps
when connection is restored so that registration can happen.

The uevent sent is a "change" uevent with environmental data
set to: "NVME_EVENT=connected".

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index dd18861f77c0..b5e452aa3c0e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4229,6 +4229,13 @@ static int nvme_class_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return ret;
 }
 
+static void nvme_change_uevent(struct nvme_ctrl *ctrl, char *envdata)
+{
+	char *envp[2] = { envdata, NULL };
+
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+}
+
 static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
 {
 	char *envp[2] = { NULL, NULL };
@@ -4396,6 +4403,8 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 		nvme_queue_scan(ctrl);
 		nvme_start_queues(ctrl);
 	}
+
+	nvme_change_uevent(ctrl, "NVME_EVENT=connected");
 }
 EXPORT_SYMBOL_GPL(nvme_start_ctrl);
 
-- 
2.34.1

[PATCHv2 2/3] nvme: Add device_update_groups()

[Martin Belanger]

From: Martin Belanger <martin.belanger@dell.com>

This is used to update the sysfs groups after device objects are fully
initialized.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/base/core.c    | 28 ++++++++++++++++++++++++++++
 include/linux/device.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3d6430eb0c6a..0b5f6d5d2459 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4473,6 +4473,34 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
 }
 EXPORT_SYMBOL_GPL(device_change_owner);
 
+/**
+ * device_update_groups - update the sysfs groups
+ * @dev: device.
+ *
+ * Returns 0 on success or error code on failure.
+ */
+int device_update_groups(struct device *dev)
+{
+	struct class *class = dev->class;
+	const struct device_type *type = dev->type;
+	int error;
+
+	if (class) {
+		error = sysfs_update_groups(&dev->kobj, class->dev_groups);
+		if (error)
+			return error;
+	}
+
+	if (type) {
+		error = sysfs_update_groups(&dev->kobj, type->groups);
+		if (error)
+			return error;
+	}
+
+	return sysfs_update_groups(&dev->kobj, dev->groups);
+}
+EXPORT_SYMBOL_GPL(device_update_groups);
+
 /**
  * device_shutdown - call ->shutdown() on each device to shutdown.
  */
diff --git a/include/linux/device.h b/include/linux/device.h
index 93459724dcde..a623790173ce 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -841,6 +841,7 @@ int device_rename(struct device *dev, const char *new_name);
 int device_move(struct device *dev, struct device *new_parent,
 		enum dpm_order dpm_order);
 int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid);
+int device_update_groups(struct device *dev);
 const char *device_get_devnode(struct device *dev, umode_t *mode, kuid_t *uid,
 			       kgid_t *gid, const char **tmp);
 int device_is_dependent(struct device *dev, void *target);
-- 
2.34.1

Re: [PATCHv2 2/3] nvme: Add device_update_groups()

[Chaitanya Kulkarni]

On 2/3/22 13:17, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> This is used to update the sysfs groups after device objects are fully
> initialized.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/base/core.c    | 28 ++++++++++++++++++++++++++++
>   include/linux/device.h |  1 +
>   2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 3d6430eb0c6a..0b5f6d5d2459 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4473,6 +4473,34 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
>   }
>   EXPORT_SYMBOL_GPL(device_change_owner);
>   
> +/**
> + * device_update_groups - update the sysfs groups
> + * @dev: device.
> + *
> + * Returns 0 on success or error code on failure.
> + */
> +int device_update_groups(struct device *dev)
> +{
> +	struct class *class = dev->class;
> +	const struct device_type *type = dev->type;
> +	int error;
> +

consider following, can be done at the time of applying patch :-
	const struct device_type *type = dev->type;
	struct class *class = dev->class;
	int error;

> +	if (class) {
> +		error = sysfs_update_groups(&dev->kobj, class->dev_groups);
> +		if (error)
> +			return error;
> +	}
> +
> +	if (type) {
> +		error = sysfs_update_groups(&dev->kobj, type->groups);
> +		if (error)
> +			return error;
> +	}
> +
> +	return sysfs_update_groups(&dev->kobj, dev->groups);
> +}
> +EXPORT_SYMBOL_GPL(device_update_groups);
> +
>   /**
>    * device_shutdown - call ->shutdown() on each device to shutdown.
>    */
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 93459724dcde..a623790173ce 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -841,6 +841,7 @@ int device_rename(struct device *dev, const char *new_name);
>   int device_move(struct device *dev, struct device *new_parent,
>   		enum dpm_order dpm_order);
>   int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid);
> +int device_update_groups(struct device *dev);
>   const char *device_get_devnode(struct device *dev, umode_t *mode, kuid_t *uid,
>   			       kgid_t *gid, const char **tmp);
>   int device_is_dependent(struct device *dev, void *target);
> 

driver core patch, was Re: [PATCHv2 2/3] nvme: Add device_update_groups()

[Christoph Hellwig]

On Thu, Feb 03, 2022 at 04:17:47PM -0500, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> This is used to update the sysfs groups after device objects are fully
> initialized.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>

This isn't a nvme patch, but a driver model one.  Adjusting the subject
to bring it to Gregs attention.

Re: driver core patch, was Re: [PATCHv2 2/3] nvme: Add device_update_groups()

[Greg KH]

On Fri, Feb 04, 2022 at 08:04:41AM +0100, Christoph Hellwig wrote:
> On Thu, Feb 03, 2022 at 04:17:47PM -0500, Martin Belanger wrote:
> > From: Martin Belanger <martin.belanger@dell.com>
> > 
> > This is used to update the sysfs groups after device objects are fully
> > initialized.
> > 
> > Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> 
> This isn't a nvme patch, but a driver model one.  Adjusting the subject
> to bring it to Gregs attention.

Thanks for that, now NAKed :)

Re: [PATCHv2 2/3] nvme: Add device_update_groups()

[Hannes Reinecke]

On 2/3/22 22:17, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> This is used to update the sysfs groups after device objects are fully
> initialized.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/base/core.c    | 28 ++++++++++++++++++++++++++++
>   include/linux/device.h |  1 +
>   2 files changed, 29 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCHv2 2/3] nvme: Add device_update_groups()

[Greg KH]

On Thu, Feb 03, 2022 at 04:17:47PM -0500, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> This is used to update the sysfs groups after device objects are fully
> initialized.

Ick, no, why?

What needs this and how is userspace going to handle groups changing
after the device is already announced to userspace?


> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>  drivers/base/core.c    | 28 ++++++++++++++++++++++++++++
>  include/linux/device.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 3d6430eb0c6a..0b5f6d5d2459 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4473,6 +4473,34 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
>  }
>  EXPORT_SYMBOL_GPL(device_change_owner);
>  
> +/**
> + * device_update_groups - update the sysfs groups
> + * @dev: device.
> + *
> + * Returns 0 on success or error code on failure.
> + */
> +int device_update_groups(struct device *dev)
> +{
> +	struct class *class = dev->class;
> +	const struct device_type *type = dev->type;
> +	int error;
> +
> +	if (class) {
> +		error = sysfs_update_groups(&dev->kobj, class->dev_groups);

So you added new groups based on the class for the device after it was
registered?

That feels really wrong.


> +		if (error)
> +			return error;
> +	}
> +
> +	if (type) {
> +		error = sysfs_update_groups(&dev->kobj, type->groups);

How can the type groups have changed for the device after it was
registered?

What problem are you trying to solve here that requires this?  How are
you changing the groups for a class or type after a device is
registered?

confused,

greg k-h

[PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Martin Belanger]

From: Martin Belanger <martin.belanger@dell.com>

TP8010 introduces the Discovery Controller Type attribute (dctype).
The dctype is returned in the response to the Identify command. This
patch exposes the dctype through the sysfs. Since the dctype depends on
the Controller Type (cntrltype), another attribute of the Identify
response, the patch also exposes the cntrltype as well. The dctype will
only be displayed for discovery controllers.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/core.c | 45 ++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  3 +++
 include/linux/nvme.h     | 10 ++++++++-
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b5e452aa3c0e..4e3db5ec3924 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->max_namespaces = le32_to_cpu(id->mnan);
 	ctrl->ctratt = le32_to_cpu(id->ctratt);
 
+	ctrl->cntrltype = id->cntrltype;
+	ctrl->dctype = id->dctype;
+
 	if (id->rtd3e) {
 		/* us -> s */
 		u32 transition_time = le32_to_cpu(id->rtd3e) / USEC_PER_SEC;
@@ -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
 			return ret;
 	}
 
+	ret = device_update_groups(ctrl->device);
+	if (ret)
+		return ret;
+
 	ctrl->identified = true;
 
 	return 0;
@@ -3523,6 +3530,40 @@ static ssize_t nvme_ctrl_fast_io_fail_tmo_store(struct device *dev,
 static DEVICE_ATTR(fast_io_fail_tmo, S_IRUGO | S_IWUSR,
 	nvme_ctrl_fast_io_fail_tmo_show, nvme_ctrl_fast_io_fail_tmo_store);
 
+static ssize_t nvme_ctrl_cntrltype_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	const char *type[] = {
+		[NVME_CTRL_IO] = "io\n",
+		[NVME_CTRL_DISC] = "discovery\n",
+		[NVME_CTRL_ADMIN] = "admin\n",
+	};
+
+	if (ctrl->cntrltype > NVME_CTRL_ADMIN || !type[ctrl->cntrltype])
+		return sysfs_emit(buf, "reserved\n");
+
+	return sysfs_emit(buf, type[ctrl->cntrltype]);
+}
+static DEVICE_ATTR(cntrltype, S_IRUGO, nvme_ctrl_cntrltype_show, NULL);
+
+static ssize_t nvme_ctrl_dctype_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	const char *type[] = {
+		[NVME_DCTYPE_NOT_REPORTED] = "none\n",
+		[NVME_DCTYPE_DDC] = "ddc\n",
+		[NVME_DCTYPE_CDC] = "cdc\n",
+	};
+
+	if (ctrl->dctype > NVME_DCTYPE_CDC || !type[ctrl->dctype])
+		return sysfs_emit(buf, "reserved\n");
+
+	return sysfs_emit(buf, type[ctrl->dctype]);
+}
+static DEVICE_ATTR(dctype, S_IRUGO, nvme_ctrl_dctype_show, NULL);
+
 static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_reset_controller.attr,
 	&dev_attr_rescan_controller.attr,
@@ -3544,6 +3585,8 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_reconnect_delay.attr,
 	&dev_attr_fast_io_fail_tmo.attr,
 	&dev_attr_kato.attr,
+	&dev_attr_cntrltype.attr,
+	&dev_attr_dctype.attr,
 	NULL
 };
 
@@ -3567,6 +3610,8 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
 		return 0;
 	if (a == &dev_attr_fast_io_fail_tmo.attr && !ctrl->opts)
 		return 0;
+	if (a == &dev_attr_dctype.attr && ctrl->cntrltype != NVME_CTRL_DISC)
+		return 0;
 
 	return a->mode;
 }
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a162f6c6da6e..e30626c00791 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -349,6 +349,9 @@ struct nvme_ctrl {
 	unsigned long discard_page_busy;
 
 	struct nvme_fault_inject fault_inject;
+
+	enum nvme_ctrl_type cntrltype;
+	enum nvme_dctype dctype;
 };
 
 enum nvme_iopolicy {
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 855dd9b3e84b..21e92e97ca88 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -43,6 +43,12 @@ enum nvme_ctrl_type {
 	NVME_CTRL_ADMIN	= 3,		/* Administrative controller */
 };
 
+enum nvme_dctype {
+	NVME_DCTYPE_NOT_REPORTED	= 0,
+	NVME_DCTYPE_DDC			= 1, /* Direct Discovery Controller */
+	NVME_DCTYPE_CDC			= 2, /* Central Discovery Controller */
+};
+
 /* Address Family codes for Discovery Log Page entry ADRFAM field */
 enum {
 	NVMF_ADDR_FAMILY_PCI	= 0,	/* PCIe */
@@ -320,7 +326,9 @@ struct nvme_id_ctrl {
 	__le16			icdoff;
 	__u8			ctrattr;
 	__u8			msdbd;
-	__u8			rsvd1804[244];
+	__u8			rsvd1804[2];
+	__u8			dctype;
+	__u8			rsvd1807[241];
 	struct nvme_id_power_state	psd[32];
 	__u8			vs[1024];
 };
-- 
2.34.1

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Hannes Reinecke]

On 2/3/22 22:17, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the Discovery Controller Type attribute (dctype).
> The dctype is returned in the response to the Identify command. This
> patch exposes the dctype through the sysfs. Since the dctype depends on
> the Controller Type (cntrltype), another attribute of the Identify
> response, the patch also exposes the cntrltype as well. The dctype will
> only be displayed for discovery controllers.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/core.c | 45 ++++++++++++++++++++++++++++++++++++++++
>   drivers/nvme/host/nvme.h |  3 +++
>   include/linux/nvme.h     | 10 ++++++++-
>   3 files changed, 57 insertions(+), 1 deletion(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Greg KH]

On Thu, Feb 03, 2022 at 04:17:48PM -0500, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the Discovery Controller Type attribute (dctype).
> The dctype is returned in the response to the Identify command. This
> patch exposes the dctype through the sysfs. Since the dctype depends on
> the Controller Type (cntrltype), another attribute of the Identify
> response, the patch also exposes the cntrltype as well. The dctype will
> only be displayed for discovery controllers.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>  drivers/nvme/host/core.c | 45 ++++++++++++++++++++++++++++++++++++++++
>  drivers/nvme/host/nvme.h |  3 +++
>  include/linux/nvme.h     | 10 ++++++++-
>  3 files changed, 57 insertions(+), 1 deletion(-)

Where is the documentation for these new sysfs files?  That is required
to be in Documentation/ABI/

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index b5e452aa3c0e..4e3db5ec3924 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>  	ctrl->max_namespaces = le32_to_cpu(id->mnan);
>  	ctrl->ctratt = le32_to_cpu(id->ctratt);
>  
> +	ctrl->cntrltype = id->cntrltype;
> +	ctrl->dctype = id->dctype;
> +
>  	if (id->rtd3e) {
>  		/* us -> s */
>  		u32 transition_time = le32_to_cpu(id->rtd3e) / USEC_PER_SEC;
> @@ -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
>  			return ret;
>  	}
>  
> +	ret = device_update_groups(ctrl->device);
> +	if (ret)
> +		return ret;
> +
>  	ctrl->identified = true;
>  
>  	return 0;
> @@ -3523,6 +3530,40 @@ static ssize_t nvme_ctrl_fast_io_fail_tmo_store(struct device *dev,
>  static DEVICE_ATTR(fast_io_fail_tmo, S_IRUGO | S_IWUSR,
>  	nvme_ctrl_fast_io_fail_tmo_show, nvme_ctrl_fast_io_fail_tmo_store);
>  
> +static ssize_t nvme_ctrl_cntrltype_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +	const char *type[] = {
> +		[NVME_CTRL_IO] = "io\n",
> +		[NVME_CTRL_DISC] = "discovery\n",
> +		[NVME_CTRL_ADMIN] = "admin\n",
> +	};
> +
> +	if (ctrl->cntrltype > NVME_CTRL_ADMIN || !type[ctrl->cntrltype])
> +		return sysfs_emit(buf, "reserved\n");
> +
> +	return sysfs_emit(buf, type[ctrl->cntrltype]);
> +}
> +static DEVICE_ATTR(cntrltype, S_IRUGO, nvme_ctrl_cntrltype_show, NULL);

DEVICE_ATTR_RO() please.

> +
> +static ssize_t nvme_ctrl_dctype_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +	const char *type[] = {
> +		[NVME_DCTYPE_NOT_REPORTED] = "none\n",
> +		[NVME_DCTYPE_DDC] = "ddc\n",
> +		[NVME_DCTYPE_CDC] = "cdc\n",
> +	};
> +
> +	if (ctrl->dctype > NVME_DCTYPE_CDC || !type[ctrl->dctype])
> +		return sysfs_emit(buf, "reserved\n");
> +
> +	return sysfs_emit(buf, type[ctrl->dctype]);
> +}
> +static DEVICE_ATTR(dctype, S_IRUGO, nvme_ctrl_dctype_show, NULL);

Same here.

thanks,

greg k-h

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Greg KH]

On Thu, Feb 03, 2022 at 04:17:48PM -0500, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the Discovery Controller Type attribute (dctype).
> The dctype is returned in the response to the Identify command. This
> patch exposes the dctype through the sysfs. Since the dctype depends on
> the Controller Type (cntrltype), another attribute of the Identify
> response, the patch also exposes the cntrltype as well. The dctype will
> only be displayed for discovery controllers.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>  drivers/nvme/host/core.c | 45 ++++++++++++++++++++++++++++++++++++++++
>  drivers/nvme/host/nvme.h |  3 +++
>  include/linux/nvme.h     | 10 ++++++++-
>  3 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index b5e452aa3c0e..4e3db5ec3924 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>  	ctrl->max_namespaces = le32_to_cpu(id->mnan);
>  	ctrl->ctratt = le32_to_cpu(id->ctratt);
>  
> +	ctrl->cntrltype = id->cntrltype;
> +	ctrl->dctype = id->dctype;
> +
>  	if (id->rtd3e) {
>  		/* us -> s */
>  		u32 transition_time = le32_to_cpu(id->rtd3e) / USEC_PER_SEC;
> @@ -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
>  			return ret;
>  	}
>  
> +	ret = device_update_groups(ctrl->device);
> +	if (ret)
> +		return ret;

Why is this needed here?  How did the class or type just change?  That
should never change over the lifespan of a device.  If it does, you need
to tear down the old device and create a new one as something really
wrong just happened.

thanks,

greg k-h

RE: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Belanger, Martin]

> On Thu, Feb 03, 2022 at 04:17:48PM -0500, Martin Belanger wrote:
> > From: Martin Belanger <martin.belanger@dell.com>
> >
> > TP8010 introduces the Discovery Controller Type attribute (dctype).
> > The dctype is returned in the response to the Identify command. This
> > patch exposes the dctype through the sysfs. Since the dctype depends
> > on the Controller Type (cntrltype), another attribute of the Identify
> > response, the patch also exposes the cntrltype as well. The dctype
> > will only be displayed for discovery controllers.
> >
> > Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> > ---
> >  drivers/nvme/host/core.c | 45
> > ++++++++++++++++++++++++++++++++++++++++
> >  drivers/nvme/host/nvme.h |  3 +++
> >  include/linux/nvme.h     | 10 ++++++++-
> >  3 files changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index
> > b5e452aa3c0e..4e3db5ec3924 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct nvme_ctrl
> *ctrl)
> >  	ctrl->max_namespaces = le32_to_cpu(id->mnan);
> >  	ctrl->ctratt = le32_to_cpu(id->ctratt);
> >
> > +	ctrl->cntrltype = id->cntrltype;
> > +	ctrl->dctype = id->dctype;
> > +
> >  	if (id->rtd3e) {
> >  		/* us -> s */
> >  		u32 transition_time = le32_to_cpu(id->rtd3e) /
> USEC_PER_SEC; @@
> > -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
> >  			return ret;
> >  	}
> >
> > +	ret = device_update_groups(ctrl->device);
> > +	if (ret)
> > +		return ret;
> 
> Why is this needed here?  How did the class or type just change?  That should
> never change over the lifespan of a device.  If it does, you need to tear down
> the old device and create a new one as something really wrong just
> happened.

Hi Greg,
I need to expose 2 nvme attributes through the sysfs. There is the controller type (cntrltype), which can take the values "io", "discovery", "admin", and "reserved". And there is the Discovery Controller Type (dctype), which takes the values "none", "ddc", "cdc", and "reserved". Note that the dctype is only meaningful for Discovery Controllers (i.e. cntrltype=discovery).

In my first iteration of this patch, I plainly exposed both attributes for all types of controllers. However, Sagi asked me to only expose the dctype if we're dealing with a Discovery Controller. Unfortunately, both cntrltype and dctype are not known at object creation. This means that when the is_visible method (i.e. nvme_dev_attrs_are_visible) is called, it is still not known whether dctype should be exposed. The value s for cntrltype and dctype are only known after a connection has been established with the controller and we have completed the Identify command.

Hannes suggested that we combine both attributes into a single one that would take the values: "io", "discovery-none", "discovery-ddc", "discovery-cdc", "discovery-reserved", and "admin". But Sagi did not like this proposal and instead proposed the changes that you see here. Basically, we want to be able to re-execute the is_visible() method after we have identified the controller so that we can determine whether the dctype should be exposed.

So now I'm in a bit of a pickle. Sagi suggested this current patch set, but from what you're saying this is not a good approach. If you could suggest a solution that would satisfy all parties, I would greatly appreciate it.

Regards,
Martin

> 
> thanks,
> 
> greg k-h

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Greg KH]

On Fri, Feb 04, 2022 at 01:51:27PM +0000, Belanger, Martin wrote:
> > On Thu, Feb 03, 2022 at 04:17:48PM -0500, Martin Belanger wrote:
> > > From: Martin Belanger <martin.belanger@dell.com>
> > >
> > > TP8010 introduces the Discovery Controller Type attribute (dctype).
> > > The dctype is returned in the response to the Identify command. This
> > > patch exposes the dctype through the sysfs. Since the dctype depends
> > > on the Controller Type (cntrltype), another attribute of the Identify
> > > response, the patch also exposes the cntrltype as well. The dctype
> > > will only be displayed for discovery controllers.
> > >
> > > Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> > > ---
> > >  drivers/nvme/host/core.c | 45
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/nvme/host/nvme.h |  3 +++
> > >  include/linux/nvme.h     | 10 ++++++++-
> > >  3 files changed, 57 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index
> > > b5e452aa3c0e..4e3db5ec3924 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct nvme_ctrl
> > *ctrl)
> > >  	ctrl->max_namespaces = le32_to_cpu(id->mnan);
> > >  	ctrl->ctratt = le32_to_cpu(id->ctratt);
> > >
> > > +	ctrl->cntrltype = id->cntrltype;
> > > +	ctrl->dctype = id->dctype;
> > > +
> > >  	if (id->rtd3e) {
> > >  		/* us -> s */
> > >  		u32 transition_time = le32_to_cpu(id->rtd3e) /
> > USEC_PER_SEC; @@
> > > -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
> > >  			return ret;
> > >  	}
> > >
> > > +	ret = device_update_groups(ctrl->device);
> > > +	if (ret)
> > > +		return ret;
> > 
> > Why is this needed here?  How did the class or type just change?  That should
> > never change over the lifespan of a device.  If it does, you need to tear down
> > the old device and create a new one as something really wrong just
> > happened.
> 
> Hi Greg,
> I need to expose 2 nvme attributes through the sysfs. There is the controller type (cntrltype), which can take the values "io", "discovery", "admin", and "reserved". And there is the Discovery Controller Type (dctype), which takes the values "none", "ddc", "cdc", and "reserved". Note that the dctype is only meaningful for Discovery Controllers (i.e. cntrltype=discovery).

/me handes you some '\n' characters...

That's fine, this should not have anything to do with the groups
associated with a device as this type is known at creation, right?

> In my first iteration of this patch, I plainly exposed both attributes for all types of controllers. However, Sagi asked me to only expose the dctype if we're dealing with a Discovery Controller. Unfortunately, both cntrltype and dctype are not known at object creation. This means that when the is_visible method (i.e. nvme_dev_attrs_are_visible) is called, it is still not known whether dctype should be exposed. The value s for cntrltype and dctype are only known after a connection has been established with the controller and we have completed the Identify command.

How can this not be known at creation?  Why not figure it out first
before you create the device?  Why not wait?

> Hannes suggested that we combine both attributes into a single one that would take the values: "io", "discovery-none", "discovery-ddc", "discovery-cdc", "discovery-reserved", and "admin". But Sagi did not like this proposal and instead proposed the changes that you see here. Basically, we want to be able to re-execute the is_visible() method after we have identified the controller so that we can determine whether the dctype should be exposed.

Nope, that's not how the driver model works and you break all userspace
tools by doing this as it only scans the list of attributes when the
device is created and announced to userspace.

By changing the files and types and such afterward no one ever notices
and so userspace is "blind" to the change unless you want it to
constantly walk all of sysfs all the time to find this out (hint, you
don't want that...)

So there's a reason you are being forced to add a driver-core change
like this, because it's not something you should do :)

> So now I'm in a bit of a pickle. Sagi suggested this current patch set, but from what you're saying this is not a good approach. If you could suggest a solution that would satisfy all parties, I would greatly appreciate it.

Wait to create the device when you know the device type.

thanks,

greg k-h

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Sagi Grimberg]

>> Hannes suggested that we combine both attributes into a single one that would take the values: "io", "discovery-none", "discovery-ddc", "discovery-cdc", "discovery-reserved", and "admin". But Sagi did not like this proposal and instead proposed the changes that you see here. Basically, we want to be able to re-execute the is_visible() method after we have identified the controller so that we can determine whether the dctype should be exposed.

Just to note that the same fundamental issue exists in both approaches,
having the "combined" string change after the controller is identified
is equivalent to having the separated attribute appear after the device
is initialized.

> Nope, that's not how the driver model works and you break all userspace
> tools by doing this as it only scans the list of attributes when the
> device is created and announced to userspace.
> 
> By changing the files and types and such afterward no one ever notices
> and so userspace is "blind" to the change unless you want it to
> constantly walk all of sysfs all the time to find this out (hint, you
> don't want that...)

The idea was to add a separate udev announcement that would tell
userspace that these attributes are stable (it actually serves
a different purpose, but it does come after that point).

> 
> So there's a reason you are being forced to add a driver-core change
> like this, because it's not something you should do :)

I do agree that its a bit backwards to do this update. But the device
even today is not fully usable when it is created, so we know no
userspace program relies on the creation announcement.

>> So now I'm in a bit of a pickle. Sagi suggested this current patch set, but from what you're saying this is not a good approach. If you could suggest a solution that would satisfy all parties, I would greatly appreciate it.
> 
> Wait to create the device when you know the device type.

The initial proposal was to defer the device creation to after the
device is fully initialized. That though is a heavier lift because the
device initialization is something that happens on every reset or
reconnection after connectivity loss, so if we move the device creation
there we have to now check if this is the first time we are doing this
or not, and also error recovery becomes more opinionated...

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Greg KH]

On Sun, Feb 06, 2022 at 11:09:40AM +0200, Sagi Grimberg wrote:
> 
> > > Hannes suggested that we combine both attributes into a single one that would take the values: "io", "discovery-none", "discovery-ddc", "discovery-cdc", "discovery-reserved", and "admin". But Sagi did not like this proposal and instead proposed the changes that you see here. Basically, we want to be able to re-execute the is_visible() method after we have identified the controller so that we can determine whether the dctype should be exposed.
> 
> Just to note that the same fundamental issue exists in both approaches,
> having the "combined" string change after the controller is identified
> is equivalent to having the separated attribute appear after the device
> is initialized.
> 
> > Nope, that's not how the driver model works and you break all userspace
> > tools by doing this as it only scans the list of attributes when the
> > device is created and announced to userspace.
> > 
> > By changing the files and types and such afterward no one ever notices
> > and so userspace is "blind" to the change unless you want it to
> > constantly walk all of sysfs all the time to find this out (hint, you
> > don't want that...)
> 
> The idea was to add a separate udev announcement that would tell
> userspace that these attributes are stable (it actually serves
> a different purpose, but it does come after that point).

like KOBJECT_CHANGE is there for?

> > So there's a reason you are being forced to add a driver-core change
> > like this, because it's not something you should do :)
> 
> I do agree that its a bit backwards to do this update. But the device
> even today is not fully usable when it is created, so we know no
> userspace program relies on the creation announcement.
> 
> > > So now I'm in a bit of a pickle. Sagi suggested this current patch set, but from what you're saying this is not a good approach. If you could suggest a solution that would satisfy all parties, I would greatly appreciate it.
> > 
> > Wait to create the device when you know the device type.
> 
> The initial proposal was to defer the device creation to after the
> device is fully initialized. That though is a heavier lift because the
> device initialization is something that happens on every reset or
> reconnection after connectivity loss,

That's fine, what's wrong with tearing it all down and bringing it back
up then?  That's what the hardware just did, don't fake things out by
not conveying the same stuff to userspace.

> so if we move the device creation there we have to now check if this
> is the first time we are doing this or not, and also error recovery
> becomes more opinionated...

I still think you should not create anything until you know what it is.

thanks,

greg k-h

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Sagi Grimberg]

>> The idea was to add a separate udev announcement that would tell
>> userspace that these attributes are stable (it actually serves
>> a different purpose, but it does come after that point).
> 
> like KOBJECT_CHANGE is there for?

Yes, that was the point, to add a change event (patch 1 in
the series).

>>> So there's a reason you are being forced to add a driver-core change
>>> like this, because it's not something you should do :)
>>
>> I do agree that its a bit backwards to do this update. But the device
>> even today is not fully usable when it is created, so we know no
>> userspace program relies on the creation announcement.
>>
>>>> So now I'm in a bit of a pickle. Sagi suggested this current patch set, but from what you're saying this is not a good approach. If you could suggest a solution that would satisfy all parties, I would greatly appreciate it.
>>>
>>> Wait to create the device when you know the device type.
>>
>> The initial proposal was to defer the device creation to after the
>> device is fully initialized. That though is a heavier lift because the
>> device initialization is something that happens on every reset or
>> reconnection after connectivity loss,
> 
> That's fine, what's wrong with tearing it all down and bringing it back
> up then?  That's what the hardware just did, don't fake things out by
> not conveying the same stuff to userspace.
> 
>> so if we move the device creation there we have to now check if this
>> is the first time we are doing this or not, and also error recovery
>> becomes more opinionated...
> 
> I still think you should not create anything until you know what it is.

We probably should take a step back and understand if we can delay the
controller device creation. There are some corner cases that I'm
concerned we might introduce if we do that, but it may be the right
thing to do...

Or, we can expose this specific attribute via a different interface like
ioctl, or embedding it in the uevent that we send anyways...

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Sagi Grimberg]

Hey Greg,

>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index b5e452aa3c0e..4e3db5ec3924 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>>   	ctrl->max_namespaces = le32_to_cpu(id->mnan);
>>   	ctrl->ctratt = le32_to_cpu(id->ctratt);
>>   
>> +	ctrl->cntrltype = id->cntrltype;
>> +	ctrl->dctype = id->dctype;
>> +
>>   	if (id->rtd3e) {
>>   		/* us -> s */
>>   		u32 transition_time = le32_to_cpu(id->rtd3e) / USEC_PER_SEC;
>> @@ -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
>>   			return ret;
>>   	}
>>   
>> +	ret = device_update_groups(ctrl->device);
>> +	if (ret)
>> +		return ret;
> 
> Why is this needed here?  How did the class or type just change?  That
> should never change over the lifespan of a device.  If it does, you need
> to tear down the old device and create a new one as something really
> wrong just happened.

The class and type do not change. The fundamental need here is to update
visible attributes of the device.

In nvme, we create the device node with its sysfs attributes and next
comes a process where we interrogate it for capabilities/personality.
In this example, the attributes cntrltype and dctype are learned from
the device identification, and the desire is that dctype will only be
visible for specific cntrltype (i.e. cntrltype is I/O controllers vs.
discovery controllers and dctype is the discovery controller type).

So perhaps a more narrowed interface to update attributes
visibility of the controller would be more appropriate instead?

Something like:
--
	ret = device_update_visibility(ctrl->device->groups);
--

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Greg KH]

On Sun, Feb 06, 2022 at 10:54:54AM +0200, Sagi Grimberg wrote:
> Hey Greg,
> 
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > index b5e452aa3c0e..4e3db5ec3924 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
> > >   	ctrl->max_namespaces = le32_to_cpu(id->mnan);
> > >   	ctrl->ctratt = le32_to_cpu(id->ctratt);
> > > +	ctrl->cntrltype = id->cntrltype;
> > > +	ctrl->dctype = id->dctype;
> > > +
> > >   	if (id->rtd3e) {
> > >   		/* us -> s */
> > >   		u32 transition_time = le32_to_cpu(id->rtd3e) / USEC_PER_SEC;
> > > @@ -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
> > >   			return ret;
> > >   	}
> > > +	ret = device_update_groups(ctrl->device);
> > > +	if (ret)
> > > +		return ret;
> > 
> > Why is this needed here?  How did the class or type just change?  That
> > should never change over the lifespan of a device.  If it does, you need
> > to tear down the old device and create a new one as something really
> > wrong just happened.
> 
> The class and type do not change. The fundamental need here is to update
> visible attributes of the device.

Then change the attributes and issue a uevent of KOBJECT_CHANGE like
other objects do today.  But be aware that userspace might have problems
with this.  Please test your tools to verify this works or not.

> In nvme, we create the device node with its sysfs attributes and next
> comes a process where we interrogate it for capabilities/personality.

That feels wrong, just delay, there's no rush.

> In this example, the attributes cntrltype and dctype are learned from
> the device identification, and the desire is that dctype will only be
> visible for specific cntrltype (i.e. cntrltype is I/O controllers vs.
> discovery controllers and dctype is the discovery controller type).
> 
> So perhaps a more narrowed interface to update attributes
> visibility of the controller would be more appropriate instead?
> 
> Something like:
> --
> 	ret = device_update_visibility(ctrl->device->groups);
> --

Again, suffers from the same issue where userspace is not notified.

Again, just delay and create the real object when you know what you
have.

thanks,

greg k-h

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Hannes Reinecke]

On 2/6/22 09:54, Sagi Grimberg wrote:
> Hey Greg,
> 
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index b5e452aa3c0e..4e3db5ec3924 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct nvme_ctrl 
>>> *ctrl)
>>>       ctrl->max_namespaces = le32_to_cpu(id->mnan);
>>>       ctrl->ctratt = le32_to_cpu(id->ctratt);
>>> +    ctrl->cntrltype = id->cntrltype;
>>> +    ctrl->dctype = id->dctype;
>>> +
>>>       if (id->rtd3e) {
>>>           /* us -> s */
>>>           u32 transition_time = le32_to_cpu(id->rtd3e) / USEC_PER_SEC;
>>> @@ -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
>>>               return ret;
>>>       }
>>> +    ret = device_update_groups(ctrl->device);
>>> +    if (ret)
>>> +        return ret;
>>
>> Why is this needed here?  How did the class or type just change?  That
>> should never change over the lifespan of a device.  If it does, you need
>> to tear down the old device and create a new one as something really
>> wrong just happened.
> 
> The class and type do not change. The fundamental need here is to update
> visible attributes of the device.
> 
> In nvme, we create the device node with its sysfs attributes and next
> comes a process where we interrogate it for capabilities/personality.
> In this example, the attributes cntrltype and dctype are learned from
> the device identification, and the desire is that dctype will only be
> visible for specific cntrltype (i.e. cntrltype is I/O controllers vs.
> discovery controllers and dctype is the discovery controller type).
> 
> So perhaps a more narrowed interface to update attributes
> visibility of the controller would be more appropriate instead?
> 
> Something like:
> -- 
>      ret = device_update_visibility(ctrl->device->groups);
> -- 

I guess it can be even simpler; to my reading the prime objection is 
that we change attributes (and attribute visibility) without informing 
userspace that we did so, thus udev and the device configuration driven 
by uevents won't work as expected.

Which means that everything should be okay if we issue an KOBJ_CHANGE 
uevent after we modify the visibility of attributes, no?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Greg KH]

On Sun, Feb 06, 2022 at 02:08:32PM +0100, Hannes Reinecke wrote:
> On 2/6/22 09:54, Sagi Grimberg wrote:
> > Hey Greg,
> > 
> > > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > > index b5e452aa3c0e..4e3db5ec3924 100644
> > > > --- a/drivers/nvme/host/core.c
> > > > +++ b/drivers/nvme/host/core.c
> > > > @@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct
> > > > nvme_ctrl *ctrl)
> > > >       ctrl->max_namespaces = le32_to_cpu(id->mnan);
> > > >       ctrl->ctratt = le32_to_cpu(id->ctratt);
> > > > +    ctrl->cntrltype = id->cntrltype;
> > > > +    ctrl->dctype = id->dctype;
> > > > +
> > > >       if (id->rtd3e) {
> > > >           /* us -> s */
> > > >           u32 transition_time = le32_to_cpu(id->rtd3e) / USEC_PER_SEC;
> > > > @@ -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
> > > >               return ret;
> > > >       }
> > > > +    ret = device_update_groups(ctrl->device);
> > > > +    if (ret)
> > > > +        return ret;
> > > 
> > > Why is this needed here?  How did the class or type just change?  That
> > > should never change over the lifespan of a device.  If it does, you need
> > > to tear down the old device and create a new one as something really
> > > wrong just happened.
> > 
> > The class and type do not change. The fundamental need here is to update
> > visible attributes of the device.
> > 
> > In nvme, we create the device node with its sysfs attributes and next
> > comes a process where we interrogate it for capabilities/personality.
> > In this example, the attributes cntrltype and dctype are learned from
> > the device identification, and the desire is that dctype will only be
> > visible for specific cntrltype (i.e. cntrltype is I/O controllers vs.
> > discovery controllers and dctype is the discovery controller type).
> > 
> > So perhaps a more narrowed interface to update attributes
> > visibility of the controller would be more appropriate instead?
> > 
> > Something like:
> > -- 
> >      ret = device_update_visibility(ctrl->device->groups);
> > -- 
> 
> I guess it can be even simpler; to my reading the prime objection is that we
> change attributes (and attribute visibility) without informing userspace
> that we did so, thus udev and the device configuration driven by uevents
> won't work as expected.
> 
> Which means that everything should be okay if we issue an KOBJ_CHANGE uevent
> after we modify the visibility of attributes, no?

Maybe.  It depends on what you want userspace to do here.  Most
KOBJ_CHANGE uevents are not watched by the normal userspace rules as
they are used to only watching for add/remove events.  change events are
much more rare and device-specific.

So I would strongly recommend you not use them unless you know your
userspace tools can handle them and will do the right thing.

thanks,

greg k-h