- have (almost) equal handling of commands based solely on sense_key
- carve out an ide_cd_breathe()-helper for fs write requests
- make code more readable
There should be no functional change resulting from this patch.
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/ide/ide-cd.c | 238 ++++++++++++++++++++++---------------------------
1 files changed, 107 insertions(+), 131 deletions(-)
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index a4afd90..8387dbf 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -266,6 +266,38 @@ static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
}
/*
+ * Allow the drive 5 seconds to recover; some devices will return NOT_READY
+ * while flushing data from cache
+ */
+static int ide_cd_breathe(ide_drive_t *drive, struct request *rq)
+{
+
+ struct cdrom_info *info = drive->driver_data;
+
+ if (!rq->errors)
+ info->write_timeout = jiffies + ATAPI_WAIT_WRITE_BUSY;
+
+ rq->errors = 1;
+
+ if (time_after(jiffies, info->write_timeout))
+ return 1;
+ else {
+ struct request_queue *q = drive->queue;
+ unsigned long flags;
+
+ /*
+ * take a breather relying on the unplug timer to kick us again
+ */
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ blk_plug_device(q);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ return 0;
+ }
+}
+
+/*
* Returns:
* 0: if the request should be continued.
* 1: if the request will be going through error recovery.
@@ -275,14 +307,15 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
{
ide_hwif_t *hwif = drive->hwif;
struct request *rq = hwif->rq;
- int err, sense_key;
+ int err, sense_key, do_end_request;
/* get the IDE error register */
err = ide_read_error(drive);
sense_key = err >> 4;
- ide_debug_log(IDE_DBG_RQ, "cmd[0]: 0x%x, rq->cmd_type: 0x%x, err: 0x%x",
- rq->cmd[0], rq->cmd_type, err);
+ ide_debug_log(IDE_DBG_RQ, "cmd: 0x%x, rq->cmd_type: 0x%x, err: 0x%x, "
+ "stat 0x%x",
+ rq->cmd[0], rq->cmd_type, err, stat);
if (blk_sense_request(rq)) {
/*
@@ -292,151 +325,93 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
*/
rq->cmd_flags |= REQ_FAILED;
return 2;
- } else if (blk_pc_request(rq) || rq->cmd_type == REQ_TYPE_ATA_PC) {
- /* All other functions, except for READ. */
+ }
- /*
- * if we have an error, pass back CHECK_CONDITION as the
- * scsi status byte
- */
- if (blk_pc_request(rq) && !rq->errors)
- rq->errors = SAM_STAT_CHECK_CONDITION;
+ /* if we got an error, pass CHECK_CONDITION as the scsi status byte */
+ if (blk_pc_request(rq) && !rq->errors)
+ rq->errors = SAM_STAT_CHECK_CONDITION;
- /* check for tray open */
- if (sense_key == NOT_READY) {
- cdrom_saw_media_change(drive);
- } else if (sense_key == UNIT_ATTENTION) {
- /* check for media change */
+ if (blk_noretry_request(rq))
+ do_end_request = 1;
+
+ switch (sense_key) {
+ case NOT_READY:
+ if (blk_fs_request(rq) && (rq_data_dir(rq) == WRITE)) {
+ if (!ide_cd_breathe(drive, rq))
+ return 1;
+ } else {
cdrom_saw_media_change(drive);
- return 0;
- } else if (sense_key == ILLEGAL_REQUEST &&
- rq->cmd[0] == GPCMD_START_STOP_UNIT) {
- /*
- * Don't print error message for this condition--
- * SFF8090i indicates that 5/24/00 is the correct
- * response to a request to close the tray if the
- * drive doesn't have that capability.
- * cdrom_log_sense() knows this!
- */
- } else if (!(rq->cmd_flags & REQ_QUIET)) {
- /* otherwise, print an error */
- ide_dump_status(drive, "packet command error", stat);
+ printk(KERN_ERR PFX "%s: tray open\n", drive->name);
}
+ do_end_request = 1;
+ break;
- rq->cmd_flags |= REQ_FAILED;
+ case UNIT_ATTENTION:
+ cdrom_saw_media_change(drive);
+ if (blk_pc_request(rq) || rq->cmd_type == REQ_TYPE_ATA_PC)
+ return 0;
+ break;
+ case ILLEGAL_REQUEST:
+ case DATA_PROTECT:
/*
- * instead of playing games with moving completions around,
- * remove failed request completely and end it when the
- * request sense has completed
+ * Don't print error message for this condition since SFF8090i
+ * indicates that 5/24/00 is the correct response to a request
+ * to close the tray if the drive doesn't have that capability.
+ *
+ * cdrom_log_sense() knows this!
*/
- goto end_request;
-
- } else if (blk_fs_request(rq)) {
- int do_end_request = 0;
-
- /* handle errors from READ and WRITE requests */
-
- if (blk_noretry_request(rq))
+ if (rq->cmd[0] != GPCMD_START_STOP_UNIT) {
+ ide_dump_status(drive, "command error", stat);
do_end_request = 1;
+ }
+ break;
- if (sense_key == NOT_READY) {
- /* tray open */
- if (rq_data_dir(rq) == READ) {
- cdrom_saw_media_change(drive);
-
- /* fail the request */
- printk(KERN_ERR PFX "%s: tray open\n",
- drive->name);
- do_end_request = 1;
- } else {
- struct cdrom_info *info = drive->driver_data;
+ case MEDIUM_ERROR:
+ /*
+ * No point in re-trying a zillion times on a bad sector. If we
+ * got here the error is not correctable.
+ */
+ ide_dump_status(drive, "media error (bad sector)", stat);
+ do_end_request = 1;
+ break;
- /*
- * Allow the drive 5 seconds to recover, some
- * devices will return this error while flushing
- * data from cache.
- */
- if (!rq->errors)
- info->write_timeout = jiffies +
- ATAPI_WAIT_WRITE_BUSY;
- rq->errors = 1;
- if (time_after(jiffies, info->write_timeout))
- do_end_request = 1;
- else {
- struct request_queue *q = drive->queue;
- unsigned long flags;
-
- /*
- * take a breather relying on the unplug
- * timer to kick us again
- */
- spin_lock_irqsave(q->queue_lock, flags);
- blk_plug_device(q);
- spin_unlock_irqrestore(q->queue_lock, flags);
-
- return 1;
- }
- }
- } else if (sense_key == UNIT_ATTENTION) {
- /* media change */
- cdrom_saw_media_change(drive);
+ case BLANK_CHECK:
+ /* disk appears blank ?? */
+ ide_dump_status(drive, "media error (blank)", stat);
+ do_end_request = 1;
+ break;
- /*
- * Arrange to retry the request but be sure to give up
- * if we've retried too many times.
- */
- if (++rq->errors > ERROR_MAX)
- do_end_request = 1;
- } else if (sense_key == ILLEGAL_REQUEST ||
- sense_key == DATA_PROTECT) {
- /*
- * No point in retrying after an illegal request or data
- * protect error.
- */
- ide_dump_status(drive, "command error", stat);
- do_end_request = 1;
- } else if (sense_key == MEDIUM_ERROR) {
- /*
- * No point in re-trying a zillion times on a bad
- * sector. If we got here the error is not correctable.
- */
- ide_dump_status(drive, "media error (bad sector)",
- stat);
- do_end_request = 1;
- } else if (sense_key == BLANK_CHECK) {
- /* disk appears blank ?? */
- ide_dump_status(drive, "media error (blank)", stat);
- do_end_request = 1;
- } else if ((err & ~ATA_ABORTED) != 0) {
+ default:
+ if (err & ~ATA_ABORTED) {
/* go to the default handler for other errors */
ide_error(drive, "cdrom_decode_status", stat);
return 1;
- } else if ((++rq->errors > ERROR_MAX)) {
- /* we've racked up too many retries, abort */
- do_end_request = 1;
}
- /*
- * End a request through request sense analysis when we have
- * sense data. We need this in order to perform end of media
- * processing.
- */
- if (do_end_request)
- goto end_request;
+ if (!(rq->cmd_flags & REQ_QUIET)) {
+ ide_dump_status(drive, "command error", stat);
+ blk_dump_rq_flags(rq, PFX "failing rq");
+ }
+ do_end_request = 1;
+ break;
+ }
- /*
- * 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;
- } else {
- blk_dump_rq_flags(rq, PFX "bad rq");
- return 2;
+ /* we've racked up too many retries, abort */
+ if (++rq->errors > ERROR_MAX)
+ do_end_request = 1;
+
+ if (do_end_request) {
+ rq->cmd_flags |= REQ_FAILED;
+ goto end_request;
}
+ /* 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;
+
end_request:
if (stat & ATA_ERR) {
struct request_queue *q = drive->queue;
@@ -614,14 +589,15 @@ static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
struct request *rq = hwif->rq;
ide_expiry_t *expiry = NULL;
int dma_error = 0, dma, thislen, uptodate = 0;
- int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc, nsectors;
+ int write, uninitialized_var(rc), nsectors;
int sense = blk_sense_request(rq);
unsigned int timeout;
u16 len;
u8 ireason, stat;
- ide_debug_log(IDE_DBG_PC, "cmd[0]: 0x%x, write: 0x%x",
- rq->cmd[0], write);
+ write = (rq_data_dir(rq) == WRITE) ? 1 : 0;
+
+ ide_debug_log(IDE_DBG_PC, "cmd: 0x%x, write: 0x%x", rq->cmd[0], write);
/* check for errors */
dma = drive->dma;
--
1.6.2.1
On Thursday 02 April 2009, Borislav Petkov wrote:
> - have (almost) equal handling of commands based solely on sense_key
I'm having a VERY hard time trying to review this patch because at
the same time that codepaths were merged if()s were replaced by switch()
which in turn resulted in change of intendation... on top of that
the patch description is very vague about this part of the changes...
We're dealing with tricky error recovery code here and it is very easy
for subtle bugs to slip in => it is very important to have the changes
easily reviewable by as many people as possible.
Sorry but this part is "almost" good and needs to be re-done.
> - carve out an ide_cd_breathe()-helper for fs write requests
This may be as well done in a pre-patch to not hold this change back
and make the main change more readable.
> - make code more readable
>
> There should be no functional change resulting from this patch.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> drivers/ide/ide-cd.c | 238 ++++++++++++++++++++++---------------------------
> 1 files changed, 107 insertions(+), 131 deletions(-)
>
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index a4afd90..8387dbf 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -266,6 +266,38 @@ static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
> }
>
> /*
> + * Allow the drive 5 seconds to recover; some devices will return NOT_READY
> + * while flushing data from cache
> + */
> +static int ide_cd_breathe(ide_drive_t *drive, struct request *rq)
> +{
> +
> + struct cdrom_info *info = drive->driver_data;
> +
> + if (!rq->errors)
> + info->write_timeout = jiffies + ATAPI_WAIT_WRITE_BUSY;
> +
> + rq->errors = 1;
> +
> + if (time_after(jiffies, info->write_timeout))
> + return 1;
> + else {
> + struct request_queue *q = drive->queue;
> + unsigned long flags;
> +
> + /*
> + * take a breather relying on the unplug timer to kick us again
> + */
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + blk_plug_device(q);
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + return 0;
> + }
> +}
> +
> +/*
> * Returns:
> * 0: if the request should be continued.
> * 1: if the request will be going through error recovery.
> @@ -275,14 +307,15 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
> {
> ide_hwif_t *hwif = drive->hwif;
> struct request *rq = hwif->rq;
> - int err, sense_key;
> + int err, sense_key, do_end_request;
>
> /* get the IDE error register */
> err = ide_read_error(drive);
> sense_key = err >> 4;
>
> - ide_debug_log(IDE_DBG_RQ, "cmd[0]: 0x%x, rq->cmd_type: 0x%x, err: 0x%x",
> - rq->cmd[0], rq->cmd_type, err);
> + ide_debug_log(IDE_DBG_RQ, "cmd: 0x%x, rq->cmd_type: 0x%x, err: 0x%x, "
> + "stat 0x%x",
> + rq->cmd[0], rq->cmd_type, err, stat);
>
> if (blk_sense_request(rq)) {
> /*
> @@ -292,151 +325,93 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
> */
> rq->cmd_flags |= REQ_FAILED;
> return 2;
> - } else if (blk_pc_request(rq) || rq->cmd_type == REQ_TYPE_ATA_PC) {
> - /* All other functions, except for READ. */
> + }
>
> - /*
> - * if we have an error, pass back CHECK_CONDITION as the
> - * scsi status byte
> - */
> - if (blk_pc_request(rq) && !rq->errors)
> - rq->errors = SAM_STAT_CHECK_CONDITION;
> + /* if we got an error, pass CHECK_CONDITION as the scsi status byte */
> + if (blk_pc_request(rq) && !rq->errors)
> + rq->errors = SAM_STAT_CHECK_CONDITION;
>
> - /* check for tray open */
> - if (sense_key == NOT_READY) {
> - cdrom_saw_media_change(drive);
> - } else if (sense_key == UNIT_ATTENTION) {
> - /* check for media change */
> + if (blk_noretry_request(rq))
> + do_end_request = 1;
> +
> + switch (sense_key) {
> + case NOT_READY:
> + if (blk_fs_request(rq) && (rq_data_dir(rq) == WRITE)) {
> + if (!ide_cd_breathe(drive, rq))
> + return 1;
> + } else {
> cdrom_saw_media_change(drive);
> - return 0;
> - } else if (sense_key == ILLEGAL_REQUEST &&
> - rq->cmd[0] == GPCMD_START_STOP_UNIT) {
> - /*
> - * Don't print error message for this condition--
> - * SFF8090i indicates that 5/24/00 is the correct
> - * response to a request to close the tray if the
> - * drive doesn't have that capability.
> - * cdrom_log_sense() knows this!
> - */
> - } else if (!(rq->cmd_flags & REQ_QUIET)) {
> - /* otherwise, print an error */
> - ide_dump_status(drive, "packet command error", stat);
> + printk(KERN_ERR PFX "%s: tray open\n", drive->name);
> }
> + do_end_request = 1;
> + break;
>
> - rq->cmd_flags |= REQ_FAILED;
> + case UNIT_ATTENTION:
> + cdrom_saw_media_change(drive);
> + if (blk_pc_request(rq) || rq->cmd_type == REQ_TYPE_ATA_PC)
> + return 0;
> + break;
>
> + case ILLEGAL_REQUEST:
> + case DATA_PROTECT:
> /*
> - * instead of playing games with moving completions around,
> - * remove failed request completely and end it when the
> - * request sense has completed
> + * Don't print error message for this condition since SFF8090i
> + * indicates that 5/24/00 is the correct response to a request
> + * to close the tray if the drive doesn't have that capability.
> + *
> + * cdrom_log_sense() knows this!
> */
> - goto end_request;
> -
> - } else if (blk_fs_request(rq)) {
> - int do_end_request = 0;
> -
> - /* handle errors from READ and WRITE requests */
> -
> - if (blk_noretry_request(rq))
> + if (rq->cmd[0] != GPCMD_START_STOP_UNIT) {
> + ide_dump_status(drive, "command error", stat);
> do_end_request = 1;
> + }
> + break;
>
> - if (sense_key == NOT_READY) {
> - /* tray open */
> - if (rq_data_dir(rq) == READ) {
> - cdrom_saw_media_change(drive);
> -
> - /* fail the request */
> - printk(KERN_ERR PFX "%s: tray open\n",
> - drive->name);
> - do_end_request = 1;
> - } else {
> - struct cdrom_info *info = drive->driver_data;
> + case MEDIUM_ERROR:
> + /*
> + * No point in re-trying a zillion times on a bad sector. If we
> + * got here the error is not correctable.
> + */
> + ide_dump_status(drive, "media error (bad sector)", stat);
> + do_end_request = 1;
> + break;
>
> - /*
> - * Allow the drive 5 seconds to recover, some
> - * devices will return this error while flushing
> - * data from cache.
> - */
> - if (!rq->errors)
> - info->write_timeout = jiffies +
> - ATAPI_WAIT_WRITE_BUSY;
> - rq->errors = 1;
> - if (time_after(jiffies, info->write_timeout))
> - do_end_request = 1;
> - else {
> - struct request_queue *q = drive->queue;
> - unsigned long flags;
> -
> - /*
> - * take a breather relying on the unplug
> - * timer to kick us again
> - */
> - spin_lock_irqsave(q->queue_lock, flags);
> - blk_plug_device(q);
> - spin_unlock_irqrestore(q->queue_lock, flags);
> -
> - return 1;
> - }
> - }
> - } else if (sense_key == UNIT_ATTENTION) {
> - /* media change */
> - cdrom_saw_media_change(drive);
> + case BLANK_CHECK:
> + /* disk appears blank ?? */
> + ide_dump_status(drive, "media error (blank)", stat);
> + do_end_request = 1;
> + break;
>
> - /*
> - * Arrange to retry the request but be sure to give up
> - * if we've retried too many times.
> - */
> - if (++rq->errors > ERROR_MAX)
> - do_end_request = 1;
> - } else if (sense_key == ILLEGAL_REQUEST ||
> - sense_key == DATA_PROTECT) {
> - /*
> - * No point in retrying after an illegal request or data
> - * protect error.
> - */
> - ide_dump_status(drive, "command error", stat);
> - do_end_request = 1;
> - } else if (sense_key == MEDIUM_ERROR) {
> - /*
> - * No point in re-trying a zillion times on a bad
> - * sector. If we got here the error is not correctable.
> - */
> - ide_dump_status(drive, "media error (bad sector)",
> - stat);
> - do_end_request = 1;
> - } else if (sense_key == BLANK_CHECK) {
> - /* disk appears blank ?? */
> - ide_dump_status(drive, "media error (blank)", stat);
> - do_end_request = 1;
> - } else if ((err & ~ATA_ABORTED) != 0) {
> + default:
> + if (err & ~ATA_ABORTED) {
> /* go to the default handler for other errors */
> ide_error(drive, "cdrom_decode_status", stat);
> return 1;
> - } else if ((++rq->errors > ERROR_MAX)) {
> - /* we've racked up too many retries, abort */
> - do_end_request = 1;
> }
>
> - /*
> - * End a request through request sense analysis when we have
> - * sense data. We need this in order to perform end of media
> - * processing.
> - */
> - if (do_end_request)
> - goto end_request;
> + if (!(rq->cmd_flags & REQ_QUIET)) {
> + ide_dump_status(drive, "command error", stat);
> + blk_dump_rq_flags(rq, PFX "failing rq");
> + }
> + do_end_request = 1;
> + break;
> + }
>
> - /*
> - * 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;
> - } else {
> - blk_dump_rq_flags(rq, PFX "bad rq");
> - return 2;
> + /* we've racked up too many retries, abort */
> + if (++rq->errors > ERROR_MAX)
> + do_end_request = 1;
> +
> + if (do_end_request) {
> + rq->cmd_flags |= REQ_FAILED;
> + goto end_request;
> }
>
> + /* 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;
> +
> end_request:
> if (stat & ATA_ERR) {
> struct request_queue *q = drive->queue;
> @@ -614,14 +589,15 @@ static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
> struct request *rq = hwif->rq;
> ide_expiry_t *expiry = NULL;
> int dma_error = 0, dma, thislen, uptodate = 0;
> - int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc, nsectors;
> + int write, uninitialized_var(rc), nsectors;
Why is uninitialized_var() here now?
> int sense = blk_sense_request(rq);
> unsigned int timeout;
> u16 len;
> u8 ireason, stat;
>
> - ide_debug_log(IDE_DBG_PC, "cmd[0]: 0x%x, write: 0x%x",
> - rq->cmd[0], write);
> + write = (rq_data_dir(rq) == WRITE) ? 1 : 0;
>
> + ide_debug_log(IDE_DBG_PC, "cmd: 0x%x, write: 0x%x", rq->cmd[0], write);
>
> /* check for errors */
> dma = drive->dma;
Hi,
On Fri, Apr 03, 2009 at 01:08:39AM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 02 April 2009, Borislav Petkov wrote:
> > - have (almost) equal handling of commands based solely on sense_key
>
> I'm having a VERY hard time trying to review this patch because at
> the same time that codepaths were merged if()s were replaced by switch()
> which in turn resulted in change of intendation... on top of that
> the patch description is very vague about this part of the changes...
I completely and exactly understand what you are saying :), I thought so
too when I looked at the diffs yesterday. Well, if it's any consolation, the
patches've been tested so they seem to work :). Anyway, split version coming
up...
> We're dealing with tricky error recovery code here and it is very easy
> for subtle bugs to slip in => it is very important to have the changes
> easily reviewable by as many people as possible.
..
> > @@ -614,14 +589,15 @@ static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
> > struct request *rq = hwif->rq;
> > ide_expiry_t *expiry = NULL;
> > int dma_error = 0, dma, thislen, uptodate = 0;
> > - int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc, nsectors;
> > + int write, uninitialized_var(rc), nsectors;
>
> Why is uninitialized_var() here now?
because gcc is whining that it might be uninitialized although I've
doublechecked all codepaths returning a valid error. It is there to shut
up this warning, actually.
> > int sense = blk_sense_request(rq);
> > unsigned int timeout;
> > u16 len;
> > u8 ireason, stat;
> >
> > - ide_debug_log(IDE_DBG_PC, "cmd[0]: 0x%x, write: 0x%x",
> > - rq->cmd[0], write);
> > + write = (rq_data_dir(rq) == WRITE) ? 1 : 0;
> >
> > + ide_debug_log(IDE_DBG_PC, "cmd: 0x%x, write: 0x%x", rq->cmd[0], write);
> >
> > /* check for errors */
> > dma = drive->dma;
--
Regards/Gruss,
Boris.
On Friday 03 April 2009, Borislav Petkov wrote:
> Hi,
>
> On Fri, Apr 03, 2009 at 01:08:39AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Thursday 02 April 2009, Borislav Petkov wrote:
> > > - have (almost) equal handling of commands based solely on sense_key
> >
> > I'm having a VERY hard time trying to review this patch because at
> > the same time that codepaths were merged if()s were replaced by switch()
> > which in turn resulted in change of intendation... on top of that
> > the patch description is very vague about this part of the changes...
>
> I completely and exactly understand what you are saying :), I thought so
> too when I looked at the diffs yesterday. Well, if it's any consolation, the
> patches've been tested so they seem to work :). Anyway, split version coming
> up...
The split version looks exactly the same except ide_cd_breathe() change
when it comes to the main part.
Did you send the wrong version by any chance?
On Fri, Apr 3, 2009 at 12:41 PM, Bartlomiej Zolnierkiewicz
<[email protected]> wrote:
> On Friday 03 April 2009, Borislav Petkov wrote:
>> Hi,
>>
>> On Fri, Apr 03, 2009 at 01:08:39AM +0200, Bartlomiej Zolnierkiewicz wrote:
>> > On Thursday 02 April 2009, Borislav Petkov wrote:
>> > > - have (almost) equal handling of commands based solely on sense_key
>> >
>> > I'm having a VERY hard time trying to review this patch because at
>> > the same time that codepaths were merged if()s were replaced by switch()
>> > which in turn resulted in change of intendation... on top of that
>> > the patch description is very vague about this part of the changes...
>>
>> I completely and exactly understand what you are saying :), I thought so
>> too when I looked at the diffs yesterday. Well, if it's any consolation, the
>> patches've been tested so they seem to work :). Anyway, split version coming
>> up...
>
> The split version looks exactly the same except ide_cd_breathe() change
> when it comes to the main part.
>
> Did you send the wrong version by any chance?
No, but I can't split those changes anymore logically. The switch-case handles
the differentiation based on the sense_key and as such cannot be broken down
anymore without doing some weird stuff and possibly introducing more bugs and
breaking bisectability. Completely hypothetically: wouldn't a before-after
juxtaposition of the change make reviewing more easier, for example you
apply the patch on a different branch and compare the before and the after
version?
--
Regards/Gruss,
Boris
On Friday 03 April 2009, Borislav Petkov wrote:
> On Fri, Apr 3, 2009 at 12:41 PM, Bartlomiej Zolnierkiewicz
> <[email protected]> wrote:
> > On Friday 03 April 2009, Borislav Petkov wrote:
> >> Hi,
> >>
> >> On Fri, Apr 03, 2009 at 01:08:39AM +0200, Bartlomiej Zolnierkiewicz wrote:
> >> > On Thursday 02 April 2009, Borislav Petkov wrote:
> >> > > - have (almost) equal handling of commands based solely on sense_key
> >> >
> >> > I'm having a VERY hard time trying to review this patch because at
> >> > the same time that codepaths were merged if()s were replaced by switch()
> >> > which in turn resulted in change of intendation... on top of that
> >> > the patch description is very vague about this part of the changes...
> >>
> >> I completely and exactly understand what you are saying :), I thought so
> >> too when I looked at the diffs yesterday. Well, if it's any consolation, the
> >> patches've been tested so they seem to work :). Anyway, split version coming
> >> up...
> >
> > The split version looks exactly the same except ide_cd_breathe() change
> > when it comes to the main part.
> >
> > Did you send the wrong version by any chance?
>
> No, but I can't split those changes anymore logically. The switch-case handles
> the differentiation based on the sense_key and as such cannot be broken down
> anymore without doing some weird stuff and possibly introducing more bugs and
Since this didn't convince me...
> breaking bisectability. Completely hypothetically: wouldn't a before-after
> juxtaposition of the change make reviewing more easier, for example you
> apply the patch on a different branch and compare the before and the after
> version?
...and this would make me spend a whole weekend trying to track down every
little change in code's behavior I went ahead and re-did your patch splitting
it on a more fine-grained logical changes (posted in separate patchset, while
at it I also fixed REQ_QUIET handling for fs requests).
After that I did a diff between ide-cd.c.old_patch and ide-cd.c.new_patchset
and besides some trivial differences I indeed found some subtle problems...
--- ide-cd.c.old_patch 2009-04-03 18:50:23.000000000 +0200
+++ ide-cd.c.new_patchset 2009-04-03 21:12:02.000000000 +0200
@@ -311,7 +311,7 @@
{
ide_hwif_t *hwif = drive->hwif;
struct request *rq = hwif->rq;
- int err, sense_key, do_end_request;
+ int err, sense_key, do_end_request = 0;
/* get the IDE error register */
err = ide_read_error(drive);
@@ -331,90 +331,104 @@
return 2;
}
- /* if we got an error, pass CHECK_CONDITION as the scsi status byte */
+ /* if we got an error, pass CHECK_CONDITION as the SCSI status byte */
if (blk_pc_request(rq) && !rq->errors)
rq->errors = SAM_STAT_CHECK_CONDITION;
if (blk_noretry_request(rq))
- do_end_request = 1;
+ do_end_request = 1;
switch (sense_key) {
case NOT_READY:
- if (blk_fs_request(rq) && (rq_data_dir(rq) == WRITE)) {
+ if (blk_fs_request(rq) == 0 || rq_data_dir(rq) == READ) {
+ cdrom_saw_media_change(drive);
+
+ if (blk_fs_request(rq) &&
+ (rq->cmd_flags & REQ_QUIET) == 0)
+ printk(KERN_ERR PFX "%s: tray open\n",
+ drive->name);
+ } else {
if (ide_cd_breathe(drive, rq))
return 1;
- } else {
- cdrom_saw_media_change(drive);
- printk(KERN_ERR PFX "%s: tray open\n", drive->name);
original code didn't spam logs for pc requests
}
do_end_request = 1;
break;
-
case UNIT_ATTENTION:
cdrom_saw_media_change(drive);
- if (blk_pc_request(rq) || rq->cmd_type == REQ_TYPE_ATA_PC)
+
+ if (blk_fs_request(rq) == 0)
return 0;
+ /*
+ * Arrange to retry the request but be sure to give up if we've
+ * retried too many times.
+ */
+ if (++rq->errors > ERROR_MAX)
+ do_end_request = 1;
break;
-
case ILLEGAL_REQUEST:
- case DATA_PROTECT:
/*
- * Don't print error message for this condition since SFF8090i
+ * Don't print error message for this condition -- SFF8090i
* indicates that 5/24/00 is the correct response to a request
* to close the tray if the drive doesn't have that capability.
*
* cdrom_log_sense() knows this!
*/
- if (rq->cmd[0] != GPCMD_START_STOP_UNIT) {
+ if (rq->cmd[0] == GPCMD_START_STOP_UNIT)
this was checked only for ILLEGAL_REQUEST and not DATA_PROTECT
+ break;
+ /* fall-through */
+ case DATA_PROTECT:
+ /*
+ * No point in retrying after an illegal request or data
+ * protect error.
+ */
+ if ((rq->cmd_flags & REQ_QUIET) == 0)
ide_dump_status(drive, "command error", stat);
original code respected REQ_QUIET for pc requests
- do_end_request = 1;
- }
+ do_end_request = 1;
break;
-
case MEDIUM_ERROR:
/*
- * No point in re-trying a zillion times on a bad sector. If we
- * got here the error is not correctable.
+ * No point in re-trying a zillion times on a bad sector.
+ * If we got here the error is not correctable.
*/
- ide_dump_status(drive, "media error (bad sector)", stat);
+ if ((rq->cmd_flags & REQ_QUIET) == 0)
+ ide_dump_status(drive, "media error "
+ "(bad sector)", stat);
ditto
do_end_request = 1;
break;
-
case BLANK_CHECK:
- /* disk appears blank ?? */
- ide_dump_status(drive, "media error (blank)", stat);
+ /* disk appears blank? */
+ if ((rq->cmd_flags & REQ_QUIET) == 0)
+ ide_dump_status(drive, "media error (blank)",
+ stat);
ditto
do_end_request = 1;
break;
-
default:
+ if (blk_fs_request(rq) == 0)
+ break;
if (err & ~ATA_ABORTED) {
/* go to the default handler for other errors */
ide_error(drive, "cdrom_decode_status", stat);
return 1;
we can think about doing this also for pc requests but original code
didn't do it so we shouldn't mix it with purely cleanup changes
- }
-
- if (!(rq->cmd_flags & REQ_QUIET)) {
- ide_dump_status(drive, "command error", stat);
- blk_dump_rq_flags(rq, PFX "failing rq");
- }
-
- do_end_request = 1;
this looks like a real regression -- in such situation original code
ended fs requests only if (++rq->errors > ERROR_MAX)
- break;
+ } else if (++rq->errors > ERROR_MAX)
+ /* we've racked up too many retries, abort */
+ do_end_request = 1;
}
- /* we've racked up too many retries, abort */
- if (++rq->errors > ERROR_MAX)
+ if (blk_fs_request(rq) == 0) {
+ rq->cmd_flags |= REQ_FAILED;
do_end_request = 1;
+ }
- if (do_end_request) {
- rq->cmd_flags |= REQ_FAILED;
we may set this flag also for fs requests but separate patch is preferred
+ /*
+ * End a request through request sense analysis when we have sense data.
+ * We need this in order to perform end of media processing.
+ */
+ if (do_end_request)
goto end_request;
- }
- /* If we got a CHECK_CONDITION status, queue a request sense command. */
+ /* 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;
end_request:
Hi,
> ...and this would make me spend a whole weekend trying to track down every
> little change in code's behavior I went ahead and re-did your patch splitting
> it on a more fine-grained logical changes (posted in separate patchset, while
> at it I also fixed REQ_QUIET handling for fs requests).
>
> After that I did a diff between ide-cd.c.old_patch and ide-cd.c.new_patchset
> and besides some trivial differences I indeed found some subtle problems...
>
> --- ide-cd.c.old_patch 2009-04-03 18:50:23.000000000 +0200
> +++ ide-cd.c.new_patchset 2009-04-03 21:12:02.000000000 +0200
> @@ -311,7 +311,7 @@
> {
> ide_hwif_t *hwif = drive->hwif;
> struct request *rq = hwif->rq;
> - int err, sense_key, do_end_request;
> + int err, sense_key, do_end_request = 0;
>
> /* get the IDE error register */
> err = ide_read_error(drive);
> @@ -331,90 +331,104 @@
> return 2;
> }
>
> - /* if we got an error, pass CHECK_CONDITION as the scsi status byte */
> + /* if we got an error, pass CHECK_CONDITION as the SCSI status byte */
> if (blk_pc_request(rq) && !rq->errors)
> rq->errors = SAM_STAT_CHECK_CONDITION;
>
> if (blk_noretry_request(rq))
> - do_end_request = 1;
> + do_end_request = 1;
>
> switch (sense_key) {
> case NOT_READY:
> - if (blk_fs_request(rq) && (rq_data_dir(rq) == WRITE)) {
> + if (blk_fs_request(rq) == 0 || rq_data_dir(rq) == READ) {
> + cdrom_saw_media_change(drive);
> +
> + if (blk_fs_request(rq) &&
> + (rq->cmd_flags & REQ_QUIET) == 0)
> + printk(KERN_ERR PFX "%s: tray open\n",
> + drive->name);
> + } else {
> if (ide_cd_breathe(drive, rq))
> return 1;
> - } else {
> - cdrom_saw_media_change(drive);
> - printk(KERN_ERR PFX "%s: tray open\n", drive->name);
>
> original code didn't spam logs for pc requests
but since we want to unify behavior I don't think we should handle
the rq verbosity cases differently based on rq type and remove that
blk_fs_request(rq) check instead. Also, you've inverted the logic for
the ide_cd_breathe() case and I'd much rather have it there explicitly
for clarity. Something like:
if (blk_fs_request(rq) && (rq_data_dir(rq) == WRITE)) {
if (ide_cd_breathe(drive, rq))
return 1;
} else {
cdrom_saw_media_change(drive);
if ((rq->cmd_flags & REQ_QUIET) == 0)
printk(KERN_ERR PFX "%s: tray open\n",
drive->name);
}
break;
> }
> do_end_request = 1;
> break;
> -
> case UNIT_ATTENTION:
> cdrom_saw_media_change(drive);
> - if (blk_pc_request(rq) || rq->cmd_type == REQ_TYPE_ATA_PC)
> +
> + if (blk_fs_request(rq) == 0)
> return 0;
> + /*
> + * Arrange to retry the request but be sure to give up if we've
> + * retried too many times.
> + */
> + if (++rq->errors > ERROR_MAX)
> + do_end_request = 1;
We could just as well kill any request which has retried ERROR_MAX
times, not only the one returning UNIT_ATTENTION, as an upper bound on
the times travelling through driver and block layer, don't you think?
That's why I moved the ERROR_MAX check _after_ the switch-case. I know,
I know, a sentence about it in the commit message would've been quilt
suitable, i know :)...
> break;
> -
> case ILLEGAL_REQUEST:
> - case DATA_PROTECT:
> /*
> - * Don't print error message for this condition since SFF8090i
> + * Don't print error message for this condition -- SFF8090i
> * indicates that 5/24/00 is the correct response to a request
> * to close the tray if the drive doesn't have that capability.
> *
> * cdrom_log_sense() knows this!
> */
> - if (rq->cmd[0] != GPCMD_START_STOP_UNIT) {
> + if (rq->cmd[0] == GPCMD_START_STOP_UNIT)
>
> this was checked only for ILLEGAL_REQUEST and not DATA_PROTECT
ah, that's correct, thanks for catching that.
> + break;
> + /* fall-through */
> + case DATA_PROTECT:
> + /*
> + * No point in retrying after an illegal request or data
> + * protect error.
> + */
> + if ((rq->cmd_flags & REQ_QUIET) == 0)
> ide_dump_status(drive, "command error", stat);
>
> original code respected REQ_QUIET for pc requests
ok, what's the rationale on being quiet per req_type? In other words, why did we
respect that for pc requests _and_ _not_ for fs requests?
> - do_end_request = 1;
> - }
> + do_end_request = 1;
> break;
> -
> case MEDIUM_ERROR:
> /*
> - * No point in re-trying a zillion times on a bad sector. If we
> - * got here the error is not correctable.
> + * No point in re-trying a zillion times on a bad sector.
> + * If we got here the error is not correctable.
> */
> - ide_dump_status(drive, "media error (bad sector)", stat);
> + if ((rq->cmd_flags & REQ_QUIET) == 0)
> + ide_dump_status(drive, "media error "
> + "(bad sector)", stat);
>
> ditto
>
> do_end_request = 1;
> break;
> -
> case BLANK_CHECK:
> - /* disk appears blank ?? */
> - ide_dump_status(drive, "media error (blank)", stat);
> + /* disk appears blank? */
> + if ((rq->cmd_flags & REQ_QUIET) == 0)
> + ide_dump_status(drive, "media error (blank)",
> + stat);
>
> ditto
>
> do_end_request = 1;
> break;
> -
> default:
> + if (blk_fs_request(rq) == 0)
> + break;
> if (err & ~ATA_ABORTED) {
> /* go to the default handler for other errors */
> ide_error(drive, "cdrom_decode_status", stat);
> return 1;
>
> we can think about doing this also for pc requests but original code
> didn't do it so we shouldn't mix it with purely cleanup changes
>
> - }
> -
> - if (!(rq->cmd_flags & REQ_QUIET)) {
> - ide_dump_status(drive, "command error", stat);
> - blk_dump_rq_flags(rq, PFX "failing rq");
> - }
> -
> - do_end_request = 1;
>
> this looks like a real regression -- in such situation original code
> ended fs requests only if (++rq->errors > ERROR_MAX)
right.
> - break;
> + } else if (++rq->errors > ERROR_MAX)
> + /* we've racked up too many retries, abort */
> + do_end_request = 1;
> }
>
> - /* we've racked up too many retries, abort */
> - if (++rq->errors > ERROR_MAX)
> + if (blk_fs_request(rq) == 0) {
> + rq->cmd_flags |= REQ_FAILED;
> do_end_request = 1;
> + }
>
> - if (do_end_request) {
> - rq->cmd_flags |= REQ_FAILED;
>
> we may set this flag also for fs requests but separate patch is preferred
>
> + /*
> + * End a request through request sense analysis when we have sense data.
> + * We need this in order to perform end of media processing.
> + */
> + if (do_end_request)
> goto end_request;
> - }
>
> - /* If we got a CHECK_CONDITION status, queue a request sense command. */
> + /* 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;
>
> end_request:
--
Regards/Gruss,
Boris.
Hi,
On Saturday 04 April 2009, Borislav Petkov wrote:
> Hi,
>
> > ...and this would make me spend a whole weekend trying to track down every
> > little change in code's behavior I went ahead and re-did your patch splitting
> > it on a more fine-grained logical changes (posted in separate patchset, while
> > at it I also fixed REQ_QUIET handling for fs requests).
> >
> > After that I did a diff between ide-cd.c.old_patch and ide-cd.c.new_patchset
> > and besides some trivial differences I indeed found some subtle problems...
> >
> > --- ide-cd.c.old_patch 2009-04-03 18:50:23.000000000 +0200
> > +++ ide-cd.c.new_patchset 2009-04-03 21:12:02.000000000 +0200
> > @@ -311,7 +311,7 @@
> > {
> > ide_hwif_t *hwif = drive->hwif;
> > struct request *rq = hwif->rq;
> > - int err, sense_key, do_end_request;
> > + int err, sense_key, do_end_request = 0;
> >
> > /* get the IDE error register */
> > err = ide_read_error(drive);
> > @@ -331,90 +331,104 @@
> > return 2;
> > }
> >
> > - /* if we got an error, pass CHECK_CONDITION as the scsi status byte */
> > + /* if we got an error, pass CHECK_CONDITION as the SCSI status byte */
> > if (blk_pc_request(rq) && !rq->errors)
> > rq->errors = SAM_STAT_CHECK_CONDITION;
> >
> > if (blk_noretry_request(rq))
> > - do_end_request = 1;
> > + do_end_request = 1;
> >
> > switch (sense_key) {
> > case NOT_READY:
> > - if (blk_fs_request(rq) && (rq_data_dir(rq) == WRITE)) {
> > + if (blk_fs_request(rq) == 0 || rq_data_dir(rq) == READ) {
> > + cdrom_saw_media_change(drive);
> > +
> > + if (blk_fs_request(rq) &&
> > + (rq->cmd_flags & REQ_QUIET) == 0)
> > + printk(KERN_ERR PFX "%s: tray open\n",
> > + drive->name);
> > + } else {
> > if (ide_cd_breathe(drive, rq))
> > return 1;
> > - } else {
> > - cdrom_saw_media_change(drive);
> > - printk(KERN_ERR PFX "%s: tray open\n", drive->name);
> >
> > original code didn't spam logs for pc requests
>
> but since we want to unify behavior I don't think we should handle
> the rq verbosity cases differently based on rq type and remove that
> blk_fs_request(rq) check instead. Also, you've inverted the logic for
Right, but it can be changed later (if you are fine with the current
patchset I'll merge it so we can base such improvements on top if it).
> the ide_cd_breathe() case and I'd much rather have it there explicitly
> for clarity. Something like:
>
> if (blk_fs_request(rq) && (rq_data_dir(rq) == WRITE)) {
[ parentheses aroung rq_data_dir() check are not necessary ]
> if (ide_cd_breathe(drive, rq))
> return 1;
> } else {
> cdrom_saw_media_change(drive);
>
> if ((rq->cmd_flags & REQ_QUIET) == 0)
> printk(KERN_ERR PFX "%s: tray open\n",
> drive->name);
> }
> break;
>
>
>
> > }
> > do_end_request = 1;
> > break;
> > -
> > case UNIT_ATTENTION:
> > cdrom_saw_media_change(drive);
> > - if (blk_pc_request(rq) || rq->cmd_type == REQ_TYPE_ATA_PC)
> > +
> > + if (blk_fs_request(rq) == 0)
> > return 0;
> > + /*
> > + * Arrange to retry the request but be sure to give up if we've
> > + * retried too many times.
> > + */
> > + if (++rq->errors > ERROR_MAX)
> > + do_end_request = 1;
>
> We could just as well kill any request which has retried ERROR_MAX
> times, not only the one returning UNIT_ATTENTION, as an upper bound on
> the times travelling through driver and block layer, don't you think?
Hmm, it seems like we already do -- in all cases that we don't end
request unconditionally we either return or do this check...
> That's why I moved the ERROR_MAX check _after_ the switch-case. I know,
> I know, a sentence about it in the commit message would've been quilt
> suitable, i know :)...
Yep, I don't have a remote mind-reading device here (yet)... ;)
[...]
> > + break;
> > + /* fall-through */
> > + case DATA_PROTECT:
> > + /*
> > + * No point in retrying after an illegal request or data
> > + * protect error.
> > + */
> > + if ((rq->cmd_flags & REQ_QUIET) == 0)
> > ide_dump_status(drive, "command error", stat);
> >
> > original code respected REQ_QUIET for pc requests
>
> ok, what's the rationale on being quiet per req_type? In other words, why did we
> respect that for pc requests _and_ _not_ for fs requests?
Historical reasons (please note that it is fixed now in patch #1/5).
On Friday 03 April 2009, Borislav Petkov wrote:
[...]
> > > @@ -614,14 +589,15 @@ static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
> > > struct request *rq = hwif->rq;
> > > ide_expiry_t *expiry = NULL;
> > > int dma_error = 0, dma, thislen, uptodate = 0;
> > > - int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc, nsectors;
> > > + int write, uninitialized_var(rc), nsectors;
> >
> > Why is uninitialized_var() here now?
>
> because gcc is whining that it might be uninitialized although I've
> doublechecked all codepaths returning a valid error. It is there to shut
> up this warning, actually.
It is gcc's fault (version dependent on top of it). However since it may
affect other people and workarounding it costs us absolutely nothing lets
be pragmatic about it.
Please resend this change in a separate patch so it can be applied.