[PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Ben Collins]

Reference: https://bugzilla.ubuntu.com/5049

The eject command was failing for a large group of users for removable
devices. The "eject -r" command, which uses the CDROMEJECT ioctl would not
work, however "eject -s", which uses SG_IO did work, but required root access.

Since SG_IO was using the same mechanism as CDROMEJECT, there should be no
difference. The main reason for getting the CDROMEJECT ioctl working was
because it didn't need root privileges like the SG_IO commands did.

One bug was noticed, and that is CDROMEJECT was setting the blk request to a
WRITE operation, when in fact it wasn't. The block layer did not like getting
WRITE requests when data_len==0 and data==NULL.

The other issue was that "eject -s" was sending two extra commands that
seemed to work for just about any removable device. CDROMEJECT works as
such:

START_STOP : 0x02

While "eject -s" sent(via SG_IO):

ALLOW_MEDIUM_REMOVAL
START_STOP : 0x01
START_STOP : 0x02

This patch fixes the WRITE vs READ issue, and also sends the extra two
commands. Anyone with an iPod connected via USB (not sure about firewire)
should be able to reproduce this issue, and verify the patch.


diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 382dea7..573da94 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -442,11 +442,38 @@ error:
 	return err;
 }
 
+/* This is only useful for the following macros */
+static int __blk_send_generic(request_queue_t *q, struct gendisk *bd_disk, int cmd, int data)
+{
+	struct request *rq;
+	int err;
+
+	rq = blk_get_request(q, READ, __GFP_WAIT);
+	rq->flags |= REQ_BLOCK_PC;
+	rq->data = NULL;
+	rq->data_len = 0;
+	rq->timeout = BLK_DEFAULT_TIMEOUT;
+	memset(rq->cmd, 0, sizeof(rq->cmd));
+	rq->cmd[0] = cmd;
+	rq->cmd[4] = data;
+	rq->cmd_len = 6;
+	err = blk_execute_rq(q, bd_disk, rq, 0);
+	blk_put_request(rq);
+
+	return err;
+}
+
+#define blk_send_start_stop(q, bd_disk, data) \
+	__blk_send_generic(q, bd_disk, GPCMD_START_STOP_UNIT, data)
+
+#define blk_send_allow_medium_removal(q, bd_disk) \
+	__blk_send_generic(q, bd_disk, GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL, 0)
+
+
 int scsi_cmd_ioctl(struct file *file, struct gendisk *bd_disk, unsigned int cmd, void __user *arg)
 {
 	request_queue_t *q;
-	struct request *rq;
-	int close = 0, err;
+	int err;
 
 	q = bd_disk->queue;
 	if (!q)
@@ -564,19 +591,14 @@ int scsi_cmd_ioctl(struct file *file, st
 			err = sg_scsi_ioctl(file, q, bd_disk, arg);
 			break;
 		case CDROMCLOSETRAY:
-			close = 1;
+			err = blk_send_start_stop(q, bd_disk, 0x03);
+			break;
 		case CDROMEJECT:
-			rq = blk_get_request(q, WRITE, __GFP_WAIT);
-			rq->flags |= REQ_BLOCK_PC;
-			rq->data = NULL;
-			rq->data_len = 0;
-			rq->timeout = BLK_DEFAULT_TIMEOUT;
-			memset(rq->cmd, 0, sizeof(rq->cmd));
-			rq->cmd[0] = GPCMD_START_STOP_UNIT;
-			rq->cmd[4] = 0x02 + (close != 0);
-			rq->cmd_len = 6;
-			err = blk_execute_rq(q, bd_disk, rq, 0);
-			blk_put_request(rq);
+			err = 0;
+
+			err |= blk_send_allow_medium_removal(q, bd_disk);
+			err |= blk_send_start_stop(q, bd_disk, 0x01);
+			err |= blk_send_start_stop(q, bd_disk, 0x02);
 			break;
 		default:
 			err = -ENOTTY;

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Jens Axboe]

On Mon, Dec 19 2005, Ben Collins wrote:
> Reference: https://bugzilla.ubuntu.com/5049
> 
> The eject command was failing for a large group of users for removable
> devices. The "eject -r" command, which uses the CDROMEJECT ioctl would not
> work, however "eject -s", which uses SG_IO did work, but required root access.
> 
> Since SG_IO was using the same mechanism as CDROMEJECT, there should be no
> difference. The main reason for getting the CDROMEJECT ioctl working was
> because it didn't need root privileges like the SG_IO commands did.
> 
> One bug was noticed, and that is CDROMEJECT was setting the blk request to a
> WRITE operation, when in fact it wasn't. The block layer did not like getting
> WRITE requests when data_len==0 and data==NULL.

False, it can't be a write request if there's no data attached. Write is
simply used there because read requests are usually more precious.

> This patch fixes the WRITE vs READ issue, and also sends the extra two
> commands. Anyone with an iPod connected via USB (not sure about firewire)
> should be able to reproduce this issue, and verify the patch.

The bug was in the SCSI layer, and James already has the fix integrated
for that. It really should make 2.6.15, James are you sending it upwards
for that?

>  		case CDROMEJECT:
> -			rq = blk_get_request(q, WRITE, __GFP_WAIT);
> -			rq->flags |= REQ_BLOCK_PC;
> -			rq->data = NULL;
> -			rq->data_len = 0;
> -			rq->timeout = BLK_DEFAULT_TIMEOUT;
> -			memset(rq->cmd, 0, sizeof(rq->cmd));
> -			rq->cmd[0] = GPCMD_START_STOP_UNIT;
> -			rq->cmd[4] = 0x02 + (close != 0);
> -			rq->cmd_len = 6;
> -			err = blk_execute_rq(q, bd_disk, rq, 0);
> -			blk_put_request(rq);
> +			err = 0;
> +
> +			err |= blk_send_allow_medium_removal(q, bd_disk);
> +			err |= blk_send_start_stop(q, bd_disk, 0x01);
> +			err |= blk_send_start_stop(q, bd_disk, 0x02);

Do this in the eject tool, if it's required for some devices.

-- 
Jens Axboe

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Jens Axboe]

On Mon, Dec 19 2005, Jens Axboe wrote:
> > This patch fixes the WRITE vs READ issue, and also sends the extra two
> > commands. Anyone with an iPod connected via USB (not sure about firewire)
> > should be able to reproduce this issue, and verify the patch.
> 
> The bug was in the SCSI layer, and James already has the fix integrated
> for that. It really should make 2.6.15, James are you sending it upwards
> for that?

I missed the fact that it is already in -rc6, so no problems there.

-- 
Jens Axboe

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Ben Collins]

On Mon, 2005-12-19 at 20:35 +0100, Jens Axboe wrote:
> On Mon, Dec 19 2005, Ben Collins wrote:
> > Reference: https://bugzilla.ubuntu.com/5049
> > 
> > The eject command was failing for a large group of users for removable
> > devices. The "eject -r" command, which uses the CDROMEJECT ioctl would not
> > work, however "eject -s", which uses SG_IO did work, but required root access.
> > 
> > Since SG_IO was using the same mechanism as CDROMEJECT, there should be no
> > difference. The main reason for getting the CDROMEJECT ioctl working was
> > because it didn't need root privileges like the SG_IO commands did.
> > 
> > One bug was noticed, and that is CDROMEJECT was setting the blk request to a
> > WRITE operation, when in fact it wasn't. The block layer did not like getting
> > WRITE requests when data_len==0 and data==NULL.
> 
> False, it can't be a write request if there's no data attached. Write is
> simply used there because read requests are usually more precious.

Did you mean "can be a write request"? If not, then you just repeated
what I said.

> > This patch fixes the WRITE vs READ issue, and also sends the extra two
> > commands. Anyone with an iPod connected via USB (not sure about firewire)
> > should be able to reproduce this issue, and verify the patch.
> 
> The bug was in the SCSI layer, and James already has the fix integrated
> for that. It really should make 2.6.15, James are you sending it upwards
> for that?

Can you point me to this fix? Also, does the "fix" fix the case for IDE
CDROM's too?

> >  		case CDROMEJECT:
> > -			rq = blk_get_request(q, WRITE, __GFP_WAIT);
> > -			rq->flags |= REQ_BLOCK_PC;
> > -			rq->data = NULL;
> > -			rq->data_len = 0;
> > -			rq->timeout = BLK_DEFAULT_TIMEOUT;
> > -			memset(rq->cmd, 0, sizeof(rq->cmd));
> > -			rq->cmd[0] = GPCMD_START_STOP_UNIT;
> > -			rq->cmd[4] = 0x02 + (close != 0);
> > -			rq->cmd_len = 6;
> > -			err = blk_execute_rq(q, bd_disk, rq, 0);
> > -			blk_put_request(rq);
> > +			err = 0;
> > +
> > +			err |= blk_send_allow_medium_removal(q, bd_disk);
> > +			err |= blk_send_start_stop(q, bd_disk, 0x01);
> > +			err |= blk_send_start_stop(q, bd_disk, 0x02);
> 
> Do this in the eject tool, if it's required for some devices.

It already is in eject tool, but as described, that requires root
access. Not something I want to force a user to do in order to eject
their CDROM/iPod/USBStick in gnome. What exactly is wrong with the
commands? If they are harmless for devices that don't need it, and fix a
huge number of problems (did you see the Cc list on the bug report?) for
users with affected devices, then what's the harm?

-- 
   Ben Collins <ben.collins@ubuntu.com>
   Developer
   Ubuntu Linux

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Jens Axboe]

On Mon, Dec 19 2005, Ben Collins wrote:
> On Mon, 2005-12-19 at 20:35 +0100, Jens Axboe wrote:
> > On Mon, Dec 19 2005, Ben Collins wrote:
> > > Reference: https://bugzilla.ubuntu.com/5049
> > > 
> > > The eject command was failing for a large group of users for removable
> > > devices. The "eject -r" command, which uses the CDROMEJECT ioctl would not
> > > work, however "eject -s", which uses SG_IO did work, but required root access.
> > > 
> > > Since SG_IO was using the same mechanism as CDROMEJECT, there should be no
> > > difference. The main reason for getting the CDROMEJECT ioctl working was
> > > because it didn't need root privileges like the SG_IO commands did.
> > > 
> > > One bug was noticed, and that is CDROMEJECT was setting the blk request to a
> > > WRITE operation, when in fact it wasn't. The block layer did not like getting
> > > WRITE requests when data_len==0 and data==NULL.
> > 
> > False, it can't be a write request if there's no data attached. Write is
> > simply used there because read requests are usually more precious.
> 
> Did you mean "can be a write request"? If not, then you just repeated
> what I said.

No, you are misreading me. Definition:

- Read request: direction bit not set, non-zero data length
- Write request: direction bit set, non-zero data length

How can you read or write anything, when there's nothing to read or
write?

> > > This patch fixes the WRITE vs READ issue, and also sends the extra two
> > > commands. Anyone with an iPod connected via USB (not sure about firewire)
> > > should be able to reproduce this issue, and verify the patch.
> > 
> > The bug was in the SCSI layer, and James already has the fix integrated
> > for that. It really should make 2.6.15, James are you sending it upwards
> > for that?
> 
> Can you point me to this fix? Also, does the "fix" fix the case for IDE
> CDROM's too?

What is the problem case for for ide-cd?

The SCSI fix is here:

http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a8c730e85e80734412f4f73ab28496a0e8b04a7b

and followed up by a fix for James to cater to all paths:

http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c9526497cf03ee775c3a6f8ba62335735f98de7a

