Bug 26644

Summary:

[PATCH] ATA/ATAPI driver doesn't implement CDIOCREADSUBCHANNEL correctly

Product:

Base System

Reporter:

un1i <un1i>

Component:

kern

Assignee:

Søren Schmidt <sos>

Status:

Closed FIXED

Severity:

Affects Only Me

Priority:

Normal

Version:

5.0-CURRENT

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
file.diff	none
file.diff	none

Description un1i 2001-04-17 13:50:01 UTC

1) The ioctl CDIOCREADSUBCHANNEL can be used to request position information,
   the media catalogue or track info. However, atapi-cd.c:acdioctl()
   always returns the position information.

2) At the same place, the function lba2msf() is used to compute the track
   relative address. However, this function adds an offset of 150 frames
   to convert from LBA (logical block address) to MSF (minute, second, frame).
   This is correct for absolute adresses, but not in this case.

Fix: The following two patches are alternative. The first one tries to stay
with the current style: copy all data from cdp->subchan to struct data
and do the lba-msf conversion with lba2msf().

The second patch isn't so lengthy, it copies data directly from
cdp->subchan to the userland and leaves the lba-msf conversion to
the drive.

I could imagine that reasons for the original style would be to isolate
struct cd_sub_channel_info and struct subchan in case one of them changes,
and to work around broken drives that can't do the lba-msf conversion.

I'm not sure how relevant these are. The code in /sys/cam/scsi/scsi_cd.c
looks more like my second patch.

You'll notice that I always ask the drive for position information first.
This is because the drive will return an error if the media catalog
or track information is requested while an audio play operation is
in progress. Therefore I only request that if the audio status is ok.

How-To-Repeat: 
1) Execute "cdcontrol -f /dev/acd0 status". Note that the media catalog
   number is bogus or contains control characters.

2) Execute "cdcontrol -f /dev/acd0 play 1 && cdcontrol -f /dev/acd0 status"
   and note that the "current position" starts at 0:02.00, not 0:00.00 as
   expected.

Comment 1 Peter Pentchev freebsd_committer

2001-04-17 14:38:55 UTC

Responsible Changed
From-To: freebsd-bugs->sos

Over to Mr. ATA :)

Comment 2 Philipp Mergenthaler 2001-04-29 17:49:10 UTC

Oops, the first patch should also include the following change. Sorry.

Bye, Philipp


Index: cdio.h
===================================================================
RCS file: /ncvs/src/sys/sys/cdio.h,v
retrieving revision 1.21
diff -r1.21 cdio.h
88c88,89
<         u_int   :8;
---
> 	u_int	control:4;
> 	u_int	addr_type:4;

Comment 3 Philipp Mergenthaler 2002-02-27 19:21:50 UTC

These are updates of the original patches in the PR, since those don't
apply cleanly anymore.  Also, the second issue in that PR (wrong time
computation) is mood; it was probably a bug in my old CD-ROM drive.

The two patch sets are alternative: The first one tries to stay with the
current style, it copies all structure members separately.
The second patch set copies the data directly from cdp->subchan to the
userland (similiar to what the SCSI-driver does).

The patches to atapi-cd.h should be orthogonal to those to atapi-cd.c,
though I haven't tested that.



Index: sys/dev/ata/atapi-cd.c
===================================================================
RCS file: /ncvs/src/sys/dev/ata/atapi-cd.c,v
retrieving revision 1.109
diff -u -r1.109 atapi-cd.c
--- sys/dev/ata/atapi-cd.c	15 Feb 2002 07:08:44 -0000	1.109
+++ sys/dev/ata/atapi-cd.c	27 Feb 2002 18:57:23 -0000
@@ -711,6 +711,8 @@
 	    struct ioc_read_subchannel *args =
 		(struct ioc_read_subchannel *)addr;
 	    struct cd_sub_channel_info *data;
+	    u_int8_t format;
+	    int i;
 	    int8_t ccb[16] = { ATAPI_READ_SUBCHANNEL, 0, 0x40, 1, 0, 0, 0,
 			       sizeof(cdp->subchan)>>8, sizeof(cdp->subchan),
 			       0, 0, 0, 0, 0, 0, 0 };
