All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] ide-tape: fix flags
@ 2009-06-02  7:05 Borislav Petkov
  2009-06-02  7:05   ` Borislav Petkov
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Borislav Petkov @ 2009-06-02  7:05 UTC (permalink / raw)
  To: bzolnier, Bartlomiej Zolnierkiewicz; +Cc: linux-ide, linux-kernel

Hi Bart,

those fix a stupid flags misuse that got spotted by Jiri Slaby. Please
pay special attention to the first patch. FWIW, those can go as hot
fixes right away and we might catch 30-rc8 since it's not out yet.

Thanks,
Boris.

drivers/ide/ide-atapi.c |    2 +-
 drivers/ide/ide-tape.c  |   64 ++++++++++++++++++++++++++++++----------------
 2 files changed, 43 insertions(+), 23 deletions(-)

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH 1/2] ide-tape: change IDE_AFLAG_IGNORE_DSC non-atomically
  2009-06-02  7:05 [PATCH 0/2] ide-tape: fix flags Borislav Petkov
@ 2009-06-02  7:05   ` Borislav Petkov
  2009-06-02  7:05   ` Borislav Petkov
  2009-06-05 18:34 ` [PATCH 0/2] ide-tape: fix flags Bartlomiej Zolnierkiewicz
  2 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2009-06-02  7:05 UTC (permalink / raw)
  To: bzolnier; +Cc: linux-ide, linux-kernel

There are two sites where the flag is being changed: ide_retry_pc
and idetape_do_request. Both codepaths are protected by hwif->busy
(ide_lock_port) and therefore we shouldn't need the atomic accesses. The
only problem would be the compiler reordering the accesses, therefore the
optimization barrier.

Spotted-by: Jiri Slaby <jirislaby@gmail.com>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c |    2 +-
 drivers/ide/ide-tape.c  |   21 ++++++++++++++++-----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index afe5a43..fbcb851 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -258,7 +258,7 @@ void ide_retry_pc(ide_drive_t *drive)
 	pc->req_xfer = sense_rq->data_len;
 
 	if (drive->media == ide_tape)
-		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
+		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
 
 	if (ide_queue_sense_rq(drive, pc))
 		ide_complete_rq(drive, -EIO, blk_rq_bytes(drive->hwif->rq));
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 203bbea..4ff50cc 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -656,15 +656,24 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 
 	if ((drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) == 0 &&
 	    (rq->cmd[13] & REQ_IDETAPE_PC2) == 0)
-		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
+		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
 
 	if (drive->dev_flags & IDE_DFLAG_POST_RESET) {
-		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
+		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
 		drive->dev_flags &= ~IDE_DFLAG_POST_RESET;
 	}
 
