Re: [BUG] ub.c badness in current bk

[Pete Zaitcev]

On Sat, 18 Sep 2004 12:08:46 +1000
Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> wrote:

> And the real one without fuckup in media_change(), sorry, I sent
> the wrong patch.

> +	 * Here's our equivalent of spinup. We probably need to better check
> +	 * the sense codes from TUR ?

This really bothers me. The key 6 is asking for a clearing and possibly
a spin-up (my ZIP 100 returns 6 until it's spun up), that's right. But
it won't start without START_STOP command, on pure TURs. So it's a delusion
to accept this as a spin-up. I do plan for a decent spin-up code, but
it's a little difficult within the ub's phylosophy (possibly it was a bad
design to rely on a state machine too much).

How about this deal. I'll add a clearly marked workaround like the
appended patch, and then ask you to try again when a proper spin-up gets
implemented?

Note that the patch is against -mm where the REQUEST SENSE is set correctly.
I strongly suggest you to use ub from -mm. Linus' tree is behind.

Also, I might have screwed something along the way, so if it fails, please
send me dmesg and the contents of /sys/..../diag.

Greetings,
-- Pete

diff -urp -X dontdiff linux-2.6.9-rc2-mm1/drivers/block/ub.c linux-2.6.9-rc2-mm1-ub/drivers/block/ub.c
--- linux-2.6.9-rc2-mm1/drivers/block/ub.c	2004-09-17 23:04:27.000000000 -0700
+++ linux-2.6.9-rc2-mm1-ub/drivers/block/ub.c	2004-09-19 20:11:07.655890215 -0700
@@ -25,6 +25,7 @@
  *  -- prune comments, they are too volumnous
  *  -- Exterminate P3 printks
  *  -- Resove XXX's
+ *  -- Redo "benh's retries", perhaps have spin-up code to handle them. V:D=?
  */
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -157,7 +158,8 @@ struct ub_scsi_cmd {
 	struct ub_scsi_cmd *next;
 
 	int error;			/* Return code - valid upon done */
-	int act_len;			/* Return size */
+	unsigned int act_len;		/* Return size */
+	unsigned char key, asc, ascq;	/* May be valid if error==-EIO */
 
 	int stat_count;			/* Retries getting status. */
 
@@ -1141,16 +1146,8 @@ static void ub_scsi_urb_compl(struct ub_
 		(*cmd->done)(sc, cmd);
 
 	} else if (cmd->state == UB_CMDST_SENSE) {
-		/* 
-		 * We do not look at sense, because even if there was no sense,
-		 * we get into UB_CMDST_SENSE from a STALL or CSW FAIL only.
-		 * We request sense because we want to clear CHECK CONDITION
-		 * on devices with delusions of SCSI, and not because we
-		 * are curious in any way about the sense itself.
-		 */
-		/* if ((cmd->top_sense[2] & 0x0F) == NO_SENSE) { foo } */
-
 		ub_state_done(sc, cmd, -EIO);
+
 	} else {
 		printk(KERN_WARNING "%s: "
 		    "wrong command state %d on device %u\n",
@@ -1309,6 +1306,10 @@ static void ub_top_sense_done(struct ub_
 	 */
 	ub_cmdtr_sense(sc, scmd, sense);
 
+	/*
+	 * Find the command which triggered the unit attention or a check,
+	 * save the sense into it, and advance its state machine.
+	 */
 	if ((cmd = ub_cmdq_peek(sc)) == NULL) {
 		printk(KERN_WARNING "%s: sense done while idle\n", sc->name);
 		return;
@@ -1326,6 +1327,10 @@ static void ub_top_sense_done(struct ub_
 		return;
 	}
 
+	cmd->key = sense[2] & 0x0F;
+	cmd->asc = sense[12];
+	cmd->ascq = sense[13];
+
 	ub_scsi_urb_compl(sc, cmd);
 }
 
@@ -1621,6 +1638,9 @@ static int ub_sync_tur(struct ub_dev *sc
 
 	rc = cmd->error;
 
+	if (rc == -EIO && cmd->key != 0)	/* Retries for benh's key */
+		rc = cmd->key;
+
 err_submit:
 	kfree(cmd);
 err_alloc:
@@ -1836,6 +1856,7 @@ static int ub_probe(struct usb_interface
 	request_queue_t *q;
 	struct gendisk *disk;
 	int rc;
+	int i;
 
 	rc = -ENOMEM;
 	if ((sc = kmalloc(sizeof(struct ub_dev), GFP_KERNEL)) == NULL)
@@ -1902,7 +1923,11 @@ static int ub_probe(struct usb_interface
 	 * has to succeed, so we clear checks with an additional one here.
 	 * In any case it's not our business how revaliadation is implemented.
 	 */
-	ub_sync_tur(sc);
+	for (i = 0; i < 3; i++) {	/* Retries for benh's key */
+		if ((rc = ub_sync_tur(sc)) <= 0) break;
+		if (rc != 0x6) break;
+		msleep(10);
+	}
 
 	sc->removable = 1;		/* XXX Query this from the device */
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/