[PATCH 0/3] some more firewire-sbp2 work

[PATCH 0/3] some more firewire-sbp2 work

[Stefan Richter]

Here come one triviality and two fixes, or at least attempts to do so.
I tried but couldn't recreate the conditions to put patch 2/3 into
effect.  But I did so with patch 3/3; that's easier due to a larger
window where this problem can happen.

1/3 firewire: fw-sbp2: unsigned int vs. unsigned
2/3 firewire: fw-sbp2: fix logout before login retry
3/3 firewire: fw-sbp2: retry login if scsi_device was offlined early

 drivers/firewire/fw-sbp2.c |   52 +++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 13 deletions(-)
-- 
Stefan Richter
-=====-==--- ---= ==-=-
http://arcgraph.de/sr/

[PATCH 1/3] firewire: fw-sbp2: unsigned int vs. unsigned

[Stefan Richter]

Standardize on "unsigned int" style.
Sort some struct members thematically.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Depends on Jarod's latest fw-sbp2 patch which I didn't even push to
linux1394-2.6.git yet.

 drivers/firewire/fw-sbp2.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -141,15 +141,13 @@ struct sbp2_logical_unit {
 struct sbp2_target {
 	struct kref kref;
 	struct fw_unit *unit;
+	struct list_head lu_list;
 
 	u64 management_agent_address;
 	int directory_id;
 	int node_id;
 	int address_high;
-
-	unsigned workarounds;
-	struct list_head lu_list;
-
+	unsigned int workarounds;
 	unsigned int mgt_orb_timeout;
 };
 
@@ -160,7 +158,7 @@ struct sbp2_target {
  */
 #define SBP2_MIN_LOGIN_ORB_TIMEOUT	5000U	/* Timeout in ms */
 #define SBP2_MAX_LOGIN_ORB_TIMEOUT	40000U	/* Timeout in ms */
-#define SBP2_ORB_TIMEOUT		2000	/* Timeout in ms */
+#define SBP2_ORB_TIMEOUT		2000U	/* Timeout in ms */
 #define SBP2_ORB_NULL			0x80000000
 #define SBP2_MAX_SG_ELEMENT_LENGTH	0xf000
 
@@ -297,7 +295,7 @@ struct sbp2_command_orb {
 static const struct {
 	u32 firmware_revision;
 	u32 model;
-	unsigned workarounds;
+	unsigned int workarounds;
 } sbp2_workarounds_table[] = {
 	/* DViCO Momobay CX-1 with TSB42AA9 bridge */ {
 		.firmware_revision	= 0x002800,
@@ -836,7 +834,7 @@ static void sbp2_init_workarounds(struct
 				  u32 firmware_revision)
 {
 	int i;
-	unsigned w = sbp2_param_workarounds;
+	unsigned int w = sbp2_param_workarounds;
 
 	if (w)
 		fw_notify("Please notify linux1394-devel@lists.sourceforge.net "
@@ -1197,7 +1195,7 @@ static int sbp2_scsi_queuecommand(struct
 	struct sbp2_logical_unit *lu = cmd->device->hostdata;
 	struct fw_device *device = fw_device(lu->tgt->unit->device.parent);
 	struct sbp2_command_orb *orb;
-	unsigned max_payload;
+	unsigned int max_payload;
 	int retval = SCSI_MLQUEUE_HOST_BUSY;
 
 	/*

-- 
Stefan Richter
-=====-==--- ---= ==-=-
http://arcgraph.de/sr/

[PATCH 2/3] firewire: fw-sbp2: fix logout before login retry

[Stefan Richter]

This fixes a "can't recognize device" kind of bug.

If the SCSI INQUIRY failed and hence __scsi_add_device failed due to a
bus reset, we tried a logout and then waited for the already scheduled
login work to happen.  So far so good, but the generation used for the
logout was outdated, hence the logout never reached the target.  The
target might therefore deny the subsequent relogin attempt, which would
also leave the target inaccessible.

Therefore fetch a fresh device->generation for the logout.  Use memory
barriers to prevent our plan being foiled by compiler or hardware
optimizations.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-sbp2.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -716,7 +716,11 @@ static void sbp2_login(struct work_struc
 	sdev = __scsi_add_device(shost, 0, 0,
 				 scsilun_to_int(&eight_bytes_lun), lu);
 	if (IS_ERR(sdev)) {
-		sbp2_send_management_orb(lu, node_id, generation,
+		smp_rmb(); /* generation may have changed */
+		generation = device->generation;
+		smp_rmb(); /* node_id must not be older than generation */
+
+		sbp2_send_management_orb(lu, device->node_id, generation,
 				SBP2_LOGOUT_REQUEST, lu->login_id, NULL);
 		/*
 		 * Set this back to sbp2_login so we fall back and

-- 
Stefan Richter
-=====-==--- ---= ==-=-
http://arcgraph.de/sr/

[PATCH 3/3] firewire: fw-sbp2: retry login if scsi_device was offlined early

[Stefan Richter]

Fixes yet another "can't recognize device" bug.

https://bugzilla.redhat.com/show_bug.cgi?id=428554#c16 :
If a bus reset happens after the login and SCSI INQUIRY succeeded ---
but before scsi_driver.init_command finished ---, SCSI core would take
the brand new scsi_device offline already, leaving the SBP-2 target
inaccessible.

The proper fix would be to allow sbp2_reconnect to happen in parallel to
__scsi_add_device.  This involves intrusive changes to fw-sbp2.  Until
then, we use the following simple workaround:  Check if the new sdev is
offline; if so, remove the device, logout, and let another login attempt
happen.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---

Depends on patch 2/3.  Has yet to be tested by the Fedora bug reporter.

 drivers/firewire/fw-sbp2.c |   40 +++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -716,21 +716,45 @@ static void sbp2_login(struct work_struc
 	sdev = __scsi_add_device(shost, 0, 0,
 				 scsilun_to_int(&eight_bytes_lun), lu);
 	if (IS_ERR(sdev)) {
-		smp_rmb(); /* generation may have changed */
-		generation = device->generation;
-		smp_rmb(); /* node_id must not be older than generation */
+		/*
+		 * The most frequent cause for __scsi_add_device() to fail
+		 * is a bus reset while sending the SCSI INQUIRY.  Try again.
+		 */
+		goto out_logout_login;
 
-		sbp2_send_management_orb(lu, device->node_id, generation,
-				SBP2_LOGOUT_REQUEST, lu->login_id, NULL);
+	} else if (sdev->sdev_state == SDEV_OFFLINE) {
 		/*
-		 * Set this back to sbp2_login so we fall back and
-		 * retry login on bus reset.
+		 * FIXME:  We are unable to perform reconnects while in
+		 * sbp2_login().  Therefore __scsi_add_device() will get
+		 * into trouble if a bus reset happens in parallel.
+		 * It will either fail (that's OK, see above) or take sdev
+		 * offline.  Here is a crude workaround for the latter.
 		 */
-		PREPARE_DELAYED_WORK(&lu->work, sbp2_login);
+		scsi_device_put(sdev);
+		scsi_remove_device(sdev);
+		goto out_logout_login;
+
 	} else {
+		/*
+		 * Can you believe it?  Everything went well.
+		 */
 		lu->sdev = sdev;
 		scsi_device_put(sdev);
+		goto out;
 	}
+
+ out_logout_login:
+	smp_rmb(); /* generation may have changed */
+	generation = device->generation;
+	smp_rmb(); /* node_id must not be older than generation */
+
+	sbp2_send_management_orb(lu, device->node_id, generation,
+				 SBP2_LOGOUT_REQUEST, lu->login_id, NULL);
+	/*
+	 * If a bus reset happened, sbp2_update will have requeued
+	 * lu->work already.  Reset the work from reconnect to login.
+	 */
+	PREPARE_DELAYED_WORK(&lu->work, sbp2_login);
  out:
 	sbp2_target_put(lu->tgt);
 }

-- 
Stefan Richter
-=====-==--- ---= ==-=-
http://arcgraph.de/sr/