@@ -718,6 +720,13 @@
 	    if (args->data_len > sizeof(struct cd_sub_channel_info) ||
 		args->data_len < sizeof(struct cd_sub_channel_header)) {
 		error = EINVAL;
+ 		break;
+ 	    }
+
+	    format=args->data_format;
+	    if ((format != CD_CURRENT_POSITION) &&
+		(format != CD_MEDIA_CATALOG) && (format != CD_TRACK_INFO)) {
+		error = EINVAL;
 		break;
 	    }
 
@@ -726,27 +735,100 @@
 					 NULL, NULL))) {
 		break;
 	    }
+
+	    /*
+	     * Ask for media catalogue or track info only if no audio play
+	     * operation is in progress or the drive will return an error.
+	     */
+
+	    if ((format == CD_MEDIA_CATALOG) || (format == CD_TRACK_INFO)) {
+		if (cdp->subchan.header.audio_status == 0x11) {
+		    error = EINVAL;
+		    break;
+		}
+
+		ccb[3] = format;
+		if (format == CD_TRACK_INFO)
+		    ccb[6] = args->track;
+
+		if ((error = atapi_queue_cmd(cdp->device, ccb,
+					     (caddr_t)&cdp->subchan, 
+					     sizeof(cdp->subchan), ATPR_F_READ,
+					     10, NULL, NULL))) {
+		    break;
+		}
+	    }
+
 	    data = malloc(sizeof(struct cd_sub_channel_info),
 			  M_ACD, M_NOWAIT | M_ZERO);
 
-	    if (args->address_format == CD_MSF_FORMAT) {
-		lba2msf(ntohl(cdp->subchan.abslba),
-		    &data->what.position.absaddr.msf.minute,
-		    &data->what.position.absaddr.msf.second,
-		    &data->what.position.absaddr.msf.frame);
-		lba2msf(ntohl(cdp->subchan.rellba),
-		    &data->what.position.reladdr.msf.minute,
-		    &data->what.position.reladdr.msf.second,
-		    &data->what.position.reladdr.msf.frame);
-	    } else {
-		data->what.position.absaddr.lba = cdp->subchan.abslba;
-		data->what.position.reladdr.lba = cdp->subchan.rellba;
-	    }
-	    data->header.audio_status = cdp->subchan.audio_status;
-	    data->what.position.control = cdp->subchan.control & 0xf;
-	    data->what.position.addr_type = cdp->subchan.control >> 4;
-	    data->what.position.track_number = cdp->subchan.track;
-	    data->what.position.index_number = cdp->subchan.indx;
+	    data->header.audio_status = cdp->subchan.header.audio_status;
+	    data->header.data_len[0] = cdp->subchan.header.data_length >> 8;
+	    data->header.data_len[1] = cdp->subchan.header.data_length & 255;
+
+	    switch (format) {
+	    case CD_CURRENT_POSITION:
+		{
+		    if (args->address_format == CD_MSF_FORMAT) {
+			lba2msf(ntohl(cdp->subchan.what.position.abslba),
+			    &data->what.position.absaddr.msf.minute,
+			    &data->what.position.absaddr.msf.second,
+			    &data->what.position.absaddr.msf.frame);
+			lba2msf(ntohl(cdp->subchan.what.position.rellba),
+			    &data->what.position.reladdr.msf.minute,
+			    &data->what.position.reladdr.msf.second,
+			    &data->what.position.reladdr.msf.frame);
+		    } else {
+			data->what.position.absaddr.lba =
+			    cdp->subchan.what.position.abslba;
+			data->what.position.reladdr.lba =
+				cdp->subchan.what.position.rellba;
+		    }
+		    data->what.position.data_format =
+			cdp->subchan.what.position.data_format;
+		    data->what.position.control =
+			cdp->subchan.what.position.control & 0xf;
+		    data->what.position.addr_type =
+			cdp->subchan.what.position.control >> 4;
+		    data->what.position.track_number =
+			cdp->subchan.what.position.track;
+		    data->what.position.index_number =
+			cdp->subchan.what.position.indx;
+		    break;
+		}
+
+	    case CD_MEDIA_CATALOG:
+		{
+		    data->what.media_catalog.data_format =
+			cdp->subchan.what.media_catalog.data_format;
+		    data->what.media_catalog.mc_valid =
+			cdp->subchan.what.media_catalog.mc_valid;
+		    for(i = 0; i < 15; i++)
+			    data->what.media_catalog.mc_number[i] =
+				cdp->subchan.what.media_catalog.mc_number[i];
+		    break;
+		}
+
+	    case CD_TRACK_INFO:
+		{
+		    data->what.track_info.data_format =
+			cdp->subchan.what.track_info.data_format;
+		    data->what.track_info.control =
+			cdp->subchan.what.track_info.control & 0xf;
+		    data->what.track_info.addr_type =
+			cdp->subchan.what.track_info.control >> 4;
+		    data->what.track_info.track_number =
+			cdp->subchan.what.track_info.track;
+		    data->what.track_info.ti_valid =
+			cdp->subchan.what.track_info.ti_valid;
+		    for(i = 0; i < 15; i++)
+			    data->what.track_info.ti_number[i] =
+				cdp->subchan.what.track_info.ti_number[i];
+		    break;
+
+		}
+ 	    }
+
 	    error = copyout(data, args->data, args->data_len);
 	    free(data, M_ACD);
 	    break;
