Date: Tue, 17 Apr 2001 14:41:37 +0200 (CEST) From: un1i@rz.uni-karlsruhe.de To: FreeBSD-gnats-submit@freebsd.org Subject: kern/26644: [PATCH] ATA/ATAPI driver doesn't implement CDIOCREADSUBCHANNEL correctly Message-ID: <200104171241.f3HCfbd01363@i609.hadiko.de>
next in thread | raw e-mail | index | archive | help
>Number: 26644
>Category: kern
>Synopsis: [PATCH] ATA/ATAPI driver doesn't implement CDIOCREADSUBCHANNEL correctly
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Tue Apr 17 05:50:01 PDT 2001
>Closed-Date:
>Last-Modified:
>Originator: Philipp Mergenthaler
>Release: FreeBSD 5.0-CURRENT i386
>Organization:
Univeristy of Karlsruhe
>Environment:
System: FreeBSD i609.hadiko.de 5.0-CURRENT FreeBSD 5.0-CURRENT #368: Tue Apr 17 12:35:00 CEST 2001 p@i609.hadiko.de:/usr/obj/usr/src/sys/I609 i386
>Description:
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.
>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.
>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.
Index: atapi-cd.c
===================================================================
RCS file: /ncvs/src/sys/dev/ata/atapi-cd.c,v
retrieving revision 1.88
diff -u -r1.88 atapi-cd.c
--- atapi-cd.c 2001/04/05 11:17:33 1.88
+++ atapi-cd.c 2001/04/17 10:31:09
@@ -726,6 +726,8 @@
struct ioc_read_subchannel *args =
(struct ioc_read_subchannel *)addr;
struct cd_sub_channel_info data;
+ u_int8_t format;
+ int i;
int len = args->data_len;
int32_t abslba, rellba;
int8_t ccb[16] = { ATAPI_READ_SUBCHANNEL, 0, 0x40, 1, 0, 0, 0,
@@ -738,31 +740,108 @@
break;
}
+ format=args->data_format;
+ if ((format != CD_CURRENT_POSITION) &&
+ (format != CD_MEDIA_CATALOG) && (format != CD_TRACK_INFO)) {
+ error = EINVAL;
+ break;
+ }
+
if ((error = atapi_queue_cmd(cdp->atp, ccb, (caddr_t)&cdp->subchan,
sizeof(cdp->subchan), ATPR_F_READ, 10,
NULL, NULL))) {
break;
}
- abslba = cdp->subchan.abslba;
- rellba = cdp->subchan.rellba;
- if (args->address_format == CD_MSF_FORMAT) {
- lba2msf(ntohl(abslba),
- &data.what.position.absaddr.msf.minute,
- &data.what.position.absaddr.msf.second,
- &data.what.position.absaddr.msf.frame);
- lba2msf(ntohl(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 = abslba;
- data.what.position.reladdr.lba = 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->atp, ccb,
+ (caddr_t)&cdp->subchan,
+ sizeof(cdp->subchan), ATPR_F_READ,
+ 10, NULL, NULL))) {
+ break;
+ }
+ }
+
+ 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:
+ {
+ abslba = cdp->subchan.what.position.abslba;
+ rellba = cdp->subchan.what.position.rellba;
+ if (args->address_format == CD_MSF_FORMAT) {
+ lba2msf(ntohl(abslba),
+ &data.what.position.absaddr.msf.minute,
+ &data.what.position.absaddr.msf.second,
+ &data.what.position.absaddr.msf.frame);
+ lba2msf(ntohl(rellba - 150),
+ &data.what.position.reladdr.msf.minute,
+ &data.what.position.reladdr.msf.second,
+ &data.what.position.reladdr.msf.frame);
+ } else {
+ data.what.position.absaddr.lba = abslba;
+ data.what.position.reladdr.lba = 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;
+
+ }
}
- 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, len);
break;
}
Index: atapi-cd.h
===================================================================
RCS file: /ncvs/src/sys/dev/ata/atapi-cd.h,v
retrieving revision 1.25
diff -u -r1.25 atapi-cd.h
--- atapi-cd.h 2001/03/21 14:59:38 1.25
+++ atapi-cd.h 2001/04/17 10:19:46
@@ -311,16 +311,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: atapi-cd.c
===================================================================
RCS file: /ncvs/src/sys/dev/ata/atapi-cd.c,v
retrieving revision 1.88
diff -u -r1.88 atapi-cd.c
--- atapi-cd.c 2001/04/05 11:17:33 1.88
+++ atapi-cd.c 2001/04/17 12:25:23
@@ -725,45 +725,57 @@
{
struct ioc_read_subchannel *args =
(struct ioc_read_subchannel *)addr;
- struct cd_sub_channel_info data;
+ u_int8_t format;
int len = args->data_len;
- int32_t abslba, rellba;
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 };
- if (len > sizeof(data) ||
+ if (len > sizeof(struct cd_sub_channel_info) ||
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;
+ }
+
+ ccb[1] = args->address_format & CD_MSF_FORMAT;
+
if ((error = atapi_queue_cmd(cdp->atp, ccb, (caddr_t)&cdp->subchan,
sizeof(cdp->subchan), ATPR_F_READ, 10,
NULL, NULL))) {
break;
}
- abslba = cdp->subchan.abslba;
- rellba = cdp->subchan.rellba;
- if (args->address_format == CD_MSF_FORMAT) {
- lba2msf(ntohl(abslba),
- &data.what.position.absaddr.msf.minute,
- &data.what.position.absaddr.msf.second,
- &data.what.position.absaddr.msf.frame);
- lba2msf(ntohl(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 = abslba;
- data.what.position.reladdr.lba = 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->atp, 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, len);
+
+ error = copyout(&cdp->subchan, args->data, len);
break;
}
Index: atapi-cd.h
===================================================================
RCS file: /ncvs/src/sys/dev/ata/atapi-cd.h,v
retrieving revision 1.25
diff -u -r1.25 atapi-cd.h
--- atapi-cd.h 2001/03/21 14:59:38 1.25
+++ atapi-cd.h 2001/04/17 10:19:46
@@ -311,16 +311,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 */
>Release-Note:
>Audit-Trail:
>Unformatted:
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200104171241.f3HCfbd01363>