Skip to contentSkip to conversation

mpt fusion: Added Firmware debug support

PATCH In review Kashyap, Desai 3 participants 6 messages Jan 6, 2009 – Jan 7, 2009

Subsystems (2)
SCSI LSI Logic MPT fusion drivers (FC/SAS/SPI)
lore
Share & tools
lore

Kashyap, Desai

author
#0 lore raw

PATCH 3/5 mpt fusion: Added Firmware debug support

Added support for Firmware debuging
---

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/message/fusion/mptbase.c +37 -0
index 55d9a7e..ea3aafb 100644
@@ -107,6 +107,14 @@ module_param_call(mpt_debug_level, mpt_set_debug_level, param_get_int,
 MODULE_PARM_DESC(mpt_debug_level, " debug level - refer to mptdebug.h \
 	- (default=0)");
 
+int mpt_fwfault_debug;
+EXPORT_SYMBOL(mpt_fwfault_debug);
+module_param_call(mpt_fwfault_debug, param_set_int, param_get_int,
+	  &mpt_fwfault_debug, 0600);
+MODULE_PARM_DESC(mpt_fwfault_debug, "Enable detection of Firmware fault"
+	" and halt Firmware on fault - (default=0)");
+
+
 
 #ifdef MFCNT
 static int mfcounter = 0;

@@ -6337,6 +6345,33 @@ mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh
 	*size = y;
 }
 
+
+/**
+ *	mpt_halt_firmware - Halts the firmware if it is operational and panic
+ *	the kernel
+ *	@ioc: Pointer to MPT_ADAPTER structure
+ *
+ **/
+void
+mpt_halt_firmware(MPT_ADAPTER *ioc)
+{
+	u32	 ioc_raw_state;
+
+	ioc_raw_state = mpt_GetIocState(ioc, 0);
+
+	if ((ioc_raw_state & MPI_IOC_STATE_MASK) == MPI_IOC_STATE_FAULT) {
+		printk(MYIOC_s_ERR_FMT "IOC is in FAULT state (%04xh)!!!\n",
+			ioc->name, ioc_raw_state & MPI_DOORBELL_DATA_MASK);
+		panic("%s: IOC Fault (%04xh)!!!\n", ioc->name,
+			ioc_raw_state & MPI_DOORBELL_DATA_MASK);
+	} else {
+		CHIPREG_WRITE32(&ioc->chip->Doorbell, 0xC0FFEE00);
+		panic("%s: Firmware is halted due to command timeout\n",
+			ioc->name);
+	}
+}
+EXPORT_SYMBOL(mpt_halt_firmware);
+
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /*
  *	Reset Handling

@@ -6369,6 +6404,8 @@ mpt_HardResetHandler(MPT_ADAPTER *ioc, int sleepFlag)
 	printk(MYIOC_s_INFO_FMT "HardResetHandler Entered!\n", ioc->name);
 	printk("MF count 0x%x !\n", ioc->mfcnt);
 #endif
+	if (mpt_fwfault_debug)
+		mpt_halt_firmware(ioc);
 
 	/* Reset the adapter. Prevent more than 1 call to
 	 * mpt_do_ioc_recovery at any instant in time.
drivers/message/fusion/mptbase.h +3 -0
index dff048c..b3e981d 100644
@@ -922,11 +922,14 @@ extern void	 mpt_free_fw_memory(MPT_ADAPTER *ioc);
 extern int	 mpt_findImVolumes(MPT_ADAPTER *ioc);
 extern int	 mptbase_sas_persist_operation(MPT_ADAPTER *ioc, u8 persist_opcode);
 extern int	 mpt_raid_phys_disk_pg0(MPT_ADAPTER *ioc, u8 phys_disk_num, pRaidPhysDiskPage0_t phys_disk);
+extern void     mpt_halt_firmware(MPT_ADAPTER *ioc);
+
 
 /*
  *  Public data decl's...
  */
 extern struct list_head	  ioc_list;