> > >  		case CDROMEJECT:
> > > -			rq = blk_get_request(q, WRITE, __GFP_WAIT);
> > > -			rq->flags |= REQ_BLOCK_PC;
> > > -			rq->data = NULL;
> > > -			rq->data_len = 0;
> > > -			rq->timeout = BLK_DEFAULT_TIMEOUT;
> > > -			memset(rq->cmd, 0, sizeof(rq->cmd));
> > > -			rq->cmd[0] = GPCMD_START_STOP_UNIT;
> > > -			rq->cmd[4] = 0x02 + (close != 0);
> > > -			rq->cmd_len = 6;
> > > -			err = blk_execute_rq(q, bd_disk, rq, 0);
> > > -			blk_put_request(rq);
> > > +			err = 0;
> > > +
> > > +			err |= blk_send_allow_medium_removal(q, bd_disk);
> > > +			err |= blk_send_start_stop(q, bd_disk, 0x01);
> > > +			err |= blk_send_start_stop(q, bd_disk, 0x02);
> > 
> > Do this in the eject tool, if it's required for some devices.
> 
> It already is in eject tool, but as described, that requires root
> access. Not something I want to force a user to do in order to eject
> their CDROM/iPod/USBStick in gnome. What exactly is wrong with the
> commands? If they are harmless for devices that don't need it, and fix a
> huge number of problems (did you see the Cc list on the bug report?) for
> users with affected devices, then what's the harm?

So the medium removal command does require write permission on the
deviec, but it doesn't require root. If they need to rw to the device
fs, surely they need write permissions on the device in the first place?

-- 
Jens Axboe

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Ben Collins]

On Mon, 2005-12-19 at 20:56 +0100, Jens Axboe wrote:
> On Mon, Dec 19 2005, Ben Collins wrote:
> > On Mon, 2005-12-19 at 20:35 +0100, Jens Axboe wrote:
> > > On Mon, Dec 19 2005, Ben Collins wrote:
> > > > Reference: https://bugzilla.ubuntu.com/5049
> > > > 
> > > > The eject command was failing for a large group of users for removable
> > > > devices. The "eject -r" command, which uses the CDROMEJECT ioctl would not
> > > > work, however "eject -s", which uses SG_IO did work, but required root access.
> > > > 
> > > > Since SG_IO was using the same mechanism as CDROMEJECT, there should be no
> > > > difference. The main reason for getting the CDROMEJECT ioctl working was
> > > > because it didn't need root privileges like the SG_IO commands did.
> > > > 
> > > > One bug was noticed, and that is CDROMEJECT was setting the blk request to a
> > > > WRITE operation, when in fact it wasn't. The block layer did not like getting
> > > > WRITE requests when data_len==0 and data==NULL.
> > > 
> > > False, it can't be a write request if there's no data attached. Write is
> > > simply used there because read requests are usually more precious.
> > 
> > Did you mean "can be a write request"? If not, then you just repeated
> > what I said.
> 
> No, you are misreading me. Definition:
> 
> - Read request: direction bit not set, non-zero data length
> - Write request: direction bit set, non-zero data length
> 
> How can you read or write anything, when there's nothing to read or
> write?

Exactly, but the block layer (and I'd have to trace back through the
code to find the exact failure point) has a check somewhere in there
that checks the direction, and if it's a WRITE, checks data_len, and
fails if it is 0.

Note, my patch does not fix the case for the bug if I leave it as WRITE.
It only works when it is READ.

> > > > This patch fixes the WRITE vs READ issue, and also sends the extra two
> > > > commands. Anyone with an iPod connected via USB (not sure about firewire)
> > > > should be able to reproduce this issue, and verify the patch.
> > > 
> > > The bug was in the SCSI layer, and James already has the fix integrated
> > > for that. It really should make 2.6.15, James are you sending it upwards
> > > for that?
> > 
> > Can you point me to this fix? Also, does the "fix" fix the case for IDE
> > CDROM's too?
> 
> What is the problem case for for ide-cd?

Yes, I know these patches. I was in the Cc when they were all being
discussed. It's has nothing to do with my patch. It's the commands that
are required. It's not about the bug in the SCSI layer.

