Skip to contentSkip to conversation

hpsa update

PATCH In review Don Brace 3 participants 11 messages Dec 9, 2015 – Dec 21, 2015

Subsystems (2)
SCSI Hewlett-packard smart array RAID driver (hpsa)
lore
Share & tools
lore

Don Brace

author
#0 lore raw

cover hpsa update

These patches are based on Linus's tree

The changes are:
- correct missing changes from snprintf to scnprintf in path_info_show by Rasmus Villemoes
- fix reported bus for SAS transport devices
- add in enclosure information

---

Don Brace (3):
hpsa: fix path_info_show
hpsa: change SAS transport devices to bus 0.
hpsa: add box and bay information for enclosure devices

 drivers/scsi/hpsa.c     |  118 ++++++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/hpsa.h     |    2 -
 drivers/scsi/hpsa_cmd.h |   13 +++++
 3 files changed, 120 insertions(+), 13 deletions(-)

--
Signature

author
#1 lore raw

PATCH 1/3 hpsa: fix path_info_show

left off some changes from Rasmus Villemoes where he changed snprintf to scnprintf

Suggested-by Rasmus Villemoes <[email protected]>
Reviewed-by Justin Lindley <[email protected]>
Reviewed-by Kevin Barnett <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Signed-off-by Don Brace <[email protected]>
---
 drivers/scsi/hpsa.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/scsi/hpsa.c +6 -6
