Skip to contentSkip to conversation

RFC/patch: bflags not applied in case of SCSI_SCAN_TARGET_PRESENT

PATCH In review Guido Guenther 2 participants 11 messages Dec 10, 2004 – Jan 14, 2005 SCSI

lore
Share & tools
lore
#0 lore raw

Hi,
we're using a qla2342 together with an EMC Symmetric. SCSI scanning the EMC reports a peripheral identifier of 3 for lun 0, which leads to a SCSI_SCAN_PRESENT return code of scsi_probe_and_add_lun, but the devices blacklist flags are not being honored. This is a problem since we need SCSI_LARGELUN for successfull scanning on LUNs past 8 in scsi_scan_target. The attached patch tries to fix this:

a) makes sure we honor the bflags in scsi_probe_and_add_lun, even when there's no device supported on this lun (lun 0 is special so it might make sense to special case this for lun 0 only)
b) passes them on to the sequential scanning part of scsi_scan_target.

drivers/scsi/scsi_scan.c +4 -2
@@ -717,6 +730,8 @@
 					"scsi scan: peripheral qualifier of 3,"
 					" no device added\n"));
 		res = SCSI_SCAN_TARGET_PRESENT;
+		if (bflagsp)
+			*bflagsp = bflags;
 		goto out_free_result;
 	}
 
@@ -1157,10 +1179,10 @@
 		/*
 		 * There's a target here, but lun 0 is offline so we
 		 * can't use the report_lun scan.  Fall back to a
-		 * sequential lun scan with a bflags of SPARSELUN and
+		 * sequential lun scan with a bflags of at least SPARSELUN and
 		 * a default scsi level of SCSI_2
 		 */
-		scsi_sequential_lun_scan(shost, channel, id, BLIST_SPARSELUN,
+		scsi_sequential_lun_scan(shost, channel, id, BLIST_SPARSELUN | bflags,
 				SCSI_SCAN_TARGET_PRESENT, SCSI_2, rescan);
 	}
 }

Does this look o.k.? Patch is against 2.6.10-rc3. I know little about scsi scanning, so this might be completely bogus.
Please cc: me on replies since I'm not subscribe to this list. Cheers,
-- Guido

#1 lore raw

PATCH 1/1 REPORT LUN scan even if no storage is available on LUN 0

Patch looks okay, but we should always be trying the report lun scan, and then failing back to the sequential scan even if there is no storage on LUN 0.

Can you test this? I don't have hardware readily available that supports sparse luns and report lun. Patch is against current bk. I built and ran with regular disks (lun 0 only).

This is a bit hacky, improvements welcomed. Ideally we should create and then use a scsi_target to scan, rather than having the scsi_device allocation trigger creation of the scsi_target, and pass a scsi_target to other scan functions.

Support REPORT LUN scan even if no storage is available on LUN 0.

drivers/scsi/scsi_scan.c +24 -14
@@ -647,6 +647,9 @@
  *     Call scsi_probe_lun, if a LUN with an attached device is found,
  *     allocate and set it up by calling scsi_add_lun.
  *
+ * Note: LUN 0 is special cased, the sdev is not released if we are
+ *     returning SCSI_SCAN_TARGET_PRESENT.
+ *
  * Return:
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a Scsi_Device
  *     SCSI_SCAN_TARGET_PRESENT: target responded, but no device is
@@ -735,7 +738,12 @@
  out_free_sreq:
 	scsi_release_request(sreq);
  out_free_sdev:
