Discussion:
[PATCH 0/5] Feature enhancements for ses module
(too old to reply)
Song Liu
2014-08-25 17:34:36 UTC
Permalink
From: Song Liu [mailto:***@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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-08-25 17:34:44 UTC
Permalink
From: Song Liu [mailto:***@fb.com]
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Hannes Reinecke
Subject: [PATCH 5/5] SES: Add power_status to SES enclosure component

Add power_status to SES enclosure component, 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 <***@fb.com>
Acked-by: Dan Williams <***@intel.com>
Reviewed-by: Jens Axboe <***@fb.com>
Cc: Hannes Reinecke <***@suse.de>
---
drivers/misc/enclosure.c | 29 ++++++++++++++
drivers/scsi/ses.c | 98 ++++++++++++++++++++++++++++++++++++++++++-----
include/linux/enclosure.h | 6 +++
3 files changed, 124 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index de335bc..0331dfe 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);
@@ -546,6 +547,31 @@ 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, "%d\n", ecomp->power_status); }
+
+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 = simple_strtoul(buf, NULL, 0);
+
+ 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) { @@ -577,6 +603,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);

@@ -585,6 +613,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 bafa301..ea6b262 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,
};
@@ -448,6 +527,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,
addl_desc_ptr);
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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-09-12 21:07:51 UTC
Permalink
New patch in next email...

Change to let power_status show "on" or "off"

Initialization of desc was removed because it will be initialized in in=
it_device_slot_control, so no need to initialize at define.