-	if (!test_and_clear_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags) &&
-	    (stat & ATA_DSC) == 0) {
+	/*
+	 * This is a precaution for IDE_AFLAG_IGNORE_DSC being conditionally set
+	 * above. We don't need a stronger enforcement of ordering because the
+	 * read below cannot precede the earlier write out-of-order since it is
+	 * to the same location. Also, since we have the ide port locked during
+	 * the ->do_request(), we only have to be aware of gcc reordering stuff.
+	 */
+	barrier();
+
+	if (!(drive->atapi_flags & IDE_AFLAG_IGNORE_DSC) &&
+	    !(stat & ATA_DSC)) {
 		if (postponed_rq == NULL) {
 			tape->dsc_polling_start = jiffies;
 			tape->dsc_poll_freq = tape->best_dsc_rw_freq;
@@ -684,7 +693,9 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 			tape->dsc_poll_freq = IDETAPE_DSC_MA_SLOW;
 		idetape_postpone_request(drive);
 		return ide_stopped;
-	}
+	} else
+		drive->atapi_flags &= ~IDE_AFLAG_IGNORE_DSC;
+
 	if (rq->cmd[13] & REQ_IDETAPE_READ) {
 		pc = &tape->queued_pc;
 		ide_tape_create_rw_cmd(tape, pc, rq, READ_6);
-- 
1.6.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 1/2] ide-tape: change IDE_AFLAG_IGNORE_DSC non-atomically
@ 2009-06-02  7:05   ` Borislav Petkov
  0 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2009-06-02  7:05 UTC (permalink / raw)
  To: bzolnier; +Cc: linux-ide, linux-kernel

There are two sites where the flag is being changed: ide_retry_pc
and idetape_do_request. Both codepaths are protected by hwif->busy
(ide_lock_port) and therefore we shouldn't need the atomic accesses. The
only problem would be the compiler reordering the accesses, therefore the
optimization barrier.

Spotted-by: Jiri Slaby <jirislaby@gmail.com>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c |    2 +-
 drivers/ide/ide-tape.c  |   21 ++++++++++++++++-----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index afe5a43..fbcb851 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -258,7 +258,7 @@ void ide_retry_pc(ide_drive_t *drive)
 	pc->req_xfer = sense_rq->data_len;
 
 	if (drive->media == ide_tape)
-		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
+		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
 
 	if (ide_queue_sense_rq(drive, pc))
 		ide_complete_rq(drive, -EIO, blk_rq_bytes(drive->hwif->rq));
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 203bbea..4ff50cc 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -656,15 +656,24 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 
 	if ((drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) == 0 &&
 	    (rq->cmd[13] & REQ_IDETAPE_PC2) == 0)
-		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
+		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
 
 	if (drive->dev_flags & IDE_DFLAG_POST_RESET) {
-		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
+		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
 		drive->dev_flags &= ~IDE_DFLAG_POST_RESET;
 	}
 
-	if (!test_and_clear_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags) &&
-	    (stat & ATA_DSC) == 0) {
+	/*
+	 * This is a precaution for IDE_AFLAG_IGNORE_DSC being conditionally set
+	 * above. We don't need a stronger enforcement of ordering because the
+	 * read below cannot precede the earlier write out-of-order since it is
+	 * to the same location. Also, since we have the ide port locked during
+	 * the ->do_request(), we only have to be aware of gcc reordering stuff.
+	 */
+	barrier();
+
+	if (!(drive->atapi_flags & IDE_AFLAG_IGNORE_DSC) &&
+	    !(stat & ATA_DSC)) {
 		if (postponed_rq == NULL) {
 			tape->dsc_polling_start = jiffies;
 			tape->dsc_poll_freq = tape->best_dsc_rw_freq;
@@ -684,7 +693,9 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 			tape->dsc_poll_freq = IDETAPE_DSC_MA_SLOW;
 		idetape_postpone_request(drive);
 		return ide_stopped;
-	}
+	} else
+		drive->atapi_flags &= ~IDE_AFLAG_IGNORE_DSC;
+
 	if (rq->cmd[13] & REQ_IDETAPE_READ) {
 		pc = &tape->queued_pc;
 		ide_tape_create_rw_cmd(tape, pc, rq, READ_6);
-- 
1.6.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 2/2] ide-tape: fix IDE_AFLAG_* atomic accesses
  2009-06-02  7:05 [PATCH 0/2] ide-tape: fix flags Borislav Petkov
@ 2009-06-02  7:05   ` Borislav Petkov
  2009-06-02  7:05   ` Borislav Petkov
  2009-06-05 18:34 ` [PATCH 0/2] ide-tape: fix flags Bartlomiej Zolnierkiewicz
  2 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2009-06-02  7:05 UTC (permalink / raw)
  To: bzolnier; +Cc: linux-ide, linux-kernel

These flags used to be bit numbers and now are single bits in the
->atapi_flags vector. Use them properly.

Spotted-by: Jiri Slaby <jirislaby@gmail.com>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-tape.c |   43 ++++++++++++++++++++++++++-----------------
 1 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 4ff50cc..a305a88 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -397,7 +397,8 @@ static int ide_tape_callback(ide_drive_t *drive, int dsc)
 		if (readpos[0] & 0x4) {
 			printk(KERN_INFO "ide-tape: Block location is unknown"
 					 "to the tape\n");
-			clear_bit(IDE_AFLAG_ADDRESS_VALID, &drive->atapi_flags);
+			clear_bit(ilog2(IDE_AFLAG_ADDRESS_VALID),
+				  &drive->atapi_flags);
 			uptodate = 0;
 			err = IDE_DRV_ERROR_GENERAL;
 		} else {
@@ -406,7 +407,8 @@ static int ide_tape_callback(ide_drive_t *drive, int dsc)
 
 			tape->partition = readpos[1];
 			tape->first_frame = be32_to_cpup((__be32 *)&readpos[4]);
-			set_bit(IDE_AFLAG_ADDRESS_VALID, &drive->atapi_flags);
+			set_bit(ilog2(IDE_AFLAG_ADDRESS_VALID),
+				&drive->atapi_flags);
 		}
 	}
 
@@ -755,7 +757,7 @@ static int idetape_wait_ready(ide_drive_t *drive, unsigned long timeout)
 	int load_attempted = 0;
 
 	/* Wait for the tape to become ready */