+extern int mpt_fwfault_debug;
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 #endif		/* } __KERNEL__ */
drivers/message/fusion/mptscsih.c +3 -0
index ee09041..e62c6bc 100644
@@ -1846,6 +1846,9 @@ mptscsih_abort(struct scsi_cmnd * SCpnt)
 	if (hd->timeouts < -1)
 		hd->timeouts++;
 
+	if (mpt_fwfault_debug)
+		mpt_halt_firmware(ioc);
+
 	/* Most important!  Set TaskMsgContext to SCpnt's MsgContext!
 	 * (the IO to be ABORT'd)
 	 *
#1 lore raw
Quoted 38 lines · Desai

Added support for Firmware debuging
---

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/message/fusion/mptbase.c +17 -0
index 55d9a7e..ea3aafb 100644
@@ -107,6 +107,14 @@ module_param_call(mpt_debug_level, mpt_set_debug_level, param_get_int,
 MODULE_PARM_DESC(mpt_debug_level, " debug level - refer to mptdebug.h \
       - (default=0)");
 
+int mpt_fwfault_debug;
+EXPORT_SYMBOL(mpt_fwfault_debug);
+module_param_call(mpt_fwfault_debug, param_set_int, param_get_int,
+         &mpt_fwfault_debug, 0600);
+MODULE_PARM_DESC(mpt_fwfault_debug, "Enable detection of Firmware fault"
+       " and halt Firmware on fault - (default=0)");
+
+
 
 #ifdef MFCNT
 static int mfcounter = 0;

@@ -6337,6 +6345,33 @@ mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh
       *size = y;
 }
 
+
+/**
+ *     mpt_halt_firmware - Halts the firmware if it is operational and panic
+ *     the kernel
+ *     @ioc: Pointer to MPT_ADAPTER structure
+ *
+ **/
+void
+mpt_halt_firmware(MPT_ADAPTER *ioc)

drivers/message/fusion/mptbase.c mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh · line 6366

Quoted 10 lines
drivers/message/fusion/mptbase.c +10 -0
+{
+       u32      ioc_raw_state;
+
+       ioc_raw_state = mpt_GetIocState(ioc, 0);
+
+       if ((ioc_raw_state & MPI_IOC_STATE_MASK) == MPI_IOC_STATE_FAULT) {
+               printk(MYIOC_s_ERR_FMT "IOC is in FAULT state (%04xh)!!!\n",
+                       ioc->name, ioc_raw_state & MPI_DOORBELL_DATA_MASK);
+               panic("%s: IOC Fault (%04xh)!!!\n", ioc->name,
+                       ioc_raw_state & MPI_DOORBELL_DATA_MASK);

Wouldn't it be better to NOT panic the machine?
One can still prod things "manually" (read config and MMIO space registers) for further debugging.

I'm thinking it would make more sense to call the variable "mpt_fwfault_panic" and use it to decide if the machine should panic or not. Thoughts?

grant

Quoted 58 lines
drivers/message/fusion/mptbase.c +10 -0
+       } else {
+               CHIPREG_WRITE32(&ioc->chip->Doorbell, 0xC0FFEE00);
+               panic("%s: Firmware is halted due to command timeout\n",
+                       ioc->name);
+       }
+}
+EXPORT_SYMBOL(mpt_halt_firmware);
+
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /*
 *     Reset Handling

@@ -6369,6 +6404,8 @@ mpt_HardResetHandler(MPT_ADAPTER *ioc, int sleepFlag)
       printk(MYIOC_s_INFO_FMT "HardResetHandler Entered!\n", ioc->name);
       printk("MF count 0x%x !\n", ioc->mfcnt);
 #endif
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
 
       /* Reset the adapter. Prevent more than 1 call to
        * mpt_do_ioc_recovery at any instant in time.
drivers/message/fusion/mptbase.h +3 -0
index dff048c..b3e981d 100644
@@ -922,11 +922,14 @@ extern void        mpt_free_fw_memory(MPT_ADAPTER *ioc);
 extern int      mpt_findImVolumes(MPT_ADAPTER *ioc);
 extern int      mptbase_sas_persist_operation(MPT_ADAPTER *ioc, u8 persist_opcode);
 extern int      mpt_raid_phys_disk_pg0(MPT_ADAPTER *ioc, u8 phys_disk_num, pRaidPhysDiskPage0_t phys_disk);
+extern void     mpt_halt_firmware(MPT_ADAPTER *ioc);
+
 
 /*
 *  Public data decl's...
 */
 extern struct list_head          ioc_list;
