RE: [PATCH 5/5] SES: Add power_status to SES enclosure component
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 init_device_slot_control, so no need to initialize at define. Thanks, Song -Original Message- From: Hannes Reinecke [mailto:h...@suse.de] Sent: Thursday, September 4, 2014 12:59 AM To: Song Liu; linux-scsi@vger.kernel.org Cc: Dan Williams; Jens Axboe Subject: Re: [PATCH 5/5] SES: Add power_status to SES enclosure component On 08/25/2014 07:34 PM, Song Liu wrote: From: Song Liu [mailto:songliubrav...@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 songliubrav...@fb.com Acked-by: Dan Williams dan.j.willi...@intel.com Reviewed-by: Jens Axboe ax...@fb.com Cc: Hannes Reinecke h...@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; +} + 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? 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
RE: [PATCH 5/5] SES: Add power_status to SES enclosure component
From 53549716d9f965e59f3a84feb5ebae9d18232b52 Mon Sep 17 00:00:00 2001 From: Song Liu songliubrav...@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 songliubrav...@fb.com Acked-by: Dan Williams dan.j.willi...@intel.com Reviewed-by: Jens Axboe ax...@fb.com Cc: Hannes Reinecke h...@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 = 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,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) { @@ -577,6 +612,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 +622,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 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 +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
Re: [PATCH 5/5] SES: Add power_status to SES enclosure component
On 08/25/2014 07:34 PM, Song Liu wrote: From: Song Liu [mailto:songliubrav...@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 songliubrav...@fb.com Acked-by: Dan Williams dan.j.willi...@intel.com Reviewed-by: Jens Axboe ax...@fb.com Cc: Hannes Reinecke h...@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; +} + 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? 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
[PATCH 5/5] SES: Add power_status to SES enclosure component
From: Song Liu [mailto:songliubrav...@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 songliubrav...@fb.com Acked-by: Dan Williams dan.j.willi...@intel.com Reviewed-by: Jens Axboe ax...@fb.com Cc: Hannes Reinecke h...@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