[PATCH] CDC_MMC_WR

[PATCH] CDC_MMC_WR

[Pat LaVarre]

Jens A:

> > > Please add ...
> > > to cdrom.c instead ... GPCMD_GET_CONFIGURATION
> > 
> > Happily will do.  I'll continue to reply
> > as I progress or not.
>
> Great, thanks.

Please tell me what we think of this new patch?

You can see I've guessed I now should launch a new thread. The
linux-scsi thread "writable mmc profiles actually are writable" walked
us thru every painful step of me learning to write this patch.

So far I see Three design goals:

a) Add CDC_MMC_WR, masked by default, to: include/cdrom.h, to
drivers/ide/ide-cd.c, and to drivers/scsi/sr.c.

b) Correct device->writeable in sr.c sometime after
drivers/cdrom/cdrom.c discovers it was wrong.

c) Teach cdrom.c to discover CDC_MMC_WR.

That last goal I see divide into Eight changes to cdrom.c:

c1) Refuse to pass thru writes only if CDC_MMC_WR, no matter
CDC_DVD_RAM.

c2) Define cdrom_get_cdc and call cdrom_get_cdc only if register_cdrom
succeeds, just before register_cdrom does succeed.

c3) In cdrom_get_cdc, say &= ~CDC_MMC_WR if ide-cd or sr said &=
~CDC_DVD_RAM.

c4) In cdrom_get_cdc, also say &= ~CDC_MMC_WR if cdrom_get_capabilities
ok and cdrom_get_configuration ok and the profile fetched is any one of
the seven standard "random writable" profiles of mmc except not the
x001A dvd+rw profile that maybe can't be overwritten more than 1000
times in place, per Andy Polyakov's:
http://fy.chalmers.se/~appro/linux/DVD+RW/#udf

c5) Define cdrom_get_capabilities in terms of cdrom_mode_sense by
block-copy-edit of other cdrom.c source.

c6) Define cdrom_get_configuration in terms of cdo->generic_packet by
block-copy-edit of other cdrom.c source

c7) Add a line of text to /proc/sys/dev/cdrom/info to reveal CDC_MMC_WR.

c8) Update change history, REVISION, VERSION.

Please tell me what we think of this new patch?

Pat LaVarre

diff -Nur linux-2.6.0-test7/drivers/cdrom/cdrom.c linux/drivers/cdrom/cdrom.c
--- linux-2.6.0-test7/drivers/cdrom/cdrom.c	2003-10-08 13:24:02.000000000 -0600
+++ linux/drivers/cdrom/cdrom.c	2003-10-10 18:05:25.343260848 -0600
@@ -228,10 +228,13 @@
    3.12 Oct 18, 2000 - Jens Axboe <axboe@suse.de>
   -- Use quiet bit on packet commands not known to work
 
+   3.13 Oct 10, 2003 - Jens Axboe <axboe@suse.de>
+  -- Pass writes thru to all CDC_MMC_WR profiles, not just CDC_DVD_RAM.
+
 -------------------------------------------------------------------------*/
 
-#define REVISION "Revision: 3.12"
-#define VERSION "Id: cdrom.c 3.12 2000/10/18"
+#define REVISION "Revision: 3.13"
+#define VERSION "Id: cdrom.c 3.13 2003/10/10"
 
 /* I use an error-log mask to give fine grain control over the type of
    messages dumped to the system logs.  The available masks include: */
@@ -313,6 +316,7 @@
 #define CHECKAUDIO if ((ret=check_for_audio_disc(cdi, cdo))) return ret
 
 /* Not-exported routines. */
+static void cdrom_get_cdc(struct cdrom_device_info *cdi);
 static int open_for_data(struct cdrom_device_info * cdi);
 static int check_for_audio_disc(struct cdrom_device_info * cdi,
 			 struct cdrom_device_ops * cdo);
@@ -381,6 +385,8 @@
 	cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
 	cdi->next = topCdromPtr; 	
 	topCdromPtr = cdi;
+
+	cdrom_get_cdc(cdi);
 	return 0;
 }
 #undef ENSURE
@@ -408,6 +414,71 @@
 	return 0;
 }
 
