Re: [PATCH 1 13/25] hpsa: simplify update scsi devices

[Hannes Reinecke]

On 10/28/2015 11:05 PM, Don Brace wrote:
> From: Kevin Barnett 
> 
> remove repeated calculation that checks for physical
> or logical devices.
> 
> Reviewed-by: Scott Teel 
> Reviewed-by: Justin Lindley 
> Reviewed-by: Kevin Barnett 
> Signed-off-by: Don Brace 
> ---
>  drivers/scsi/hpsa.c |   23 ++-
>  drivers/scsi/hpsa.h |1 +
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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

Re: [PATCH 1 13/25] hpsa: simplify update scsi devices

[Tomas Henzl]

On 28.10.2015 23:05, Don Brace wrote:
> From: Kevin Barnett 
>
> remove repeated calculation that checks for physical
> or logical devices.
>
> Reviewed-by: Scott Teel 
> Reviewed-by: Justin Lindley 
> Reviewed-by: Kevin Barnett 
> Signed-off-by: Don Brace 

Reviewed-by: Tomas Henzl 

Tomas

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

Re: [PATCH 1 13/25] hpsa: simplify update scsi devices

[Matthew R. Ochs]

> On Oct 28, 2015, at 5:05 PM, Don Brace  wrote:
> 
> From: Kevin Barnett 
> 
> remove repeated calculation that checks for physical
> or logical devices.
> 
> Reviewed-by: Scott Teel 
> Reviewed-by: Justin Lindley 
> Reviewed-by: Kevin Barnett 
> Signed-off-by: Don Brace 
> ---
> drivers/scsi/hpsa.c |   23 ++-
> drivers/scsi/hpsa.h |1 +
> 2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index d011540..7c1a552 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -3761,6 +3761,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info 
> *h, int hostno)
>   int ncurrent = 0;
>   int i, n_ext_target_devs, ndevs_to_allocate;
>   int raid_ctlr_position;
> + bool physical_device;

Any particular reason for using a bool here and a u8 when you cache the value?

>   DECLARE_BITMAP(lunzerobits, MAX_EXT_TARGETS);
> 
>   currentsd = kzalloc(sizeof(*currentsd) * HPSA_MAX_DEVICES, GFP_KERNEL);
> @@ -3821,16 +3822,17 @@ static void hpsa_update_scsi_devices(struct ctlr_info 
> *h, int hostno)
>   int rc = 0;
>   int phys_dev_index = i - (raid_ctlr_position == 0);
> 
> + physical_device = i < nphysicals + (raid_ctlr_position == 0);
> +
>   /* Figure out where the LUN ID info is coming from */
>   lunaddrbytes = figure_lunaddrbytes(h, raid_ctlr_position,
>   i, nphysicals, nlogicals, physdev_list, logdev_list);
> 
>   /* skip masked non-disk devices */
> - if (MASKED_DEVICE(lunaddrbytes))
> - if (i < nphysicals + (raid_ctlr_position == 0) &&
> - (physdev_list->
> - LUN[phys_dev_index].device_flags & 0x01))
> - continue;
> + if (physical_device &&
> + MASKED_DEVICE(lunaddrbytes) &&
> + (physdev_list->LUN[phys_dev_index].device_flags & 0x01))
> + continue;

In this conditional you swapped the ordering, evaluating physical_device first, 
why?

