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 <mro...@linux.vnet.ibm.com> wrote:
>
>> On Dec 9, 2015, at 3:18 PM, Don Brace <don.br...@pmcs.com> wrote:
>
> No commit message?
You can disregard this one as I saw you added a message in v1.
>
>>
>> Reviewed-by: Justin Lindley <justin.lind...@pmcs.com>
>> Reviewed-by: Kevin Barnett <kevin.barn...@pmcs.com>
>> Reviewed-by: Scott Teel <scott.t...@pmcs.com>
>> Signed-off-by: Don Brace <don.br...@pmcs.com>
>> ---
>> 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 = &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;
>
> 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(&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:
>
> Is it 'okay' that these two types are falling through and will call this new
> enclosure info routine?
>
>> 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.
>
>> 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 =
>> diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
>> index d92ef0d..6a919ad 100644
>> --- a/drivers/scsi/hpsa_cmd.h
>> +++ b/drivers/scsi/hpsa_cmd.h
>> @@ -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 majord...@vger.kernel.org
>> 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 majord...@vger.kernel.org
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
