* [PATCH] cdrom sane fallback vs 2.4.20-pre1
@ 2002-08-11 21:59 Erik Andersen
2002-08-13 2:58 ` Marcelo Tosatti
0 siblings, 1 reply; 12+ messages in thread
From: Erik Andersen @ 2002-08-11 21:59 UTC (permalink / raw)
To: Marcelo Tosatti, lkml, Jens Axboe
Here is an update to cdrom.c. SCSI-II devices are not required
to support the READ_CD packet command. Currently, the cdrom
driver assumes that _all_ READ_CD packet command failures are due
to READ_CD being unsupported. Obviously, there are a million
other reasons for a READ_CD packet command to fail. Here at my
house, the most common reason for READ_CD failures is that my
kids have, once again, scratched up my CDs resulting in bad
sectors. So the drive hits an uncorrectable error and thinks
that READ_CD is unsupported, and then trys again using READ_10
(which takes another a few seconds to fail and, of course, again
returns an L-EC Uncorrectable Error).
This patch teaches cdrom.c to only fall back to READ_10 when
the drive reports that we sent it an invalid command...
-Erik
--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--
--- drivers/cdrom/cdrom.c~ Sun Aug 11 15:37:20 2002
+++ drivers/cdrom/cdrom.c Sun Aug 11 15:37:24 2002
@@ -1916,6 +1916,7 @@
{
struct cdrom_device_ops *cdo = cdi->ops;
struct cdrom_generic_command cgc;
+ struct request_sense sense;
kdev_t dev = cdi->dev;
char buffer[32];
int ret = 0;
@@ -1951,9 +1952,11 @@
cgc.buffer = (char *) kmalloc(blocksize, GFP_KERNEL);
if (cgc.buffer == NULL)
return -ENOMEM;
+ memset(&sense, 0, sizeof(sense));
+ cgc.sense = &sense;
cgc.data_direction = CGC_DATA_READ;
ret = cdrom_read_block(cdi, &cgc, lba, 1, format, blocksize);
- if (ret) {
+ if (ret && sense.sense_key==0x05 && sense.asc==0x20 && sense.ascq==0x00) {
/*
* SCSI-II devices are not required to support
* READ_CD, so let's try switching block size
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-11 21:59 [PATCH] cdrom sane fallback vs 2.4.20-pre1 Erik Andersen
@ 2002-08-13 2:58 ` Marcelo Tosatti
2002-08-13 4:12 ` Erik Andersen
2002-08-13 8:42 ` Jens Axboe
0 siblings, 2 replies; 12+ messages in thread
From: Marcelo Tosatti @ 2002-08-13 2:58 UTC (permalink / raw)
To: Erik Andersen; +Cc: lkml, Jens Axboe
On Sun, 11 Aug 2002, Erik Andersen wrote:
> --- drivers/cdrom/cdrom.c~ Sun Aug 11 15:37:20 2002
> +++ drivers/cdrom/cdrom.c Sun Aug 11 15:37:24 2002
> @@ -1916,6 +1916,7 @@
> {
> struct cdrom_device_ops *cdo = cdi->ops;
> struct cdrom_generic_command cgc;
> + struct request_sense sense;
> kdev_t dev = cdi->dev;
> char buffer[32];
> int ret = 0;
> @@ -1951,9 +1952,11 @@
> cgc.buffer = (char *) kmalloc(blocksize, GFP_KERNEL);
> if (cgc.buffer == NULL)
> return -ENOMEM;
> + memset(&sense, 0, sizeof(sense));
> + cgc.sense = &sense;
> cgc.data_direction = CGC_DATA_READ;
> ret = cdrom_read_block(cdi, &cgc, lba, 1, format, blocksize);
> - if (ret) {
> + if (ret && sense.sense_key==0x05 && sense.asc==0x20 && sense.ascq==0x00) {
Do you really need to hardcode this values ?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-13 2:58 ` Marcelo Tosatti
@ 2002-08-13 4:12 ` Erik Andersen
2002-08-16 3:12 ` Oliver Xymoron
2002-08-13 8:42 ` Jens Axboe
1 sibling, 1 reply; 12+ messages in thread
From: Erik Andersen @ 2002-08-13 4:12 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: lkml, Jens Axboe
On Mon Aug 12, 2002 at 11:58:26PM -0300, Marcelo Tosatti wrote:
>
>
> On Sun, 11 Aug 2002, Erik Andersen wrote:
>
> > --- drivers/cdrom/cdrom.c~ Sun Aug 11 15:37:20 2002
> > +++ drivers/cdrom/cdrom.c Sun Aug 11 15:37:24 2002
> > @@ -1916,6 +1916,7 @@
> > {
> > struct cdrom_device_ops *cdo = cdi->ops;
> > struct cdrom_generic_command cgc;
> > + struct request_sense sense;
> > kdev_t dev = cdi->dev;
> > char buffer[32];
> > int ret = 0;
> > @@ -1951,9 +1952,11 @@
> > cgc.buffer = (char *) kmalloc(blocksize, GFP_KERNEL);
> > if (cgc.buffer == NULL)
> > return -ENOMEM;
> > + memset(&sense, 0, sizeof(sense));
> > + cgc.sense = &sense;
> > cgc.data_direction = CGC_DATA_READ;
> > ret = cdrom_read_block(cdi, &cgc, lba, 1, format, blocksize);
> > - if (ret) {
> > + if (ret && sense.sense_key==0x05 && sense.asc==0x20 && sense.ascq==0x00) {
>
> Do you really need to hardcode this values ?
This allows it to falls back to READ_10 only when the drive
reports "Hey! You gave me an invalid command!" which is the one
and only case when a fall back to READ_10 is appropriate. I am
not aware of any other reason for which a fallback to READ_10 is
useful.
-Erik
--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-13 2:58 ` Marcelo Tosatti
2002-08-13 4:12 ` Erik Andersen
@ 2002-08-13 8:42 ` Jens Axboe
1 sibling, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2002-08-13 8:42 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Erik Andersen, lkml
On Mon, Aug 12 2002, Marcelo Tosatti wrote:
>
>
> On Sun, 11 Aug 2002, Erik Andersen wrote:
>
> > --- drivers/cdrom/cdrom.c~ Sun Aug 11 15:37:20 2002
> > +++ drivers/cdrom/cdrom.c Sun Aug 11 15:37:24 2002
> > @@ -1916,6 +1916,7 @@
> > {
> > struct cdrom_device_ops *cdo = cdi->ops;
> > struct cdrom_generic_command cgc;
> > + struct request_sense sense;
> > kdev_t dev = cdi->dev;
> > char buffer[32];
> > int ret = 0;
> > @@ -1951,9 +1952,11 @@
> > cgc.buffer = (char *) kmalloc(blocksize, GFP_KERNEL);
> > if (cgc.buffer == NULL)
> > return -ENOMEM;
> > + memset(&sense, 0, sizeof(sense));
> > + cgc.sense = &sense;
> > cgc.data_direction = CGC_DATA_READ;
> > ret = cdrom_read_block(cdi, &cgc, lba, 1, format, blocksize);
> > - if (ret) {
> > + if (ret && sense.sense_key==0x05 && sense.asc==0x20 && sense.ascq==0x00) {
>
> Do you really need to hardcode this values ?
the values above are well known to anyone familiar with atapi and/or
scsi, so it's not a worry. the patch looks good to me, I can recommend
it for inclusion.
--
Jens Axboe
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-13 4:12 ` Erik Andersen
@ 2002-08-16 3:12 ` Oliver Xymoron
0 siblings, 0 replies; 12+ messages in thread
From: Oliver Xymoron @ 2002-08-16 3:12 UTC (permalink / raw)
To: Erik Andersen, Marcelo Tosatti, lkml, Jens Axboe
On Mon, Aug 12, 2002 at 10:12:44PM -0600, Erik Andersen wrote:
> On Mon Aug 12, 2002 at 11:58:26PM -0300, Marcelo Tosatti wrote:
> >
> >
> > On Sun, 11 Aug 2002, Erik Andersen wrote:
> >
> > > +++ drivers/cdrom/cdrom.c Sun Aug 11 15:37:24 2002
> > > @@ -1916,6 +1916,7 @@
> > > {
> > > struct cdrom_device_ops *cdo = cdi->ops;
> > > struct cdrom_generic_command cgc;
> > > + struct request_sense sense;
> > > kdev_t dev = cdi->dev;
> > > char buffer[32];
> > > int ret = 0;
> > > @@ -1951,9 +1952,11 @@
> > > cgc.buffer = (char *) kmalloc(blocksize, GFP_KERNEL);
> > > if (cgc.buffer == NULL)
> > > return -ENOMEM;
> > > + memset(&sense, 0, sizeof(sense));
> > > + cgc.sense = &sense;
> > > cgc.data_direction = CGC_DATA_READ;
> > > ret = cdrom_read_block(cdi, &cgc, lba, 1, format, blocksize);
> > > - if (ret) {
> > > + if (ret && sense.sense_key==0x05 && sense.asc==0x20 && sense.ascq==0x00) {
> >
> > Do you really need to hardcode this values ?
>
> This allows it to falls back to READ_10 only when the drive
> reports "Hey! You gave me an invalid command!" which is the one
> and only case when a fall back to READ_10 is appropriate. I am
> not aware of any other reason for which a fallback to READ_10 is
> useful.
A comment to that effect would be useful. Not so much the
interpretation of the numbers (which are easy enough to look up) but
the rationale.
--
"Love the dolphins," she advised him. "Write by W.A.S.T.E.."
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-13 16:37 ` Jens Axboe
@ 2002-08-13 17:10 ` James Bottomley
0 siblings, 0 replies; 12+ messages in thread
From: James Bottomley @ 2002-08-13 17:10 UTC (permalink / raw)
To: Jens Axboe; +Cc: Randy.Dunlap, James Bottomley, Marcelo Tosatti, linux-kernel
axboe@suse.de said:
> I wouldn't mind unifying lots of the atapi and cdrom stuff. Most of
> the stuff I took care off was atapi and scsi cdrom unification, but it
> could be taken a step further.
Yes, perhaps in conjunction with a rationalisation of command packet interface
to struct request and the block queue that made no prejudgement of what
actually the command was packetised for. One modification I was thinking of
would be simply to make struct_request.cmd a pointer rather than a char array.
That way, the prep functions can allocate it and fill it in with an arbitrary
packet command to be interpreted by the downstream driver (it will also get us
out of the command size limitation problem---of course, the SCSI committee
would never consider another command size increase...)
> Come to think of it, the reason why I didn't completey unify SCSI
> request sense (eg) with cdrom stuff was that you quickly run into all
> sorts of ugliness with the various scsi versions, atapi version,
> reserved fields, added fields, etc. Something that looks good and
> should be good, suddenly breaks cdrom model XYZ that used to work
> (perhaps by chance) and still _ought_ to work.
In fact, it would be a particularly deep and rancid can of worms?
> But hey, knock yourself out :-)
Sure, by the time you've got a rational and unified interface that looks good,
you'll probably have re-written big chunks of scsi and cdrom. However, if you
can come up with a good scheme for the sense codes, it would be nice (the ASC
ASCQ code combinations are in a table that covers thirteen pages of close
printed type in the SCSI spec---constants.c only has the more frequently used
ones).
James
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-13 16:25 ` Randy.Dunlap
@ 2002-08-13 16:37 ` Jens Axboe
2002-08-13 17:10 ` James Bottomley
0 siblings, 1 reply; 12+ messages in thread
From: Jens Axboe @ 2002-08-13 16:37 UTC (permalink / raw)
To: Randy.Dunlap; +Cc: James Bottomley, Marcelo Tosatti, linux-kernel
On Tue, Aug 13 2002, Randy.Dunlap wrote:
> On Tue, 13 Aug 2002, James Bottomley wrote:
>
> | rddunlap@osdl.org said:
> | > and that's precisely the wrong attitude IMO.
> |
> | I wasn't expressing an opinion, just stating what could and could not be done
> | in 2.4.
>
> I guess that at least Jens and I (not trying to speak for Jens)
> see it as a style issue and somewhat as an education issue.
> At least we both used /IMO/i.
Right!
> | > I was glad to see that Marcelo asked about the hardcoded values. They
> | > hurt.
> |
> | Well, this is a rather big and particularly rancid can of worms. If you look
> | a little further, you'll see that cdrom.h has its own definition of the
> | (effectively SCSI) struct request_sense that sr.c uses, yet the sense key is
> | defined in scsi/scsi.h. Then you notice that cdrom.h also duplicates all of
> | the scsi commands with a GPCMD_ prefix.
> |
> | If you'd like to take this particular can of worms off somewhere, clean it out
> | and return it neatly labelled, I'd be more than grateful...just don't take the
> | lid off too close to me.
>
> I'm not sure that it could ever get by Jens, but I'll take a look at it.
You make me sound like an old grump [1] :-)
I wouldn't mind unifying lots of the atapi and cdrom stuff. Most of the
stuff I took care off was atapi and scsi cdrom unification, but it could
be taken a step further.
GPCMD_ means general packet command, and signifies a command that is the
same for atapi and scsi.
Come to think of it, the reason why I didn't completey unify SCSI
request sense (eg) with cdrom stuff was that you quickly run into all
sorts of ugliness with the various scsi versions, atapi version,
reserved fields, added fields, etc. Something that looks good and should
be good, suddenly breaks cdrom model XYZ that used to work (perhaps by
chance) and still _ought_ to work.
But hey, knock yourself out :-)
[1] I'm not old
--
Jens Axboe
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-13 16:21 ` James Bottomley
@ 2002-08-13 16:25 ` Randy.Dunlap
2002-08-13 16:37 ` Jens Axboe
0 siblings, 1 reply; 12+ messages in thread
From: Randy.Dunlap @ 2002-08-13 16:25 UTC (permalink / raw)
To: James Bottomley; +Cc: Marcelo Tosatti, linux-kernel
On Tue, 13 Aug 2002, James Bottomley wrote:
| rddunlap@osdl.org said:
| > and that's precisely the wrong attitude IMO.
|
| I wasn't expressing an opinion, just stating what could and could not be done
| in 2.4.
I guess that at least Jens and I (not trying to speak for Jens)
see it as a style issue and somewhat as an education issue.
At least we both used /IMO/i.
| > I was glad to see that Marcelo asked about the hardcoded values. They
| > hurt.
|
| Well, this is a rather big and particularly rancid can of worms. If you look
| a little further, you'll see that cdrom.h has its own definition of the
| (effectively SCSI) struct request_sense that sr.c uses, yet the sense key is
| defined in scsi/scsi.h. Then you notice that cdrom.h also duplicates all of
| the scsi commands with a GPCMD_ prefix.
|
| If you'd like to take this particular can of worms off somewhere, clean it out
| and return it neatly labelled, I'd be more than grateful...just don't take the
| lid off too close to me.
I'm not sure that it could ever get by Jens, but I'll take a look at it.
--
~Randy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-13 15:48 ` Randy.Dunlap
2002-08-13 16:14 ` Jens Axboe
@ 2002-08-13 16:21 ` James Bottomley
2002-08-13 16:25 ` Randy.Dunlap
1 sibling, 1 reply; 12+ messages in thread
From: James Bottomley @ 2002-08-13 16:21 UTC (permalink / raw)
To: Randy.Dunlap
Cc: James Bottomley, Marcelo Tosatti, linux-kernel, Erik Andersen
rddunlap@osdl.org said:
> and that's precisely the wrong attitude IMO.
I wasn't expressing an opinion, just stating what could and could not be done
in 2.4.
> I was glad to see that Marcelo asked about the hardcoded values. They
> hurt.
Well, this is a rather big and particularly rancid can of worms. If you look
a little further, you'll see that cdrom.h has its own definition of the
(effectively SCSI) struct request_sense that sr.c uses, yet the sense key is
defined in scsi/scsi.h. Then you notice that cdrom.h also duplicates all of
the scsi commands with a GPCMD_ prefix.
If you'd like to take this particular can of worms off somewhere, clean it out
and return it neatly labelled, I'd be more than grateful...just don't take the
lid off too close to me.
James
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-13 15:48 ` Randy.Dunlap
@ 2002-08-13 16:14 ` Jens Axboe
2002-08-13 16:21 ` James Bottomley
1 sibling, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2002-08-13 16:14 UTC (permalink / raw)
To: Randy.Dunlap
Cc: James Bottomley, Marcelo Tosatti, linux-kernel, Erik Andersen
On Tue, Aug 13 2002, Randy.Dunlap wrote:
> On Tue, 13 Aug 2002, James Bottomley wrote:
>
> | > > - if (ret) {
> | > > + if (ret && sense.sense_key==0x05 && sense.asc==0x20 && sense.ascq==0x00) {
> | >
> | > Do you really need to hardcode this values ?
> |
> | We have no #defines for the asc and ascq codes (they are interpreted in
> | constants.c but the values are hardcoded in there too). There is a #define
> | for sense_key 0x05 as ILLEGAL_REQUEST in scsi/scsi.h, but these #defines have
> | annoyed a lot of people by being rather namespace polluting.
>
> and that's precisely the wrong attitude IMO.
>
> I was glad to see that Marcelo asked about the hardcoded values.
> They hurt.
I usually find it a hell of a lot easier remembering that 5/20/00 is
illegal opcode, 5/24/00 is illegal field, etc. There are just too many
of these to be named sanely. sense_key can be done, agreed, but asc and
ascq just gets silly imo, and it makes it harder to read for those that
know the codes. Encouraging others to look up the values (it's not hard,
you can see it's asc and ascq and it relates to sense info) does
definitely not hurt, they might pick up something else along the way.
--
Jens Axboe
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
2002-08-13 14:13 James Bottomley
@ 2002-08-13 15:48 ` Randy.Dunlap
2002-08-13 16:14 ` Jens Axboe
2002-08-13 16:21 ` James Bottomley
0 siblings, 2 replies; 12+ messages in thread
From: Randy.Dunlap @ 2002-08-13 15:48 UTC (permalink / raw)
To: James Bottomley; +Cc: Marcelo Tosatti, linux-kernel, Erik Andersen
On Tue, 13 Aug 2002, James Bottomley wrote:
| > > - if (ret) {
| > > + if (ret && sense.sense_key==0x05 && sense.asc==0x20 && sense.ascq==0x00) {
| >
| > Do you really need to hardcode this values ?
|
| We have no #defines for the asc and ascq codes (they are interpreted in
| constants.c but the values are hardcoded in there too). There is a #define
| for sense_key 0x05 as ILLEGAL_REQUEST in scsi/scsi.h, but these #defines have
| annoyed a lot of people by being rather namespace polluting.
and that's precisely the wrong attitude IMO.
I was glad to see that Marcelo asked about the hardcoded values.
They hurt.
--
~Randy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cdrom sane fallback vs 2.4.20-pre1
@ 2002-08-13 14:13 James Bottomley
2002-08-13 15:48 ` Randy.Dunlap
0 siblings, 1 reply; 12+ messages in thread
From: James Bottomley @ 2002-08-13 14:13 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: linux-kernel, James.Bottomley, Erik Andersen
> > - if (ret) {
> > + if (ret && sense.sense_key==0x05 && sense.asc==0x20 && sense.ascq==0x00) {
>
> Do you really need to hardcode this values ?
We have no #defines for the asc and ascq codes (they are interpreted in
constants.c but the values are hardcoded in there too). There is a #define
for sense_key 0x05 as ILLEGAL_REQUEST in scsi/scsi.h, but these #defines have
annoyed a lot of people by being rather namespace polluting.
James
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2002-08-16 3:08 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2002-08-11 21:59 [PATCH] cdrom sane fallback vs 2.4.20-pre1 Erik Andersen
2002-08-13 2:58 ` Marcelo Tosatti
2002-08-13 4:12 ` Erik Andersen
2002-08-16 3:12 ` Oliver Xymoron
2002-08-13 8:42 ` Jens Axboe
2002-08-13 14:13 James Bottomley
2002-08-13 15:48 ` Randy.Dunlap
2002-08-13 16:14 ` Jens Axboe
2002-08-13 16:21 ` James Bottomley
2002-08-13 16:25 ` Randy.Dunlap
2002-08-13 16:37 ` Jens Axboe
2002-08-13 17:10 ` James Bottomley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).