SCSI fixes for 4.10-rc3

The major fix is the bfa firmware, since the latest 10Gb cards fail probing with the current firmware. The rest is a set of minor fixes:
one missed Kconfig dependency causing randconfig failures, a missed error return on an error leg, a change for how multiqueue waits on a blocked device and a don't reset while in reset fix.

The patch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-fixes

The short changelog is:

Bart Van Assche (1):
scsi: scsi-mq: Wait for .queue_rq() if necessary

Benjamin Poirier (1):
scsi: bfa: Increase requested firmware version to 3.2.5.1

Burak Ok (1):
scsi: snic: Return error code on memory allocation failure

Randy Dunlap (1):
scsi: qedi: fix build, depends on UIO

Satish Kharat (1):
scsi: fnic: Avoid sending reset to firmware when another reset is in progress

And the diffstat:

 drivers/scsi/bfa/bfad.c       |  6 +++---
 drivers/scsi/bfa/bfad_drv.h   |  2 +-
 drivers/scsi/fnic/fnic.h      |  1 +
 drivers/scsi/fnic/fnic_scsi.c | 16 ++++++++++++++++
 drivers/scsi/qedi/Kconfig     |  2 +-
 drivers/scsi/scsi_lib.c       |  2 +-
 drivers/scsi/snic/snic_main.c |  3 +++
 7 files changed, 26 insertions(+), 6 deletions(-)

With full diff below.

James

---

index d9e1521..5caf5f3 100644
@@ -64,9 +64,9 @@ int		max_rport_logins = BFA_FCS_MAX_RPORT_LOGINS;
 u32	bfi_image_cb_size, bfi_image_ct_size, bfi_image_ct2_size;
 u32	*bfi_image_cb, *bfi_image_ct, *bfi_image_ct2;
 
-#define BFAD_FW_FILE_CB		"cbfw-3.2.3.0.bin"
-#define BFAD_FW_FILE_CT		"ctfw-3.2.3.0.bin"
-#define BFAD_FW_FILE_CT2	"ct2fw-3.2.3.0.bin"
+#define BFAD_FW_FILE_CB		"cbfw-3.2.5.1.bin"
+#define BFAD_FW_FILE_CT		"ctfw-3.2.5.1.bin"
+#define BFAD_FW_FILE_CT2	"ct2fw-3.2.5.1.bin"
 
 static u32 *bfad_load_fwimg(struct pci_dev *pdev);
 static void bfad_free_fwimg(void);

index f9e8620..cfcfff4 100644
@@ -58,7 +58,7 @@
 #ifdef BFA_DRIVER_VERSION
 #define BFAD_DRIVER_VERSION    BFA_DRIVER_VERSION
 #else
-#define BFAD_DRIVER_VERSION    "3.2.25.0"
+#define BFAD_DRIVER_VERSION    "3.2.25.1"
 #endif
 
 #define BFAD_PROTO_NAME FCPI_NAME

