Skip to contentSkip to conversation

Fusion SAS and Fibre Channel: target missing after resetting external raid

PATCH In review Michael Reed 2 participants 4 messages Mar 4, 2008 – Mar 20, 2008

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

Michael Reed

author
#0 lore raw

PATCH 1/1 Fusion SAS and Fibre Channel: target missing after resetting external raid

Following a hard reset of a SAS raid, one of the raid targets is occasionally missing. I tracked this down to a pretty obscure little bug.

The LSI fusion drivers for SAS and Fibre Channel both use their respective transport layers. Those transport layers increment the target number assigned to new targets.

The routine __scsi_scan_target uses the "this_id" element of the Scsi_Host structure to avoid scanning the scsi host adapter. Both fusion drivers set "this_id" from a value returned in a firmware PortFacts response. For my particular test case (SAS) the firmware id assigned to the initiator was
173. After enough raid resets to cause the raid targets to go and come a sufficient number of times, the id assigned by the transport to a raid target would match the id assigned by the host adapter to the "this_id" field, resulting in that target not being scanned.

static void __scsi_scan_target(struct device *parent, unsigned int channel,
unsigned int id, unsigned int lun, int rescan)

{
        struct Scsi_Host *shost = dev_to_shost(parent);
        int bflags = 0;
        int res;
        struct scsi_target *starget;

        if (shost->this_id == id)
                /*
                 * Don't scan the host adapter
                 */
                return;

....

The fix is simple. Fusion SAS and Fibre Channel (subject to same bug) should just leave "this_id" initialized to "-1".

Applies to 2.6.25-rc3-git5.

Signed-off-by Michael Reed <[email protected]>

--

drivers/message/fusion/mptfc.c +0 -2
@@ -1238,8 +1238,6 @@ mptfc_probe(struct pci_dev *pdev, const 
 	sh->max_id = ioc->pfacts->MaxDevices;
 	sh->max_lun = max_lun;
 
-	sh->this_id = ioc->pfacts[0].PortSCSIID;
-
 	/* Required entry.
 	 */
 	sh->unique_id = ioc->id;
drivers/message/fusion/mptsas.c +0 -2
@@ -3176,8 +3176,6 @@ mptsas_probe(struct pci_dev *pdev, const
 
 	sh->transportt = mptsas_transport_template;
 
-	sh->this_id = ioc->pfacts[0].PortSCSIID;
-
 	/* Required entry.
 	 */
 	sh->unique_id = ioc->id;
#1 lore raw
Quoted 74 lines · Michael Reed

Subject: [PATCH 1/1] Fusion SAS and Fibre Channel: target missing after resetting external raid

Following a hard reset of a SAS raid, one of the raid targets is occasionally
missing. I tracked this down to a pretty obscure little bug.

The LSI fusion drivers for SAS and Fibre Channel both use their respective
transport layers. Those transport layers increment the target number assigned to new targets.

The routine __scsi_scan_target uses the "this_id" element of the Scsi_Host
structure to avoid scanning the scsi host adapter. Both fusion drivers set
"this_id" from a value returned in a firmware PortFacts response. For my
particular test case (SAS) the firmware id assigned to the initiator was
173. After enough raid resets to cause the raid targets to go and come a
sufficient number of times, the id assigned by the transport to a raid target would match the id assigned by the host adapter to the "this_id"
field, resulting in that target not being scanned.

static void __scsi_scan_target(struct device *parent, unsigned int channel,
unsigned int id, unsigned int lun, int rescan)

{
        struct Scsi_Host *shost = dev_to_shost(parent);
        int bflags = 0;
        int res;
        struct scsi_target *starget;

        if (shost->this_id == id)
                /*
                 * Don't scan the host adapter
                 */
                return;

....

The fix is simple. Fusion SAS and Fibre Channel (subject to same bug) should
just leave "this_id" initialized to "-1".

Applies to 2.6.25-rc3-git5.

Signed-off-by Michael Reed <[email protected]>

--

--- kou/drivers/message/fusion/mptfc.c 2008-01-24
16:58:37.000000000 -0600
+++ ko/drivers/message/fusion/mptfc.c 2008-03-04
09:01:18.428176326 -0600

drivers/message/fusion/mptfc.c +0 -3
@@ -1238,8 +1238,6 @@ mptfc_probe(struct pci_dev *pdev, const 
 	sh->max_id = ioc->pfacts->MaxDevices;
 	sh->max_lun = max_lun;
 
-	sh->this_id = ioc->pfacts[0].PortSCSIID;
-
 	/* Required entry.
 	 */
 	sh->unique_id = ioc->id;
--- kou/drivers/message/fusion/mptsas.c	2008-03-04

08:38:58.000000000 -0600
+++ ko/drivers/message/fusion/mptsas.c 2008-03-04
09:01:04.284807301 -0600

drivers/message/fusion/mptsas.c mptsas_probe(struct pci_dev *pdev, const · line 3181

Quoted 8 lines
drivers/message/fusion/mptsas.c +0 -2
@@ -3176,8 +3176,6 @@ mptsas_probe(struct pci_dev *pdev, const
 
 	sh->transportt = mptsas_transport_template;
 
-	sh->this_id = ioc->pfacts[0].PortSCSIID;
-
 	/* Required entry.
 	 */
 	sh->unique_id = ioc->id;

This looks good. I had deleted setting this_id in mptsas internal
sources long ago. In addition to this change, we need to fix
mptscsih_slave_configure so it doesn't set the queue depth to 1 for SAS
protocal when sdev->id is greater than sh->max_id. The sas transport
layer assigns the target ids, incrementing with each hotplug add, with large topologies, it doesn't take long to hit this threshold.

Eric

author
#2 lore raw
Quoted 1 line · Eric
Quoted 1 line · Michael Reed

<snip>

Quoted 10 lines
Quoted 10 lines

The fix is simple. Fusion SAS and Fibre Channel (subject to same bug) should just leave "this_id" initialized to "-1".

Applies to 2.6.25-rc3-git5.

Signed-off-by Michael Reed <[email protected]>

--

<snip>

Quoted 9 lines

This looks good. I had deleted setting this_id in mptsas internal
sources long ago. In addition to this change, we need to fix
mptscsih_slave_configure so it doesn't set the queue depth to 1 for SAS
protocal when sdev->id is greater than sh->max_id. The sas transport
layer assigns the target ids, incrementing with each hotplug add, with large topologies, it doesn't take long to hit this threshold.

Eric

Adjusting the patch to reflect Eric's comments. The change WRT max_id is from Hannes. FC should also be subject to the same issue.

Signed-off-by Michael Reed <[email protected]>
drivers/message/fusion/mptfc.c +0 -2
@@ -1238,8 +1238,6 @@ mptfc_probe(struct pci_dev *pdev, const 
 	sh->max_id = ioc->pfacts->MaxDevices;
 	sh->max_lun = max_lun;
 
-	sh->this_id = ioc->pfacts[0].PortSCSIID;
-
 	/* Required entry.
 	 */
 	sh->unique_id = ioc->id;
drivers/message/fusion/mptsas.c +0 -2
@@ -3181,8 +3181,6 @@ mptsas_probe(struct pci_dev *pdev, const
 
 	sh->transportt = mptsas_transport_template;
 
-	sh->this_id = ioc->pfacts[0].PortSCSIID;
-
 	/* Required entry.
 	 */
 	sh->unique_id = ioc->id;
drivers/message/fusion/mptscsih.c +0 -8
@@ -2442,12 +2442,6 @@ mptscsih_slave_configure(struct scsi_dev
 		    ioc->name, sdev->sdtr, sdev->wdtr,
 		    sdev->ppr, sdev->inquiry_len));
 
-	if (sdev->id > sh->max_id) {
-		/* error case, should never happen */
-		scsi_adjust_queue_depth(sdev, 0, 1);
-		goto slave_configure_exit;
-	}
-
 	vdevice->configured_lun = 1;
 	mptscsih_change_queue_depth(sdev, MPT_SCSI_CMD_PER_DEV_HIGH);
 

@@ -2461,8 +2455,6 @@ mptscsih_slave_configure(struct scsi_dev
 		    ioc->name, vtarget->negoFlags, vtarget->maxOffset,
 		    vtarget->minSyncFactor));
 
-slave_configure_exit:
-
 	dsprintk(ioc, printk(MYIOC_s_DEBUG_FMT
 		"tagged %d, simple %d, ordered %d\n",
 		ioc->name,sdev->tagged_supported, sdev->simple_tags,
#3 lore raw
Quoted 50 lines · Michael Reed

Adjusting the patch to reflect Eric's comments. The change WRT max_id is from Hannes. FC should also be subject to the same issue.

Signed-off-by Michael Reed <[email protected]>

--- gitu/drivers/message/fusion/mptfc.c 2008-01-24
14:58:37.000000000 -0800
+++ git/drivers/message/fusion/mptfc.c 2008-03-13
12:37:52.176015612 -0700

drivers/message/fusion/mptfc.c +0 -3
@@ -1238,8 +1238,6 @@ mptfc_probe(struct pci_dev *pdev, const 
 	sh->max_id = ioc->pfacts->MaxDevices;
 	sh->max_lun = max_lun;
 
-	sh->this_id = ioc->pfacts[0].PortSCSIID;
-
 	/* Required entry.
 	 */
 	sh->unique_id = ioc->id;
--- gitu/drivers/message/fusion/mptsas.c	2008-03-10

13:23:46.000000000 -0700
+++ git/drivers/message/fusion/mptsas.c 2008-03-13
12:38:01.912440308 -0700

drivers/message/fusion/mptsas.c +0 -3
@@ -3181,8 +3181,6 @@ mptsas_probe(struct pci_dev *pdev, const
 
 	sh->transportt = mptsas_transport_template;
 
-	sh->this_id = ioc->pfacts[0].PortSCSIID;
-
 	/* Required entry.
 	 */
 	sh->unique_id = ioc->id;
--- gitu/drivers/message/fusion/mptscsih.c	2008-03-10

13:20:30.000000000 -0700
+++ git/drivers/message/fusion/mptscsih.c 2008-03-13
12:38:38.070017332 -0700

drivers/message/fusion/mptscsih.c +0 -6
@@ -2442,12 +2442,6 @@ mptscsih_slave_configure(struct scsi_dev
 		    ioc->name, sdev->sdtr, sdev->wdtr,
 		    sdev->ppr, sdev->inquiry_len));
 
-	if (sdev->id > sh->max_id) {
-		/* error case, should never happen */
-		scsi_adjust_queue_depth(sdev, 0, 1);
-		goto slave_configure_exit;
-	}
-
 	vdevice->configured_lun = 1;
 	mptscsih_change_queue_depth(sdev, MPT_SCSI_CMD_PER_DEV_HIGH);

drivers/message/fusion/mptscsih.c mptscsih_slave_configure(struct scsi_dev · line 2460

Quoted 8 lines
drivers/message/fusion/mptscsih.c +0 -2
@@ -2461,8 +2455,6 @@ mptscsih_slave_configure(struct scsi_dev
 		    ioc->name, vtarget->negoFlags, vtarget->maxOffset,
 		    vtarget->minSyncFactor));
 
-slave_configure_exit:
-
 	dsprintk(ioc, printk(MYIOC_s_DEBUG_FMT
 		"tagged %d, simple %d, ordered %d\n",
 		ioc->name,sdev->tagged_supported, sdev->simple_tags,

ACK