Thanks,
Song
-----Original Message-----
Sent: Thursday, September 4, 2014 12:59 AM
Cc: Dan Williams; Jens Axboe
Subject: Re: [PATCH 5/5] SES: Add power_status to SES enclosure compo=
nent
=20
Post by Song Liu
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Hannes Reinecke
Subject: [PATCH 5/5] SES: Add power_status to SES enclosure compone=
nt
Post by Song Liu
Add power_status to SES enclosure component, so we can power on/off
the HDDs behind the enclosure.
Post by Song Liu
Check firmware status in ses_set_* before sending control pages to
firmware.
Post by Song Liu
---
drivers/misc/enclosure.c | 29 ++++++++++++++
drivers/scsi/ses.c | 98
++++++++++++++++++++++++++++++++++++++++++-----
Post by Song Liu
include/linux/enclosure.h | 6 +++
3 files changed, 124 insertions(+), 9 deletions(-)
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c in=
dex
Post by Song Liu
de335bc..0331dfe 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -148,6 +148,7 @@ enclosure_register(struct device *dev, const ch=
ar
*name, int components,
Post by Song Liu
for (i =3D 0; i < components; i++) {
edev->component[i].number =3D -1;
edev->component[i].slot =3D -1;
+ edev->component[i].power_status =3D 1;
}
mutex_lock(&container_list_lock);
@@ -546,6 +547,31 @@ static ssize_t set_component_locate(struct
device *cdev,
Post by Song Liu
return count;
}
+static ssize_t get_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct enclosure_device *edev =3D to_enclosure_device(cdev->paren=
t);
Post by Song Liu
+ struct enclosure_component *ecomp =3D
to_enclosure_component(cdev);
Post by Song Liu
+
+ if (edev->cb->get_power_status)
+ edev->cb->get_power_status(edev, ecomp);
+ return snprintf(buf, 40, "%d\n", ecomp->power_status); }
+
+static ssize_t set_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ const char *buf, size_t count) {
+ struct enclosure_device *edev =3D to_enclosure_device(cdev->paren=
t);
Post by Song Liu
+ struct enclosure_component *ecomp =3D
to_enclosure_component(cdev);
Post by Song Liu
+ int val =3D simple_strtoul(buf, NULL, 0);
+
+ if (edev->cb->set_power_status)
+ edev->cb->set_power_status(edev, ecomp, val);
+ return count;
+}
+
Just using a number here doesn't seem to be very instructive; what is=
this
number? Can't we have a decode setting here to allow even the uniniti=
ated
to do some sensible decisions here?
=20
Post by Song Liu
static ssize_t get_component_type(struct device *cdev,
struct device_attribute *attr, char *buf)
get_component_active,
Post by Song Liu
set_component_active);
static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate=
,
Post by Song Liu
set_component_locate);
+static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR,
get_component_power_status,
Post by Song Liu
+ set_component_power_status);
static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL); stat=
ic
Post by Song Liu
DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
@@ -585,6 +613,7 @@ static struct attribute
*enclosure_component_attrs[] =3D {
Post by Song Liu
&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
bafa301..ea6b262 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -67,6 +67,20 @@ static int ses_probe(struct device *dev) #defin=
e
Post by Song Liu
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] =3D 0;
+ /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+ if (ecomp->type =3D=3D ENCLOSURE_COMPONENT_DEVICE)
+ dest_desc[1] =3D 0;
+ dest_desc[2] &=3D 0xde;
+ dest_desc[3] &=3D 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_dev=
ice
*edev,
Post by Song Liu
struct enclosure_component *ecomp,
enum enclosure_component_setting val) {
- unsigned char desc[4] =3D {0 };
+ unsigned char desc[4];
+ unsigned char *desc_ptr;
+
+ desc_ptr =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
switch (val) {
- /* zero is disabled */
+ desc[3] &=3D 0xdf;
break;
- desc[3] =3D 0x20;
+ desc[3] |=3D 0x20;
break;
static int ses_set_locate(struct enclosure_device *edev,
Hmm? Garbled patch?
=20
Post by Song Liu
struct enclosure_component *ecomp,
enum enclosure_component_setting val) {
- unsigned char desc[4] =3D {0 };
+ unsigned char desc[4];
Why did you remove the initialisation here?
Post by Song Liu
+ unsigned char *desc_ptr;
+
+ desc_ptr =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
switch (val) {
- /* zero is disabled */
+ desc[2] &=3D 0xfd;
break;
- desc[2] =3D 0x02;
+ desc[2] |=3D 0x02;
break;
struct enclosure_component *ecomp,
enum enclosure_component_setting val) {
- unsigned char desc[4] =3D {0 };
+ unsigned char desc[4];
+ unsigned char *desc_ptr;
+
+ desc_ptr =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
switch (val) {
- /* zero is disabled */
+ desc[2] &=3D 0x7f;
ecomp->active =3D 0;
break;
- desc[2] =3D 0x80;
+ desc[2] |=3D 0x80;
ecomp->active =3D 1;
break;
@@ -265,12 +303,53 @@ static int ses_show_id(struct enclosure_devic=
e
*edev, char *buf)
Post by Song Liu
return sprintf(buf, "%#llx\n", id);
}
+static void ses_get_power_status(struct enclosure_device *edev,
+ struct enclosure_component *ecomp) {
+ unsigned char *desc;
+
+ desc =3D ses_get_page2_descriptor(edev, ecomp);
+ if (desc)
+ ecomp->power_status =3D (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 =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
+
+ switch (val) {
+ /* power =3D 1 is device_off =3D 0 and vice versa */
+ desc[3] |=3D 0x10;
+ break;
+ desc[3] &=3D 0xef;
+ break;
+ return -EINVAL;
+ }
+ ecomp->power_status =3D val;
+ return ses_set_page2_descriptor(edev, ecomp, desc); }
+
static struct enclosure_component_callbacks ses_enclosure_callback=
s =3D {
Post by Song Liu
.get_fault =3D ses_get_fault,
.set_fault =3D ses_set_fault,
.get_status =3D ses_get_status,
.get_locate =3D ses_get_locate,
.set_locate =3D ses_set_locate,
+ .get_power_status =3D ses_get_power_status,
+ .set_power_status =3D ses_set_power_status,
.set_active =3D ses_set_active,
.show_id =3D ses_show_id,
};
@@ -448,6 +527,7 @@ static void ses_enclosure_data_process(struct
enclosure_device *edev,
Post by Song Liu
ecomp =3D &edev-
component[components++];
if (!IS_ERR(ecomp)) {
+ ses_get_power_status(edev, ecomp);
if (addl_desc_ptr)
ses_process_descriptor(ecomp,
addl_desc_ptr);
Post by Song Liu
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
Cheers,
=20
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg
GF: J. Hawn, J. Guild, F. Imend=F6rffer, HRB 16746 (AG N=FCrnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" i=
n
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-09-12 21:08:31 UTC
Permalink
=46rom 53549716d9f965e59f3a84feb5ebae9d18232b52 Mon Sep 17 00:00:00 200=
1
=46rom: Song Liu <***@fb.com>
Date: Tue, 19 Aug 2014 23:58:58 -0700
Subject: [PATCH 5/5] SES: Add power_status to SES enclosure component

Add power_status to SES enclosure component, 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 <***@fb.com>
Acked-by: Dan Williams <***@intel.com>
Reviewed-by: Jens Axboe <***@fb.com>
Cc: Hannes Reinecke <***@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 2e3eafa..819f2f2 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 =3D 0; i < components; i++) {
edev->component[i].number =3D -1;
edev->component[i].slot =3D -1;
+ edev->component[i].power_status =3D 1;
}
=20
mutex_lock(&container_list_lock);
@@ -546,6 +547,40 @@ static ssize_t set_component_locate(struct device =
*cdev,
return count;
}
=20
+static ssize_t get_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct enclosure_device *edev =3D to_enclosure_device(cdev->parent);
+ struct enclosure_component *ecomp =3D 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 =3D to_enclosure_device(cdev->parent);
+ struct enclosure_component *ecomp =3D to_enclosure_component(cdev);
+ int val;
+
+ if (strncmp(buf, "on", 2) =3D=3D 0 &&
+ (buf[2] =3D=3D '\n' || buf[2] =3D=3D '\0'))
+ val =3D 1;
+ else if (strncmp(buf, "off", 3) =3D=3D 0 &&
+ (buf[3] =3D=3D '\n' || buf[3] =3D=3D '\0'))
+ val =3D 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)
{
@@ -577,6 +612,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, get_c=
omponent_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_powe=
r_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);
=20
@@ -585,6 +622,7 @@ static struct attribute *enclosure_component_attrs[=
] =3D {
&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 3e59a5d..8ba3c78 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
=20
+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] =3D 0;
+ /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+ if (ecomp->type =3D=3D ENCLOSURE_COMPONENT_DEVICE)
+ dest_desc[1] =3D 0;
+ dest_desc[2] &=3D 0xde;
+ dest_desc[3] &=3D 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] =3D {0 };
+ unsigned char desc[4];
+ unsigned char *desc_ptr;
+
+ desc_ptr =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
=20
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
- /* zero is disabled */
+ desc[3] &=3D 0xdf;
break;
case ENCLOSURE_SETTING_ENABLED:
- desc[3] =3D 0x20;
+ desc[3] |=3D 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] =3D {0 };
+ unsigned char desc[4];
+ unsigned char *desc_ptr;
+
+ desc_ptr =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
=20
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
- /* zero is disabled */
+ desc[2] &=3D 0xfd;
break;
case ENCLOSURE_SETTING_ENABLED:
- desc[2] =3D 0x02;
+ desc[2] |=3D 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] =3D {0 };
+ unsigned char desc[4];
+ unsigned char *desc_ptr;
+
+ desc_ptr =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
=20
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
- /* zero is disabled */
+ desc[2] &=3D 0x7f;
ecomp->active =3D 0;
break;
case ENCLOSURE_SETTING_ENABLED:
- desc[2] =3D 0x80;
+ desc[2] |=3D 0x80;
ecomp->active =3D 1;
break;
default:
@@ -265,12 +303,53 @@ static int ses_show_id(struct enclosure_device *e=
dev, char *buf)
return sprintf(buf, "%#llx\n", id);
}
=20
+static void ses_get_power_status(struct enclosure_device *edev,
+ struct enclosure_component *ecomp)
+{
+ unsigned char *desc;
+
+ desc =3D ses_get_page2_descriptor(edev, ecomp);
+ if (desc)
+ ecomp->power_status =3D (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 =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
+
+ switch (val) {
+ /* power =3D 1 is device_off =3D 0 and vice versa */
+ case 0:
+ desc[3] |=3D 0x10;
+ break;
+ case 1:
+ desc[3] &=3D 0xef;
+ break;
+ default:
+ return -EINVAL;
+ }
+ ecomp->power_status =3D val;
+ return ses_set_page2_descriptor(edev, ecomp, desc);
+}
+
static struct enclosure_component_callbacks ses_enclosure_callbacks =3D=
{
.get_fault =3D ses_get_fault,
.set_fault =3D ses_set_fault,
.get_status =3D ses_get_status,
.get_locate =3D ses_get_locate,
.set_locate =3D ses_set_locate,
+ .get_power_status =3D ses_get_power_status,
+ .set_power_status =3D ses_set_power_status,
.set_active =3D ses_set_active,
.show_id =3D ses_show_id,
};
@@ -449,6 +528,7 @@ static void ses_enclosure_data_process(struct enclo=
sure_device *edev,
ecomp =3D &edev->component[components++];
=20
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);
};
=20
@@ -94,6 +99,7 @@ struct enclosure_component {
int locate;
int slot;
enum enclosure_status status;
+ int power_status;
};
=20
struct enclosure_device {
--=20
1.8.1
-----Original Message-----
From: Song Liu
Sent: Friday, September 12, 2014 2:08 PM
Cc: Dan Williams; Jens Axboe
Subject: RE: [PATCH 5/5] SES: Add power_status to SES enclosure compo=
nent
=20
New patch in next email...
=20
Change to let power_status show "on" or "off"
=20
Initialization of desc was removed because it will be initialized in
init_device_slot_control, so no need to initialize at define.
=20
Thanks,
Song
=20
-----Original Message-----
Sent: Thursday, September 4, 2014 12:59 AM
Cc: Dan Williams; Jens Axboe
Subject: Re: [PATCH 5/5] SES: Add power_status to SES enclosure
component
Post by Song Liu
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Hannes Reinecke
Subject: [PATCH 5/5] SES: Add power_status to SES enclosure component
Add power_status to SES enclosure component, so we can power on/o=
ff
the HDDs behind the enclosure.
Post by Song Liu
Check firmware status in ses_set_* before sending control pages t=
o
firmware.
Post by Song Liu
---
drivers/misc/enclosure.c | 29 ++++++++++++++
drivers/scsi/ses.c | 98
++++++++++++++++++++++++++++++++++++++++++-----
Post by Song Liu
include/linux/enclosure.h | 6 +++
3 files changed, 124 insertions(+), 9 deletions(-)
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index de335bc..0331dfe 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,
Post by Song Liu
for (i =3D 0; i < components; i++) {
edev->component[i].number =3D -1;
edev->component[i].slot =3D -1;
+ edev->component[i].power_status =3D 1;
}
mutex_lock(&container_list_lock);
@@ -546,6 +547,31 @@ static ssize_t set_component_locate(struct
device *cdev,
Post by Song Liu
return count;
}
+static ssize_t get_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct enclosure_device *edev =3D to_enclosure_device(cdev->par=
ent);
Post by Song Liu
+ struct enclosure_component *ecomp =3D
to_enclosure_component(cdev);
Post by Song Liu
+
+ if (edev->cb->get_power_status)
+ edev->cb->get_power_status(edev, ecomp);
+ return snprintf(buf, 40, "%d\n", ecomp->power_status); }
+
+static ssize_t set_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ const char *buf, size_t count) {
+ struct enclosure_device *edev =3D to_enclosure_device(cdev->par=
ent);
Post by Song Liu
+ struct enclosure_component *ecomp =3D
to_enclosure_component(cdev);
Post by Song Liu
+ int val =3D simple_strtoul(buf, NULL, 0);
+
+ if (edev->cb->set_power_status)
+ edev->cb->set_power_status(edev, ecomp, val);
+ return count;
+}
+
Just using a number here doesn't seem to be very instructive; what =
is
this number? Can't we have a decode setting here to allow even the
uninitiated to do some sensible decisions here?
Post by Song Liu
static ssize_t get_component_type(struct device *cdev,
struct device_attribute *attr, char *buf)
get_component_active,
Post by Song Liu
set_component_active);
static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR,
get_component_locate,
Post by Song Liu
set_component_locate);
+static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR,
get_component_power_status,
Post by Song Liu
+ set_component_power_status);
static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
@@ -585,6 +613,7 @@ static struct attribute
*enclosure_component_attrs[] =3D {
Post by Song Liu
&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
bafa301..ea6b262 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -67,6 +67,20 @@ static int ses_probe(struct device *dev) #def=
ine
Post by Song Liu
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] =3D 0;
+ /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+ if (ecomp->type =3D=3D ENCLOSURE_COMPONENT_DEVICE)
+ dest_desc[1] =3D 0;
+ dest_desc[2] &=3D 0xde;
+ dest_desc[3] &=3D 0x3c;
+}
+
+
static int ses_recv_diag(struct scsi_device *sdev, int page_code=
,
Post by Song Liu
void *buf, int bufflen)
{
@@ -178,14 +192,22 @@ static int ses_set_fault(struct
enclosure_device
*edev,
Post by Song Liu
struct enclosure_component *ecomp,
enum enclosure_component_setting val) {
- unsigned char desc[4] =3D {0 };
+ unsigned char desc[4];
+ unsigned char *desc_ptr;
+
+ desc_ptr =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
switch (val) {
- /* zero is disabled */
+ desc[3] &=3D 0xdf;
break;
- desc[3] =3D 0x20;
+ desc[3] |=3D 0x20;
break;
static int ses_set_locate(struct enclosure_device *edev,
Hmm? Garbled patch?
Post by Song Liu
struct enclosure_component *ecomp,
enum enclosure_component_setting val) {
- unsigned char desc[4] =3D {0 };
+ unsigned char desc[4];
Why did you remove the initialisation here?
Post by Song Liu
+ unsigned char *desc_ptr;
+
+ desc_ptr =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
switch (val) {
- /* zero is disabled */
+ desc[2] &=3D 0xfd;
break;
- desc[2] =3D 0x02;
+ desc[2] |=3D 0x02;
break;
struct enclosure_component *ecomp,
enum enclosure_component_setting val) {
- unsigned char desc[4] =3D {0 };
+ unsigned char desc[4];
+ unsigned char *desc_ptr;
+
+ desc_ptr =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
switch (val) {
- /* zero is disabled */
+ desc[2] &=3D 0x7f;
ecomp->active =3D 0;
break;
- desc[2] =3D 0x80;
+ desc[2] |=3D 0x80;
ecomp->active =3D 1;
break;
@@ -265,12 +303,53 @@ static int ses_show_id(struct enclosure_dev=
ice
*edev, char *buf)
Post by Song Liu
return sprintf(buf, "%#llx\n", id); }
+static void ses_get_power_status(struct enclosure_device *edev,
+ struct enclosure_component *ecomp) {
+ unsigned char *desc;
+
+ desc =3D ses_get_page2_descriptor(edev, ecomp);
+ if (desc)
+ ecomp->power_status =3D (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 =3D ses_get_page2_descriptor(edev, ecomp);
+
+ if (!desc_ptr)
+ return -EIO;
+
+ init_device_slot_control(desc, ecomp, desc_ptr);
+
+ switch (val) {
+ /* power =3D 1 is device_off =3D 0 and vice versa */
+ desc[3] |=3D 0x10;
+ break;
+ desc[3] &=3D 0xef;
+ break;
+ return -EINVAL;
+ }
+ ecomp->power_status =3D val;
+ return ses_set_page2_descriptor(edev, ecomp, desc); }
+
static struct enclosure_component_callbacks ses_enclosure_callba=
cks =3D
{
Post by Song Liu
.get_fault =3D ses_get_fault,
.set_fault =3D ses_set_fault,
.get_status =3D ses_get_status,
.get_locate =3D ses_get_locate,
.set_locate =3D ses_set_locate,
+ .get_power_status =3D ses_get_power_status,
+ .set_power_status =3D ses_set_power_status,
.set_active =3D ses_set_active,
.show_id =3D ses_show_id,
};
@@ -448,6 +527,7 @@ static void ses_enclosure_data_process(struct
enclosure_device *edev,
Post by Song Liu
ecomp =3D &edev-
component[components++];
if (!IS_ERR(ecomp)) {
+ ses_get_power_status(edev, ecomp);
if (addl_desc_ptr)
ses_process_descriptor(ecomp,
addl_desc_ptr);
Post by Song Liu
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.=
h
Post by Song Liu
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
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg
GF: J. Hawn, J. Guild, F. Imend=F6rffer, HRB 16746 (AG N=FCrnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" i=
n
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-08-25 17:34:42 UTC
Permalink
From: Song Liu [mailto:***@fb.com]
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Dan Williams; Hannes Reinecke
Subject: [PATCH 4/5] SES: add reliable slot attribute

From: Dan 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 <***@intel.com>
Signed-off-by: Song Liu <***@fb.com>
Reviewed-by: Jens Axboe <***@fb.com>
Cc: Hannes Reinecke <***@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 646068a..de335bc 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); @@ -552,6 +554,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);
@@ -562,6 +578,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,
@@ -569,6 +586,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 61deb4e..bafa301 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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-09-12 21:04:38 UTC
Permalink
From 1cb4f9b4c57425cc3462b9615ac5bd013f7f4a88 Mon Sep 17 00:00:00 2001
From: Dan Williams <***@intel.com>
Date: Thu, 21 Aug 2014 11:43:26 -0700
Subject: [PATCH 4/5] SES: add reliable slot attribute

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 <***@intel.com>
Signed-off-by: Song Liu <***@fb.com>
Reviewed-by: Jens Axboe <***@fb.com>
Reviewed-by: Hannes Reinecke <***@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 18b87de..2e3eafa 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);
@@ -552,6 +554,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);
@@ -562,6 +578,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,
@@ -569,6 +586,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 696d5d8..3e59a5d 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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-08-25 17:34:39 UTC
Permalink
From: Song Liu [mailto:***@fb.com]
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Dan Williams; Hannes Reinecke
Subject: [PATCH 2/5] SES: generate KOBJ_CHANGE on enclosure attach

From: Dan 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 <***@intel.com>
Signed-off-by: Song Liu <***@fb.com>
Reviewed-by: Jens Axboe <***@fb.com>
Cc: Hannes Reinecke <***@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 c2e8a98..8f0a62a 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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-09-12 21:03:24 UTC
Permalink
From 741aa4b6043af741e28c0c414d48249f878353dc Mon Sep 17 00:00:00 2001
From: Dan Williams <***@intel.com>
Date: Thu, 21 Aug 2014 11:43:22 -0700
Subject: [PATCH 2/5] SES: generate KOBJ_CHANGE on enclosure attach

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 <***@intel.com>
Signed-off-by: Song Liu <***@fb.com>
Reviewed-by: Jens Axboe <***@fb.com>
Reviewed-by: Hannes Reinecke <***@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 c52fd98..87cf970b 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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-08-25 17:34:41 UTC
Permalink
From: Song Liu [mailto:***@fb.com]
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Dan Williams; Hannes Reinecke
Subject: [PATCH 3/5] SES: add enclosure logical id

From: Dan 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 <***@intel.com>
Signed-off-by: Song Liu <***@fb.com>
Reviewed-by: Jens Axboe <***@fb.com>
Cc: Hannes Reinecke <***@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 15faf61..646068a 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -395,8 +395,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 0;
+}
+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 8f0a62a..61deb4e 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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-09-12 21:04:16 UTC
Permalink
=46rom 849dd255bdd2158cb621697e41448cf6b230d2f4 Mon Sep 17 00:00:00 200=
1
=46rom: Dan Williams <***@intel.com>
Date: Thu, 21 Aug 2014 11:43:24 -0700
Subject: [PATCH 3/5] SES: add enclosure logical id

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 <***@intel.com>
Signed-off-by: Song Liu <***@fb.com>
Reviewed-by: Jens Axboe <***@fb.com>
Cc: Hannes Reinecke <***@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 15faf61..18b87de 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -395,8 +395,21 @@ static ssize_t components_show(struct device *cdev=
,
}
static DEVICE_ATTR_RO(components);
=20
+static ssize_t id_show(struct device *cdev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct enclosure_device *edev =3D 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[] =3D {
&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 87cf970b..696d5d8 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);
}
=20
+static int ses_show_id(struct enclosure_device *edev, char *buf)
+{
+ struct ses_device *ses_dev =3D edev->scratch;
+ unsigned long long id =3D get_unaligned_be64(ses_dev->page1+8+4);
+
+ return sprintf(buf, "%#llx\n", id);
+}
+
static struct enclosure_component_callbacks ses_enclosure_callbacks =3D=
{
.get_fault =3D ses_get_fault,
.set_fault =3D ses_set_fault,
@@ -265,6 +273,7 @@ static struct enclosure_component_callbacks ses_enc=
losure_callbacks =3D {
.get_locate =3D ses_get_locate,
.set_locate =3D ses_set_locate,
.set_active =3D ses_set_active,
+ .show_id =3D ses_show_id,
};
=20
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);
};
=20
=20
--=20
1.8.1
-----Original Message-----
Sent: Thursday, September 4, 2014 12:55 AM
Cc: Dan Williams; Jens Axboe
Subject: Re: [PATCH 3/5] SES: add enclosure logical id
=20
Post by Song Liu
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Dan Williams; Hannes Reinecke
Subject: [PATCH 3/5] SES: add enclosure logical id
Export the NAA logical id for the enclosure. This is optionally av=
ailable
from the sas_transport_class, but it is really a property of the encl=
osure.
Post by Song Liu
---
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 in=
dex
Post by Song Liu
15faf61..646068a 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -395,8 +395,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 =3D to_enclosure_device(cdev);
+
+ if (edev->cb->show_id)
+ return edev->cb->show_id(edev, buf);
+ return 0;
+}
+static DEVICE_ATTR_RO(id);
+
static struct attribute *enclosure_class_attrs[] =3D {
&dev_attr_components.attr,
+ &dev_attr_id.attr,
NULL,
};
ATTRIBUTE_GROUPS(enclosure_class);
=20
Maybe you should return -EINVAL or something here; '0' would mean an
enclosure id of length '0', which is a different meaning from 'enclos=
ure id not
available'.
=20
Post by Song Liu
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index
8f0a62a..61deb4e 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -258,6 +258,14 @@ static int ses_set_active(struct enclosure_dev=
ice
*edev,
Post by Song Liu
return ses_set_page2_descriptor(edev, ecomp, desc); }
+static int ses_show_id(struct enclosure_device *edev, char *buf) {
+ struct ses_device *ses_dev =3D edev->scratch;
+ unsigned long long id =3D get_unaligned_be64(ses_dev->page1+8+4);
+
+ return sprintf(buf, "%#llx\n", id);
+}
+
static struct enclosure_component_callbacks ses_enclosure_callback=
s =3D {
Post by Song Liu
.get_fault =3D ses_get_fault,
.set_fault =3D ses_set_fault,
@@ -265,6 +273,7 @@ static struct enclosure_component_callbacks
ses_enclosure_callbacks =3D {
Post by Song Liu
.get_locate =3D ses_get_locate,
.set_locate =3D ses_set_locate,
.set_active =3D ses_set_active,
+ .show_id =3D 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
Cheers,
=20
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg
GF: J. Hawn, J. Guild, F. Imend=F6rffer, HRB 16746 (AG N=FCrnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" i=
n
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Jens Axboe
2014-10-22 19:12:01 UTC
Permalink
Post by Song Liu
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Subject: [PATCH 0/5] Feature enhancements for 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
# 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.
SES: close potential registration race
SES: generate KOBJ_CHANGE on enclosure attach
SES: add enclosure logical id
SES: add reliable slot attribute
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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Douglas Gilbert
2014-10-22 22:28:41 UTC
Permalink
Post by Jens Axboe
Post by Song Liu
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Subject: [PATCH 0/5] Feature enhancements for 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
# 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
Post by Jens Axboe
Post by Song Liu
SES: close potential registration race
SES: generate KOBJ_CHANGE on enclosure attach
SES: add enclosure logical id
SES: add reliable slot attribute
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-10-22 23:01:35 UTC
Permalink
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-----
Sent: Wednesday, October 22, 2014 3:29 PM
Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
Subject: Re: [PATCH 0/5] Feature enhancements for ses module
Post by Jens Axboe
Post by Song Liu
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Subject: [PATCH 0/5] Feature enhancements for 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
# 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
Post by Jens Axboe
Post by Song Liu
SES: close potential registration race
SES: generate KOBJ_CHANGE on enclosure attach
SES: add enclosure logical id
SES: add reliable slot attribute
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Douglas Gilbert
2014-10-23 01:17:20 UTC
Permalink
Post by Song Liu
Hi Doug,
+ 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.
Post by Song Liu
-----Original Message-----
Sent: Wednesday, October 22, 2014 3:29 PM
Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
Subject: Re: [PATCH 0/5] Feature enhancements for ses module
Post by Jens Axboe
Post by Song Liu
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Subject: [PATCH 0/5] Feature enhancements for 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
# 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
Post by Jens Axboe
Post by Song Liu
SES: close potential registration race
SES: generate KOBJ_CHANGE on enclosure attach
SES: add enclosure logical id
SES: add reliable slot attribute
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
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Song Liu
2014-10-23 03:42:09 UTC
Permalink
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-----
Sent: Wednesday, October 22, 2014 6:17 PM
Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
Subject: Re: [PATCH 0/5] Feature enhancements for ses module
Post by Song Liu
Hi Doug,
I am not sure whether I fully understand the scenario. Actually patch 5 tries to
+ 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
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];
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.
Post by Song Liu
-----Original Message-----
Sent: Wednesday, October 22, 2014 3:29 PM
Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
Subject: Re: [PATCH 0/5] Feature enhancements for ses module
Post by Jens Axboe
Post by Song Liu
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Subject: [PATCH 0/5] Feature enhancements for 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
# 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.
Post by Song Liu
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
Post by Jens Axboe
Post by Song Liu
SES: close potential registration race
SES: generate KOBJ_CHANGE on enclosure attach
SES: add enclosure logical id
SES: add reliable slot attribute
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"
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
Post by Song Liu
4aa8201dde45d6
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-10-23 08:45:04 UTC
Permalink
Post by Jens Axboe
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Loading...