[RFC] libsas: the trouble with ata resets

[RFC] libsas: the trouble with ata resets

[Dan Williams]

Currently libsas has a problem with prematurely dropping sata devices
during recovery.  Libata knows that some devices can take quite a
while to recover from a reset and re-establish the link.  The fact
that sas_ata_hard_reset() ignores its 'deadline'  parameter is
evidence that it ignores the link management aspects of what libata
wants from a ->hardreset() handler.

item1: teach sas_ata_hard_reset() to check that the link came back up.
 For direct attached devices the lldd will need the deadline
parameter, and for expander attached perform smp polling to wait for
the link to come back.

Now, during this time that libata is trying to recover the connection
in the host-eh context libsas will start receiving BCNs in the
host-workqueue context.  In the unfortunate cases libsas may take
removal action on a device that will come back with a bit more time.
While libata-eh is in progress libsas should not take any action on
the ata phys in question..

item2:  flush eh before trying to determine what action to take on a phy.

In the case of libsas not all resets are initiated by the eh process
(the sas transport class can reset a phy directly).  It seems libata
takes care to arrange for user requested resets to occur under the
control of eh, and libsas should do the same.

item3: teach all reset entry points to kick and flush eh for ata devices

A corollary for items 1 and 3 is that there is a difference between
scheduling the reset and performing the reset.
->lldd_I_T_nexus_reset() is currently called twice, once by sas-eh to
manage sas_tasks and again by ata-eh to recover the device.  Likely we
need a new ->lldd_ata_hard_reset() handler that is called by ata-eh,
while ->lldd_I_T_nexus_reset() cleans up the sas_tasks and just
schedules reset on the ata_port.

item4: allow for lldd's to provide a direct ->lldd_ata_hard_reset()
which can be assumed to only be called from ata-eh context.

Any other pain points in reset handling?

--
Dan

Re:[RFC] libsas: the trouble with ata resets

[Jack Wang]

I have seen this problem too, but have not figure out how to do the right
thing. Add Jeff and James to CC, maybe they have more insight idea to fix
this.

BTW: how to push Redhat and SUSE to include the libsas fix like: T-T support
and others will cause this oops.

Jack

[RFC] libsas: the trouble with ata resets
> 
> Currently libsas has a problem with prematurely dropping sata devices
> during recovery.  Libata knows that some devices can take quite a
> while to recover from a reset and re-establish the link.  The fact
> that sas_ata_hard_reset() ignores its 'deadline'  parameter is
> evidence that it ignores the link management aspects of what libata
> wants from a ->hardreset() handler.
> 
> item1: teach sas_ata_hard_reset() to check that the link came back up.
>  For direct attached devices the lldd will need the deadline
> parameter, and for expander attached perform smp polling to wait for
> the link to come back.
> 
> Now, during this time that libata is trying to recover the connection
> in the host-eh context libsas will start receiving BCNs in the
> host-workqueue context.  In the unfortunate cases libsas may take
> removal action on a device that will come back with a bit more time.
> While libata-eh is in progress libsas should not take any action on
> the ata phys in question..
> 
> item2:  flush eh before trying to determine what action to take on a phy.
> 
> In the case of libsas not all resets are initiated by the eh process
> (the sas transport class can reset a phy directly).  It seems libata
> takes care to arrange for user requested resets to occur under the
> control of eh, and libsas should do the same.
> 
> item3: teach all reset entry points to kick and flush eh for ata devices
> 
> A corollary for items 1 and 3 is that there is a difference between
> scheduling the reset and performing the reset.
> ->lldd_I_T_nexus_reset() is currently called twice, once by sas-eh to
> manage sas_tasks and again by ata-eh to recover the device.  Likely we
> need a new ->lldd_ata_hard_reset() handler that is called by ata-eh,
> while ->lldd_I_T_nexus_reset() cleans up the sas_tasks and just
> schedules reset on the ata_port.
> 
> item4: allow for lldd's to provide a direct ->lldd_ata_hard_reset()
> which can be assumed to only be called from ata-eh context.
> 
> Any other pain points in reset handling?
> 
> --
> Dan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] enclosure & ses: modernize and add target power management

[Mark Salyzyn]

[-- Attachment #1: Type: text/plain, Size: 9044 bytes --]

The enclosed code change suggestion is proposed to modernize the
enclosure drivers. The changes outlined are but only compile &
checkpatch tested so do not propagate. We will be testing this on a real
enclosure... Inspection is invited. We would however like the answer to
a question (with two parts) and a few others:

1) Is /sys/.../enclosure_device:#/device_power an acceptable node name
for switching the power on and off for the target?
2) Do you think that target power management be made part of the Linux
power management hierarchy (and clues/pointers to how would be
nice :-) )
3) Add other enclosure bits (eg: Lock & LED control) while we are here?
4) Should the enclosure_component retained values be changed to
bit-fields?
5) Does Evolution work better than Outlook for delivering patch content?

Coincident repair of enclosure and ses device support. Fix problems with
setting enclosure values careful to preserve other settings. Corrected
fault setting as well. Modernize code. Add support for Device Power
Management.

Future: Add target device power-cycle to error recovery escalation,
between target reset and host bus adapter reset (has to be added to each
lib<transport> handler, difficult to visualize in error-handler common
code :-( ) ?

Signed-off-by: mark_salyzyn@us.xyratex.com

include/linux/enclosure.h |    6 +++
drivers/misc/enclosure.c  |   45 +++++++++++++++++++-----
drivers/scsi/ses.c        |   83
+++++++++++++++++++++++++++++++++-------------
3 files changed, 103 insertions(+), 31 deletions(-)

diff -rup a/include/linux/enclosure.h b/include/linux/enclosure.h
--- a/include/linux/enclosure.h 2011-11-11 10:14:07.000000000 -0500
+++ b/include/linux/enclosure.h 2011-11-11 10:09:00.000000000 -0500
@@ -79,6 +79,11 @@ struct enclosure_component_callbacks {
int (*set_locate)(struct enclosure_device *,
  struct enclosure_component *,
  enum enclosure_component_setting);
+ void (*get_device_power)(struct enclosure_device *,
+    struct enclosure_component *);
+ int (*set_device_power)(struct enclosure_device *,
+   struct enclosure_component *,
+   enum enclosure_component_setting);
};


@@ -91,6 +96,7 @@ struct enclosure_component {
int fault;
int active;
int locate;
+ int device_power;
enum enclosure_status status;
};

diff -rup a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
--- a/drivers/misc/enclosure.c 2011-11-11 10:19:45.000000000 -0500
+++ b/drivers/misc/enclosure.c 2011-11-11 10:36:35.000000000 -0500
@@ -416,10 +416,10 @@ static ssize_t set_component_fault(struc
{
struct enclosure_device *edev = to_enclosure_device(cdev->parent);
struct enclosure_component *ecomp = to_enclosure_component(cdev);
- int val = simple_strtoul(buf, NULL, 0);
+ unsigned long val;

- if (edev->cb->set_fault)
- edev->cb->set_fault(edev, ecomp, val);
+ if (!strict_strtoul(buf, 0, &val) && edev->cb->set_fault)
+ edev->cb->set_fault(edev, ecomp, (int)val);
return count;
}

@@ -474,10 +474,10 @@ static ssize_t set_component_active(stru
{
struct enclosure_device *edev = to_enclosure_device(cdev->parent);
struct enclosure_component *ecomp = to_enclosure_component(cdev);
- int val = simple_strtoul(buf, NULL, 0);
+ unsigned long val;

- if (edev->cb->set_active)
- edev->cb->set_active(edev, ecomp, val);
+ if (!strict_strtoul(buf, 0, &val) && edev->cb->set_active)
+ edev->cb->set_active(edev, ecomp, (int)val);
return count;
}

@@ -498,10 +498,34 @@ static ssize_t set_component_locate(stru
{
struct enclosure_device *edev = to_enclosure_device(cdev->parent);
struct enclosure_component *ecomp = to_enclosure_component(cdev);
- int val = simple_strtoul(buf, NULL, 0);
+ unsigned long val;

- if (edev->cb->set_locate)
- edev->cb->set_locate(edev, ecomp, val);
+ if (!strict_strtoul(buf, 0, &val) && edev->cb->set_locate)
+ edev->cb->set_locate(edev, ecomp, (int)val);
+ return count;
+}
+
+static ssize_t get_component_device_power(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_device_power)
+ edev->cb->get_device_power(edev, ecomp);
+ return snprintf(buf, 40, "%d\n", ecomp->device_power);
+}
+
+static ssize_t set_component_device_power(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);
+ unsigned long val;
+
+ if (!strict_strtoul(buf, 0, &val) && edev->cb->set_device_power)
+ edev->cb->set_device_power(edev, ecomp, (int)val);
return count;
}

@@ -522,6 +546,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_I
   set_component_active);
static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
   set_component_locate);
+static DEVICE_ATTR(device_power, S_IRUGO | S_IWUSR,
get_component_device_power,
+    set_component_device_power);
static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);

static struct attribute *enclosure_component_attrs[] = {
@@ -529,6 +555,7 @@ static struct attribute *enclosure_compo
&dev_attr_status.attr,
&dev_attr_active.attr,
&dev_attr_locate.attr,
+ &dev_attr_device_power.attr,
&dev_attr_type.attr,
NULL
};
diff -rup a/drivers/scsi/ses.c b/drivers/scsi/ses.c
--- a/drivers/scsi/ses.c 2011-11-11 10:14:19.000000000 -0500
+++ b/drivers/scsi/ses.c 2011-11-11 10:35:14.000000000 -0500
@@ -167,18 +167,38 @@ static void ses_get_fault(struct enclosu
ecomp->fault = (desc[3] & 0x60) >> 4;
}

