[PATCH 0/4] ses: simple subenclosure support

[PATCH 0/4] ses: simple subenclosure support

[Hannes Reinecke]

Hi all,

some arrays (most notably 3Par) only support simple subenclosures.
Sadly our ses implementation doesn't handle this properly, so we're
greeted with error messages like:

scsi 1:0:0:254: Wrong diagnostic page; asked for 2 got 0
scsi 1:0:0:254: Failed to get diagnostic page 0xffffffea
scsi 1:0:0:254: Failed to bind enclosure -19
ses 1:0:0:254: Attached Enclosure device

This patchset fixes up our ses implementation to work properly
with simple subenclosures.

As usual, comments and reviews are welcome.

Hannes Reinecke (4):
  scsi: Fixup ses page check
  ses: check return code from ses_recv_diag()
  ses: Fixup error message 'failed to get diagnostic page 0xffffffea'
  ses: make page2 support optional

 drivers/scsi/ses.c | 63 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 11 deletions(-)

-- 
1.8.5.6

[PATCH 1/4] scsi: Fixup ses page check

[Hannes Reinecke]

The error code from a scsi_execute_req() is a SCSI status, not
a normal errno. So whenever it returns a value here an error
occurred and there's no point in looking at the page number.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/ses.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index f1cdf32..a37aec8 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -99,8 +99,8 @@ static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 
 	ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
 				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
-	if (unlikely(!ret))
-		return ret;
+	if (unlikely(ret))
+		return -EIO;
 
 	recv_page_code = ((unsigned char *)buf)[0];
 
-- 
1.8.5.6

[PATCH 2/4] ses: check return code from ses_recv_diag()

[Hannes Reinecke]

We should be checking the return code from ses_recv_diag() to
avoid accessing invalid data.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/ses.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index a37aec8..fc70c00 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -179,7 +179,8 @@ static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev,
 	unsigned char *type_ptr = ses_dev->page1_types;
 	unsigned char *desc_ptr = ses_dev->page2 + 8;
 