+static int cdrom_get_capabilities(struct cdrom_device_info *cdi)
+{
+	struct cdrom_generic_command cgc0;
+	struct cdrom_generic_command * cgc = &cgc0;
+	u_char buf[0x18]; /* ide-cd length = x18, sr length = x80 */
+	int ret;
+	memset(cgc, 0, sizeof *cgc);
+	memset(&buf[0], 0, sizeof(buf));
+	cgc->buffer = buf;
+	cgc->buflen = sizeof buf;
+	ret = cdrom_mode_sense(cdi, cgc, GPMODE_CAPABILITIES_PAGE, 0);
+	return ret;
+}
+
+static int cdrom_get_configuration(struct cdrom_device_info *cdi)
+{
+	struct cdrom_generic_command cgc0;
+	struct cdrom_generic_command * cgc = &cgc0;
+	struct cdrom_device_ops *cdo = cdi->ops;
+	u_char buf[8];
+	int ret;
+	int profile;
+	memset(cgc, 0, sizeof *cgc);
+	memset(&buf[0], 0, sizeof(buf));
+	cgc->buffer = buf;
+	cgc->buflen = sizeof buf;
+	cgc->cmd[0] = GPCMD_GET_CONFIGURATION;
+	cgc->cmd[7] = cgc->buflen >> 8;
+	cgc->cmd[8] = cgc->buflen & 0xff;
+	cgc->data_direction = CGC_DATA_READ;
+	ret = cdo->generic_packet(cdi, cgc);
+	if (ret) return ret;
+	profile = buf[6] << 8 | buf[7];
+	cdinfo(CD_REG_UNREG, "profile x%04X\n", profile);
+	switch (profile) {
+		case 0x0001: /* Non-Removable Disk */
+		case 0x0002: /* Removable Disk */
+		case 0x0003: /* Magneto-Optical Erasable */
+		case 0x0005: /* AS-MO */
+		case 0x0012: /* DVD-RAM */
+		case 0x0022: /* DDCD-RW */
+			cdi->mask &= ~CDC_MMC_WR;
+			break;
+		case 0x001A: /* DVD+RW = not very rewritable in place */
+			break;
+		default:
+			break;
+	}
+	return ret;
+}
+
+static void cdrom_get_cdc(struct cdrom_device_info *cdi)
+{
+	cdinfo(CD_REG_UNREG, "cdrom_get_cdc\n");
+	if (CDROM_CAN(CDC_DVD_RAM)) {
+		cdi->mask &= ~CDC_MMC_WR;
+	} else if (cdrom_get_capabilities(cdi)) {
+		cdinfo(CD_WARNING,  "GET_CAPABILITIES not\n");
+	} else {
+		if (cdrom_get_configuration(cdi)) {
+			cdinfo(CD_WARNING,  "GET_CONFIGURATION not\n");
+		}
+	}
+}
+
 /* We use the open-option O_NONBLOCK to indicate that the
  * purpose of opening is only for subsequent ioctl() calls; no device
  * integrity checks are performed.
@@ -426,8 +497,9 @@
 	if ((fp->f_flags & O_NONBLOCK) && (cdi->options & CDO_USE_FFLAGS))
 		ret = cdi->ops->open(cdi, 1);
 	else {
-		if ((fp->f_mode & FMODE_WRITE) && !CDROM_CAN(CDC_DVD_RAM))
+		if ((fp->f_mode & FMODE_WRITE) && !CDROM_CAN(CDC_MMC_WR)) {
 			return -EROFS;
+		}
 
 		ret = open_for_data(cdi);
 	}
@@ -2406,6 +2478,10 @@
 	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
 	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
 
+	pos += sprintf(info+pos, "\nTolerates random write:");
+	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
+	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MMC_WR) != 0);
+
 	strcpy(info+pos,"\n\n");
 		
         return proc_dostring(ctl, write, filp, buffer, lenp);
diff -Nur linux-2.6.0-test7/include/linux/cdrom.h linux/include/linux/cdrom.h
--- linux-2.6.0-test7/include/linux/cdrom.h	2003-10-08 13:24:00.000000000 -0600
+++ linux/include/linux/cdrom.h	2003-10-10 15:37:16.000000000 -0600
@@ -388,6 +388,7 @@
 #define CDC_DVD_R		0x10000	/* drive can write DVD-R */
 #define CDC_DVD_RAM		0x20000	/* drive can write DVD-RAM */
 #define CDC_MO_DRIVE		0x40000 /* drive is an MO device */
+#define CDC_MMC_WR		0x80000	/* profile includes random write */
 
 /* drive status possibilities returned by CDROM_DRIVE_STATUS ioctl */
 #define CDS_NO_INFO		0	/* if not implemented */
diff -Nur linux-2.6.0-test7/drivers/ide/ide-cd.c linux/drivers/ide/ide-cd.c
--- linux-2.6.0-test7/drivers/ide/ide-cd.c	2003-10-08 13:24:04.000000000 -0600
+++ linux/drivers/ide/ide-cd.c	2003-10-10 15:36:33.000000000 -0600
@@ -2822,7 +2822,7 @@
 				CDC_MEDIA_CHANGED | CDC_PLAY_AUDIO | CDC_RESET |
 				CDC_IOCTLS | CDC_DRIVE_STATUS | CDC_CD_R |
 				CDC_CD_RW | CDC_DVD | CDC_DVD_R| CDC_DVD_RAM |
-				CDC_GENERIC_PACKET | CDC_MO_DRIVE,
+				CDC_GENERIC_PACKET | CDC_MO_DRIVE | CDC_MMC_WR,
 	.generic_packet		= ide_cdrom_packet,
 };
 
@@ -2832,7 +2832,7 @@
 	struct cdrom_device_info *devinfo = &info->devinfo;
 
 	devinfo->ops = &ide_cdrom_dops;
-	devinfo->mask = 0;
+	devinfo->mask = CDC_MMC_WR;
 	devinfo->speed = CDROM_STATE_FLAGS(drive)->current_speed;
 	devinfo->capacity = nslots;
 	devinfo->handle = (void *) drive;
diff -Nur linux-2.6.0-test7/drivers/scsi/sr.c linux/drivers/scsi/sr.c
--- linux-2.6.0-test7/drivers/scsi/sr.c	2003-10-08 13:24:03.000000000 -0600
+++ linux/drivers/scsi/sr.c	2003-10-10 15:48:12.000000000 -0600
@@ -67,7 +67,8 @@
 	(CDC_CLOSE_TRAY|CDC_OPEN_TRAY|CDC_LOCK|CDC_SELECT_SPEED| \
 	 CDC_SELECT_DISC|CDC_MULTI_SESSION|CDC_MCN|CDC_MEDIA_CHANGED| \
 	 CDC_PLAY_AUDIO|CDC_RESET|CDC_IOCTLS|CDC_DRIVE_STATUS| \
-	 CDC_CD_R|CDC_CD_RW|CDC_DVD|CDC_DVD_R|CDC_DVD_RAM|CDC_GENERIC_PACKET)
+	 CDC_CD_R|CDC_CD_RW|CDC_DVD|CDC_DVD_R|CDC_DVD_RAM|CDC_GENERIC_PACKET| \
+	 CDC_MMC_WR)
 
 static int sr_probe(struct device *);
 static int sr_remove(struct device *);
