iprdd-devel Mailing List for IPR Linux device driver

Both functions copied the file parameter via strcpy() into
image.file (char[100]), causing a buffer overflow if the caller
supplied a path of 100 or more characters.

Replace strcpy() with strncpy() plus explicit NUL termination, and
add a pre-check that rejects NULL pointers and paths that are too
long, logging the error via syslog and returning -EINVAL.

Since the length check guarantees strlen(file) < sizeof(image.file),
the strncpy will not actually truncate; it serves as a secondary
defence.

Signed-off-by: litenglong <lit...@ky...>
---
 iprconfig.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/iprconfig.c b/iprconfig.c
index 2c4b95a..948bf36 100644
--- a/iprconfig.c
+++ b/iprconfig.c
@@ -15701,7 +15701,12 @@ static int update_ioa_ucode(struct ipr_ioa *ioa, char *file)
 	struct ipr_fw_images image;
 	int rc;
 
-	strcpy(image.file, file);
+	if (file == NULL || strlen(file) >= sizeof(image.file)) {
+		syslog(LOG_ERR, "Firmware file path is NULL or too long\n");
+		return -EINVAL;
+	}
+	strncpy(image.file, file, sizeof(image.file) - 1);
+	image.file[sizeof(image.file) - 1] = '\0';
 	image.version = get_ioa_ucode_version(file);
 	image.has_header = 0;
 
@@ -15725,7 +15730,12 @@ static int update_dev_ucode(struct ipr_dev *dev, char *file)
 	struct ipr_fw_images image;
 	int rc;
 
