[PATCH]: mpt2sas crashes on shutdown

[PATCH]: mpt2sas crashes on shutdown

[David Miller]

The mpt2sas driver accesses I/O space as virtual addresses when
saving and restoring the MSIX table, this only works by luck on x86.

One needs to use the appropriate {read,write}{b,w,l}() APIs.

This is fixed in v3.2.x because all of this code got rewritten for
NUMA I/O support.

But both 3.0.x and 3.1.x still have this bug, and my Niagara sparc
machines crash on shutdown every single time due to this bug making my
-stable work more difficult than it needs to be.

Please apply to both 3.0.x-stable and 3.1.x-stable, thanks.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
index 83035bd..8cec3bd 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
@@ -1096,7 +1096,7 @@ _base_save_msix_table(struct MPT2SAS_ADAPTER *ioc)
 		return;
 
 	for (i = 0; i < ioc->msix_vector_count; i++)
-		ioc->msix_table_backup[i] = ioc->msix_table[i];
+		ioc->msix_table_backup[i] = readl(&ioc->msix_table[i]);
 }
 
 /**
@@ -1113,7 +1113,7 @@ _base_restore_msix_table(struct MPT2SAS_ADAPTER *ioc)
 		return;
 
 	for (i = 0; i < ioc->msix_vector_count; i++)
-		ioc->msix_table[i] = ioc->msix_table_backup[i];
+		writel(ioc->msix_table_backup[i], &ioc->msix_table[i]);
 }
 
 /**
@@ -1144,7 +1144,7 @@ _base_check_enable_msix(struct MPT2SAS_ADAPTER *ioc)
 	/* get msix table  */
 	pci_read_config_dword(ioc->pdev, base + 4, &msix_table_offset);
 	msix_table_offset &= 0xFFFFFFF8;