@@ -327,8 +328,12 @@
 	}
 
 	if (rq_data_dir(SCpnt->request) == WRITE) {
-		if (!cd->device->writeable)
-			return 0;
+		if (!cd->device->writeable) {
+			if ((cd->cdi.mask & CDC_MMC_WR) != 0) {
+				return 0;
+			}
+			cd->device->writeable = 1;
+		}
 		SCpnt->cmnd[0] = WRITE_10;
 		SCpnt->sc_data_direction = SCSI_DATA_WRITE;
 	} else if (rq_data_dir(SCpnt->request) == READ) {
@@ -550,7 +555,7 @@
 
 	cd->cdi.ops = &sr_dops;
 	cd->cdi.handle = cd;
-	cd->cdi.mask = 0;
+	cd->cdi.mask = CDC_MMC_WR;
 	cd->cdi.capacity = 1;
 	sprintf(cd->cdi.name, "sr%d", minor);
 

Re: [PATCH] CDC_MMC_WR

[Jens Axboe]

On Fri, Oct 10 2003, Pat LaVarre wrote:
> Jens A:
> 
> > > > Please add ...
> > > > to cdrom.c instead ... GPCMD_GET_CONFIGURATION
> > > 
> > > Happily will do.  I'll continue to reply
> > > as I progress or not.
> >
> > Great, thanks.
> 
> Please tell me what we think of this new patch?
> 
> You can see I've guessed I now should launch a new thread. The
> linux-scsi thread "writable mmc profiles actually are writable" walked
> us thru every painful step of me learning to write this patch.
> 
> So far I see Three design goals:
> 
> a) Add CDC_MMC_WR, masked by default, to: include/cdrom.h, to
> drivers/ide/ide-cd.c, and to drivers/scsi/sr.c.
> 
> b) Correct device->writeable in sr.c sometime after
> drivers/cdrom/cdrom.c discovers it was wrong.
> 
> c) Teach cdrom.c to discover CDC_MMC_WR.
> 
> That last goal I see divide into Eight changes to cdrom.c:
> 
> c1) Refuse to pass thru writes only if CDC_MMC_WR, no matter
> CDC_DVD_RAM.
> 
> c2) Define cdrom_get_cdc and call cdrom_get_cdc only if register_cdrom
> succeeds, just before register_cdrom does succeed.
> 
> c3) In cdrom_get_cdc, say &= ~CDC_MMC_WR if ide-cd or sr said &=
> ~CDC_DVD_RAM.
> 
> c4) In cdrom_get_cdc, also say &= ~CDC_MMC_WR if cdrom_get_capabilities
> ok and cdrom_get_configuration ok and the profile fetched is any one of
> the seven standard "random writable" profiles of mmc except not the
> x001A dvd+rw profile that maybe can't be overwritten more than 1000
> times in place, per Andy Polyakov's:
> http://fy.chalmers.se/~appro/linux/DVD+RW/#udf
> 
> c5) Define cdrom_get_capabilities in terms of cdrom_mode_sense by
> block-copy-edit of other cdrom.c source.
> 
> c6) Define cdrom_get_configuration in terms of cdo->generic_packet by
> block-copy-edit of other cdrom.c source
> 
> c7) Add a line of text to /proc/sys/dev/cdrom/info to reveal CDC_MMC_WR.
> 
> c8) Update change history, REVISION, VERSION.
> 
> Please tell me what we think of this new patch?

Lots of small nits, but in general I like it.

> +   3.13 Oct 10, 2003 - Jens Axboe <axboe@suse.de>
> +  -- Pass writes thru to all CDC_MMC_WR profiles, not just CDC_DVD_RAM.

That should be you :)

> +static int cdrom_get_capabilities(struct cdrom_device_info *cdi)
> +{
> +	struct cdrom_generic_command cgc0;
> +	struct cdrom_generic_command * cgc = &cgc0;

I see this repeated, what is the point? you are wasting 4/8 bytes on the
stack each time.


> +	u_char buf[0x18]; /* ide-cd length = x18, sr length = x80 */
> +	int ret;
> +	memset(cgc, 0, sizeof *cgc);

sizeof(*cgc);

> +	memset(&buf[0], 0, sizeof(buf));
> +	cgc->buffer = buf;
> +	cgc->buflen = sizeof buf;

sizeof(buf). There's a init_cdrom_command() for these types of
situations, please use it.

> +	ret = cdrom_mode_sense(cdi, cgc, GPMODE_CAPABILITIES_PAGE, 0);
> +	return ret;

No need for ret, either.

> +}
> +
> +static int cdrom_get_configuration(struct cdrom_device_info *cdi)
> +{
> +	struct cdrom_generic_command cgc0;
> +	struct cdrom_generic_command * cgc = &cgc0;

Ditto

> +	struct cdrom_device_ops *cdo = cdi->ops;
> +	u_char buf[8];
> +	int ret;
> +	int profile;
> +	memset(cgc, 0, sizeof *cgc);

Ditto

> +	memset(&buf[0], 0, sizeof(buf));
> +	cgc->buffer = buf;
> +	cgc->buflen = sizeof buf;
> +	cgc->cmd[0] = GPCMD_GET_CONFIGURATION;
> +	cgc->cmd[7] = cgc->buflen >> 8;
> +	cgc->cmd[8] = cgc->buflen & 0xff;
> +	cgc->data_direction = CGC_DATA_READ;
> +	ret = cdo->generic_packet(cdi, cgc);
> +	if (ret) return ret;
> +	profile = buf[6] << 8 | buf[7];
> +	cdinfo(CD_REG_UNREG, "profile x%04X\n", profile);
> +	switch (profile) {
> +		case 0x0001: /* Non-Removable Disk */
> +		case 0x0002: /* Removable Disk */
> +		case 0x0003: /* Magneto-Optical Erasable */
> +		case 0x0005: /* AS-MO */
> +		case 0x0012: /* DVD-RAM */
> +		case 0x0022: /* DDCD-RW */
> +			cdi->mask &= ~CDC_MMC_WR;
> +			break;
> +		case 0x001A: /* DVD+RW = not very rewritable in place */
> +			break;
> +		default:
> +			break;
> +	}
> +	return ret;

ret must be 0 here, so return 0 to make that clear.

> +static void cdrom_get_cdc(struct cdrom_device_info *cdi)
> +{
> +	cdinfo(CD_REG_UNREG, "cdrom_get_cdc\n");
> +	if (CDROM_CAN(CDC_DVD_RAM)) {
> +		cdi->mask &= ~CDC_MMC_WR;
> +	} else if (cdrom_get_capabilities(cdi)) {
> +		cdinfo(CD_WARNING,  "GET_CAPABILITIES not\n");
> +	} else {
> +		if (cdrom_get_configuration(cdi)) {
> +			cdinfo(CD_WARNING,  "GET_CONFIGURATION not\n");
> +		}
> +	}
> +}

Ugly flow here. And what is the point of the cdrom_get_capabilities()
call? If you just want to see if it's supported, the probe sequence in
ide-cd/sr should already have done that. I'd greatly prefer something
ala:

static void cdrom_get_cdc(struct cdrom_device_info *cdi)
{
	cdrom_get_configuration(cdi);

	if (CDROM_CAN(CDC_DVD_RAM))
		cdi->mask &= ~CDC_MMC_WR
}

> @@ -426,8 +497,9 @@
>  	if ((fp->f_flags & O_NONBLOCK) && (cdi->options & CDO_USE_FFLAGS))
>  		ret = cdi->ops->open(cdi, 1);
>  	else {
> -		if ((fp->f_mode & FMODE_WRITE) && !CDROM_CAN(CDC_DVD_RAM))
> +		if ((fp->f_mode & FMODE_WRITE) && !CDROM_CAN(CDC_MMC_WR)) {
>  			return -EROFS;
> +		}
>  
>  		ret = open_for_data(cdi);
>  	}
> @@ -2406,6 +2478,10 @@
>  	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
>  	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
>  
> +	pos += sprintf(info+pos, "\nTolerates random write:");
> +	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> +	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MMC_WR) != 0);
> +
>  	strcpy(info+pos,"\n\n");
>  		
>          return proc_dostring(ctl, write, filp, buffer, lenp);
> diff -Nur linux-2.6.0-test7/include/linux/cdrom.h linux/include/linux/cdrom.h
> --- linux-2.6.0-test7/include/linux/cdrom.h	2003-10-08 13:24:00.000000000 -0600
> +++ linux/include/linux/cdrom.h	2003-10-10 15:37:16.000000000 -0600
> @@ -388,6 +388,7 @@
>  #define CDC_DVD_R		0x10000	/* drive can write DVD-R */
>  #define CDC_DVD_RAM		0x20000	/* drive can write DVD-RAM */
>  #define CDC_MO_DRIVE		0x40000 /* drive is an MO device */
> +#define CDC_MMC_WR		0x80000	/* profile includes random write */
>  
>  /* drive status possibilities returned by CDROM_DRIVE_STATUS ioctl */
>  #define CDS_NO_INFO		0	/* if not implemented */
> diff -Nur linux-2.6.0-test7/drivers/ide/ide-cd.c linux/drivers/ide/ide-cd.c
> --- linux-2.6.0-test7/drivers/ide/ide-cd.c	2003-10-08 13:24:04.000000000 -0600
> +++ linux/drivers/ide/ide-cd.c	2003-10-10 15:36:33.000000000 -0600
> @@ -2822,7 +2822,7 @@
>  				CDC_MEDIA_CHANGED | CDC_PLAY_AUDIO | CDC_RESET |
>  				CDC_IOCTLS | CDC_DRIVE_STATUS | CDC_CD_R |
>  				CDC_CD_RW | CDC_DVD | CDC_DVD_R| CDC_DVD_RAM |
> -				CDC_GENERIC_PACKET | CDC_MO_DRIVE,
> +				CDC_GENERIC_PACKET | CDC_MO_DRIVE | CDC_MMC_WR,
>  	.generic_packet		= ide_cdrom_packet,
>  };
>  
> @@ -2832,7 +2832,7 @@
>  	struct cdrom_device_info *devinfo = &info->devinfo;
>  
>  	devinfo->ops = &ide_cdrom_dops;
> -	devinfo->mask = 0;
> +	devinfo->mask = CDC_MMC_WR;