index 9ddc920..9e4b770 100644
@@ -248,6 +248,7 @@ struct fnic {
 	struct completion *remove_wait; /* device remove thread blocks */
 
 	atomic_t in_flight;		/* io counter */
+	bool internal_reset_inprogress;
 	u32 _reserved;			/* fill hole */
 	unsigned long state_flags;	/* protected by host lock */
 	enum fnic_state state;

index 2544a37..adb3d58 100644
@@ -2581,6 +2581,19 @@ int fnic_host_reset(struct scsi_cmnd *sc)
 	unsigned long wait_host_tmo;
 	struct Scsi_Host *shost = sc->device->host;
 	struct fc_lport *lp = shost_priv(shost);
+	struct fnic *fnic = lport_priv(lp);
+	unsigned long flags;
+
+	spin_lock_irqsave(&fnic->fnic_lock, flags);
+	if (fnic->internal_reset_inprogress == 0) {
+		fnic->internal_reset_inprogress = 1;
+	} else {
+		spin_unlock_irqrestore(&fnic->fnic_lock, flags);
+		FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+			"host reset in progress skipping another host reset\n");
+		return SUCCESS;
+	}
+	spin_unlock_irqrestore(&fnic->fnic_lock, flags);
 
 	/*
 	 * If fnic_reset is successful, wait for fabric login to complete

@@ -2601,6 +2614,9 @@ int fnic_host_reset(struct scsi_cmnd *sc)
 		}
 	}
 
+	spin_lock_irqsave(&fnic->fnic_lock, flags);
+	fnic->internal_reset_inprogress = 0;
+	spin_unlock_irqrestore(&fnic->fnic_lock, flags);
 	return ret;
 }
 

index 23ca8a2..2133145 100644
@@ -1,6 +1,6 @@
 config QEDI
 	tristate "QLogic QEDI 25/40/100Gb iSCSI Initiator Driver Support"
-	depends on PCI && SCSI
+	depends on PCI && SCSI && UIO
 	depends on QED
 	select SCSI_ISCSI_ATTRS
 	select QED_LL2

index c35b6de..9fd9a97 100644
@@ -2893,7 +2893,7 @@ scsi_internal_device_block(struct scsi_device *sdev)
 	 * request queue. 
 	 */
 	if (q->mq_ops) {
-		blk_mq_stop_hw_queues(q);
+		blk_mq_quiesce_queue(q);
 	} else {
 		spin_lock_irqsave(q->queue_lock, flags);
 		blk_stop_queue(q);

index 396b32d..7cf70aa 100644
@@ -591,6 +591,7 @@ snic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!pool) {
 		SNIC_HOST_ERR(shost, "dflt sgl pool creation failed\n");
 
+		ret = -ENOMEM;
 		goto err_free_res;
 	}
 

@@ -601,6 +602,7 @@ snic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!pool) {
 		SNIC_HOST_ERR(shost, "max sgl pool creation failed\n");
 
+		ret = -ENOMEM;
 		goto err_free_dflt_sgl_pool;
 	}
 

@@ -611,6 +613,7 @@ snic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!pool) {
 		SNIC_HOST_ERR(shost, "snic tmreq info pool creation failed.\n");
 
+		ret = -ENOMEM;
 		goto err_free_max_sgl_pool;
 	}
 

So there's a new mpt3sas SCSI driver boot regression, introduced in this merge window, which made one of my servers unbootable.

The kernel, starting at upstream commit a829a8445f09, would hang thusly:

[    6.230363] Linux agpgart interface v0.103
[    6.245029] brd: module loaded
[    6.253233] loop: module loaded
[    6.256695] mpt3sas version 14.101.00.00 loaded
[    6.261890] mpt2sas_cm0: 64 BIT PCI BUS DMA ADDRESSING SUPPORTED, total mem (65950628 kB)
[    6.326222] mpt2sas_cm0: MSI-X vectors supported: 1, no of cores: 32, max_msix_vectors: -1
[    6.334953] mpt2sas0-msix0: PCI-MSI-X enabled: IRQ 24
[    6.340237] mpt2sas_cm0: iomem(0x00000000dff3c000), mapped(0xffffc90007414000), size(16384)
[    6.349002] mpt2sas_cm0: ioport(0x000000000000e000), size(256)
[    6.410830] mpt2sas_cm0: sending message unit reset !!
[    6.417739] mpt2sas_cm0: message unit reset: SUCCESS
[    6.463486] mpt2sas_cm0: Allocated physical memory: size(8199 kB)
[    6.469820] mpt2sas_cm0: Current Controller Queue Depth(3640),Max Controller Queue Depth(3712)
[    6.478840] mpt2sas_cm0: Scatter Gather Elements per IO(128)
[    6.530653] mpt2sas_cm0: LSISAS2008: FWVersion(12.00.00.00), ChipRevision(0x03), BiosVersion(07.23.01.00)
[    6.540621] mpt2sas_cm0: Protocol=(
[    6.540622] Initiator
[    6.544346] ,Target
[    6.546844] ), 
[    6.549168] Capabilities=(
[    6.551165] TLR
[    6.554098] ,EEDP
[    6.556095] ,Snapshot Buffer
[    6.558249] ,Diag Trace Buffer
[    6.561359] ,Task Set Full
[    6.564666] ,NCQ
[    6.567594] )
[    6.571517] scsi host0: Fusion MPT SAS Host
[    6.576539] mpt2sas_cm0: sending port enable !!
[    6.576699] ahci 0000:00:11.0: version 3.0
[    6.577285] ahci 0000:00:11.0: AHCI 0001.0100 32 slots 4 ports 3 Gbps 0xf impl SATA mode
[    6.577290] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck pm led clo pmp pio slum part ccc 
[    6.579218] scsi host1: ahci
[    6.579685] scsi host2: ahci
[    6.5800[   39.972084] sd 0:0:0:0: attempting task abort! scmd(ffff881014cb9500)
[   39.978809] sd 0:0:0:0: [sda] tag#0 CDB: ATA command pass through(12)/Blank a1 08 2e 00 01 00 00 00 00 ec 00 00
[   39.989346] scsi target0:0:0: handle(0x0009), sas_address(0x4433221100000000), phy(0)
[   39.997584] scsi target0:0:0: enclosure_logical_id(0x5003048003e10c00), slot(31)
[   40.005425] sd 0:0:0:0: task abort: SUCCESS scmd(ffff881014cb9500)

udevd[472]: timeout 'ata_id --export /dev/sda' udevd[472]: timeout: killing 'ata_id --export /dev/sda' [503] udevd[472]: timeout: killing 'ata_id --export /dev/sda' [503] udevd[472]: timeout: killing 'ata_id --export /dev/sda' [503] udevd[472]: timeout: killing 'ata_id --export /dev/sda' [503] udevd[472]: timeout: killing 'ata_id --export /dev/sda' [503] udevd[472]: timeout: killing 'ata_id --export /dev/sda' [503] udevd[472]: timeout: killing 'ata_id --export /dev/sda' [503] udevd[472]: timeout: killing 'ata_id --export /dev/sda' [503]

[ this would continue ad infinitum. ]

The correct bootup sequence would be:

[    6.252918] loop: module loaded
[    6.256390] mpt3sas version 14.101.00.00 loaded
[    6.261554] mpt2sas_cm0: 64 BIT PCI BUS DMA ADDRESSING SUPPORTED, total mem (65950628 kB)
[    6.325894] mpt2sas_cm0: MSI-X vectors supported: 1, no of cores: 32, max_msix_vectors: -1
[    6.334640] mpt2sas0-msix0: PCI-MSI-X enabled: IRQ 24
[    6.339925] mpt2sas_cm0: iomem(0x00000000dff3c000), mapped(0xffffc900073f4000), size(16384)
[    6.348672] mpt2sas_cm0: ioport(0x000000000000e000), size(256)
[    6.410508] mpt2sas_cm0: sending message unit reset !!
[    6.417437] mpt2sas_cm0: message unit reset: SUCCESS
[    6.463275] mpt2sas_cm0: Allocated physical memory: size(8199 kB)
[    6.469627] mpt2sas_cm0: Current Controller Queue Depth(3640),Max Controller Queue Depth(3712)
[    6.478635] mpt2sas_cm0: Scatter Gather Elements per IO(128)
[    6.530433] mpt2sas_cm0: LSISAS2008: FWVersion(12.00.00.00), ChipRevision(0x03), BiosVersion(07.23.01.00)
[    6.540424] mpt2sas_cm0: Protocol=(
[    6.540425] Initiator
[    6.544150] ,Target
[    6.546644] ), 
[    6.548968] Capabilities=(
[    6.550943] TLR
[    6.553901] ,EEDP
[    6.555898] ,Snapshot Buffer
[    6.558050] ,Diag Trace Buffer
[    6.561159] ,Task Set Full
[    6.564462] ,NCQ
[    6.567395] )
[    6.571316] scsi host0: Fusion MPT SAS Host
[    6.576344] mpt2sas_cm0: sending port enable !!
[    6.576495] ahci 0000:00:11.0: version 3.0
[    6.577100] ahci 0000:00:11.0: AHCI 0001.0100 32 slots 4 ports 3 Gbps 0xf impl SATA mode
[    6.577105] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck pm led clo pmp pio slum part ccc 
[    6.579016] scsi host1: ahci
[    6.579387] scsi host2: ahci

[ 6.[
[32m OK
[0m] Started Journal Service.
...

(BTW., note the various broken printk lines - which is an unrelated bug.)

I bisected the regression back to this upstream merge commit done by Linus:

commit a829a8445f09036404060f4d6489cb13433f4304
Merge: 84b607913442 f5b893c94715
Author: Linus Torvalds <[email protected]>
Date: Wed Dec 14 10:49:33 2016 -0800

Merge tag 'scsi-misc' of git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi

... which is a head-scratcher, so I double checked the key bisection points, but the bisection result is robust. I also re-created Linus's merge and double checked the conflict resolution - which looks fine as well.

After (much) more testing it turns out that the bug is some sort of combination bug, in that scsi-next didn't have all the SCSI fixes that upstream already had, in particular it didn't have these commits:

7ff723ad0f87 scsi: mpt3sas: Unblock device after controller reset
18f6084a989b scsi: mpt3sas: Fix secure erase premature termination 6d3a56ed0985 scsi: mpt3sas: Fix for block device of raid exists even after deleting raid disk

When Linus pulled in scsi-next-minus-fixes these two sets of commits combined and produced the regression - and made the bisection lead to the merge commit.

So I manually rebased those 3 fixes on top of the scsi-next tree (f5b893c94715) and indeed one of them broke my box:

18f6084a989b scsi: mpt3sas: Fix secure erase premature termination

I reverted it from latest upstream (with a minor conflict resolution), and that makes my box boot fine again. I have no idea which scsi-next commit this change interacted with, and it's not easy to find out so I'm not volunteering! It must be one of these 256 commits:

e3a00f68e426..f5b893c94715

Note that reverting the first commit alone does not help:

7ff723ad0f87 scsi: mpt3sas: Unblock device after controller reset

So it's reverting 18f6084a989b (while keeping ata_12_16_cmd() around to enable the 7ff723ad0f87 fix) that does the trick.

Thanks,

Ingo

====================>

From: Ingo Molnar <[email protected]>
Date: Sun, 15 Jan 2017 09:59:39 +0100
Subject: [PATCH] Revert "scsi: mpt3sas: Fix secure erase premature termination"

This reverts commit 18f6084a989ba1b38702f9af37a2e4049a924be6.

Conflicts:
drivers/scsi/mpt3sas/mpt3sas_scsih.c

---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 10 ----------
 1 file changed, 10 deletions(-)

index b5c966e319d3..3573daa2cce8 100644
@@ -4063,13 +4063,6 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
 	if (ioc->logging_level & MPT_DEBUG_SCSI)
 		scsi_print_command(scmd);
 
-	/*
-	 * Lock the device for any subsequent command until command is
-	 * done.
-	 */
-	if (ata_12_16_cmd(scmd))
-		scsi_internal_device_block(scmd->device);
-
 	sas_device_priv_data = scmd->device->hostdata;
 	if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
 		scmd->result = DID_NO_CONNECT << 16;

@@ -4650,9 +4643,6 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
 	if (scmd == NULL)
 		return 1;
 
-	if (ata_12_16_cmd(scmd))
-		scsi_internal_device_unblock(scmd->device, SDEV_RUNNING);
-
 	mpi_request = mpt3sas_base_get_msg_frame(ioc, smid);
 
 	if (mpi_reply == NULL) {

We're not reverting a fix that would cause regressions for others.
However, The fix was manifestly wrong, so does this fix of the fix
work for you:

http://marc.info/?l=linux-scsi&m=148329237807604

It's been languishing a bit because no-one seemed to care enough to test or review it. IOf you can add a tested by, that will give the two we need to push it.

James

Oh HELL YES we are.

The rule is that we never break old stuff. Some new fix that fixes something that never used to work, but breaks something else, gets reverted very aggressively.

So if a new bugfix or workaround causes problems for existing users, it gets reverted. The fact that it fixed something else is COMPLETELY IRRELEVANT.

We do not do the "one step forward, two steps back" dance. If you can't fix a bug without breaking old systems, the "fix" gets reverted.

Apparently there is already a possible real fix in flight, so I won't actually do the revert, but I very much want to object to your statement.

Reverts happen.

Linus

Can we compromise on "try not to revert a fix ...". The problem with bugs in regression fixes is that we now have two constituencies: the people who get the regression back if we revert the fix and the people who are bitten by the bug in the original regression fix. In this particular case, I think we should always try to fix the fix because reversion also violates "never break old stuff". There are corner cases, of course, like if the latter constituency is much bigger and the fix is hard to fix, then we might revert and try again.

James

No.

It's about timing, and about how serious the regression is.

For example, if this happened in rc7, I would have reverted immediately. No questions asked.

In this case, the "fix" was was also much less important then the problem it caused. Some specialized pass-through command not working right, vs a machine not even booting? There's just no question what-so-ever.

So the "fix" you claim just wasn't nearly important enough. It was also pretty recent and clearly things had worked for _years_ without it.

In fact, I'm still somewhat inclined to revert it, just to have a working rc4 release later today. But I'm hoping maybe Ingo has time to test things (although I suspect he's already asleep).

Linus

OK, so the patch to revert would actually be

commit 669f044170d8933c3d66d231b69ea97cb8447338

Author: Bart Van Assche <[email protected]>
Date:   Tue Nov 22 16:17:13 2016 -0800

scsi: srp_transport: Move queuecommand() wait code to SCSI core

Because that change in the wait code broke the "fix" in mpt3sas.
Before that was applied, it actually worked even though I think it's a
wrong fix.

James

Hello James,

I disagree. Even if my patch would be reverted that still wouldn't fix the severe race condition that was introduced in the mpt3sas driver by the patch that triggers the lockup during boot. As I explained two weeks ago (see also https://www.spinics.net/lists/kernel/msg2411413.htm
l), commit 18f6084a989b ("scsi: mpt3sas: Fix secure erase premature termination") is the one that should be reverted instead of my patch. I agree with Linus that the offending mpt3sas patch already should have been reverted.

Bart.

In addition to that ATA passthrough through scsi CDBs is a fringe feature compared to normal operation of the HBA.

So how about, instead of arguing about reversion, one or other of you reviews the proposed fix of the fix which would avoid having to revert anything?

James

As far as I can tell everyone is waiting for you to resend it with the review comment from the Broadcom maintainers adddressed.

You really need to reconsider that stance ...

I have tested your other patch that you pointed to:

http://marc.info/?l=linux-scsi&m=148449968522828

Which patch fixes the bug too (I removed my revert first) - so you can add my:

BTW., is it wise to work around the out of spec firmware in the mpt3sas code and leave the overly optimistic assumptions in the SCSI code intact? The problem is that other SCSI hardware could be affected as well - and especially enterprise class server hardware has long testing and thus regression latencies (as my example proves).

Wouldn't it be more robust to only submit one pass-through command at a time from the SCSI layer, and maybe opt-in hardware that is known to implement the SAT standard fully?

(But I'm just kibitzing here really.)

Thanks,

Ingo

Thanks ... just checking you tested the second version with the concurrency part?

Realistically, there is no other card. Every other SAS implementation uses the in-kernel SAT, which does the right thing. We've suggested on a few occasions that the mpt SAS might like to use it as well, given we keep tripping on SAT problems in their firmware.

Unfortunately it's a lot more complex: the standard gives a queueing mechanism for SAT pass through, so mostly you *can* have multiple commands outstanding, so it looks like we shouldn't globally restrict that. However, it seems the mpt3 firmware is using a queue single command model *and* not doing the right thing with return codes hence the failure. Since the failure mode is mpt3 specific, I think the best place for the fix is in their code. We can revisit this decision if something else comes along that also has this problem (UAS springs to mind).

James

I tested the one I linked to:

http://marc.info/?l=linux-scsi&m=148449968522828

I don't know which version that is exactly, I just tested what you asked me to test.

Thanks,

Ingo