-	if (res == SCSI_SCAN_LUN_PRESENT) {
+	if ((res == SCSI_SCAN_LUN_PRESENT) ||
+	    ((lun == 0) && res == SCSI_SCAN_TARGET_PRESENT)) {
+		/*
+		 * The LUN 0 check is for support of report lun scanning
+		 * when no storage is configured on LUN 0.
+		 */
 		if (sdevp)
 			*sdevp = sdev;
 	} else {
@@ -1076,6 +1084,8 @@
 	down(&shost->scan_mutex);
 	res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL,
 				     &sdev, 1, hostdata);
+	if ((lun == 0) && (res == SCSI_SCAN_TARGET_PRESENT))
+		put_device(&sdev->sdev_gendev);
 	if (res != SCSI_SCAN_LUN_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
 	up(&shost->scan_mutex);
@@ -1134,18 +1144,19 @@
 		/*
 		 * Scan for a specific host/chan/id/lun.
 		 */
-		scsi_probe_and_add_lun(shost, channel, id, lun, NULL, NULL,
-				       rescan, NULL);
-		return;
+		res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL,
+					     &sdev, rescan, NULL);
+		goto completed;
 	}
 
 	/*
 	 * Scan LUN 0, if there is some response, scan further. Ideally, we
 	 * would not configure LUN 0 until all LUNs are scanned.
 	 */
-	res = scsi_probe_and_add_lun(shost, channel, id, 0, &bflags, &sdev,
+	lun = 0;
+	res = scsi_probe_and_add_lun(shost, channel, id, lun, &bflags, &sdev,
 				     rescan, NULL);
-	if (res == SCSI_SCAN_LUN_PRESENT) {
+	if (res != SCSI_SCAN_NO_RESPONSE) {
 		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
 			/*
 			 * The REPORT LUN did not scan the target,
@@ -1153,16 +1164,15 @@
 			 */
 			scsi_sequential_lun_scan(shost, channel, id, bflags,
 				       	res, sdev->scsi_level, rescan);
-	} else if (res == SCSI_SCAN_TARGET_PRESENT) {
+	}
+
+ completed:
+	if ((lun == 0) && (res == SCSI_SCAN_TARGET_PRESENT))
 		/*
-		 * There's a target here, but lun 0 is offline so we
-		 * can't use the report_lun scan.  Fall back to a
-		 * sequential lun scan with a bflags of SPARSELUN and
-		 * a default scsi level of SCSI_2
+		 * LUN 0 is handled specially so the sdev can be
+		 * used with the REPORT LUN scan, release it now.
 		 */
-		scsi_sequential_lun_scan(shost, channel, id, BLIST_SPARSELUN,
-				SCSI_SCAN_TARGET_PRESENT, SCSI_2, rescan);
-	}
+		put_device(&sdev->sdev_gendev);
 }
 
 static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
#2 lore raw

Re: [PATCH] REPORT LUN scan even if no storage is available on LUN 0

Sorry it took me so long to test this, first time I'm near this hardware again.

Quoted 7 lines · Patrick Mansfield

Patch looks okay, but we should always be trying the report lun scan, and then failing back to the sequential scan even if there is no storage on LUN 0.

Can you test this? I don't have hardware readily available that supports sparse luns and report lun. Patch is against current bk. I built and ran with regular disks (lun 0 only).

Doesn't work here. I doen't see any devices in /proc/scsi/scsi now.
-- Guido

Quoted 95 lines

This is a bit hacky, improvements welcomed. Ideally we should create and then use a scsi_target to scan, rather than having the scsi_device allocation trigger creation of the scsi_target, and pass a scsi_target to other scan functions.

Support REPORT LUN scan even if no storage is available on LUN 0.

drivers/scsi/scsi_scan.c +24 -14
@@ -647,6 +647,9 @@
  *     Call scsi_probe_lun, if a LUN with an attached device is found,
  *     allocate and set it up by calling scsi_add_lun.
  *
+ * Note: LUN 0 is special cased, the sdev is not released if we are
+ *     returning SCSI_SCAN_TARGET_PRESENT.
+ *
  * Return:
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a Scsi_Device
  *     SCSI_SCAN_TARGET_PRESENT: target responded, but no device is
@@ -735,7 +738,12 @@
  out_free_sreq:
 	scsi_release_request(sreq);
  out_free_sdev:
-	if (res == SCSI_SCAN_LUN_PRESENT) {
+	if ((res == SCSI_SCAN_LUN_PRESENT) ||
+	    ((lun == 0) && res == SCSI_SCAN_TARGET_PRESENT)) {
+		/*
+		 * The LUN 0 check is for support of report lun scanning
+		 * when no storage is configured on LUN 0.
+		 */
 		if (sdevp)
 			*sdevp = sdev;
 	} else {
@@ -1076,6 +1084,8 @@
 	down(&shost->scan_mutex);
 	res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL,
 				     &sdev, 1, hostdata);
+	if ((lun == 0) && (res == SCSI_SCAN_TARGET_PRESENT))
+		put_device(&sdev->sdev_gendev);
 	if (res != SCSI_SCAN_LUN_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
 	up(&shost->scan_mutex);
@@ -1134,18 +1144,19 @@
 		/*
 		 * Scan for a specific host/chan/id/lun.
 		 */
-		scsi_probe_and_add_lun(shost, channel, id, lun, NULL, NULL,
-				       rescan, NULL);
-		return;
+		res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL,
+					     &sdev, rescan, NULL);
+		goto completed;
 	}
 
 	/*
 	 * Scan LUN 0, if there is some response, scan further. Ideally, we
 	 * would not configure LUN 0 until all LUNs are scanned.
 	 */
-	res = scsi_probe_and_add_lun(shost, channel, id, 0, &bflags, &sdev,
+	lun = 0;
+	res = scsi_probe_and_add_lun(shost, channel, id, lun, &bflags, &sdev,
 				     rescan, NULL);
-	if (res == SCSI_SCAN_LUN_PRESENT) {
+	if (res != SCSI_SCAN_NO_RESPONSE) {
 		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
 			/*
 			 * The REPORT LUN did not scan the target,
@@ -1153,16 +1164,15 @@
 			 */
 			scsi_sequential_lun_scan(shost, channel, id, bflags,
 				       	res, sdev->scsi_level, rescan);
-	} else if (res == SCSI_SCAN_TARGET_PRESENT) {
+	}
+
+ completed:
+	if ((lun == 0) && (res == SCSI_SCAN_TARGET_PRESENT))
 		/*
-		 * There's a target here, but lun 0 is offline so we
-		 * can't use the report_lun scan.  Fall back to a
-		 * sequential lun scan with a bflags of SPARSELUN and
-		 * a default scsi level of SCSI_2
+		 * LUN 0 is handled specially so the sdev can be
+		 * used with the REPORT LUN scan, release it now.
 		 */
-		scsi_sequential_lun_scan(shost, channel, id, BLIST_SPARSELUN,
-				SCSI_SCAN_TARGET_PRESENT, SCSI_2, rescan);
-	}
+		put_device(&sdev->sdev_gendev);
 }
 
 static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
#3 lore raw

Re: [PATCH] REPORT LUN scan even if no storage is available on LUN 0

Quoted 7 lines · Guido Guenther

Sorry it took me so long to test this, first time I'm near this hardware again.

Quoted 3 lines · Patrick Mansfield

Patch looks okay, but we should always be trying the report lun scan, and then failing back to the sequential scan even if there is no storage on LUN 0.

Quoted 5 lines
Quoted 3 lines

Can you test this? I don't have hardware readily available that supports sparse luns and report lun. Patch is against current bk. I built and ran with regular disks (lun 0 only).

Doesn't work here. I doen't see any devices in /proc/scsi/scsi now.
-- Guido

So it worked some without the patch (I think you only saw some of the LUNs before)?

What is output during the scan?

Can you turn on some scsi logging, and send that?