Must be left-over junk, should still be 0 here.

>  	devinfo->speed = CDROM_STATE_FLAGS(drive)->current_speed;
>  	devinfo->capacity = nslots;
>  	devinfo->handle = (void *) drive;
> diff -Nur linux-2.6.0-test7/drivers/scsi/sr.c linux/drivers/scsi/sr.c
> --- linux-2.6.0-test7/drivers/scsi/sr.c	2003-10-08 13:24:03.000000000 -0600
> +++ linux/drivers/scsi/sr.c	2003-10-10 15:48:12.000000000 -0600
> @@ -67,7 +67,8 @@
>  	(CDC_CLOSE_TRAY|CDC_OPEN_TRAY|CDC_LOCK|CDC_SELECT_SPEED| \
>  	 CDC_SELECT_DISC|CDC_MULTI_SESSION|CDC_MCN|CDC_MEDIA_CHANGED| \
>  	 CDC_PLAY_AUDIO|CDC_RESET|CDC_IOCTLS|CDC_DRIVE_STATUS| \
> -	 CDC_CD_R|CDC_CD_RW|CDC_DVD|CDC_DVD_R|CDC_DVD_RAM|CDC_GENERIC_PACKET)
> +	 CDC_CD_R|CDC_CD_RW|CDC_DVD|CDC_DVD_R|CDC_DVD_RAM|CDC_GENERIC_PACKET| \
> +	 CDC_MMC_WR)
>  
>  static int sr_probe(struct device *);
>  static int sr_remove(struct device *);
> @@ -327,8 +328,12 @@
>  	}
>  
>  	if (rq_data_dir(SCpnt->request) == WRITE) {
> -		if (!cd->device->writeable)
> -			return 0;
> +		if (!cd->device->writeable) {
> +			if ((cd->cdi.mask & CDC_MMC_WR) != 0) {
> +				return 0;
> +			}
> +			cd->device->writeable = 1;
> +		}

This is ugly, don't hack around it like that. If CDC_MMC_WR set, sr
should have set device->writeable as well. Needs to go.


>  		SCpnt->cmnd[0] = WRITE_10;
>  		SCpnt->sc_data_direction = SCSI_DATA_WRITE;
>  	} else if (rq_data_dir(SCpnt->request) == READ) {
> @@ -550,7 +555,7 @@
>  
>  	cd->cdi.ops = &sr_dops;
>  	cd->cdi.handle = cd;
> -	cd->cdi.mask = 0;
> +	cd->cdi.mask = CDC_MMC_WR;
>  	cd->cdi.capacity = 1;
>  	sprintf(cd->cdi.name, "sr%d", minor);

Ditto, why are you changing this?! Why do you think CDC_MMC_WR is
special compared to the other mask flags?

-- 
Jens Axboe

Re: [PATCH] CDC_MMC_WR

[Pat LaVarre]

Jens A:

Much better now?

Pat LaVarre

diff -Nur linux-2.6.0-test7/drivers/cdrom/cdrom.c linux/drivers/cdrom/cdrom.c
--- linux-2.6.0-test7/drivers/cdrom/cdrom.c	2003-10-08 13:24:02.000000000 -0600
+++ linux/drivers/cdrom/cdrom.c	2003-10-13 08:45:24.550496936 -0600
@@ -1,7 +1,7 @@
 /* linux/drivers/cdrom/cdrom.c. 
    Copyright (c) 1996, 1997 David A. van Leeuwen.
    Copyright (c) 1997, 1998 Erik Andersen <andersee@debian.org>
-   Copyright (c) 1998, 1999 Jens Axboe <axboe@image.dk>
+   Copyright (c) 1998, 1999, 2003 Jens Axboe <axboe@image.dk>
 
    May be copied or modified under the terms of the GNU General Public
    License.  See linux/COPYING for more information.
@@ -228,10 +228,13 @@
    3.12 Oct 18, 2000 - Jens Axboe <axboe@suse.de>
   -- Use quiet bit on packet commands not known to work
 
+   3.13 Oct 13, 2003 - Jens Axboe <axboe@suse.de>
+  -- Now mask CDC_MMC_WR only if not std. rewritable, was mask if not DVD_RAM.
+
 -------------------------------------------------------------------------*/
 
-#define REVISION "Revision: 3.12"
-#define VERSION "Id: cdrom.c 3.12 2000/10/18"
+#define REVISION "Revision: 3.13"
+#define VERSION "Id: cdrom.c 3.13 2003/10/13"
 
 /* I use an error-log mask to give fine grain control over the type of
    messages dumped to the system logs.  The available masks include: */
@@ -313,6 +316,7 @@
 #define CHECKAUDIO if ((ret=check_for_audio_disc(cdi, cdo))) return ret
 
 /* Not-exported routines. */
+static void cdrom_get_cdc(struct cdrom_device_info *cdi);
 static int open_for_data(struct cdrom_device_info * cdi);
 static int check_for_audio_disc(struct cdrom_device_info * cdi,
 			 struct cdrom_device_ops * cdo);
@@ -381,6 +385,8 @@
 	cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
 	cdi->next = topCdromPtr; 	
 	topCdromPtr = cdi;
+
+	cdrom_get_cdc(cdi);
 	return 0;
 }
 #undef ENSURE