-	strcpy(image.file, file);
+	if (file == NULL || strlen(file) >= sizeof(image.file)) {
+		syslog(LOG_ERR, "Firmware file path is NULL or too long\n");
+		return -EINVAL;
+	}
+	strncpy(image.file, file, sizeof(image.file) - 1);
+	image.file[sizeof(image.file) - 1] = '\0';
 
 	if (dev->scsi_dev_data->type == TYPE_ENCLOSURE ||
 	    dev->scsi_dev_data->type == TYPE_PROCESSOR) {
-- 
2.25.1

strncpy(cmnd, editor, sizeof(cmnd)-1) did not guarantee NUL termination
when editor was at least sizeof(cmnd)-1 characters long, causing strtok()
to read past the cmnd buffer. Fix by explicitly NUL-terminating cmnd
after strncpy.

malloc(strlen(tok)) allocated one byte too few for the trailing NUL,
so the subsequent strcpy wrote past the heap allocation. Replace with
strdup() which allocates strlen(tok)+1 bytes.

Add NULL checks for strtok() return and strdup() return to guard
against degenerate inputs (editor containing only whitespace) and
allocation failures, exiting the forked child cleanly.

Signed-off-by: litenglong <lit...@ky...>
---
 iprconfig.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/iprconfig.c b/iprconfig.c
index 0453648..2c4b95a 100644
--- a/iprconfig.c
+++ b/iprconfig.c
@@ -12202,17 +12202,20 @@ static int invoke_editor(char* filename)
 
 		/* copy editor name to argv[0] */
 		strncpy(cmnd, editor, sizeof(cmnd) - 1);
+		cmnd[sizeof(cmnd) - 1] = '\0';
 		tok = strtok(cmnd, " ");
-		argv[0] = malloc(strlen(tok));
-		strcpy(argv[0], tok);
+		if (tok == NULL)
+			_exit(EXIT_FAILURE);
+		argv[0] = strdup(tok);
+		if (argv[0] == NULL)
+			_exit(EXIT_FAILURE);
 
 		/* handle editor arguments, if any */
 		for (i = 1; i < EDITOR_MAX_ARGS - 2; ++i) {
 			tok = strtok(NULL, " ");
 			if (tok == NULL)
 				break;
-			argv[i] = malloc(strlen(tok));
-			strcpy(argv[i], tok);
+			argv[i] = strdup(tok);
 		}
 		argv[i] = filename;
 		argv[i+1] = NULL;
-- 
2.25.1

Looks good.

Reviewed-by: Greg Joyce<gj...@li...>

On Wed, 2025-09-24 at 11:33 -0500, wen...@li... wrote:
> From: Wen Xiong <wen...@li...>
> 
> The patch checkes if device supports inquiry c7 page before
> calling to inquiry c7 page.
> 
> Signed-off-by: Wen Xiong <wen...@li...>
> ---
>  iprlib.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/iprlib.c b/iprlib.c
> index 624dcdd..c024ea2 100644
> --- a/iprlib.c
> +++ b/iprlib.c
> @@ -6508,6 +6508,25 @@ int ipr_get_logical_block_size(struct ipr_dev
> *dev)
>  	return rc;
>  }
>  
> +bool inquiry_support_pages(struct ipr_dev *dev, u_int page_no)
> +{
> +	struct ipr_inquiry_page0 page0_inq;
> +	int rc, i;
> +
> +	rc = ipr_inquiry(dev, 0, &page0_inq, sizeof(page0_inq));
> +
> +	if (rc != 0) {
> +		scsi_dbg(dev, "Inquiry support pages failed =
> `%d\n", rc);
> +		return false;
> +	}
> +
> +	for (i = 0; i < page0_inq.page_length; i++)
> +		if (page0_inq.supported_page_codes[i] == page_no)
> +			return true;
> +
> +	return false;
> +}
> +
>  /**
>   * init_inquiry_c7 - Page 0xC7 Inquiry to disks
>   * @dev:		ipr dev struct
> @@ -6770,7 +6789,8 @@ void __check_current_config(bool
> allow_rebuild_refresh, bool device_details_only
>  				memcpy(ra, &res_addr, sizeof(*ra));
>  
>  			if (ipr_is_gscsi(dev) ||
> ipr_is_af_dasd_device(dev) && !device_details_only)
> -				init_inquiry_c7(dev);
> +				if (inquiry_support_pages(dev,
> 0xc7))
> +					init_inquiry_c7(dev);
>  
>  			if (!dev->qac_entry)
>  				continue;
> @@ -7366,7 +7386,8 @@ static int get_format_timeout(struct ipr_dev
> *dev)
>  	int rc, i, records, timeout;
>  	char temp[100];
>  
> -	rc = init_inquiry_c7(dev);
> +	if (inquiry_support_pages(dev, 0xc7))
> +		rc = init_inquiry_c7(dev);
>  
>  	if (rc && ipr_is_af_dasd_device(dev)) {
>  		rc = ipr_query_dasd_timeouts(dev, &tos);

From: Wen Xiong <wen...@li...>

The patch checkes if device supports inquiry c7 page before
calling to inquiry c7 page.

Signed-off-by: Wen Xiong <wen...@li...>
---
 iprlib.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/iprlib.c b/iprlib.c
index 624dcdd..c024ea2 100644
--- a/iprlib.c
+++ b/iprlib.c
@@ -6508,6 +6508,25 @@ int ipr_get_logical_block_size(struct ipr_dev *dev)
 	return rc;
 }
 
+bool inquiry_support_pages(struct ipr_dev *dev, u_int page_no)
+{
+	struct ipr_inquiry_page0 page0_inq;
+	int rc, i;
+
+	rc = ipr_inquiry(dev, 0, &page0_inq, sizeof(page0_inq));
+
+	if (rc != 0) {
+		scsi_dbg(dev, "Inquiry support pages failed = `%d\n", rc);
+		return false;
+	}
+
+	for (i = 0; i < page0_inq.page_length; i++)
+		if (page0_inq.supported_page_codes[i] == page_no)
+			return true;
+
+	return false;
+}
+
 /**
  * init_inquiry_c7 - Page 0xC7 Inquiry to disks
  * @dev:		ipr dev struct
@@ -6770,7 +6789,8 @@ void __check_current_config(bool allow_rebuild_refresh, bool device_details_only
 				memcpy(ra, &res_addr, sizeof(*ra));
 
 			if (ipr_is_gscsi(dev) || ipr_is_af_dasd_device(dev) && !device_details_only)
-				init_inquiry_c7(dev);
+				if (inquiry_support_pages(dev, 0xc7))
+					init_inquiry_c7(dev);
 
 			if (!dev->qac_entry)
 				continue;
@@ -7366,7 +7386,8 @@ static int get_format_timeout(struct ipr_dev *dev)
 	int rc, i, records, timeout;
 	char temp[100];
 
-	rc = init_inquiry_c7(dev);
+	if (inquiry_support_pages(dev, 0xc7))
+		rc = init_inquiry_c7(dev);
 
 	if (rc && ipr_is_af_dasd_device(dev)) {
 		rc = ipr_query_dasd_timeouts(dev, &tos);
-- 
2.47.3

Applied.

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

From: Wen Xiong <wen...@li...>

The patch fixes several wrong values in Fuel Gauge:
- byte written
- life remaining
- pfa trip

Signed-off-by: Wen Xiong <wen...@li...>
---
 iprconfig.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/iprconfig.c b/iprconfig.c
index f74a70c..0453648 100644
--- a/iprconfig.c
+++ b/iprconfig.c
@@ -19152,7 +19152,7 @@ static char *print_ssd_report(struct ipr_dev *dev, char *body)
 	struct ipr_sas_log_write_err_cnt_attr *bytes_counter;
 
 	char buffer[BUFSIZ];
-	int rc, len, nentries = 0;
+	int rc, len;
 	u64 aux;
 	u64 total_gb_writ = 0;
 	u32 uptime = 0;
@@ -19201,7 +19201,7 @@ static char *print_ssd_report(struct ipr_dev *dev, char *body)
 	if (rc)
 		return NULL;
 
-	smart_attr = ipr_sas_log_get_param(&page34_log, 0xE7, NULL);
+	smart_attr = ipr_sas_log_get_param(&page34_log, 0xB1, NULL);
 	if (smart_attr && smart_attr->norm_worst_val > 0)
 		life_remain = smart_attr->norm_worst_val;
 
@@ -19215,17 +19215,14 @@ static char *print_ssd_report(struct ipr_dev *dev, char *body)
 
 	bytes_counter = ipr_sas_log_get_param(&page02_log, 0x05, NULL);
 	if (bytes_counter)
-		total_gb_writ = be64toh(*(u64 *) &bytes_counter->counter) >> 14;
+		total_gb_writ = be64toh(*((u64 *) &bytes_counter->counter))/1024/1024/1024;
 
 	rc = ipr_log_sense(dev, 0x2F, &page2F_log, sizeof(page2F_log));
 	if (rc)
 		return NULL;
-
-	inf_except_attr = ipr_sas_log_get_param(&page2F_log, 0x0, &nentries);
+	inf_except_attr = ipr_sas_log_get_param(&page2F_log, 0x0, NULL);
 	if (inf_except_attr)
 		pfa_trip = inf_except_attr->inf_except_add_sense_code;
-	if (nentries > 1)
-		pfa_trip = 1;
 
 	body = add_line_to_body(body, "", NULL);
 	/* FRU Number */
@@ -19254,7 +19251,7 @@ static char *print_ssd_report(struct ipr_dev *dev, char *body)
 	body = add_line_to_body(body, _("Firmware Version"), buffer);
 
 	/* Bytes written */
-	snprintf(buffer, BUFSIZ, "%ld GB", total_gb_writ);
+	snprintf(buffer, BUFSIZ, "%llu GB", total_gb_writ);
 	body = add_line_to_body(body, _("Total Bytes Written"), buffer);
 
 	/* Max bytes. */
-- 
2.27.0

Applied,

Thanks!

Brian


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

From: Wen Xiong <wen...@li...>

Without freeing the raid_cmd_head list before creating the raid menu,
GUI menu of creating/deleting an array got mess up.

Signed-off-by: Wen Xiong <wen...@li...>
---
 iprconfig.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/iprconfig.c b/iprconfig.c
index 5f31beb..f74a70c 100644
--- a/iprconfig.c
+++ b/iprconfig.c
@@ -2980,6 +2980,8 @@ int hot_spare_screen(i_container *i_con)
 **/
 int raid_screen(i_container *i_con)
 {
+	free_raid_cmds();
+
 	return display_features_menu(i_con, &n_raid_screen);
 }
 
-- 
1.6.0.2

On 9/13/19 9:17 PM, Brahadambal Srinivasan wrote:
> New drives that default to reporting descriptor format sense data,
> do not have support for this in iprutils and it makes it impossible
> to use the drives. This patch adds code to convert the sense data
> to the format that user expects.

Applied.

Thanks!

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

New drives that default to reporting descriptor format sense data,
do not have support for this in iprutils and it makes it impossible
to use the drives. This patch adds code to convert the sense data
to the format that user expects.

Signed-off-by: Brahadambal Srinivasan <la...@li...>
Reviewed-by: Rick Lindsley <ric...@li...>
Reported-by: Timothy O'Callaghan <tj...@us...>
---
 iprlib.c | 29 +++++++++++++++++++++++++++--
 iprlib.h | 11 +++++++++++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/iprlib.c b/iprlib.c
index 516f069..e3b49b8 100644
--- a/iprlib.c
+++ b/iprlib.c
@@ -5291,6 +5291,8 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
 	int buff_len, segment_size;
 	void *dxferp;
 	u8 *buf;
+	struct sense_data_t sd;
+	struct df_sense_data_t *dfsdp = NULL;
 
 	/* check if scatter gather should be used */
 	if (xfer_len > IPR_MAX_XFER) {
@@ -5320,7 +5322,7 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
 
 	for (i = 0; i < (retries + 1); i++) {
 		memset(&io_hdr_t, 0, sizeof(io_hdr_t));
-		memset(sense_data, 0, sizeof(struct sense_data_t));
+		memset(&sd, 0, sizeof(struct sense_data_t));
 
 		io_hdr_t.interface_id = 'S';
 		io_hdr_t.cmd_len = cdb_size[(cdb[0] >> 5) & 0x7];
@@ -5328,7 +5330,7 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
 		io_hdr_t.flags = 0;
 		io_hdr_t.pack_id = 0;
 		io_hdr_t.usr_ptr = 0;
-		io_hdr_t.sbp = (unsigned char *)sense_data;
+		io_hdr_t.sbp = (unsigned char *)&sd;
 		io_hdr_t.mx_sb_len = sizeof(struct sense_data_t);
 		io_hdr_t.timeout = timeout_in_sec * 1000;
 		io_hdr_t.cmdp = cdb;
@@ -5352,6 +5354,29 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
 			break;
 	}
 
+	memset(sense_data, 0, sizeof(struct sense_data_t));
+
+	if (((sd.error_code & 0x7F) == 0x72) || ((sd.error_code & 0x7F) == 0x73)) {
+		dfsdp = (struct df_sense_data_t *)&sd;
+		/* change error_codes 0x72 to 0x70 and 0x73 to 0x71 */
+		sense_data->error_code = dfsdp->error_code & 0xFD;
+
+		/* Do not change the order of the next two assignments 
+ 		 * In the same u8, the 4th bit of fixed format corresponds 
+ 		 * to SDAT_OVLF and the last 4 bits to sense_key.
+ 		 */
+		sense_data->sense_key = dfsdp->sense_key & 0x0F;
+		if (dfsdp->rfield & 0x80)
+			sense_data->sense_key |= 0x10;
+
+		/* copy the other values */
+		sense_data->add_sense_code = dfsdp->add_sense_code;
+		sense_data->add_sense_code_qual = dfsdp->add_sense_code_qual;
+		sense_data->add_sense_len = 0; 
+	} else if(sd.error_code & 0x7F) {
+		memcpy(sense_data, &sd, sizeof(struct sense_data_t));
+	}
+
 out:
 	if (iovec_count) {
 		for (i = 0, buf = (u8 *)data; i < iovec_count; i++) {
diff --git a/iprlib.h b/iprlib.h
index aa07989..ebd1c81 100644
--- a/iprlib.h
+++ b/iprlib.h
@@ -1727,6 +1727,17 @@ struct sense_data_t {
 	u8 add_sense_bytes[0];
 };
 
+/* Structure for Descriptive Format Sense Data */
+struct df_sense_data_t {
+	u8 error_code;
+	u8 sense_key;
+	u8 add_sense_code;
+	u8 add_sense_code_qual;
+	u8 rfield;
+	u8 rsrvd[2];
+	u8 add_sense_len;
+};
+
 struct ipr_ioa_mode_page {
 	struct ipr_mode_page_hdr hdr;
 	u8 reserved;
-- 
2.19.1

Thanks Latha! Looks good. A few minor comments below.

On 9/13/19 9:48 AM, Brahadambal Srinivasan wrote:
> New drives that default to reporting descriptor format sense data,
> do not have support for this in iprutils and it makes it impossible
> to use the drives. This patch adds code to convert the sense data
> to the format that user expects.
> 
> Signed-off-by: Brahadambal Srinivasan <la...@li...>
> Signed-off-by: Brian King <br...@li...>

My signed off should be removed from the patch submission and I
will add it when committing the patch.

> Reviewed-by: Rick Lindsley <ric...@li...>
> Reported-by: Timothy O'Callaghan <tj...@us...>
> 
> ---
>  iprlib.c | 31 +++++++++++++++++++++++++++++--
>  iprlib.h | 11 +++++++++++
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/iprlib.c b/iprlib.c
> index 516f069..032fe1a 100644
> --- a/iprlib.c
> +++ b/iprlib.c
> @@ -16,6 +16,8 @@
>  #ifndef iprlib_h
>  #include "iprlib.h"
>  #endif
> +#include <stdio.h>
> +#include <stdlib.h>

Why are these needed? Can they be removed?

> @@ -5352,6 +5356,29 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
>  			break;
>  	}
> 
> +	memset(sense_data, 0, sizeof(struct sense_data_t));
> +
> +	if (((sd.error_code & 0x7F) == 0x72) || ((sd.error_code & 0x7F) == 0x73)) {
> +		dfsdp = (struct df_sense_data_t *)&sd;
> +		/* change error_codes 0x72 to 0x70 and 0x73 to 0x71 */
> +		sense_data->error_code = dfsdp->error_code & 0xFD;
> +
> +		/* Do not change the order of the next two assignments 
> + 		 * In the same u8, bit 4 of the fixed format corresponds 
> + 		 * to SDAT_OVLF and the last 4 bits to sense_key.
> + 		 */
> +		sense_data->sense_key = dfsdp->sense_key & 0x0F;
> +		if (dfsdp->rfield & 0x80)
> +			sense_data->sense_key |= 0x10;
> +
> +		/* copy the other values */
> +		sense_data->add_sense_code = dfsdp->add_sense_code;
> +		sense_data->add_sense_code_qual = dfsdp->add_sense_code_qual;
> +		sense_data->add_sense_len = dfsdp->add_sense_len;

I would suggest leaving the add_sense_len to zero here. There is no additional
sense data you are copying over and the format would be different anyway, so
you can't easily do that.

> +	} else if (((sd.error_code & 0x7F) == 0x70) || ((sd.error_code & 0x7F) == 0x71)) {
> +		memcpy((void *)sense_data, (void *)&sd, sizeof(struct sense_data_t)); 

Some trailing whitespace on the memcpy line. Also, you can probably remove the void * casts
here. Were you getting compile warnings without them? Also, you can probably just simplify
the check here to be something like:

else if (sd.error_code & 0x7F)

That way any vendor specific sense data doesn't get lost.

> +	}
> +
>  out:
>  	if (iovec_count) {
>  		for (i = 0, buf = (u8 *)data; i < iovec_count; i++) {



-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

New drives that default to reporting descriptor format sense data,
do not have support for this in iprutils and it makes it impossible
to use the drives. This patch adds code to convert the sense data
to the format that user expects.

Signed-off-by: Brahadambal Srinivasan <la...@li...>
Signed-off-by: Brian King <br...@li...>
Reviewed-by: Rick Lindsley <ric...@li...>
Reported-by: Timothy O'Callaghan <tj...@us...>

---
 iprlib.c | 31 +++++++++++++++++++++++++++++--
 iprlib.h | 11 +++++++++++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/iprlib.c b/iprlib.c
index 516f069..032fe1a 100644
--- a/iprlib.c
+++ b/iprlib.c
@@ -16,6 +16,8 @@
 #ifndef iprlib_h
 #include "iprlib.h"
 #endif
+#include <stdio.h>
+#include <stdlib.h>
 
 static void default_exit_func()
 {
@@ -5291,6 +5293,8 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
 	int buff_len, segment_size;
 	void *dxferp;
 	u8 *buf;
+	struct sense_data_t sd;
+	struct df_sense_data_t *dfsdp = NULL;
 
 	/* check if scatter gather should be used */
 	if (xfer_len > IPR_MAX_XFER) {
@@ -5320,7 +5324,7 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
 
 	for (i = 0; i < (retries + 1); i++) {
 		memset(&io_hdr_t, 0, sizeof(io_hdr_t));
-		memset(sense_data, 0, sizeof(struct sense_data_t));
+		memset(&sd, 0, sizeof(struct sense_data_t));
 
 		io_hdr_t.interface_id = 'S';
 		io_hdr_t.cmd_len = cdb_size[(cdb[0] >> 5) & 0x7];
@@ -5328,7 +5332,7 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
 		io_hdr_t.flags = 0;
 		io_hdr_t.pack_id = 0;
 		io_hdr_t.usr_ptr = 0;
-		io_hdr_t.sbp = (unsigned char *)sense_data;
+		io_hdr_t.sbp = (unsigned char *)&sd;
 		io_hdr_t.mx_sb_len = sizeof(struct sense_data_t);
 		io_hdr_t.timeout = timeout_in_sec * 1000;
 		io_hdr_t.cmdp = cdb;
@@ -5352,6 +5356,29 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
 			break;
 	}
 
+	memset(sense_data, 0, sizeof(struct sense_data_t));
+
+	if (((sd.error_code & 0x7F) == 0x72) || ((sd.error_code & 0x7F) == 0x73)) {
+		dfsdp = (struct df_sense_data_t *)&sd;
+		/* change error_codes 0x72 to 0x70 and 0x73 to 0x71 */
+		sense_data->error_code = dfsdp->error_code & 0xFD;
+
+		/* Do not change the order of the next two assignments 
+ 		 * In the same u8, bit 4 of the fixed format corresponds 
+ 		 * to SDAT_OVLF and the last 4 bits to sense_key.
+ 		 */
+		sense_data->sense_key = dfsdp->sense_key & 0x0F;
+		if (dfsdp->rfield & 0x80)
+			sense_data->sense_key |= 0x10;
+
+		/* copy the other values */
+		sense_data->add_sense_code = dfsdp->add_sense_code;
+		sense_data->add_sense_code_qual = dfsdp->add_sense_code_qual;
+		sense_data->add_sense_len = dfsdp->add_sense_len;
+	} else if (((sd.error_code & 0x7F) == 0x70) || ((sd.error_code & 0x7F) == 0x71)) {
+		memcpy((void *)sense_data, (void *)&sd, sizeof(struct sense_data_t)); 
+	}
+
 out:
 	if (iovec_count) {
 		for (i = 0, buf = (u8 *)data; i < iovec_count; i++) {
diff --git a/iprlib.h b/iprlib.h
index aa07989..ebd1c81 100644
--- a/iprlib.h
+++ b/iprlib.h
@@ -1727,6 +1727,17 @@ struct sense_data_t {
 	u8 add_sense_bytes[0];
 };
 
+/* Structure for Descriptive Format Sense Data */
+struct df_sense_data_t {
+	u8 error_code;
+	u8 sense_key;
+	u8 add_sense_code;
+	u8 add_sense_code_qual;
+	u8 rfield;
+	u8 rsrvd[2];
+	u8 add_sense_len;
+};
+
 struct ipr_ioa_mode_page {
 	struct ipr_mode_page_hdr hdr;
 	u8 reserved;
-- 
1.8.3.1

When formatting a disk from JBOD to RAID, we unbind the disk
from the sd driver, including the remote path, in the case of
dual adapters, and flush unused multipaths. However, due to
a bug, this code wasn't actually working once the SCSI
host, bus, or target exceeded a single character.

Signed-off-by: Brian King <br...@li...>
---
 iprlib.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/iprlib.c b/iprlib.c
index 4d3ab5f..516f069 100644
--- a/iprlib.c
+++ b/iprlib.c
@@ -10345,7 +10345,8 @@ int ipr_jbod_sysfs_bind(struct ipr_dev *dev, u8 op)
 	char *sysfs_device_name;
 
 	sysfs_device_name = dev->scsi_dev_data->sysfs_device_name;
-	size = strnlen(sysfs_device_name, sizeof(sysfs_device_name));
+	size = strnlen(sysfs_device_name,
+		       sizeof(dev->scsi_dev_data->sysfs_device_name));
 
 	if (op == IPR_JBOD_SYSFS_BIND) {
 		fd = open("/sys/bus/scsi/drivers/sd/bind", O_WRONLY);
@@ -10367,7 +10368,8 @@ int ipr_jbod_sysfs_bind(struct ipr_dev *dev, u8 op)
 	mp_dev = find_multipath_jbod(dev);
 	if (mp_dev) {
 		sysfs_device_name = mp_dev->scsi_dev_data->sysfs_device_name;
-		size = strnlen(sysfs_device_name, sizeof(sysfs_device_name));
+		size = strnlen(sysfs_device_name,
+			       sizeof(mp_dev->scsi_dev_data->sysfs_device_name));
 		rc = write(fd, sysfs_device_name, size);
 		if (rc < 0) {
 			close(fd);
-- 
1.8.3.1

These two patches fix up some hang conditions that have been
seen when formatting drives for RAID function.

Brian King (2):
  iprutils: Fix driver unbind on format
  iprutils: Fix format for RAID hang

 iprconfig.c | 2 +-
 iprlib.c    | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
1.8.3.1

After completing all device formats when formatting disks for
RAID, we wait until the drives get converted from JBOD
resources to RAID device resources. We wait for 30 seconds for
this to complete. However, due to a bug, we ended up waiting here
forever. This ensures we timeout after 30 seconds.

Signed-off-by: Brian King <br...@li...>
---
 iprconfig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/iprconfig.c b/iprconfig.c
index 6a5fd21..5f31beb 100644
--- a/iprconfig.c
+++ b/iprconfig.c
@@ -232,7 +232,7 @@ static int wait_for_formatted_af_dasd(int timeout_in_secs)
 	int num_devs, j, af_found, jbod2af_formats, num_secs;
 	u64 device_id;
 
-	for (num_secs = 0; num_secs < timeout_in_secs; timeout_in_secs++) {
+	for (num_secs = 0; num_secs < timeout_in_secs; num_secs++) {
 		af_found = 0;
 		jbod2af_formats = 0;
 		scsi_devs = NULL;
-- 
1.8.3.1

On 05/25/2018 09:00 AM, wen...@li... wrote:
> From: Wen Xiong <wen...@li...>
> 
> This patch adds formatting error overlay 0x41 to improve debug
> capabilities.
> 
> Signed-off-by: Wen Xiong <wen...@li...>
> ---

Acked-by: Brian King <br...@li...>


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

From: Wen Xiong <wen...@li...>

This patch adds formatting error overlay 0x41 to improve debug
capabilities.

Signed-off-by: Wen Xiong <wen...@li...>
---
 drivers/scsi/ipr.c |   25 +++++++++++++++++++++++++
 drivers/scsi/ipr.h |    7 +++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 6615ad8..d51eb71 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -2413,6 +2413,28 @@ static void ipr_log_sis64_fabric_error(struct ipr_ioa_cfg *ioa_cfg,
 }
 
 /**
+ * ipr_log_sis64_service_required_error - Log a sis64 service required error.
+ * @ioa_cfg:    ioa config struct
+ * @hostrcb:    hostrcb struct
+ *
+ * Return value:
+ *      none
+ **/
+static void ipr_log_sis64_service_required_error(struct ipr_ioa_cfg *ioa_cfg,
+				       struct ipr_hostrcb *hostrcb)
+{
+	struct ipr_hostrcb_type_41_error *error;
+
+	error = &hostrcb->hcam.u.error64.u.type_41_error;
+
+	error->failure_reason[sizeof(error->failure_reason) - 1] = '\0';
+	ipr_err("Primary Failure Reason: %s\n", error->failure_reason);
+	ipr_log_hex_data(ioa_cfg, error->data,
+			 be32_to_cpu(hostrcb->hcam.length) -
+			 (offsetof(struct ipr_hostrcb_error, u) +
+			  offsetof(struct ipr_hostrcb_type_41_error, data)));
+}
+/**
  * ipr_log_generic_error - Log an adapter error.
  * @ioa_cfg:	ioa config struct
  * @hostrcb:	hostrcb struct
@@ -2587,6 +2609,9 @@ static void ipr_handle_log_data(struct ipr_ioa_cfg *ioa_cfg,
 	case IPR_HOST_RCB_OVERLAY_ID_30:
 		ipr_log_sis64_fabric_error(ioa_cfg, hostrcb);
 		break;
+	case IPR_HOST_RCB_OVERLAY_ID_41:
+		ipr_log_sis64_service_required_error(ioa_cfg, hostrcb);
+		break;
 	case IPR_HOST_RCB_OVERLAY_ID_1:
 	case IPR_HOST_RCB_OVERLAY_ID_DEFAULT:
 	default:
diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
index 9357073..68afbbd 100644
--- a/drivers/scsi/ipr.h
+++ b/drivers/scsi/ipr.h
@@ -1135,6 +1135,11 @@ struct ipr_hostrcb_type_30_error {
 	struct ipr_hostrcb64_fabric_desc desc[1];
 }__attribute__((packed, aligned (4)));
 
+struct ipr_hostrcb_type_41_error {
+	u8 failure_reason[64];
+	 __be32 data[200];
+}__attribute__((packed, aligned (4)));
+
 struct ipr_hostrcb_error {
 	__be32 fd_ioasc;
 	struct ipr_res_addr fd_res_addr;
@@ -1173,6 +1178,7 @@ struct ipr_hostrcb64_error {
 		struct ipr_hostrcb_type_23_error type_23_error;
 		struct ipr_hostrcb_type_24_error type_24_error;
 		struct ipr_hostrcb_type_30_error type_30_error;
+		struct ipr_hostrcb_type_41_error type_41_error;
 	} u;
 }__attribute__((packed, aligned (8)));
 
@@ -1218,6 +1224,7 @@ struct ipr_hcam {
 #define IPR_HOST_RCB_OVERLAY_ID_24				0x24
 #define IPR_HOST_RCB_OVERLAY_ID_26				0x26
 #define IPR_HOST_RCB_OVERLAY_ID_30				0x30
+#define IPR_HOST_RCB_OVERLAY_ID_41				0x41
 #define IPR_HOST_RCB_OVERLAY_ID_DEFAULT				0xFF
 
 	u8 reserved1[3];
-- 
1.6.0.2

From: Wen Xiong <wen...@li...>

This patch adds new adapter error log for P9 system with the new
AZ SAS cable.

Signed-off-by: Wen Xiong <wen...@li...>
Acked-by: Brian King <br...@li...>

---
 drivers/scsi/ipr.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index dda1a64..6615ad8 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -435,6 +435,8 @@ struct ipr_error_table_t ipr_error_table[] = {
 	"4080: IOA exceeded maximum operating temperature"},
 	{0x060B8000, 0, IPR_DEFAULT_LOG_LEVEL,
 	"4085: Service required"},
+	{0x060B8100, 0, IPR_DEFAULT_LOG_LEVEL,
+	"4086: SAS Adapter Hardware Configuration Error"},
 	{0x06288000, 0, IPR_DEFAULT_LOG_LEVEL,
 	"3140: Device bus not ready to ready transition"},
 	{0x06290000, 0, IPR_DEFAULT_LOG_LEVEL,
-- 
1.6.0.2

While attempting to further optimize show-details performance,
it was observed that get_drive_phy_loc was iterating over all
ioa's, even though a specific ioa is passed into the function.
On large configurations, this resulted in show-details taking
longer than 5 seconds. If we remove this redundant code, show-details
will now complete in less than 1 second in a similar configuration.

Signed-off-by: Brian King <br...@li...>
---
 iprconfig.c | 94 ++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 46 insertions(+), 48 deletions(-)

diff --git a/iprconfig.c b/iprconfig.c
index 9f4f61f..6a5fd21 100644
--- a/iprconfig.c
+++ b/iprconfig.c
@@ -17780,66 +17780,64 @@ static int get_drive_phy_loc(struct ipr_ioa *ioa)
 	char phy_loc[PHYSICAL_LOCATION_LENGTH + 1];
 	int times, index;
 
-	for_each_ioa(ioa)  {
-		is_spi = ioa_is_spi(ioa);
+	is_spi = ioa_is_spi(ioa);
 
-		for_each_hotplug_dev(ioa, dev) {
-			if (ioa->sis64)
-				get_res_path(dev);
-			else
-				get_res_addrs(dev);
-		}
+	for_each_hotplug_dev(ioa, dev) {
+		if (ioa->sis64)
+			get_res_path(dev);
+		else
+			get_res_addrs(dev);
+	}
 
-		for_each_ses(ioa, ses) {
-			times = 5;
-			if (strlen(ses->physical_location) == 0)
-				get_ses_phy_loc(ses);
-			while (times--) {
-				if (!ipr_receive_diagnostics(ses, 2, &ses_data, sizeof(ses_data)))
-					break;
-			}
-			if (times < 0 ) continue;
+	for_each_ses(ioa, ses) {
+		times = 5;
+		if (strlen(ses->physical_location) == 0)
+			get_ses_phy_loc(ses);
+		while (times--) {
+			if (!ipr_receive_diagnostics(ses, 2, &ses_data, sizeof(ses_data)))
+				break;
+		}
+		if (times < 0 ) continue;
 
-			if (ipr_receive_diagnostics(ses, 1, &ses_cfg, sizeof(ses_cfg)))
-				continue;
+		if (ipr_receive_diagnostics(ses, 1, &ses_cfg, sizeof(ses_cfg)))
+			continue;
 
-			if (ipr_receive_diagnostics(ses, 7, &drive_data, sizeof(drive_data)))
-				continue;
+		if (ipr_receive_diagnostics(ses, 7, &drive_data, sizeof(drive_data)))
+			continue;
 
-			overall = ipr_get_overall_elem(&ses_data, &ses_cfg);
-			ses_bus = ses->scsi_dev_data->channel;
-			scsi_id_found = 0;
+		overall = ipr_get_overall_elem(&ses_data, &ses_cfg);
+		ses_bus = ses->scsi_dev_data->channel;
+		scsi_id_found = 0;
 
-			if (!is_spi && (overall->device_environment == 0))
-				is_vses = 1;
-			else
-				is_vses = 0;
+		if (!is_spi && (overall->device_environment == 0))
+			is_vses = 1;
+		else
+			is_vses = 0;
 
-			scsi_dbg(ses, "%s\n", is_vses ? "Found VSES" : "Found real SES");
+		scsi_dbg(ses, "%s\n", is_vses ? "Found VSES" : "Found real SES");
 
-			for_each_elem_status(elem_status, &ses_data, &ses_cfg) {
-				index = index_in_page2(&ses_data, elem_status->slot_id);
-				if (index != -1)
-					get_drive_phy_loc_with_ses_phy_loc(ses, &drive_data, index, phy_loc, 0);
-
-				if (elem_status->status == IPR_DRIVE_ELEM_STATUS_UNSUPP)
-					continue;
-				if (elem_status->status == IPR_DRIVE_ELEM_STATUS_NO_ACCESS)
-					continue;
-				if (is_spi && (scsi_id_found & (1 << elem_status->slot_id)))
-					continue;
-				scsi_id_found |= (1 << elem_status->slot_id);
+		for_each_elem_status(elem_status, &ses_data, &ses_cfg) {
+			index = index_in_page2(&ses_data, elem_status->slot_id);
+			if (index != -1)
+				get_drive_phy_loc_with_ses_phy_loc(ses, &drive_data, index, phy_loc, 0);
 
-				if (ioa->sis64)
-					dev = get_dev_for_slot_64bit(ses, elem_status->slot_id, phy_loc);
-				else
-					dev = get_dev_for_slot(ses, elem_status->slot_id, is_vses, phy_loc);
-				if (!dev)
-					continue;
+			if (elem_status->status == IPR_DRIVE_ELEM_STATUS_UNSUPP)
+				continue;
+			if (elem_status->status == IPR_DRIVE_ELEM_STATUS_NO_ACCESS)
+				continue;
+			if (is_spi && (scsi_id_found & (1 << elem_status->slot_id)))
+				continue;
+			scsi_id_found |= (1 << elem_status->slot_id);
 
-			}
+			if (ioa->sis64)
+				dev = get_dev_for_slot_64bit(ses, elem_status->slot_id, phy_loc);
+			else
+				dev = get_dev_for_slot(ses, elem_status->slot_id, is_vses, phy_loc);
+			if (!dev)
+				continue;
 
 		}
+
 	}
 	return 0;
 }
-- 
1.8.3.1

On 04/25/2018 05:40 PM, wen...@li... wrote:
> From: Wen Xiong <wen...@li...>
> 
> This patch adds new adapter error log for P9 system with the new
> AZ SAS cable.
> ---
>  drivers/scsi/ipr.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index dda1a64..6615ad8 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -435,6 +435,8 @@ struct ipr_error_table_t ipr_error_table[] = {
>  	"4080: IOA exceeded maximum operating temperature"},
>  	{0x060B8000, 0, IPR_DEFAULT_LOG_LEVEL,
>  	"4085: Service required"},
> +	{0x060B8100, 0, IPR_DEFAULT_LOG_LEVEL,
> +	"4086: SAS Adapter Hardware Configuration Error"},
>  	{0x06288000, 0, IPR_DEFAULT_LOG_LEVEL,
>  	"3140: Device bus not ready to ready transition"},
>  	{0x06290000, 0, IPR_DEFAULT_LOG_LEVEL,
> 

Acked-by: Brian King <br...@li...>

Please go ahead and send to linux-scsi

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

From: Wen Xiong <wen...@li...>

This patch adds new adapter error log for P9 system with the new
AZ SAS cable.
---
 drivers/scsi/ipr.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index dda1a64..6615ad8 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -435,6 +435,8 @@ struct ipr_error_table_t ipr_error_table[] = {
 	"4080: IOA exceeded maximum operating temperature"},
 	{0x060B8000, 0, IPR_DEFAULT_LOG_LEVEL,
 	"4085: Service required"},
+	{0x060B8100, 0, IPR_DEFAULT_LOG_LEVEL,
+	"4086: SAS Adapter Hardware Configuration Error"},
 	{0x06288000, 0, IPR_DEFAULT_LOG_LEVEL,
 	"3140: Device bus not ready to ready transition"},
 	{0x06290000, 0, IPR_DEFAULT_LOG_LEVEL,
-- 
1.6.0.2

Applied.

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

From: Wen Xiong <wen...@li...>

When creating the arrays with RI and non-RI ssd disks, this patch
improves the error message in GUI and command line.

Signed-off-by: Wen Xiong <wen...@li...>
---
 iprconfig.c |   27 ++++++++++++++++++++++++++-
 iprconfig.h |    2 ++
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/iprconfig.c b/iprconfig.c
index 5da12cc..7f37387 100644
--- a/iprconfig.c
+++ b/iprconfig.c
@@ -4102,6 +4102,7 @@ int confirm_raid_start(i_container *i_con)
 	int header_lines;
 	int toggle = 0;
 	struct screen_output *s_out;
+	int is_ri_count = 0, non_ri_count = 0;
 
 	rc = RC_SUCCESS;
 
@@ -4113,12 +4114,24 @@ int confirm_raid_start(i_container *i_con)
 			print_dev(k, cur_raid_cmd->dev, buffer, "1", k);
 
 			for_each_af_dasd(ioa, dev) {
-				if (dev->dev_rcd->issue_cmd)
+				if (dev->dev_rcd->issue_cmd) {
 					print_dev(k, dev, buffer, "1", k);
+					if (dev->block_dev_class & IPR_SSD) {
+						if (dev->read_intensive & IPR_RI)
+							is_ri_count++;
+                   				else
+							non_ri_count++;
+					}
+				}
 			}
 		}
 	}
 
+	if (is_ri_count > 0 && non_ri_count > 0) {
+		rc = RC_95_Mixed_RISSD_SSDs;
+		return rc;
+	}
+
 	toggle_field = 0;
 
 	do {
@@ -14115,6 +14128,7 @@ static int raid_create(char **args, int num_args)
 {
 	int i, num_devs = 0, ssd_num_devs = 0, rc, prot_level;
 	int non_4k_count = 0, is_4k_count = 0;
+	int non_ri_count = 0, is_ri_count = 0;
 	int next_raid_level, next_stripe_size, next_qdepth, next_label;
 	char *raid_level = IPR_DEFAULT_RAID_LVL;
 	char label[8];
@@ -14210,6 +14224,12 @@ static int raid_create(char **args, int num_args)
 			is_4k_count++;
 		else
 			non_4k_count++;
+
+		if (dev->block_dev_class & IPR_SSD)
+			if (dev->read_intensive & IPR_RI)
+				is_ri_count++;
+			else
+				non_ri_count++;
 	}
 
 	if (is_4k_count > 0 && non_4k_count > 0) {
@@ -14217,6 +14237,11 @@ static int raid_create(char **args, int num_args)
 		return -EINVAL;
 	}
 
+	if (is_ri_count > 0 && non_ri_count > 0) {
+		syslog(LOG_ERR, _("SSD disks and RI SSD disks can not be mixed in an array.\n"));
+		return -EINVAL;
+	}
+
 	if (!ioa) {
 		syslog(LOG_ERR, _("No valid devices specified.\n"));
 		return -EINVAL;
diff --git a/iprconfig.h b/iprconfig.h
index a6f6e75..ff6dd02 100644
--- a/iprconfig.h
+++ b/iprconfig.h
@@ -1717,6 +1717,7 @@ const char *screen_status[] = {
 	/* 92 */ __("Create disk array failed - can not build with read intensive disks only."),
 	/* 93 */ __("All devices up to date"),
 	/* 94 */ __("Temporary log file creation failed: %s"),
+	/* 95 */ __("Create disk array failed - can not mix SSDs and RI SSDs."),
 
       /* NOTE:  127 maximum limit */
 };
@@ -1818,6 +1819,7 @@ enum {
 	RC_92_UNSUPT_REQ_BLK_DEV_CLASS,
 	RC_93_All_Up_To_Date,
 	RC_94_Tmp_Log_Fail,
+	RC_95_Mixed_RISSD_SSDs,
 
 	/* NOTE:  127 maximum limit */
 };
-- 
1.6.0.2

Probably need to check for P8 and P9.

Thanks,
Wendy


From:	Brian King <br...@li...>
To:	wen...@li..., ipr...@li...
Cc:	Wen Xiong/Austin/IBM@IBMUS
Date:	03/15/2018 01:44 PM
Subject:	Re: [Iprdd-devel] [PATCH] iprutils: Remove the Analyze Log in
            main menu



On 03/14/2018 03:28 PM, wen...@li... wrote:
> From: Wen Xiong <wen...@li...>
>
> iprconfig Analyze log is broken in most distros are moving towards
> using journalctl for viewing the system logs. This patch removes
> the Analze log in iprconfig.

This looks good. Thanks for doing this. Prior to getting this into a
release we'll need to update the reference guide to remove reference
to this option. Can you look into that?

Thanks,

Brian

--
Brian King
Power Linux I/O
IBM Linux Technology Center

On 03/14/2018 03:28 PM, wen...@li... wrote:
> From: Wen Xiong <wen...@li...>
> 
> iprconfig Analyze log is broken in most distros are moving towards
> using journalctl for viewing the system logs. This patch removes
> the Analze log in iprconfig.

This looks good. Thanks for doing this. Prior to getting this into a
release we'll need to update the reference guide to remove reference
to this option. Can you look into that?

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center