+static void ses_read_page2_descriptor(struct enclosure_device *edev,
+       struct enclosure_component *ecomp,
+       unsigned char *desc)
+{
+ unsigned char *desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+ if (desc_ptr)
+ memcpy(desc, desc_ptr, 4);
+ /* preserve active state */
+ if (ecomp->active)
+ desc[2] |= 0x80;
+ else
+ desc[2] &= ~0x80;
+ /* Clear all reserved/alternate-purpose bits */
+ desc[2] &= 0xE7;
+ desc[3] &= 0x3C;
+}
+
static int ses_set_fault(struct enclosure_device *edev,
  struct enclosure_component *ecomp,
enum enclosure_component_setting val)
{
unsigned char desc[4] = {0 };

+ ses_read_page2_descriptor(edev, ecomp, desc);
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
/* zero is disabled */
+ desc[3] &= ~0x20;
break;
case ENCLOSURE_SETTING_ENABLED:
- desc[2] = 0x02;
+ desc[3] |= 0x20;
break;
default:
/* SES doesn't do the SGPIO blink settings */
@@ -214,12 +234,46 @@ static int ses_set_locate(struct enclosu
{
unsigned char desc[4] = {0 };

+ ses_read_page2_descriptor(edev, ecomp, desc);
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
/* zero is disabled */
+ desc[2] &= ~0x02;
+ break;
+ case ENCLOSURE_SETTING_ENABLED:
+ desc[2] |= 0x02;
+ break;
+ default:
+ /* SES doesn't do the SGPIO blink settings */
+ return -EINVAL;
+ }
+ return ses_set_page2_descriptor(edev, ecomp, desc);
+}
+
+static void ses_get_device_power(struct enclosure_device *edev,
+    struct enclosure_component *ecomp)
+{
+ unsigned char *desc;
+
+ desc = ses_get_page2_descriptor(edev, ecomp);
+ if (desc)
+ ecomp->device_power = (desc[3] & 0x10) ? 0 : 1;
+}
+
+static int ses_set_device_power(struct enclosure_device *edev,
+   struct enclosure_component *ecomp,
+   enum enclosure_component_setting val)
+{
+ unsigned char desc[4] = {0 };
+
+ ses_read_page2_descriptor(edev, ecomp, desc);
+ switch (val) {
+ case ENCLOSURE_SETTING_DISABLED:
+ /* one is disabled */
+ desc[3] |= 0x10;
break;
case ENCLOSURE_SETTING_ENABLED:
- desc[2] = 0x02;
+ desc[3] &= ~0x10;
break;
default:
/* SES doesn't do the SGPIO blink settings */
@@ -234,13 +288,15 @@ static int ses_set_active(struct enclosu
{
unsigned char desc[4] = {0 };

+ ses_read_page2_descriptor(edev, ecomp, desc);
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
/* zero is disabled */
+ desc[2] &= ~0x80;
ecomp->active = 0;
break;
case ENCLOSURE_SETTING_ENABLED:
- desc[2] = 0x80;
+ desc[2] |= 0x80;
ecomp->active = 1;
break;
default:
@@ -256,6 +312,8 @@ static struct enclosure_component_callba
.get_status = ses_get_status,
.get_locate = ses_get_locate,
.set_locate = ses_set_locate,
+ .get_device_power = ses_get_device_power,
+ .set_device_power = ses_set_device_power,
.set_active = ses_set_active,
};

@@ -264,25 +322,6 @@ struct ses_host_edev {
struct enclosure_device *edev;
};

-#if 0
-int ses_match_host(struct enclosure_device *edev, void *data)
-{
- struct ses_host_edev *sed = data;
- struct scsi_device *sdev;
-
- if (!scsi_is_sdev_device(edev->edev.parent))
- return 0;
-
- sdev = to_scsi_device(edev->edev.parent);
-
- if (sdev->host != sed->shost)
- return 0;
-
- sed->edev = edev;
- return 1;
-}
-#endif  /*  0  */
-
static void ses_process_descriptor(struct enclosure_component *ecomp,
   unsigned char *desc)
{


[-- Attachment #2: enclosure_add_power.patch --]
[-- Type: text/x-patch, Size: 8225 bytes --]

Coincident repair of enclosure and ses device support. Fix problems with
setting enclosure values careful to preserve other settings. Corrected
fault setting to. Modernize code. Add support for Device Power Enable.

Signed-off-by: mark_salyzyn@us.xyratex.com

 include/linux/enclosure.h |    6 +++
 drivers/misc/enclosure.c  |   45 +++++++++++++++++++-----
 drivers/scsi/ses.c        |   83 +++++++++++++++++++++++++++++++++-------------
 3 files changed, 103 insertions(+), 31 deletions(-)

diff -rup a/include/linux/enclosure.h b/include/linux/enclosure.h
--- a/include/linux/enclosure.h	2011-11-11 10:14:07.000000000 -0500
+++ b/include/linux/enclosure.h	2011-11-11 10:09:00.000000000 -0500
@@ -79,6 +79,11 @@ struct enclosure_component_callbacks {
 	int (*set_locate)(struct enclosure_device *,
 			  struct enclosure_component *,
 			  enum enclosure_component_setting);
+	void (*get_device_power)(struct enclosure_device *,
+			   struct enclosure_component *);
+	int (*set_device_power)(struct enclosure_device *,
+			  struct enclosure_component *,
+			  enum enclosure_component_setting);
 };
 
 
@@ -91,6 +96,7 @@ struct enclosure_component {
 	int fault;
 	int active;
 	int locate;
+	int device_power;
 	enum enclosure_status status;
 };
 
diff -rup a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
--- a/drivers/misc/enclosure.c	2011-11-11 10:19:45.000000000 -0500
+++ b/drivers/misc/enclosure.c	2011-11-11 10:36:35.000000000 -0500
@@ -416,10 +416,10 @@ static ssize_t set_component_fault(struc
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
+	unsigned long val;
 
-	if (edev->cb->set_fault)
-		edev->cb->set_fault(edev, ecomp, val);
+	if (!strict_strtoul(buf, 0, &val) && edev->cb->set_fault)
+		edev->cb->set_fault(edev, ecomp, (int)val);
 	return count;
 }
 
@@ -474,10 +474,10 @@ static ssize_t set_component_active(stru
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
+	unsigned long val;
 
-	if (edev->cb->set_active)
-		edev->cb->set_active(edev, ecomp, val);
+	if (!strict_strtoul(buf, 0, &val) && edev->cb->set_active)
+		edev->cb->set_active(edev, ecomp, (int)val);
 	return count;
 }
 
@@ -498,10 +498,34 @@ static ssize_t set_component_locate(stru
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
+	unsigned long val;
 
-	if (edev->cb->set_locate)
-		edev->cb->set_locate(edev, ecomp, val);
+	if (!strict_strtoul(buf, 0, &val) && edev->cb->set_locate)
+		edev->cb->set_locate(edev, ecomp, (int)val);
+	return count;
+}
+
+static ssize_t get_component_device_power(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_device_power)
+		edev->cb->get_device_power(edev, ecomp);
+	return snprintf(buf, 40, "%d\n", ecomp->device_power);
+}
+
+static ssize_t set_component_device_power(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);
+	unsigned long val;
+
+	if (!strict_strtoul(buf, 0, &val) && edev->cb->set_device_power)
+		edev->cb->set_device_power(edev, ecomp, (int)val);
 	return count;
 }
 
@@ -522,6 +546,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_I
 		   set_component_active);
 static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
 		   set_component_locate);
+static DEVICE_ATTR(device_power, S_IRUGO | S_IWUSR, get_component_device_power,
+		   set_component_device_power);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
 
 static struct attribute *enclosure_component_attrs[] = {
@@ -529,6 +555,7 @@ static struct attribute *enclosure_compo
 	&dev_attr_status.attr,
 	&dev_attr_active.attr,
 	&dev_attr_locate.attr,
+	&dev_attr_device_power.attr,
 	&dev_attr_type.attr,
 	NULL
 };
diff -rup a/drivers/scsi/ses.c b/drivers/scsi/ses.c
--- a/drivers/scsi/ses.c	2011-11-11 10:14:19.000000000 -0500
+++ b/drivers/scsi/ses.c	2011-11-11 10:35:14.000000000 -0500
@@ -167,18 +167,38 @@ static void ses_get_fault(struct enclosu
 		ecomp->fault = (desc[3] & 0x60) >> 4;
 }
 
+static void ses_read_page2_descriptor(struct enclosure_device *edev,
+				      struct enclosure_component *ecomp,
+				      unsigned char *desc)
+{
+	unsigned char *desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+	if (desc_ptr)
+		memcpy(desc, desc_ptr, 4);
+	/* preserve active state */
+	if (ecomp->active)
+		desc[2] |= 0x80;
+	else
+		desc[2] &= ~0x80;
+	/* Clear all reserved/alternate-purpose bits */
+	desc[2] &= 0xE7;
+	desc[3] &= 0x3C;
+}
+
 static int ses_set_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			 enum enclosure_component_setting val)
 {
 	unsigned char desc[4] = {0 };
 
+	ses_read_page2_descriptor(edev, ecomp, desc);
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
 		/* zero is disabled */
+		desc[3] &= ~0x20;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[2] = 0x02;
+		desc[3] |= 0x20;
 		break;
 	default:
 		/* SES doesn't do the SGPIO blink settings */
@@ -214,12 +234,46 @@ static int ses_set_locate(struct enclosu
 {
 	unsigned char desc[4] = {0 };
 
+	ses_read_page2_descriptor(edev, ecomp, desc);
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
 		/* zero is disabled */
+		desc[2] &= ~0x02;
+		break;
+	case ENCLOSURE_SETTING_ENABLED:
+		desc[2] |= 0x02;
+		break;
+	default:
+		/* SES doesn't do the SGPIO blink settings */
+		return -EINVAL;
+	}
+	return ses_set_page2_descriptor(edev, ecomp, desc);
+}
+
+static void ses_get_device_power(struct enclosure_device *edev,
+			   struct enclosure_component *ecomp)
+{
+	unsigned char *desc;
+
+	desc = ses_get_page2_descriptor(edev, ecomp);
+	if (desc)
+		ecomp->device_power = (desc[3] & 0x10) ? 0 : 1;
+}
+
+static int ses_set_device_power(struct enclosure_device *edev,
+			  struct enclosure_component *ecomp,
+			  enum enclosure_component_setting val)
+{
+	unsigned char desc[4] = {0 };
+
+	ses_read_page2_descriptor(edev, ecomp, desc);
+	switch (val) {
+	case ENCLOSURE_SETTING_DISABLED:
+		/* one is disabled */
+		desc[3] |= 0x10;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[2] = 0x02;
+		desc[3] &= ~0x10;
 		break;
 	default:
 		/* SES doesn't do the SGPIO blink settings */
@@ -234,13 +288,15 @@ static int ses_set_active(struct enclosu
 {
 	unsigned char desc[4] = {0 };
 
+	ses_read_page2_descriptor(edev, ecomp, desc);
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
 		/* zero is disabled */
+		desc[2] &= ~0x80;
 		ecomp->active = 0;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[2] = 0x80;
+		desc[2] |= 0x80;
 		ecomp->active = 1;
 		break;
 	default:
@@ -256,6 +312,8 @@ static struct enclosure_component_callba
 	.get_status		= ses_get_status,
 	.get_locate		= ses_get_locate,
 	.set_locate		= ses_set_locate,
+	.get_device_power	= ses_get_device_power,
+	.set_device_power	= ses_set_device_power,
 	.set_active		= ses_set_active,
 };
 
@@ -264,25 +322,6 @@ struct ses_host_edev {
 	struct enclosure_device *edev;
 };
 
-#if 0
-int ses_match_host(struct enclosure_device *edev, void *data)
-{
-	struct ses_host_edev *sed = data;
-	struct scsi_device *sdev;
-
-	if (!scsi_is_sdev_device(edev->edev.parent))
-		return 0;
-
-	sdev = to_scsi_device(edev->edev.parent);
-
-	if (sdev->host != sed->shost)
-		return 0;
-
-	sed->edev = edev;
-	return 1;
-}
-#endif  /*  0  */
-
 static void ses_process_descriptor(struct enclosure_component *ecomp,
 				   unsigned char *desc)
 {

Re: [RFC] enclosure & ses: modernize and add target power management

[Douglas Gilbert]

On 11-11-11 11:14 AM, Mark Salyzyn wrote:
> The enclosed code change suggestion is proposed to modernize the
> enclosure drivers. The changes outlined are but only compile&
> checkpatch tested so do not propagate. We will be testing this on a real
> enclosure... Inspection is invited. We would however like the answer to
> a question (with two parts) and a few others:
>
> 1) Is /sys/.../enclosure_device:#/device_power an acceptable node name
> for switching the power on and off for the target?
> 2) Do you think that target power management be made part of the Linux
> power management hierarchy (and clues/pointers to how would be
> nice :-) )
> 3) Add other enclosure bits (eg: Lock&  LED control) while we are here?
> 4) Should the enclosure_component retained values be changed to
> bit-fields?
> 5) Does Evolution work better than Outlook for delivering patch content?
>
> Coincident repair of enclosure and ses device support. Fix problems with
> setting enclosure values careful to preserve other settings. Corrected
> fault setting as well. Modernize code. Add support for Device Power
> Management.

I agree that the code was pretty fragile and needs cleaning up.

