Re: block/bsg.c

Re: block/bsg.c

[Bartlomiej Zolnierkiewicz]

Hi,

Some more bsg findings...


block/Kconfig:

endif # BLOCK

Shouldn't BLK_DEV_BSG depend on BLOCK?

config BLK_DEV_BSG
        bool "Block layer SG support"
        depends on (SCSI=y) && EXPERIMENTAL
        default y
        ---help---
        Saying Y here will enable generic SG (SCSI generic) v4
        support for any block device.


block/bsg.c:

...

/*
 * TODO
 *      - Should this get merged, block/scsi_ioctl.c will be migrated into
 *        this file. To keep maintenance down, it's easier to have them
 *        seperated right now.
 *
 */

This TODO should be fixed/removed.

Firstly bsg got merged. ;)  Moreover bsg depends on SCSI and scsi_ioctl doesn't.
Even if SCSI dependency is fixed bsg requires block driver to have struct
class devices which is not a case for scsi_ioctl.

...

static struct bsg_device *__bsg_get_device(int minor)
{
        struct hlist_head *list = &bsg_device_list[bsg_list_idx(minor)];

bsg_device_list[] access should be done under bsg_mutex lock.

May not be a problem currently because of lock_kernel but worth fixing anyway.

        struct bsg_device *bd = NULL;
        struct hlist_node *entry;

