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
-
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/