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

[Don Brace]

On 12/18/2015 02:19 PM, Matthew R. Ochs wrote:

Hi Don,

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


-matt


On Dec 9, 2015, at 5:41 PM, Matthew R. Ochs  wrote:


On Dec 9, 2015, at 3:18 PM, Don Brace  wrote:

No commit message?

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


Reviewed-by: Justin Lindley 
Reviewed-by: Kevin Barnett 
Reviewed-by: Scott Teel 
Signed-off-by: Don Brace 
---
drivers/scsi/hpsa.c |  108 ---
drivers/scsi/hpsa_cmd.h |   13 ++
2 files changed, 114 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index f8370b8..6f84ec7 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -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)) {

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.



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", ...



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 = >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(>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.



+
+   memset(id_phys, 0, sizeof(*id_phys));

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

Another good catch.



+   rc = hpsa_bmic_id_physical_device(h, scsi3addr, bmic_device_index,
+   id_phys, sizeof(*id_phys));
+   if (rc) {
+   dev_warn(>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 

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

[Matthew R. Ochs]

Hi Don,

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


-matt

> On Dec 9, 2015, at 5:41 PM, Matthew R. Ochs  wrote:
> 
>> On Dec 9, 2015, at 3:18 PM, Don Brace  wrote:
> 
> No commit message?

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

> 
>> 
>> Reviewed-by: Justin Lindley 
>> Reviewed-by: Kevin Barnett 
>> Reviewed-by: Scott Teel 
>> Signed-off-by: Don Brace 
>> ---
>> drivers/scsi/hpsa.c |  108 
>> ---
>> drivers/scsi/hpsa_cmd.h |   13 ++
>> 2 files changed, 114 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
>> index f8370b8..6f84ec7 100644
>> --- a/drivers/scsi/hpsa.c
>> +++ b/drivers/scsi/hpsa.c
>> @@ -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)) {
> 
> How does removing the check for external here relate to the rest of this 
> commit?
> 
>>  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.
> 
>>  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 = >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(>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?
> 
>> +
>> +memset(id_phys, 0, sizeof(*id_phys));
> 
> Didn't you just obtain zeroed memory from kzalloc()?
> 
>> +rc = hpsa_bmic_id_physical_device(h, scsi3addr, bmic_device_index,
>> +id_phys, sizeof(*id_phys));
>> +if (rc) {
>> +dev_warn(>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;
>> +}

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

[Matthew R. Ochs]

> On Dec 9, 2015, at 3:18 PM, Don Brace  wrote:

No commit message?

> 
> Reviewed-by: Justin Lindley 
> Reviewed-by: Kevin Barnett 
> Reviewed-by: Scott Teel 
> Signed-off-by: Don Brace 
> ---
> drivers/scsi/hpsa.c |  108 ---
> drivers/scsi/hpsa_cmd.h |   13 ++
> 2 files changed, 114 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index f8370b8..6f84ec7 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -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)) {

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

>   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.

>   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 = >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(>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?

> +
> + memset(id_phys, 0, sizeof(*id_phys));

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

> + rc = hpsa_bmic_id_physical_device(h, scsi3addr, bmic_device_index,
> + id_phys, sizeof(*id_phys));
> + if (rc) {
> + dev_warn(>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(_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)
>