> Future: Add target device power-cycle to error recovery escalation,
> between target reset and host bus adapter reset (has to be added to each
> lib<transport>  handler, difficult to visualize in error-handler common
> code :-( ) ?

I wonder about the approach of the ses/enclosure driver
that uses element descriptors as unique labels for overall
and individual element instances. I have an LSI SAS-2
expander (Intel RES2SV240) with a built in SES device
that does exactly that.

However I have output from a "XYRATEX HB-2435"
enclosure where almost all of the array instances
have element descriptors which are empty strings.
Yet things like a power supply element instance
have an element descriptor like this:
"TP=9C;SN=xxx;F1=0311;VR=03;VC=yyy;PN=zzz;"
Is that an abnormal case?
SES-3 says nothing about element descriptors apart
from being ASCII strings. Is it reasonable to assume
they are unique labels?


Anyway I have been doing some work on sg_ses again
(re-jigging its indexing for a second time). My
latest is in the sg3_utils beta on this page:
    http://sg.danny.cz/sg/

With my equipment, setting "device off" has no
effect. [What does "device off" do on your test
hardware?] Since my test setup has unique element
descriptors I can do this:
   sg_ses -D ArrayDevice05 -S devoff /dev/sg3

Alternatively an element index and individual element
index can be used to address that slot:
   sg_ses -I 0,5 -S devoff /dev/sg3


To your question about target power management, the
kernel ses driver is probably better placed than a
user space utility like sg_ses. Yet there are several
ways of target (and related) power management. There
is the Power condition mode page in SCSI which recent
disks support (and the older START STOP UNIT command).
In the SAS (transport) domain there are also power
saving techniques, for example the expander phys facing
the target disk can be put into slumber state (and I
presume the target disk would go into a low power
state in response to that).

Doug Gilbert

RE: [RFC] enclosure & ses: modernize and add target power management

[Mark Salyzyn]

Partially absorbed your response.

Douglas Gilbert [mailto:dgilbert@interlog.com] sez:
Sent: Friday, November 11, 2011 4:33 PM

> With my equipment, setting "device off" has no effect.
> [What does "device off" do on your test hardware?]

I have yet to integrate and unit test the kernel-side <grin>, but our
PMC-based expanders and Xyratex Firmware in Xyratex enclosures does in
fact power the targets off (using raw) via the sg tools. What would be
nice to know is how do we determine if the device off bit works, do you
see the bits coming back with the reflected state change, or do the raw
bits on page2 remain in the power-off disabled (power still on) state?
Xyratex has supported target power management for a long time, only way
to truly keep the breakers from tripping in large installations when
someone hits the big red switch on the front.

Hopefully so regarding the feedback on the bit, then we can 'see' the
success or failure.

> To your question about target power management, the
> kernel ses driver is probably better placed than a
> user space utility like sg_ses. Yet there are several
> ways of target (and related) power management. There
> is the Power condition mode page in SCSI which recent
> disks support (and the older START STOP UNIT command).
> In the SAS (transport) domain there are also power
> saving techniques, for example the expander phys facing
> the target disk can be put into slumber state (and I
> presume the target disk would go into a low power
> state in response to that).

Hmmm, this also means I should consider checking the ses page for the
ability to turn the power (back) on for a specified target (catch22)
associated with the Start command? Same tactics for taking a drive in
'green' mode to full-speed?

I have two reactions to this associated with the dip into the water
regarding ses (goes along with all the voices in my head):

1) If you built it, they will come, my 'idea' as-is and not specifically
architected for the future but with an unsupported ifdef around the
sysfs-visible node for testing purposes only. Later someone will figure
out how to add this to the power and spin-up management system in Linux
that makes sense. The net result may be to move the sysfs-visible
drive_power node somewhere else? This follows the ideal of Linux is
Evolutionary. In this case, split into two patches (a) to fix the
existing problems and (b) to add power control.

2) Take some time to explore and create the future, and see if power
states can be created at the target-level, where power condition,
start-stop and enclosure power levers are all coalesced into a single
kobj to manage. This sounds like some additional work in the scsi
layers... This follows the ideal of Linux is Intelligent Design.

Sincerely -- Mark Salyzyn

[PATCH][SCSI] enclosure & ses: modernize and add target power management

[Mark Salyzyn]

Coincident repair of enclosure and ses device support. Fix problems with setting enclosure values careful to preserve other settings. Corrected fault setting as well. Modernize code. Add support for Device Power Management via a new r/w device_power node in sysfs and some internal interfaces to permit them to be called in the future by error recovery escalation. Enclosures that do not support individual power management will always return a status that the device has power.

The controversial adjustment in this patch is the support in the ses driver to punch-through error recovery, a specification requirement. Debate could be that scsi_error.c needs to export a scsi command queueing interface; also a controversial subject ...

Future: Add target device power-cycle to error recovery escalation before considering hba reset (tested in a libsas skunkwork on a 2.6.32 vintage kernel)

Signed-off-by: Mark Salyzyn <mark_salyzyn@xyratex.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>

 drivers/misc/enclosure.c  |   62 ++++++++-
 drivers/scsi/ses.c        |  293 ++++++++++++++++++++++++++++++++++++++++++----
 include/linux/enclosure.h |    8 +
 3 files changed, 329 insertions(+), 34 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 00e5fca..287903b 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -27,7 +27,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
-#include <linux/slab.h>
 
 static LIST_HEAD(container_list);
 static DEFINE_MUTEX(container_list_lock);
@@ -420,10 +419,10 @@ static ssize_t set_component_fault(struct device *cdev,
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
+	unsigned long val;
 
-	if (edev->cb->set_fault)
-		edev->cb->set_fault(edev, ecomp, val);
+	if (!kstrtoul(buf, 0, &val) && edev->cb->set_fault)
+		edev->cb->set_fault(edev, ecomp, (int)val);
 	return count;
 }
 
@@ -478,10 +477,10 @@ static ssize_t set_component_active(struct device *cdev,
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
+	unsigned long val;
 
-	if (edev->cb->set_active)
-		edev->cb->set_active(edev, ecomp, val);
+	if (!kstrtoul(buf, 0, &val) && edev->cb->set_active)
+		edev->cb->set_active(edev, ecomp, (int)val);
 	return count;
 }
 
@@ -502,13 +501,53 @@ static ssize_t set_component_locate(struct device *cdev,
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
+	unsigned long val;
 
-	if (edev->cb->set_locate)
-		edev->cb->set_locate(edev, ecomp, val);
+	if (!kstrtoul(buf, 0, &val) && edev->cb->set_locate)
+		edev->cb->set_locate(edev, ecomp, (int)val);
 	return count;
 }
 
+int enclosure_get_device_power(struct device *cdev)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+	if (edev->cb->get_device_power)
+		edev->cb->get_device_power(edev, ecomp);
+	return ecomp->device_power;
+}
+EXPORT_SYMBOL_GPL(enclosure_get_device_power);
+
+void enclosure_set_device_power(struct device *cdev, int val)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+	if (edev->cb->set_device_power)
+		edev->cb->set_device_power(edev, ecomp, val);
+}
+EXPORT_SYMBOL_GPL(enclosure_set_device_power);
+
+#ifdef CONFIG_EXPERIMENTAL
+static ssize_t get_component_device_power(struct device *cdev,
+				    struct device_attribute *attr, char *buf)
+{
+	return snprintf(buf, 40, "%d\n", enclosure_get_device_power(cdev));
+}
+
+static ssize_t set_component_device_power(struct device *cdev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (!kstrtoul(buf, 0, &val))
+		enclosure_set_device_power(cdev, (int)val);
+	return count;
+}
+#endif
+
 static ssize_t get_component_type(struct device *cdev,
 				  struct device_attribute *attr, char *buf)
 {
@@ -526,6 +565,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(device_power, S_IRUGO | S_IWUSR, get_component_device_power,
+		   set_component_device_power);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
 
 static struct attribute *enclosure_component_attrs[] = {
@@ -533,6 +574,7 @@ static struct attribute *enclosure_component_attrs[] = {
 	&dev_attr_status.attr,
 	&dev_attr_active.attr,
 	&dev_attr_locate.attr,
+	&dev_attr_device_power.attr,
 	&dev_attr_type.attr,
 	NULL
 };
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index eba183c..27bc0ef 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -21,7 +21,6 @@
 **-----------------------------------------------------------------------------
 */
 
-#include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/enclosure.h>
@@ -32,6 +31,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_eh.h>
 
 struct ses_device {
 	unsigned char *page1;
@@ -67,6 +67,196 @@ static int ses_probe(struct device *dev)
 #define SES_TIMEOUT (30 * HZ)
 #define SES_RETRIES 3
 
+/* Copy-n-Paste from scsi_error.c, names changed to protect the innocent */
+
+/**
+ * ses_scsi_eh_done - Completion function for error handling.
+ * @scmd:	Cmd that is done.
+ */
+static void ses_scsi_eh_done(struct scsi_cmnd *scmd)
+{
+	struct completion     *eh_action;
+
+	eh_action = scmd->device->host->eh_action;
+	if (eh_action)
+		complete(eh_action);
+}
+
+/**
+ * ses_scsi_eh_completed_normally - Disposition a eh cmd on return from LLD.
+ * @scmd:	SCSI cmd to examine.
+ *
+ * Notes:
+ *    This is *only* called when we are examining the status of commands
+ *    queued during error recovery.  the main difference here is that we
+ *    don't allow for the possibility of retries here, and we are a lot
+ *    more restrictive about what we consider acceptable.
+ */
+static int ses_scsi_eh_completed_normally(struct scsi_cmnd *scmd)
+{
+	/*
+	 * first check the host byte, to see if there is anything in there
+	 * that would indicate what we need to do.
+	 */
+	if (host_byte(scmd->result) == DID_RESET)
+		return SUCCESS;
+	if (host_byte(scmd->result) != DID_OK)
+		return FAILED;
+
+	/*
+	 * next, check the message byte.
+	 */
+	if (msg_byte(scmd->result) != COMMAND_COMPLETE)
+		return FAILED;
+
+	/*
+	 * now, check the status byte to see if this indicates
+	 * anything special.
+	 */
+	switch (status_byte(scmd->result)) {
+	case GOOD:
+	case COMMAND_TERMINATED:
+		return SUCCESS;
+	case CHECK_CONDITION:
+		return SUCCESS;
+	case CONDITION_GOOD:
+	case INTERMEDIATE_GOOD:
+	case INTERMEDIATE_C_GOOD:
+		/*
+		 * who knows?  FIXME(eric)
+		 */
+		return SUCCESS;
+	case RESERVATION_CONFLICT:
+		if (scmd->cmnd[0] == TEST_UNIT_READY)
+			/* it is a success, we probed the device and
+			 * found it */
+			return SUCCESS;
+		/* otherwise, we failed to send the command */
+		return FAILED;
+	case QUEUE_FULL:
+		/* fall through */
+	case BUSY:
+		return NEEDS_RETRY;
+	default:
+		return FAILED;
+	}
+	return FAILED;
+}
+
+/**
+ * ses_scsi_send_eh_cmnd  - submit a scsi command as part of error recory
+ * @scmd:       SCSI command structure to hijack
+ * @cmnd:       CDB to send
+ * @cmnd_size:  size in bytes of @cmnd
+ * @timeout:    timeout for this request
+ * @sense_bytes: size of sense data to copy or 0
+ *
+ * This function is used to send a scsi command down to a target device
+ * as part of the error recovery process. See also scsi_eh_prep_cmnd() above.
+ *
+ * Return value:
+ *    SUCCESS or FAILED or NEEDS_RETRY
+ */
+static int ses_scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
+			     int cmnd_size, int timeout, unsigned sense_bytes)
+{
+	struct scsi_device *sdev = scmd->device;
+	struct Scsi_Host *shost = sdev->host;
+	DECLARE_COMPLETION_ONSTACK(done);
+	unsigned long timeleft;
+	unsigned long flags;
+	struct scsi_eh_save ses;
+	int rtn;
+
+	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
+	shost->eh_action = &done;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	shost->hostt->queuecommand(scmd, ses_scsi_eh_done);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	timeleft = wait_for_completion_timeout(&done, timeout);
+
+	shost->eh_action = NULL;
+
+	/*
+	 * If there is time left ses_scsi_eh_done got called, and we will
+	 * examine the actual status codes to see whether the command
+	 * actually did complete normally, else tell the host to forget
+	 * about this command.
+	 */
+	if (timeleft) {
+		rtn = ses_scsi_eh_completed_normally(scmd);
+
+		switch (rtn) {
+		case SUCCESS:
+		case NEEDS_RETRY:
+		case FAILED:
+		case TARGET_ERROR:
+			break;
+		case ADD_TO_MLQUEUE:
+			rtn = NEEDS_RETRY;
+			break;
+		default:
+			rtn = FAILED;
+			break;
+		}
+	} else {
+		/* WASS */
+		rtn = TARGET_ERROR;
+	}
+
+	scsi_eh_restore_cmnd(scmd, &ses);
+	return rtn;
+}
+
+/*
+ * Function:	ses_scsi_execute_req
+ *
+ * Purpose:	Send requested reset to a bus or device at any phase.
+ *
+ * Arguments:	device	- device to send reset to
+ *		flag - reset type (see scsi.h)
+ *
+ * Returns:	SUCCESS/FAILURE.
+ *
+ * Notes:	This is used by the SCSI Generic driver to provide
+ *		Bus/Device reset capability.
+ */
+int
+ses_scsi_execute_req(struct scsi_device *sdev, unsigned char *cmd,
+		     int data_direction, void *buffer, unsigned bufflen)
+{
+	struct scsi_cmnd *scmd;
+	struct request req;
+	int result = DRIVER_ERROR << 24;
+
+	blk_rq_init(NULL, &req);
+	req.cmd_len = COMMAND_SIZE(cmd[0]);
+	memcpy(req.cmd, cmd, req.cmd_len);
+	req.sense = NULL;
+	req.sense_len = 0;
+	req.retries = SES_RETRIES;
+	req.timeout = SES_TIMEOUT;
+	req.cmd_type = REQ_TYPE_BLOCK_PC;
+	req.cmd_flags = REQ_QUIET | REQ_PREEMPT;
+	if (bufflen && blk_rq_map_kern(sdev->request_queue, &req,
+				buffer, bufflen, __GFP_WAIT))
+		return result;
+
+	scmd = scsi_get_command(sdev, GFP_KERNEL);
+	scmd->request = &req;
+	scmd->cmnd = req.cmd;
+	scmd->scsi_done = ses_scsi_eh_done;
+	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+	scmd->cmd_len = 6;
+	scmd->sc_data_direction = data_direction;
+
+	result = ses_scsi_send_eh_cmnd(scmd, cmd, 6, SES_TIMEOUT, 0);
+
+	return result;
+}
+
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 			 void *buf, int bufflen)
 {
@@ -79,6 +269,13 @@ static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 		0
 	};
 
+	/* Allow command to punch-through host error recovery? */
+	if (scsi_host_in_recovery(sdev->host) && !list_empty(&sdev->cmd_list))
+		return DRIVER_SOFT << 24;
+
+	if (scsi_host_in_recovery(sdev->host))
+		return ses_scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE,
+					buf, bufflen);
 	return scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
 				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
 }
