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:hare@xxxxxxx] > Sent: Thursday, September 4, 2014 12:59 AM > To: Song Liu; linux-scsi@xxxxxxxxxxxxxxx > 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:songliubraving@xxxxxx] > > 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 <songliubraving@xxxxxx> > > Acked-by: Dan Williams <dan.j.williams@xxxxxxxxx> > > Reviewed-by: Jens Axboe <axboe@xxxxxx> > > Cc: Hannes Reinecke <hare@xxxxxxx> > > --- > > 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; > > 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, > Hmm? Garbled patch? > > > struct enclosure_component *ecomp, > > enum enclosure_component_setting val) { > > - unsigned char desc[4] = {0 }; > > + unsigned char desc[4]; > Why did you remove the initialisation here? > > > + 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 > > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke zSeries & Storage > hare@xxxxxxx +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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
