diff mbox

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

Message ID 20151028220549.5323.85641.stgit@brunhilda (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Don Brace Oct. 28, 2015, 10:05 p.m. UTC 
From: Kevin Barnett <kevin.barnett@pmcs.com>

remove repeated calculation that checks for physical
or logical devices.

Reviewed-by: Scott Teel <scott.teel@pmcs.com>
Reviewed-by: Justin Lindley <justin.lindley@pmcs.com>
Reviewed-by: Kevin Barnett <kevin.barnett@pmcs.com>
Signed-off-by: Don Brace <don.brace@pmcs.com>
---
 drivers/scsi/hpsa.c |   23 ++++++++++++++---------
 drivers/scsi/hpsa.h |    1 +
 2 files changed, 15 insertions(+), 9 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tomas Henzl Oct. 29, 2015, 3:53 p.m. UTC | #1 
On 28.10.2015 23:05, Don Brace wrote:
> From: Kevin Barnett <kevin.barnett@pmcs.com>
>
> remove repeated calculation that checks for physical
> or logical devices.
>
> Reviewed-by: Scott Teel <scott.teel@pmcs.com>
> Reviewed-by: Justin Lindley <justin.lindley@pmcs.com>
> Reviewed-by: Kevin Barnett <kevin.barnett@pmcs.com>
> Signed-off-by: Don Brace <don.brace@pmcs.com>

Reviewed-by: Tomas Henzl <thenzl@redhat.com>

Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew R. Ochs Oct. 29, 2015, 4:43 p.m. UTC | #2 
> On Oct 28, 2015, at 5:05 PM, Don Brace <don.brace@pmcs.com> wrote:
> 
> From: Kevin Barnett <kevin.barnett@pmcs.com>
> 
> remove repeated calculation that checks for physical
> or logical devices.
> 
> Reviewed-by: Scott Teel <scott.teel@pmcs.com>
> Reviewed-by: Justin Lindley <justin.lindley@pmcs.com>
> Reviewed-by: Kevin Barnett <kevin.barnett@pmcs.com>
> Signed-off-by: Don Brace <don.brace@pmcs.com>
> ---
> 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 majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Don Brace Oct. 29, 2015, 7:01 p.m. UTC | #3 
On 10/29/2015 11:43 AM, Matthew R. Ochs wrote:
>> On Oct 28, 2015, at 5:05 PM, Don Brace <don.brace@pmcs.com> wrote:
>>
>> From: Kevin Barnett <kevin.barnett@pmcs.com>
>>
>> remove repeated calculation that checks for physical
>> or logical devices.
>>
>> Reviewed-by: Scott Teel <scott.teel@pmcs.com>
>> Reviewed-by: Justin Lindley <justin.lindley@pmcs.com>
>> Reviewed-by: Kevin Barnett <kevin.barnett@pmcs.com>
>> Signed-off-by: Don Brace <don.brace@pmcs.com>
>> ---
>> 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 majordomo@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 majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew R. Ochs Oct. 29, 2015, 8:28 p.m. UTC | #4 
> On Oct 29, 2015, at 2:01 PM, Don Brace <brace77070@gmail.com> wrote:
> 
> On 10/29/2015 11:43 AM, Matthew R. Ochs wrote:
>>> On Oct 28, 2015, at 5:05 PM, Don Brace <don.brace@pmcs.com> wrote:
>>> 
>>> From: Kevin Barnett <kevin.barnett@pmcs.com>
>>> 
>>> remove repeated calculation that checks for physical
>>> or logical devices.
>>> 
>>> Reviewed-by: Scott Teel <scott.teel@pmcs.com>
>>> Reviewed-by: Justin Lindley <justin.lindley@pmcs.com>
>>> Reviewed-by: Kevin Barnett <kevin.barnett@pmcs.com>
>>> Signed-off-by: Don Brace <don.brace@pmcs.com>
>>> ---
>>> 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 <mrochs@linux.vnet.ibm.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Reinecke Oct. 30, 2015, 8:05 a.m. UTC | #5 
On 10/28/2015 11:05 PM, Don Brace wrote:
> From: Kevin Barnett <kevin.barnett@pmcs.com>
> 
> remove repeated calculation that checks for physical
> or logical devices.
> 
> Reviewed-by: Scott Teel <scott.teel@pmcs.com>
> Reviewed-by: Justin Lindley <justin.lindley@pmcs.com>
> Reviewed-by: Kevin Barnett <kevin.barnett@pmcs.com>
> Signed-off-by: Don Brace <don.brace@pmcs.com>
> ---
>  drivers/scsi/hpsa.c |   23 ++++++++++++++---------
>  drivers/scsi/hpsa.h |    1 +
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
diff mbox

Patch

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