        mutex_lock(&bsg_mutex);

...

static int
bsg_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
          unsigned long arg)
{
...
        case SCSI_IOCTL_SEND_COMMAND: {

Do we really want to add support for this *deprecated* ioctl to
the *new* and shiny bsg driver?

                void __user *uarg = (void __user *) arg;
                return scsi_cmd_ioctl(file, bd->queue, NULL, cmd, uarg);
        }

...

int bsg_register_queue(struct request_queue *q, const char *name)
{
...
        memset(bcd, 0, sizeof(*bcd));
...
        dev = MKDEV(BSG_MAJOR, bcd->minor);
        class_dev = class_device_create(bsg_class, NULL, dev, bcd->dev, "%s", name);

bcd->dev is always 0 (NULL).

Is it OK to pass NULL struct device *dev argument to class_device_create()?

It should be fixed by either removing bcd->dev or by setting it to something
other than zero.

...

MODULE_AUTHOR("Jens Axboe");
MODULE_DESCRIPTION("Block layer SGSI generic (sg) driver");

SGSI? :)

MODULE_LICENSE("GPL");


Now back to the first bsg commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3:

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index f429be8..a21f585 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -1258,19 +1258,25 @@ static void idefloppy_create_rw_cmd (idefloppy_floppy_t *floppy, idefloppy_pc_t
 	set_bit(PC_DMA_RECOMMENDED, &pc->flags);
 }
 
-static int
+static void
 idefloppy_blockpc_cmd(idefloppy_floppy_t *floppy, idefloppy_pc_t *pc, struct request *rq)
 {
-	/*
-	 * just support eject for now, it would not be hard to make the
-	 * REQ_BLOCK_PC support fully-featured
-	 */
-	if (rq->cmd[0] != IDEFLOPPY_START_STOP_CMD)
-		return 1;
-
 	idefloppy_init_pc(pc);
+	pc->callback = &idefloppy_rw_callback;
 	memcpy(pc->c, rq->cmd, sizeof(pc->c));
-	return 0;
+	pc->rq = rq;
+	pc->b_count = rq->data_len;
+	if (rq->data_len && rq_data_dir(rq) == WRITE)
+		set_bit(PC_WRITING, &pc->flags);
+	pc->buffer = rq->data;
+	if (rq->bio)
+		set_bit(PC_DMA_RECOMMENDED, &pc->flags);

Great to see SG_IO support being added to ide-floppy but this change has
nothing to do with the addition of bsg driver so WTF they have been mixed
within the same commit?

I also can't recall seeing this patch on linux-ide mailing list...

+	/*
+	 * possibly problematic, doesn't look like ide-floppy correctly
+	 * handled scattered requests if dma fails...
+	 */

Could you give some details on this?

+	pc->request_transfer = pc->buffer_size = rq->data_len;
 }
 
 /*
@@ -1317,10 +1323,7 @@ static ide_startstop_t idefloppy_do_request (ide_drive_t *drive, struct request
 		pc = (idefloppy_pc_t *) rq->buffer;
 	} else if (blk_pc_request(rq)) {
 		pc = idefloppy_next_pc_storage(drive);
-		if (idefloppy_blockpc_cmd(floppy, pc, rq)) {
-			idefloppy_do_end_request(drive, 0, 0);
-			return ide_stopped;
-		}
+		idefloppy_blockpc_cmd(floppy, pc, rq);
 	} else {
 		blk_dump_rq_flags(rq,
 			"ide-floppy: unsupported command in queue");
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index c948a5c..9ae60a7 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c

ide.c changes also have nothing to do with addition of bsg driver.

@@ -1049,9 +1049,13 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
 	unsigned long flags;
 	ide_driver_t *drv;
 	void __user *p = (void __user *)arg;
-	int err = 0, (*setfunc)(ide_drive_t *, int);
+	int err, (*setfunc)(ide_drive_t *, int);
 	u8 *val;
 
+	err = scsi_cmd_ioctl(file, bdev->bd_disk, cmd, p);
+	if (err != -ENOTTY)
+		return err;
+

Probably the goal was to add SG_IO IOCTL support for ide-floppy but instead
it adds support for *all* scsi_ioctl.c IOCTLs to *all* IDE device drivers.

ide-{disk,scsi,tape} don't support REQ_TYPE_BLOCK_PC requests et all
(so attempts to use SG_IO, CDROM_SEND_PACKET and SCSI_IOCTL_SEND_COMMAND
will result in requests being failed and error messages in the kernel logs).
Without REQ_TYPE_BLOCK_PC support there is no sense in supporting any other
scsi_ioctl.c IOCTLs (while at it: SG_{GET,SET}_RESERVED seems to have no
real effect in the current tree).

ide-cd already supports all scsi_ioctl.c IOCTLs through cdrom_ioctl() call.

This leaves us with ide-floppy where the above scsi_cmd_ioctl() call should
belong (we may also want to filter out deprecated SCSI_IOCTL_SEND_COMMAND
and legacy CDROM_SEND_PACKET IOCTLs).

Please fix it.

 	switch (cmd) {
 	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
 	case HDIO_GET_KEEPSETTINGS: val = &drive->keep_settings; goto read_val;
@@ -1171,10 +1175,6 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
 			return 0;
 		}
 
-		case CDROMEJECT:
-		case CDROMCLOSETRAY:
-			return scsi_cmd_ioctl(file, bdev->bd_disk, cmd, p);
-

This chunk is OK, handling of these IOCTLs should go to ide-floppy
(ide-cd already handles them fine).

Thanks,
Bart

Re: block/bsg.c

[Andrew Morton]

On Tue, 17 Jul 2007 22:52:25 +0200
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> ide-{disk,scsi,tape} don't support REQ_TYPE_BLOCK_PC requests et all
> (so attempts to use SG_IO, CDROM_SEND_PACKET and SCSI_IOCTL_SEND_COMMAND
> will result in requests being failed and error messages in the kernel logs).

In my case it results in a completely tits-up machine.  CPU stuck somewhere
spinning with local interrutps disabled.

If you see the log output I sent, I'm suspecting this might be because
this BSG brainfart has exposed underlying bugs in IDE or SCSI.

Note that I saw literally 20,000 lines of output in the bit which I snipped,
so something has gone pretty wild in the handling of these bogus commands.

Re: block/bsg.c

[Bartlomiej Zolnierkiewicz]

On Tuesday 17 July 2007, Andrew Morton wrote:
> On Tue, 17 Jul 2007 22:52:25 +0200
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > ide-{disk,scsi,tape} don't support REQ_TYPE_BLOCK_PC requests et all
> > (so attempts to use SG_IO, CDROM_SEND_PACKET and SCSI_IOCTL_SEND_COMMAND
> > will result in requests being failed and error messages in the kernel logs).
> 
> In my case it results in a completely tits-up machine.  CPU stuck somewhere
> spinning with local interrutps disabled.
> 
> If you see the log output I sent, I'm suspecting this might be because

I don't. :(

> this BSG brainfart has exposed underlying bugs in IDE or SCSI.

Possible but ATM I have an (overdue) IDE update to push, people waiting for
(overdue) patches to try and some (overdue) patches to finish and post...

Would be great if somebody beats me to investigate this issue.

Hint: this would be a great way to redempt pushing buggy commit behind
my back. ;-)

Thanks,
Bart

Re: block/bsg.c

[FUJITA Tomonori]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 22:52:25 +0200

> /*
>  * TODO
>  *      - Should this get merged, block/scsi_ioctl.c will be migrated into
>  *        this file. To keep maintenance down, it's easier to have them
>  *        seperated right now.
>  *
>  */
> 
> This TODO should be fixed/removed.

Yeah, this should be removed now. I'll do.


> Firstly bsg got merged. ;)  Moreover bsg depends on SCSI and scsi_ioctl doesn't.
> Even if SCSI dependency is fixed bsg requires block driver to have struct
> class devices which is not a case for scsi_ioctl.
> 
> ...
> 
> static struct bsg_device *__bsg_get_device(int minor)
> {
>         struct hlist_head *list = &bsg_device_list[bsg_list_idx(minor)];
> 
> bsg_device_list[] access should be done under bsg_mutex lock.
> 
> May not be a problem currently because of lock_kernel but worth fixing anyway.
> 
>         struct bsg_device *bd = NULL;
>         struct hlist_node *entry;
> 
>         mutex_lock(&bsg_mutex);
> 

They were fixed. Please check the latest code:

git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git bsg

We wait for Linus to pull from it.


> static int
> bsg_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>           unsigned long arg)
> {
> ...
>         case SCSI_IOCTL_SEND_COMMAND: {
> 
> Do we really want to add support for this *deprecated* ioctl to
> the *new* and shiny bsg driver?
> 
>                 void __user *uarg = (void __user *) arg;
>                 return scsi_cmd_ioctl(file, bd->queue, NULL, cmd, uarg);
>         }

We might remove it.


> int bsg_register_queue(struct request_queue *q, const char *name)
> {
> ...
>         memset(bcd, 0, sizeof(*bcd));
> ...
>         dev = MKDEV(BSG_MAJOR, bcd->minor);
>         class_dev = class_device_create(bsg_class, NULL, dev, bcd->dev, "%s", name);
> 
> bcd->dev is always 0 (NULL).
> 
> Is it OK to pass NULL struct device *dev argument to class_device_create()?

It's ok, I guess.


> It should be fixed by either removing bcd->dev or by setting it to something
> other than zero.
> 
> ...
> 
> MODULE_AUTHOR("Jens Axboe");
> MODULE_DESCRIPTION("Block layer SGSI generic (sg) driver");
> 
> SGSI? :)

It was fixed in the latest code.


Thanks,

Re: block/bsg.c

[Bartlomiej Zolnierkiewicz]

On Wednesday 18 July 2007, FUJITA Tomonori wrote:

> They were fixed. Please check the latest code:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git bsg
> 
> We wait for Linus to pull from it.

It has been merged.  Thanks for fixing the mentioned issues.

Thanks,
Bart

Re: block/bsg.c

[FUJITA Tomonori]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: Re: block/bsg.c
Date: Wed, 18 Jul 2007 22:39:38 +0200

> 
> On Wednesday 18 July 2007, FUJITA Tomonori wrote:
> 
> > They were fixed. Please check the latest code:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git bsg
> > 
> > We wait for Linus to pull from it.
> 
> It has been merged.  Thanks for fixing the mentioned issues.

You're welcome. Thanks for the ide-floppy fix.

Re: block/bsg.c

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday 18 July 2007, James Bottomley wrote:
> On Wed, 2007-07-18 at 02:43 +0200, Bartlomiej Zolnierkiewicz wrote:
> > [ James, please remeber to cc: linux-ide on IDE patches, thanks. ]
> 
> Blame Andrew ... I assumed he'd be reporting the problem to the relevant
> lists, so I just did a reply all ...

No need to blame anybody, especially since it seems that one more person
forgot about adding linux-ide ML.  ;)