-	set_bit(IDE_AFLAG_MEDIUM_PRESENT, &drive->atapi_flags);
+	set_bit(ilog2(IDE_AFLAG_MEDIUM_PRESENT), &drive->atapi_flags);
 	timeout += jiffies;
 	while (time_before(jiffies, timeout)) {
 		if (ide_do_test_unit_ready(drive, disk) == 0)
@@ -831,7 +833,7 @@ static void __ide_tape_discard_merge_buffer(ide_drive_t *drive)
 	if (tape->chrdev_dir != IDETAPE_DIR_READ)
 		return;
 
-	clear_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags);
+	clear_bit(ilog2(IDE_AFLAG_FILEMARK), &drive->atapi_flags);
 	tape->valid = 0;
 	if (tape->buf != NULL) {
 		kfree(tape->buf);
@@ -1124,7 +1126,8 @@ static int idetape_space_over_filemarks(ide_drive_t *drive, short mt_op,
 
 	if (tape->chrdev_dir == IDETAPE_DIR_READ) {
 		tape->valid = 0;
-		if (test_and_clear_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags))
+		if (test_and_clear_bit(ilog2(IDE_AFLAG_FILEMARK),
+				       &drive->atapi_flags))
 			++count;
 		ide_tape_discard_merge_buffer(drive, 0);
 	}
@@ -1179,7 +1182,7 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
 	debug_log(DBG_CHRDEV, "Enter %s, count %Zd\n", __func__, count);
 
 	if (tape->chrdev_dir != IDETAPE_DIR_READ) {
-		if (test_bit(IDE_AFLAG_DETECT_BS, &drive->atapi_flags))
+		if (test_bit(ilog2(IDE_AFLAG_DETECT_BS), &drive->atapi_flags))
 			if (count > tape->blk_size &&
 			    (count % tape->blk_size) == 0)
 				tape->user_bs_factor = count / tape->blk_size;
@@ -1195,7 +1198,8 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
 		/* refill if staging buffer is empty */
 		if (!tape->valid) {
 			/* If we are at a filemark, nothing more to read */
-			if (test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags))
+			if (test_bit(ilog2(IDE_AFLAG_FILEMARK),
+				     &drive->atapi_flags))
 				break;
 			/* read */
 			if (idetape_queue_rw_tail(drive, REQ_IDETAPE_READ,
@@ -1213,7 +1217,7 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
 		done += todo;
 	}
 
-	if (!done && test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags)) {
+	if (!done && test_bit(ilog2(IDE_AFLAG_FILEMARK), &drive->atapi_flags)) {
 		debug_log(DBG_SENSE, "%s: spacing over filemark\n", tape->name);
 
 		idetape_space_over_filemarks(drive, MTFSF, 1);
@@ -1347,7 +1351,8 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
 		ide_tape_discard_merge_buffer(drive, 0);
 		retval = ide_do_start_stop(drive, disk, !IDETAPE_LU_LOAD_MASK);
 		if (!retval)
-			clear_bit(IDE_AFLAG_MEDIUM_PRESENT, &drive->atapi_flags);
+			clear_bit(ilog2(IDE_AFLAG_MEDIUM_PRESENT),
+				  &drive->atapi_flags);
 		return retval;
 	case MTNOP:
 		ide_tape_discard_merge_buffer(drive, 0);
@@ -1369,9 +1374,11 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
 			    mt_count % tape->blk_size)
 				return -EIO;
 			tape->user_bs_factor = mt_count / tape->blk_size;
-			clear_bit(IDE_AFLAG_DETECT_BS, &drive->atapi_flags);
+			clear_bit(ilog2(IDE_AFLAG_DETECT_BS),
+				  &drive->atapi_flags);
 		} else
-			set_bit(IDE_AFLAG_DETECT_BS, &drive->atapi_flags);
+			set_bit(ilog2(IDE_AFLAG_DETECT_BS),
+				&drive->atapi_flags);
 		return 0;
 	case MTSEEK:
 		ide_tape_discard_merge_buffer(drive, 0);
@@ -1516,20 +1523,20 @@ static int idetape_chrdev_open(struct inode *inode, struct file *filp)
 
 	filp->private_data = tape;
 
-	if (test_and_set_bit(IDE_AFLAG_BUSY, &drive->atapi_flags)) {
+	if (test_and_set_bit(ilog2(IDE_AFLAG_BUSY), &drive->atapi_flags)) {
 		retval = -EBUSY;
 		goto out_put_tape;
 	}
 
 	retval = idetape_wait_ready(drive, 60 * HZ);
 	if (retval) {
-		clear_bit(IDE_AFLAG_BUSY, &drive->atapi_flags);
+		clear_bit(ilog2(IDE_AFLAG_BUSY), &drive->atapi_flags);
 		printk(KERN_ERR "ide-tape: %s: drive not ready\n", tape->name);
 		goto out_put_tape;
 	}
 
 	idetape_read_position(drive);
-	if (!test_bit(IDE_AFLAG_ADDRESS_VALID, &drive->atapi_flags))
+	if (!test_bit(ilog2(IDE_AFLAG_ADDRESS_VALID), &drive->atapi_flags))
 		(void)idetape_rewind_tape(drive);
 
 	/* Read block size and write protect status from drive. */
@@ -1545,7 +1552,7 @@ static int idetape_chrdev_open(struct inode *inode, struct file *filp)
 	if (tape->write_prot) {
 		if ((filp->f_flags & O_ACCMODE) == O_WRONLY ||
 		    (filp->f_flags & O_ACCMODE) == O_RDWR) {
-			clear_bit(IDE_AFLAG_BUSY, &drive->atapi_flags);
+			clear_bit(ilog2(IDE_AFLAG_BUSY), &drive->atapi_flags);
 			retval = -EROFS;
 			goto out_put_tape;
 		}
@@ -1602,15 +1609,17 @@ static int idetape_chrdev_release(struct inode *inode, struct file *filp)
 			ide_tape_discard_merge_buffer(drive, 1);
 	}
 
-	if (minor < 128 && test_bit(IDE_AFLAG_MEDIUM_PRESENT, &drive->atapi_flags))
+	if (minor < 128 && test_bit(ilog2(IDE_AFLAG_MEDIUM_PRESENT),
+				    &drive->atapi_flags))
 		(void) idetape_rewind_tape(drive);
+
 	if (tape->chrdev_dir == IDETAPE_DIR_NONE) {
 		if (tape->door_locked == DOOR_LOCKED) {
 			if (!ide_set_media_lock(drive, tape->disk, 0))
 				tape->door_locked = DOOR_UNLOCKED;
 		}
 	}
-	clear_bit(IDE_AFLAG_BUSY, &drive->atapi_flags);
+	clear_bit(ilog2(IDE_AFLAG_BUSY), &drive->atapi_flags);
 	ide_tape_put(tape);
 	unlock_kernel();
 	return 0;
-- 
1.6.3.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 2/2] ide-tape: fix IDE_AFLAG_* atomic accesses
@ 2009-06-02  7:05   ` Borislav Petkov
  0 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2009-06-02  7:05 UTC (permalink / raw)
  To: bzolnier; +Cc: linux-ide, linux-kernel

These flags used to be bit numbers and now are single bits in the
->atapi_flags vector. Use them properly.

Spotted-by: Jiri Slaby <jirislaby@gmail.com>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-tape.c |   43 ++++++++++++++++++++++++++-----------------
 1 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 4ff50cc..a305a88 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -397,7 +397,8 @@ static int ide_tape_callback(ide_drive_t *drive, int dsc)
 		if (readpos[0] & 0x4) {
 			printk(KERN_INFO "ide-tape: Block location is unknown"
 					 "to the tape\n");
-			clear_bit(IDE_AFLAG_ADDRESS_VALID, &drive->atapi_flags);
+			clear_bit(ilog2(IDE_AFLAG_ADDRESS_VALID),
+				  &drive->atapi_flags);
 			uptodate = 0;
 			err = IDE_DRV_ERROR_GENERAL;
 		} else {
@@ -406,7 +407,8 @@ static int ide_tape_callback(ide_drive_t *drive, int dsc)
 
 			tape->partition = readpos[1];
 			tape->first_frame = be32_to_cpup((__be32 *)&readpos[4]);
-			set_bit(IDE_AFLAG_ADDRESS_VALID, &drive->atapi_flags);
+			set_bit(ilog2(IDE_AFLAG_ADDRESS_VALID),
+				&drive->atapi_flags);
 		}
 	}
 
@@ -755,7 +757,7 @@ static int idetape_wait_ready(ide_drive_t *drive, unsigned long timeout)
 	int load_attempted = 0;
 
 	/* Wait for the tape to become ready */
-	set_bit(IDE_AFLAG_MEDIUM_PRESENT, &drive->atapi_flags);
+	set_bit(ilog2(IDE_AFLAG_MEDIUM_PRESENT), &drive->atapi_flags);
 	timeout += jiffies;
 	while (time_before(jiffies, timeout)) {
 		if (ide_do_test_unit_ready(drive, disk) == 0)
@@ -831,7 +833,7 @@ static void __ide_tape_discard_merge_buffer(ide_drive_t *drive)
 	if (tape->chrdev_dir != IDETAPE_DIR_READ)
 		return;
 
-	clear_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags);
+	clear_bit(ilog2(IDE_AFLAG_FILEMARK), &drive->atapi_flags);
 	tape->valid = 0;
 	if (tape->buf != NULL) {
 		kfree(tape->buf);
@@ -1124,7 +1126,8 @@ static int idetape_space_over_filemarks(ide_drive_t *drive, short mt_op,
 
 	if (tape->chrdev_dir == IDETAPE_DIR_READ) {
 		tape->valid = 0;
-		if (test_and_clear_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags))
+		if (test_and_clear_bit(ilog2(IDE_AFLAG_FILEMARK),
+				       &drive->atapi_flags))
 			++count;
 		ide_tape_discard_merge_buffer(drive, 0);
 	}
@@ -1179,7 +1182,7 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
 	debug_log(DBG_CHRDEV, "Enter %s, count %Zd\n", __func__, count);
 
 	if (tape->chrdev_dir != IDETAPE_DIR_READ) {
-		if (test_bit(IDE_AFLAG_DETECT_BS, &drive->atapi_flags))
+		if (test_bit(ilog2(IDE_AFLAG_DETECT_BS), &drive->atapi_flags))
 			if (count > tape->blk_size &&
 			    (count % tape->blk_size) == 0)
 				tape->user_bs_factor = count / tape->blk_size;
@@ -1195,7 +1198,8 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
 		/* refill if staging buffer is empty */
 		if (!tape->valid) {
 			/* If we are at a filemark, nothing more to read */
-			if (test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags))
+			if (test_bit(ilog2(IDE_AFLAG_FILEMARK),
+				     &drive->atapi_flags))
 				break;
 			/* read */
 			if (idetape_queue_rw_tail(drive, REQ_IDETAPE_READ,
@@ -1213,7 +1217,7 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
 		done += todo;
 	}
 
-	if (!done && test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags)) {
+	if (!done && test_bit(ilog2(IDE_AFLAG_FILEMARK), &drive->atapi_flags)) {
 		debug_log(DBG_SENSE, "%s: spacing over filemark\n", tape->name);
 
 		idetape_space_over_filemarks(drive, MTFSF, 1);
@@ -1347,7 +1351,8 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
 		ide_tape_discard_merge_buffer(drive, 0);
 		retval = ide_do_start_stop(drive, disk, !IDETAPE_LU_LOAD_MASK);
 		if (!retval)
-			clear_bit(IDE_AFLAG_MEDIUM_PRESENT, &drive->atapi_flags);
+			clear_bit(ilog2(IDE_AFLAG_MEDIUM_PRESENT),
+				  &drive->atapi_flags);
 		return retval;
 	case MTNOP:
 		ide_tape_discard_merge_buffer(drive, 0);
@@ -1369,9 +1374,11 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
 			    mt_count % tape->blk_size)
 				return -EIO;
 			tape->user_bs_factor = mt_count / tape->blk_size;
-			clear_bit(IDE_AFLAG_DETECT_BS, &drive->atapi_flags);
+			clear_bit(ilog2(IDE_AFLAG_DETECT_BS),
+				  &drive->atapi_flags);
 		} else
-			set_bit(IDE_AFLAG_DETECT_BS, &drive->atapi_flags);
+			set_bit(ilog2(IDE_AFLAG_DETECT_BS),
+				&drive->atapi_flags);
 		return 0;
 	case MTSEEK:
 		ide_tape_discard_merge_buffer(drive, 0);
@@ -1516,20 +1523,20 @@ static int idetape_chrdev_open(struct inode *inode, struct file *filp)
 
 	filp->private_data = tape;
 
-	if (test_and_set_bit(IDE_AFLAG_BUSY, &drive->atapi_flags)) {
+	if (test_and_set_bit(ilog2(IDE_AFLAG_BUSY), &drive->atapi_flags)) {
 		retval = -EBUSY;
 		goto out_put_tape;
 	}
 
 	retval = idetape_wait_ready(drive, 60 * HZ);
 	if (retval) {
-		clear_bit(IDE_AFLAG_BUSY, &drive->atapi_flags);
+		clear_bit(ilog2(IDE_AFLAG_BUSY), &drive->atapi_flags);
 		printk(KERN_ERR "ide-tape: %s: drive not ready\n", tape->name);
 		goto out_put_tape;
 	}
 
 	idetape_read_position(drive);
-	if (!test_bit(IDE_AFLAG_ADDRESS_VALID, &drive->atapi_flags))
+	if (!test_bit(ilog2(IDE_AFLAG_ADDRESS_VALID), &drive->atapi_flags))
 		(void)idetape_rewind_tape(drive);
 
 	/* Read block size and write protect status from drive. */
@@ -1545,7 +1552,7 @@ static int idetape_chrdev_open(struct inode *inode, struct file *filp)
 	if (tape->write_prot) {
 		if ((filp->f_flags & O_ACCMODE) == O_WRONLY ||
 		    (filp->f_flags & O_ACCMODE) == O_RDWR) {
-			clear_bit(IDE_AFLAG_BUSY, &drive->atapi_flags);
+			clear_bit(ilog2(IDE_AFLAG_BUSY), &drive->atapi_flags);
 			retval = -EROFS;
 			goto out_put_tape;
 		}
@@ -1602,15 +1609,17 @@ static int idetape_chrdev_release(struct inode *inode, struct file *filp)
 			ide_tape_discard_merge_buffer(drive, 1);
 	}
 
-	if (minor < 128 && test_bit(IDE_AFLAG_MEDIUM_PRESENT, &drive->atapi_flags))
+	if (minor < 128 && test_bit(ilog2(IDE_AFLAG_MEDIUM_PRESENT),
+				    &drive->atapi_flags))
 		(void) idetape_rewind_tape(drive);
+
 	if (tape->chrdev_dir == IDETAPE_DIR_NONE) {
 		if (tape->door_locked == DOOR_LOCKED) {
 			if (!ide_set_media_lock(drive, tape->disk, 0))
 				tape->door_locked = DOOR_UNLOCKED;
 		}
 	}
-	clear_bit(IDE_AFLAG_BUSY, &drive->atapi_flags);
+	clear_bit(ilog2(IDE_AFLAG_BUSY), &drive->atapi_flags);
 	ide_tape_put(tape);
 	unlock_kernel();
 	return 0;
-- 
1.6.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] ide-tape: change IDE_AFLAG_IGNORE_DSC non-atomically
  2009-06-02  7:05   ` Borislav Petkov
  (?)
@ 2009-06-02 10:18   ` Bartlomiej Zolnierkiewicz
  2009-06-02 13:08     ` Borislav Petkov
  -1 siblings, 1 reply; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-06-02 10:18 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: linux-ide, linux-kernel


Hi,

On Tuesday 02 June 2009 09:05:07 Borislav Petkov wrote:
> There are two sites where the flag is being changed: ide_retry_pc
> and idetape_do_request. Both codepaths are protected by hwif->busy
> (ide_lock_port) and therefore we shouldn't need the atomic accesses. The
> only problem would be the compiler reordering the accesses, therefore the
> optimization barrier.
> 
> Spotted-by: Jiri Slaby <jirislaby@gmail.com>
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

[...]

> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -656,15 +656,24 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
>  
>  	if ((drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) == 0 &&
>  	    (rq->cmd[13] & REQ_IDETAPE_PC2) == 0)
> -		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
> +		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
>  
>  	if (drive->dev_flags & IDE_DFLAG_POST_RESET) {
> -		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
> +		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
>  		drive->dev_flags &= ~IDE_DFLAG_POST_RESET;
>  	}
>  
> -	if (!test_and_clear_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags) &&
> -	    (stat & ATA_DSC) == 0) {
> +	/*
> +	 * This is a precaution for IDE_AFLAG_IGNORE_DSC being conditionally set
> +	 * above. We don't need a stronger enforcement of ordering because the
> +	 * read below cannot precede the earlier write out-of-order since it is
> +	 * to the same location. Also, since we have the ide port locked during
> +	 * the ->do_request(), we only have to be aware of gcc reordering stuff.
> +	 */
> +	barrier();

Are you seeing a real problem with gcc here?  No sane compiler should need
a barrier() here (we would probably need zillions of them in kernel if it
really does).

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] ide-tape: change IDE_AFLAG_IGNORE_DSC non-atomically
  2009-06-02 10:18   ` Bartlomiej Zolnierkiewicz
@ 2009-06-02 13:08     ` Borislav Petkov
  2009-06-02 13:17       ` Bartlomiej Zolnierkiewicz
  0 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2009-06-02 13:08 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide, linux-kernel

Hi,

On Tue, Jun 2, 2009 at 12:18 PM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:

..

>> --- a/drivers/ide/ide-tape.c
>> +++ b/drivers/ide/ide-tape.c
>> @@ -656,15 +656,24 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
>>
>>       if ((drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) == 0 &&
>>           (rq->cmd[13] & REQ_IDETAPE_PC2) == 0)
>> -             set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
>> +             drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
>>
>>       if (drive->dev_flags & IDE_DFLAG_POST_RESET) {
>> -             set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
>> +             drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
>>               drive->dev_flags &= ~IDE_DFLAG_POST_RESET;
>>       }
>>
>> -     if (!test_and_clear_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags) &&
>> -         (stat & ATA_DSC) == 0) {
>> +     /*
>> +      * This is a precaution for IDE_AFLAG_IGNORE_DSC being conditionally set
>> +      * above. We don't need a stronger enforcement of ordering because the
>> +      * read below cannot precede the earlier write out-of-order since it is
>> +      * to the same location. Also, since we have the ide port locked during
>> +      * the ->do_request(), we only have to be aware of gcc reordering stuff.
>> +      */
>> +     barrier();
>
> Are you seeing a real problem with gcc here?  No sane compiler should need
> a barrier() here (we would probably need zillions of them in kernel if it
> really does).

No, this is just a precaution. The asm I checked looked fine but since
the flag is set and right afterwards checked, it will be bad if this
somehow got reordered. I actually haven't checked whether anything like
that would be possible, at all.

-- 
Regards/Gruss,
Boris

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] ide-tape: change IDE_AFLAG_IGNORE_DSC non-atomically
  2009-06-02 13:08     ` Borislav Petkov
@ 2009-06-02 13:17       ` Bartlomiej Zolnierkiewicz
  2009-06-03  5:39         ` Borislav Petkov
  0 siblings, 1 reply; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-06-02 13:17 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: linux-ide, linux-kernel

On Tuesday 02 June 2009 15:08:27 Borislav Petkov wrote:
> Hi,
> 
> On Tue, Jun 2, 2009 at 12:18 PM, Bartlomiej Zolnierkiewicz
> <bzolnier@gmail.com> wrote:
> 
> ..
> 
> >> --- a/drivers/ide/ide-tape.c
> >> +++ b/drivers/ide/ide-tape.c
> >> @@ -656,15 +656,24 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
> >>
> >>       if ((drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) == 0 &&
> >>           (rq->cmd[13] & REQ_IDETAPE_PC2) == 0)
> >> -             set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
> >> +             drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
> >>
> >>       if (drive->dev_flags & IDE_DFLAG_POST_RESET) {
> >> -             set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
> >> +             drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
> >>               drive->dev_flags &= ~IDE_DFLAG_POST_RESET;
> >>       }
> >>
> >> -     if (!test_and_clear_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags) &&
> >> -         (stat & ATA_DSC) == 0) {
> >> +     /*
> >> +      * This is a precaution for IDE_AFLAG_IGNORE_DSC being conditionally set
> >> +      * above. We don't need a stronger enforcement of ordering because the
> >> +      * read below cannot precede the earlier write out-of-order since it is
> >> +      * to the same location. Also, since we have the ide port locked during
> >> +      * the ->do_request(), we only have to be aware of gcc reordering stuff.
> >> +      */
> >> +     barrier();
> >
> > Are you seeing a real problem with gcc here?  No sane compiler should need
> > a barrier() here (we would probably need zillions of them in kernel if it
> > really does).
> 
> No, this is just a precaution. The asm I checked looked fine but since
> the flag is set and right afterwards checked, it will be bad if this
> somehow got reordered. I actually haven't checked whether anything like
> that would be possible, at all.

Please remove it then.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] ide-tape: change IDE_AFLAG_IGNORE_DSC non-atomically
  2009-06-02 13:17       ` Bartlomiej Zolnierkiewicz
@ 2009-06-03  5:39         ` Borislav Petkov
  0 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2009-06-03  5:39 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide, linux-kernel