-	ses_recv_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
+	if (ses_recv_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len) < 0)
+		return NULL;
 
 	for (i = 0; i < ses_dev->page1_num_types; i++, type_ptr += 4) {
 		for (j = 0; j < type_ptr[1]; j++) {
-- 
1.8.5.6

[PATCH 3/4] ses: Fixup error message 'failed to get diagnostic page 0xffffffea'

[Hannes Reinecke]

The printk was using the result as argument, leading to a slightly
confusing log message.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/ses.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index fc70c00..46984cd 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -602,7 +602,7 @@ static int ses_intf_add(struct device *cdev,
 {
 	struct scsi_device *sdev = to_scsi_device(cdev->parent);
 	struct scsi_device *tmp_sdev;
-	unsigned char *buf = NULL, *hdr_buf, *type_ptr;
+	unsigned char *buf = NULL, *hdr_buf, *type_ptr, page;
 	struct ses_device *ses_dev;
 	u32 result;
 	int i, types, len, components = 0;
@@ -631,7 +631,8 @@ static int ses_intf_add(struct device *cdev,
 	if (!hdr_buf || !ses_dev)
 		goto err_init_free;
 
-	result = ses_recv_diag(sdev, 1, hdr_buf, INIT_ALLOC_SIZE);
+	page = 1;
+	result = ses_recv_diag(sdev, page, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
 		goto recv_failed;
 
@@ -640,7 +641,7 @@ static int ses_intf_add(struct device *cdev,
 	if (!buf)
 		goto err_free;
 
-	result = ses_recv_diag(sdev, 1, buf, len);
+	result = ses_recv_diag(sdev, page, buf, len);
 	if (result)
 		goto recv_failed;
 
@@ -670,7 +671,8 @@ static int ses_intf_add(struct device *cdev,
 	ses_dev->page1_len = len;
 	buf = NULL;
 
-	result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
+	page = 2;
+	result = ses_recv_diag(sdev, page, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
 		goto recv_failed;
 
@@ -689,7 +691,8 @@ static int ses_intf_add(struct device *cdev,
 
 	/* The additional information page --- allows us
 	 * to match up the devices */
-	result = ses_recv_diag(sdev, 10, hdr_buf, INIT_ALLOC_SIZE);
+	page = 10;
+	result = ses_recv_diag(sdev, page, hdr_buf, INIT_ALLOC_SIZE);
 	if (!result) {
 
 		len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
@@ -697,7 +700,7 @@ static int ses_intf_add(struct device *cdev,
 		if (!buf)
 			goto err_free;
 
-		result = ses_recv_diag(sdev, 10, buf, len);
+		result = ses_recv_diag(sdev, page, buf, len);
 		if (result)
 			goto recv_failed;
 		ses_dev->page10 = buf;
@@ -735,7 +738,7 @@ static int ses_intf_add(struct device *cdev,
 
  recv_failed:
 	sdev_printk(KERN_ERR, sdev, "Failed to get diagnostic page 0x%x\n",
-		    result);
+		    page);
 	err = -ENODEV;
  err_free:
 	kfree(buf);
-- 
1.8.5.6

[PATCH 4/4] ses: make page2 support optional

[Hannes Reinecke]

Simple subenclosures do not need to support SES page 2, so make
it optional.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/ses.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 46984cd..c52d110 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -51,6 +51,13 @@ struct ses_component {
 	u64 addr;
 };
 
+static bool ses_page2_supported(struct enclosure_device *edev)
+{
+	struct ses_device *ses_dev = edev->scratch;
+
+	return (ses_dev->page2 != NULL);
+}
+
 static int ses_probe(struct device *dev)
 {
 	struct scsi_device *sdev = to_scsi_device(dev);
@@ -204,6 +211,10 @@ static void ses_get_fault(struct enclosure_device *edev,
 {
 	unsigned char *desc;
 
+	if (!ses_page2_supported(edev)) {
+		ecomp->fault = 0;
+		return;
+	}
 	desc = ses_get_page2_descriptor(edev, ecomp);
 	if (desc)
 		ecomp->fault = (desc[3] & 0x60) >> 4;
@@ -216,6 +227,9 @@ static int ses_set_fault(struct enclosure_device *edev,
 	unsigned char desc[4];
 	unsigned char *desc_ptr;
 
+	if (!ses_page2_supported(edev))
+		return -EINVAL;
+
 	desc_ptr = ses_get_page2_descriptor(edev, ecomp);
 
 	if (!desc_ptr)
@@ -243,6 +257,10 @@ static void ses_get_status(struct enclosure_device *edev,
 {
 	unsigned char *desc;
 
+	if (!ses_page2_supported(edev)) {
+		ecomp->status = 0;
+		return;
+	}
 	desc = ses_get_page2_descriptor(edev, ecomp);
 	if (desc)
 		ecomp->status = (desc[0] & 0x0f);
@@ -253,6 +271,10 @@ static void ses_get_locate(struct enclosure_device *edev,
 {
 	unsigned char *desc;
 
+	if (!ses_page2_supported(edev)) {
+		ecomp->locate = 0;
+		return;
+	}
 	desc = ses_get_page2_descriptor(edev, ecomp);
 	if (desc)
 		ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
@@ -265,6 +287,9 @@ static int ses_set_locate(struct enclosure_device *edev,
 	unsigned char desc[4];
 	unsigned char *desc_ptr;
 
+	if (!ses_page2_supported(edev))
+		return -EINVAL;
+
 	desc_ptr = ses_get_page2_descriptor(edev, ecomp);
 
 	if (!desc_ptr)
@@ -293,6 +318,9 @@ static int ses_set_active(struct enclosure_device *edev,
 	unsigned char desc[4];
 	unsigned char *desc_ptr;
 
+	if (!ses_page2_supported(edev))
+		return -EINVAL;
+
 	desc_ptr = ses_get_page2_descriptor(edev, ecomp);
 
 	if (!desc_ptr)
@@ -329,6 +357,11 @@ static void ses_get_power_status(struct enclosure_device *edev,
 {
 	unsigned char *desc;
 
+	if (!ses_page2_supported(edev)) {
+		ecomp->power_status = 0;
+		return;
+	}
+
 	desc = ses_get_page2_descriptor(edev, ecomp);
 	if (desc)
 		ecomp->power_status = (desc[3] & 0x10) ? 0 : 1;
@@ -341,6 +374,9 @@ static int ses_set_power_status(struct enclosure_device *edev,
 	unsigned char desc[4];
 	unsigned char *desc_ptr;
 
+	if (!ses_page2_supported(edev))
+		return -EINVAL;
+
 	desc_ptr = ses_get_page2_descriptor(edev, ecomp);
 
 	if (!desc_ptr)
@@ -674,7 +710,7 @@ static int ses_intf_add(struct device *cdev,
 	page = 2;
 	result = ses_recv_diag(sdev, page, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
-		goto recv_failed;
+		goto page2_not_supported;
 
 	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
 	buf = kzalloc(len, GFP_KERNEL);
@@ -707,6 +743,7 @@ static int ses_intf_add(struct device *cdev,
 		ses_dev->page10_len = len;
 		buf = NULL;
 	}
+page2_not_supported:
 	scomp = kzalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
 	if (!scomp)
 		goto err_free;
-- 
1.8.5.6

Re: [PATCH 1/4] scsi: Fixup ses page check

[Martin K. Petersen]

Hannes,

> The error code from a scsi_execute_req() is a SCSI status, not
> a normal errno. So whenever it returns a value here an error
> occurred and there's no point in looking at the page number.

424f727b9413 scsi: ses: Fix wrong page error

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/4] ses: simple subenclosure support

[Martin K. Petersen]

Hannes,

> some arrays (most notably 3Par) only support simple subenclosures.
> Sadly our ses implementation doesn't handle this properly, so we're
> greeted with error messages like:
>
> scsi 1:0:0:254: Wrong diagnostic page; asked for 2 got 0
> scsi 1:0:0:254: Failed to get diagnostic page 0xffffffea
> scsi 1:0:0:254: Failed to bind enclosure -19
> ses 1:0:0:254: Attached Enclosure device
>
> This patchset fixes up our ses implementation to work properly
> with simple subenclosures.

Looks OK to me. Applied to 4.14/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering