Re: block/bsg.c
From: Bartlomiej Zolnierkiewicz
Date: Tue Jul 17 2007 - 16:35:34 EST
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
-
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/