@@ -408,6 +414,48 @@
 	return 0;
 }
 
+static int cdrom_get_configuration(struct cdrom_device_info *cdi)
+{
+	struct cdrom_generic_command cgc;
+	struct cdrom_device_ops *cdo = cdi->ops;
+	u_char buf[8];
+	int ret;
+	init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_READ);
+	cgc.cmd[0] = GPCMD_GET_CONFIGURATION;
+	cgc.cmd[7] = cgc.buflen >> 8;
+	cgc.cmd[8] = cgc.buflen & 0xff;
+	cgc.data_direction = CGC_DATA_READ;
+	ret = cdo->generic_packet(cdi, &cgc);
+	if (!ret) {
+		int profile = buf[6] << 8 | buf[7];
+		cdinfo(CD_REG_UNREG, "profile x%04X\n", profile);
+		switch (profile) {
+			case 0x0001: /* Non-Removable Disk */
+			case 0x0002: /* Removable Disk */
+			case 0x0003: /* Magneto-Optical Erasable */
+			case 0x0005: /* AS-MO */
+			case 0x0012: /* DVD-RAM */
+			case 0x0022: /* DDCD-RW */
+				return 0;
+			case 0x001A: /* DVD+RW = not much rewrite in place */
+			default:
+				break;
+		}
+	}
+	cdi->mask |= CDC_MMC_WR;
+	return ret;
+}
+
+static void cdrom_get_cdc(struct cdrom_device_info *cdi)
+{
+	if (CDROM_CAN(CDC_MMC_WR) && !CDROM_CAN(CDC_DVD_RAM)) {
+		cdinfo(CD_REG_UNREG, "cdrom_get_configuration\n");
+		if (cdrom_get_configuration(cdi)) {
+			cdinfo(CD_REG_UNREG, "cdrom_get_configuration not\n");
+		}
+	}
+}
+
 /* We use the open-option O_NONBLOCK to indicate that the
  * purpose of opening is only for subsequent ioctl() calls; no device
  * integrity checks are performed.
@@ -426,8 +474,9 @@
 	if ((fp->f_flags & O_NONBLOCK) && (cdi->options & CDO_USE_FFLAGS))
 		ret = cdi->ops->open(cdi, 1);
 	else {
-		if ((fp->f_mode & FMODE_WRITE) && !CDROM_CAN(CDC_DVD_RAM))
+		if ((fp->f_mode & FMODE_WRITE) && !CDROM_CAN(CDC_MMC_WR)) {
 			return -EROFS;
+		}
 
 		ret = open_for_data(cdi);
 	}
@@ -2406,6 +2455,10 @@
 	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
 	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
 
+	pos += sprintf(info+pos, "\nTolerates random write:");
+	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
+	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MMC_WR) != 0);
+
 	strcpy(info+pos,"\n\n");
 		
         return proc_dostring(ctl, write, filp, buffer, lenp);
diff -Nur linux-2.6.0-test7/include/linux/cdrom.h linux/include/linux/cdrom.h
--- linux-2.6.0-test7/include/linux/cdrom.h	2003-10-08 13:24:00.000000000 -0600
+++ linux/include/linux/cdrom.h	2003-10-10 15:37:16.000000000 -0600
@@ -388,6 +388,7 @@
 #define CDC_DVD_R		0x10000	/* drive can write DVD-R */
 #define CDC_DVD_RAM		0x20000	/* drive can write DVD-RAM */
 #define CDC_MO_DRIVE		0x40000 /* drive is an MO device */
+#define CDC_MMC_WR		0x80000	/* profile includes random write */
 
 /* drive status possibilities returned by CDROM_DRIVE_STATUS ioctl */
 #define CDS_NO_INFO		0	/* if not implemented */
diff -Nur linux-2.6.0-test7/drivers/ide/ide-cd.c linux/drivers/ide/ide-cd.c
--- linux-2.6.0-test7/drivers/ide/ide-cd.c	2003-10-08 13:24:04.000000000 -0600
+++ linux/drivers/ide/ide-cd.c	2003-10-13 08:35:49.362938736 -0600
@@ -2822,7 +2822,7 @@
 				CDC_MEDIA_CHANGED | CDC_PLAY_AUDIO | CDC_RESET |
 				CDC_IOCTLS | CDC_DRIVE_STATUS | CDC_CD_R |
 				CDC_CD_RW | CDC_DVD | CDC_DVD_R| CDC_DVD_RAM |
-				CDC_GENERIC_PACKET | CDC_MO_DRIVE,
+				CDC_GENERIC_PACKET | CDC_MO_DRIVE | CDC_MMC_WR,
 	.generic_packet		= ide_cdrom_packet,
 };
 
diff -Nur linux-2.6.0-test7/drivers/scsi/sr.c linux/drivers/scsi/sr.c
--- linux-2.6.0-test7/drivers/scsi/sr.c	2003-10-08 13:24:03.000000000 -0600
+++ linux/drivers/scsi/sr.c	2003-10-13 08:38:40.603906152 -0600
@@ -67,7 +67,8 @@
 	(CDC_CLOSE_TRAY|CDC_OPEN_TRAY|CDC_LOCK|CDC_SELECT_SPEED| \
 	 CDC_SELECT_DISC|CDC_MULTI_SESSION|CDC_MCN|CDC_MEDIA_CHANGED| \
 	 CDC_PLAY_AUDIO|CDC_RESET|CDC_IOCTLS|CDC_DRIVE_STATUS| \
-	 CDC_CD_R|CDC_CD_RW|CDC_DVD|CDC_DVD_R|CDC_DVD_RAM|CDC_GENERIC_PACKET)
+	 CDC_CD_R|CDC_CD_RW|CDC_DVD|CDC_DVD_R|CDC_DVD_RAM|CDC_GENERIC_PACKET| \
+	 CDC_MMC_WR)
 
 static int sr_probe(struct device *);
 static int sr_remove(struct device *);
@@ -327,8 +328,9 @@
 	}
 
 	if (rq_data_dir(SCpnt->request) == WRITE) {
-		if (!cd->device->writeable)
+		if ((cd->cdi.mask & CDC_MMC_WR) != 0) {
 			return 0;
+		}
 		SCpnt->cmnd[0] = WRITE_10;
 		SCpnt->sc_data_direction = SCSI_DATA_WRITE;
 	} else if (rq_data_dir(SCpnt->request) == READ) {
@@ -788,8 +790,6 @@
 	if ((buffer[n + 3] & 0x20) == 0) {
 		/* can't write DVD-RAM media */
 		cd->cdi.mask |= CDC_DVD_RAM;
-	} else {
-		cd->device->writeable = 1;
 	}
 	if ((buffer[n + 3] & 0x10) == 0)
 		/* can't write DVD-R media */

Re: [PATCH] CDC_MMC_WR

[Pat LaVarre]

I double-checked.

My same -test7 patch applies without complaint to -test8:

patching file drivers/cdrom/cdrom.c
patching file include/linux/cdrom.h
patching file drivers/ide/ide-cd.c
patching file drivers/scsi/sr.c

`diff` thereafter confirms that precisely the same lines have changed as
in -test7.

I'm guessing this patch changes no behaviour except that pdt x05 dvd/cd
devices that support mode page x5A Capabilities thereafter get a chance
via op x46 Get Configuration to end with CDC_MMC_WR unmasked ...

I'm curious to know how wrong I am.

Pat LaVarre

Re: [PATCH] CDC_MMC_WR

[Pat LaVarre]

My 2003-10-13 -test7 patch, with the q=raw suffix included to help ask
for hard tabs etc. intact, appears archived as:

http://marc.theaimsgroup.com/?l=linux-scsi&m=106669072008705&q=raw

Just now I triple-checked at my desk.  To -test9, that same patch
applies without complaint, produces precisely the same diff, and works.

I think that patch teaches Linux that standard mmc writable profile
devices are writable, in the least disruptive way possible.  I'm
delighted to see that patch shrunk and shrunk again as we tutored me
here.

Pat LaVarre

Re: [PATCH] CDC_MMC_WR

[Pat LaVarre]

Jens A:

> in general I like it ...

Good to hear, thanks for saying we progress.

I interpret your remarks as follows, please tell me
where I err.

> > ... linux/drivers/ide/ide-cd.c ...
> > - devinfo->mask = 0;
> > + devinfo->mask = CDC_MMC_WR;
>
> Must be left-over junk,
> should still be 0 here ...
>
> > ... linux/drivers/scsi/sr.c ...
> > - cd->cdi.mask = 0;
> > + cd->cdi.mask = CDC_MMC_WR;
>
> Ditto, why are you changing this?! Why do you
> think CDC_MMC_WR is special compared to the
> other mask flags?

I guess you mean we should:

1) Unmask CDC_MMC_WR in ide-cd and sr when and only
when unmasking CDC_DVD_RAM.  Start with mask = 0 same
as before, but substitute (CDC_DVD_RAM|CDC_MMC_WR)
when we mask &= ~CDC_DVD_RAM and when we mask |=
CDC_DVD_RAM.

2) Unmask CDC_MMC_WR in cdrom if trying op x46
GPCMD_GET_CONFIGURATION discovers a random writable
profile.

Correct?

If Not correct, then perhaps I erred by failing to say
only ide-cd/sr unmask CDC_MMC_WR.  If yes correct,
then I erred mainly by trying to say only cdrom
unmasks CDC_MMC_WR.  In any case I ask:

Do we want this patch or a second patch soon after to
move the mode sense of page x2A
GPMODE_CAPABILITIES_PAGE into cdrom.c register_cdrom
from ide-cd and sr?

Perhaps no?

I think No mainly because I see ide-cd
ide_cdrom_get_capabilities tries buflen = x18 usually,
x1C sometimes, and sr get_capabilities always tries
buflen = x80.  I see no easy way to preserve that
difference if we move this code to cdrom.

In passing, I think the sr variation actually provokes
devices to copy in less than the buflen of data, why
that works reliably thru usb/ storage/ I do not yet
know, I will pursue and report back.