@@ -97,7 +294,16 @@ static int ses_send_diag(struct scsi_device *sdev, int page_code,
 		0
 	};
 
-	result = scsi_execute_req(sdev, cmd, DMA_TO_DEVICE, buf, bufflen,
+	/* Allow command to punch-through host error recovery? */
+	if (scsi_host_in_recovery(sdev->host) && !list_empty(&sdev->cmd_list))
+		return DRIVER_SOFT << 24;
+
+	if (scsi_host_in_recovery(sdev->host))
+		result = ses_scsi_execute_req(sdev, cmd, DMA_TO_DEVICE,
+				  buf, bufflen);
+	else
+		result = scsi_execute_req(sdev, cmd, DMA_TO_DEVICE,
+				  buf, bufflen,
 				  NULL, SES_TIMEOUT, SES_RETRIES, NULL);
 	if (result)
 		sdev_printk(KERN_ERR, sdev, "SEND DIAGNOSTIC result: %8x\n",
@@ -174,18 +380,38 @@ static void ses_get_fault(struct enclosure_device *edev,
 		ecomp->fault = (desc[3] & 0x60) >> 4;
 }
 
+static void ses_read_page2_descriptor(struct enclosure_device *edev,
+				      struct enclosure_component *ecomp,
+				      unsigned char *desc)
+{
+	unsigned char *desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+	if (desc_ptr)
+		memcpy(desc, desc_ptr, 4);
+	/* preserve active state */
+	if (ecomp->active)
+		desc[2] |= 0x80;
+	else
+		desc[2] &= ~0x80;
+	/* Clear all reserved/alternate-purpose bits */
+	desc[2] &= 0xE7;
+	desc[3] &= 0x3C;
+}
+
 static int ses_set_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			 enum enclosure_component_setting val)
 {
 	unsigned char desc[4] = {0 };
 
+	ses_read_page2_descriptor(edev, ecomp, desc);
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
 		/* zero is disabled */
+		desc[3] &= ~0x20;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[3] = 0x20;
+		desc[3] |= 0x20;
 		break;
 	default:
 		/* SES doesn't do the SGPIO blink settings */
@@ -221,12 +447,46 @@ static int ses_set_locate(struct enclosure_device *edev,
 {
 	unsigned char desc[4] = {0 };
 
+	ses_read_page2_descriptor(edev, ecomp, desc);
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
 		/* zero is disabled */
+		desc[2] &= ~0x02;
+		break;
+	case ENCLOSURE_SETTING_ENABLED:
+		desc[2] |= 0x02;
+		break;
+	default:
+		/* SES doesn't do the SGPIO blink settings */
+		return -EINVAL;
+	}
+	return ses_set_page2_descriptor(edev, ecomp, desc);
+}
+
+static void ses_get_device_power(struct enclosure_device *edev,
+			   struct enclosure_component *ecomp)
+{
+	unsigned char *desc;
+
+	desc = ses_get_page2_descriptor(edev, ecomp);
+	if (desc)
+		ecomp->device_power = (desc[3] & 0x10) ? 0 : 1;
+}
+
+static int ses_set_device_power(struct enclosure_device *edev,
+			  struct enclosure_component *ecomp,
+			  enum enclosure_component_setting val)
+{
+	unsigned char desc[4] = {0 };
+
+	ses_read_page2_descriptor(edev, ecomp, desc);
+	switch (val) {
+	case ENCLOSURE_SETTING_DISABLED:
+		/* one is disabled */
+		desc[3] |= 0x10;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[2] = 0x02;
+		desc[3] &= ~0x10;
 		break;
 	default:
 		/* SES doesn't do the SGPIO blink settings */
@@ -241,13 +501,15 @@ static int ses_set_active(struct enclosure_device *edev,
 {
 	unsigned char desc[4] = {0 };
 
+	ses_read_page2_descriptor(edev, ecomp, desc);
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
 		/* zero is disabled */
+		desc[2] &= ~0x80;
 		ecomp->active = 0;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[2] = 0x80;
+		desc[2] |= 0x80;
 		ecomp->active = 1;
 		break;
 	default:
@@ -263,6 +525,8 @@ static struct enclosure_component_callbacks ses_enclosure_callbacks = {
 	.get_status		= ses_get_status,
 	.get_locate		= ses_get_locate,
 	.set_locate		= ses_set_locate,
+	.get_device_power	= ses_get_device_power,
+	.set_device_power	= ses_set_device_power,
 	.set_active		= ses_set_active,
 };
 
@@ -271,25 +535,6 @@ struct ses_host_edev {
 	struct enclosure_device *edev;
 };
 
-#if 0
-int ses_match_host(struct enclosure_device *edev, void *data)
-{
-	struct ses_host_edev *sed = data;
-	struct scsi_device *sdev;
-
-	if (!scsi_is_sdev_device(edev->edev.parent))
-		return 0;
-
-	sdev = to_scsi_device(edev->edev.parent);
-
-	if (sdev->host != sed->shost)
-		return 0;
-
-	sed->edev = edev;
-	return 1;
-}
-#endif  /*  0  */
-
 static void ses_process_descriptor(struct enclosure_component *ecomp,
 				   unsigned char *desc)
 {
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 9a33c5f..46f4482 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_device_power)(struct enclosure_device *,
+			   struct enclosure_component *);
+	int (*set_device_power)(struct enclosure_device *,
+			  struct enclosure_component *,
+			  enum enclosure_component_setting);
 };
 
 
@@ -91,6 +96,7 @@ struct enclosure_component {
 	int fault;
 	int active;
 	int locate;
+	int device_power;
 	enum enclosure_status status;
 };
 
@@ -129,5 +135,7 @@ struct enclosure_device *enclosure_find(struct device *dev,
 					struct enclosure_device *start);
 int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
 			      void *data);
+int enclosure_get_device_power(struct device *cdev);
+void enclosure_set_device_power(struct device *cdev, int val);
 
 #endif /* _LINUX_ENCLOSURE_H_ */

Re: [PATCH][SCSI] enclosure & ses: modernize and add target power management

[James Bottomley]

On Thu, 2012-01-12 at 12:03 -0500, Mark Salyzyn wrote:
> Coincident repair of enclosure and ses device support. Fix problems
> with setting enclosure values careful to preserve other settings.
> Corrected fault setting as well. Modernize code. Add support for
> Device Power Management via a new r/w device_power node in sysfs and
> some internal interfaces to permit them to be called in the future by
> error recovery escalation. Enclosures that do not support individual
> power management will always return a status that the device has
> power.
> 
> The controversial adjustment in this patch is the support in the ses
> driver to punch-through error recovery, a specification requirement.
> Debate could be that scsi_error.c needs to export a scsi command
> queueing interface; also a controversial subject ...

Hmm, it is controversial, but I see where it's required.  It would be
nice to export the eh interfaces for this rather than clone them,
though.

> Future: Add target device power-cycle to error recovery escalation
> before considering hba reset (tested in a libsas skunkwork on a 2.6.32
> vintage kernel)

Did you actually test this?  It won't compile for me without the
following patch because of the lack of slab.h include and the argument
problems in ->queuecommand.

James

---

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 287903b..06978f8 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -27,6 +27,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/slab.h>
 
 static LIST_HEAD(container_list);
 static DEFINE_MUTEX(container_list_lock);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 27bc0ef..c31427b 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -170,9 +170,10 @@ static int ses_scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
 	shost->eh_action = &done;
+	scmd->scsi_done = ses_scsi_eh_done;
 
 	spin_lock_irqsave(shost->host_lock, flags);
-	shost->hostt->queuecommand(scmd, ses_scsi_eh_done);
+	shost->hostt->queuecommand(shost, scmd);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	timeleft = wait_for_completion_timeout(&done, timeout);

[PATCH][SCSI] enclosure & ses: modernize and add target power management (take 2)

[Mark Salyzyn]

Coincident repair of enclosure and ses device support. Fix problems with setting enclosure values careful to preserve other settings. Corrected fault setting as well. Modernize code. Add support for Device Power Management via a new r/w device_power node in sysfs and some internal interfaces to permit them to be called in the future by error recovery escalation. Enclosures that do not support individual power management will always return a status that the device has power.
 
This patch has been adjusted to deal with review comments. The controversial adjustment in this patch is the support in the ses driver to punch-through error recovery, a specification requirement. scsi_error.c added an export for a scsi command queueing interface. This command scsi_execute_eh_req(struct scsi_device *sdev, unsigned char *cmd, int data_direction, void *buffer, unsigned buffoon) uses hard-coded timeouts.

Future: Add target device power-cycle to error recovery escalation before considering hba reset (back-port tested in a libsas skunkwork on a 2.6.32 vintage kernel, this patch compile tested with scsi-misc-2.6)

Signed-off-by: Mark Salyzyn <mark_salyzyn@xyratex.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>

 drivers/misc/enclosure.c  |   61 +++++++++++++++++++++++----
 drivers/scsi/scsi_error.c |   49 +++++++++++++++++++++
 drivers/scsi/ses.c        |  103 +++++++++++++++++++++++++++++++++++-----------
 include/linux/enclosure.h |    8 +++
 include/scsi/scsi_eh.h    |    3 +
 5 files changed, 191 insertions(+), 33 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 00e5fca..06978f8 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -420,10 +420,10 @@ static ssize_t set_component_fault(struct device *cdev,
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
+	unsigned long val;
 
-	if (edev->cb->set_fault)
-		edev->cb->set_fault(edev, ecomp, val);
+	if (!kstrtoul(buf, 0, &val) && edev->cb->set_fault)
+		edev->cb->set_fault(edev, ecomp, (int)val);
 	return count;
 }
 
@@ -478,10 +478,10 @@ static ssize_t set_component_active(struct device *cdev,
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
+	unsigned long val;
 
-	if (edev->cb->set_active)
-		edev->cb->set_active(edev, ecomp, val);
+	if (!kstrtoul(buf, 0, &val) && edev->cb->set_active)
+		edev->cb->set_active(edev, ecomp, (int)val);
 	return count;
 }
 
@@ -502,13 +502,53 @@ static ssize_t set_component_locate(struct device *cdev,
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
+	unsigned long val;
 
-	if (edev->cb->set_locate)
-		edev->cb->set_locate(edev, ecomp, val);
+	if (!kstrtoul(buf, 0, &val) && edev->cb->set_locate)
+		edev->cb->set_locate(edev, ecomp, (int)val);
 	return count;
 }
 
+int enclosure_get_device_power(struct device *cdev)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+	if (edev->cb->get_device_power)
+		edev->cb->get_device_power(edev, ecomp);
+	return ecomp->device_power;
+}
+EXPORT_SYMBOL_GPL(enclosure_get_device_power);
+
+void enclosure_set_device_power(struct device *cdev, int val)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+	if (edev->cb->set_device_power)
+		edev->cb->set_device_power(edev, ecomp, val);
+}
+EXPORT_SYMBOL_GPL(enclosure_set_device_power);
+
+#ifdef CONFIG_EXPERIMENTAL
+static ssize_t get_component_device_power(struct device *cdev,
+				    struct device_attribute *attr, char *buf)
+{
+	return snprintf(buf, 40, "%d\n", enclosure_get_device_power(cdev));
+}
+
+static ssize_t set_component_device_power(struct device *cdev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (!kstrtoul(buf, 0, &val))
+		enclosure_set_device_power(cdev, (int)val);
+	return count;
+}
+#endif
+
 static ssize_t get_component_type(struct device *cdev,
 				  struct device_attribute *attr, char *buf)
 {
@@ -526,6 +566,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(device_power, S_IRUGO | S_IWUSR, get_component_device_power,
+		   set_component_device_power);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
 
 static struct attribute *enclosure_component_attrs[] = {
@@ -533,6 +575,7 @@ static struct attribute *enclosure_component_attrs[] = {
 	&dev_attr_status.attr,
 	&dev_attr_active.attr,
 	&dev_attr_locate.attr,
+	&dev_attr_device_power.attr,
 	&dev_attr_type.attr,
 	NULL
 };
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 2cfcbff..3153609 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -842,6 +842,55 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 }
 
 /**
+ * scsi_execute_eh_req  - submit a scsi command to bypass error recovery.
+ * @scmd:       SCSI command structure to hijack
+ * @cmnd:       CDB to send
+ * @cmnd_size:  size in bytes of @cmnd
+ * @timeout:    timeout for this request
+ * @sense_bytes: size of sense data to copy or 0
+ *
+ * This function is used to send a scsi command down to a target device
+ * to bypass error recovery process.
+ *
+ * Return value:
+ *    SUCCESS or FAILED or NEEDS_RETRY
+ */
+int
+scsi_execute_eh_req(struct scsi_device *sdev, unsigned char *cmd,
+		    int data_direction, void *buffer, unsigned bufflen)
+{
+	struct scsi_cmnd *scmd;
+	struct request req;
+	int result = DRIVER_ERROR << 24;
+
+	blk_rq_init(NULL, &req);
+	req.cmd_len = COMMAND_SIZE(cmd[0]);
+	memcpy(req.cmd, cmd, req.cmd_len);
+	req.sense = NULL;
+	req.sense_len = 0;
+	req.retries = 3;
+	req.timeout = 30 * HZ;
+	req.cmd_type = REQ_TYPE_BLOCK_PC;
+	req.cmd_flags = REQ_QUIET | REQ_PREEMPT;
+	if (bufflen && blk_rq_map_kern(sdev->request_queue, &req,
+				buffer, bufflen, __GFP_WAIT))
+		return result;
+
+	scmd = scsi_get_command(sdev, GFP_KERNEL);
+	scmd->request = &req;
+	scmd->cmnd = req.cmd;
+	scmd->scsi_done = scsi_eh_done;
+	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+	scmd->cmd_len = 6;
+	scmd->sc_data_direction = data_direction;
+
+	result = scsi_send_eh_cmnd(scmd, cmd, 6, 30 * HZ, 0);
+
+	return result;
+}
+EXPORT_SYMBOL_GPL(scsi_execute_eh_req);
+
+/**
  * scsi_request_sense - Request sense data from a particular target.
  * @scmd:	SCSI cmd for request sense.
  *
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index eba183c..2fa62e9 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -21,7 +21,6 @@
 **-----------------------------------------------------------------------------
 */
 
-#include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/enclosure.h>
@@ -32,6 +31,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_eh.h>
 
 struct ses_device {
 	unsigned char *page1;
@@ -79,6 +79,13 @@ static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 		0
 	};
 
+	/* Allow command to punch-through host error recovery? */
+	if (scsi_host_in_recovery(sdev->host) && !list_empty(&sdev->cmd_list))
+		return DRIVER_SOFT << 24;
+
+	if (scsi_host_in_recovery(sdev->host))
+		return scsi_execute_eh_req(sdev, cmd, DMA_FROM_DEVICE,
+					buf, bufflen);
 	return scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
 				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
 }
@@ -97,7 +104,16 @@ static int ses_send_diag(struct scsi_device *sdev, int page_code,
 		0
 	};
 
-	result = scsi_execute_req(sdev, cmd, DMA_TO_DEVICE, buf, bufflen,
+	/* Allow command to punch-through host error recovery? */
+	if (scsi_host_in_recovery(sdev->host) && !list_empty(&sdev->cmd_list))
+		return DRIVER_SOFT << 24;
+
+	if (scsi_host_in_recovery(sdev->host))
+		result = scsi_execute_eh_req(sdev, cmd, DMA_TO_DEVICE,
+				  buf, bufflen);
+	else
+		result = scsi_execute_req(sdev, cmd, DMA_TO_DEVICE,
+				  buf, bufflen,
 				  NULL, SES_TIMEOUT, SES_RETRIES, NULL);
 	if (result)
 		sdev_printk(KERN_ERR, sdev, "SEND DIAGNOSTIC result: %8x\n",
@@ -174,18 +190,38 @@ static void ses_get_fault(struct enclosure_device *edev,
 		ecomp->fault = (desc[3] & 0x60) >> 4;
 }
 
+static void ses_read_page2_descriptor(struct enclosure_device *edev,
+				      struct enclosure_component *ecomp,
+				      unsigned char *desc)
+{
+	unsigned char *desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+	if (desc_ptr)
+		memcpy(desc, desc_ptr, 4);
+	/* preserve active state */
+	if (ecomp->active)
+		desc[2] |= 0x80;
+	else
+		desc[2] &= ~0x80;
+	/* Clear all reserved/alternate-purpose bits */
+	desc[2] &= 0xE7;
+	desc[3] &= 0x3C;
+}
+
 static int ses_set_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			 enum enclosure_component_setting val)
 {
 	unsigned char desc[4] = {0 };
 
+	ses_read_page2_descriptor(edev, ecomp, desc);
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
 		/* zero is disabled */
+		desc[3] &= ~0x20;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[3] = 0x20;
+		desc[3] |= 0x20;
 		break;
 	default:
 		/* SES doesn't do the SGPIO blink settings */
@@ -221,12 +257,46 @@ static int ses_set_locate(struct enclosure_device *edev,
 {
 	unsigned char desc[4] = {0 };
 
+	ses_read_page2_descriptor(edev, ecomp, desc);
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
 		/* zero is disabled */
+		desc[2] &= ~0x02;
+		break;
+	case ENCLOSURE_SETTING_ENABLED:
+		desc[2] |= 0x02;
+		break;
+	default:
+		/* SES doesn't do the SGPIO blink settings */
+		return -EINVAL;
+	}
+	return ses_set_page2_descriptor(edev, ecomp, desc);
+}
+
+static void ses_get_device_power(struct enclosure_device *edev,
+			   struct enclosure_component *ecomp)
+{
+	unsigned char *desc;
+
+	desc = ses_get_page2_descriptor(edev, ecomp);
+	if (desc)
+		ecomp->device_power = (desc[3] & 0x10) ? 0 : 1;
+}
+
+static int ses_set_device_power(struct enclosure_device *edev,
+			  struct enclosure_component *ecomp,
+			  enum enclosure_component_setting val)
+{
+	unsigned char desc[4] = {0 };
+
+	ses_read_page2_descriptor(edev, ecomp, desc);
+	switch (val) {
+	case ENCLOSURE_SETTING_DISABLED:
+		/* one is disabled */
+		desc[3] |= 0x10;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[2] = 0x02;
+		desc[3] &= ~0x10;
 		break;
 	default:
 		/* SES doesn't do the SGPIO blink settings */
@@ -241,13 +311,15 @@ static int ses_set_active(struct enclosure_device *edev,
 {
 	unsigned char desc[4] = {0 };
 
+	ses_read_page2_descriptor(edev, ecomp, desc);
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
 		/* zero is disabled */
+		desc[2] &= ~0x80;
 		ecomp->active = 0;
 		break;
 	case ENCLOSURE_SETTING_ENABLED:
-		desc[2] = 0x80;
+		desc[2] |= 0x80;
 		ecomp->active = 1;
 		break;
 	default:
@@ -263,6 +335,8 @@ static struct enclosure_component_callbacks ses_enclosure_callbacks = {
 	.get_status		= ses_get_status,
 	.get_locate		= ses_get_locate,
 	.set_locate		= ses_set_locate,
+	.get_device_power	= ses_get_device_power,
+	.set_device_power	= ses_set_device_power,
 	.set_active		= ses_set_active,
 };
 
@@ -271,25 +345,6 @@ struct ses_host_edev {
 	struct enclosure_device *edev;
 };
 
-#if 0
-int ses_match_host(struct enclosure_device *edev, void *data)
-{
-	struct ses_host_edev *sed = data;
-	struct scsi_device *sdev;
-
-	if (!scsi_is_sdev_device(edev->edev.parent))
-		return 0;
-
-	sdev = to_scsi_device(edev->edev.parent);
-
-	if (sdev->host != sed->shost)
-		return 0;
-
-	sed->edev = edev;
-	return 1;
-}
-#endif  /*  0  */
-
 static void ses_process_descriptor(struct enclosure_component *ecomp,
 				   unsigned char *desc)
 {
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 9a33c5f..46f4482 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_device_power)(struct enclosure_device *,
+			   struct enclosure_component *);
+	int (*set_device_power)(struct enclosure_device *,
+			  struct enclosure_component *,
+			  enum enclosure_component_setting);
 };
 
 
@@ -91,6 +96,7 @@ struct enclosure_component {
 	int fault;
 	int active;
 	int locate;
+	int device_power;
 	enum enclosure_status status;
 };
 
@@ -129,5 +135,7 @@ struct enclosure_device *enclosure_find(struct device *dev,
 					struct enclosure_device *start);
 int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
 			      void *data);
+int enclosure_get_device_power(struct device *cdev);
+void enclosure_set_device_power(struct device *cdev, int val);
 
 #endif /* _LINUX_ENCLOSURE_H_ */
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 06a8790..cac0809 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -92,4 +92,7 @@ extern void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
 extern void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
 		struct scsi_eh_save *ses);
 
+extern int scsi_execute_eh_req(struct scsi_device *sdev, unsigned char *cmd,
+		    int data_direction, void *buffer, unsigned bufflen);
+
 #endif /* _SCSI_SCSI_EH_H */

Re: [PATCH][SCSI] enclosure & ses: modernize and add target power management (take 2)

[Douglas Gilbert]

On 12-02-22 02:09 PM, Mark Salyzyn wrote:
> Coincident repair of enclosure and ses device support. Fix problems with setting enclosure values careful to preserve other settings. Corrected fault setting as well. Modernize code. Add support for Device Power Management via a new r/w device_power node in sysfs and some internal interfaces to permit them to be called in the future by error recovery escalation. Enclosures that do not support individual power management will always return a status that the device has power.

Mark,
One problem I found with the existing kernel SES/enclosure
support is that it assumes there is a well-formed
Element Descriptor diagnostic page. By well-formed I mean that
the page exists and there is a unique, sensible entry for each
element. By sensible I mean _not_ like this:
   "TP=9C;SN=PMW82562000C39A;F1=0311;VR=03;VC=6B58AD13;PN=0082562-11;"

I have a SAS-2 expander with an associated SES device that breaks
all those tidy assumptions (i.e. I did not make up that string).
I assume that your patches do not change the reliance on a
well-formed Element Descriptor diagnostic page. Is the peculiar
SES device I have (and most SES-1 devices which don't have
Element Descriptor diagnostic pages) worth worrying about?

Doug Gilbert

Re: [PATCH][SCSI] enclosure & ses: modernize and add target power management (take 2)

[Mark Salyzyn]

Nah :-( I did not fix any of that. My tests were performed on a compliant enclosure, and a smattering of others (all from the same vendor, so hardly a large group). The aim was to be more compliant with the specification in the ses module, and the fixes were for the 'write' portion, the 'read' portion worked before on the enclosures I tested.

>From the standpoint of the additional of power management, the lack of the unique entry likely means it does not have individual power control either.

Yes it is worth worrying about, ident and slot mapping is a useful feature, but this is an issue for the vendor (End Of Life notwithstanding) to update their enclosure firmware to be compliant. Fixing the ses module for non-compliant enclosures that are EOL and unlikely to be repaired, if possible, should be in the hands of those that have the enclosures to test and a vested interest ... it has been my experience that one only should do so carefully, ever seen the hacks in an x86 BIOS to deal with decades of legacy? :-)

Sincerely -- Mark Salyzyn

On Feb 22, 2012, at 3:12 PM, Douglas Gilbert wrote:

> On 12-02-22 02:09 PM, Mark Salyzyn wrote:
>> Coincident repair of enclosure and ses device support. Fix problems with setting enclosure values careful to preserve other settings. Corrected fault setting as well. Modernize code. Add support for Device Power Management via a new r/w device_power node in sysfs and some internal interfaces to permit them to be called in the future by error recovery escalation. Enclosures that do not support individual power management will always return a status that the device has power.
> 
> Mark,
> One problem I found with the existing kernel SES/enclosure
> support is that it assumes there is a well-formed
> Element Descriptor diagnostic page. By well-formed I mean that
> the page exists and there is a unique, sensible entry for each
> element. By sensible I mean _not_ like this:
>  "TP=9C;SN=PMW82562000C39A;F1=0311;VR=03;VC=6B58AD13;PN=0082562-11;"
> 
> I have a SAS-2 expander with an associated SES device that breaks
> all those tidy assumptions (i.e. I did not make up that string).
> I assume that your patches do not change the reliance on a
> well-formed Element Descriptor diagnostic page. Is the peculiar
> SES device I have (and most SES-1 devices which don't have
> Element Descriptor diagnostic pages) worth worrying about?
> 
> Doug Gilbert

[PATCH][SCSI] panic within ses.ko during insmod

[Mark Salyzyn]

[-- Attachment #1: Type: text/plain, Size: 5023 bytes --]

Rarely, ses.ko load while scanning was taking place resulted in a panic. Discovered that the panic occurred while the inquiry field for a scsi device was NULL and an unprotected call to scsi_device_enclosure() occurred. Suggest that the inline function scsi_device_enclosure be modified, but for this panic, we can address this specific issue as outlined at the bottom of this patch submission.

device BUG: unable to handle kernel NULL pointer dereference at 0000000000000006
IP: [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
PGD 80e354067 PUD 805f3c067 PMD 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/devices/pci0000:00/0000:00:1d.0/usb1/idVendor
CPU 0
Modules linked in: ses(+) enclosure mlx4_ib ib_mad ib_core mlx4_en mlx4_core ext4 mbcache jbd2 usb_storage sd_mod crc_t10dif ahci isci libsas mpt2sas scsi_transport_sas raid_class dm_mod

Modules linked in: ses(+) enclosure mlx4_ib ib_mad ib_core mlx4_en mlx4_core ext4 mbcache jbd2 usb_storage sd_mod crc_t10dif ahci isci libsas mpt2sas scsi_transport_sas raid_class dm_mod
Pid: 1757, comm: modprobe Tainted: G        W  ----------------
RIP: 0010:[<ffffffffa00230f1>]  [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
RSP: 0018:ffff8807ed73be38  EFLAGS: 00010246
RAX: ffff8808081dd000 RBX: ffff880809d59000 RCX: 000000000000007f
RDX: 0000000000000000 RSI: ffffffff812654a0 RDI: 0000000000000000
RBP: ffff8807ed73be98 R08: ffffffff81bfdec8 R09: 0000000000000040
R10: 0000000000000000 R11: 0000000000000026 R12: ffff8808053259a0
R13: 0000000000000530 R14: ffff88080b640000 R15: ffff8808051c8540
FS:  00007f337c0cd700(0000) GS:ffff880047e00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000006 CR3: 0000000807403000 CR4: 00000000000406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process modprobe (pid: 1757, threadinfo ffff8807ed73a000, task ffff88080c542040)
Stack:
 ffff8807ed73be78 ffffffff814be88f ffff8808081d0000 ffff880809d59358
<0> ffff880809d59138 ffff88080dfb5000 0000000000000000 ffffffffa0023660
<0> ffffffff81b00fe0 ffff8807ed73bea8 0000000000000000 0000000000000000

Call Trace:
 [<ffffffff814be88f>] ? klist_next+0x7f/0xf0
 [<ffffffff813383b9>] class_interface_register+0xa9/0xe0
 [<ffffffffa00fe000>] ? ses_init+0x0/0x3c [ses]
 [<ffffffff81357f16>] scsi_register_interface+0x16/0x20
 [<ffffffffa00fe014>] ses_init+0x14/0x3c [ses]
 [<ffffffff8100204c>] do_one_initcall+0x3c/0x1d0
 [<ffffffff810aca7f>] sys_init_module+0xdf/0x250
 [<ffffffff8100b172>] system_call_fastpath+0x16/0x1b
Code: 32 e1 48 85 c0 75 13 eb 51 90 48 8b 3b 48 89 c6 e8 45 6d 32 e1 48 85 c0 74 40 8b b8 84 00 00 00 85 ff 75 e6 48 8b 90 a8 00 00 00 <f6> 42 06 40 75 d9 48 89 c6 4c 89 f7 48 89 45 b0 e8 aa fa ff ff
RIP  [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
 RSP <ffff8807ed73be38>
CR2: 0000000000000006
---[ end trace 766a927d5bc5e5f3 ]---
Kernel panic - not syncing: Fatal exception
Pid: 1757, comm: modprobe Tainted: G      D W  ---------------- 
Call Trace:
 [<ffffffff814d65a8>] ? panic+0x78/0x143
 [<ffffffff814da5f4>] ? oops_end+0xe4/0x100
 [<ffffffff81040c9b>] ? no_context+0xfb/0x260
 [<ffffffff81040f25>] ? __bad_area_nosemaphore+0x125/0x1e0
 [<ffffffff8104104e>] ? bad_area+0x4e/0x60
 [<ffffffff81041773>] ? __do_page_fault+0x3c3/0x480
 [<ffffffff811e745c>] ? sysfs_add_one+0x2c/0x130
 [<ffffffff811e7988>] ? sysfs_do_create_link+0xd8/0x170
 [<ffffffff811e7a53>] ? sysfs_create_link+0x13/0x20
 [<ffffffffa00e2617>] ? enclosure_add_device+0x137/0x160 [enclosure]
 [<ffffffff814dc5de>] ? do_page_fault+0x3e/0xa0
 [<ffffffff814d9965>] ? page_fault+0x25/0x30
 [<ffffffff812654a0>] ? kobject_release+0x0/0x240
 [<ffffffffa00230f1>] ? ses_intf_add+0x2f1/0x5e0 [ses]
 [<ffffffffa0023115>] ? ses_intf_add+0x315/0x5e0 [ses]
 [<ffffffff814be88f>] ? klist_next+0x7f/0xf0
 [<ffffffff813383b9>] ? class_interface_register+0xa9/0xe0
 [<ffffffffa00fe000>] ? ses_init+0x0/0x3c [ses]
 [<ffffffff81357f16>] ? scsi_register_interface+0x16/0x20
 [<ffffffffa00fe014>] ? ses_init+0x14/0x3c [ses]
 [<ffffffff8100204c>] ? do_one_initcall+0x3c/0x1d0
 [<ffffffff810aca7f>] ? sys_init_module+0xdf/0x250
 [<ffffffff8100b172>] ? system_call_fastpath+0x16/0x1b

Signed-off-by: Mark Salyzyn <Mark_Salyzyn@xyratex.com>

 drivers/scsi/ses.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index eba183c..0e69cf9 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -628,7 +628,9 @@ static int ses_intf_add(struct device *cdev,
 	/* see if there are any devices matching before
 	 * we found the enclosure */
 	shost_for_each_device(tmp_sdev, sdev->host) {
-		if (tmp_sdev->lun != 0 || scsi_device_enclosure(tmp_sdev))
+		if (tmp_sdev->lun != 0
+		 || tmp_sdev->inquiry != NULL
+		 || scsi_device_enclosure(tmp_sdev))
 			continue;
 		ses_match_to_enclosure(edev, tmp_sdev);
 	}


[-- Attachment #2: ses-panic.patch --]
[-- Type: application/octet-stream, Size: 4922 bytes --]

ses.ko load while scanning was taking place resulted in a panic. Discovered
that the panic occurred while the inquiry field for a scsi device was NULL
and an unprotected call to scsi_device_enclosure() occurred. Suggest that
the inline function scsi_device_enclosure be modified, but for this panic, we
can address this specific issue as outlined at the bottom of this
patch submission.

device BUG: unable to handle kernel NULL pointer dereference at 0000000000000006
IP: [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
PGD 80e354067 PUD 805f3c067 PMD 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/devices/pci0000:00/0000:00:1d.0/usb1/idVendor
CPU 0
Modules linked in: ses(+) enclosure mlx4_ib ib_mad ib_core mlx4_en mlx4_core ext4 mbcache jbd2 usb_storage sd_mod crc_t10dif ahci isci libsas mpt2sas scsi_transport_sas raid_class dm_mod

Modules linked in: ses(+) enclosure mlx4_ib ib_mad ib_core mlx4_en mlx4_core ext4 mbcache jbd2 usb_storage sd_mod crc_t10dif ahci isci libsas mpt2sas scsi_transport_sas raid_class dm_mod
Pid: 1757, comm: modprobe Tainted: G        W  ----------------
RIP: 0010:[<ffffffffa00230f1>]  [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
RSP: 0018:ffff8807ed73be38  EFLAGS: 00010246
RAX: ffff8808081dd000 RBX: ffff880809d59000 RCX: 000000000000007f
RDX: 0000000000000000 RSI: ffffffff812654a0 RDI: 0000000000000000
RBP: ffff8807ed73be98 R08: ffffffff81bfdec8 R09: 0000000000000040
R10: 0000000000000000 R11: 0000000000000026 R12: ffff8808053259a0
R13: 0000000000000530 R14: ffff88080b640000 R15: ffff8808051c8540
FS:  00007f337c0cd700(0000) GS:ffff880047e00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000006 CR3: 0000000807403000 CR4: 00000000000406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process modprobe (pid: 1757, threadinfo ffff8807ed73a000, task ffff88080c542040)
Stack:
 ffff8807ed73be78 ffffffff814be88f ffff8808081d0000 ffff880809d59358
<0> ffff880809d59138 ffff88080dfb5000 0000000000000000 ffffffffa0023660
<0> ffffffff81b00fe0 ffff8807ed73bea8 0000000000000000 0000000000000000

Call Trace:
 [<ffffffff814be88f>] ? klist_next+0x7f/0xf0
 [<ffffffff813383b9>] class_interface_register+0xa9/0xe0
 [<ffffffffa00fe000>] ? ses_init+0x0/0x3c [ses]
 [<ffffffff81357f16>] scsi_register_interface+0x16/0x20
 [<ffffffffa00fe014>] ses_init+0x14/0x3c [ses]
 [<ffffffff8100204c>] do_one_initcall+0x3c/0x1d0
 [<ffffffff810aca7f>] sys_init_module+0xdf/0x250
 [<ffffffff8100b172>] system_call_fastpath+0x16/0x1b
Code: 32 e1 48 85 c0 75 13 eb 51 90 48 8b 3b 48 89 c6 e8 45 6d 32 e1 48 85 c0 74 40 8b b8 84 00 00 00 85 ff 75 e6 48 8b 90 a8 00 00 00 <f6> 42 06 40 75 d9 48 89 c6 4c 89 f7 48 89 45 b0 e8 aa fa ff ff
RIP  [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
 RSP <ffff8807ed73be38>
CR2: 0000000000000006
---[ end trace 766a927d5bc5e5f3 ]---
Kernel panic - not syncing: Fatal exception
Pid: 1757, comm: modprobe Tainted: G      D W  ---------------- 
Call Trace:
 [<ffffffff814d65a8>] ? panic+0x78/0x143
 [<ffffffff814da5f4>] ? oops_end+0xe4/0x100
 [<ffffffff81040c9b>] ? no_context+0xfb/0x260
 [<ffffffff81040f25>] ? __bad_area_nosemaphore+0x125/0x1e0
 [<ffffffff8104104e>] ? bad_area+0x4e/0x60
 [<ffffffff81041773>] ? __do_page_fault+0x3c3/0x480
 [<ffffffff811e745c>] ? sysfs_add_one+0x2c/0x130
 [<ffffffff811e7988>] ? sysfs_do_create_link+0xd8/0x170
 [<ffffffff811e7a53>] ? sysfs_create_link+0x13/0x20
 [<ffffffffa00e2617>] ? enclosure_add_device+0x137/0x160 [enclosure]
 [<ffffffff814dc5de>] ? do_page_fault+0x3e/0xa0
 [<ffffffff814d9965>] ? page_fault+0x25/0x30
 [<ffffffff812654a0>] ? kobject_release+0x0/0x240
 [<ffffffffa00230f1>] ? ses_intf_add+0x2f1/0x5e0 [ses]
 [<ffffffffa0023115>] ? ses_intf_add+0x315/0x5e0 [ses]
 [<ffffffff814be88f>] ? klist_next+0x7f/0xf0
 [<ffffffff813383b9>] ? class_interface_register+0xa9/0xe0
 [<ffffffffa00fe000>] ? ses_init+0x0/0x3c [ses]
 [<ffffffff81357f16>] ? scsi_register_interface+0x16/0x20
 [<ffffffffa00fe014>] ? ses_init+0x14/0x3c [ses]
 [<ffffffff8100204c>] ? do_one_initcall+0x3c/0x1d0
 [<ffffffff810aca7f>] ? sys_init_module+0xdf/0x250
 [<ffffffff8100b172>] ? system_call_fastpath+0x16/0x1b

Signed-off-by: Mark Salyzyn <Mark_Salyzyn@xyratex.com>

 drivers/scsi/ses.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index eba183c..0e69cf9 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -628,7 +628,9 @@ static int ses_intf_add(struct device *cdev,
 	/* see if there are any devices matching before
 	 * we found the enclosure */
 	shost_for_each_device(tmp_sdev, sdev->host) {
-		if (tmp_sdev->lun != 0 || scsi_device_enclosure(tmp_sdev))
+		if (tmp_sdev->lun != 0
+		 || tmp_sdev->inquiry != NULL
+		 || scsi_device_enclosure(tmp_sdev))
 			continue;
 		ses_match_to_enclosure(edev, tmp_sdev);
 	}

[PATCH][SCSI] panic within ses.ko during insmod (take 2)

[Mark Salyzyn]

[-- Attachment #1: Type: text/plain, Size: 5136 bytes --]

Ooops, condition check was wrong, had to reverse <sigh> thanks for a fellow Xyratex Employee pointing this out.

Rarely, ses.ko load while scanning was taking place resulted in a panic. Discovered that the panic occurred while the inquiry field for a scsi device was NULL and an unprotected call to scsi_device_enclosure() occurred. Suggest that the inline function scsi_device_enclosure be modified, but for this panic, we can address this specific issue as outlined at the bottom of this patch submission.

device BUG: unable to handle kernel NULL pointer dereference at 0000000000000006
IP: [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
PGD 80e354067 PUD 805f3c067 PMD 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/devices/pci0000:00/0000:00:1d.0/usb1/idVendor
CPU 0
Modules linked in: ses(+) enclosure mlx4_ib ib_mad ib_core mlx4_en mlx4_core ext4 mbcache jbd2 usb_storage sd_mod crc_t10dif ahci isci libsas mpt2sas scsi_transport_sas raid_class dm_mod

Modules linked in: ses(+) enclosure mlx4_ib ib_mad ib_core mlx4_en mlx4_core ext4 mbcache jbd2 usb_storage sd_mod crc_t10dif ahci isci libsas mpt2sas scsi_transport_sas raid_class dm_mod
Pid: 1757, comm: modprobe Tainted: G        W  ----------------
RIP: 0010:[<ffffffffa00230f1>]  [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
RSP: 0018:ffff8807ed73be38  EFLAGS: 00010246
RAX: ffff8808081dd000 RBX: ffff880809d59000 RCX: 000000000000007f
RDX: 0000000000000000 RSI: ffffffff812654a0 RDI: 0000000000000000
RBP: ffff8807ed73be98 R08: ffffffff81bfdec8 R09: 0000000000000040
R10: 0000000000000000 R11: 0000000000000026 R12: ffff8808053259a0
R13: 0000000000000530 R14: ffff88080b640000 R15: ffff8808051c8540
FS:  00007f337c0cd700(0000) GS:ffff880047e00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000006 CR3: 0000000807403000 CR4: 00000000000406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process modprobe (pid: 1757, threadinfo ffff8807ed73a000, task ffff88080c542040)
Stack:
 ffff8807ed73be78 ffffffff814be88f ffff8808081d0000 ffff880809d59358
<0> ffff880809d59138 ffff88080dfb5000 0000000000000000 ffffffffa0023660
<0> ffffffff81b00fe0 ffff8807ed73bea8 0000000000000000 0000000000000000

Call Trace:
 [<ffffffff814be88f>] ? klist_next+0x7f/0xf0
 [<ffffffff813383b9>] class_interface_register+0xa9/0xe0
 [<ffffffffa00fe000>] ? ses_init+0x0/0x3c [ses]
 [<ffffffff81357f16>] scsi_register_interface+0x16/0x20
 [<ffffffffa00fe014>] ses_init+0x14/0x3c [ses]
 [<ffffffff8100204c>] do_one_initcall+0x3c/0x1d0
 [<ffffffff810aca7f>] sys_init_module+0xdf/0x250
 [<ffffffff8100b172>] system_call_fastpath+0x16/0x1b
Code: 32 e1 48 85 c0 75 13 eb 51 90 48 8b 3b 48 89 c6 e8 45 6d 32 e1 48 85 c0 74 40 8b b8 84 00 00 00 85 ff 75 e6 48 8b 90 a8 00 00 00 <f6> 42 06 40 75 d9 48 89 c6 4c 89 f7 48 89 45 b0 e8 aa fa ff ff
RIP  [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
 RSP <ffff8807ed73be38>
CR2: 0000000000000006
---[ end trace 766a927d5bc5e5f3 ]---
Kernel panic - not syncing: Fatal exception
Pid: 1757, comm: modprobe Tainted: G      D W  ---------------- 
Call Trace:
 [<ffffffff814d65a8>] ? panic+0x78/0x143
 [<ffffffff814da5f4>] ? oops_end+0xe4/0x100
 [<ffffffff81040c9b>] ? no_context+0xfb/0x260
 [<ffffffff81040f25>] ? __bad_area_nosemaphore+0x125/0x1e0
 [<ffffffff8104104e>] ? bad_area+0x4e/0x60
 [<ffffffff81041773>] ? __do_page_fault+0x3c3/0x480
 [<ffffffff811e745c>] ? sysfs_add_one+0x2c/0x130
 [<ffffffff811e7988>] ? sysfs_do_create_link+0xd8/0x170
 [<ffffffff811e7a53>] ? sysfs_create_link+0x13/0x20
 [<ffffffffa00e2617>] ? enclosure_add_device+0x137/0x160 [enclosure]
 [<ffffffff814dc5de>] ? do_page_fault+0x3e/0xa0
 [<ffffffff814d9965>] ? page_fault+0x25/0x30
 [<ffffffff812654a0>] ? kobject_release+0x0/0x240
 [<ffffffffa00230f1>] ? ses_intf_add+0x2f1/0x5e0 [ses]
 [<ffffffffa0023115>] ? ses_intf_add+0x315/0x5e0 [ses]
 [<ffffffff814be88f>] ? klist_next+0x7f/0xf0
 [<ffffffff813383b9>] ? class_interface_register+0xa9/0xe0
 [<ffffffffa00fe000>] ? ses_init+0x0/0x3c [ses]
 [<ffffffff81357f16>] ? scsi_register_interface+0x16/0x20
 [<ffffffffa00fe014>] ? ses_init+0x14/0x3c [ses]
 [<ffffffff8100204c>] ? do_one_initcall+0x3c/0x1d0
 [<ffffffff810aca7f>] ? sys_init_module+0xdf/0x250
 [<ffffffff8100b172>] ? system_call_fastpath+0x16/0x1b

Signed-off-by: Mark Salyzyn <Mark_Salyzyn@xyratex.com>

 drivers/scsi/ses.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index eba183c..0e69cf9 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -628,7 +628,9 @@ static int ses_intf_add(struct device *cdev,
 	/* see if there are any devices matching before
 	 * we found the enclosure */
 	shost_for_each_device(tmp_sdev, sdev->host) {
-		if (tmp_sdev->lun != 0 || scsi_device_enclosure(tmp_sdev))
+		if (tmp_sdev->lun != 0
+		 || tmp_sdev->inquiry == NULL
+		 || scsi_device_enclosure(tmp_sdev))
 			continue;
 		ses_match_to_enclosure(edev, tmp_sdev);
 	}

[-- Attachment #2: ses-panic-2.patch --]
[-- Type: application/octet-stream, Size: 4930 bytes --]

Rarely, ses.ko load while scanning was taking place resulted in a panic.
Discovered that the panic occurred while the inquiry field for a scsi device
was NULL and an unprotected call to scsi_device_enclosure() occurred. Suggest
that the inline function scsi_device_enclosure be modified, but for this panic,
we can address this specific issue as outlined at the bottom of this patch
submission.

device BUG: unable to handle kernel NULL pointer dereference at 0000000000000006
IP: [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
PGD 80e354067 PUD 805f3c067 PMD 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/devices/pci0000:00/0000:00:1d.0/usb1/idVendor
CPU 0
Modules linked in: ses(+) enclosure mlx4_ib ib_mad ib_core mlx4_en mlx4_core ext4 mbcache jbd2 usb_storage sd_mod crc_t10dif ahci isci libsas mpt2sas scsi_transport_sas raid_class dm_mod

Modules linked in: ses(+) enclosure mlx4_ib ib_mad ib_core mlx4_en mlx4_core ext4 mbcache jbd2 usb_storage sd_mod crc_t10dif ahci isci libsas mpt2sas scsi_transport_sas raid_class dm_mod
Pid: 1757, comm: modprobe Tainted: G        W  ----------------
RIP: 0010:[<ffffffffa00230f1>]  [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
RSP: 0018:ffff8807ed73be38  EFLAGS: 00010246
RAX: ffff8808081dd000 RBX: ffff880809d59000 RCX: 000000000000007f
RDX: 0000000000000000 RSI: ffffffff812654a0 RDI: 0000000000000000
RBP: ffff8807ed73be98 R08: ffffffff81bfdec8 R09: 0000000000000040
R10: 0000000000000000 R11: 0000000000000026 R12: ffff8808053259a0
R13: 0000000000000530 R14: ffff88080b640000 R15: ffff8808051c8540
FS:  00007f337c0cd700(0000) GS:ffff880047e00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000006 CR3: 0000000807403000 CR4: 00000000000406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process modprobe (pid: 1757, threadinfo ffff8807ed73a000, task ffff88080c542040)
Stack:
 ffff8807ed73be78 ffffffff814be88f ffff8808081d0000 ffff880809d59358
<0> ffff880809d59138 ffff88080dfb5000 0000000000000000 ffffffffa0023660
<0> ffffffff81b00fe0 ffff8807ed73bea8 0000000000000000 0000000000000000

Call Trace:
 [<ffffffff814be88f>] ? klist_next+0x7f/0xf0
 [<ffffffff813383b9>] class_interface_register+0xa9/0xe0
 [<ffffffffa00fe000>] ? ses_init+0x0/0x3c [ses]
 [<ffffffff81357f16>] scsi_register_interface+0x16/0x20
 [<ffffffffa00fe014>] ses_init+0x14/0x3c [ses]
 [<ffffffff8100204c>] do_one_initcall+0x3c/0x1d0
 [<ffffffff810aca7f>] sys_init_module+0xdf/0x250
 [<ffffffff8100b172>] system_call_fastpath+0x16/0x1b
Code: 32 e1 48 85 c0 75 13 eb 51 90 48 8b 3b 48 89 c6 e8 45 6d 32 e1 48 85 c0 74 40 8b b8 84 00 00 00 85 ff 75 e6 48 8b 90 a8 00 00 00 <f6> 42 06 40 75 d9 48 89 c6 4c 89 f7 48 89 45 b0 e8 aa fa ff ff
RIP  [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
 RSP <ffff8807ed73be38>
CR2: 0000000000000006
---[ end trace 766a927d5bc5e5f3 ]---
Kernel panic - not syncing: Fatal exception
Pid: 1757, comm: modprobe Tainted: G      D W  ---------------- 
Call Trace:
 [<ffffffff814d65a8>] ? panic+0x78/0x143
 [<ffffffff814da5f4>] ? oops_end+0xe4/0x100
 [<ffffffff81040c9b>] ? no_context+0xfb/0x260
 [<ffffffff81040f25>] ? __bad_area_nosemaphore+0x125/0x1e0
 [<ffffffff8104104e>] ? bad_area+0x4e/0x60
 [<ffffffff81041773>] ? __do_page_fault+0x3c3/0x480
 [<ffffffff811e745c>] ? sysfs_add_one+0x2c/0x130
 [<ffffffff811e7988>] ? sysfs_do_create_link+0xd8/0x170
 [<ffffffff811e7a53>] ? sysfs_create_link+0x13/0x20
 [<ffffffffa00e2617>] ? enclosure_add_device+0x137/0x160 [enclosure]
 [<ffffffff814dc5de>] ? do_page_fault+0x3e/0xa0
 [<ffffffff814d9965>] ? page_fault+0x25/0x30
 [<ffffffff812654a0>] ? kobject_release+0x0/0x240
 [<ffffffffa00230f1>] ? ses_intf_add+0x2f1/0x5e0 [ses]
 [<ffffffffa0023115>] ? ses_intf_add+0x315/0x5e0 [ses]
 [<ffffffff814be88f>] ? klist_next+0x7f/0xf0
 [<ffffffff813383b9>] ? class_interface_register+0xa9/0xe0
 [<ffffffffa00fe000>] ? ses_init+0x0/0x3c [ses]
 [<ffffffff81357f16>] ? scsi_register_interface+0x16/0x20
 [<ffffffffa00fe014>] ? ses_init+0x14/0x3c [ses]
 [<ffffffff8100204c>] ? do_one_initcall+0x3c/0x1d0
 [<ffffffff810aca7f>] ? sys_init_module+0xdf/0x250
 [<ffffffff8100b172>] ? system_call_fastpath+0x16/0x1b

Signed-off-by: Mark Salyzyn <Mark_Salyzyn@xyratex.com>

 drivers/scsi/ses.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index eba183c..0e69cf9 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -628,7 +628,9 @@ static int ses_intf_add(struct device *cdev,
 	/* see if there are any devices matching before
 	 * we found the enclosure */
 	shost_for_each_device(tmp_sdev, sdev->host) {
-		if (tmp_sdev->lun != 0 || scsi_device_enclosure(tmp_sdev))
+		if (tmp_sdev->lun != 0
+		 || tmp_sdev->inquiry == NULL
+		 || scsi_device_enclosure(tmp_sdev))
 			continue;
 		ses_match_to_enclosure(edev, tmp_sdev);
 	}

Re: [PATCH][SCSI] panic within ses.ko during insmod

[Dan Williams]

On Thu, May 10, 2012 at 1:48 PM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote:
> Rarely, ses.ko load while scanning was taking place resulted in a panic. Discovered that the panic occurred while the inquiry field for a scsi device was NULL and an unprotected call to scsi_device_enclosure() occurred. Suggest that the inline function scsi_device_enclosure be modified, but for this panic, we can address this specific issue as outlined at the bottom of this patch submission.
>
> device BUG: unable to handle kernel NULL pointer dereference at 0000000000000006
> IP: [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]

Wasn't this fixed by:

commit d1e12de804f9d8ad114786ca7c2ce593cba79891
Author: Krishnasamy, Somasundaram <Somasundaram.Krishnasamy@lsi.com>
Date:   Mon Feb 28 18:13:22 2011 -0500

    [SCSI] ses: Avoid kernel panic when lun 0 is not mapped

    During device discovery, scsi mid layer sends INQUIRY command to LUN
    0. If the LUN 0 is not mapped to host, it creates a temporary
    scsi_device with LUN id 0 and sends REPORT_LUNS command to it. After
    the REPORT_LUNS succeeds, it walks through the LUN table and adds each
    LUN found to sysfs. At the end of REPORT_LUNS lun table scan, it will
    delete the temporary scsi_device of LUN 0.

    When scsi devices are added to sysfs, it calls add_dev function of all
    the registered class interfaces. If ses driver has been registered,
    ses_intf_add() of ses module will be called. This function calls
    scsi_device_enclosure() to check the inquiry data for EncServ
    bit. Since inquiry was not allocated for temporary LUN 0 scsi_device,
    it will cause NULL pointer exception.

    To fix the problem, sdev->inquiry is checked for NULL before reading it.

    Signed-off-by: Somasundaram Krishnasamy <Somasundaram.Krishnasamy@lsi.com>
    Signed-off-by: Babu Moger <babu.moger@lsi.com>
    Cc: stable@kernel.org
    Signed-off-by: James Bottomley <James.Bottomley@suse.de>

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index f171c65..2d3ec50 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -462,7 +462,7 @@ static inline int scsi_device_qas(struct scsi_device *sdev)
 }
 static inline int scsi_device_enclosure(struct scsi_device *sdev)
 {
-       return sdev->inquiry[6] & (1<<6);
+       return sdev->inquiry ? (sdev->inquiry[6] & (1<<6)) : 1;
 }

 static inline int scsi_device_protection(struct scsi_device *sdev)

Re: [PATCH][SCSI] panic within ses.ko during insmod

[Mark Salyzyn]

It appears so, I did a 100% fresh pull of the tree, and the fix in scsi_device.h is there.

NAK this patch

Sincerely -- Mark Salyzyn

On May 11, 2012, at 12:08 PM, Dan Williams wrote:

> On Thu, May 10, 2012 at 1:48 PM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote:
>> Rarely, ses.ko load while scanning was taking place resulted in a panic. Discovered that the panic occurred while the inquiry field for a scsi device was NULL and an unprotected call to scsi_device_enclosure() occurred. Suggest that the inline function scsi_device_enclosure be modified, but for this panic, we can address this specific issue as outlined at the bottom of this patch submission.
>> 
>> device BUG: unable to handle kernel NULL pointer dereference at 0000000000000006
>> IP: [<ffffffffa00230f1>] ses_intf_add+0x2f1/0x5e0 [ses]
> 
> Wasn't this fixed by:
> 
> commit d1e12de804f9d8ad114786ca7c2ce593cba79891
> Author: Krishnasamy, Somasundaram <Somasundaram.Krishnasamy@lsi.com>
> Date:   Mon Feb 28 18:13:22 2011 -0500
> 
>    [SCSI] ses: Avoid kernel panic when lun 0 is not mapped
> 
>    During device discovery, scsi mid layer sends INQUIRY command to LUN
>    0. If the LUN 0 is not mapped to host, it creates a temporary
>    scsi_device with LUN id 0 and sends REPORT_LUNS command to it. After
>    the REPORT_LUNS succeeds, it walks through the LUN table and adds each
>    LUN found to sysfs. At the end of REPORT_LUNS lun table scan, it will
>    delete the temporary scsi_device of LUN 0.
> 
>    When scsi devices are added to sysfs, it calls add_dev function of all
>    the registered class interfaces. If ses driver has been registered,
>    ses_intf_add() of ses module will be called. This function calls
>    scsi_device_enclosure() to check the inquiry data for EncServ
>    bit. Since inquiry was not allocated for temporary LUN 0 scsi_device,
>    it will cause NULL pointer exception.
> 
>    To fix the problem, sdev->inquiry is checked for NULL before reading it.
> 
>    Signed-off-by: Somasundaram Krishnasamy <Somasundaram.Krishnasamy@lsi.com>
>    Signed-off-by: Babu Moger <babu.moger@lsi.com>
>    Cc: stable@kernel.org
>    Signed-off-by: James Bottomley <James.Bottomley@suse.de>
> 
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index f171c65..2d3ec50 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -462,7 +462,7 @@ static inline int scsi_device_qas(struct scsi_device *sdev)
> }
> static inline int scsi_device_enclosure(struct scsi_device *sdev)
> {
> -       return sdev->inquiry[6] & (1<<6);
> +       return sdev->inquiry ? (sdev->inquiry[6] & (1<<6)) : 1;
> }
> 
> static inline int scsi_device_protection(struct scsi_device *sdev)