* [PATCH] megaraid_sas: some corrections in megasas_transition_to_ready
@ 2011-02-25 16:22 Tomas Henzl
2011-02-25 22:37 ` adam radford
0 siblings, 1 reply; 3+ messages in thread
From: Tomas Henzl @ 2011-02-25 16:22 UTC (permalink / raw)
To: 'linux-scsi@vger.kernel.org'; +Cc: adam radford, Yang, Bo
Hi,
megasas_transition_to_ready seems to use some code which isn't needed.
I removed some variables, I hope this makes the code easier thus better readable.
max_wait - the assigned value is the same in al cases
cur_state - values are assigned but never used
also removed some extensive calls to read_fw_status_reg - it seems useless.
I have to admit - the code has many paths, I couldn't test it sufficiently,
I hope for help from LSI with testing.
Thanks, Tomas
Signed-off-by: Tomas Henzl <thenzl@redhat.com>>
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 07254aa..052ae4d 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -1831,12 +1831,11 @@ static int
megasas_transition_to_ready(struct megasas_instance* instance)
{
int i;
- u8 max_wait;
u32 fw_state;
- u32 cur_state;
u32 abs_state, curr_abs_state;
- fw_state = instance->instancet->read_fw_status_reg(instance->reg_set) & MFI_STATE_MASK;
+ abs_state = instance->instancet->read_fw_status_reg(instance->reg_set);
+ fw_state = abs_state & MFI_STATE_MASK;
if (fw_state != MFI_STATE_READY)
printk(KERN_INFO "megasas: Waiting for FW to come to ready"
@@ -1844,16 +1843,10 @@ megasas_transition_to_ready(struct megasas_instance* instance)
while (fw_state != MFI_STATE_READY) {
- abs_state =
- instance->instancet->read_fw_status_reg(instance->reg_set);
-
switch (fw_state) {
case MFI_STATE_FAULT:
-
printk(KERN_DEBUG "megasas: FW in FAULT state!!\n");
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_FAULT;
break;
case MFI_STATE_WAIT_HANDSHAKE:
@@ -1873,9 +1866,6 @@ megasas_transition_to_ready(struct megasas_instance* instance)
MFI_INIT_CLEAR_HANDSHAKE|MFI_INIT_HOTPLUG,
&instance->reg_set->inbound_doorbell);
}
-
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_WAIT_HANDSHAKE;
break;
case MFI_STATE_BOOT_MESSAGE_PENDING:
@@ -1888,9 +1878,6 @@ megasas_transition_to_ready(struct megasas_instance* instance)
} else
writel(MFI_INIT_HOTPLUG,
&instance->reg_set->inbound_doorbell);
-
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_BOOT_MESSAGE_PENDING;
break;
case MFI_STATE_OPERATIONAL:
@@ -1907,42 +1894,14 @@ megasas_transition_to_ready(struct megasas_instance* instance)
} else
writel(MFI_RESET_FLAGS,
&instance->reg_set->inbound_doorbell);
-
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_OPERATIONAL;
break;
case MFI_STATE_UNDEFINED:
- /*
- * This state should not last for more than 2 seconds
- */
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_UNDEFINED;
- break;
-
case MFI_STATE_BB_INIT:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_BB_INIT;
- break;
-
case MFI_STATE_FW_INIT:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_FW_INIT;
- break;
-
case MFI_STATE_FW_INIT_2:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_FW_INIT_2;
- break;
-
case MFI_STATE_DEVICE_SCAN:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_DEVICE_SCAN;
- break;
-
case MFI_STATE_FLUSH_CACHE:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_FLUSH_CACHE;
break;
default:
@@ -1952,31 +1911,32 @@ megasas_transition_to_ready(struct megasas_instance* instance)
}
/*
- * The cur_state should not last for more than max_wait secs
+ * The cur_state should not last for more than
+ * MEGASAS_RESET_WAIT_TIME secs
*/
- for (i = 0; i < (max_wait * 1000); i++) {
- fw_state = instance->instancet->read_fw_status_reg(instance->reg_set) &
- MFI_STATE_MASK ;
- curr_abs_state =
- instance->instancet->read_fw_status_reg(instance->reg_set);
+ for (i = 0; i < MEGASAS_RESET_WAIT_TIME * 1000; i++) {
+ curr_abs_state =
+ instance->instancet->read_fw_status_reg(instance->reg_set);
- if (abs_state == curr_abs_state) {
+ if (abs_state == curr_abs_state)
msleep(1);
- } else
+ else
break;
}
-
/*
- * Return error if fw_state hasn't changed after max_wait
+ * Return error if fw_state hasn't changed
+ * after MEGASAS_RESET_WAIT_TIME
*/
if (curr_abs_state == abs_state) {
printk(KERN_DEBUG "FW state [%d] hasn't changed "
- "in %d secs\n", fw_state, max_wait);
+ "in %d secs\n", fw_state, MEGASAS_RESET_WAIT_TIME);
return -ENODEV;
}
- };
- printk(KERN_INFO "megasas: FW now in Ready state\n");
+ abs_state = curr_abs_state;
+ fw_state = abs_state & MFI_STATE_MASK;
+ }
+ printk(KERN_INFO "megasas: FW now in Ready state\n");
return 0;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] megaraid_sas: some corrections in megasas_transition_to_ready
2011-02-25 16:22 [PATCH] megaraid_sas: some corrections in megasas_transition_to_ready Tomas Henzl
@ 2011-02-25 22:37 ` adam radford
2011-02-28 14:15 ` Tomas Henzl
0 siblings, 1 reply; 3+ messages in thread
From: adam radford @ 2011-02-25 22:37 UTC (permalink / raw)
To: Tomas Henzl; +Cc: linux-scsi@vger.kernel.org, Yang, Bo
On Fri, Feb 25, 2011 at 8:22 AM, Tomas Henzl <thenzl@redhat.com> wrote:
> Hi,
>
> megasas_transition_to_ready seems to use some code which isn't needed.
> I removed some variables, I hope this makes the code easier thus better readable.
> max_wait - the assigned value is the same in al cases
Tomas,
Your patch removes the ability to easily assign different wait times
for each controller state. Although we are currently using a maximum
of 180 seconds for all states, there is talk within LSI (at this very
moment) about altering how long we wait in each state.
-Adam
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] megaraid_sas: some corrections in megasas_transition_to_ready
2011-02-25 22:37 ` adam radford
@ 2011-02-28 14:15 ` Tomas Henzl
0 siblings, 0 replies; 3+ messages in thread
From: Tomas Henzl @ 2011-02-28 14:15 UTC (permalink / raw)
To: adam radford; +Cc: linux-scsi@vger.kernel.org, Yang, Bo
On 02/25/2011 11:37 PM, adam radford wrote:
> On Fri, Feb 25, 2011 at 8:22 AM, Tomas Henzl <thenzl@redhat.com> wrote:
>
>> Hi,
>>
>> megasas_transition_to_ready seems to use some code which isn't needed.
>> I removed some variables, I hope this makes the code easier thus better readable.
>> max_wait - the assigned value is the same in al cases
>>
> Tomas,
>
> Your patch removes the ability to easily assign different wait times
> for each controller state. Although we are currently using a maximum
> of 180 seconds for all states, there is talk within LSI (at this very
> moment) about altering how long we wait in each state.
>
I added the max_wait back for you, if you decide to differentiate the wait times
you can simply add a corresponding line to a case statement :)
If you wanted to say that you are going to rewrite this function, it's OK
for me to drop this patch.
In your original code you read the value of read_fw_status_reg every time twice and assign
it to different variables - I think that when the hw state changes in between that it will
_not_ cause problems in this function, but it looks strange.
...
megasas_transition_to_ready seems to use some code which isn't needed.
I removed some variables, I hope this makes the code easier thus better readable.
max_wait - the assigned value is the same in al cases, the value is now assigned outside the switch
cur_state - values are assigned but never used
also removed some extensive calls to read_fw_status_reg - it seems useless.
Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 07254aa..14e6f33 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -1830,13 +1830,12 @@ static irqreturn_t megasas_isr(int irq, void *devp)
static int
megasas_transition_to_ready(struct megasas_instance* instance)
{
- int i;
- u8 max_wait;
+ int i, max_wait;
u32 fw_state;
- u32 cur_state;
u32 abs_state, curr_abs_state;
- fw_state = instance->instancet->read_fw_status_reg(instance->reg_set) & MFI_STATE_MASK;
+ abs_state = instance->instancet->read_fw_status_reg(instance->reg_set);
+ fw_state = abs_state & MFI_STATE_MASK;
if (fw_state != MFI_STATE_READY)
printk(KERN_INFO "megasas: Waiting for FW to come to ready"
@@ -1844,16 +1843,12 @@ megasas_transition_to_ready(struct megasas_instance* instance)
while (fw_state != MFI_STATE_READY) {
- abs_state =
- instance->instancet->read_fw_status_reg(instance->reg_set);
+ max_wait = MEGASAS_RESET_WAIT_TIME;
switch (fw_state) {
case MFI_STATE_FAULT:
-
printk(KERN_DEBUG "megasas: FW in FAULT state!!\n");
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_FAULT;
break;
case MFI_STATE_WAIT_HANDSHAKE:
@@ -1873,9 +1868,6 @@ megasas_transition_to_ready(struct megasas_instance* instance)
MFI_INIT_CLEAR_HANDSHAKE|MFI_INIT_HOTPLUG,
&instance->reg_set->inbound_doorbell);
}
-
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_WAIT_HANDSHAKE;
break;
case MFI_STATE_BOOT_MESSAGE_PENDING:
@@ -1888,9 +1880,6 @@ megasas_transition_to_ready(struct megasas_instance* instance)
} else
writel(MFI_INIT_HOTPLUG,
&instance->reg_set->inbound_doorbell);
-
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_BOOT_MESSAGE_PENDING;
break;
case MFI_STATE_OPERATIONAL:
@@ -1907,42 +1896,14 @@ megasas_transition_to_ready(struct megasas_instance* instance)
} else
writel(MFI_RESET_FLAGS,
&instance->reg_set->inbound_doorbell);
-
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_OPERATIONAL;
break;
case MFI_STATE_UNDEFINED:
- /*
- * This state should not last for more than 2 seconds
- */
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_UNDEFINED;
- break;
-
case MFI_STATE_BB_INIT:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_BB_INIT;
- break;
-
case MFI_STATE_FW_INIT:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_FW_INIT;
- break;
-
case MFI_STATE_FW_INIT_2:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_FW_INIT_2;
- break;
-
case MFI_STATE_DEVICE_SCAN:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_DEVICE_SCAN;
- break;
-
case MFI_STATE_FLUSH_CACHE:
- max_wait = MEGASAS_RESET_WAIT_TIME;
- cur_state = MFI_STATE_FLUSH_CACHE;
break;
default:
@@ -1954,29 +1915,29 @@ megasas_transition_to_ready(struct megasas_instance* instance)
/*
* The cur_state should not last for more than max_wait secs
*/
- for (i = 0; i < (max_wait * 1000); i++) {
- fw_state = instance->instancet->read_fw_status_reg(instance->reg_set) &
- MFI_STATE_MASK ;
- curr_abs_state =
- instance->instancet->read_fw_status_reg(instance->reg_set);
+ for (i = 0; i < max_wait * 1000; i++) {
+ curr_abs_state =
+ instance->instancet->read_fw_status_reg(instance->reg_set);
- if (abs_state == curr_abs_state) {
+ if (abs_state == curr_abs_state)
msleep(1);
- } else
+ else
break;
}
-
/*
- * Return error if fw_state hasn't changed after max_wait
+ * Return error if fw_state hasn't changed
+ * after MEGASAS_RESET_WAIT_TIME
*/
if (curr_abs_state == abs_state) {
printk(KERN_DEBUG "FW state [%d] hasn't changed "
- "in %d secs\n", fw_state, max_wait);
+ "in %d secs\n", fw_state, MEGASAS_RESET_WAIT_TIME);
return -ENODEV;
}
- };
- printk(KERN_INFO "megasas: FW now in Ready state\n");
+ abs_state = curr_abs_state;
+ fw_state = abs_state & MFI_STATE_MASK;
+ }
+ printk(KERN_INFO "megasas: FW now in Ready state\n");
return 0;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-02-28 14:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-25 16:22 [PATCH] megaraid_sas: some corrections in megasas_transition_to_ready Tomas Henzl
2011-02-25 22:37 ` adam radford
2011-02-28 14:15 ` Tomas Henzl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).