> 
>   /* Get device type, vendor, model, device id */
>   rc = hpsa_update_device_info(h, lunaddrbytes, tmpdevice,
> @@ -3866,10 +3868,13 @@ static void hpsa_update_scsi_devices(struct ctlr_info 
> *h, int hostno)
>   }
> 
>   *this_device = *tmpdevice;
> + this_device->physical_device = physical_device;
> 
> - /* do not expose masked devices */
> - if (MASKED_DEVICE(lunaddrbytes) &&
> - i < nphysicals + (raid_ctlr_position == 0))
> + /*
> +  * Expose all devices except for physical devices that
> +  * are masked.
> +  */
> + if (MASKED_DEVICE(lunaddrbytes) && this_device->physical_device)
>   this_device->expose_device = 0;
>   else
>   this_device->expose_device = 1;
> @@ -3887,7 +3892,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info 
> *h, int hostno)
>   ncurrent++;
>   break;
>   case TYPE_DISK:
> - if (i < nphysicals + (raid_ctlr_position == 0)) {
> + if (this_device->physical_device) {
>   /* The disk is in HBA mode. */
>   /* Never use RAID mapper in HBA mode. */
>   this_device->offload_enabled = 0;
> diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
> index a6ead07..808b520 100644
> --- a/drivers/scsi/hpsa.h
> +++ b/drivers/scsi/hpsa.h
> @@ -37,6 +37,7 @@ struct hpsa_scsi_dev_t {
>   unsigned int devtype;
>   int bus, target, lun;   /* as presented to the OS */
>   unsigned char scsi3addr[8]; /* as presented to the HW */
> + u8 physical_device;
>   u8 expose_device;
> #define RAID_CTLR_LUNID "\0\0\0\0\0\0\0\0"
>   unsigned char device_id[16];/* from inquiry pg. 0x83 */
> 
> --
> 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

Re: [PATCH 1 13/25] hpsa: simplify update scsi devices

[Don Brace]

On 10/29/2015 11:43 AM, Matthew R. Ochs wrote:

On Oct 28, 2015, at 5:05 PM, Don Brace  wrote:

From: Kevin Barnett 

remove repeated calculation that checks for physical
or logical devices.

Reviewed-by: Scott Teel 
Reviewed-by: Justin Lindley 
Reviewed-by: Kevin Barnett 
Signed-off-by: Don Brace 
---
drivers/scsi/hpsa.c |   23 ++-
drivers/scsi/hpsa.h |1 +
2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index d011540..7c1a552 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3761,6 +3761,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h, 
int hostno)
int ncurrent = 0;
int i, n_ext_target_devs, ndevs_to_allocate;
int raid_ctlr_position;
+   bool physical_device;

Any particular reason for using a bool here and a u8 when you cache the value?

Changed definition to u8 physical_device : 1; in hpsa_scsi_dev_t




DECLARE_BITMAP(lunzerobits, MAX_EXT_TARGETS);

currentsd = kzalloc(sizeof(*currentsd) * HPSA_MAX_DEVICES, GFP_KERNEL);
@@ -3821,16 +3822,17 @@ static void hpsa_update_scsi_devices(struct ctlr_info 
*h, int hostno)
int rc = 0;
int phys_dev_index = i - (raid_ctlr_position == 0);

+   physical_device = i < nphysicals + (raid_ctlr_position == 0);
+
/* Figure out where the LUN ID info is coming from */
lunaddrbytes = figure_lunaddrbytes(h, raid_ctlr_position,
i, nphysicals, nlogicals, physdev_list, logdev_list);

/* skip masked non-disk devices */
-   if (MASKED_DEVICE(lunaddrbytes))
-   if (i < nphysicals + (raid_ctlr_position == 0) &&
-   (physdev_list->
-   LUN[phys_dev_index].device_flags & 0x01))
-   continue;
+   if (physical_device &&
+   MASKED_DEVICE(lunaddrbytes) &&
+   (physdev_list->LUN[phys_dev_index].device_flags & 0x01))
+   continue;

In this conditional you swapped the ordering, evaluating physical_device first, 
why?

Changed it back. Better to be consistent.



/* Get device type, vendor, model, device id */
rc = hpsa_update_device_info(h, lunaddrbytes, tmpdevice,
@@ -3866,10 +3868,13 @@ static void hpsa_update_scsi_devices(struct ctlr_info 
*h, int hostno)
}

*this_device = *tmpdevice;
+   this_device->physical_device = physical_device;

-   /* do not expose masked devices */
-   if (MASKED_DEVICE(lunaddrbytes) &&
-   i < nphysicals + (raid_ctlr_position == 0))
+   /*
+* Expose all devices except for physical devices that
+* are masked.
+*/
+   if (MASKED_DEVICE(lunaddrbytes) && this_device->physical_device)
this_device->expose_device = 0;
else
this_device->expose_device = 1;
@@ -3887,7 +3892,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h, 
int hostno)
ncurrent++;
break;
case TYPE_DISK:
-   if (i < nphysicals + (raid_ctlr_position == 0)) {
+   if (this_device->physical_device) {
/* The disk is in HBA mode. */
/* Never use RAID mapper in HBA mode. */
this_device->offload_enabled = 0;
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index a6ead07..808b520 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -37,6 +37,7 @@ struct hpsa_scsi_dev_t {
unsigned int devtype;
int bus, target, lun;   /* as presented to the OS */
unsigned char scsi3addr[8]; /* as presented to the HW */
+   u8 physical_device;
u8 expose_device;
#define RAID_CTLR_LUNID "\0\0\0\0\0\0\0\0"
unsigned char device_id[16];/* from inquiry pg. 0x83 */

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

Re: [PATCH 1 13/25] hpsa: simplify update scsi devices

[Matthew R. Ochs]

> On Oct 29, 2015, at 2:01 PM, Don Brace  wrote:
> 
> On 10/29/2015 11:43 AM, Matthew R. Ochs wrote:
>>> On Oct 28, 2015, at 5:05 PM, Don Brace  wrote:
>>> 
>>> From: Kevin Barnett 
>>> 
>>> remove repeated calculation that checks for physical
>>> or logical devices.
>>> 
>>> Reviewed-by: Scott Teel 
>>> Reviewed-by: Justin Lindley 
>>> Reviewed-by: Kevin Barnett 
>>> Signed-off-by: Don Brace 
>>> ---
>>> drivers/scsi/hpsa.c |   23 ++-
>>> drivers/scsi/hpsa.h |1 +
>>> 2 files changed, 15 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
>>> index d011540..7c1a552 100644
>>> --- a/drivers/scsi/hpsa.c
>>> +++ b/drivers/scsi/hpsa.c
>>> @@ -3761,6 +3761,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info 
>>> *h, int hostno)
>>> int ncurrent = 0;
>>> int i, n_ext_target_devs, ndevs_to_allocate;
>>> int raid_ctlr_position;
>>> +   bool physical_device;
>> Any particular reason for using a bool here and a u8 when you cache the 
>> value?
> Changed definition to u8 physical_device : 1; in hpsa_scsi_dev_t
> 
>> 
>>> DECLARE_BITMAP(lunzerobits, MAX_EXT_TARGETS);
>>> 
>>> currentsd = kzalloc(sizeof(*currentsd) * HPSA_MAX_DEVICES, GFP_KERNEL);
>>> @@ -3821,16 +3822,17 @@ static void hpsa_update_scsi_devices(struct 
>>> ctlr_info *h, int hostno)
>>> int rc = 0;
>>> int phys_dev_index = i - (raid_ctlr_position == 0);
>>> 
>>> +   physical_device = i < nphysicals + (raid_ctlr_position == 0);
>>> +
>>> /* Figure out where the LUN ID info is coming from */
>>> lunaddrbytes = figure_lunaddrbytes(h, raid_ctlr_position,
>>> i, nphysicals, nlogicals, physdev_list, logdev_list);
>>> 
>>> /* skip masked non-disk devices */
>>> -   if (MASKED_DEVICE(lunaddrbytes))
>>> -   if (i < nphysicals + (raid_ctlr_position == 0) &&
>>> -   (physdev_list->
>>> -   LUN[phys_dev_index].device_flags & 0x01))
>>> -   continue;
>>> +   if (physical_device &&
>>> +   MASKED_DEVICE(lunaddrbytes) &&
>>> +   (physdev_list->LUN[phys_dev_index].device_flags & 0x01))
>>> +   continue;
>> In this conditional you swapped the ordering, evaluating physical_device 
>> first, why?
> Changed it back. Better to be consistent.

With these changes I'm fine with you adding

Reviewed-by: Matthew R. Ochs 

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

[PATCH 1 13/25] hpsa: simplify update scsi devices

[Don Brace]

From: Kevin Barnett 

remove repeated calculation that checks for physical
or logical devices.

Reviewed-by: Scott Teel 
Reviewed-by: Justin Lindley 
Reviewed-by: Kevin Barnett 
Signed-off-by: Don Brace 
---
 drivers/scsi/hpsa.c |   23 ++-
 drivers/scsi/hpsa.h |1 +
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index d011540..7c1a552 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3761,6 +3761,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h, 
int hostno)
int ncurrent = 0;
int i, n_ext_target_devs, ndevs_to_allocate;
int raid_ctlr_position;
+   bool physical_device;
DECLARE_BITMAP(lunzerobits, MAX_EXT_TARGETS);
 
currentsd = kzalloc(sizeof(*currentsd) * HPSA_MAX_DEVICES, GFP_KERNEL);
@@ -3821,16 +3822,17 @@ static void hpsa_update_scsi_devices(struct ctlr_info 
*h, int hostno)
int rc = 0;
int phys_dev_index = i - (raid_ctlr_position == 0);
 
+   physical_device = i < nphysicals + (raid_ctlr_position == 0);
+
/* Figure out where the LUN ID info is coming from */
lunaddrbytes = figure_lunaddrbytes(h, raid_ctlr_position,
i, nphysicals, nlogicals, physdev_list, logdev_list);
 
/* skip masked non-disk devices */
-   if (MASKED_DEVICE(lunaddrbytes))
-   if (i < nphysicals + (raid_ctlr_position == 0) &&
-   (physdev_list->
-   LUN[phys_dev_index].device_flags & 0x01))
-   continue;
+   if (physical_device &&
+   MASKED_DEVICE(lunaddrbytes) &&
+   (physdev_list->LUN[phys_dev_index].device_flags & 0x01))
+   continue;
 
/* Get device type, vendor, model, device id */
rc = hpsa_update_device_info(h, lunaddrbytes, tmpdevice,
@@ -3866,10 +3868,13 @@ static void hpsa_update_scsi_devices(struct ctlr_info 
*h, int hostno)
}
 
*this_device = *tmpdevice;
+   this_device->physical_device = physical_device;
 
-   /* do not expose masked devices */
-   if (MASKED_DEVICE(lunaddrbytes) &&
-   i < nphysicals + (raid_ctlr_position == 0))
+   /*
+* Expose all devices except for physical devices that
+* are masked.
+*/
+   if (MASKED_DEVICE(lunaddrbytes) && this_device->physical_device)
this_device->expose_device = 0;
else
this_device->expose_device = 1;
@@ -3887,7 +3892,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info *h, 
int hostno)
ncurrent++;
break;
case TYPE_DISK:
-   if (i < nphysicals + (raid_ctlr_position == 0)) {
+   if (this_device->physical_device) {
/* The disk is in HBA mode. */
/* Never use RAID mapper in HBA mode. */
this_device->offload_enabled = 0;
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index a6ead07..808b520 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -37,6 +37,7 @@ struct hpsa_scsi_dev_t {
unsigned int devtype;
int bus, target, lun;   /* as presented to the OS */
unsigned char scsi3addr[8]; /* as presented to the HW */
+   u8 physical_device;
u8 expose_device;
 #define RAID_CTLR_LUNID "\0\0\0\0\0\0\0\0"
unsigned char device_id[16];/* from inquiry pg. 0x83 */

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