> > On Wednesday 18 July 2007, Jeff Garzik wrote:
> > > James Bottomley wrote:
> > > > @@ -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;
> > > 
> > > 
> > > At that point you might as well use an 'if'.
> > > 
> > > But overall -- agreed.  ACK.
> > 
> > James/Jeff thanks for following the issue but NAK. ;)
> > 
> > Causes regression wrt ide-floppy CDROMEJECT/CDROMCLOSETRAY support when
> > compared to 2.6.22 and SG_IO is not supported by ide-{disk,scsi,tape}.
> 
> Well ... that was why I put the case statement in ... I was sure there
> would be other ioctls I missed.

The thing is that ide-{disk,scsi,tape} really don't support
REQ_TYPE_BLOCK_PC type requests so ide.c::generic_ide_ioctl()
is not the best place to add handling of SG_IO.

Patch attached.

> > Luckily Linus has already fixed the issue properly.
> 
> Actually, no, that's just a reversion.  I think we need the attached to
> complete all of this.
> 
> > BTW cmd == 1 IOCTL is not defined/used by IDE driver.
> 
> Andrew's trace clearly shows that something is sending cmd == 1 down, so

I still haven't got the trace.  Andrew, please (re)send it in PM, thanks.

> I'm nearly positive at one time this was a legitimate ioctl for IDE.

Could be...

> Anyway it's probably time to kill this deprecated ioctl in SCSI.

Fully agreed.

Thanks,
Bart


[PATCH] ide: add support for SCSI ioctls to ide-floppy

Now that ide-floppy supports SG_IO we can add support for SCSI ioctls
(except deprecated SCSI_IOCTL_SEND_COMMAND and legacy CDROM_SEND_PACKET
ones - we can add them later iff really needed).