+extern int mpt_fwfault_debug;
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 #endif         /* } __KERNEL__ */
drivers/message/fusion/mptscsih.c +3 -1
index ee09041..e62c6bc 100644
@@ -1846,6 +1846,9 @@ mptscsih_abort(struct scsi_cmnd * SCpnt)
       if (hd->timeouts < -1)
               hd->timeouts++;
 
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
+
       /* Most important!  Set TaskMsgContext to SCpnt's MsgContext!
        * (the IO to be ABORT'd)
        *
--

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

author
#2 lore raw

Grant,

I do agree with you, but we have reasons doing things this way.

1. This is only for development purpose. Normally when Firmware fault occurs driver will reset firmware so that further communication with firmware can resume. In this condition we were not able to track root cause of fault. In debug environment mpt_fwfault_debug will be set. In this case Driver will not reset firmware rather it will panic the system.

Reason doing Fault.
If we don't panic the system, we will get huge IO errors if any IO stress is running and error message will flood the syslog.(specially this things happens in Overnight stress test)

Thoughts?

Thanks
Kashyap Desai

-----Original Message-----
From: Grant Grundler [mailto:[email protected]]
Sent: Wednesday, January 07, 2009 1:23 AM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya; [email protected]
Subject: Re: [PATCH 3/5] mpt fusion: Added Firmware debug support

Quoted 38 lines · Desai

Added support for Firmware debuging
---

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/message/fusion/mptbase.c +17 -0
index 55d9a7e..ea3aafb 100644
@@ -107,6 +107,14 @@ module_param_call(mpt_debug_level, mpt_set_debug_level, param_get_int,
 MODULE_PARM_DESC(mpt_debug_level, " debug level - refer to mptdebug.h \
       - (default=0)");
 
+int mpt_fwfault_debug;
+EXPORT_SYMBOL(mpt_fwfault_debug);
+module_param_call(mpt_fwfault_debug, param_set_int, param_get_int,
+         &mpt_fwfault_debug, 0600);
+MODULE_PARM_DESC(mpt_fwfault_debug, "Enable detection of Firmware fault"
+       " and halt Firmware on fault - (default=0)");
+
+
 
 #ifdef MFCNT
 static int mfcounter = 0;

@@ -6337,6 +6345,33 @@ mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh
       *size = y;
 }
 
+
+/**
+ *     mpt_halt_firmware - Halts the firmware if it is operational and panic
+ *     the kernel
+ *     @ioc: Pointer to MPT_ADAPTER structure
+ *
+ **/
+void
+mpt_halt_firmware(MPT_ADAPTER *ioc)

drivers/message/fusion/mptbase.c mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh · line 6366

