[PATCH 0/5] Feature enhancements for ses module

[PATCH 0/5] Feature enhancements for ses module

[Song Liu]

1: close potential race condition by at enclosure race condition

2,3,4: add enclosure id and device slot, so we can create symlink
       in /dev/disk/by-slot:
  # ls -d /dev/disk/by-slot/*
    /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0

5: add ability to power on/off device with power_status file in
   sysfs.

Due to the complexity of SES standard, the module is not to replace implement \
all features of sg_ses (sg3_utils).

Patch 5 and existing features for device element and array device elements control \
of HDDs. It is helpful to handle some HDD related fields in the kernel, as the \
kernel can generate mapping between a device to the SES device element (or array \
device element):

/sys/block/sdc/device/enclosure_deviceXXX/

With patch 5, we can easily power off a running HDD by

echo off > /sys/block/sdc/device/enclosure_deviceXXX/power_status

This is very useful for systems like Cold Storage, where HDDs are being powered \
on/off frequently


Dan Williams (4):
  ses: close potential registration race
  ses: generate KOBJ_CHANGE on enclosure attach
  ses: add enclosure logical id
  ses: add reliable slot attribute

Song Liu (1):
  ses: Add power_status to SES device slot

 drivers/misc/enclosure.c  | 106 +++++++++++++++++++++++++++++----
 drivers/scsi/ses.c        | 148 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/enclosure.h |  13 +++-
 3 files changed, 232 insertions(+), 35 deletions(-)

-- 
1.8.1

[PATCH 1/5] ses: close potential registration race

[Song Liu]

From: Dan Williams <dan.j.williams@intel.com>