If you have root on scsi, you should shutdown syslogd (though this should not generate lots of logging for small amounts of IO it generates some, and if you screw up you'll be able to disable the logging), and then capture the console/serial output.

If root on ide, just leave syslog on and get /var/log/messages output.

Turn on per-command and scan logging (0x2400 | 0x01c0):

	sysctl -w dev.scsi.logging_level=0x25c0

Set back off afterwards with:

sysctl -w dev.scsi.logging_level=0

-- Patrick Mansfield

#4 lore raw

Re: [PATCH] REPORT LUN scan even if no storage is available on LUN 0

Quoted 8 lines · Patrick Mansfield
Quoted 7 lines · Guido Guenther

Sorry it took me so long to test this, first time I'm near this hardware again.

Quoted 3 lines · Patrick Mansfield

Patch looks okay, but we should always be trying the report lun scan, and then failing back to the sequential scan even if there is no storage on LUN 0.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Can you test this? I don't have hardware readily available that supports sparse luns and report lun. Patch is against current bk. I built and ran with regular disks (lun 0 only).

Doesn't work here. I doen't see any devices in /proc/scsi/scsi now.
-- Guido

So it worked some without the patch (I think you only saw some of the LUNs before)?

No. Without any patches we had all the available devices in /proc/scsi/scsi but none was marked as "claimed by the OS" (all were marked with a star, but could then be "added" via "scsi add-single-device ... > /proc/scsi/scsi").
This was the actual problem I tried to solve with the patch. Now I don't see any devices at all in /proc/scsi/scsi.

Quoted 1 line

What is output during the scan?

qla2300 0000:06:01.0: Found an ISP2312, irq 11, iobase 0xf8806000 qla2300 0000:06:01.0: Configuring PCI space...
qla2300 0000:06:01.0: Configure NVRAM parameters...
qla2300 0000:06:01.0: Verifying loaded RISC code...
qla2300 0000:06:01.0: Waiting for LIP to complete...
qla2300 0000:06:01.0: LOOP UP detected (1 Gbps).
qla2300 0000:06:01.0: Topology - (F_Port), Host Loop address 0xffff scsi0 : qla2xxx
QLogic Fibre Channel HBA Driver: 8.00.00b15-k
QLogic QLA2342 -
qla2300 0000:06:01.0: Found an ISP2312, irq 11, iobase 0xf8806000
ISP2312: PCI-X (100 MHz) @ 0000:06:01.0 hdma-, host#=0, fw=3.02.30 IPX

qla2300 0000:06:01.1: Found an ISP2312, irq 10, iobase 0xf8854000 qla2300 0000:06:01.1: Configuring PCI space...
qla2300 0000:06:01.1: Configure NVRAM parameters...
qla2300 0000:06:01.1: Verifying loaded RISC code...
qla2300 0000:06:01.1: Waiting for LIP to complete...
qla2300 0000:06:01.1: LOOP UP detected (1 Gbps).
qla2300 0000:06:01.1: Topology - (F_Port), Host Loop address 0xffff scsi1 : qla2xxx
QLogic Fibre Channel HBA Driver: 8.00.00b15-k
QLogic QLA2342 -
ISP2312: PCI-X (100 MHz) @ 0000:06:01.1 hdma-, host#=1, fw=3.02.30 IPX

Quoted 1 line

Can you turn on some scsi logging, and send that?

Sure. I tried scsi_logging_level=... on the kernel command line but it didn't show anything, did I miss something obvious? /proc/sys/dev/scsi/logging_level was still zero afterwards. Cheers,
-- Guido

Quoted 17 lines

If you have root on scsi, you should shutdown syslogd (though this should not generate lots of logging for small amounts of IO it generates some, and if you screw up you'll be able to disable the logging), and then capture the console/serial output.

If root on ide, just leave syslog on and get /var/log/messages output.

Turn on per-command and scan logging (0x2400 | 0x01c0):

	sysctl -w dev.scsi.logging_level=0x25c0

Set back off afterwards with:

sysctl -w dev.scsi.logging_level=0

-- Patrick Mansfield

#5 lore raw

Re: [PATCH] REPORT LUN scan even if no storage is available on LUN 0

Quoted 4 lines · Guido Guenther
Quoted 1 line

Can you turn on some scsi logging, and send that?

Sure. I tried scsi_logging_level=... on the kernel command line but it didn't show anything, did I miss something obvious? /proc/sys/dev/scsi/logging_level was still zero afterwards.

It is a sysctl variable, so you can set it after you are booted, per my other email (and still below), and man sysctl.

If you are building the qlogic driver into the kernel you can still rescan the LUNs via:

	echo "- - -" > /sys/class/scsi_host/hostN/scan
Quoted 16 lines
Quoted 16 lines

If you have root on scsi, you should shutdown syslogd (though this should not generate lots of logging for small amounts of IO it generates some, and if you screw up you'll be able to disable the logging), and then capture the console/serial output.

If root on ide, just leave syslog on and get /var/log/messages output.

Turn on per-command and scan logging (0x2400 | 0x01c0):

	sysctl -w dev.scsi.logging_level=0x25c0

Set back off afterwards with:

sysctl -w dev.scsi.logging_level=0

-- Patrick Mansfield

#6 lore raw

Re: [PATCH] REPORT LUN scan even if no storage is available on LUN 0

Quoted 9 lines · Patrick Mansfield
Quoted 4 lines · Guido Guenther
Quoted 1 line

Can you turn on some scsi logging, and send that?

Sure. I tried scsi_logging_level=... on the kernel command line but it didn't show anything, did I miss something obvious? /proc/sys/dev/scsi/logging_level was still zero afterwards.

It is a sysctl variable, so you can set it after you are booted, per my other email (and still below), and man sysctl.

Yes I saw that, I was just wondering why scsi_logging_level doesn't work also it's a module option in scsi.c.

Quoted 4 lines

If you are building the qlogic driver into the kernel you can still rescan the LUNs via:

	echo "- - -" > /sys/class/scsi_host/hostN/scan

O.k. Thanks.
-- Guido

Quoted 16 lines
Quoted 16 lines
Quoted 16 lines

If you have root on scsi, you should shutdown syslogd (though this should not generate lots of logging for small amounts of IO it generates some, and if you screw up you'll be able to disable the logging), and then capture the console/serial output.

If root on ide, just leave syslog on and get /var/log/messages output.

Turn on per-command and scan logging (0x2400 | 0x01c0):

	sysctl -w dev.scsi.logging_level=0x25c0

Set back off afterwards with:

sysctl -w dev.scsi.logging_level=0

-- Patrick Mansfield

#7 lore raw

Re: [PATCH] REPORT LUN scan even if no storage is available on LUN 0

Quoted 4 lines · Guido Guenther
Quoted 3 lines · Patrick Mansfield
Quoted 1 line · Guido Guenther
Quoted 1 line

Can you turn on some scsi logging, and send that?

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

Sure. I tried scsi_logging_level=... on the kernel command line but it didn't show anything, did I miss something obvious? /proc/sys/dev/scsi/logging_level was still zero afterwards.

It is a sysctl variable, so you can set it after you are booted, per my other email (and still below), and man sysctl.

Yes I saw that, I was just wondering why scsi_logging_level doesn't work also it's a module option in scsi.c.

In-kernel module params have to be prefixed by the module name, like:

	scsi_mod.scsi_logging_level=0x2400

AFAIR if the param name is unknown, the kernel outputs a warning.

Note that most (all?) of the other scsi params have the "scsi_" removed, though the variable name does not, like max_scsi_luns is set via max_luns.

-- Patrick Mansfield

#8 lore raw

Re: [PATCH] REPORT LUN scan even if no storage is available on LUN 0

In reply to Guido Guenther
Quoted 4 lines · Guido Guenther

No. Without any patches we had all the available devices in /proc/scsi/scsi but none was marked as "claimed by the OS" (all were marked with a star, but could then be "added" via "scsi add-single-device ... > /proc/scsi/scsi").

Oh ... the "*" is used in the /proc/scsi/qla2xxx/N files, not /proc/scsi/scsi.

Is /proc/scsi/qla2xxx/N not showing any LUNs?

If so, that is a different problem.

-- Patrick Mansfield

#9 lore raw

Re: [PATCH] REPORT LUN scan even if no storage is available on LUN 0

Quoted 9 lines · Patrick Mansfield
Quoted 4 lines · Guido Guenther

No. Without any patches we had all the available devices in /proc/scsi/scsi but none was marked as "claimed by the OS" (all were marked with a star, but could then be "added" via "scsi add-single-device ... > /proc/scsi/scsi").

Oh ... the "*" is used in the /proc/scsi/qla2xxx/N files, not /proc/scsi/scsi.

You're absolutely right. The above should read:

Without any patches we had all the available devices in /proc/scsi/qla2xxx/N but none was marked as "claimed by the OS" (all were marked with a star, but could then be "added" via "scsi add-single-device ... > /proc/scsi/scsi").

Sorry for the confusion.

This is the target that has all the devices:

# echo "0 0 -" > /sys/class/scsi_host/host0/scan
scsi_scan_host_selected: <0:0:0:4294967295>
scsi scan: INQUIRY to host 0 channel 0 id 0 lun 0
scsi <0:0:0:0> send                  scsi0 : destination target 0, lun 0
        command = Inquiry 00 00 00 24 00
scsi <0:0:0:0> done SUCCESS        0 scsi0 : destination target 0, lun 0
        command = Inquiry 00 00 00 24 00
scsi scan: 1st INQUIRY successful with code 0x0
scsi <0:0:0:0> send                  scsi0 : destination target 0, lun 0
        command = Inquiry 00 00 00 95 00
scsi <0:0:0:0> done SUCCESS        0 scsi0 : destination target 0, lun 0
        command = Inquiry 00 00 00 95 00
scsi scan: 2nd INQUIRY successful with code 0x0
scsi scan: peripheral qualifier of 3, no device added
scsi scan: Sequential scan of host 0 channel 0 id 0

While this is an empty one:

# echo "0 1 -" > /sys/class/scsi_host/host0/scan
scsi_scan_host_selected: <0:0:1:4294967295>
scsi scan: INQUIRY to host 0 channel 0 id 1 lun 0
scsi <0:0:1:0> send                  scsi0 : destination target 1, lun 0
        command = Inquiry 00 00 00 24 00
scsi <0:0:1:0> done SUCCESS    10000 scsi0 : destination target 1, lun 0
        command = Inquiry 00 00 00 24 00
scsi scan: 1st INQUIRY failed with code 0x10000

Cheers,
-- Guido

#10 lore raw

Re: [PATCH] REPORT LUN scan even if no storage is available on LUN 0

Quoted 20 lines · Guido Guenther

On Thu, Jan 13, 2005 at 04:21:34PM -0800, Patrick Mansfield wrote:

This is the target that has all the devices:

# echo "0 0 -" > /sys/class/scsi_host/host0/scan
scsi_scan_host_selected: <0:0:0:4294967295>
scsi scan: INQUIRY to host 0 channel 0 id 0 lun 0
scsi <0:0:0:0> send                  scsi0 : destination target 0, lun 0
        command = Inquiry 00 00 00 24 00
scsi <0:0:0:0> done SUCCESS        0 scsi0 : destination target 0, lun 0
        command = Inquiry 00 00 00 24 00
scsi scan: 1st INQUIRY successful with code 0x0
scsi <0:0:0:0> send                  scsi0 : destination target 0, lun 0
        command = Inquiry 00 00 00 95 00
scsi <0:0:0:0> done SUCCESS        0 scsi0 : destination target 0, lun 0
        command = Inquiry 00 00 00 95 00
scsi scan: 2nd INQUIRY successful with code 0x0
scsi scan: peripheral qualifier of 3, no device added
scsi scan: Sequential scan of host 0 channel 0 id 0

While this is an empty one:

Quoted 8 lines
# echo "0 1 -" > /sys/class/scsi_host/host0/scan
scsi_scan_host_selected: <0:0:1:4294967295>
scsi scan: INQUIRY to host 0 channel 0 id 1 lun 0
scsi <0:0:1:0> send                  scsi0 : destination target 1, lun 0
        command = Inquiry 00 00 00 24 00
scsi <0:0:1:0> done SUCCESS    10000 scsi0 : destination target 1, lun 0
        command = Inquiry 00 00 00 24 00
scsi scan: 1st INQUIRY failed with code 0x10000

Your device must be reporting as SCSI-2, that makes sense, since you needed other bflags set. The logging should really dump the scsi_level. You need to add an entry to the devinfo list to force a REPORT LUNs scan.

I don't know why it is not already in scsi_devinfo.c, AFAIR REPORT LUNs has always worked with EMC, and should be in its devinfo flag (and probably the only devinfo flag, since sparse, largelun and forcelun are meaningless for a REPORT LUN scan).

module params can be used, but /proc/scsi/device_info is easiest. Make sure I have the correct vendor:model, this should work:

	echo "EMC:SYMMETRIX:0x20000" > /proc/scsi/device_info

BLIST_REPORTLUN2 is 0x20000.

-- Patrick Mansfield