> The SCSI fix is here:
> 
> http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a8c730e85e80734412f4f73ab28496a0e8b04a7b
> 
> and followed up by a fix for James to cater to all paths:
> 
> http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c9526497cf03ee775c3a6f8ba62335735f98de7a
> 
> > > >  		case CDROMEJECT:
> > > > -			rq = blk_get_request(q, WRITE, __GFP_WAIT);
> > > > -			rq->flags |= REQ_BLOCK_PC;
> > > > -			rq->data = NULL;
> > > > -			rq->data_len = 0;
> > > > -			rq->timeout = BLK_DEFAULT_TIMEOUT;
> > > > -			memset(rq->cmd, 0, sizeof(rq->cmd));
> > > > -			rq->cmd[0] = GPCMD_START_STOP_UNIT;
> > > > -			rq->cmd[4] = 0x02 + (close != 0);
> > > > -			rq->cmd_len = 6;
> > > > -			err = blk_execute_rq(q, bd_disk, rq, 0);
> > > > -			blk_put_request(rq);
> > > > +			err = 0;
> > > > +
> > > > +			err |= blk_send_allow_medium_removal(q, bd_disk);
> > > > +			err |= blk_send_start_stop(q, bd_disk, 0x01);
> > > > +			err |= blk_send_start_stop(q, bd_disk, 0x02);
> > > 
> > > Do this in the eject tool, if it's required for some devices.
> > 
> > It already is in eject tool, but as described, that requires root
> > access. Not something I want to force a user to do in order to eject
> > their CDROM/iPod/USBStick in gnome. What exactly is wrong with the
> > commands? If they are harmless for devices that don't need it, and fix a
> > huge number of problems (did you see the Cc list on the bug report?) for
> > users with affected devices, then what's the harm?
> 
> So the medium removal command does require write permission on the
> deviec, but it doesn't require root. If they need to rw to the device
> fs, surely they need write permissions on the device in the first place?

bcollins@colorless:~$ id -a
uid=1000(bcollins) gid=1000(bcollins)
groups=4(adm),20(dialout),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev),104(lpadmin),105(scanner),106(admin),1000(bcollins)
bcollins@colorless:~$ ls -l /dev/hdc
brw-rw---- 1 root plugdev 22, 0 Dec 19  2005 /dev/hdc
bcollins@colorless:~$ eject -s /dev/hdc
eject: unable to eject, last error: Operation not permitted
bcollins@colorless:~$ eject -r /dev/hdc
bcollins@colorless:~$

Write permissions is not enough.

-- 
   Ben Collins <ben.collins@ubuntu.com>
   Developer
   Ubuntu Linux

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Jens Axboe]

On Mon, Dec 19 2005, Ben Collins wrote:
> > - Read request: direction bit not set, non-zero data length
> > - Write request: direction bit set, non-zero data length
> > 
> > How can you read or write anything, when there's nothing to read or
> > write?
> 
> Exactly, but the block layer (and I'd have to trace back through the
> code to find the exact failure point) has a check somewhere in there
> that checks the direction, and if it's a WRITE, checks data_len, and
> fails if it is 0.

I'd like to see that, if it's true it wants fixing of course.

> Note, my patch does not fix the case for the bug if I leave it as WRITE.
> It only works when it is READ.

We need to get that fixed as well, it should not make a difference.

> > So the medium removal command does require write permission on the
> > deviec, but it doesn't require root. If they need to rw to the device
> > fs, surely they need write permissions on the device in the first place?
> 
> bcollins@colorless:~$ id -a
> uid=1000(bcollins) gid=1000(bcollins)
> groups=4(adm),20(dialout),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev),104(lpadmin),105(scanner),106(admin),1000(bcollins)
> bcollins@colorless:~$ ls -l /dev/hdc
> brw-rw---- 1 root plugdev 22, 0 Dec 19  2005 /dev/hdc
> bcollins@colorless:~$ eject -s /dev/hdc
> eject: unable to eject, last error: Operation not permitted
> bcollins@colorless:~$ eject -r /dev/hdc

Does eject open the device O_RDWR?

-- 
Jens Axboe

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Ben Collins]

On Mon, 2005-12-19 at 21:46 +0100, Jens Axboe wrote:
> On Mon, Dec 19 2005, Ben Collins wrote:
> > > So the medium removal command does require write permission on the
> > > deviec, but it doesn't require root. If they need to rw to the device
> > > fs, surely they need write permissions on the device in the first place?
> > 
> > bcollins@colorless:~$ id -a
> > uid=1000(bcollins) gid=1000(bcollins)
> > groups=4(adm),20(dialout),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev),104(lpadmin),105(scanner),106(admin),1000(bcollins)
> > bcollins@colorless:~$ ls -l /dev/hdc
> > brw-rw---- 1 root plugdev 22, 0 Dec 19  2005 /dev/hdc
> > bcollins@colorless:~$ eject -s /dev/hdc
> > eject: unable to eject, last error: Operation not permitted
> > bcollins@colorless:~$ eject -r /dev/hdc
> 
> Does eject open the device O_RDWR?