Quoted 10 lines
drivers/message/fusion/mptbase.c +10 -0
+{
+       u32      ioc_raw_state;
+
+       ioc_raw_state = mpt_GetIocState(ioc, 0);
+
+       if ((ioc_raw_state & MPI_IOC_STATE_MASK) == MPI_IOC_STATE_FAULT) {
+               printk(MYIOC_s_ERR_FMT "IOC is in FAULT state (%04xh)!!!\n",
+                       ioc->name, ioc_raw_state & MPI_DOORBELL_DATA_MASK);
+               panic("%s: IOC Fault (%04xh)!!!\n", ioc->name,
+                       ioc_raw_state & MPI_DOORBELL_DATA_MASK);

Wouldn't it be better to NOT panic the machine?
One can still prod things "manually" (read config and MMIO space registers) for further debugging.

I'm thinking it would make more sense to call the variable "mpt_fwfault_panic" and use it to decide if the machine should panic or not. Thoughts?

grant

Quoted 58 lines
drivers/message/fusion/mptbase.c +10 -0
+       } else {
+               CHIPREG_WRITE32(&ioc->chip->Doorbell, 0xC0FFEE00);
+               panic("%s: Firmware is halted due to command timeout\n",
+                       ioc->name);
+       }
+}
+EXPORT_SYMBOL(mpt_halt_firmware);
+
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /*
 *     Reset Handling

@@ -6369,6 +6404,8 @@ mpt_HardResetHandler(MPT_ADAPTER *ioc, int sleepFlag)
       printk(MYIOC_s_INFO_FMT "HardResetHandler Entered!\n", ioc->name);
       printk("MF count 0x%x !\n", ioc->mfcnt);
 #endif
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
 
       /* Reset the adapter. Prevent more than 1 call to
        * mpt_do_ioc_recovery at any instant in time.
drivers/message/fusion/mptbase.h +3 -0
index dff048c..b3e981d 100644
@@ -922,11 +922,14 @@ extern void        mpt_free_fw_memory(MPT_ADAPTER *ioc);
 extern int      mpt_findImVolumes(MPT_ADAPTER *ioc);
 extern int      mptbase_sas_persist_operation(MPT_ADAPTER *ioc, u8 persist_opcode);
 extern int      mpt_raid_phys_disk_pg0(MPT_ADAPTER *ioc, u8 phys_disk_num, pRaidPhysDiskPage0_t phys_disk);
+extern void     mpt_halt_firmware(MPT_ADAPTER *ioc);
+
 
 /*
 *  Public data decl's...
 */
 extern struct list_head          ioc_list;
+extern int mpt_fwfault_debug;
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 #endif         /* } __KERNEL__ */
drivers/message/fusion/mptscsih.c +3 -1
index ee09041..e62c6bc 100644
@@ -1846,6 +1846,9 @@ mptscsih_abort(struct scsi_cmnd * SCpnt)
       if (hd->timeouts < -1)
               hd->timeouts++;
 
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
+
       /* Most important!  Set TaskMsgContext to SCpnt's MsgContext!
        * (the IO to be ABORT'd)
        *
--

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

#3 lore raw
Quoted 5 lines · Kashyap

Grant,

I do agree with you, but we have reasons doing things this way.

1. This is only for development purpose. Normally when Firmware fault occurs driver will reset firmware so that further communication with firmware can resume. In this condition we were not able to track root cause of fault. In debug environment mpt_fwfault_debug will be set. In this case Driver will not reset firmware rather it will panic the system.

developement flags should be disabled by default, not enabled.

Quoted 2 lines

Reason doing Fault.
If we don't panic the system, we will get huge IO errors if any IO stress is running and error message will flood the syslog.(specially this things happens in Overnight stress test)

It makes sense to have the flag but not to enable it by default.

hth,
grant

Quoted 132 lines

Thoughts?

Thanks
Kashyap Desai

-----Original Message-----
From: Grant Grundler [mailto:[email protected]]
Sent: Wednesday, January 07, 2009 1:23 AM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya; [email protected]
Subject: Re: [PATCH 3/5] mpt fusion: Added Firmware debug support

Quoted 38 lines · Desai

Added support for Firmware debuging
---

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/message/fusion/mptbase.c +17 -0
index 55d9a7e..ea3aafb 100644
@@ -107,6 +107,14 @@ module_param_call(mpt_debug_level, mpt_set_debug_level, param_get_int,
 MODULE_PARM_DESC(mpt_debug_level, " debug level - refer to mptdebug.h \
       - (default=0)");
 
+int mpt_fwfault_debug;
+EXPORT_SYMBOL(mpt_fwfault_debug);
+module_param_call(mpt_fwfault_debug, param_set_int, param_get_int,
+         &mpt_fwfault_debug, 0600);
+MODULE_PARM_DESC(mpt_fwfault_debug, "Enable detection of Firmware fault"
+       " and halt Firmware on fault - (default=0)");
+
+
 
 #ifdef MFCNT
 static int mfcounter = 0;

@@ -6337,6 +6345,33 @@ mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh
       *size = y;
 }
 
+
+/**
+ *     mpt_halt_firmware - Halts the firmware if it is operational and panic
+ *     the kernel
+ *     @ioc: Pointer to MPT_ADAPTER structure
+ *
+ **/
+void
+mpt_halt_firmware(MPT_ADAPTER *ioc)