> From: p_lavarre@yahoo.com

Please forgive the aggressive line breaks in this
email from me, please forgive my intermittently broken
>From address.  Reply-to header should correctly
suggest p.lavarre@ieee.org, bur also reply-to-from
should work if you try it.

> > +static void cdrom_get_cdc(
> > struct cdrom_device_info *cdi)
> > +{
> > +    cdinfo(CD_REG_UNREG, "cdrom_get_cdc\n");
> > +    if (CDROM_CAN(CDC_DVD_RAM)) {
> > +        cdi->mask &= ~CDC_MMC_WR;
> > +    } else if (cdrom_get_capabilities(cdi)) {
> > +        cdinfo(CD_WARNING,
> > "GET_CAPABILITIES not\n");
> > +    } else {
> > +        if (cdrom_get_configuration(cdi)) {
> > +            cdinfo(CD_WARNING,
> > "GET_CONFIGURATION not\n");
> > +        }
> > +    }
> > +}
>
> Ugly flow here. And what is the point of the
> cdrom_get_capabilities() call? If you just want to
> see if it's supported, the probe sequence in
> ide-cd/sr should already have done that.

To my eye, these remarks again raise the same issues
discussed above.

For the sake of offering immediate discussion, let's
suppose we do change ide-cd/sr to unmask CDC_MMC_WR
for CDC_DVD_RAM and we add no cdinfo.  Then I think I
see the ugly flow improves slightly:

if (!CDROM_CAN(CDC_MMC_WR)) {
    if (!cdrom_get_capabilities(cdi)) {
            (void) cdrom_get_configuration(cdi);
    }
}

But is this what we want?

Please tell me how often we wish to try op x46
GPCMD_GET_CONFIGURATION, new since 1999?

I'm guessing the less often the better, for the sake
of compatibility with old fragile devices.  I'm
guessing don't risk op x46 unless
!CDROM_CAN(CDC_MMC_WR), since as yet that risk buys us
nothing.  I'm guessing don't risk op x46 unless the
ide-cd/sr mode sense of page x2A
GPMODE_CAPABILITIES_PAGE did succeed, since without
such success the 1999 mmc standard that defines op x46
does not apply.

Regretfully I agree here as yet I have actually
written something slightly different.  Here I see I
have written don't risk op x46 unless
!CDROM_CAN(CDC_DVD_RAM) &&
!cdrom_get_capabilities(cdi).

I have written that only because I don't yet know how
cdrom may test to see if ide-cd/sr mode sense of page
x2A GPMODE_CAPABILITIES_PAGE did succeed.  When first
I looked I only found some seemingly illegit ways e.g.
capability bits that today get set only if a mode
sense of page x2A GPMODE_CAPABILITIES_PAGE did
succeed.

My bottom line remains: please tell me how often we
wish to try op x46 GPCMD_GET_CONFIGURATION, new since
1999?

> > +        cdinfo(CD_WARNING,
> > "GET_CAPABILITIES not\n");
> > +    } else {
>
> Ugly flow here. And ...

Please also tell me how much/little cdinfo we wish to
add?  I included cdinfo in every branch in the belief
that all change breaks some devices.

> I'd greatly prefer something ala:
> static void cdrom_get_cdc(
> struct cdrom_device_info *cdi)
> {
>     cdrom_get_configuration(cdi);
>     if (CDROM_CAN(CDC_DVD_RAM))
>         cdi->mask &= ~CDC_MMC_WR
> }

Please say again if yes still we prefer precisely this
(with unbroken lines of course).  My copy of your
original had an additional blank line, please say if
we want that too.

> > ... linux/drivers/scsi/sr.c ...
> > +    if (!cd->device->writeable) {
> > +        if ((cd->cdi.mask & CDC_MMC_WR) != 0) {
> > +            return 0;
> > +        }
> > +        cd->device->writeable = 1;
> > +    }
>
> This is ugly, don't hack around it like that ...
> Needs to go.

Yes please.

> If CDC_MMC_WR set, sr should have set
> device->writeable as well.

To my eye, these remarks again raise the same issues
discussed above.

I wonder: should sr alone, cdrom alone, or do both
unmask CDC_MMC_WR?

If cdrom ever does unmask CDC_MMC_WR, how then should
cdrom arrange for sr to set device->writeable?

May we delete device->writeable altogether, and have
sr trust cdrom to check CDC_MMC_WR, just as ide-cd
trusts cdrom to check CDC_MMC_WR?

> Lots of small nits,

Honestly I did try to copy style, thanks for telling
me specifically where I failed.

> > + struct cdrom_generic_command cgc0;
> > + struct cdrom_generic_command * cgc = &cgc0;
> > + ... cgc->...
>
> I see this repeated, what is the point? ...
> wasting 4/8 bytes on the stack each time.

I wrote cgc-> where I could have written cgc0. so that
I could copy more lines of code without changing them.
 I'm too new to know yet what actually equivalent C
idioms we tell gcc to accept as equivalent.  I thought
I had accurately copied this seemingly pointless waste
from other cdrom/cdrom.c and scsi/sr.c code.  

I see cgc-> appears without init_cdrom_command in
cdrom_mode_sense, cdrom_mode_select, etc.  Now that
you prompt me to look, I find cgc. appears with
init_cdrom_command in cdrom_read_subchannel. 
Hereafter I will block-copy-edit from there instead.

> ... sizeof(*cgc); ...
> ... sizeof(buf) ...
> init_cdrom_command() ... please use ...
> No need for ret, either ...

All these follow from choices like cdrom_mode_sense
vs. cdrom_read_subchannel choice, will try to comply.

> ret must be 0 ... so return 0 to make that clear.

Style, will comply.  The other camp on this one likes
all return statements to appear identical, to help gcc
understand how often we write return when we mean
goto.

> > +   3.13 Oct 10, 2003 - Jens Axboe <axboe@suse.de>
> > +  -- Pass writes thru to all CDC_MMC_WR profiles,
> not just CDC_DVD_RAM.
>
> That should be you :)

Sorry in my newbie ignorance I do not know what the :)
smiley means in this context.  Unless you say
different, in my next version of this patch I will
leave the explicit credit/ blame/ copyright as in my
last version, so that you can easily choose to correct
it again or not.

