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

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

[Borislav Petkov]

Remove unused SECTOR_SIZE while at it.

There should be no functional change resulting from this patch.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 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

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

[Bartlomiej Zolnierkiewicz]

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 <petkovbb@gmail.com>

[...]

> @@ -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?

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

[Borislav Petkov]

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 <petkovbb@gmail.com>
> 
> [...]
> 
> > @@ -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.