drivers/message/fusion/mptbase.c mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh · line 6366

Quoted 10 lines
drivers/message/fusion/mptbase.c +10 -0
+{
+       u32      ioc_raw_state;
+
+       ioc_raw_state = mpt_GetIocState(ioc, 0);
+
+       if ((ioc_raw_state & MPI_IOC_STATE_MASK) == MPI_IOC_STATE_FAULT) {
+               printk(MYIOC_s_ERR_FMT "IOC is in FAULT state (%04xh)!!!\n",
+                       ioc->name, ioc_raw_state & MPI_DOORBELL_DATA_MASK);
+               panic("%s: IOC Fault (%04xh)!!!\n", ioc->name,
+                       ioc_raw_state & MPI_DOORBELL_DATA_MASK);

Wouldn't it be better to NOT panic the machine?
One can still prod things "manually" (read config and MMIO space registers) for further debugging.

I'm thinking it would make more sense to call the variable "mpt_fwfault_panic" and use it to decide if the machine should panic or not. Thoughts?

grant

Quoted 58 lines
drivers/message/fusion/mptbase.c +10 -0
+       } else {
+               CHIPREG_WRITE32(&ioc->chip->Doorbell, 0xC0FFEE00);
+               panic("%s: Firmware is halted due to command timeout\n",
+                       ioc->name);
+       }
+}
+EXPORT_SYMBOL(mpt_halt_firmware);
+
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /*
 *     Reset Handling

@@ -6369,6 +6404,8 @@ mpt_HardResetHandler(MPT_ADAPTER *ioc, int sleepFlag)
       printk(MYIOC_s_INFO_FMT "HardResetHandler Entered!\n", ioc->name);
       printk("MF count 0x%x !\n", ioc->mfcnt);
 #endif
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
 
       /* Reset the adapter. Prevent more than 1 call to
        * mpt_do_ioc_recovery at any instant in time.
drivers/message/fusion/mptbase.h +3 -0
index dff048c..b3e981d 100644
@@ -922,11 +922,14 @@ extern void        mpt_free_fw_memory(MPT_ADAPTER *ioc);
 extern int      mpt_findImVolumes(MPT_ADAPTER *ioc);
 extern int      mptbase_sas_persist_operation(MPT_ADAPTER *ioc, u8 persist_opcode);
 extern int      mpt_raid_phys_disk_pg0(MPT_ADAPTER *ioc, u8 phys_disk_num, pRaidPhysDiskPage0_t phys_disk);
+extern void     mpt_halt_firmware(MPT_ADAPTER *ioc);
+
 
 /*
 *  Public data decl's...
 */
 extern struct list_head          ioc_list;