Hi,

> > > Are you seeing a real problem with gcc here?  No sane compiler should need
> > > a barrier() here (we would probably need zillions of them in kernel if it
> > > really does).
> > 
> > No, this is just a precaution. The asm I checked looked fine but since
> > the flag is set and right afterwards checked, it will be bad if this
> > somehow got reordered. I actually haven't checked whether anything like
> > that would be possible, at all.
> 
> Please remove it then.

---

From: Borislav Petkov <petkovbb@gmail.com>
Date: Mon, 1 Jun 2009 13:43:49 +0200
Subject: [PATCH 1/2] ide-tape: change IDE_AFLAG_IGNORE_DSC non-atomically

There are two sites where the flag is being changed: ide_retry_pc
and idetape_do_request. Both codepaths are protected by hwif->busy
(ide_lock_port) and therefore we shouldn't need the atomic accesses.

Spotted-by: Jiri Slaby <jirislaby@gmail.com>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-atapi.c |    2 +-
 drivers/ide/ide-tape.c  |   12 +++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index afe5a43..fbcb851 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -258,7 +258,7 @@ void ide_retry_pc(ide_drive_t *drive)
 	pc->req_xfer = sense_rq->data_len;
 
 	if (drive->media == ide_tape)
-		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
+		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
 
 	if (ide_queue_sense_rq(drive, pc))
 		ide_complete_rq(drive, -EIO, blk_rq_bytes(drive->hwif->rq));
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 203bbea..f1d3c7b 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -656,15 +656,15 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 
 	if ((drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) == 0 &&
 	    (rq->cmd[13] & REQ_IDETAPE_PC2) == 0)