The slot and address fields have a small window of instability when
userspace can read them before initialization. Separate
enclosure_component
allocation from registration.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Jens Axboe <axboe@fb.com>
Cc: Hannes Reinecke <hare@suse.de>
---
 drivers/misc/enclosure.c  | 35 +++++++++++++++++++++++++----------
 drivers/scsi/ses.c        | 21 ++++++++++++++-------
 include/linux/enclosure.h |  5 +++--
 3 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 180a544..d566f0f 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -273,23 +273,22 @@ enclosure_component_find_by_name(struct enclosure_device *edev,
 static const struct attribute_group *enclosure_component_groups[];
 
 /**
- * enclosure_component_register - add a particular component to an enclosure
+ * enclosure_component_alloc - prepare a new enclosure component
  * @edev:	the enclosure to add the component
  * @num:	the device number
  * @type:	the type of component being added
  * @name:	an optional name to appear in sysfs (leave NULL if none)
  *
- * Registers the component.  The name is optional for enclosures that
- * give their components a unique name.  If not, leave the field NULL
- * and a name will be assigned.
+ * The name is optional for enclosures that give their components a unique
+ * name.  If not, leave the field NULL and a name will be assigned.
  *
  * Returns a pointer to the enclosure component or an error.
  */
 struct enclosure_component *
-enclosure_component_register(struct enclosure_device *edev,
-			     unsigned int number,
-			     enum enclosure_component_type type,
-			     const char *name)
+enclosure_component_alloc(struct enclosure_device *edev,
+			  unsigned int number,
+			  enum enclosure_component_type type,
+			  const char *name)
 {
 	struct enclosure_component *ecomp;
 	struct device *cdev;
@@ -327,14 +326,30 @@ enclosure_component_register(struct enclosure_device *edev,
 	cdev->release = enclosure_component_release;
 	cdev->groups = enclosure_component_groups;
 
+	return ecomp;
+}
+EXPORT_SYMBOL_GPL(enclosure_component_alloc);
+
+/**
+ * enclosure_component_register - publishes an initialized enclosure component
+ * @ecomp:	component to add
+ *
+ * Returns 0 on successful registration, releases the component otherwise
+ */
+int enclosure_component_register(struct enclosure_component *ecomp)
+{
+	struct device *cdev;
+	int err;
+
+	cdev = &ecomp->cdev;
 	err = device_register(cdev);
 	if (err) {
 		ecomp->number = -1;
 		put_device(cdev);
-		return ERR_PTR(err);
+		return err;
 	}
 
-	return ecomp;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(enclosure_component_register);
 
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index b7e79e7..7dd9cf5 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -423,16 +423,23 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 			    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
 
 				if (create)
-					ecomp =	enclosure_component_register(edev,
-									     components++,
-									     type_ptr[0],
-									     name);
+					ecomp =	enclosure_component_alloc(
+						edev,
+						components++,
+						type_ptr[0],
+						name);
 				else
 					ecomp = &edev->component[components++];
 
-				if (!IS_ERR(ecomp) && addl_desc_ptr)
-					ses_process_descriptor(ecomp,
-							       addl_desc_ptr);
+				if (!IS_ERR(ecomp)) {
+					if (addl_desc_ptr)
+						ses_process_descriptor(
+							ecomp,
+							addl_desc_ptr);
+					if (create)
+						enclosure_component_register(
+							ecomp);
+				}
 			}
 			if (desc_ptr)
 				desc_ptr += len;
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 9a33c5f..a835d33 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -120,8 +120,9 @@ enclosure_register(struct device *, const char *, int,
 		   struct enclosure_component_callbacks *);
 void enclosure_unregister(struct enclosure_device *);
 struct enclosure_component *
-enclosure_component_register(struct enclosure_device *, unsigned int,
-				 enum enclosure_component_type, const char *);
+enclosure_component_alloc(struct enclosure_device *, unsigned int,
+			  enum enclosure_component_type, const char *);
+int enclosure_component_register(struct enclosure_component *);
 int enclosure_add_device(struct enclosure_device *enclosure, int component,
 			 struct device *dev);
 int enclosure_remove_device(struct enclosure_device *, struct device *);
-- 
1.8.1

Re: [PATCH 1/5] ses: close potential registration race

[Hannes Reinecke]

On 12/30/2014 11:46 PM, Song Liu wrote:
> From: Dan Williams <dan.j.williams@intel.com>
> 
> The slot and address fields have a small window of instability when
> userspace can read them before initialization. Separate
> enclosure_component
> allocation from registration.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> Reviewed-by: Jens Axboe <axboe@fb.com>
> Cc: Hannes Reinecke <hare@suse.de>

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] ses: generate KOBJ_CHANGE on enclosure attach

[Song Liu]

From: Dan Williams <dan.j.williams@intel.com>

In support of a /dev/disk/by-slot populated with data from the enclosure
and ses modules udev needs notification when the new interface
files/links are available.  Otherwise, any udev rules specified for the
disk cannot assume that the enclosure topology has settled.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Jens Axboe <axboe@fb.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/ses.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 7dd9cf5..6662b0c 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -349,7 +349,8 @@ static int ses_enclosure_find_by_addr(struct enclosure_device *edev,
 		if (scomp->addr != efd->addr)
 			continue;
 
-		enclosure_add_device(edev, i, efd->dev);
+		if (enclosure_add_device(edev, i, efd->dev) == 0)
+			kobject_uevent(&efd->dev->kobj, KOBJ_CHANGE);
 		return 1;
 	}
 	return 0;
-- 
1.8.1

[PATCH 3/5] ses: add enclosure logical id

[Song Liu]

From: Dan Williams <dan.j.williams@intel.com>

Export the NAA logical id for the enclosure.  This is optionally
available from the sas_transport_class, but it is really a property of
the enclosure.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Jens Axboe <axboe@fb.com>
Cc: Hannes Reinecke <hare@suse.de>
---
 drivers/misc/enclosure.c  | 13 +++++++++++++
 drivers/scsi/ses.c        |  9 +++++++++
 include/linux/enclosure.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index d566f0f..ab4de85 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -432,8 +432,21 @@ static ssize_t components_show(struct device *cdev,
 }
 static DEVICE_ATTR_RO(components);
 
+static ssize_t id_show(struct device *cdev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev);
+
+	if (edev->cb->show_id)
+		return edev->cb->show_id(edev, buf);
+	return -EINVAL;
+}
+static DEVICE_ATTR_RO(id);
+
 static struct attribute *enclosure_class_attrs[] = {
 	&dev_attr_components.attr,
+	&dev_attr_id.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(enclosure_class);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 6662b0c..1041556 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -258,6 +258,14 @@ static int ses_set_active(struct enclosure_device *edev,
 	return ses_set_page2_descriptor(edev, ecomp, desc);
 }
 
+static int ses_show_id(struct enclosure_device *edev, char *buf)
+{
+	struct ses_device *ses_dev = edev->scratch;
+	unsigned long long id = get_unaligned_be64(ses_dev->page1+8+4);
+
+	return sprintf(buf, "%#llx\n", id);
+}
+
 static struct enclosure_component_callbacks ses_enclosure_callbacks = {
 	.get_fault		= ses_get_fault,
 	.set_fault		= ses_set_fault,
@@ -265,6 +273,7 @@ static struct enclosure_component_callbacks ses_enclosure_callbacks = {
 	.get_locate		= ses_get_locate,
 	.set_locate		= ses_set_locate,
 	.set_active		= ses_set_active,
+	.show_id		= ses_show_id,
 };
 
 struct ses_host_edev {
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index a835d33..807622b 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -79,6 +79,7 @@ struct enclosure_component_callbacks {
 	int (*set_locate)(struct enclosure_device *,
 			  struct enclosure_component *,
 			  enum enclosure_component_setting);
+	int (*show_id)(struct enclosure_device *, char *buf);
 };
 
 
-- 
1.8.1

Re: [PATCH 3/5] ses: add enclosure logical id

[Hannes Reinecke]

On 12/30/2014 11:46 PM, Song Liu wrote:
> From: Dan Williams <dan.j.williams@intel.com>
> 
> Export the NAA logical id for the enclosure.  This is optionally
> available from the sas_transport_class, but it is really a property of
> the enclosure.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> Reviewed-by: Jens Axboe <axboe@fb.com>
> Cc: Hannes Reinecke <hare@suse.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] ses: add reliable slot attribute

[Song Liu]

From: Dan Williams <dan.j.williams@intel.com>

The name provided by firmware is in a vendor specific format, publish
the slot number to have a reliable mechanism for identifying slots
across firmware implementations.  If the enclosure does not provide a
slot number fallback to the component number which is guaranteed unique,
and usually mirrors the slot number.

Cleaned up the unused ses_component.desc in the process.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Jens Axboe <axboe@fb.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/misc/enclosure.c  | 20 +++++++++++++++++++-
 drivers/scsi/ses.c        | 17 ++++++++++++-----
 include/linux/enclosure.h |  1 +
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index ab4de85..b7995ed 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -145,8 +145,10 @@ enclosure_register(struct device *dev, const char *name, int components,
 	if (err)
 		goto err;
 
-	for (i = 0; i < components; i++)
+	for (i = 0; i < components; i++) {
 		edev->component[i].number = -1;
+		edev->component[i].slot = -1;
+	}
 
 	mutex_lock(&container_list_lock);
 	list_add_tail(&edev->node, &container_list);
@@ -589,6 +591,20 @@ static ssize_t get_component_type(struct device *cdev,
 	return snprintf(buf, 40, "%s\n", enclosure_type[ecomp->type]);
 }
 
+static ssize_t get_component_slot(struct device *cdev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+	int slot;
+
+	/* if the enclosure does not override then use 'number' as a stand-in */
+	if (ecomp->slot >= 0)
+		slot = ecomp->slot;
+	else
+		slot = ecomp->number;
+
+	return snprintf(buf, 40, "%d\n", slot);
+}
 
 static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
 		    set_component_fault);
@@ -599,6 +615,7 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, get_component_active,
 static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
 		   set_component_locate);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
+static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
 static struct attribute *enclosure_component_attrs[] = {
 	&dev_attr_fault.attr,
@@ -606,6 +623,7 @@ static struct attribute *enclosure_component_attrs[] = {
 	&dev_attr_active.attr,
 	&dev_attr_locate.attr,
 	&dev_attr_type.attr,
+	&dev_attr_slot.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(enclosure_component);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 1041556..433de8e 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -47,7 +47,6 @@ struct ses_device {
 
 struct ses_component {
 	u64 addr;
-	unsigned char *desc;
 };
 
 static int ses_probe(struct device *dev)
@@ -307,19 +306,26 @@ static void ses_process_descriptor(struct enclosure_component *ecomp,
 	int invalid = desc[0] & 0x80;
 	enum scsi_protocol proto = desc[0] & 0x0f;
 	u64 addr = 0;
+	int slot = -1;
 	struct ses_component *scomp = ecomp->scratch;
 	unsigned char *d;
 
-	scomp->desc = desc;
-
 	if (invalid)
 		return;
 
 	switch (proto) {
+	case SCSI_PROTOCOL_FCP:
+		if (eip) {
+			d = desc + 4;
+			slot = d[3];
+		}
+		break;
 	case SCSI_PROTOCOL_SAS:
-		if (eip)
+		if (eip) {
+			d = desc + 4;
+			slot = d[3];
 			d = desc + 8;
-		else
+		} else
 			d = desc + 4;
 		/* only take the phy0 addr */
 		addr = (u64)d[12] << 56 |
@@ -335,6 +341,7 @@ static void ses_process_descriptor(struct enclosure_component *ecomp,
 		/* FIXME: Need to add more protocols than just SAS */
 		break;
 	}
+	ecomp->slot = slot;
 	scomp->addr = addr;
 }
 
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 807622b..0f826c1 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -92,6 +92,7 @@ struct enclosure_component {
 	int fault;
 	int active;
 	int locate;
+	int slot;
 	enum enclosure_status status;
 };
 
-- 
1.8.1

[PATCH 5/5] ses: Add power_status to SES device slot

[Song Liu]

Add power_status to SES device slot, so we can power on/off the
HDDs behind the enclosure.

Check firmware status in ses_set_* before sending control pages to
firmware.

Signed-off-by: Song Liu <songliubraving@fb.com>
Acked-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jens Axboe <axboe@fb.com>
Cc: Hannes Reinecke <hare@suse.de>
---
 drivers/misc/enclosure.c  | 38 ++++++++++++++++++
 drivers/scsi/ses.c        | 98 ++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/enclosure.h |  6 +++
 3 files changed, 133 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index b7995ed..3289d4d 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -148,6 +148,7 @@ enclosure_register(struct device *dev, const char *name, int components,
 	for (i = 0; i < components; i++) {
 		edev->component[i].number = -1;
 		edev->component[i].slot = -1;
+		edev->component[i].power_status = 1;
 	}
 
 	mutex_lock(&container_list_lock);
@@ -583,6 +584,40 @@ static ssize_t set_component_locate(struct device *cdev,
 	return count;
 }
 
+static ssize_t get_component_power_status(struct device *cdev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+	if (edev->cb->get_power_status)
+		edev->cb->get_power_status(edev, ecomp);
+	return snprintf(buf, 40, "%s\n", ecomp->power_status ? "on" : "off");
+}
+
+static ssize_t set_component_power_status(struct device *cdev,
+					  struct device_attribute *attr,
+					  const char *buf, size_t count)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+	int val;
+
+	if (strncmp(buf, "on", 2) == 0 &&
+	    (buf[2] == '\n' || buf[2] == '\0'))
+		val = 1;
+	else if (strncmp(buf, "off", 3) == 0 &&
+	    (buf[3] == '\n' || buf[3] == '\0'))
+		val = 0;
+	else
+		return -EINVAL;
+
+	if (edev->cb->set_power_status)
+		edev->cb->set_power_status(edev, ecomp, val);
+	return count;
+}
+
 static ssize_t get_component_type(struct device *cdev,
 				  struct device_attribute *attr, char *buf)
 {
@@ -614,6 +649,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, get_component_active,
 		   set_component_active);
 static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
 		   set_component_locate);
+static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR, get_component_power_status,
+		   set_component_power_status);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
 static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
@@ -622,6 +659,7 @@ static struct attribute *enclosure_component_attrs[] = {
 	&dev_attr_status.attr,
 	&dev_attr_active.attr,
 	&dev_attr_locate.attr,
+	&dev_attr_power_status.attr,
 	&dev_attr_type.attr,
 	&dev_attr_slot.attr,
 	NULL
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 433de8e..dcb0d76 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -67,6 +67,20 @@ static int ses_probe(struct device *dev)
 #define SES_TIMEOUT (30 * HZ)
 #define SES_RETRIES 3
 
+static void init_device_slot_control(unsigned char *dest_desc,
+				     struct enclosure_component *ecomp,
+				     unsigned char *status)
+{
+	memcpy(dest_desc, status, 4);
+	dest_desc[0] = 0;
+	/* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+	if (ecomp->type == ENCLOSURE_COMPONENT_DEVICE)
+		dest_desc[1] = 0;
+	dest_desc[2] &= 0xde;
+	dest_desc[3] &= 0x3c;
+}
+
+
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 			 void *buf, int bufflen)
 {
@@ -178,14 +192,22 @@ static int ses_set_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			 enum enclosure_component_setting val)
 {
-	unsigned char desc[4] = {0 };
+	unsigned char desc[4];
+	unsigned char *desc_ptr;
+
+	desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+	if (!desc_ptr)
+		return -EIO;
+
+	init_device_slot_control(desc, ecomp, desc_ptr);
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
-		/* zero is disabled */
+		desc[3] &= 0xdf;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[3] = 0x20;
+		desc[3] |= 0x20;
 		break;
 	default:
 		/* SES doesn't do the SGPIO blink settings */
@@ -219,14 +241,22 @@ static int ses_set_locate(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			  enum enclosure_component_setting val)
 {
-	unsigned char desc[4] = {0 };
+	unsigned char desc[4];
+	unsigned char *desc_ptr;
+
+	desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+	if (!desc_ptr)
+		return -EIO;
+
+	init_device_slot_control(desc, ecomp, desc_ptr);
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
-		/* zero is disabled */
+		desc[2] &= 0xfd;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[2] = 0x02;
+		desc[2] |= 0x02;
 		break;
 	default:
 		/* SES doesn't do the SGPIO blink settings */
@@ -239,15 +269,23 @@ static int ses_set_active(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			  enum enclosure_component_setting val)
 {
-	unsigned char desc[4] = {0 };
+	unsigned char desc[4];
+	unsigned char *desc_ptr;
+
+	desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+	if (!desc_ptr)
+		return -EIO;
+
+	init_device_slot_control(desc, ecomp, desc_ptr);
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
-		/* zero is disabled */
+		desc[2] &= 0x7f;
 		ecomp->active = 0;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[2] = 0x80;
+		desc[2] |= 0x80;
 		ecomp->active = 1;
 		break;
 	default:
@@ -265,12 +303,53 @@ static int ses_show_id(struct enclosure_device *edev, char *buf)
 	return sprintf(buf, "%#llx\n", id);
 }
 
+static void ses_get_power_status(struct enclosure_device *edev,
+				 struct enclosure_component *ecomp)
+{
+	unsigned char *desc;
+
+	desc = ses_get_page2_descriptor(edev, ecomp);
+	if (desc)
+		ecomp->power_status = (desc[3] & 0x10) ? 0 : 1;
+}
+
+static int ses_set_power_status(struct enclosure_device *edev,
+				struct enclosure_component *ecomp,
+				int val)
+{
+	unsigned char desc[4];
+	unsigned char *desc_ptr;
+
+	desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+	if (!desc_ptr)
+		return -EIO;
+
+	init_device_slot_control(desc, ecomp, desc_ptr);
+
+	switch (val) {
+	/* power = 1 is device_off = 0 and vice versa */
+	case 0:
+		desc[3] |= 0x10;
+		break;
+	case 1:
+		desc[3] &= 0xef;
+		break;
+	default:
+		return -EINVAL;
+	}
+	ecomp->power_status = val;
+	return ses_set_page2_descriptor(edev, ecomp, desc);
+}
+
 static struct enclosure_component_callbacks ses_enclosure_callbacks = {
 	.get_fault		= ses_get_fault,
 	.set_fault		= ses_set_fault,
 	.get_status		= ses_get_status,
 	.get_locate		= ses_get_locate,
 	.set_locate		= ses_set_locate,
+	.get_power_status	= ses_get_power_status,
+	.set_power_status	= ses_set_power_status,
 	.set_active		= ses_set_active,
 	.show_id		= ses_show_id,
 };
@@ -449,6 +528,7 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 					ecomp = &edev->component[components++];
 
 				if (!IS_ERR(ecomp)) {
+					ses_get_power_status(edev, ecomp);
 					if (addl_desc_ptr)
 						ses_process_descriptor(
 							ecomp,
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 0f826c1..7be22da 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -79,6 +79,11 @@ struct enclosure_component_callbacks {
 	int (*set_locate)(struct enclosure_device *,
 			  struct enclosure_component *,
 			  enum enclosure_component_setting);
+	void (*get_power_status)(struct enclosure_device *,
+				 struct enclosure_component *);
+	int (*set_power_status)(struct enclosure_device *,
+				struct enclosure_component *,
+				int);
 	int (*show_id)(struct enclosure_device *, char *buf);
 };
 
@@ -94,6 +99,7 @@ struct enclosure_component {
 	int locate;
 	int slot;
 	enum enclosure_status status;
+	int power_status;
 };
 
 struct enclosure_device {
-- 
1.8.1

Re: [PATCH 5/5] ses: Add power_status to SES device slot

[Hannes Reinecke]

On 12/30/2014 11:46 PM, Song Liu wrote:
> Add power_status to SES device slot, so we can power on/off the
> HDDs behind the enclosure.
> 
> Check firmware status in ses_set_* before sending control pages to
> firmware.
> 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Jens Axboe <axboe@fb.com>
> Cc: Hannes Reinecke <hare@suse.de>

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Feature enhancements for ses module

[Christoph Hellwig]

Thanks, applied to scsi-for-3.20.

[PATCH 0/5] Feature enhancements for ses module

[Song Liu]

From: Song Liu [mailto:songliubraving@fb.com] 
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Subject: [PATCH 0/5] Feature enhancements for ses module

These patches include a few enhancements to ses module:

1: close potential race condition by at enclosure race condition

2,3,4: add enclosure id and device slot, so we can create symlink
       in /dev/disk/by-slot:
  # ls -d /dev/disk/by-slot/*
    /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0

5: add ability to power on/off device with power_status file in
   sysfs.

Dan Williams (4):
  SES: close potential registration race
  SES: generate KOBJ_CHANGE on enclosure attach
  SES: add enclosure logical id
  SES: add reliable slot attribute

Song Liu (1):
  SES: Add power_status to SES enclosure component

 drivers/misc/enclosure.c  |  98 +++++++++++++++++++++++++++----
 drivers/scsi/ses.c        | 145 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/enclosure.h |  13 ++++-
 3 files changed, 220 insertions(+), 36 deletions(-)

-- 
1.8.1

Re: [PATCH 0/5] Feature enhancements for ses module

[Jens Axboe]

On 08/25/2014 11:34 AM, Song Liu wrote:
> From: Song Liu [mailto:songliubraving@fb.com] 
> Sent: Monday, August 25, 2014 10:26 AM
> To: Song Liu
> Subject: [PATCH 0/5] Feature enhancements for ses module
> 
> These patches include a few enhancements to ses module:
> 
> 1: close potential race condition by at enclosure race condition
> 
> 2,3,4: add enclosure id and device slot, so we can create symlink
>        in /dev/disk/by-slot:
>   # ls -d /dev/disk/by-slot/*
>     /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0
> 
> 5: add ability to power on/off device with power_status file in
>    sysfs.
> 
> Dan Williams (4):
>   SES: close potential registration race
>   SES: generate KOBJ_CHANGE on enclosure attach
>   SES: add enclosure logical id
>   SES: add reliable slot attribute
> 
> Song Liu (1):
>   SES: Add power_status to SES enclosure component

Guys, where are we with these? Seemed like they got reviewed (and
updates/fixes posted), then nothing else happened. Would be nice to get
these upstream, so we don't have to carry them at FB.

-- 
Jens Axboe

Re: [PATCH 0/5] Feature enhancements for ses module

[Douglas Gilbert]

On 14-10-22 09:12 PM, Jens Axboe wrote:
> On 08/25/2014 11:34 AM, Song Liu wrote:
>> From: Song Liu [mailto:songliubraving@fb.com]
>> Sent: Monday, August 25, 2014 10:26 AM
>> To: Song Liu
>> Subject: [PATCH 0/5] Feature enhancements for ses module
>>
>> These patches include a few enhancements to ses module:
>>
>> 1: close potential race condition by at enclosure race condition
>>
>> 2,3,4: add enclosure id and device slot, so we can create symlink
>>         in /dev/disk/by-slot:
>>    # ls -d /dev/disk/by-slot/*
>>      /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0
>>
>> 5: add ability to power on/off device with power_status file in
>>     sysfs.

Had a rude awakening with sg_ses recently when setting a field
in the enclosure control dpage. That is what is being done in
point 5: above.

The time honoured technique is to read the corresponding enclosure
status dpage, find the correct element, twiddle the field of
interest, set the SELECT bit and write it back. The idea is maintain
any other field settings in that element. And this is what the ses
module does.

There is at least one SES device out there that rejects the write
if there are bits set in RESERVED locations. According to SPC-4 a
device may do that. Look at the status (read) and control (write)
variants of each element type: many times the control variant has
less fields.

To fix that the read-modify-write cycle needs to be changed to a
read-mask-modify-write cycle where the "mask" is the allowable
write mask (there would be one for each element type).

This is ugly and may create problems in the future if and when
T10 adds a new field in an element. About that time T10 should
think about refining the meaning of RESERVED in SES to outlaw
SES devices creating this particular time waster.

Doug Gilbert

>> Dan Williams (4):
>>    SES: close potential registration race
>>    SES: generate KOBJ_CHANGE on enclosure attach
>>    SES: add enclosure logical id
>>    SES: add reliable slot attribute
>>
>> Song Liu (1):
>>    SES: Add power_status to SES enclosure component
>
> Guys, where are we with these? Seemed like they got reviewed (and
> updates/fixes posted), then nothing else happened. Would be nice to get
> these upstream, so we don't have to carry them at FB.
>

RE: [PATCH 0/5] Feature enhancements for ses module

[Song Liu]

Hi Doug, 

I am not sure whether I fully understand the scenario. Actually patch 5 tries to clear all reserved bits:

+ dest_desc[0] = 0;
+ /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+ if (ecomp->type == ENCLOSURE_COMPONENT_DEVICE)
+   dest_desc[1] = 0;
+ dest_desc[2] &= 0xde;
+ dest_desc[3] &= 0x3c;

Would this work for device that rejects request with 1 in RESERVED areas? 

Thanks,
Song

> -----Original Message-----
> From: Douglas Gilbert [mailto:dgilbert@interlog.com]
> Sent: Wednesday, October 22, 2014 3:29 PM
> To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org
> Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
> Subject: Re: [PATCH 0/5] Feature enhancements for ses module
> 
> On 14-10-22 09:12 PM, Jens Axboe wrote:
> > On 08/25/2014 11:34 AM, Song Liu wrote:
> >> From: Song Liu [mailto:songliubraving@fb.com]
> >> Sent: Monday, August 25, 2014 10:26 AM
> >> To: Song Liu
> >> Subject: [PATCH 0/5] Feature enhancements for ses module
> >>
> >> These patches include a few enhancements to ses module:
> >>
> >> 1: close potential race condition by at enclosure race condition
> >>
> >> 2,3,4: add enclosure id and device slot, so we can create symlink
> >>         in /dev/disk/by-slot:
> >>    # ls -d /dev/disk/by-slot/*
> >>      /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0
> >>
> >> 5: add ability to power on/off device with power_status file in
> >>     sysfs.
> 
> Had a rude awakening with sg_ses recently when setting a field in the
> enclosure control dpage. That is what is being done in point 5: above.
> 
> The time honoured technique is to read the corresponding enclosure status
> dpage, find the correct element, twiddle the field of interest, set the SELECT bit
> and write it back. The idea is maintain any other field settings in that element.
> And this is what the ses module does.
> 
> There is at least one SES device out there that rejects the write if there are bits
> set in RESERVED locations. According to SPC-4 a device may do that. Look at
> the status (read) and control (write) variants of each element type: many times
> the control variant has less fields.
> 
> To fix that the read-modify-write cycle needs to be changed to a read-mask-
> modify-write cycle where the "mask" is the allowable write mask (there would
> be one for each element type).
> 
> This is ugly and may create problems in the future if and when
> T10 adds a new field in an element. About that time T10 should think about
> refining the meaning of RESERVED in SES to outlaw SES devices creating this
> particular time waster.
> 
> Doug Gilbert
> 
> >> Dan Williams (4):
> >>    SES: close potential registration race
> >>    SES: generate KOBJ_CHANGE on enclosure attach
> >>    SES: add enclosure logical id
> >>    SES: add reliable slot attribute
> >>
> >> Song Liu (1):
> >>    SES: Add power_status to SES enclosure component
> >
> > Guys, where are we with these? Seemed like they got reviewed (and
> > updates/fixes posted), then nothing else happened. Would be nice to
> > get these upstream, so we don't have to carry them at FB.
> >

Re: [PATCH 0/5] Feature enhancements for ses module

[Douglas Gilbert]

On 14-10-23 01:01 AM, Song Liu wrote:
> Hi Doug,
>
> I am not sure whether I fully understand the scenario. Actually patch 5 tries to clear all reserved bits:
>
> + dest_desc[0] = 0;
> + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
> + if (ecomp->type == ENCLOSURE_COMPONENT_DEVICE)
> +   dest_desc[1] = 0;
> + dest_desc[2] &= 0xde;
> + dest_desc[3] &= 0x3c;
>
> Would this work for device that rejects request with 1 in RESERVED areas?


That is a pretty asymmetric element type, assuming we are talking
about the "enclosure control" and "enclosure status" elements
(i.e. etc=0xe). My guess would be:

     dest_desc[0] = 0x80 | (src_desc[0] & 40);
     dest_desc[1] = 0x80 & src_desc[1];
     dest_desc[2] = (pc_req << 6) | pc_delay;
     dest_desc[3] = 0xff & src_desc[3];
or if you have a new power_off_duration:
     dest_desc[3] = (power_off_duration << 2) | (src_desc[3] & 0x3);

In byte 0 the top bit (SELECT) must be set or the enclosure
will ignore any other settings in that element. If the PRDFAIL
bit is already set, then that setting will be maintained.
SES-3 has a note about clearing DISABLE and SWAP.

In byte 1 is if the identifier (LED ?) is active (saying blinking)
prior to this power cycle request, then it will stay blinking
until the power drops. If the enclosure was really clever it might
keep blinking after the power cycle :-)

Also notice that the requested power cycle can be cancelled
up to the "time until power cycle" drops to zero.

>> -----Original Message-----
>> From: Douglas Gilbert [mailto:dgilbert@interlog.com]
>> Sent: Wednesday, October 22, 2014 3:29 PM
>> To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org
>> Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
>> Subject: Re: [PATCH 0/5] Feature enhancements for ses module
>>
>> On 14-10-22 09:12 PM, Jens Axboe wrote:
>>> On 08/25/2014 11:34 AM, Song Liu wrote:
>>>> From: Song Liu [mailto:songliubraving@fb.com]
>>>> Sent: Monday, August 25, 2014 10:26 AM
>>>> To: Song Liu
>>>> Subject: [PATCH 0/5] Feature enhancements for ses module
>>>>
>>>> These patches include a few enhancements to ses module:
>>>>
>>>> 1: close potential race condition by at enclosure race condition
>>>>
>>>> 2,3,4: add enclosure id and device slot, so we can create symlink
>>>>          in /dev/disk/by-slot:
>>>>     # ls -d /dev/disk/by-slot/*
>>>>       /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0
>>>>
>>>> 5: add ability to power on/off device with power_status file in
>>>>      sysfs.
>>
>> Had a rude awakening with sg_ses recently when setting a field in the
>> enclosure control dpage. That is what is being done in point 5: above.
>>
>> The time honoured technique is to read the corresponding enclosure status
>> dpage, find the correct element, twiddle the field of interest, set the SELECT bit
>> and write it back. The idea is maintain any other field settings in that element.
>> And this is what the ses module does.
>>
>> There is at least one SES device out there that rejects the write if there are bits
>> set in RESERVED locations. According to SPC-4 a device may do that. Look at
>> the status (read) and control (write) variants of each element type: many times
>> the control variant has less fields.
>>
>> To fix that the read-modify-write cycle needs to be changed to a read-mask-
>> modify-write cycle where the "mask" is the allowable write mask (there would
>> be one for each element type).
>>
>> This is ugly and may create problems in the future if and when
>> T10 adds a new field in an element. About that time T10 should think about
>> refining the meaning of RESERVED in SES to outlaw SES devices creating this
>> particular time waster.
>>
>> Doug Gilbert
>>
>>>> Dan Williams (4):
>>>>     SES: close potential registration race
>>>>     SES: generate KOBJ_CHANGE on enclosure attach
>>>>     SES: add enclosure logical id
>>>>     SES: add reliable slot attribute
>>>>
>>>> Song Liu (1):
>>>>     SES: Add power_status to SES enclosure component
>>>
>>> Guys, where are we with these? Seemed like they got reviewed (and
>>> updates/fixes posted), then nothing else happened. Would be nice to
>>> get these upstream, so we don't have to carry them at FB.
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

RE: [PATCH 0/5] Feature enhancements for ses module

[Song Liu]

Hi Doug, 

The power on/off field together with "fault", "locate", and "active" are for HDD (i.e. DeviceSlot and ArrayDeviceSlot). They are not for enclosure or other elements. So we are not dealing with power off duration, etc. here. 

Thanks,
Song

> -----Original Message-----
> From: Douglas Gilbert [mailto:dgilbert@interlog.com]
> Sent: Wednesday, October 22, 2014 6:17 PM
> To: Song Liu; Jens Axboe; linux-scsi@vger.kernel.org
> Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
> Subject: Re: [PATCH 0/5] Feature enhancements for ses module
> 
> On 14-10-23 01:01 AM, Song Liu wrote:
> > Hi Doug,
> >
> > I am not sure whether I fully understand the scenario. Actually patch 5 tries to
> clear all reserved bits:
> >
> > + dest_desc[0] = 0;
> > + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */ if
> > + (ecomp->type == ENCLOSURE_COMPONENT_DEVICE)
> > +   dest_desc[1] = 0;
> > + dest_desc[2] &= 0xde;
> > + dest_desc[3] &= 0x3c;
> >
> > Would this work for device that rejects request with 1 in RESERVED areas?
> 
> 
> That is a pretty asymmetric element type, assuming we are talking about the
> "enclosure control" and "enclosure status" elements (i.e. etc=0xe). My guess
> would be:
> 
>      dest_desc[0] = 0x80 | (src_desc[0] & 40);
>      dest_desc[1] = 0x80 & src_desc[1];
>      dest_desc[2] = (pc_req << 6) | pc_delay;
>      dest_desc[3] = 0xff & src_desc[3];
> or if you have a new power_off_duration:
>      dest_desc[3] = (power_off_duration << 2) | (src_desc[3] & 0x3);
> 
> In byte 0 the top bit (SELECT) must be set or the enclosure will ignore any other
> settings in that element. If the PRDFAIL bit is already set, then that setting will
> be maintained.
> SES-3 has a note about clearing DISABLE and SWAP.
> 
> In byte 1 is if the identifier (LED ?) is active (saying blinking) prior to this power
> cycle request, then it will stay blinking until the power drops. If the enclosure
> was really clever it might keep blinking after the power cycle :-)
> 
> Also notice that the requested power cycle can be cancelled up to the "time
> until power cycle" drops to zero.
> 
> >> -----Original Message-----
> >> From: Douglas Gilbert [mailto:dgilbert@interlog.com]
> >> Sent: Wednesday, October 22, 2014 3:29 PM
> >> To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org
> >> Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
> >> Subject: Re: [PATCH 0/5] Feature enhancements for ses module
> >>
> >> On 14-10-22 09:12 PM, Jens Axboe wrote:
> >>> On 08/25/2014 11:34 AM, Song Liu wrote:
> >>>> From: Song Liu [mailto:songliubraving@fb.com]
> >>>> Sent: Monday, August 25, 2014 10:26 AM
> >>>> To: Song Liu
> >>>> Subject: [PATCH 0/5] Feature enhancements for ses module
> >>>>
> >>>> These patches include a few enhancements to ses module:
> >>>>
> >>>> 1: close potential race condition by at enclosure race condition
> >>>>
> >>>> 2,3,4: add enclosure id and device slot, so we can create symlink
> >>>>          in /dev/disk/by-slot:
> >>>>     # ls -d /dev/disk/by-slot/*
> >>>>       /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0
> >>>>
> >>>> 5: add ability to power on/off device with power_status file in
> >>>>      sysfs.
> >>
> >> Had a rude awakening with sg_ses recently when setting a field in the
> >> enclosure control dpage. That is what is being done in point 5: above.
> >>
> >> The time honoured technique is to read the corresponding enclosure
> >> status dpage, find the correct element, twiddle the field of
> >> interest, set the SELECT bit and write it back. The idea is maintain any other
> field settings in that element.
> >> And this is what the ses module does.
> >>
> >> There is at least one SES device out there that rejects the write if
> >> there are bits set in RESERVED locations. According to SPC-4 a device
> >> may do that. Look at the status (read) and control (write) variants
> >> of each element type: many times the control variant has less fields.
> >>
> >> To fix that the read-modify-write cycle needs to be changed to a
> >> read-mask- modify-write cycle where the "mask" is the allowable write
> >> mask (there would be one for each element type).
> >>
> >> This is ugly and may create problems in the future if and when
> >> T10 adds a new field in an element. About that time T10 should think
> >> about refining the meaning of RESERVED in SES to outlaw SES devices
> >> creating this particular time waster.
> >>
> >> Doug Gilbert
> >>
> >>>> Dan Williams (4):
> >>>>     SES: close potential registration race
> >>>>     SES: generate KOBJ_CHANGE on enclosure attach
> >>>>     SES: add enclosure logical id
> >>>>     SES: add reliable slot attribute
> >>>>
> >>>> Song Liu (1):
> >>>>     SES: Add power_status to SES enclosure component
> >>>
> >>> Guys, where are we with these? Seemed like they got reviewed (and
> >>> updates/fixes posted), then nothing else happened. Would be nice to
> >>> get these upstream, so we don't have to carry them at FB.
> >>>
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at
> > https://urldefense.proofpoint.com/v1/url?u=http://vger.kernel.org/majo
> > rdomo-
> info.html&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=J3jGe56dIPfS5TN6DM
> >
> 82UYbbeR1j2viaiSJI40tv6lE%3D%0A&m=CIfJIr9ka37ZOOTBDMcaf3cmkg1%2BV
> Rv4oz
> >
> 0zbf%2Fb24o%3D%0A&s=792fd953749b38a8e8181e2f36a2b9102ae9a3096f85f
> 95690
> > 4aa8201dde45d6
> >

RE: [PATCH 0/5] Feature enhancements for ses module

[Song Liu]

Hi Doug, 

I agree what it is difficult to handle all elements, and thus using sg_ses probably makes more sense. However, it helps to handle some HDD related fields in the kernel, as the kernel can generate mapping between a device to the SES device element (or array device element): 

/sys/block/sdc/device/enclosure_deviceXXX/

With patch 5, we can easily power off a running HDD by 

echo off > /sys/block/sdc/device/enclosure_deviceXXX/power_status

This is very useful for systems like Cold Storage, where HDDs are being powered on/off frequently. 


In current code, ses_set_page2_descriptor already clear reserved field for all elements, and only send non-zero for the device element or array device. Patch 5 also handles reserved field of these two elements in init_device_slot_control:

dest_desc[2] &= 0xde;
dest_desc[3] &= 0x3c;

So I think we can control fault, locate, active, and power_status of each HDD without issue. 

Please let me know your suggestion on this. 

Thanks,
Song

> -----Original Message-----
> From: Song Liu
> Sent: Wednesday, October 22, 2014 8:42 PM
> To: 'dgilbert@interlog.com'; Jens Axboe; linux-scsi@vger.kernel.org
> Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
> Subject: RE: [PATCH 0/5] Feature enhancements for ses module
> 
> Hi Doug,
> 
> The power on/off field together with "fault", "locate", and "active" are for HDD
> (i.e. DeviceSlot and ArrayDeviceSlot). They are not for enclosure or other
> elements. So we are not dealing with power off duration, etc. here.
> 
> Thanks,
> Song
> 
> > -----Original Message-----
> > From: Douglas Gilbert [mailto:dgilbert@interlog.com]
> > Sent: Wednesday, October 22, 2014 6:17 PM
> > To: Song Liu; Jens Axboe; linux-scsi@vger.kernel.org
> > Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
> > Subject: Re: [PATCH 0/5] Feature enhancements for ses module
> >
> > On 14-10-23 01:01 AM, Song Liu wrote:
> > > Hi Doug,
> > >
> > > I am not sure whether I fully understand the scenario. Actually
> > > patch 5 tries to
> > clear all reserved bits:
> > >
> > > + dest_desc[0] = 0;
> > > + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */ if
> > > + (ecomp->type == ENCLOSURE_COMPONENT_DEVICE)
> > > +   dest_desc[1] = 0;
> > > + dest_desc[2] &= 0xde;
> > > + dest_desc[3] &= 0x3c;
> > >
> > > Would this work for device that rejects request with 1 in RESERVED areas?
> >
> >
> > That is a pretty asymmetric element type, assuming we are talking
> > about the "enclosure control" and "enclosure status" elements (i.e.
> > etc=0xe). My guess would be:
> >
> >      dest_desc[0] = 0x80 | (src_desc[0] & 40);
> >      dest_desc[1] = 0x80 & src_desc[1];
> >      dest_desc[2] = (pc_req << 6) | pc_delay;
> >      dest_desc[3] = 0xff & src_desc[3]; or if you have a new
> > power_off_duration:
> >      dest_desc[3] = (power_off_duration << 2) | (src_desc[3] & 0x3);
> >
> > In byte 0 the top bit (SELECT) must be set or the enclosure will
> > ignore any other settings in that element. If the PRDFAIL bit is
> > already set, then that setting will be maintained.
> > SES-3 has a note about clearing DISABLE and SWAP.
> >
> > In byte 1 is if the identifier (LED ?) is active (saying blinking)
> > prior to this power cycle request, then it will stay blinking until
> > the power drops. If the enclosure was really clever it might keep
> > blinking after the power cycle :-)
> >
> > Also notice that the requested power cycle can be cancelled up to the
> > "time until power cycle" drops to zero.
> >
> > >> -----Original Message-----
> > >> From: Douglas Gilbert [mailto:dgilbert@interlog.com]
> > >> Sent: Wednesday, October 22, 2014 3:29 PM
> > >> To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org
> > >> Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
> > >> Subject: Re: [PATCH 0/5] Feature enhancements for ses module
> > >>
> > >> On 14-10-22 09:12 PM, Jens Axboe wrote:
> > >>> On 08/25/2014 11:34 AM, Song Liu wrote:
> > >>>> From: Song Liu [mailto:songliubraving@fb.com]
> > >>>> Sent: Monday, August 25, 2014 10:26 AM
> > >>>> To: Song Liu
> > >>>> Subject: [PATCH 0/5] Feature enhancements for ses module
> > >>>>
> > >>>> These patches include a few enhancements to ses module:
> > >>>>
> > >>>> 1: close potential race condition by at enclosure race condition
> > >>>>
> > >>>> 2,3,4: add enclosure id and device slot, so we can create symlink
> > >>>>          in /dev/disk/by-slot:
> > >>>>     # ls -d /dev/disk/by-slot/*
> > >>>>       /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0
> > >>>>
> > >>>> 5: add ability to power on/off device with power_status file in
> > >>>>      sysfs.
> > >>
> > >> Had a rude awakening with sg_ses recently when setting a field in
> > >> the enclosure control dpage. That is what is being done in point 5: above.
> > >>
> > >> The time honoured technique is to read the corresponding enclosure
> > >> status dpage, find the correct element, twiddle the field of
> > >> interest, set the SELECT bit and write it back. The idea is
> > >> maintain any other
> > field settings in that element.
> > >> And this is what the ses module does.
> > >>
> > >> There is at least one SES device out there that rejects the write
> > >> if there are bits set in RESERVED locations. According to SPC-4 a
> > >> device may do that. Look at the status (read) and control (write)
> > >> variants of each element type: many times the control variant has less
> fields.
> > >>
> > >> To fix that the read-modify-write cycle needs to be changed to a
> > >> read-mask- modify-write cycle where the "mask" is the allowable
> > >> write mask (there would be one for each element type).
> > >>
> > >> This is ugly and may create problems in the future if and when
> > >> T10 adds a new field in an element. About that time T10 should
> > >> think about refining the meaning of RESERVED in SES to outlaw SES
> > >> devices creating this particular time waster.
> > >>
> > >> Doug Gilbert
> > >>
> > >>>> Dan Williams (4):
> > >>>>     SES: close potential registration race
> > >>>>     SES: generate KOBJ_CHANGE on enclosure attach
> > >>>>     SES: add enclosure logical id
> > >>>>     SES: add reliable slot attribute
> > >>>>
> > >>>> Song Liu (1):
> > >>>>     SES: Add power_status to SES enclosure component
> > >>>
> > >>> Guys, where are we with these? Seemed like they got reviewed (and
> > >>> updates/fixes posted), then nothing else happened. Would be nice
> > >>> to get these upstream, so we don't have to carry them at FB.
> > >>>
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> > > in the body of a message to majordomo@vger.kernel.org More majordomo
> > > info at
> > > https://urldefense.proofpoint.com/v1/url?u=http://vger.kernel.org/ma
> > > jo
> > > rdomo-
> >
> info.html&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=J3jGe56dIPfS5TN6DM
> > >
> >
> 82UYbbeR1j2viaiSJI40tv6lE%3D%0A&m=CIfJIr9ka37ZOOTBDMcaf3cmkg1%2BV
> > Rv4oz
> > >
> >
> 0zbf%2Fb24o%3D%0A&s=792fd953749b38a8e8181e2f36a2b9102ae9a3096f85f
> > 95690
> > > 4aa8201dde45d6
> > >

Re: [PATCH 0/5] Feature enhancements for ses module

[Christoph Hellwig]

On Wed, Oct 22, 2014 at 01:12:01PM -0600, Jens Axboe wrote:
> Guys, where are we with these? Seemed like they got reviewed (and
> updates/fixes posted), then nothing else happened. Would be nice to get
> these upstream, so we don't have to carry them at FB.

Nothign happened, I guess mostly like due to the odd way it was posted,
seemingly in reply to something else that wasn't sent in mutt.  I'll
way for Song and Doug to agree on the reserved field handling and will
apply it once all remaining issues there are sorted out.