+extern int mpt_fwfault_debug;
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 #endif         /* } __KERNEL__ */
drivers/message/fusion/mptscsih.c +3 -1
index ee09041..e62c6bc 100644
@@ -1846,6 +1846,9 @@ mptscsih_abort(struct scsi_cmnd * SCpnt)
       if (hd->timeouts < -1)
               hd->timeouts++;
 
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
+
       /* Most important!  Set TaskMsgContext to SCpnt's MsgContext!
        * (the IO to be ABORT'd)
        *
--

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

#4 lore raw

Grant,
I see it is disabled by default. And it can be enabled by passing a module parameter. Do you see anywhere the patch sets the flag to enable? Thanks
Sathya

-----Original Message-----
From: Grant Grundler [mailto:[email protected]]
Sent: Thursday, January 08, 2009 1:34 AM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya; [email protected]
Subject: Re: [PATCH 3/5] mpt fusion: Added Firmware debug support

Quoted 5 lines · Kashyap

Grant,

I do agree with you, but we have reasons doing things this way.

1. This is only for development purpose. Normally when Firmware fault occurs driver will reset firmware so that further communication with firmware can resume. In this condition we were not able to track root cause of fault. In debug environment mpt_fwfault_debug will be set. In this case Driver will not reset firmware rather it will panic the system.

developement flags should be disabled by default, not enabled.

Quoted 4 lines

Reason doing Fault.
If we don't panic the system, we will get huge IO errors if any IO stress is running and error message will flood the syslog.(specially this things happens in Overnight stress test)

It makes sense to have the flag but not to enable it by default.

hth,
grant

Quoted 137 lines

Thoughts?

Thanks
Kashyap Desai

-----Original Message-----
From: Grant Grundler [mailto:[email protected]]
Sent: Wednesday, January 07, 2009 1:23 AM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya; [email protected]
Subject: Re: [PATCH 3/5] mpt fusion: Added Firmware debug support

Quoted 39 lines · Desai

Added support for Firmware debuging
---

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/message/fusion/mptbase.c +17 -0
index 55d9a7e..ea3aafb 100644
@@ -107,6 +107,14 @@ module_param_call(mpt_debug_level,
 mpt_set_debug_level, param_get_int,  MODULE_PARM_DESC(mpt_debug_level, " debug level - refer to mptdebug.h \
       - (default=0)");
 
+int mpt_fwfault_debug;
+EXPORT_SYMBOL(mpt_fwfault_debug);
+module_param_call(mpt_fwfault_debug, param_set_int, param_get_int,
+         &mpt_fwfault_debug, 0600);
+MODULE_PARM_DESC(mpt_fwfault_debug, "Enable detection of Firmware fault"
+       " and halt Firmware on fault - (default=0)");
+
+
 
 #ifdef MFCNT
 static int mfcounter = 0;

@@ -6337,6 +6345,33 @@ mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh
       *size = y;
 }
 
+
+/**
+ *     mpt_halt_firmware - Halts the firmware if it is operational and panic
+ *     the kernel
+ *     @ioc: Pointer to MPT_ADAPTER structure
+ *
+ **/
+void
+mpt_halt_firmware(MPT_ADAPTER *ioc)

drivers/message/fusion/mptbase.c mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh · line 6366

Quoted 10 lines
drivers/message/fusion/mptbase.c +10 -0
+{
+       u32      ioc_raw_state;
+
+       ioc_raw_state = mpt_GetIocState(ioc, 0);
+
+       if ((ioc_raw_state & MPI_IOC_STATE_MASK) == MPI_IOC_STATE_FAULT) {
+               printk(MYIOC_s_ERR_FMT "IOC is in FAULT state (%04xh)!!!\n",
+                       ioc->name, ioc_raw_state & MPI_DOORBELL_DATA_MASK);
+               panic("%s: IOC Fault (%04xh)!!!\n", ioc->name,
+                       ioc_raw_state & MPI_DOORBELL_DATA_MASK);

Wouldn't it be better to NOT panic the machine?
One can still prod things "manually" (read config and MMIO space registers) for further debugging.

I'm thinking it would make more sense to call the variable "mpt_fwfault_panic" and use it to decide if the machine should panic or not. Thoughts?

grant

Quoted 61 lines
drivers/message/fusion/mptbase.c +10 -1
+       } else {
+               CHIPREG_WRITE32(&ioc->chip->Doorbell, 0xC0FFEE00);
+               panic("%s: Firmware is halted due to command timeout\n",
+                       ioc->name);
+       }
+}
+EXPORT_SYMBOL(mpt_halt_firmware);
+
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
-=-=-=-=*/
 /*
 *     Reset Handling

@@ -6369,6 +6404,8 @@ mpt_HardResetHandler(MPT_ADAPTER *ioc, int sleepFlag)
       printk(MYIOC_s_INFO_FMT "HardResetHandler Entered!\n", ioc->name);
       printk("MF count 0x%x !\n", ioc->mfcnt);  #endif
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
 
       /* Reset the adapter. Prevent more than 1 call to
        * mpt_do_ioc_recovery at any instant in time.
drivers/message/fusion/mptbase.h +3 -0
index dff048c..b3e981d 100644
@@ -922,11 +922,14 @@ extern void        mpt_free_fw_memory(MPT_ADAPTER *ioc);
 extern int      mpt_findImVolumes(MPT_ADAPTER *ioc);
 extern int      mptbase_sas_persist_operation(MPT_ADAPTER *ioc, u8 persist_opcode);
 extern int      mpt_raid_phys_disk_pg0(MPT_ADAPTER *ioc, u8 phys_disk_num, pRaidPhysDiskPage0_t phys_disk);
+extern void     mpt_halt_firmware(MPT_ADAPTER *ioc);
+
 
 /*
 *  Public data decl's...
 */
 extern struct list_head          ioc_list;
