Re: [PATCH] fix cdrom mt rainier probe

Re: [PATCH] fix cdrom mt rainier probe

[Pat LaVarre]

> > Mt rainier probe must be deferred to media load time, ...
> ...
> something broke in deciding disc rewritable or not? ...
> Only for /dev/hdd PATAPI, not for /dev/scd0 USB?

Can any of us easily sketch a legit fix for this?  By now we know,

a)

The key evil is not getting back 0 0 in reply to the query:

$ sudo blockdev --getro /dev/hdd /dev/scd0
1
0
$

b)

A completely bogus hack of a workaround is:

sudo blockdev --setrw /dev/hdd

c)

Below is that same completely bogus hack, expressed as a kernel patch
Not suitable for kernel.org.

Pat LaVarre
http://linux-pel.blog-city.com/read/728344.htm

--- linux-2.6.8-rc1/drivers/ide/ide-cd.c	2004-07-13 08:26:05.000000000 -0600
+++ linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c	2004-07-13 14:33:40.000000000 -0600
@@ -3251,6 +3251,7 @@ int ide_cdrom_setup (ide_drive_t *drive)
 	 * set correct block size and read-only for non-ram media
 	 */
 	set_disk_ro(drive->disk, !CDROM_CONFIG_FLAGS(drive)->ram);
+	set_disk_ro(drive->disk, 0);
 	blk_queue_hardsect_size(drive->queue, CD_FRAMESIZE);
 
 #if 0

Re: [PATCH] fix cdrom mt rainier probe

[Jens Axboe]

On Tue, Jul 13 2004, Pat LaVarre wrote:
> > > Mt rainier probe must be deferred to media load time, ...
> > ...
> > something broke in deciding disc rewritable or not? ...
> > Only for /dev/hdd PATAPI, not for /dev/scd0 USB?
> 
> Can any of us easily sketch a legit fix for this?  By now we know,
> 
> a)
> 
> The key evil is not getting back 0 0 in reply to the query:
> 
> $ sudo blockdev --getro /dev/hdd /dev/scd0
> 1
> 0
> $
> 
> b)
> 
> A completely bogus hack of a workaround is:
> 
> sudo blockdev --setrw /dev/hdd
> 
> c)
> 
> Below is that same completely bogus hack, expressed as a kernel patch
> Not suitable for kernel.org.
> 
> Pat LaVarre
> http://linux-pel.blog-city.com/read/728344.htm
> 
> --- linux-2.6.8-rc1/drivers/ide/ide-cd.c	2004-07-13 08:26:05.000000000 -0600
> +++ linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c	2004-07-13 14:33:40.000000000 -0600
> @@ -3251,6 +3251,7 @@ int ide_cdrom_setup (ide_drive_t *drive)
>  	 * set correct block size and read-only for non-ram media
>  	 */
>  	set_disk_ro(drive->disk, !CDROM_CONFIG_FLAGS(drive)->ram);
> +	set_disk_ro(drive->disk, 0);

Trace that backwards and find out why ->ram isn't getting set.

-- 
Jens Axboe

Re: [PATCH] fix cdrom mt rainier probe

[Pat LaVarre]

Jens A:

> > --- linux-2.6.8-rc1/drivers/ide/ide-cd.c      2004-07-13 08:26:05.000000000 -0600
> > +++ linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c  2004-07-13 14:33:40.000000000 -0600
> > @@ -3251,6 +3251,7 @@ int ide_cdrom_setup (ide_drive_t *drive)
> >        * set correct block size and read-only for non-ram media
> >        */
> >       set_disk_ro(drive->disk, !CDROM_CONFIG_FLAGS(drive)->ram);
> > +     set_disk_ro(drive->disk, 0);
> 
> Trace that backwards and find out why ->ram isn't getting set.

An answer to that helpfully concrete question is:

2.6.8-rc1 differs from 2.6.7 by lacking the three-line patch below.

Back in the ide_cdrom_probe_capabilities of 2.6.7, we were saying any
drive that offered "feature" x0020 "Random Writable" was set_disk_ro
rewritable.  Now in 2.6.8-rc1 we accept only:

        cap.dvd_ram_write || (drive->media == ide_optical)