Pat LaVarre
p.lavarre@ieee.org


__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

Re: [PATCH] CDC_MMC_WR

[Pat LaVarre]

Sorry I didn't dream this up earlier, but now, unless
redirected, next I will try a new & more creative
alternative, specifically ...

In four parts:

1) Unmask CDC_MMC_WR in ide-cd/sr by default.  !! Yes
by default !!

2) Mask CDC_MMC_WR in ide-cd/sr if op x1A/5A Mode
Sense of x2A Capabilities fails.

3) Try op x46 Get Configuration in cdrom, just before
register_cdrom succeeds, only if !CDC_DVD_RAM and also
CDC_MMC_WR was left unmasked by ide-cd/sr.  Mask
CDC_MMC_WR if op x46 fails and mask CDC_MMC_WR if op
x46 says profile not a standard writable profiles.

4) Change sr to refuse to pass thru writes only if
!CDC_MMC_WR.  Delete all mention of device->writeable.

By 1 2 3 we will try op x46 Get Configuration only
after the ide-cd/sr flavour of op x1A/5A Mode Sense
fails.

By 4, we preserve the behaviour of sr even when sr
runs without cdrom e.g. if register_cdrom fails.

Like it?  Me I'm delighted to see us disentangle
CDC_MMC_WR from the debates over whether to ask to
copy In x18 or x1C or x80 bytes of Data when mode
sensing page x2A Capabilities.

Pat LaVarre


__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

Re: [PATCH] CDC_MMC_WR

[Pat LaVarre]

[[[ Ouch I see my English was poor: often I wrote the
wrong number of not's when speaking of bits not
unmasked.  In case I was therefore not comprehensible,
I will now try again to express myself correctly.  Or
maybe you'd rather wait for the code. ]]]

Sorry I didn't dream this up earlier, but now, unless
redirected, next I will try a new & more creative
alternative, specifically ...

In four parts:

1) Patch ide-cd/sr to unmask CDC_MMC_WR by default. 
!! Yes by default !!

2) Except patch ide-cd/sr to mask CDC_MMC_WR if op
x1A/5A Mode Sense of x2A Capabilities fails.

3) Patch cdrom to try op x46 Get Configuration, just
before register_cdrom succeeds, only if ide-cd/sr left
CDC_MMC_WR unmasked and also left CDC_DVD_RAM masked. 
Afterwards, mask CDC_MMC_WR if op x46 fails, also mask
CDC_MMC_WR if op x46 fetches any profile other than a
standard writable profile.

4) Change sr to refuse to pass thru writes unless
CDC_MMC_WR is unmasked.  Delete all mention of
device->writeable.

In conclusion I hope:

By 1 2 3 we will try op x46 Get Configuration only
if and after the ide-cd/sr flavour of op x1A/5A Mode
Sense succeeds.

By 4, we will preserve the legacy 2.6.0-test7
behaviour of sr even when sr runs without cdrom e.g.
if register_cdrom fails.

Like this idea?

Me I'm delighted to see us disentangle CDC_MMC_WR from
the debates over whether ide-cd, sr, and/or cdrom mode
senses page x2A Capabilities and which of these asks
to copy In x18 or x1C or x80 bytes of Data.

Pat LaVarre



__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

Re: [PATCH] CDC_MMC_WR

[Pat LaVarre]

> My 2003-10-13 -test7 patch,
> with the q=raw suffix included
> to help ask for hard tabs etc. intact, appears archived as:

Bzzzzt.  Wrong link, sorry.

To see that 2.6.0-test7 patch that works -test8 or -test9 as well, the
correct link into the depths of history is:

http://marc.theaimsgroup.com/?l=linux-scsi&m=106605749929120&q=raw

I know this because now for a friend I'm trying to apply this patch to
2.4.22.

Pat LaVarre

P.S. Of course unless you add an eol to the linux-scsi majordomo
signature, you will be warned: "patch unexpectedly ends in middle of
line".

Re: [PATCH] CDC_MMC_WR

[Pat LaVarre]

Kindly offline I hear we want a better patch, specifically:

Have "the ->writeable bit actually be set if the device is writeable".

Anybody able to help me decrypt that remark more rapidly?

Does the ->writeable bit exist for ide-cd devices, or only for sr
devices?

When cdrom.ko decides a device is not writable, how does cdrom.ko name
that bit, else how does cdrom.ko call into ide-cd/sr to clear that bit?

Pat LaVarre

P.S. The 2.4.22 version of this 2.6.0-test7 -test8 -test9 patch is:

http://marc.theaimsgroup.com/?l=linux-scsi&m=106764981303759

List:     linux-scsi
Subject:  [PATCH] Backport CDC_MMC_WR
Date:     2003-11-01 1:22:49

Re: [PATCH] CDC_MMC_WR

[Pat LaVarre]

> Does the ->writeable bit exist for ide-cd devices, or only for sr
> devices?

I'm guessing only sr devices, since grep writeable fails in drivers/ide/
and in drivers/cdrom/.

> When cdrom.ko decides a device is not writable, how does cdrom.ko name
> that bit, else how does cdrom.ko call into ide-cd/sr to clear that bit?

Call back into sr from cdrom may occur via the cdrom_device_ops.

We define cdrom_ioctl CDROM_GET_CAPABILITY but no SET of capability,
though we do define CDROM_SET_OPTIONS.

Maybe we're telling me to add a call back to the cdrom_device_ops for
cdrom to notify ide-cd/sr that the capabilities have changed.  ide-cd
could ignore that notification, sr could set
scsi_cd->scsi_device->writeable accordingly.

> Have "the ->writeable bit actually be set if the device is writeable".

grep is failing to find code for me that fetches this bit.

Is this bit ever fetched?

http://lxr.linux.no/ident?v=2.6.0-test7&i=writeable
no hits.

http://lxr.linux.no/search?v=2.6.0-test7&string=writeable
over seventy hits.  Apparently none relevant, except to say scsi_scan
sets this bit for sd, clears this bit for sr, and sr sometimes sets and
fetches this bit until I patch sr to branch on CDC_MMC_WR instead.

Pat LaVarre