+extern int mpt_fwfault_debug;
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 #endif         /* } __KERNEL__ */
drivers/message/fusion/mptscsih.c +3 -1
index ee09041..e62c6bc 100644
@@ -1846,6 +1846,9 @@ mptscsih_abort(struct scsi_cmnd * SCpnt)
       if (hd->timeouts < -1)
               hd->timeouts++;
 
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
+
       /* Most important!  Set TaskMsgContext to SCpnt's MsgContext!
        * (the IO to be ABORT'd)
        *
--

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

#5 lore raw
Quoted 2 lines · Sathya

Grant,
I see it is disabled by default.

Sorry, I expressed myself poorly in the last email. The message is disabled by default too.

The message is appropriate without a flag; the panic needs a flag and which should be disable by default. That make more sense?

Anyway, it's ultimately up to you guys how this should work. I'm just trying to provide feedback based on the type of bug reports I can do something with. If the driver reports nothing, users will report vague, occasional poor performance (or very high latency) instead of specific dmesg output pointing to a known problem (firmware hangs/resets) that might already be fixed in newer firmware revs.

Quoted 1 line

And it can be enabled by passing a module parameter. Do you see anywhere the patch sets the flag to enable?

This is correct. The submitted patch has the flag disabled.

thanks,
grant

Quoted 167 lines

Thanks
Sathya

-----Original Message-----
From: Grant Grundler [mailto:[email protected]]
Sent: Thursday, January 08, 2009 1:34 AM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya; [email protected]
Subject: Re: [PATCH 3/5] mpt fusion: Added Firmware debug support

Quoted 5 lines · Kashyap

Grant,

I do agree with you, but we have reasons doing things this way.

1. This is only for development purpose. Normally when Firmware fault occurs driver will reset firmware so that further communication with firmware can resume. In this condition we were not able to track root cause of fault. In debug environment mpt_fwfault_debug will be set. In this case Driver will not reset firmware rather it will panic the system.

developement flags should be disabled by default, not enabled.

Quoted 4 lines

Reason doing Fault.
If we don't panic the system, we will get huge IO errors if any IO stress is running and error message will flood the syslog.(specially this things happens in Overnight stress test)

It makes sense to have the flag but not to enable it by default.

hth,
grant

Quoted 137 lines

Thoughts?

Thanks
Kashyap Desai

-----Original Message-----
From: Grant Grundler [mailto:[email protected]]
Sent: Wednesday, January 07, 2009 1:23 AM
To: Desai, Kashyap
Cc: [email protected]; Moore, Eric; Prakash, Sathya; [email protected]
Subject: Re: [PATCH 3/5] mpt fusion: Added Firmware debug support

Quoted 39 lines · Desai

Added support for Firmware debuging
---

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/message/fusion/mptbase.c +17 -0
index 55d9a7e..ea3aafb 100644
@@ -107,6 +107,14 @@ module_param_call(mpt_debug_level,
 mpt_set_debug_level, param_get_int,  MODULE_PARM_DESC(mpt_debug_level, " debug level - refer to mptdebug.h \
       - (default=0)");
 
+int mpt_fwfault_debug;
+EXPORT_SYMBOL(mpt_fwfault_debug);
+module_param_call(mpt_fwfault_debug, param_set_int, param_get_int,
+         &mpt_fwfault_debug, 0600);
+MODULE_PARM_DESC(mpt_fwfault_debug, "Enable detection of Firmware fault"
+       " and halt Firmware on fault - (default=0)");
+
+
 
 #ifdef MFCNT
 static int mfcounter = 0;

@@ -6337,6 +6345,33 @@ mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh
       *size = y;
 }
 
+
+/**
+ *     mpt_halt_firmware - Halts the firmware if it is operational and panic
+ *     the kernel
+ *     @ioc: Pointer to MPT_ADAPTER structure
+ *
+ **/
+void
+mpt_halt_firmware(MPT_ADAPTER *ioc)

