Re: block/bsg.c

[James Bottomley <James.Bottomley@SteelEye.com>]

On Tue, 2007-07-17 at 13:22 -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2007 12:18:21 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Tue, 17 Jul 2007 08:38:11 +0200 Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > > > In terms of presentation: this code hit the tree as base patch plus what
> > > > appear to be 20 bugfixes, none of which are really interesting or relevant
> > > > to mainline.  Personally I think it would be nicer if all that out-of-tree
> > > > development work was cleaned up and the new code goes in as a single hit.
> > > > 
> > > > This makes it a lot easier to find out "wtf does this code all do".  One
> > > > finds the first commit and reads the changlog.  But this algorithm yields:
> > > > 
> > > >     bsg: support for full generic block layer SG v3
> > > > 
> > > > which is not helpful.
> > > 
> > > I agree, I did consider rebasing the merging all patches into a single
> > > commit prior to submission. In retrospect that would have been better,
> > > the bug fixes commits prior to inclusion is not that interesting.
> > 
> > I'm doing a git-bisect and....
> > 
> > block/bsg.c: In function 'bsg_register_queue':
> > block/bsg.c:1014: error: 'struct kobject' has no member named 'dentry'
> > 
> > That's easily fixable in config, but it's another reason for doing
> > that consolidation prior to merging.
> 
> So this rather painful and compiler-errorful bisection session ended up at:
> 
> commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3
> Author: Jens Axboe <jens.axboe@oracle.com>
> Date:   Mon Jul 9 12:38:05 2007 +0200
> 
>     bsg: support for full generic block layer SG v3                                 
> 
> 
> What is happening is that my old dual-PIII IDE-PIIX box running
> hacked-about FC1 is locking up early in initscripts, just after "Finding
> module dependencies".
> 
> Config is http://userweb.kernel.org/~akpm/config-vmm.txt with CONFIG_SCSI=n.
> 
> Occasionally I'll get an NMI watchdog timeout, but it's a rather
> uninteresting one: the stack trace just points up into the idle task.
> 
> This:
> 
> --- a/drivers/ide/ide.c~a
> +++ a/drivers/ide/ide.c
> @@ -1052,9 +1052,9 @@ int generic_ide_ioctl(ide_drive_t *drive
>  	int err, (*setfunc)(ide_drive_t *, int);
>  	u8 *val;
>  
> -	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> -	if (err != -ENOTTY)
> -		return err;
> +//	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> +//	if (err != -ENOTTY)
> +//		return err;
>  
>  	switch (cmd) {
>  	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
> _
> 
> fixes it.
> 
> 
> I added this:
> 
> --- a/drivers/ide/ide.c~a
> +++ a/drivers/ide/ide.c
> @@ -1052,7 +1052,9 @@ int generic_ide_ioctl(ide_drive_t *drive
>  	int err, (*setfunc)(ide_drive_t *, int);
>  	u8 *val;
>  
> +	printk("%s: cmd=%d\n", __FUNCTION__, cmd);
>  	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> +	printk("%s: err=%d\n", __FUNCTION__, err);
>  	if (err != -ENOTTY)
>  		return err;
>  
> _
> 
> 
> and got:
> 
> default.hotplug used greatest stack depth: 6448 bytes left
> hotplug used greatest stack depth: 6396 bytes left
> hotplug used greatest stack depth: 5540 bytes left
> EXT3 FS on hdc2, internal journal
> Adding 1020116k swap on /dev/hdc3.  Priority:-1 extents:1 across:1020116k
> generic_ide_ioctl: cmd=21382
> generic_ide_ioctl: err=0
> generic_ide_ioctl: cmd=1
> program scsi_unique_id is using a deprecated SCSI ioctl, please convert it to SG_IO

I can tell you what went wrong:

This cmd=1 is SCSI_IOCTL_SEND_COMMAND, but that doesn't seem to be
what's intended ... I'm guessing it's a legacy ide ioctl value which
suddenly has become interpreted as a scsi_ioctl ... and certainly a non
CD IDE device cannot handle a SCSI command, so all hell breaks loose.

I suspect all we really want from this addition is to be able to get
SG_IO on the ide device, in which case this should be the fix (I put a
case statement instead of an if so we can add other ioctl values to it
in case I missed any).

James

diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index 8cd7694..9e96662 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -156,6 +156,8 @@
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
+#include <scsi/sg.h>
+
 
 /* default maximum number of failures */
 #define IDE_DEFAULT_MAX_FAILURES 	1
@@ -1052,9 +1054,10 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
 	int err, (*setfunc)(ide_drive_t *, int);
 	u8 *val;
 
-	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
-	if (err != -ENOTTY)
-		return err;
+	switch (cmd) {
+	case SG_IO:
+		return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
+	}
 
 	switch (cmd) {
 	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;