2009-04-05 09:16:39

by Borislav Petkov

[permalink] [raw]
Subject: [PATCH 2/2] ide-cd: cdrom_decode_status: use return codes instead of naked numbers

Remove unused SECTOR_SIZE while at it.

There should be no functional change resulting from this patch.

Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/ide/ide-cd.c | 22 ++++++++--------------
drivers/ide/ide-cd.h | 12 ++++++------
2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 2aa13d8..1dc61fc 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -301,12 +301,6 @@ static int ide_cd_breathe(ide_drive_t *drive, struct request *rq)
}
}

-/**
- * Returns:
- * 0: if the request should be continued.
- * 1: if the request will be going through error recovery.
- * 2: if the request should be ended.
- */
static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
{
ide_hwif_t *hwif = drive->hwif;
@@ -329,7 +323,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
* Just give up.
*/
rq->cmd_flags |= REQ_FAILED;
- return 2;
+ return REQ_FAIL;
}

/* if we have an error, pass CHECK_CONDITION as the SCSI status byte */
@@ -343,7 +337,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
case NOT_READY:
if (blk_fs_request(rq) && rq_data_dir(rq) == WRITE) {
if (ide_cd_breathe(drive, rq))
- return 1;
+ return REQ_RECOVER;
} else {
cdrom_saw_media_change(drive);

@@ -357,7 +351,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
cdrom_saw_media_change(drive);

if (blk_fs_request(rq) == 0)
- return 0;
+ return REQ_CONT;

/*
* Arrange to retry the request but be sure to give up if we've
@@ -409,7 +403,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
if (err & ~ATA_ABORTED) {
/* go to the default handler for other errors */
ide_error(drive, "cdrom_decode_status", stat);
- return 1;
+ return REQ_RECOVER;
} else if (++rq->errors > ERROR_MAX)
/* we've racked up too many retries, abort */
do_end_request = 1;
@@ -431,7 +425,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
/* if we got a CHECK_CONDITION status, queue a request sense command */
if (stat & ATA_ERR)
cdrom_queue_request_sense(drive, NULL, NULL);
- return 1;
+ return REQ_RECOVER;

end_request:
if (stat & ATA_ERR) {
@@ -445,9 +439,9 @@ end_request:
hwif->rq = NULL;

cdrom_queue_request_sense(drive, rq->sense, rq);
- return 1;
- } else
- return 2;
+ return REQ_RECOVER;
+ }
+ return REQ_FAIL;
}

/*
diff --git a/drivers/ide/ide-cd.h b/drivers/ide/ide-cd.h
index 1d97101..7e17e70 100644
--- a/drivers/ide/ide-cd.h
+++ b/drivers/ide/ide-cd.h
@@ -18,15 +18,15 @@

#define ATAPI_WAIT_WRITE_BUSY (10 * HZ)

-/************************************************************************/
-
-#define SECTOR_BITS 9
-#ifndef SECTOR_SIZE
-#define SECTOR_SIZE (1 << SECTOR_BITS)
-#endif
+#define SECTOR_BITS 9
#define SECTORS_PER_FRAME (CD_FRAMESIZE >> SECTOR_BITS)
#define SECTOR_BUFFER_SIZE (CD_FRAMESIZE * 32)

+/* internal decode_status codes */
+#define REQ_CONT 0
+#define REQ_RECOVER 1
+#define REQ_FAIL 2
+
/* Capabilities Page size including 8 bytes of Mode Page Header */
#define ATAPI_CAPABILITIES_PAGE_SIZE (8 + 20)
#define ATAPI_CAPABILITIES_PAGE_PAD_SIZE 4
--
1.6.2.1


Subject: Re: [PATCH 2/2] ide-cd: cdrom_decode_status: use return codes instead of naked numbers

On Sunday 05 April 2009, Borislav Petkov wrote:
> Remove unused SECTOR_SIZE while at it.
>
> There should be no functional change resulting from this patch.
>
> Signed-off-by: Borislav Petkov <[email protected]>

[...]

> @@ -431,7 +425,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
> /* if we got a CHECK_CONDITION status, queue a request sense command */
> if (stat & ATA_ERR)
> cdrom_queue_request_sense(drive, NULL, NULL);
> - return 1;
> + return REQ_RECOVER;
>
> end_request:
> if (stat & ATA_ERR) {
> @@ -445,9 +439,9 @@ end_request:
> hwif->rq = NULL;
>
> cdrom_queue_request_sense(drive, rq->sense, rq);
> - return 1;
> - } else
> - return 2;
> + return REQ_RECOVER;
> + }
> + return REQ_FAIL;
> }

Could it be that cdrom_newpc_intr() chunk got lost somewhere along the way,
IIRC it was there?

[...]

> +/* internal decode_status codes */
> +#define REQ_CONT 0
> +#define REQ_RECOVER 1
> +#define REQ_FAIL 2

Did you notice my comments about REQ_* in previous mail?

2009-04-07 06:39:47

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH 2/2] ide-cd: cdrom_decode_status: use return codes instead of naked numbers

Hi,

On Mon, Apr 06, 2009 at 11:00:15PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 05 April 2009, Borislav Petkov wrote:
> > Remove unused SECTOR_SIZE while at it.
> >
> > There should be no functional change resulting from this patch.
> >
> > Signed-off-by: Borislav Petkov <[email protected]>
>
> [...]
>
> > @@ -431,7 +425,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
> > /* if we got a CHECK_CONDITION status, queue a request sense command */
> > if (stat & ATA_ERR)
> > cdrom_queue_request_sense(drive, NULL, NULL);
> > - return 1;
> > + return REQ_RECOVER;
> >
> > end_request:
> > if (stat & ATA_ERR) {
> > @@ -445,9 +439,9 @@ end_request:
> > hwif->rq = NULL;
> >
> > cdrom_queue_request_sense(drive, rq->sense, rq);
> > - return 1;
> > - } else
> > - return 2;
> > + return REQ_RECOVER;
> > + }
> > + return REQ_FAIL;
> > }
>
> Could it be that cdrom_newpc_intr() chunk got lost somewhere along the way,
> IIRC it was there?

That I dropped :), my bad.

> > +/* internal decode_status codes */
> > +#define REQ_CONT 0
> > +#define REQ_RECOVER 1
> > +#define REQ_FAIL 2
>
> Did you notice my comments about REQ_* in previous mail?

Yep, about those - is there a valid reason for calling them IDE_RQ_*?
I know REQ_* is too generic but since they're private to the driver it
really is the only proper naming you _can_ have without adding too much
information to the name. FWIW, SCSI has even more generic names for them
- SUCCESS, FAILED, etc. And the IDE_* prefix is only then called for
when they're going to be visible/used by some other IDE parts. So IMHO
REQ_* or RQ_* is actually better in this case.

--
Regards/Gruss,
Boris.