That criterion denies the rewritability of the drive I'm testing i.e.
the "Iomega" "RRD" drive.

I'd like Linux to perceive the reality that RRD discs are as rewritable
as DVD RAM discs are - can I help further?

Pat LaVarre

--- linux-2.6.8-rc1/drivers/ide/ide-cd.c	2004-07-13 08:26:05.000000000 -0600
+++ linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c	2004-07-14 17:00:28.000000000 -0600
@@ -2925,7 +2925,7 @@ int ide_cdrom_probe_capabilities (ide_dr
 	struct cdrom_info *info = drive->driver_data;
 	struct cdrom_device_info *cdi = &info->devinfo;
 	struct atapi_capabilities_page cap;
-	int nslots = 1;
+	int nslots = 1, ram_write = 0;
 
 	if (drive->media == ide_optical) {
 		CDROM_CONFIG_FLAGS(drive)->mo_drive = 1;
@@ -2954,6 +2954,10 @@ int ide_cdrom_probe_capabilities (ide_dr
 	if (ide_cdrom_get_capabilities(drive, &cap))
 		return 0;
 
+	if (!cdrom_is_random_writable(cdi, &ram_write))
+		if (ram_write)
+			CDROM_CONFIG_FLAGS(drive)->ram = 1;
+
 	if (cap.lock == 0)
 		CDROM_CONFIG_FLAGS(drive)->no_doorlock = 1;
 	if (cap.eject)

Re: [PATCH] fix cdrom mt rainier probe

[Pat LaVarre]

Jens A:

Annihilating the calls to set_disk_ro from scsi/sr.c and ide/ide-cd.c
withdraws blanket write protection from all USB/ PATAPI etc. DVD/ CD
devices, and instead gives to all DVD/ CD only the late, open-time,
distinction between rewritable and read-only discs.

For example, with this patch applied, I see:

$ sudo blockdev --getro /dev/hdd /dev/scd0
0
0
$
$ sudo dd of=/dev/hdd bs=64K seek=0 count=1 if=/dev/zero
dd: opening `/dev/hdd': Read-only file system
$
$ # eject read-only disc, insert rewritable disc
$
$ sudo dd of=/dev/hdd bs=64K seek=0 count=1 if=/dev/zero
1+0 records in
1+0 records out
$

As expected, I saw exactly the same results with USB /dev/scd0 as with
the PATAPI /dev/hdd shown above.

Do you like this behaviour better than what we have now in 2.6.8-rc1?

(I do, because USB and PATAPI come into agreement with each other, and
because Iomega RRD discs correctly become rewritable by default.)

This idea, kindly suggested to me offline, is enough simpler than
learning to call set_disk_ro after each disc change that I am able to
propose the specific patch quoted inline below.

In this patch you will see four parts, to give you two choices I thought
you might like to consider.

1) The first part changes cdrom_open to check CDC_RAM earlier in an
FMODE_WRITE open i.e. before open_for_data.  I'm asking you to choose to
check earlier or not.

2) The second part draws your attention to a set_disk_ro I actually did
not delete.  That remaining set_disk_ro dynamically forces ro true in
response to particular write errors.  The /* != scsi/sr.c */ text of the
comment I used to draw you attention to that source line means to say
that this is an asymmetry between ide/ide-cd.c and scsi/sr.c, in that
only ide/ide-cd.c works this hard.  I'm asking you to choose to delete
that call to set_disk_ro, to comment on its asymmetry, or to leave it
unchanged.

3, 4) The last two parts of the patch merely delete the once-per-plugin
calls to set_disk_ro.

Pat LaVarre

diff -urp linux-2.6.8-rc1/drivers/cdrom/cdrom.c linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c
--- linux-2.6.8-rc1/drivers/cdrom/cdrom.c	2004-07-13 08:26:02.000000000 -0600
+++ linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c	2004-07-15 18:21:48.564652904 -0600
@@ -892,13 +892,16 @@ int cdrom_open(struct cdrom_device_info 
 	if ((fp->f_flags & O_NONBLOCK) && (cdi->options & CDO_USE_FFLAGS)) {
 		ret = cdi->ops->open(cdi, 1);
 	} else {
+		if (fp->f_mode & FMODE_WRITE) {
+			ret = -EROFS;
+			if (!CDROM_CAN(CDC_RAM))
+				goto err;
+		}
 		ret = open_for_data(cdi);
 		if (ret)
 			goto err;
 		if (fp->f_mode & FMODE_WRITE) {
 			ret = -EROFS;
-			if (!CDROM_CAN(CDC_RAM))
-				goto err;
 			if (cdrom_open_write(cdi))
 				goto err;
 			ret = 0;
diff -urp linux-2.6.8-rc1/drivers/ide/ide-cd.c linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c
--- linux-2.6.8-rc1/drivers/ide/ide-cd.c	2004-07-13 08:26:05.000000000 -0600
+++ linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c	2004-07-15 14:38:23.000000000 -0600
@@ -790,7 +790,7 @@ static int cdrom_decode_status(ide_drive
 			 */
 			if (rq_data_dir(rq) == WRITE) {
 				printk("ide-cd: media marked write protected\n");
-				set_disk_ro(drive->disk, 1);
+				set_disk_ro(drive->disk, 1); /* != scsi/sr.c */
 			}
 
 			/* No point in retrying after an illegal
@@ -3248,9 +3248,8 @@ int ide_cdrom_setup (ide_drive_t *drive)
 	nslots = ide_cdrom_probe_capabilities (drive);
 
 	/*
-	 * set correct block size and read-only for non-ram media
+	 * set correct block size
 	 */
-	set_disk_ro(drive->disk, !CDROM_CONFIG_FLAGS(drive)->ram);
 	blk_queue_hardsect_size(drive->queue, CD_FRAMESIZE);
 
 #if 0
diff -urp linux-2.6.8-rc1/drivers/scsi/sr.c linux-2.6.8-rc1-pel/drivers/scsi/sr.c
--- linux-2.6.8-rc1/drivers/scsi/sr.c	2004-07-13 08:26:16.000000000 -0600
+++ linux-2.6.8-rc1-pel/drivers/scsi/sr.c	2004-07-15 14:29:34.000000000 -0600
@@ -775,9 +775,6 @@ static void get_capabilities(struct scsi
 		""
 	};
 
-	/* Set read only initially */
-	set_disk_ro(cd->disk, 1);
-
 	/* allocate a request for the TEST_UNIT_READY */
 	SRpnt = scsi_allocate_request(cd->device, GFP_KERNEL);
 	if (!SRpnt) {
@@ -885,7 +882,6 @@ static void get_capabilities(struct scsi
 	if ((cd->cdi.mask & (CDC_DVD_RAM | CDC_MRW_W | CDC_RAM)) !=
 			(CDC_DVD_RAM | CDC_MRW_W | CDC_RAM)) {
 		cd->device->writeable = 1;
-		set_disk_ro(cd->disk, 0);
 	}
 
 	scsi_release_request(SRpnt);

Re: [PATCH] fix cdrom mt rainier probe

[Jens Axboe]

On Thu, Jul 15 2004, Pat LaVarre wrote:
> 1) The first part changes cdrom_open to check CDC_RAM earlier in an
> FMODE_WRITE open i.e. before open_for_data.  I'm asking you to choose to
> check earlier or not.
> 
> 2) The second part draws your attention to a set_disk_ro I actually did
> not delete.  That remaining set_disk_ro dynamically forces ro true in
> response to particular write errors.  The /* != scsi/sr.c */ text of the
> comment I used to draw you attention to that source line means to say
> that this is an asymmetry between ide/ide-cd.c and scsi/sr.c, in that
> only ide/ide-cd.c works this hard.  I'm asking you to choose to delete
> that call to set_disk_ro, to comment on its asymmetry, or to leave it
> unchanged.
> 
> 3, 4) The last two parts of the patch merely delete the once-per-plugin
> calls to set_disk_ro.
> 
> Pat LaVarre
> 
> diff -urp linux-2.6.8-rc1/drivers/cdrom/cdrom.c linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c
> --- linux-2.6.8-rc1/drivers/cdrom/cdrom.c	2004-07-13 08:26:02.000000000 -0600
> +++ linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c	2004-07-15 18:21:48.564652904 -0600
> @@ -892,13 +892,16 @@ int cdrom_open(struct cdrom_device_info 
>  	if ((fp->f_flags & O_NONBLOCK) && (cdi->options & CDO_USE_FFLAGS)) {
>  		ret = cdi->ops->open(cdi, 1);
>  	} else {
> +		if (fp->f_mode & FMODE_WRITE) {
> +			ret = -EROFS;
> +			if (!CDROM_CAN(CDC_RAM))
> +				goto err;
> +		}
>  		ret = open_for_data(cdi);
>  		if (ret)
>  			goto err;
>  		if (fp->f_mode & FMODE_WRITE) {
>  			ret = -EROFS;
> -			if (!CDROM_CAN(CDC_RAM))
> -				goto err;
>  			if (cdrom_open_write(cdi))
>  				goto err;
>  			ret = 0;

This looks strange - cdrom_open_write() is the one that checks whether
the media is suitable for writing or not. In fact, it looks as if just
transposing the two checks should be fine:

                if (fp->f_mode & FMODE_WRITE) {
                        ret = -EROFS;
                        if (cdrom_open_write(cdi))
                                goto err;
                        if (!CDROM_CAN(CDC_RAM))
                                goto err;
                        ret = 0;
                }

> diff -urp linux-2.6.8-rc1/drivers/ide/ide-cd.c linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c
> --- linux-2.6.8-rc1/drivers/ide/ide-cd.c	2004-07-13 08:26:05.000000000 -0600
> +++ linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c	2004-07-15 14:38:23.000000000 -0600
> @@ -790,7 +790,7 @@ static int cdrom_decode_status(ide_drive
>  			 */
>  			if (rq_data_dir(rq) == WRITE) {
>  				printk("ide-cd: media marked write protected\n");
> -				set_disk_ro(drive->disk, 1);
> +				set_disk_ro(drive->disk, 1); /* != scsi/sr.c */
>  			}

This needs to go as well, unless you mark it writable again after close
of this media.

>  			/* No point in retrying after an illegal
> @@ -3248,9 +3248,8 @@ int ide_cdrom_setup (ide_drive_t *drive)
>  	nslots = ide_cdrom_probe_capabilities (drive);
>  
>  	/*
> -	 * set correct block size and read-only for non-ram media
> +	 * set correct block size
>  	 */
> -	set_disk_ro(drive->disk, !CDROM_CONFIG_FLAGS(drive)->ram);
>  	blk_queue_hardsect_size(drive->queue, CD_FRAMESIZE);
>  
>  #if 0
> diff -urp linux-2.6.8-rc1/drivers/scsi/sr.c linux-2.6.8-rc1-pel/drivers/scsi/sr.c
> --- linux-2.6.8-rc1/drivers/scsi/sr.c	2004-07-13 08:26:16.000000000 -0600
> +++ linux-2.6.8-rc1-pel/drivers/scsi/sr.c	2004-07-15 14:29:34.000000000 -0600
> @@ -775,9 +775,6 @@ static void get_capabilities(struct scsi
>  		""
>  	};
>  
> -	/* Set read only initially */
> -	set_disk_ro(cd->disk, 1);
> -
>  	/* allocate a request for the TEST_UNIT_READY */
>  	SRpnt = scsi_allocate_request(cd->device, GFP_KERNEL);
>  	if (!SRpnt) {
> @@ -885,7 +882,6 @@ static void get_capabilities(struct scsi
>  	if ((cd->cdi.mask & (CDC_DVD_RAM | CDC_MRW_W | CDC_RAM)) !=
>  			(CDC_DVD_RAM | CDC_MRW_W | CDC_RAM)) {
>  		cd->device->writeable = 1;
> -		set_disk_ro(cd->disk, 0);
>  	}
>  
>  	scsi_release_request(SRpnt);

These are fine.

-- 
Jens Axboe

Re: [PATCH] fix cdrom mt rainier probe

[Jens Axboe]

On Fri, Jul 16 2004, Jens Axboe wrote:
> On Thu, Jul 15 2004, Pat LaVarre wrote:
> > 1) The first part changes cdrom_open to check CDC_RAM earlier in an
> > FMODE_WRITE open i.e. before open_for_data.  I'm asking you to choose to
> > check earlier or not.
> > 
> > 2) The second part draws your attention to a set_disk_ro I actually did
> > not delete.  That remaining set_disk_ro dynamically forces ro true in
> > response to particular write errors.  The /* != scsi/sr.c */ text of the
> > comment I used to draw you attention to that source line means to say
> > that this is an asymmetry between ide/ide-cd.c and scsi/sr.c, in that
> > only ide/ide-cd.c works this hard.  I'm asking you to choose to delete
> > that call to set_disk_ro, to comment on its asymmetry, or to leave it
> > unchanged.
> > 
> > 3, 4) The last two parts of the patch merely delete the once-per-plugin
> > calls to set_disk_ro.
> > 
> > Pat LaVarre
> > 
> > diff -urp linux-2.6.8-rc1/drivers/cdrom/cdrom.c linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c
> > --- linux-2.6.8-rc1/drivers/cdrom/cdrom.c	2004-07-13 08:26:02.000000000 -0600
> > +++ linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c	2004-07-15 18:21:48.564652904 -0600
> > @@ -892,13 +892,16 @@ int cdrom_open(struct cdrom_device_info 
> >  	if ((fp->f_flags & O_NONBLOCK) && (cdi->options & CDO_USE_FFLAGS)) {
> >  		ret = cdi->ops->open(cdi, 1);
> >  	} else {
> > +		if (fp->f_mode & FMODE_WRITE) {
> > +			ret = -EROFS;
> > +			if (!CDROM_CAN(CDC_RAM))
> > +				goto err;
> > +		}
> >  		ret = open_for_data(cdi);
> >  		if (ret)
> >  			goto err;
> >  		if (fp->f_mode & FMODE_WRITE) {
> >  			ret = -EROFS;
> > -			if (!CDROM_CAN(CDC_RAM))
> > -				goto err;
> >  			if (cdrom_open_write(cdi))
> >  				goto err;
> >  			ret = 0;
> 
> This looks strange - cdrom_open_write() is the one that checks whether
> the media is suitable for writing or not. In fact, it looks as if just
> transposing the two checks should be fine:
> 
>                 if (fp->f_mode & FMODE_WRITE) {
>                         ret = -EROFS;
>                         if (cdrom_open_write(cdi))
>                                 goto err;
>                         if (!CDROM_CAN(CDC_RAM))
>                                 goto err;
>                         ret = 0;
>                 }

Actually, looking at the above makes it clear that we should probably
just drop CDC_RAM again and just let cdrom_open_write() return whether
we are allowed to open the media for write or not. Now that CDC_RAM is
a per-media capability flag, it doesn't make sense to set/clear it on
every open when you can just return ok or not.

But that cleanup can be applied of this patch, so please just continue.

-- 
Jens Axboe

Re: [PATCH] fix cdrom mt rainier probe

[Pat LaVarre]

Jens A:

> cdrom_open ... transposing the two checks ...
> cdrom_decode_status ... set_disk_ro ... needs to go ... unless ...
> ide_cdrom_setup ...
> get_capabilities ... These are fine.

This e-mail ends with that four part patch you describe, thank you for
those clear instructions.

I also specifically confirmed that deleting rq_data_dir has no side
effect:

$ grep rq_data_dir include/linux/*
include/linux/blkdev.h:#define rq_data_dir(rq)          ((rq)->flags & 1)
$

> Now that CDC_RAM is a per-media capability flag, it doesn't make sense
> to set/clear it on every open when you can just return ok or not.

Agreed.

I had suggested always trying a fetch of mode page x2A "Capabilities"
before op x46 "GET CONFIGURATION" out of fear of legacy DVD/ CD choking
over the new op.

I see in the release early often spirit we can try simplifying our host
code first and waiting for someone to complain, maybe no one ever
actually will.

> > +			if (!CDROM_CAN(CDC_RAM))
> > +				goto err;
> ...
> >  			if (cdrom_open_write(cdi))
> ...
> This looks strange - 
> cdrom_open_write() is the one that checks whether 
> the media is suitable for writing or not

Agreed.

Me thinking like this is why I don't understand how CDC_RAM ever got set
in my patched PATAPI code.  But since we're abandoning CDC_RAM, I won't
analyse that mystery further, unless you tell me I should.

> Date: Fri, 16 Jul 2004 14:28:45 +0200
> Date: Fri, 16 Jul 2004 14:25:55 +0200

Ouch I was asleep then.  (: I need to route my e-mail to my phone. :)

Pat LaVarre

diff -urp linux-2.6.8-rc1/drivers/cdrom/cdrom.c linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c
--- linux-2.6.8-rc1/drivers/cdrom/cdrom.c	2004-07-13 08:26:02.000000000 -0600
+++ linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c	2004-07-16 09:40:22.765020896 -0600
@@ -897,10 +897,10 @@ int cdrom_open(struct cdrom_device_info 
 			goto err;
 		if (fp->f_mode & FMODE_WRITE) {
 			ret = -EROFS;
-			if (!CDROM_CAN(CDC_RAM))
-				goto err;
 			if (cdrom_open_write(cdi))
 				goto err;
+			if (!CDROM_CAN(CDC_RAM))
+				goto err;
 			ret = 0;
 		}
 	}
diff -urp linux-2.6.8-rc1/drivers/ide/ide-cd.c linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c
--- linux-2.6.8-rc1/drivers/ide/ide-cd.c	2004-07-13 08:26:05.000000000 -0600
+++ linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c	2004-07-16 09:37:07.613688408 -0600
@@ -785,14 +785,6 @@ static int cdrom_decode_status(ide_drive
 				do_end_request = 1;
 		} else if (sense_key == ILLEGAL_REQUEST ||
 			   sense_key == DATA_PROTECT) {
-			/*
-			 * check if this was a write protected media
-			 */
-			if (rq_data_dir(rq) == WRITE) {
-				printk("ide-cd: media marked write protected\n");
-				set_disk_ro(drive->disk, 1);
-			}
-
 			/* No point in retrying after an illegal
 			   request or data protect error.*/
 			ide_dump_status (drive, "command error", stat);
@@ -3248,9 +3240,8 @@ int ide_cdrom_setup (ide_drive_t *drive)
 	nslots = ide_cdrom_probe_capabilities (drive);
 
 	/*
-	 * set correct block size and read-only for non-ram media
+	 * set correct block size
 	 */
-	set_disk_ro(drive->disk, !CDROM_CONFIG_FLAGS(drive)->ram);
 	blk_queue_hardsect_size(drive->queue, CD_FRAMESIZE);
 
 #if 0
diff -urp linux-2.6.8-rc1/drivers/scsi/sr.c linux-2.6.8-rc1-pel/drivers/scsi/sr.c
--- linux-2.6.8-rc1/drivers/scsi/sr.c	2004-07-13 08:26:16.000000000 -0600
+++ linux-2.6.8-rc1-pel/drivers/scsi/sr.c	2004-07-15 14:29:34.000000000 -0600
@@ -775,9 +775,6 @@ static void get_capabilities(struct scsi
 		""
 	};
 
-	/* Set read only initially */
-	set_disk_ro(cd->disk, 1);
-
 	/* allocate a request for the TEST_UNIT_READY */
 	SRpnt = scsi_allocate_request(cd->device, GFP_KERNEL);
 	if (!SRpnt) {
@@ -885,7 +882,6 @@ static void get_capabilities(struct scsi
 	if ((cd->cdi.mask & (CDC_DVD_RAM | CDC_MRW_W | CDC_RAM)) !=
 			(CDC_DVD_RAM | CDC_MRW_W | CDC_RAM)) {
 		cd->device->writeable = 1;
-		set_disk_ro(cd->disk, 0);
 	}
 
 	scsi_release_request(SRpnt);

Re: [PATCH] fix cdrom mt rainier probe

[Jens Axboe]

On Fri, Jul 16 2004, Pat LaVarre wrote:
> diff -urp linux-2.6.8-rc1/drivers/cdrom/cdrom.c linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c
> --- linux-2.6.8-rc1/drivers/cdrom/cdrom.c	2004-07-13 08:26:02.000000000 -0600
> +++ linux-2.6.8-rc1-pel/drivers/cdrom/cdrom.c	2004-07-16 09:40:22.765020896 -0600
> @@ -897,10 +897,10 @@ int cdrom_open(struct cdrom_device_info 
>  			goto err;
>  		if (fp->f_mode & FMODE_WRITE) {
>  			ret = -EROFS;
> -			if (!CDROM_CAN(CDC_RAM))
> -				goto err;
>  			if (cdrom_open_write(cdi))
>  				goto err;
> +			if (!CDROM_CAN(CDC_RAM))
> +				goto err;
>  			ret = 0;
>  		}
>  	}
> diff -urp linux-2.6.8-rc1/drivers/ide/ide-cd.c linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c
> --- linux-2.6.8-rc1/drivers/ide/ide-cd.c	2004-07-13 08:26:05.000000000 -0600
> +++ linux-2.6.8-rc1-pel/drivers/ide/ide-cd.c	2004-07-16 09:37:07.613688408 -0600
> @@ -785,14 +785,6 @@ static int cdrom_decode_status(ide_drive
>  				do_end_request = 1;
>  		} else if (sense_key == ILLEGAL_REQUEST ||
>  			   sense_key == DATA_PROTECT) {
> -			/*
> -			 * check if this was a write protected media
> -			 */
> -			if (rq_data_dir(rq) == WRITE) {
> -				printk("ide-cd: media marked write protected\n");
> -				set_disk_ro(drive->disk, 1);
> -			}
> -
>  			/* No point in retrying after an illegal
>  			   request or data protect error.*/
>  			ide_dump_status (drive, "command error", stat);
> @@ -3248,9 +3240,8 @@ int ide_cdrom_setup (ide_drive_t *drive)
>  	nslots = ide_cdrom_probe_capabilities (drive);
>  
>  	/*
> -	 * set correct block size and read-only for non-ram media
> +	 * set correct block size
>  	 */
> -	set_disk_ro(drive->disk, !CDROM_CONFIG_FLAGS(drive)->ram);
>  	blk_queue_hardsect_size(drive->queue, CD_FRAMESIZE);
>  
>  #if 0
> diff -urp linux-2.6.8-rc1/drivers/scsi/sr.c linux-2.6.8-rc1-pel/drivers/scsi/sr.c
> --- linux-2.6.8-rc1/drivers/scsi/sr.c	2004-07-13 08:26:16.000000000 -0600
> +++ linux-2.6.8-rc1-pel/drivers/scsi/sr.c	2004-07-15 14:29:34.000000000 -0600
> @@ -775,9 +775,6 @@ static void get_capabilities(struct scsi
>  		""
>  	};
>  
> -	/* Set read only initially */
> -	set_disk_ro(cd->disk, 1);
> -
>  	/* allocate a request for the TEST_UNIT_READY */
>  	SRpnt = scsi_allocate_request(cd->device, GFP_KERNEL);
>  	if (!SRpnt) {
> @@ -885,7 +882,6 @@ static void get_capabilities(struct scsi
>  	if ((cd->cdi.mask & (CDC_DVD_RAM | CDC_MRW_W | CDC_RAM)) !=
>  			(CDC_DVD_RAM | CDC_MRW_W | CDC_RAM)) {
>  		cd->device->writeable = 1;
> -		set_disk_ro(cd->disk, 0);
>  	}
>  
>  	scsi_release_request(SRpnt);
> 
> 
> 

This looks perfect! Have you tested it works as expected as well, with
rw and ro media?

-- 
Jens Axboe

Re: [PATCH] fix cdrom mt rainier probe

[Pat LaVarre]

Jens A:

> Have you tested it works as expected as well, with
> rw and ro media?

Ouch, yes, please, never assume I've tested unless I say so explicitly.

So far I've just seen the expected, with one rewritable and one
read-only disc, in a pair of USB and PATAPI drives:

$ sudo plscsi /dev/scd0 /dev/hdd
export PLSCSI=/dev/scd0       // B Iomega   RRD              DB76
export PLSCSI=/dev/hdd        // @ Iomega   RRD              74.B
$
$ sudo blockdev --getro /dev/scd0 /dev/hdd
0
0
$
$ sudo dd of=/dev/scd0 bs=64K seek=0 count=1 if=/dev/zero
dd: opening `/dev/scd0': Read-only file system
$ sudo dd of=/dev/hdd bs=64K seek=0 count=1 if=/dev/zero
1+0 records in
1+0 records out
$
$ # swap discs
$
$ sudo dd of=/dev/scd0 bs=64K seek=0 count=1 if=/dev/zero
1+0 records in
1+0 records out
$ sudo dd of=/dev/hdd bs=64K seek=0 count=1 if=/dev/zero
dd: opening `/dev/hdd': Read-only file system
$

> looks perfect!

Excellent, I conclude I have understood correctly.

> Have you tested ...

I trust you will tell me if I can help further.

Pat LaVarre

Re: [PATCH] fix cdrom mt rainier probe

[Jens Axboe]

On Fri, Jul 16 2004, Pat LaVarre wrote:
> Jens A:
> 
> > Have you tested it works as expected as well, with
> > rw and ro media?
> 
> Ouch, yes, please, never assume I've tested unless I say so explicitly.
> 
> So far I've just seen the expected, with one rewritable and one
> read-only disc, in a pair of USB and PATAPI drives:
> 
> $ sudo plscsi /dev/scd0 /dev/hdd
> export PLSCSI=/dev/scd0       // B Iomega   RRD              DB76
> export PLSCSI=/dev/hdd        // @ Iomega   RRD              74.B
> $
> $ sudo blockdev --getro /dev/scd0 /dev/hdd
> 0
> 0
> $
> $ sudo dd of=/dev/scd0 bs=64K seek=0 count=1 if=/dev/zero
> dd: opening `/dev/scd0': Read-only file system
> $ sudo dd of=/dev/hdd bs=64K seek=0 count=1 if=/dev/zero
> 1+0 records in
> 1+0 records out
> $
> $ # swap discs
> $
> $ sudo dd of=/dev/scd0 bs=64K seek=0 count=1 if=/dev/zero
> 1+0 records in
> 1+0 records out
> $ sudo dd of=/dev/hdd bs=64K seek=0 count=1 if=/dev/zero
> dd: opening `/dev/hdd': Read-only file system
> $

Looks good, thanks.

> > looks perfect!
> 
> Excellent, I conclude I have understood correctly.

You have.

> > Have you tested ...
> 
> I trust you will tell me if I can help further.

I'll start by forwarding this patch, so it at least will work in 2.6.8.

-- 
Jens Axboe

Re: [PATCH] fix cdrom mt rainier probe

[Pat LaVarre]

Jens A:

> I'll start by forwarding this patch,
> so it at least will work in 2.6.8.

2.6.8-rc1-bk6 looks good here now.

diff says the patch arrived intact.  tty shows the expected results.

Thank you.

Pat LaVarre

---

$ uname -msr
Linux 2.6.8-rc1-bk6 i686
$
$ sudo plscsi /dev/scd0 /dev/hdd
export PLSCSI=/dev/scd0       // B Iomega   RRD              74.B
export PLSCSI=/dev/hdd        // @ Iomega   RRD              74.B
$
$ sudo blockdev --getro /dev/scd0 /dev/hdd
0
0
$
$ sudo dd of=/dev/scd0 bs=64K seek=0 count=1 if=/dev/zero
dd: opening `/dev/scd0': Read-only file system
$ sudo dd of=/dev/hdd bs=64K seek=0 count=1 if=/dev/zero
1+0 records in
1+0 records out
$
$ # swap discs
$
$ sudo dd of=/dev/scd0 bs=64K seek=0 count=1 if=/dev/zero
1+0 records in
1+0 records out
$ sudo dd of=/dev/hdd bs=64K seek=0 count=1 if=/dev/zero
dd: opening `/dev/hdd': Read-only file system
$

---