While at it remove handling of CDROMEJECT and CDROMCLOSETRAY ioctls from
generic_ide_ioctl():

- This prevents ide-{disk,tape,scsi} device drivers from obtaining
  REQ_TYPE_BLOCK_PC type requests which are currently unsupported by
  these drivers and which are potentially harmful (as reported by Andrew).

- There is no functionality loss since aforementioned ioctls will now be
  handled by idefloppy_ioctl()->scsi_cmd_ioctl() (for devices using
  ide-floppy driver) and by idecd_ioctl->cdrom_ioctl()->scsi_cmd_ioctl()
  (for devices using ide-cd driver).

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: James Bottomley <James.Bottomley@steeleye.com>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/ide/ide-floppy.c |   18 +++++++++++++++++-
 drivers/ide/ide.c        |    4 ----
 2 files changed, 17 insertions(+), 5 deletions(-)

Index: b/drivers/ide/ide-floppy.c
===================================================================
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -99,6 +99,8 @@
 #include <linux/bitops.h>
 #include <linux/mutex.h>
 
+#include <scsi/scsi_ioctl.h>
+
 #include <asm/byteorder.h>
 #include <asm/irq.h>
 #include <asm/uaccess.h>
@@ -2099,7 +2101,21 @@ static int idefloppy_ioctl(struct inode 
 	case IDEFLOPPY_IOCTL_FORMAT_GET_PROGRESS:
 		return idefloppy_get_format_progress(drive, argp);
 	}
-	return generic_ide_ioctl(drive, file, bdev, cmd, arg);
+
+	/*
+	 * skip SCSI_IOCTL_SEND_COMMAND (deprecated)
+	 * and CDROM_SEND_PACKET (legacy) ioctls
+	 */
+	if (cmd != CDROM_SEND_PACKET && cmd != SCSI_IOCTL_SEND_COMMAND)
+		err = scsi_cmd_ioctl(file, bdev->bd_disk->queue,
+					bdev->bd_disk, cmd, argp);
+	else
+		err = -ENOTTY;
+
+	if (err == -ENOTTY)
+		err = generic_ide_ioctl(drive, file, bdev, cmd, arg);
+
+	return err;
 }
 
 static int idefloppy_media_changed(struct gendisk *disk)
Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -1171,10 +1171,6 @@ int generic_ide_ioctl(ide_drive_t *drive
 			return 0;
 		}
 
-		case CDROMEJECT:
-		case CDROMCLOSETRAY:
-			return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
-
 		case HDIO_GET_BUSSTATE:
 			if (!capable(CAP_SYS_ADMIN))
 				return -EACCES;

Re: block/bsg.c

[James Bottomley]

On Wed, 2007-07-18 at 22:32 +0200, Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> On Wednesday 18 July 2007, James Bottomley wrote:
> > On Wed, 2007-07-18 at 02:43 +0200, Bartlomiej Zolnierkiewicz wrote:
> > > [ James, please remeber to cc: linux-ide on IDE patches, thanks. ]
> > 
> > Blame Andrew ... I assumed he'd be reporting the problem to the relevant
> > lists, so I just did a reply all ...
> 
> No need to blame anybody, especially since it seems that one more person
> forgot about adding linux-ide ML.  ;)
> 
> > > On Wednesday 18 July 2007, Jeff Garzik wrote:
> > > > James Bottomley wrote:
> > > > > @@ -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;
> > > > 
> > > > 
> > > > At that point you might as well use an 'if'.
> > > > 
> > > > But overall -- agreed.  ACK.
> > > 
> > > James/Jeff thanks for following the issue but NAK. ;)
> > > 
> > > Causes regression wrt ide-floppy CDROMEJECT/CDROMCLOSETRAY support when
> > > compared to 2.6.22 and SG_IO is not supported by ide-{disk,scsi,tape}.
> > 
> > Well ... that was why I put the case statement in ... I was sure there
> > would be other ioctls I missed.
> 
> The thing is that ide-{disk,scsi,tape} really don't support
> REQ_TYPE_BLOCK_PC type requests so ide.c::generic_ide_ioctl()
> is not the best place to add handling of SG_IO.

SG_IO has become a general packet request ioctl, so I think the idea is
to allow it to send taskfiles down ... I'll let Jens speak to where the
actual pieces to do this currently stand.

> Patch attached.
> 
> > > Luckily Linus has already fixed the issue properly.
> > 
> > Actually, no, that's just a reversion.  I think we need the attached to
> > complete all of this.
> > 
> > > BTW cmd == 1 IOCTL is not defined/used by IDE driver.
> > 
> > Andrew's trace clearly shows that something is sending cmd == 1 down, so
> 
> I still haven't got the trace.  Andrew, please (re)send it in PM, thanks.

http://marc.info/?l=linux-scsi&m=118470384305607

James