index a386036..f8370b8 100644
@@ -795,7 +795,7 @@ static ssize_t path_info_show(struct device *dev,
 		if (hdev->external ||
 			hdev->devtype == TYPE_RAID ||
 			is_logical_device(hdev)) {
-			output_len += snprintf(buf + output_len,
+			output_len += scnprintf(buf + output_len,
 						PAGE_SIZE - output_len,
 						"%s\n", active);
 			continue;

@@ -809,28 +809,28 @@ static ssize_t path_info_show(struct device *dev,
 		if (phys_connector[1] < '0')
 			phys_connector[1] = '0';
 		if (hdev->phys_connector[i] > 0)
-			output_len += snprintf(buf + output_len,
+			output_len += scnprintf(buf + output_len,
 				PAGE_SIZE - output_len,
 				"PORT: %.2s ",
 				phys_connector);
 		if (hdev->devtype == TYPE_DISK && hdev->expose_device) {
 			if (box == 0 || box == 0xFF) {
-				output_len += snprintf(buf + output_len,
+				output_len += scnprintf(buf + output_len,
 					PAGE_SIZE - output_len,
 					"BAY: %hhu %s\n",
 					bay, active);
 			} else {
-				output_len += snprintf(buf + output_len,
+				output_len += scnprintf(buf + output_len,
 					PAGE_SIZE - output_len,
 					"BOX: %hhu BAY: %hhu %s\n",
 					box, bay, active);
 			}
 		} else if (box != 0 && box != 0xFF) {
-			output_len += snprintf(buf + output_len,
+			output_len += scnprintf(buf + output_len,
 				PAGE_SIZE - output_len, "BOX: %hhu %s\n",
 				box, active);
 		} else
-			output_len += snprintf(buf + output_len,
+			output_len += scnprintf(buf + output_len,
 				PAGE_SIZE - output_len, "%s\n", active);
 	}
author
#3 lore raw

PATCH 2/3 hpsa: change SAS transport devices to bus 0.

In reply to Don Brace

sas transport places devices on bus 0 but driver was setting the bus to 3.

Reviewed-by Justin Lindley <[email protected]>
Reviewed-by Kevin Barnett <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Signed-off-by Don Brace <[email protected]>
---
 drivers/scsi/hpsa.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/hpsa.h +1 -1
index ae5beda..fdd39fc 100644
@@ -400,7 +400,7 @@ struct offline_device_entry {
 #define HPSA_PHYSICAL_DEVICE_BUS	0
 #define HPSA_RAID_VOLUME_BUS		1
 #define HPSA_EXTERNAL_RAID_VOLUME_BUS	2
-#define HPSA_HBA_BUS			3
+#define HPSA_HBA_BUS			0
 
 /*
 	Send the command to the hardware
#4 lore raw

Re: [PATCH 2/3] hpsa: change SAS transport devices to bus 0.

Quoted 18 lines · Don Brace

sas transport places devices on bus 0 but driver was setting the bus to 3.

Reviewed-by Justin Lindley <[email protected]>
Reviewed-by Kevin Barnett <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Signed-off-by Don Brace <[email protected]>

---
drivers/scsi/hpsa.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/hpsa.h +0 -0
index ae5beda..fdd39fc 100644

drivers/scsi/hpsa.h struct offline_device_entry { · line 403

Quoted 5 lines
drivers/scsi/hpsa.h +1 -1
@@ -400,7 +400,7 @@ struct offline_device_entry {
 #define HPSA_PHYSICAL_DEVICE_BUS	0
 #define HPSA_RAID_VOLUME_BUS		1
 #define HPSA_EXTERNAL_RAID_VOLUME_BUS	2
-#define HPSA_HBA_BUS			3
+#define HPSA_HBA_BUS			0

Is this not the same as using HPSA_PHYSICAL_DEVICE_BUS?

#5 lore raw

Re: [PATCH 2/3] hpsa: change SAS transport devices to bus 0.

Quoted 13 lines · Matthew R. Ochs
Quoted 13 lines · Don Brace

sas transport places devices on bus 0 but driver was setting the bus to 3.

Reviewed-by Justin Lindley <[email protected]>
Reviewed-by Kevin Barnett <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Signed-off-by Don Brace <[email protected]>

---
drivers/scsi/hpsa.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 11 lines

drivers/scsi/hpsa.h struct offline_device_entry { · line 403

Quoted 10 lines
drivers/scsi/hpsa.h +1 -1
index ae5beda..fdd39fc 100644
@@ -400,7 +400,7 @@ struct offline_device_entry {
 #define HPSA_PHYSICAL_DEVICE_BUS	0
 #define HPSA_RAID_VOLUME_BUS		1
 #define HPSA_EXTERNAL_RAID_VOLUME_BUS	2
-#define HPSA_HBA_BUS			3
+#define HPSA_HBA_BUS			0

Is this not the same as using HPSA_PHYSICAL_DEVICE_BUS?

I was just trying to minimize the changes. I can update the driver is necessary.

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Reviewed-by
#6 lore raw

Re: [PATCH 2/3] hpsa: change SAS transport devices to bus 0.

Quoted 27 lines · Don Brace
Quoted 13 lines · Matthew R. Ochs
Quoted 13 lines · Don Brace

sas transport places devices on bus 0 but driver was setting the bus to 3.

Reviewed-by Justin Lindley <[email protected]>
Reviewed-by Kevin Barnett <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Signed-off-by Don Brace <[email protected]>

---
drivers/scsi/hpsa.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 11 lines

drivers/scsi/hpsa.h struct offline_device_entry { · line 403

Quoted 10 lines
drivers/scsi/hpsa.h +1 -1
index ae5beda..fdd39fc 100644
@@ -400,7 +400,7 @@ struct offline_device_entry {
 #define HPSA_PHYSICAL_DEVICE_BUS	0
 #define HPSA_RAID_VOLUME_BUS		1
 #define HPSA_EXTERNAL_RAID_VOLUME_BUS	2
-#define HPSA_HBA_BUS			3
+#define HPSA_HBA_BUS			0

Is this not the same as using HPSA_PHYSICAL_DEVICE_BUS?

Quoted 2 lines

I was just trying to minimize the changes. I can update the driver is necessary.

Understandable.

Reviewed-by Matthew R. Ochs <[email protected]>
author
#7 lore raw

PATCH 3/3 hpsa: add box and bay information for enclosure devices

In reply to Don Brace
Reviewed-by Justin Lindley <[email protected]>
Reviewed-by Kevin Barnett <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Signed-off-by Don Brace <[email protected]>
---
 drivers/scsi/hpsa.c     |  108 ++++++++++++++++++++++++++++++++++++++++++++---
 drivers/scsi/hpsa_cmd.h |   13 ++++++
 2 files changed, 114 insertions(+), 7 deletions(-)
drivers/scsi/hpsa.c +101 -7
index f8370b8..6f84ec7 100644
@@ -750,7 +750,6 @@ static ssize_t host_show_hp_ssd_smart_path_enabled(struct device *dev,
 }
 
 #define MAX_PATHS 8
-
 static ssize_t path_info_show(struct device *dev,
 	     struct device_attribute *attr, char *buf)
 {

@@ -792,9 +791,7 @@ static ssize_t path_info_show(struct device *dev,
 				hdev->bus, hdev->target, hdev->lun,
 				scsi_device_type(hdev->devtype));
 
-		if (hdev->external ||
-			hdev->devtype == TYPE_RAID ||
-			is_logical_device(hdev)) {
+		if (hdev->devtype == TYPE_RAID || is_logical_device(hdev)) {
 			output_len += scnprintf(buf + output_len,
 						PAGE_SIZE - output_len,
 						"%s\n", active);

@@ -808,8 +805,7 @@ static ssize_t path_info_show(struct device *dev,
 			phys_connector[0] = '0';
 		if (phys_connector[1] < '0')
 			phys_connector[1] = '0';
-		if (hdev->phys_connector[i] > 0)
-			output_len += scnprintf(buf + output_len,
+		output_len += scnprintf(buf + output_len,
 				PAGE_SIZE - output_len,
 				"PORT: %.2s ",
 				phys_connector);

@@ -3191,6 +3187,90 @@ out:
 	return rc;
 }
 
+/*
+ * get enclosure information
+ * struct ReportExtendedLUNdata *rlep - Used for BMIC drive number
+ * struct hpsa_scsi_dev_t *encl_dev - device entry for enclosure
+ * Uses id_physical_device to determine the box_index.
+ */
+static int hpsa_get_enclosure_info(struct ctlr_info *h,
+			unsigned char *scsi3addr,
+			struct ReportExtendedLUNdata *rlep, int rle_index,
+			struct hpsa_scsi_dev_t *encl_dev)
+{
+	int rc = -1;
+	struct CommandList *c = NULL;
+	struct ErrorInfo *ei = NULL;
+	struct bmic_sense_storage_box_params *bssbp = NULL;
+	struct bmic_identify_physical_device *id_phys = NULL;
+	struct ext_report_lun_entry *rle = &rlep->LUN[rle_index];
+	u16 bmic_device_index = 0;
+
+
+	bssbp = kzalloc(sizeof(*bssbp), GFP_KERNEL);
+	if (!bssbp)
+		goto out;
+
+	id_phys = kzalloc(sizeof(*id_phys), GFP_KERNEL);
+	if (!id_phys)
+		goto out;
+
+	bmic_device_index = GET_BMIC_DRIVE_NUMBER(&rle->lunid[0]);
+
+	if (bmic_device_index == 0xFF00)
+		goto out;
+
+	memset(id_phys, 0, sizeof(*id_phys));
+	rc = hpsa_bmic_id_physical_device(h, scsi3addr, bmic_device_index,
+						id_phys, sizeof(*id_phys));
+	if (rc) {
+		dev_warn(&h->pdev->dev, "%s: id_phys failed %d bdi[0x%x]\n",
+			__func__, encl_dev->external, bmic_device_index);
+		goto out;
+	}
+
+	c = cmd_alloc(h);
+
+	rc = fill_cmd(c, BMIC_SENSE_STORAGE_BOX_PARAMS, h, bssbp,
+			sizeof(*bssbp), 0, RAID_CTLR_LUNID, TYPE_CMD);
+
+	if (rc)
+		goto out;
+
+	if (id_phys->phys_connector[1] == 'E')
+		c->Request.CDB[5] = id_phys->box_index;
+	else
+		c->Request.CDB[5] = 0;
+
+	rc = hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_FROMDEVICE,
+						NO_TIMEOUT);
+	if (rc)
+		goto out;
+
+	ei = c->err_info;
+	if (ei->CommandStatus != 0 && ei->CommandStatus != CMD_DATA_UNDERRUN) {
+		rc = -1;
+		goto out;
+	}
+
+	encl_dev->box[id_phys->active_path_number] = bssbp->phys_box_on_port;
+	memcpy(&encl_dev->phys_connector[id_phys->active_path_number],
+		bssbp->phys_connector, sizeof(bssbp->phys_connector));
+
+	rc = IO_OK;
+out:
+	kfree(bssbp);
+	kfree(id_phys);
+
+	if (c)
+		cmd_free(h, c);
+
+	if (rc != IO_OK)
+		hpsa_show_dev_msg(KERN_INFO, h, encl_dev,
+			"Error, could not get enclosure information\n");
+	return rc;
+}
+
 static u64 hpsa_get_sas_address_from_report_physical(struct ctlr_info *h,
 						unsigned char *scsi3addr)
 {

@@ -4032,7 +4112,8 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h)
 
 		/* skip masked non-disk devices */
 		if (MASKED_DEVICE(lunaddrbytes) && physical_device &&
-			(physdev_list->LUN[phys_dev_index].device_flags & 0x01))
+		   (physdev_list->LUN[phys_dev_index].device_type != 0x06) &&
+		   (physdev_list->LUN[phys_dev_index].device_flags & 0x01))
 			continue;
 
 		/* Get device type, vendor, model, device id */

@@ -4117,6 +4198,9 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h)
 		case TYPE_TAPE:
 		case TYPE_MEDIUM_CHANGER:
 		case TYPE_ENCLOSURE:
+			hpsa_get_enclosure_info(h, lunaddrbytes,
+						physdev_list, phys_dev_index,
+						this_device);
 			ncurrent++;
 			break;
 		case TYPE_RAID:

@@ -6629,6 +6713,16 @@ static int fill_cmd(struct CommandList *c, u8 cmd, struct ctlr_info *h,
 			c->Request.CDB[7] = (size >> 16) & 0xFF;
 			c->Request.CDB[8] = (size >> 8) & 0XFF;
 			break;
+		case BMIC_SENSE_STORAGE_BOX_PARAMS:
+			c->Request.CDBLen = 10;
+			c->Request.type_attr_dir =
+				TYPE_ATTR_DIR(cmd_type, ATTR_SIMPLE, XFER_READ);
+			c->Request.Timeout = 0;
+			c->Request.CDB[0] = BMIC_READ;
+			c->Request.CDB[6] = BMIC_SENSE_STORAGE_BOX_PARAMS;
+			c->Request.CDB[7] = (size >> 16) & 0xFF;
+			c->Request.CDB[8] = (size >> 8) & 0XFF;
+			break;
 		case BMIC_IDENTIFY_CONTROLLER:
 			c->Request.CDBLen = 10;
 			c->Request.type_attr_dir =
drivers/scsi/hpsa_cmd.h +13 -0
index d92ef0d..6a919ad 100644
@@ -291,6 +291,7 @@ struct SenseSubsystem_info {
 #define BMIC_SENSE_DIAG_OPTIONS 0xF5
 #define HPSA_DIAG_OPTS_DISABLE_RLD_CACHING 0x40000000
 #define BMIC_SENSE_SUBSYSTEM_INFORMATION 0x66
+#define BMIC_SENSE_STORAGE_BOX_PARAMS 0x65
 
 /* Command List Structure */
 union SCSI3Addr {

@@ -842,5 +843,17 @@ struct bmic_sense_subsystem_info {
 	u8	pad[332];
 };
 
+struct bmic_sense_storage_box_params {
+	u8	reserved[36];
+	u8	inquiry_valid;
+	u8	reserved_1[68];
+	u8	phys_box_on_port;
+	u8	reserved_2[22];
+	u16	connection_info;
+	u8	reserver_3[84];
+	u8	phys_connector[2];
+	u8	reserved_4[296];
+};
+
 #pragma pack()
 #endif /* HPSA_CMD_H */
#8 lore raw

Re: [PATCH 3/3] hpsa: add box and bay information for enclosure devices

Quoted 1 line · Don Brace

No commit message?

Quoted 23 lines
Reviewed-by Justin Lindley <[email protected]>
Reviewed-by Kevin Barnett <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Signed-off-by Don Brace <[email protected]>

---
drivers/scsi/hpsa.c | 108 ++++++++++++++++++++++++++++++++++++++++++++---
drivers/scsi/hpsa_cmd.h | 13 ++++++

2 files changed, 114 insertions(+), 7 deletions(-)
drivers/scsi/hpsa.c +0 -1
index f8370b8..6f84ec7 100644
@@ -750,7 +750,6 @@ static ssize_t host_show_hp_ssd_smart_path_enabled(struct device *dev,
 }
 
 #define MAX_PATHS 8
-
 static ssize_t path_info_show(struct device *dev,
 	     struct device_attribute *attr, char *buf)
 {

drivers/scsi/hpsa.c static ssize_t path_info_show(struct device *dev, · line 794

Quoted 7 lines
drivers/scsi/hpsa.c +1 -3
@@ -792,9 +791,7 @@ static ssize_t path_info_show(struct device *dev,
 				hdev->bus, hdev->target, hdev->lun,
 				scsi_device_type(hdev->devtype));
 
-		if (hdev->external ||
-			hdev->devtype == TYPE_RAID ||
-			is_logical_device(hdev)) {
+		if (hdev->devtype == TYPE_RAID || is_logical_device(hdev)) {

How does removing the check for external here relate to the rest of this commit?

drivers/scsi/hpsa.c static ssize_t path_info_show(struct device *dev, · line 808

Quoted 10 lines
drivers/scsi/hpsa.c +1 -2
 			output_len += scnprintf(buf + output_len,
 						PAGE_SIZE - output_len,
 						"%s\n", active);

@@ -808,8 +805,7 @@ static ssize_t path_info_show(struct device *dev,
 			phys_connector[0] = '0';
 		if (phys_connector[1] < '0')
 			phys_connector[1] = '0';
-		if (hdev->phys_connector[i] > 0)
-			output_len += scnprintf(buf + output_len,
+		output_len += scnprintf(buf + output_len,

Same question regarding the removal of the > 0 check.

Quoted 29 lines
drivers/scsi/hpsa.c +22 -0
 				PAGE_SIZE - output_len,
 				"PORT: %.2s ",
 				phys_connector);

@@ -3191,6 +3187,90 @@ out:
 	return rc;
 }
 
+/*
+ * get enclosure information
+ * struct ReportExtendedLUNdata *rlep - Used for BMIC drive number
+ * struct hpsa_scsi_dev_t *encl_dev - device entry for enclosure
+ * Uses id_physical_device to determine the box_index.
+ */
+static int hpsa_get_enclosure_info(struct ctlr_info *h,
+			unsigned char *scsi3addr,
+			struct ReportExtendedLUNdata *rlep, int rle_index,
+			struct hpsa_scsi_dev_t *encl_dev)
+{
+	int rc = -1;
+	struct CommandList *c = NULL;
+	struct ErrorInfo *ei = NULL;
+	struct bmic_sense_storage_box_params *bssbp = NULL;
+	struct bmic_identify_physical_device *id_phys = NULL;
+	struct ext_report_lun_entry *rle = &rlep->LUN[rle_index];
+	u16 bmic_device_index = 0;
+
+
+	bssbp = kzalloc(sizeof(*bssbp), GFP_KERNEL);
+	if (!bssbp)

drivers/scsi/hpsa.c out: · line 3221

Quoted 10 lines
drivers/scsi/hpsa.c +10 -0
+		goto out;
+
+	id_phys = kzalloc(sizeof(*id_phys), GFP_KERNEL);
+	if (!id_phys)
+		goto out;
+
+	bmic_device_index = GET_BMIC_DRIVE_NUMBER(&rle->lunid[0]);
+
+	if (bmic_device_index == 0xFF00)
+		goto out;

Why not put this check before the memory allocations so you can avoid them if this condition is not met?

drivers/scsi/hpsa.c out: · line 3223

Quoted 2 lines
drivers/scsi/hpsa.c +2 -0
+
+	memset(id_phys, 0, sizeof(*id_phys));

Didn't you just obtain zeroed memory from kzalloc()?

Quoted 64 lines
drivers/scsi/hpsa.c +52 -1
+	rc = hpsa_bmic_id_physical_device(h, scsi3addr, bmic_device_index,
+						id_phys, sizeof(*id_phys));
+	if (rc) {
+		dev_warn(&h->pdev->dev, "%s: id_phys failed %d bdi[0x%x]\n",
+			__func__, encl_dev->external, bmic_device_index);
+		goto out;
+	}
+
+	c = cmd_alloc(h);
+
+	rc = fill_cmd(c, BMIC_SENSE_STORAGE_BOX_PARAMS, h, bssbp,
+			sizeof(*bssbp), 0, RAID_CTLR_LUNID, TYPE_CMD);
+
+	if (rc)
+		goto out;
+
+	if (id_phys->phys_connector[1] == 'E')
+		c->Request.CDB[5] = id_phys->box_index;
+	else
+		c->Request.CDB[5] = 0;
+
+	rc = hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_FROMDEVICE,
+						NO_TIMEOUT);
+	if (rc)
+		goto out;
+
+	ei = c->err_info;
+	if (ei->CommandStatus != 0 && ei->CommandStatus != CMD_DATA_UNDERRUN) {
+		rc = -1;
+		goto out;
+	}
+
+	encl_dev->box[id_phys->active_path_number] = bssbp->phys_box_on_port;
+	memcpy(&encl_dev->phys_connector[id_phys->active_path_number],
+		bssbp->phys_connector, sizeof(bssbp->phys_connector));
+
+	rc = IO_OK;
+out:
+	kfree(bssbp);
+	kfree(id_phys);
+
+	if (c)
+		cmd_free(h, c);
+
+	if (rc != IO_OK)
+		hpsa_show_dev_msg(KERN_INFO, h, encl_dev,
+			"Error, could not get enclosure information\n");
+	return rc;
+}
+
 static u64 hpsa_get_sas_address_from_report_physical(struct ctlr_info *h,
 						unsigned char *scsi3addr)
 {

@@ -4032,7 +4112,8 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h)
 
 		/* skip masked non-disk devices */
 		if (MASKED_DEVICE(lunaddrbytes) && physical_device &&
-			(physdev_list->LUN[phys_dev_index].device_flags & 0x01))
+		   (physdev_list->LUN[phys_dev_index].device_type != 0x06) &&
+		   (physdev_list->LUN[phys_dev_index].device_flags & 0x01))
 			continue;
 
 		/* Get device type, vendor, model, device id */

drivers/scsi/hpsa.c static void hpsa_update_scsi_devices(struct ctlr_info *h) · line 4199

Quoted 2 lines
drivers/scsi/hpsa.c +0 -0
@@ -4117,6 +4198,9 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h)
 		case TYPE_TAPE:
 		case TYPE_MEDIUM_CHANGER:

Is it 'okay' that these two types are falling through and will call this new enclosure info routine?

drivers/scsi/hpsa.c static void hpsa_update_scsi_devices(struct ctlr_info *h) · line 4203

Quoted 4 lines
drivers/scsi/hpsa.c +3 -0
 		case TYPE_ENCLOSURE:
+			hpsa_get_enclosure_info(h, lunaddrbytes,
+						physdev_list, phys_dev_index,
+						this_device);

Any reason this routine wasn't made a void? The return code is not being used and the other similar helpers in this path are void.

Quoted 56 lines
drivers/scsi/hpsa.c +10 -0
 			ncurrent++;
 			break;
 		case TYPE_RAID:

@@ -6629,6 +6713,16 @@ static int fill_cmd(struct CommandList *c, u8 cmd, struct ctlr_info *h,
 			c->Request.CDB[7] = (size >> 16) & 0xFF;
 			c->Request.CDB[8] = (size >> 8) & 0XFF;
 			break;
+		case BMIC_SENSE_STORAGE_BOX_PARAMS:
+			c->Request.CDBLen = 10;
+			c->Request.type_attr_dir =
+				TYPE_ATTR_DIR(cmd_type, ATTR_SIMPLE, XFER_READ);
+			c->Request.Timeout = 0;
+			c->Request.CDB[0] = BMIC_READ;
+			c->Request.CDB[6] = BMIC_SENSE_STORAGE_BOX_PARAMS;
+			c->Request.CDB[7] = (size >> 16) & 0xFF;
+			c->Request.CDB[8] = (size >> 8) & 0XFF;
+			break;
 		case BMIC_IDENTIFY_CONTROLLER:
 			c->Request.CDBLen = 10;
 			c->Request.type_attr_dir =
drivers/scsi/hpsa_cmd.h +13 -1
index d92ef0d..6a919ad 100644
@@ -291,6 +291,7 @@ struct SenseSubsystem_info {
 #define BMIC_SENSE_DIAG_OPTIONS 0xF5
 #define HPSA_DIAG_OPTS_DISABLE_RLD_CACHING 0x40000000
 #define BMIC_SENSE_SUBSYSTEM_INFORMATION 0x66
+#define BMIC_SENSE_STORAGE_BOX_PARAMS 0x65
 
 /* Command List Structure */
 union SCSI3Addr {

@@ -842,5 +843,17 @@ struct bmic_sense_subsystem_info {
 	u8	pad[332];
 };
 
+struct bmic_sense_storage_box_params {
+	u8	reserved[36];
+	u8	inquiry_valid;
+	u8	reserved_1[68];
+	u8	phys_box_on_port;
+	u8	reserved_2[22];
+	u16	connection_info;
+	u8	reserver_3[84];
+	u8	phys_connector[2];
+	u8	reserved_4[296];
+};
+
 #pragma pack()
 #endif /* HPSA_CMD_H */
 
--

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#9 lore raw

Re: [PATCH 3/3] hpsa: add box and bay information for enclosure devices

Hi Don,

Did you see these comments I had from my review of this patch?

-matt

Quoted 5 lines · Matthew R. Ochs
Quoted 2 lines · Don Brace

No commit message?

You can disregard this one as I saw you added a message in v1.

Quoted 235 lines
Quoted 24 lines
Reviewed-by Justin Lindley <[email protected]>
Reviewed-by Kevin Barnett <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Signed-off-by Don Brace <[email protected]>

---
drivers/scsi/hpsa.c | 108 ++++++++++++++++++++++++++++++++++++++++++++---
drivers/scsi/hpsa_cmd.h | 13 ++++++

2 files changed, 114 insertions(+), 7 deletions(-)
drivers/scsi/hpsa.c +0 -1
index f8370b8..6f84ec7 100644
@@ -750,7 +750,6 @@ static ssize_t host_show_hp_ssd_smart_path_enabled(struct device *dev,
 }
 
 #define MAX_PATHS 8
-
 static ssize_t path_info_show(struct device *dev,
 	     struct device_attribute *attr, char *buf)
 {

drivers/scsi/hpsa.c static ssize_t path_info_show(struct device *dev, · line 794

Quoted 7 lines
drivers/scsi/hpsa.c +1 -3
@@ -792,9 +791,7 @@ static ssize_t path_info_show(struct device *dev,
 				hdev->bus, hdev->target, hdev->lun,
 				scsi_device_type(hdev->devtype));
 
-		if (hdev->external ||
-			hdev->devtype == TYPE_RAID ||
-			is_logical_device(hdev)) {
+		if (hdev->devtype == TYPE_RAID || is_logical_device(hdev)) {

How does removing the check for external here relate to the rest of this commit?

drivers/scsi/hpsa.c static ssize_t path_info_show(struct device *dev, · line 808

Quoted 10 lines
drivers/scsi/hpsa.c +1 -2
 			output_len += scnprintf(buf + output_len,
 						PAGE_SIZE - output_len,
 						"%s\n", active);

@@ -808,8 +805,7 @@ static ssize_t path_info_show(struct device *dev,
 			phys_connector[0] = '0';
 		if (phys_connector[1] < '0')
 			phys_connector[1] = '0';
-		if (hdev->phys_connector[i] > 0)
-			output_len += scnprintf(buf + output_len,
+		output_len += scnprintf(buf + output_len,

Same question regarding the removal of the > 0 check.

Quoted 29 lines
drivers/scsi/hpsa.c +22 -0
 				PAGE_SIZE - output_len,
 				"PORT: %.2s ",
 				phys_connector);

@@ -3191,6 +3187,90 @@ out:
 	return rc;
 }
 
+/*
+ * get enclosure information
+ * struct ReportExtendedLUNdata *rlep - Used for BMIC drive number
+ * struct hpsa_scsi_dev_t *encl_dev - device entry for enclosure
+ * Uses id_physical_device to determine the box_index.
+ */
+static int hpsa_get_enclosure_info(struct ctlr_info *h,
+			unsigned char *scsi3addr,
+			struct ReportExtendedLUNdata *rlep, int rle_index,
+			struct hpsa_scsi_dev_t *encl_dev)
+{
+	int rc = -1;
+	struct CommandList *c = NULL;
+	struct ErrorInfo *ei = NULL;
+	struct bmic_sense_storage_box_params *bssbp = NULL;
+	struct bmic_identify_physical_device *id_phys = NULL;
+	struct ext_report_lun_entry *rle = &rlep->LUN[rle_index];
+	u16 bmic_device_index = 0;
+
+
+	bssbp = kzalloc(sizeof(*bssbp), GFP_KERNEL);
+	if (!bssbp)

drivers/scsi/hpsa.c out: · line 3221

Quoted 10 lines
drivers/scsi/hpsa.c +10 -0
+		goto out;
+
+	id_phys = kzalloc(sizeof(*id_phys), GFP_KERNEL);
+	if (!id_phys)
+		goto out;
+
+	bmic_device_index = GET_BMIC_DRIVE_NUMBER(&rle->lunid[0]);
+
+	if (bmic_device_index == 0xFF00)
+		goto out;

Why not put this check before the memory allocations so you can avoid them if this condition is not met?

drivers/scsi/hpsa.c out: · line 3223

Quoted 2 lines
drivers/scsi/hpsa.c +2 -0
+
+	memset(id_phys, 0, sizeof(*id_phys));

Didn't you just obtain zeroed memory from kzalloc()?

Quoted 64 lines
drivers/scsi/hpsa.c +52 -1
+	rc = hpsa_bmic_id_physical_device(h, scsi3addr, bmic_device_index,
+						id_phys, sizeof(*id_phys));
+	if (rc) {
+		dev_warn(&h->pdev->dev, "%s: id_phys failed %d bdi[0x%x]\n",
+			__func__, encl_dev->external, bmic_device_index);
+		goto out;
+	}
+
+	c = cmd_alloc(h);
+
+	rc = fill_cmd(c, BMIC_SENSE_STORAGE_BOX_PARAMS, h, bssbp,
+			sizeof(*bssbp), 0, RAID_CTLR_LUNID, TYPE_CMD);
+
+	if (rc)
+		goto out;
+
+	if (id_phys->phys_connector[1] == 'E')
+		c->Request.CDB[5] = id_phys->box_index;
+	else
+		c->Request.CDB[5] = 0;
+
+	rc = hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_FROMDEVICE,
+						NO_TIMEOUT);
+	if (rc)
+		goto out;
+
+	ei = c->err_info;
+	if (ei->CommandStatus != 0 && ei->CommandStatus != CMD_DATA_UNDERRUN) {
+		rc = -1;
+		goto out;
+	}
+
+	encl_dev->box[id_phys->active_path_number] = bssbp->phys_box_on_port;
+	memcpy(&encl_dev->phys_connector[id_phys->active_path_number],
+		bssbp->phys_connector, sizeof(bssbp->phys_connector));
+
+	rc = IO_OK;
+out:
+	kfree(bssbp);
+	kfree(id_phys);
+
+	if (c)
+		cmd_free(h, c);
+
+	if (rc != IO_OK)
+		hpsa_show_dev_msg(KERN_INFO, h, encl_dev,
+			"Error, could not get enclosure information\n");
+	return rc;
+}
+
 static u64 hpsa_get_sas_address_from_report_physical(struct ctlr_info *h,
 						unsigned char *scsi3addr)
 {

@@ -4032,7 +4112,8 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h)
 
 		/* skip masked non-disk devices */
 		if (MASKED_DEVICE(lunaddrbytes) && physical_device &&
-			(physdev_list->LUN[phys_dev_index].device_flags & 0x01))
+		   (physdev_list->LUN[phys_dev_index].device_type != 0x06) &&
+		   (physdev_list->LUN[phys_dev_index].device_flags & 0x01))
 			continue;
 
 		/* Get device type, vendor, model, device id */

drivers/scsi/hpsa.c static void hpsa_update_scsi_devices(struct ctlr_info *h) · line 4199

Quoted 2 lines
drivers/scsi/hpsa.c +0 -0
@@ -4117,6 +4198,9 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h)
 		case TYPE_TAPE:
 		case TYPE_MEDIUM_CHANGER:

Is it 'okay' that these two types are falling through and will call this new enclosure info routine?

drivers/scsi/hpsa.c static void hpsa_update_scsi_devices(struct ctlr_info *h) · line 4203

Quoted 4 lines
drivers/scsi/hpsa.c +3 -0
 		case TYPE_ENCLOSURE:
+			hpsa_get_enclosure_info(h, lunaddrbytes,
+						physdev_list, phys_dev_index,
+						this_device);

Any reason this routine wasn't made a void? The return code is not being used and the other similar helpers in this path are void.

Quoted 52 lines
drivers/scsi/hpsa.c +10 -0
 			ncurrent++;
 			break;
 		case TYPE_RAID:

@@ -6629,6 +6713,16 @@ static int fill_cmd(struct CommandList *c, u8 cmd, struct ctlr_info *h,
 			c->Request.CDB[7] = (size >> 16) & 0xFF;
 			c->Request.CDB[8] = (size >> 8) & 0XFF;
 			break;
+		case BMIC_SENSE_STORAGE_BOX_PARAMS:
+			c->Request.CDBLen = 10;
+			c->Request.type_attr_dir =
+				TYPE_ATTR_DIR(cmd_type, ATTR_SIMPLE, XFER_READ);
+			c->Request.Timeout = 0;
+			c->Request.CDB[0] = BMIC_READ;
+			c->Request.CDB[6] = BMIC_SENSE_STORAGE_BOX_PARAMS;
+			c->Request.CDB[7] = (size >> 16) & 0xFF;
+			c->Request.CDB[8] = (size >> 8) & 0XFF;
+			break;
 		case BMIC_IDENTIFY_CONTROLLER:
 			c->Request.CDBLen = 10;
 			c->Request.type_attr_dir =
drivers/scsi/hpsa_cmd.h +13 -1
index d92ef0d..6a919ad 100644
@@ -291,6 +291,7 @@ struct SenseSubsystem_info {
 #define BMIC_SENSE_DIAG_OPTIONS 0xF5
 #define HPSA_DIAG_OPTS_DISABLE_RLD_CACHING 0x40000000
 #define BMIC_SENSE_SUBSYSTEM_INFORMATION 0x66
+#define BMIC_SENSE_STORAGE_BOX_PARAMS 0x65
 
 /* Command List Structure */
 union SCSI3Addr {

@@ -842,5 +843,17 @@ struct bmic_sense_subsystem_info {
 	u8	pad[332];
 };
 
+struct bmic_sense_storage_box_params {
+	u8	reserved[36];
+	u8	inquiry_valid;
+	u8	reserved_1[68];
+	u8	phys_box_on_port;
+	u8	reserved_2[22];
+	u16	connection_info;
+	u8	reserver_3[84];
+	u8	phys_connector[2];
+	u8	reserved_4[296];
+};
+
 #pragma pack()
 #endif /* HPSA_CMD_H */
 
--
Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#10 lore raw

Re: [PATCH 3/3] hpsa: add box and bay information for enclosure devices

Quoted 5 lines · Matthew R. Ochs

Hi Don,

Did you see these comments I had from my review of this patch?

Quoted 38 lines

-matt

Quoted 4 lines · Matthew R. Ochs
Quoted 2 lines · Don Brace

No commit message?

You can disregard this one as I saw you added a message in v1.

Quoted 30 lines
Quoted 22 lines
Reviewed-by Justin Lindley <[email protected]>
Reviewed-by Kevin Barnett <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Signed-off-by Don Brace <[email protected]>

---
drivers/scsi/hpsa.c | 108 ++++++++++++++++++++++++++++++++++++++++++++---
drivers/scsi/hpsa_cmd.h | 13 ++++++

2 files changed, 114 insertions(+), 7 deletions(-)
drivers/scsi/hpsa.c +0 -1
index f8370b8..6f84ec7 100644
@@ -750,7 +750,6 @@ static ssize_t host_show_hp_ssd_smart_path_enabled(struct device *dev,
 }
 
 #define MAX_PATHS 8
-
 static ssize_t path_info_show(struct device *dev,
 	     struct device_attribute *attr, char *buf)
 {

drivers/scsi/hpsa.c static ssize_t path_info_show(struct device *dev, · line 794

Quoted 7 lines
drivers/scsi/hpsa.c +1 -3
@@ -792,9 +791,7 @@ static ssize_t path_info_show(struct device *dev,
 				hdev->bus, hdev->target, hdev->lun,
 				scsi_device_type(hdev->devtype));
 
-		if (hdev->external ||
-			hdev->devtype == TYPE_RAID ||
-			is_logical_device(hdev)) {
+		if (hdev->devtype == TYPE_RAID || is_logical_device(hdev)) {

How does removing the check for external here relate to the rest of this commit?

We have enclosure devices that would not show up properly if this check was not removed.

Quoted 12 lines
Quoted 12 lines

drivers/scsi/hpsa.c static ssize_t path_info_show(struct device *dev, · line 808

Quoted 11 lines
drivers/scsi/hpsa.c +1 -2
 			output_len += scnprintf(buf + output_len,
 						PAGE_SIZE - output_len,
 						"%s\n", active);

@@ -808,8 +805,7 @@ static ssize_t path_info_show(struct device *dev,
 			phys_connector[0] = '0';
 		if (phys_connector[1] < '0')
 			phys_connector[1] = '0';
-		if (hdev->phys_connector[i] > 0)
-			output_len += scnprintf(buf + output_len,
+		output_len += scnprintf(buf + output_len,

Same question regarding the removal of the > 0 check.

The connector contains alpha-numeric data. We were missing connector info like "2I" "2E", ...

Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
drivers/scsi/hpsa.c +26 -0
 				PAGE_SIZE - output_len,
 				"PORT: %.2s ",
 				phys_connector);

@@ -3191,6 +3187,90 @@ out:
 	return rc;
 }
 
+/*
+ * get enclosure information
+ * struct ReportExtendedLUNdata *rlep - Used for BMIC drive number
+ * struct hpsa_scsi_dev_t *encl_dev - device entry for enclosure
+ * Uses id_physical_device to determine the box_index.
+ */
+static int hpsa_get_enclosure_info(struct ctlr_info *h,
+			unsigned char *scsi3addr,
+			struct ReportExtendedLUNdata *rlep, int rle_index,
+			struct hpsa_scsi_dev_t *encl_dev)
+{
+	int rc = -1;
+	struct CommandList *c = NULL;
+	struct ErrorInfo *ei = NULL;
+	struct bmic_sense_storage_box_params *bssbp = NULL;
+	struct bmic_identify_physical_device *id_phys = NULL;
+	struct ext_report_lun_entry *rle = &rlep->LUN[rle_index];
+	u16 bmic_device_index = 0;
+
+
+	bssbp = kzalloc(sizeof(*bssbp), GFP_KERNEL);
+	if (!bssbp)
+		goto out;
+
+	id_phys = kzalloc(sizeof(*id_phys), GFP_KERNEL);
+	if (!id_phys)
Quoted 8 lines
Quoted 8 lines

drivers/scsi/hpsa.c out: · line 3221

Quoted 6 lines
drivers/scsi/hpsa.c +6 -0
+		goto out;
+
+	bmic_device_index = GET_BMIC_DRIVE_NUMBER(&rle->lunid[0]);
+
+	if (bmic_device_index == 0xFF00)
+		goto out;

Why not put this check before the memory allocations so you can avoid them if this condition is not met?

Good point. I will do this for V3.

Quoted 4 lines
Quoted 4 lines

drivers/scsi/hpsa.c out: · line 3223

Quoted 3 lines
drivers/scsi/hpsa.c +2 -0
+
+	memset(id_phys, 0, sizeof(*id_phys));

Didn't you just obtain zeroed memory from kzalloc()?

Another good catch.

Quoted 63 lines
Quoted 63 lines
Quoted 63 lines
drivers/scsi/hpsa.c +52 -1
+	rc = hpsa_bmic_id_physical_device(h, scsi3addr, bmic_device_index,
+						id_phys, sizeof(*id_phys));
+	if (rc) {
+		dev_warn(&h->pdev->dev, "%s: id_phys failed %d bdi[0x%x]\n",
+			__func__, encl_dev->external, bmic_device_index);
+		goto out;
+	}
+
+	c = cmd_alloc(h);
+
+	rc = fill_cmd(c, BMIC_SENSE_STORAGE_BOX_PARAMS, h, bssbp,
+			sizeof(*bssbp), 0, RAID_CTLR_LUNID, TYPE_CMD);
+
+	if (rc)
+		goto out;
+
+	if (id_phys->phys_connector[1] == 'E')
+		c->Request.CDB[5] = id_phys->box_index;
+	else
+		c->Request.CDB[5] = 0;
+
+	rc = hpsa_scsi_do_simple_cmd_with_retry(h, c, PCI_DMA_FROMDEVICE,
+						NO_TIMEOUT);
+	if (rc)
+		goto out;
+
+	ei = c->err_info;
+	if (ei->CommandStatus != 0 && ei->CommandStatus != CMD_DATA_UNDERRUN) {
+		rc = -1;
+		goto out;
+	}
+
+	encl_dev->box[id_phys->active_path_number] = bssbp->phys_box_on_port;
+	memcpy(&encl_dev->phys_connector[id_phys->active_path_number],
+		bssbp->phys_connector, sizeof(bssbp->phys_connector));
+
+	rc = IO_OK;
+out:
+	kfree(bssbp);
+	kfree(id_phys);
+
+	if (c)
+		cmd_free(h, c);
+
+	if (rc != IO_OK)
+		hpsa_show_dev_msg(KERN_INFO, h, encl_dev,
+			"Error, could not get enclosure information\n");
+	return rc;
+}
+
 static u64 hpsa_get_sas_address_from_report_physical(struct ctlr_info *h,
 						unsigned char *scsi3addr)
 {

@@ -4032,7 +4112,8 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h)
 
 		/* skip masked non-disk devices */
 		if (MASKED_DEVICE(lunaddrbytes) && physical_device &&
-			(physdev_list->LUN[phys_dev_index].device_flags & 0x01))
+		   (physdev_list->LUN[phys_dev_index].device_type != 0x06) &&
+		   (physdev_list->LUN[phys_dev_index].device_flags & 0x01))
 			continue;
Quoted 6 lines
Quoted 6 lines

drivers/scsi/hpsa.c static void hpsa_update_scsi_devices(struct ctlr_info *h) · line 4199

Quoted 4 lines
drivers/scsi/hpsa.c +0 -0
 		/* Get device type, vendor, model, device id */

@@ -4117,6 +4198,9 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h)
 		case TYPE_TAPE:
 		case TYPE_MEDIUM_CHANGER:

Is it 'okay' that these two types are falling through and will call this new enclosure info routine?

Good Point.

Quoted 7 lines
Quoted 7 lines

drivers/scsi/hpsa.c static void hpsa_update_scsi_devices(struct ctlr_info *h) · line 4203

Quoted 5 lines
drivers/scsi/hpsa.c +3 -0
 		case TYPE_ENCLOSURE:
+			hpsa_get_enclosure_info(h, lunaddrbytes,
+						physdev_list, phys_dev_index,
+						this_device);

Any reason this routine wasn't made a void? The return code is not being used and the other similar helpers in this path are void.

Good point.

Quoted 66 lines
Quoted 52 lines
Quoted 52 lines
drivers/scsi/hpsa.c +10 -0
 			ncurrent++;
 			break;
 		case TYPE_RAID:

@@ -6629,6 +6713,16 @@ static int fill_cmd(struct CommandList *c, u8 cmd, struct ctlr_info *h,
 			c->Request.CDB[7] = (size >> 16) & 0xFF;
 			c->Request.CDB[8] = (size >> 8) & 0XFF;
 			break;
+		case BMIC_SENSE_STORAGE_BOX_PARAMS:
+			c->Request.CDBLen = 10;
+			c->Request.type_attr_dir =
+				TYPE_ATTR_DIR(cmd_type, ATTR_SIMPLE, XFER_READ);
+			c->Request.Timeout = 0;
+			c->Request.CDB[0] = BMIC_READ;
+			c->Request.CDB[6] = BMIC_SENSE_STORAGE_BOX_PARAMS;
+			c->Request.CDB[7] = (size >> 16) & 0xFF;
+			c->Request.CDB[8] = (size >> 8) & 0XFF;
+			break;
 		case BMIC_IDENTIFY_CONTROLLER:
 			c->Request.CDBLen = 10;
 			c->Request.type_attr_dir =
drivers/scsi/hpsa_cmd.h +13 -0
index d92ef0d..6a919ad 100644
@@ -291,6 +291,7 @@ struct SenseSubsystem_info {
 #define BMIC_SENSE_DIAG_OPTIONS 0xF5
 #define HPSA_DIAG_OPTS_DISABLE_RLD_CACHING 0x40000000
 #define BMIC_SENSE_SUBSYSTEM_INFORMATION 0x66
+#define BMIC_SENSE_STORAGE_BOX_PARAMS 0x65
 
 /* Command List Structure */
 union SCSI3Addr {

@@ -842,5 +843,17 @@ struct bmic_sense_subsystem_info {
 	u8	pad[332];
 };
 
+struct bmic_sense_storage_box_params {
+	u8	reserved[36];
+	u8	inquiry_valid;
+	u8	reserved_1[68];
+	u8	phys_box_on_port;
+	u8	reserved_2[22];
+	u16	connection_info;
+	u8	reserver_3[84];
+	u8	phys_connector[2];
+	u8	reserved_4[296];
+};
+
 #pragma pack()
 #endif /* HPSA_CMD_H */
Quoted 10 lines
Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html