[PATCH] sd: no errors allowed during async probing

[PATCH] sd: no errors allowed during async probing

[Alan Stern]

This patch (as1252) fixes a bug in the sd probing code.  When the
probe routine was split up into a synchronous and an asynchronous
part, too much was put into the asynchronous part.  It's important
that all the possible failure modes occur synchronously, so that the
driver core knows whether the probe was successful even before the
async part is complete.

Another bug is that device removal has to wait for the async probing
to finish!  The patch addresses both bugs, by moving some code back
from sd_probe_async() to sd_probe() and by adding a call to
async_synchronize_full() at the start of sd_remove().

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

James:

I haven't seen any progress on this patch yet, so I'm officially 
submitting it as a bug fix.  Admittedly, it's kind of presumptuous to 
put my own Signed-off-by on it, but there doesn't seem to be any other 
choice.

Alan Stern



Index: usb-2.6/drivers/scsi/sd.c
===================================================================
--- usb-2.6.orig/drivers/scsi/sd.c
+++ usb-2.6/drivers/scsi/sd.c
@@ -1902,24 +1902,6 @@ static void sd_probe_async(void *data, a
 	index = sdkp->index;
 	dev = &sdp->sdev_gendev;
 
-	if (!sdp->request_queue->rq_timeout) {
-		if (sdp->type != TYPE_MOD)
-			blk_queue_rq_timeout(sdp->request_queue, SD_TIMEOUT);
-		else
-			blk_queue_rq_timeout(sdp->request_queue,
-					     SD_MOD_TIMEOUT);
-	}
-
-	device_initialize(&sdkp->dev);
-	sdkp->dev.parent = &sdp->sdev_gendev;
-	sdkp->dev.class = &sd_disk_class;
-	dev_set_name(&sdkp->dev, dev_name(&sdp->sdev_gendev));
-
-	if (device_add(&sdkp->dev))
-		goto out_free_index;
-
-	get_device(&sdp->sdev_gendev);
-
 	if (index < SD_MAX_DISKS) {
 		gd->major = sd_major((index & 0xf0) >> 4);
 		gd->first_minor = ((index & 0xf) << 4) | (index & 0xfff00);
@@ -1954,11 +1936,6 @@ static void sd_probe_async(void *data, a
 
 	sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
 		  sdp->removable ? "removable " : "");
-
-	return;
-
- out_free_index:
-	ida_remove(&sd_index_ida, index);
 }
 
 /**
@@ -2026,6 +2003,24 @@ static int sd_probe(struct device *dev)
 	sdkp->openers = 0;
 	sdkp->previous_state = 1;
 
+	if (!sdp->request_queue->rq_timeout) {
+		if (sdp->type != TYPE_MOD)
+			blk_queue_rq_timeout(sdp->request_queue, SD_TIMEOUT);
+		else
+			blk_queue_rq_timeout(sdp->request_queue,
+					     SD_MOD_TIMEOUT);
+	}
+
+	device_initialize(&sdkp->dev);
+	sdkp->dev.parent = &sdp->sdev_gendev;
+	sdkp->dev.class = &sd_disk_class;
+	dev_set_name(&sdkp->dev, dev_name(&sdp->sdev_gendev));
+
+	if (device_add(&sdkp->dev))
+		goto out_free_index;
+
+	get_device(&sdp->sdev_gendev);
+
 	async_schedule(sd_probe_async, sdkp);
 
 	return 0;
@@ -2055,7 +2050,11 @@ static int sd_probe(struct device *dev)
  **/
 static int sd_remove(struct device *dev)
 {
-	struct scsi_disk *sdkp = dev_get_drvdata(dev);
+	struct scsi_disk *sdkp;
+
+	/* Wait for sd_probe_async to finish */
+	async_synchronize_full();
+	sdkp = dev_get_drvdata(dev);
 
 	device_del(&sdkp->dev);
 	del_gendisk(sdkp->disk);

Re: [PATCH] sd: no errors allowed during async probing

[James Bottomley]

On Tue, 2009-06-02 at 14:05 -0400, Alan Stern wrote:
> This patch (as1252) fixes a bug in the sd probing code.  When the
> probe routine was split up into a synchronous and an asynchronous
> part, too much was put into the asynchronous part.  It's important
> that all the possible failure modes occur synchronously, so that the
> driver core knows whether the probe was successful even before the
> async part is complete.
> 
> Another bug is that device removal has to wait for the async probing
> to finish!  The patch addresses both bugs, by moving some code back
> from sd_probe_async() to sd_probe() and by adding a call to
> async_synchronize_full() at the start of sd_remove().
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

This is pretty much line by line identical to the patch I already
posted, isn't it?  If not, help me understand what's different.

James

Re: [PATCH] sd: no errors allowed during async probing

[Alan Stern]

On Tue, 2 Jun 2009, James Bottomley wrote:

> On Tue, 2009-06-02 at 14:05 -0400, Alan Stern wrote:
> > This patch (as1252) fixes a bug in the sd probing code.  When the
> > probe routine was split up into a synchronous and an asynchronous
> > part, too much was put into the asynchronous part.  It's important
> > that all the possible failure modes occur synchronously, so that the
> > driver core knows whether the probe was successful even before the
> > async part is complete.
> > 
> > Another bug is that device removal has to wait for the async probing
> > to finish!  The patch addresses both bugs, by moving some code back
> > from sd_probe_async() to sd_probe() and by adding a call to
> > async_synchronize_full() at the start of sd_remove().
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> This is pretty much line by line identical to the patch I already
> posted, isn't it?  If not, help me understand what's different.

It is the same except for the very end, where you realized that you had 
forgotten to wait for the async_synchronize_full() to complete before 
calling dev_get_drvdata().  If you ever posted a corrected version of 
the patch, I never saw it.

My intention wasn't to steal your thunder or anything like that.  It
was just to get a final, correct version of the patch into circulation
for people to test and for merging.  I had (and still have!) no way of 
knowing whether you ever finished the patch or merged it into any git 
trees -- it isn't currently in scsi-misc.

Alan Stern

Re: [PATCH] sd: no errors allowed during async probing

[James Bottomley]

On Tue, 2009-06-02 at 14:41 -0400, Alan Stern wrote:
> On Tue, 2 Jun 2009, James Bottomley wrote:
> 
> > On Tue, 2009-06-02 at 14:05 -0400, Alan Stern wrote:
> > > This patch (as1252) fixes a bug in the sd probing code.  When the
> > > probe routine was split up into a synchronous and an asynchronous
> > > part, too much was put into the asynchronous part.  It's important
> > > that all the possible failure modes occur synchronously, so that the
> > > driver core knows whether the probe was successful even before the
> > > async part is complete.
> > > 
> > > Another bug is that device removal has to wait for the async probing
> > > to finish!  The patch addresses both bugs, by moving some code back
> > > from sd_probe_async() to sd_probe() and by adding a call to
> > > async_synchronize_full() at the start of sd_remove().
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > 
> > This is pretty much line by line identical to the patch I already
> > posted, isn't it?  If not, help me understand what's different.
> 
> It is the same except for the very end, where you realized that you had 
> forgotten to wait for the async_synchronize_full() to complete before 
> calling dev_get_drvdata().  If you ever posted a corrected version of 
> the patch, I never saw it.

It's in my internal tree ... I didn't repost for something that trivial.

> My intention wasn't to steal your thunder or anything like that.  It
> was just to get a final, correct version of the patch into circulation
> for people to test and for merging.  I had (and still have!) no way of 
> knowing whether you ever finished the patch or merged it into any git 
> trees -- it isn't currently in scsi-misc.

Yes, I haven't quite decided ... It's a bug, but it doesn't look like
it's likely to be tripped in the ordinary course of events, so I was
hesitating about putting it in the last rc-fixes tree.  Of course, if it
turns out to fix the device_del() oops that would change things
entirely.

James