-		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
+		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
 
 	if (drive->dev_flags & IDE_DFLAG_POST_RESET) {
-		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
+		drive->atapi_flags |= IDE_AFLAG_IGNORE_DSC;
 		drive->dev_flags &= ~IDE_DFLAG_POST_RESET;
 	}
 
-	if (!test_and_clear_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags) &&
-	    (stat & ATA_DSC) == 0) {
+	if (!(drive->atapi_flags & IDE_AFLAG_IGNORE_DSC) &&
+	    !(stat & ATA_DSC)) {
 		if (postponed_rq == NULL) {
 			tape->dsc_polling_start = jiffies;
 			tape->dsc_poll_freq = tape->best_dsc_rw_freq;
@@ -684,7 +684,9 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 			tape->dsc_poll_freq = IDETAPE_DSC_MA_SLOW;
 		idetape_postpone_request(drive);
 		return ide_stopped;
-	}
+	} else
+		drive->atapi_flags &= ~IDE_AFLAG_IGNORE_DSC;
+
 	if (rq->cmd[13] & REQ_IDETAPE_READ) {
 		pc = &tape->queued_pc;
 		ide_tape_create_rw_cmd(tape, pc, rq, READ_6);
-- 
1.6.3.1


-- 
Regards/Gruss,
    Boris.

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] ide-tape: fix flags
  2009-06-02  7:05 [PATCH 0/2] ide-tape: fix flags Borislav Petkov
  2009-06-02  7:05   ` Borislav Petkov
  2009-06-02  7:05   ` Borislav Petkov
@ 2009-06-05 18:34 ` Bartlomiej Zolnierkiewicz
  2009-06-06 17:58   ` Borislav Petkov
  2 siblings, 1 reply; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-06-05 18:34 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: linux-ide, linux-kernel

On Tuesday 02 June 2009 09:05:06 Borislav Petkov wrote:
> Hi Bart,
> 
> those fix a stupid flags misuse that got spotted by Jiri Slaby. Please
> pay special attention to the first patch. FWIW, those can go as hot
> fixes right away and we might catch 30-rc8 since it's not out yet.

The problem is that patches seem to be against ide-2.6.git/for-next...

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] ide-tape: fix flags
  2009-06-05 18:34 ` [PATCH 0/2] ide-tape: fix flags Bartlomiej Zolnierkiewicz
@ 2009-06-06 17:58   ` Borislav Petkov
  2009-06-07 13:04     ` Bartlomiej Zolnierkiewicz
  0 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2009-06-06 17:58 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide, linux-kernel

On Fri, Jun 05, 2009 at 08:34:06PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 02 June 2009 09:05:06 Borislav Petkov wrote:
> > Hi Bart,
> > 
> > those fix a stupid flags misuse that got spotted by Jiri Slaby. Please
> > pay special attention to the first patch. FWIW, those can go as hot
> > fixes right away and we might catch 30-rc8 since it's not out yet.
> 
> The problem is that patches seem to be against ide-2.6.git/for-next...

Sh*t, this is hitting the broken buffer allocation that Tejun removed
(I'm getting the same OOM-kill as the one Tejun's mentioning here
http://marc.info/?l=linux-ide&m=124191711928890) so even with those
patches, ide-tape is still broken.

I'm guessing backport all of Tejun's ide-tape cleanup stuff after Linus
pulls them into 31-rc1?

-- 
Regards/Gruss,
    Boris.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] ide-tape: fix flags
  2009-06-06 17:58   ` Borislav Petkov