No, it was opening it RDONLY. I changed it to RDWR, and it allowed the
SG_IO commands to work without root. I'll send a patch to eject for
that.

-- 
   Ben Collins <ben.collins@ubuntu.com>
   Developer
   Ubuntu Linux

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Linus Torvalds]

On Mon, 19 Dec 2005, Ben Collins wrote:
> 
> > >  		case CDROMEJECT:
> > > -			rq = blk_get_request(q, WRITE, __GFP_WAIT);
> > > -			rq->flags |= REQ_BLOCK_PC;
> > > -			rq->data = NULL;
> > > -			rq->data_len = 0;
> > > -			rq->timeout = BLK_DEFAULT_TIMEOUT;
> > > -			memset(rq->cmd, 0, sizeof(rq->cmd));
> > > -			rq->cmd[0] = GPCMD_START_STOP_UNIT;
> > > -			rq->cmd[4] = 0x02 + (close != 0);
> > > -			rq->cmd_len = 6;
> > > -			err = blk_execute_rq(q, bd_disk, rq, 0);
> > > -			blk_put_request(rq);
> > > +			err = 0;
> > > +
> > > +			err |= blk_send_allow_medium_removal(q, bd_disk);
> > > +			err |= blk_send_start_stop(q, bd_disk, 0x01);
> > > +			err |= blk_send_start_stop(q, bd_disk, 0x02);
> > 
> > Do this in the eject tool, if it's required for some devices.
> 
> It already is in eject tool, but as described, that requires root
> access. Not something I want to force a user to do in order to eject
> their CDROM/iPod/USBStick in gnome. What exactly is wrong with the
> commands? If they are harmless for devices that don't need it, and fix a
> huge number of problems (did you see the Cc list on the bug report?) for
> users with affected devices, then what's the harm?

I do agree that the suggested patch seems to be a real cleanup, regardless 
of whether the original code bug has now been fixed or not.

Are there devices that really want the old sequence? 

Also, do we really need to send fist a start_stop 1 and then a 2?

Wouldn't the _logical_ thing be to replace the old code with just a 
cleaned-up-version of what the old code did, ie just do

	err = blk_send_start_stop(q, bd_disk, 0x02);

for the eject case? That way we could do the patch as a pure cleanup, and 
then a separate patch might change the singe "start_stop 2" with the more 
complex sequence.

(IOW, I'd prefer to separate out the cleanup from the "make the eject 
sequence more complete" part).

		Linus

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Jens Axboe]

On Mon, Dec 19 2005, Linus Torvalds wrote:
> 
> 
> On Mon, 19 Dec 2005, Ben Collins wrote:
> > 
> > > >  		case CDROMEJECT:
> > > > -			rq = blk_get_request(q, WRITE, __GFP_WAIT);
> > > > -			rq->flags |= REQ_BLOCK_PC;
> > > > -			rq->data = NULL;
> > > > -			rq->data_len = 0;
> > > > -			rq->timeout = BLK_DEFAULT_TIMEOUT;
> > > > -			memset(rq->cmd, 0, sizeof(rq->cmd));
> > > > -			rq->cmd[0] = GPCMD_START_STOP_UNIT;
> > > > -			rq->cmd[4] = 0x02 + (close != 0);
> > > > -			rq->cmd_len = 6;
> > > > -			err = blk_execute_rq(q, bd_disk, rq, 0);
> > > > -			blk_put_request(rq);
> > > > +			err = 0;
> > > > +
> > > > +			err |= blk_send_allow_medium_removal(q, bd_disk);
> > > > +			err |= blk_send_start_stop(q, bd_disk, 0x01);
> > > > +			err |= blk_send_start_stop(q, bd_disk, 0x02);
> > > 
> > > Do this in the eject tool, if it's required for some devices.
> > 
> > It already is in eject tool, but as described, that requires root
> > access. Not something I want to force a user to do in order to eject
> > their CDROM/iPod/USBStick in gnome. What exactly is wrong with the
> > commands? If they are harmless for devices that don't need it, and fix a
> > huge number of problems (did you see the Cc list on the bug report?) for
> > users with affected devices, then what's the harm?
> 
> I do agree that the suggested patch seems to be a real cleanup, regardless 
> of whether the original code bug has now been fixed or not.