Index: sys/dev/ata/atapi-cd.h
===================================================================
RCS file: /ncvs/src/sys/dev/ata/atapi-cd.h,v
retrieving revision 1.29
diff -u -r1.29 atapi-cd.h
--- sys/dev/ata/atapi-cd.h	4 Feb 2002 19:23:40 -0000	1.29
+++ sys/dev/ata/atapi-cd.h	27 Feb 2002 18:33:11 -0000
@@ -316,16 +316,40 @@
     struct audiopage		au;		/* audio page info */
     struct audiopage		aumask;		/* audio page mask */
     struct cappage		cap;		/* capabilities page info */
-    struct {					/* subchannel info */
-	u_int8_t	void0;
-	u_int8_t	audio_status;
-	u_int16_t	data_length;
-	u_int8_t	data_format;
-	u_int8_t	control;
-	u_int8_t	track;
-	u_int8_t	indx;
-	u_int32_t	abslba;
-	u_int32_t	rellba;
+    struct {
+	struct {
+		u_int8_t	void0;
+		u_int8_t	audio_status;
+		u_int16_t	data_length;
+	} header;
+	union {					/* subchannel info */
+	    struct {
+		u_int8_t	data_format;
+		u_int8_t	control;
+		u_int8_t	track;
+		u_int8_t	indx;
+		u_int32_t	abslba;
+		u_int32_t	rellba;
+	    } position;
+	    struct {
+		u_int8_t	data_format;
+		u_int8_t	:8;
+		u_int8_t	:8;
+		u_int8_t	:8;
+		u_int8_t	:7;
+		u_int8_t	mc_valid:1;
+		u_int8_t	mc_number[15];
+	    } media_catalog;
+	    struct {
+		u_int8_t	data_format;
+		u_int8_t	control;
+		u_int8_t	track;
+		u_int8_t	:8;
+		u_int8_t	:7;
+		u_int8_t	ti_valid:1;
+		u_int8_t	ti_number[15];
+	    } track_info;
+	} what;
     } subchan;
     struct changer		*changer_info;	/* changer info */
     struct acd_softc		**driver;	/* softc's of changer slots */
Index: sys/sys/cdio.h
===================================================================
RCS file: /ncvs/src/sys/sys/cdio.h,v
retrieving revision 1.22
diff -u -r1.22 cdio.h
--- sys/sys/cdio.h	5 Sep 2001 01:22:14 -0000	1.22
+++ sys/sys/cdio.h	21 Feb 2002 00:10:27 -0000
@@ -85,7 +85,8 @@
 
 struct cd_sub_channel_track_info {
         u_char  data_format;
-        u_int   :8;
+	u_int	control:4;
+	u_int	addr_type:4;
         u_char  track_number;
         u_int   :8;
         u_int   :7;






---------------- Second patch set: ------------------------




Index: sys/dev/ata/atapi-cd.c
===================================================================
RCS file: /ncvs/src/sys/dev/ata/atapi-cd.c,v
retrieving revision 1.109
diff -u -r1.109 atapi-cd.c
--- sys/dev/ata/atapi-cd.c	15 Feb 2002 07:08:44 -0000	1.109
+++ sys/dev/ata/atapi-cd.c	27 Feb 2002 18:11:09 -0000
@@ -710,7 +710,7 @@
 	{
 	    struct ioc_read_subchannel *args =
 		(struct ioc_read_subchannel *)addr;
-	    struct cd_sub_channel_info *data;
+	    u_int8_t format;
 	    int8_t ccb[16] = { ATAPI_READ_SUBCHANNEL, 0, 0x40, 1, 0, 0, 0,
 			       sizeof(cdp->subchan)>>8, sizeof(cdp->subchan),
 			       0, 0, 0, 0, 0, 0, 0 };
@@ -721,34 +721,45 @@
 		break;
 	    }
 
+	    format=args->data_format;
+	    if ((format != CD_CURRENT_POSITION) &&
+		(format != CD_MEDIA_CATALOG) && (format != CD_TRACK_INFO)) {
+		error = EINVAL;
+		break;
+	    }
+
+	    ccb[1] = args->address_format & CD_MSF_FORMAT;
+
 	    if ((error = atapi_queue_cmd(cdp->device,ccb,(caddr_t)&cdp->subchan,
 					 sizeof(cdp->subchan), ATPR_F_READ, 10,
 					 NULL, NULL))) {
 		break;
 	    }
-	    data = malloc(sizeof(struct cd_sub_channel_info),
-			  M_ACD, M_NOWAIT | M_ZERO);
 
-	    if (args->address_format == CD_MSF_FORMAT) {
-		lba2msf(ntohl(cdp->subchan.abslba),
-		    &data->what.position.absaddr.msf.minute,
-		    &data->what.position.absaddr.msf.second,
-		    &data->what.position.absaddr.msf.frame);
-		lba2msf(ntohl(cdp->subchan.rellba),
-		    &data->what.position.reladdr.msf.minute,
-		    &data->what.position.reladdr.msf.second,
-		    &data->what.position.reladdr.msf.frame);
-	    } else {
-		data->what.position.absaddr.lba = cdp->subchan.abslba;
-		data->what.position.reladdr.lba = cdp->subchan.rellba;
+	    /*
+	     * Ask for media catalogue or track info only if no audio play
+	     * operation is in progress or the drive will return an error.
+	     */
+
+	    if ((format == CD_MEDIA_CATALOG) || (format == CD_TRACK_INFO)) {
+		if (cdp->subchan.header.audio_status == 0x11) {
+		    error = EINVAL;
+		    break;
+		}
+
+		ccb[3] = format;
+		if (format == CD_TRACK_INFO)
+		    ccb[6] = args->track;
+
+		if ((error = atapi_queue_cmd(cdp->device, ccb,
+					     (caddr_t)&cdp->subchan, 
+					     sizeof(cdp->subchan), ATPR_F_READ,
+					     10, NULL, NULL))) {
+		    break;
+		}
 	    }
-	    data->header.audio_status = cdp->subchan.audio_status;
-	    data->what.position.control = cdp->subchan.control & 0xf;
-	    data->what.position.addr_type = cdp->subchan.control >> 4;
-	    data->what.position.track_number = cdp->subchan.track;
-	    data->what.position.index_number = cdp->subchan.indx;
-	    error = copyout(data, args->data, args->data_len);
-	    free(data, M_ACD);
+
+	    error = copyout(&cdp->subchan, args->data, args->data_len);
 	    break;
 	}
 
Index: sys/dev/ata/atapi-cd.h
===================================================================
RCS file: /ncvs/src/sys/dev/ata/atapi-cd.h,v
retrieving revision 1.29
diff -u -r1.29 atapi-cd.h
--- sys/dev/ata/atapi-cd.h	4 Feb 2002 19:23:40 -0000	1.29
+++ sys/dev/ata/atapi-cd.h	27 Feb 2002 17:48:02 -0000
@@ -316,17 +316,7 @@
     struct audiopage		au;		/* audio page info */
     struct audiopage		aumask;		/* audio page mask */
     struct cappage		cap;		/* capabilities page info */
-    struct {					/* subchannel info */
-	u_int8_t	void0;
-	u_int8_t	audio_status;
-	u_int16_t	data_length;
-	u_int8_t	data_format;
-	u_int8_t	control;
-	u_int8_t	track;
-	u_int8_t	indx;
-	u_int32_t	abslba;
-	u_int32_t	rellba;
-    } subchan;
+    struct cd_sub_channel_info	subchan;	/* from sys/cdio.h. */
     struct changer		*changer_info;	/* changer info */
     struct acd_softc		**driver;	/* softc's of changer slots */
     int				slot;		/* this instance slot number */

Comment 4 Søren Schmidt freebsd_committer

2002-03-16 17:02:30 UTC

State Changed
From-To: open->closed

Second patch committed to -current, MFC shortly.