-	ioc->msix_table = (u32 *)((void *)ioc->chip + msix_table_offset);
+	ioc->msix_table = ((void __iomem *)ioc->chip + msix_table_offset);
 
 	dinitprintk(ioc, printk(MPT2SAS_INFO_FMT "msix is supported, "
 	    "vector_count(%d), table_offset(0x%08x), table(%p)\n", ioc->name,
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h b/drivers/scsi/mpt2sas/mpt2sas_base.h
index 41a57a7..a8782f3 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -768,7 +768,7 @@ struct MPT2SAS_ADAPTER {
 
 	u8		msix_enable;
 	u16		msix_vector_count;
-	u32		*msix_table;
+	u32 __iomem	*msix_table;
 	u32		*msix_table_backup;
 	u32		ioc_reset_count;
 

RE: [PATCH]: mpt2sas crashes on shutdown

[Moore, Eric]

[-- Attachment #1: Type: text/plain, Size: 3187 bytes --]

David - The backup and restoring of the MSIX vector capabilities registers across diagnostic reset was only needed for our pre-production cards (A0 parts). It was a workaround for some hardware errata.  This is not needed anymore as there was a fix addressed before those controllers when to production, so I finally removed this workaround in the driver during phase 10 in the LSI in-house drivers, which was about 10 months ago.  Attached is my commit for the in-house drivers.  You should be able to remove the save_msix_table and restore_msix_table routines as I did.

On Monday, December 26, 2011 12:59 PM, David Miller wrote:
> 
> The mpt2sas driver accesses I/O space as virtual addresses when saving and
> restoring the MSIX table, this only works by luck on x86.
> 
> One needs to use the appropriate {read,write}{b,w,l}() APIs.
> 
> This is fixed in v3.2.x because all of this code got rewritten for NUMA I/O
> support.
> 
> But both 3.0.x and 3.1.x still have this bug, and my Niagara sparc machines
> crash on shutdown every single time due to this bug making my -stable work
> more difficult than it needs to be.
> 
> Please apply to both 3.0.x-stable and 3.1.x-stable, thanks.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c
> b/drivers/scsi/mpt2sas/mpt2sas_base.c
> index 83035bd..8cec3bd 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> @@ -1096,7 +1096,7 @@ _base_save_msix_table(struct MPT2SAS_ADAPTER *ioc)
>  		return;
> 
>  	for (i = 0; i < ioc->msix_vector_count; i++)
> -		ioc->msix_table_backup[i] = ioc->msix_table[i];
> +		ioc->msix_table_backup[i] = readl(&ioc->msix_table[i]);
>  }
> 
>  /**
> @@ -1113,7 +1113,7 @@ _base_restore_msix_table(struct MPT2SAS_ADAPTER *ioc)
>  		return;
> 
>  	for (i = 0; i < ioc->msix_vector_count; i++)
> -		ioc->msix_table[i] = ioc->msix_table_backup[i];
> +		writel(ioc->msix_table_backup[i], &ioc->msix_table[i]);
>  }
> 
>  /**
> @@ -1144,7 +1144,7 @@ _base_check_enable_msix(struct MPT2SAS_ADAPTER *ioc)
>  	/* get msix table  */
>  	pci_read_config_dword(ioc->pdev, base + 4, &msix_table_offset);
>  	msix_table_offset &= 0xFFFFFFF8;
> -	ioc->msix_table = (u32 *)((void *)ioc->chip + msix_table_offset);
> +	ioc->msix_table = ((void __iomem *)ioc->chip + msix_table_offset);
> 
>  	dinitprintk(ioc, printk(MPT2SAS_INFO_FMT "msix is supported, "
>  	    "vector_count(%d), table_offset(0x%08x), table(%p)\n", ioc->name,
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
> b/drivers/scsi/mpt2sas/mpt2sas_base.h
> index 41a57a7..a8782f3 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_base.h
> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
> @@ -768,7 +768,7 @@ struct MPT2SAS_ADAPTER {
> 
>  	u8		msix_enable;
>  	u16		msix_vector_count;
> -	u32		*msix_table;
> +	u32 __iomem	*msix_table;
>  	u32		*msix_table_backup;
>  	u32		ioc_reset_count;
> 
> --
> 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

[-- Attachment #2: removing_msix_table_backup.patch --]
[-- Type: application/octet-stream, Size: 7904 bytes --]

diff --git a/mpt2sas_base.c b/mpt2sas_base.c
index 4136717..a186f5b 100644
--- a/mpt2sas_base.c
+++ b/mpt2sas_base.c
@@ -1245,41 +1245,6 @@ _base_config_dma_addressing(struct MPT2SAS_ADAPTER *ioc, struct pci_dev *pdev)
 #endif
 
 /**
- * mpt2sas_base_save_msix_table - backup msix vector table
- * @ioc: per adapter object
- *
- * This address an errata where diag reset clears out the table
- */
-void
-mpt2sas_base_save_msix_table(struct MPT2SAS_ADAPTER *ioc)
-{
-	int i;
-
-	if (!ioc->msix_enable || ioc->msix_table_backup == NULL)
-		return;
-
-	for (i = 0; i < ioc->msix_vector_count; i++)
-		ioc->msix_table_backup[i] = ioc->msix_table[i];
-}
-
-/**
- * mpt2sas_base_restore_msix_table - this restores the msix vector table
- * @ioc: per adapter object
- *
- */
-void
-mpt2sas_base_restore_msix_table(struct MPT2SAS_ADAPTER *ioc)
-{
-	int i;
-
-	if (!ioc->msix_enable || ioc->msix_table_backup == NULL)
-		return;
-
-	for (i = 0; i < ioc->msix_vector_count; i++)
-		ioc->msix_table[i] = ioc->msix_table_backup[i];
-}
-
-/**
  * _base_check_enable_msix - checks MSIX capabable.
  * @ioc: per adapter object
  *
@@ -1291,7 +1256,6 @@ _base_check_enable_msix(struct MPT2SAS_ADAPTER *ioc)
 {
 	int base;
 	u16 message_control;
-	u32 msix_table_offset;
 
 	base = pci_find_capability(ioc->pdev, PCI_CAP_ID_MSIX);
 	if (!base) {
@@ -1304,14 +1268,8 @@ _base_check_enable_msix(struct MPT2SAS_ADAPTER *ioc)
 	pci_read_config_word(ioc->pdev, base + 2, &message_control);
 	ioc->msix_vector_count = (message_control & 0x3FF) + 1;
 
-	/* get msix table  */
-	pci_read_config_dword(ioc->pdev, base + 4, &msix_table_offset);
-	msix_table_offset &= 0xFFFFFFF8;
-	ioc->msix_table = (u32 *)((void *)ioc->chip + msix_table_offset);
-
 	dinitprintk(ioc, printk(MPT2SAS_INFO_FMT "msix is supported, "
-	    "vector_count(%d), table_offset(0x%08x), table(%p)\n", ioc->name,
-	    ioc->msix_vector_count, msix_table_offset, ioc->msix_table));
+	    "vector_count(%d)\n", ioc->name, ioc->msix_vector_count));
 	return 0;
 }
 
@@ -3955,8 +3913,6 @@ _base_diag_reset(struct MPT2SAS_ADAPTER *ioc, int sleep_flag)
 
 	printk(MPT2SAS_INFO_FMT "sending diag reset !!\n", ioc->name);
 
-	mpt2sas_base_save_msix_table(ioc);
-
 	drsprintk(ioc, printk(MPT2SAS_INFO_FMT "clear interrupts\n",
 	    ioc->name));
 
@@ -4052,7 +4008,6 @@ _base_diag_reset(struct MPT2SAS_ADAPTER *ioc, int sleep_flag)
 		goto out;
 	}
 
-	mpt2sas_base_restore_msix_table(ioc);
 	printk(MPT2SAS_INFO_FMT "diag reset: SUCCESS\n", ioc->name);
 	return 0;
 
@@ -4210,7 +4165,8 @@ _base_make_ioc_operational(struct MPT2SAS_ADAPTER *ioc, int sleep_flag)
 		ioc->reply_free[i] = cpu_to_le32(reply_address);
 
 	/* initialize reply queues */
-	_base_assign_reply_queues(ioc);
+	if (ioc->is_driver_loading)
+		_base_assign_reply_queues(ioc);
 
 	/* initialize Reply Post Free Queue */
 	reply_post_free = (long)ioc->reply_post_free;
@@ -4224,19 +4180,28 @@ _base_make_ioc_operational(struct MPT2SAS_ADAPTER *ioc, int sleep_flag)
 			reply_q->reply_post_free[i].Words =
 			    cpu_to_le64(ULLONG_MAX);
 		if (!_base_is_controller_msix_enabled(ioc))
-			goto skip;
+			goto skip_init_reply_post_free_queue;
 		reply_post_free += reply_post_free_sz;
 	}
- skip:
+ skip_init_reply_post_free_queue:
 
 	r = _base_send_ioc_init(ioc, sleep_flag);
 	if (r)
 		return r;
 
-	/* initialize the index's */
+	/* initialize reply free host index */
 	ioc->reply_free_host_index = ioc->reply_free_queue_depth - 1;
 	writel(ioc->reply_free_host_index, &ioc->chip->ReplyFreeHostIndex);
-	writel(0, &ioc->chip->ReplyPostHostIndex);
+
+	/* initialize reply post host index */
+	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
+		writel(reply_q->msix_index << MPI2_RPHI_MSIX_INDEX_SHIFT,
+		    &ioc->chip->ReplyPostHostIndex);
+		if (!_base_is_controller_msix_enabled(ioc))
+			goto skip_init_reply_post_host_index;
+	}
+
+ skip_init_reply_post_host_index:
 
 	_base_unmask_interrupts(ioc);
 	r = _base_event_notification(ioc, sleep_flag);
@@ -4340,18 +4305,6 @@ mpt2sas_base_attach(struct MPT2SAS_ADAPTER *ioc)
 	if (r)
 		return r;
 
-	if (ioc->msix_enable) {
-		ioc->msix_table_backup = kcalloc(ioc->msix_vector_count,
-		    sizeof(u32), GFP_KERNEL);
-		if (!ioc->msix_table_backup) {
-			dfailprintk(ioc, printk(MPT2SAS_INFO_FMT
-			    "allocation for msix_table_backup failed!!!\n",
-			    ioc->name));
-			r = -ENOMEM;
-			goto out_free_resources;
-		}
-	}
-
 	pci_set_drvdata(ioc->pdev, ioc->shost);
 	r = _base_get_ioc_facts(ioc, CAN_SLEEP);
 	if (r)
@@ -4484,7 +4437,6 @@ mpt2sas_base_attach(struct MPT2SAS_ADAPTER *ioc)
 	kfree(ioc->cpu_msix_table);
 	kfree(ioc->pd_handles);
 	kfree(ioc->pend_os_device_add);
-	kfree(ioc->msix_table_backup);
 	kfree(ioc->tm_cmds.reply);
 	kfree(ioc->transport_cmds.reply);
 	kfree(ioc->scsih_cmds.reply);
@@ -4526,7 +4478,6 @@ mpt2sas_base_detach(struct MPT2SAS_ADAPTER *ioc)
 	kfree(ioc->cpu_msix_table);
 	kfree(ioc->pd_handles);
 	kfree(ioc->pend_os_device_add);
-	kfree(ioc->msix_table_backup);
 	kfree(ioc->pfacts);
 	kfree(ioc->ctl_cmds.reply);
 	kfree(ioc->ctl_cmds.sense);
diff --git a/mpt2sas_base.h b/mpt2sas_base.h
index 6602a7f..05bb541 100644
--- a/mpt2sas_base.h
+++ b/mpt2sas_base.h
@@ -71,11 +71,11 @@
 #define MPT2SAS_DRIVER_NAME		"mpt2sas"
 #define MPT2SAS_AUTHOR	"LSI Corporation <DL-MPTFusionLinux@lsi.com>"
 #define MPT2SAS_DESCRIPTION	"LSI MPT Fusion SAS 2.0 Device Driver"
-#define MPT2SAS_DRIVER_VERSION		"09.255.02.04"
+#define MPT2SAS_DRIVER_VERSION		"09.255.02.05"
 #define MPT2SAS_MAJOR_VERSION		9
 #define MPT2SAS_MINOR_VERSION		255
 #define MPT2SAS_BUILD_VERSION		2
-#define MPT2SAS_RELEASE_VERSION		4
+#define MPT2SAS_RELEASE_VERSION		5
 
 /*
  * Set MPT2SAS_SG_DEPTH value based on user input.
@@ -683,7 +683,6 @@ enum mutex_type {
  * @msix_enable: flag indicating msix is enabled
  * @msix_vector_count: number msix vectors
  * @msix_table: virt address to the msix table
- * @msix_table_backup: backup msix table
  * @cpu_msix_table: table for mapping cpus to msix index
  * @cpu_msix_table_sz: table size
  * @scsi_io_cb_idx: shost generated commands
@@ -859,8 +858,6 @@ struct MPT2SAS_ADAPTER {
 
 	u8		msix_enable;
 	u16		msix_vector_count;
-	u32		*msix_table;
-	u32		*msix_table_backup;
 	u8		*cpu_msix_table;
 	u16		cpu_msix_table_sz;
 	u32		ioc_reset_count;
@@ -1044,8 +1041,6 @@ typedef u8 (*MPT_CALLBACK)(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
 extern struct list_head mpt2sas_ioc_list;
 void mpt2sas_base_start_watchdog(struct MPT2SAS_ADAPTER *ioc);
 void mpt2sas_base_stop_watchdog(struct MPT2SAS_ADAPTER *ioc);
-void mpt2sas_base_save_msix_table(struct MPT2SAS_ADAPTER *ioc);
-void mpt2sas_base_restore_msix_table(struct MPT2SAS_ADAPTER *ioc);
 
 int mpt2sas_base_attach(struct MPT2SAS_ADAPTER *ioc);
 void mpt2sas_base_detach(struct MPT2SAS_ADAPTER *ioc);
diff --git a/mpt2sas_scsih.c b/mpt2sas_scsih.c
index faccdf1..ae582a9 100644
--- a/mpt2sas_scsih.c
+++ b/mpt2sas_scsih.c
@@ -9685,9 +9685,6 @@ _scsih_suspend(struct pci_dev *pdev, pm_message_t state)
 	    "operating state [D%d]\n", ioc->name, pdev,
 	    pci_name(pdev), device_state);
 
-#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,6,18))
-	mpt2sas_base_save_msix_table(ioc);
-#endif
 	pci_save_state(pdev);
 	mpt2sas_base_free_resources(ioc);
 	pci_set_power_state(pdev, device_state);
@@ -9720,9 +9717,6 @@ _scsih_resume(struct pci_dev *pdev)
 	if (r)
 		return r;
 
-#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,6,18))
-	mpt2sas_base_restore_msix_table(ioc);
-#endif
 	mpt2sas_base_hard_reset_handler(ioc, CAN_SLEEP, SOFT_RESET);
 	scsi_unblock_requests(shost);
 	mpt2sas_base_start_watchdog(ioc);
@@ -9796,10 +9790,6 @@ _scsih_pci_slot_reset(struct pci_dev *pdev)
 	if (rc)
 		return PCI_ERS_RESULT_DISCONNECT;
 
-#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,6,18))
-	mpt2sas_base_restore_msix_table(ioc);
-#endif
-
 	rc = mpt2sas_base_hard_reset_handler(ioc, CAN_SLEEP,
 	    FORCE_BIG_HAMMER);
 

Re: [PATCH]: mpt2sas crashes on shutdown

[David Miller]

From: "Moore, Eric" <Eric.Moore@lsi.com>
Date: Tue, 27 Dec 2011 10:09:28 -0700

> David - The backup and restoring of the MSIX vector capabilities registers across diagnostic reset was only needed for our pre-production cards (A0 parts). It was a workaround for some hardware errata.  This is not needed anymore as there was a fix addressed before those controllers when to production, so I finally removed this workaround in the driver during phase 10 in the LSI in-house drivers, which was about 10 months ago.  Attached is my commit for the in-house drivers.  You should be able to remove the save_msix_table and restore_msix_table routines as I did.

I don't care how it is fixed, I only care that a fix makes it's way into -stable.

You LSI guys seem to ignore the -stable series and submit _ZERO_ bug
fixes for your driver there, leaving it to people like me suffering
from the bugs to submit the fixes for you.

Therefore it is kind of out of place for you to suggest a specific way you want
the bug fixed, because obviously you guys don't care about backporting bug
fixes to -stable in the first place.

With that in mind, why not leave the decision to people who actually
care, have actually tested the fix, and bothered to submit it to -stable?

Re: [PATCH]: mpt2sas crashes on shutdown

[Douglas Gilbert]

On 11-12-27 01:05 PM, David Miller wrote:
> From: "Moore, Eric"<Eric.Moore@lsi.com>
> Date: Tue, 27 Dec 2011 10:09:28 -0700
>
>> David - The backup and restoring of the MSIX vector capabilities registers across diagnostic reset was only needed for our pre-production cards (A0 parts). It was a workaround for some hardware errata.  This is not needed anymore as there was a fix addressed before those controllers when to production, so I finally removed this workaround in the driver during phase 10 in the LSI in-house drivers, which was about 10 months ago.  Attached is my commit for the in-house drivers.  You should be able to remove the save_msix_table and restore_msix_table routines as I did.
>
> I don't care how it is fixed, I only care that a fix makes it's way into -stable.
>
> You LSI guys seem to ignore the -stable series and submit _ZERO_ bug
> fixes for your driver there, leaving it to people like me suffering
> from the bugs to submit the fixes for you.
>
> Therefore it is kind of out of place for you to suggest a specific way you want
> the bug fixed, because obviously you guys don't care about backporting bug
> fixes to -stable in the first place.
>
> With that in mind, why not leave the decision to people who actually
> care, have actually tested the fix, and bothered to submit it to -stable?

... and goodwill to all.

Re: [PATCH]: mpt2sas crashes on shutdown

[David Miller]

From: Douglas Gilbert <dgilbert@interlog.com>
Date: Tue, 27 Dec 2011 13:24:19 -0500

> ... and goodwill to all.

Think I'm being unreasonable?  You really don't get it do you?

Do you not see how self-serving and ironic it is that a driver
maintainer only cares about -stable submissions when someone else
tries to take care of the bugs for them?

And in doing so, all of a sudden cares enough to say that the fix
should be done in a certain way?

If they cared so much, they should be feeding bug fixes to -stable in the
first place instead of relying upon others to do so.