Apparently two seperate issues.

> 
> Are there devices that really want the old sequence? 
> 
> Also, do we really need to send fist a start_stop 1 and then a 2?

The 0x01 looks really suspicious to me, it should just cause extra wait
and activity on most devices.

> Wouldn't the _logical_ thing be to replace the old code with just a 
> cleaned-up-version of what the old code did, ie just do
> 
> 	err = blk_send_start_stop(q, bd_disk, 0x02);
> 
> for the eject case? That way we could do the patch as a pure cleanup, and 
> then a separate patch might change the singe "start_stop 2" with the more 
> complex sequence.

That would work.

-- 
Jens Axboe

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Ben Collins]

On Mon, 2005-12-19 at 12:02 -0800, Linus Torvalds wrote:

> for the eject case? That way we could do the patch as a pure cleanup, and 
> then a separate patch might change the singe "start_stop 2" with the more 
> complex sequence.
> 
> (IOW, I'd prefer to separate out the cleanup from the "make the eject 
> sequence more complete" part).

Sure thing.

-- 
   Ben Collins <ben.collins@ubuntu.com>
   Developer
   Ubuntu Linux

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Ben Collins]

On Mon, 2005-12-19 at 20:35 +0100, Jens Axboe wrote:
> On Mon, Dec 19 2005, Ben Collins wrote:
> > This patch fixes the WRITE vs READ issue, and also sends the extra two
> > commands. Anyone with an iPod connected via USB (not sure about firewire)
> > should be able to reproduce this issue, and verify the patch.
> 
> The bug was in the SCSI layer, and James already has the fix integrated
> for that. It really should make 2.6.15, James are you sending it upwards
> for that?

You mean this patch?

James Bottomley:
      [SCSI] Consolidate REQ_BLOCK_PC handling path (fix ipod panic)

This fixes an oops with data direction because sbp2 was not checking
enough itself.

I seriously doubt this will fix the issue being reported. Changing the
blk request to a READ did not fix the problem. The problem was only
fixed by sending the extra two commands. The direction was just a side
issue.

Is there a problem with sending the commands? If they don't bother
unaffected devices, but it does fix a large number of other devices,
what's the problem?

-- 
   Ben Collins <ben.collins@ubuntu.com>
   Developer
   Ubuntu Linux

Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust

[Jens Axboe]

On Mon, Dec 19 2005, Ben Collins wrote:
> On Mon, 2005-12-19 at 20:35 +0100, Jens Axboe wrote:
> > On Mon, Dec 19 2005, Ben Collins wrote:
> > > This patch fixes the WRITE vs READ issue, and also sends the extra two
> > > commands. Anyone with an iPod connected via USB (not sure about firewire)
> > > should be able to reproduce this issue, and verify the patch.
> > 
> > The bug was in the SCSI layer, and James already has the fix integrated
> > for that. It really should make 2.6.15, James are you sending it upwards
> > for that?
> 
> You mean this patch?
> 
> James Bottomley:
>       [SCSI] Consolidate REQ_BLOCK_PC handling path (fix ipod panic)
> 
> This fixes an oops with data direction because sbp2 was not checking
> enough itself.
> 
> I seriously doubt this will fix the issue being reported. Changing the
> blk request to a READ did not fix the problem. The problem was only
> fixed by sending the extra two commands. The direction was just a side
> issue.

Your report surely made it seem like an issue, hence I pointed you at
the fix(es). What part of the block layer didn't like these commands?

> Is there a problem with sending the commands? If they don't bother
> unaffected devices, but it does fix a large number of other devices,
> what's the problem?

It's not necessarily a problem, especially if other paths end up doing
it in some way or another. Hard to say whether it bothers other device,
but I agree it should not. Please resend after 2.6.15 is released (and
do make those macros functions, thanks).

-- 
Jens Axboe