@ 2009-06-07 13:04     ` Bartlomiej Zolnierkiewicz
  0 siblings, 0 replies; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-06-07 13:04 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: linux-ide, linux-kernel, Jiri Slaby


[ added Jiri to cc: ]

On Saturday 06 June 2009 19:58:36 Borislav Petkov wrote:
> On Fri, Jun 05, 2009 at 08:34:06PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Tuesday 02 June 2009 09:05:06 Borislav Petkov wrote:
> > > Hi Bart,
> > > 
> > > those fix a stupid flags misuse that got spotted by Jiri Slaby. Please
> > > pay special attention to the first patch. FWIW, those can go as hot
> > > fixes right away and we might catch 30-rc8 since it's not out yet.
> > 
> > The problem is that patches seem to be against ide-2.6.git/for-next...
> 
> Sh*t, this is hitting the broken buffer allocation that Tejun removed
> (I'm getting the same OOM-kill as the one Tejun's mentioning here
> http://marc.info/?l=linux-ide&m=124191711928890) so even with those
> patches, ide-tape is still broken.
> 
> I'm guessing backport all of Tejun's ide-tape cleanup stuff after Linus
> pulls them into 31-rc1?

Seems so, though it may be too much work for too little gain
(we still have other open ide-tape issues like DMA related one).

In the meantime I applied your patches to for-next..

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2009-06-07 13:44 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-02  7:05 [PATCH 0/2] ide-tape: fix flags Borislav Petkov
2009-06-02  7:05 ` [PATCH 1/2] ide-tape: change IDE_AFLAG_IGNORE_DSC non-atomically Borislav Petkov
2009-06-02  7:05   ` Borislav Petkov
2009-06-02 10:18   ` Bartlomiej Zolnierkiewicz
2009-06-02 13:08     ` Borislav Petkov
2009-06-02 13:17       ` Bartlomiej Zolnierkiewicz
2009-06-03  5:39         ` Borislav Petkov
2009-06-02  7:05 ` [PATCH 2/2] ide-tape: fix IDE_AFLAG_* atomic accesses Borislav Petkov
2009-06-02  7:05   ` Borislav Petkov
2009-06-05 18:34 ` [PATCH 0/2] ide-tape: fix flags Bartlomiej Zolnierkiewicz
2009-06-06 17:58   ` Borislav Petkov
2009-06-07 13:04     ` Bartlomiej Zolnierkiewicz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.