drivers/message/fusion/mptbase.c mpt_print_ioc_summary(MPT_ADAPTER *ioc, char *buffer, int *size, int len, int sh · line 6366

Quoted 10 lines
drivers/message/fusion/mptbase.c +10 -0
+{
+       u32      ioc_raw_state;
+
+       ioc_raw_state = mpt_GetIocState(ioc, 0);
+
+       if ((ioc_raw_state & MPI_IOC_STATE_MASK) == MPI_IOC_STATE_FAULT) {
+               printk(MYIOC_s_ERR_FMT "IOC is in FAULT state (%04xh)!!!\n",
+                       ioc->name, ioc_raw_state & MPI_DOORBELL_DATA_MASK);
+               panic("%s: IOC Fault (%04xh)!!!\n", ioc->name,
+                       ioc_raw_state & MPI_DOORBELL_DATA_MASK);

Wouldn't it be better to NOT panic the machine?
One can still prod things "manually" (read config and MMIO space registers) for further debugging.

I'm thinking it would make more sense to call the variable "mpt_fwfault_panic" and use it to decide if the machine should panic or not. Thoughts?

grant

Quoted 61 lines
drivers/message/fusion/mptbase.c +10 -1
+       } else {
+               CHIPREG_WRITE32(&ioc->chip->Doorbell, 0xC0FFEE00);
+               panic("%s: Firmware is halted due to command timeout\n",
+                       ioc->name);
+       }
+}
+EXPORT_SYMBOL(mpt_halt_firmware);
+
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
-=-=-=-=*/
 /*
 *     Reset Handling

@@ -6369,6 +6404,8 @@ mpt_HardResetHandler(MPT_ADAPTER *ioc, int sleepFlag)
       printk(MYIOC_s_INFO_FMT "HardResetHandler Entered!\n", ioc->name);
       printk("MF count 0x%x !\n", ioc->mfcnt);  #endif
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
 
       /* Reset the adapter. Prevent more than 1 call to
        * mpt_do_ioc_recovery at any instant in time.
drivers/message/fusion/mptbase.h +3 -0
index dff048c..b3e981d 100644
@@ -922,11 +922,14 @@ extern void        mpt_free_fw_memory(MPT_ADAPTER *ioc);
 extern int      mpt_findImVolumes(MPT_ADAPTER *ioc);
 extern int      mptbase_sas_persist_operation(MPT_ADAPTER *ioc, u8 persist_opcode);
 extern int      mpt_raid_phys_disk_pg0(MPT_ADAPTER *ioc, u8 phys_disk_num, pRaidPhysDiskPage0_t phys_disk);
+extern void     mpt_halt_firmware(MPT_ADAPTER *ioc);
+
 
 /*
 *  Public data decl's...
 */
 extern struct list_head          ioc_list;
+extern int mpt_fwfault_debug;
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 #endif         /* } __KERNEL__ */
drivers/message/fusion/mptscsih.c +3 -1
index ee09041..e62c6bc 100644
@@ -1846,6 +1846,9 @@ mptscsih_abort(struct scsi_cmnd * SCpnt)
       if (hd->timeouts < -1)
               hd->timeouts++;
 
+       if (mpt_fwfault_debug)
+               mpt_halt_firmware(ioc);
+
       /* Most important!  Set TaskMsgContext to SCpnt's MsgContext!
        * (the IO to be ABORT'd)
        *
--

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