error in drivers/block/scsi_ioctl.c and ll_rw_block.c?

error in drivers/block/scsi_ioctl.c and ll_rw_block.c?

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

While working on usb-storage (a virtual SCSI HBA), I noticed that the
command 'eject /dev/scd0' sent a START_STOP command to the device with the
data direction set to SCSI_DATA_WRITE but a transfer length of zero.  This
causes a problem for some code paths.

For clarity, the START_STOP command doesn't want to move any data at all.

It looks to me like the error is a combination of
drivers/block/scsi_ioctl.c and ll_rw_block.c

scsi_ioctl.c calls blk_get_request(q, WRITE, __GFP_WAIT) to allocate the
request -- specifying WRITE here is one problem.

In ll_rw_block.c, blk_get_request() calls BUG_ON(rq != READ && rw != WRITE)
-- in other words, it can only allocate a request for reading or writing,
but not for no data.  I'm not familiar with this code, but it looks like
requests are tracked by data direction, so making this accept NONE may be
difficult.

One possible solution may be to re-write the CDROMEJECT ioctl into a call
to sg_scsi_ioctl(), but that doesn't fix the general problem with
ll_rw_block.c -- if, indeed, that is a problem.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

It's not that hard.  No matter what the problem is, tell the customer 
to reinstall Windows.
					-- Nurse
User Friendly, 3/22/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: error in drivers/block/scsi_ioctl.c and ll_rw_block.c?

[Jens Axboe]

On Sun, Sep 28 2003, Matthew Dharm wrote:
> While working on usb-storage (a virtual SCSI HBA), I noticed that the
> command 'eject /dev/scd0' sent a START_STOP command to the device with the
> data direction set to SCSI_DATA_WRITE but a transfer length of zero.  This
> causes a problem for some code paths.
> 
> For clarity, the START_STOP command doesn't want to move any data at all.
> 
> It looks to me like the error is a combination of
> drivers/block/scsi_ioctl.c and ll_rw_block.c
> 
> scsi_ioctl.c calls blk_get_request(q, WRITE, __GFP_WAIT) to allocate the
> request -- specifying WRITE here is one problem.
> 
> In ll_rw_block.c, blk_get_request() calls BUG_ON(rq != READ && rw != WRITE)
> -- in other words, it can only allocate a request for reading or writing,
> but not for no data.  I'm not familiar with this code, but it looks like
> requests are tracked by data direction, so making this accept NONE may be
> difficult.
> 
> One possible solution may be to re-write the CDROMEJECT ioctl into a call
> to sg_scsi_ioctl(), but that doesn't fix the general problem with
> ll_rw_block.c -- if, indeed, that is a problem.

No it's not a problem, clearly the request doesn't want to move any data
if rq->data_len == 0.

===== drivers/scsi/sr.c 1.93 vs edited =====
--- 1.93/drivers/scsi/sr.c	Fri Sep  5 13:31:51 2003
+++ edited/drivers/scsi/sr.c	Mon Sep 29 09:49:43 2003
@@ -289,12 +289,12 @@
 			return 0;
 
 		memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
-		if (rq_data_dir(rq) == WRITE)
+		if (!rq->data_len)
+			SCpnt->sc_data_direction = SCSI_DATA_NONE;
+		else if (rq_data_dir(rq) == WRITE)
 			SCpnt->sc_data_direction = SCSI_DATA_WRITE;
-		else if (rq->data_len)
+		else (rq->data_len)
 			SCpnt->sc_data_direction = SCSI_DATA_READ;
-		else
-			SCpnt->sc_data_direction = SCSI_DATA_NONE;
 
 		this_count = rq->data_len;
 		if (rq->timeout)

-- 
Jens Axboe

Re: error in drivers/block/scsi_ioctl.c and ll_rw_block.c?

[Jens Axboe]

On Mon, Sep 29 2003, Jens Axboe wrote:
> On Sun, Sep 28 2003, Matthew Dharm wrote:
> > While working on usb-storage (a virtual SCSI HBA), I noticed that the
> > command 'eject /dev/scd0' sent a START_STOP command to the device with the
> > data direction set to SCSI_DATA_WRITE but a transfer length of zero.  This
> > causes a problem for some code paths.
> > 
> > For clarity, the START_STOP command doesn't want to move any data at all.
> > 
> > It looks to me like the error is a combination of
> > drivers/block/scsi_ioctl.c and ll_rw_block.c
> > 
> > scsi_ioctl.c calls blk_get_request(q, WRITE, __GFP_WAIT) to allocate the
> > request -- specifying WRITE here is one problem.
> > 
> > In ll_rw_block.c, blk_get_request() calls BUG_ON(rq != READ && rw != WRITE)
> > -- in other words, it can only allocate a request for reading or writing,
> > but not for no data.  I'm not familiar with this code, but it looks like
> > requests are tracked by data direction, so making this accept NONE may be
> > difficult.
> > 
> > One possible solution may be to re-write the CDROMEJECT ioctl into a call
> > to sg_scsi_ioctl(), but that doesn't fix the general problem with
> > ll_rw_block.c -- if, indeed, that is a problem.
> 
> No it's not a problem, clearly the request doesn't want to move any data
> if rq->data_len == 0.

This one compiles :)

===== drivers/scsi/sr.c 1.93 vs edited =====
--- 1.93/drivers/scsi/sr.c	Fri Sep  5 13:31:51 2003
+++ edited/drivers/scsi/sr.c	Mon Sep 29 09:55:19 2003
@@ -289,12 +289,12 @@
 			return 0;
 
 		memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
-		if (rq_data_dir(rq) == WRITE)
+		if (!rq->data_len)
+			SCpnt->sc_data_direction = SCSI_DATA_NONE;
+		else if (rq_data_dir(rq) == WRITE)
 			SCpnt->sc_data_direction = SCSI_DATA_WRITE;
-		else if (rq->data_len)
-			SCpnt->sc_data_direction = SCSI_DATA_READ;
 		else
-			SCpnt->sc_data_direction = SCSI_DATA_NONE;
+			SCpnt->sc_data_direction = SCSI_DATA_READ;
 
 		this_count = rq->data_len;
 		if (rq->timeout)

-- 
Jens Axboe