Hi, Bartlomiej, Jens.
This RFC patchset cleans up ide-tape. Note that the patchset
currently is only compile tested, so it's likely to be broken, so the
RFC status. Albert Lee is sending me some IDE tape drives, so I'll
test these patches as soon as I receive them and repost with whatever
necessary fixes.
The ide-tape drive is quite interesting in that it implements its own
multi-segment buffer management using struct idetape_bh. Its rw path
doesn't use standard IDE or block data transfer mechanisms and
sometimes abuses them in interesting ways.
This patchset converts ide-tape to use bio like the rest of the world.
Currently, this is done using single contiguous buffer which uses an
order higher allocation than the original code if the buffer size is
not power of two. The maximum being single order 4 allocation per
device, which is the same as the original code, I don't think this is
an issue but if it ever is adopting sg-based multi-segment buffer
handling isn't difficult and can be nicely built on top of the updated
bio-based code.
This patchset simplifies ide-tape quite a bit as will be show in the
diffstat at the end of the message and also simplifies ide-atapi and
ide-io a bit. Most importantly, it enables further work on block
layer by unifying API usage.
This patchset contains the following ten patches.
0001-ide-atapi-allow-pc_callback-to-change-rq-data_.patch
0002-ide-tape-use-single-continuous-buffer.patch
0003-ide-tape-convert-to-bio.patch
0004-ide-tape-use-standard-data-transfer-mechanism.patch
0005-ide-tape-kill-idetape_bh.patch
0006-ide-tape-unify-r-w-init-paths.patch
0007-ide-tape-use-byte-size-instead-of-sectors-on-rw-iss.patch
0008-ide-tape-simplify-read-write-functions.patch
0009-ide-atapi-kill-unused-fields-and-callbacks.patch
0010-ide-drop-rq-data-handling-from-ide_map_sg.patch
0001-0005 converts ide-tape to bio and kills idetape_bh. 0006-0008
makes additional clean ups on ide-tape. 0009-0010 removes now
unnecessary stuff from ide-atapi and ide-io.
This patchset is on top of linux-next pata-2.6 tree as of 2009-03-23 +
ide-rq-buffer-data-special-and-misc-cleanups patchset[1] and available
in the following git tree.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git ide-phase2
http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=shortlog;h=ide-phase2
diffstat follows.
drivers/ide/ide-atapi.c | 31 --
drivers/ide/ide-io.c | 6
drivers/ide/ide-tape.c | 714 +++++++++---------------------------------------
include/linux/ide.h | 12
4 files changed, 160 insertions(+), 603 deletions(-)
Thanks.
--
tejun
[1] http://thread.gmane.org/gmane.linux.ide/39275
Impact: use standard way to transfer data
Now that data area is represented with bio, there is no need to use
custom data transfer methods. Drop idetape_io_buffers() and
idetape_update_buffers(). pc->bh is set to null to tell ide-atapi to
use standard data transfer mechanism and idetape_bh byte counts are
updated by the issuer on completion using the residual count.
Signed-off-by: Tejun Heo <[email protected]>
---
drivers/ide/ide-tape.c | 84 +++--------------------------------------------
1 files changed, 6 insertions(+), 78 deletions(-)
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index bd0e839..dd37fd7 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -292,65 +292,6 @@ static struct ide_tape_obj *ide_tape_chrdev_get(unsigned int i)
return tape;
}
-static int idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
- unsigned int bcount)
-{
- struct idetape_bh *bh = pc->bh;
- int count;
-
- if (bcount && bh) {
- count = min(
- (unsigned int)(bh->b_size - atomic_read(&bh->b_count)),
- bcount);
- drive->hwif->tp_ops->input_data(drive, NULL, bh->b_data +
- atomic_read(&bh->b_count), count);
- bcount -= count;
- atomic_add(count, &bh->b_count);
- if (atomic_read(&bh->b_count) == bh->b_size)
- pc->bh = NULL;
- }
-
- return bcount;
-}
-
-static int idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
- unsigned int bcount)
-{
- struct idetape_bh *bh = pc->bh;
- int count;
-
- if (bcount && bh) {
- count = min((unsigned int)pc->b_count, (unsigned int)bcount);
- drive->hwif->tp_ops->output_data(drive, NULL, pc->b_data, count);
- bcount -= count;
- pc->b_data += count;
- pc->b_count -= count;
- if (!pc->b_count)
- pc->bh = NULL;
- }
-
- return bcount;
-}
-
-static void idetape_update_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc)
-{
- struct idetape_bh *bh = pc->bh;
- unsigned int bcount = pc->xferred;
-
- if (pc->flags & PC_FLAG_WRITING)
- return;
- if (bcount) {
- if (bh == NULL || bcount > bh->b_size) {
- printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
- __func__);
- return;
- }
- atomic_set(&bh->b_count, bcount);
- if (atomic_read(&bh->b_count) == bh->b_size)
- pc->bh = NULL;
- }
-}
-
/*
* called on each failed packet command retry to analyze the request sense. We
* currently do not utilize this information.
@@ -368,12 +309,10 @@ static void idetape_analyze_error(ide_drive_t *drive, u8 *sense)
pc->c[0], tape->sense_key, tape->asc, tape->ascq);
/* Correct pc->xferred by asking the tape. */
- if (pc->flags & PC_FLAG_DMA_ERROR) {
+ if (pc->flags & PC_FLAG_DMA_ERROR)
pc->xferred = pc->req_xfer -
tape->blk_size *
get_unaligned_be32(&sense[3]);
- idetape_update_buffers(drive, pc);
- }
/*
* If error was the result of a zero-length read or write command,
@@ -520,19 +459,6 @@ static void ide_tape_handle_dsc(ide_drive_t *drive)
idetape_postpone_request(drive);
}
-static int ide_tape_io_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
- unsigned int bcount, int write)
-{
- unsigned int bleft;
-
- if (write)
- bleft = idetape_output_buffers(drive, pc, bcount);
- else
- bleft = idetape_input_buffers(drive, pc, bcount);
-
- return bcount - bleft;
-}
-
/*
* Packet Command Interface
*
@@ -685,7 +611,7 @@ static void ide_tape_create_rw_cmd(idetape_tape_t *tape,
ide_init_pc(pc);
put_unaligned(cpu_to_be32(length), (unsigned int *) &pc->c[1]);
pc->c[1] = 1;
- pc->bh = bh;
+ pc->bh = NULL;
pc->buf = NULL;
pc->buf_size = length * tape->blk_size;
pc->req_xfer = pc->buf_size;
@@ -1083,7 +1009,11 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
blk_execute_rq(drive->queue, tape->disk, rq, 0);
+ /* calculate the number of transferred bytes and update bh */
size -= rq->data_len;
+ if (cmd == REQ_IDETAPE_READ)
+ atomic_add(size, &bh->b_count);
+
ret = size;
if (rq->errors == IDE_DRV_ERROR_GENERAL)
ret = -EIO;
@@ -2037,8 +1967,6 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)
u16 *ctl = (u16 *)&tape->caps[12];
drive->pc_callback = ide_tape_callback;
- drive->pc_update_buffers = idetape_update_buffers;
- drive->pc_io_buffers = ide_tape_io_buffers;
drive->dev_flags |= IDE_DFLAG_DSC_OVERLAP;
--
1.6.0.2
Impact: simpler buffer allocation and handling, fix DMA transfers
ide-tape has its own multiple buffer mechanism using struct
idetape_bh. It allocates buffer with decreasing order-of-two
allocations so that it results in minimum number of segments.
However, the implementation is quite complex and works in a way that
no other block or ide driver works necessitating a lot of special case
handling.
The benefit this complex allocation scheme brings is questionable as
PIO or DMA the number of segments (16 maximum) doesn't make any
noticeable difference and it also doesn't negate the need for multiple
order allocation which can fail under memory pressure or high
fragmentation although it does lower the highest order necessary by
one when the buffer size isn't power of two.
As the first step to remove the custom buffer management, this patch
makes ide-tape allocate single continous buffer. The maximum order is
four. I doubt the change would cause any trouble but if it ever
matters, it should be converted to regular sg mechanism like everyone
else and even in that case dropping custom buffer handling and moving
to standard mechanism first make sense as an intermediate step.
This patch makes the first bh to contain the whole buffer and drops
multi bh handling code. Following patches will make further changes.
This patch has the side effect of fixing DMA transfers. Previously,
commands were passed to DMA engine without DMA-mapping all the
segments.
Signed-off-by: Tejun Heo <[email protected]>
---
drivers/ide/ide-tape.c | 258 +++++++++++-------------------------------------
1 files changed, 59 insertions(+), 199 deletions(-)
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index d52f8b3..3a7bd98 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -134,7 +134,6 @@ enum {
struct idetape_bh {
u32 b_size;
atomic_t b_count;
- struct idetape_bh *b_reqnext;
char *b_data;
};
@@ -228,10 +227,6 @@ typedef struct ide_tape_obj {
char *b_data;
int b_count;
- int pages_per_buffer;
- /* Wasted space in each stage */
- int excess_bh_size;
-
/* Measures average tape speed */
unsigned long avg_time;
int avg_size;
@@ -303,9 +298,7 @@ static int idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
struct idetape_bh *bh = pc->bh;
int count;
- while (bcount) {
- if (bh == NULL)
- break;
+ if (bcount && bh) {
count = min(
(unsigned int)(bh->b_size - atomic_read(&bh->b_count)),
bcount);
@@ -313,15 +306,10 @@ static int idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
atomic_read(&bh->b_count), count);
bcount -= count;
atomic_add(count, &bh->b_count);
- if (atomic_read(&bh->b_count) == bh->b_size) {
- bh = bh->b_reqnext;
- if (bh)
- atomic_set(&bh->b_count, 0);
- }
+ if (atomic_read(&bh->b_count) == bh->b_size)
+ pc->bh = NULL;
}
- pc->bh = bh;
-
return bcount;
}
@@ -331,22 +319,14 @@ static int idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
struct idetape_bh *bh = pc->bh;
int count;
- while (bcount) {
- if (bh == NULL)
- break;
+ if (bcount && bh) {
count = min((unsigned int)pc->b_count, (unsigned int)bcount);
drive->hwif->tp_ops->output_data(drive, NULL, pc->b_data, count);
bcount -= count;
pc->b_data += count;
pc->b_count -= count;
- if (!pc->b_count) {
- bh = bh->b_reqnext;
- pc->bh = bh;
- if (bh) {
- pc->b_data = bh->b_data;
- pc->b_count = atomic_read(&bh->b_count);
- }
- }
+ if (!pc->b_count)
+ pc->bh = NULL;
}
return bcount;
@@ -355,24 +335,20 @@ static int idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
static void idetape_update_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc)
{
struct idetape_bh *bh = pc->bh;
- int count;
unsigned int bcount = pc->xferred;
if (pc->flags & PC_FLAG_WRITING)
return;
- while (bcount) {
- if (bh == NULL) {
+ if (bcount) {
+ if (bh == NULL || bcount > bh->b_size) {
printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
__func__);
return;
}
- count = min((unsigned int)bh->b_size, (unsigned int)bcount);
- atomic_set(&bh->b_count, count);
+ atomic_set(&bh->b_count, bcount);
if (atomic_read(&bh->b_count) == bh->b_size)
- bh = bh->b_reqnext;
- bcount -= count;
+ pc->bh = NULL;
}
- pc->bh = bh;
}
/*
@@ -439,24 +415,10 @@ static void idetape_analyze_error(ide_drive_t *drive, u8 *sense)
/* Free data buffers completely. */
static void ide_tape_kfree_buffer(idetape_tape_t *tape)
{
- struct idetape_bh *prev_bh, *bh = tape->merge_bh;
-
- while (bh) {
- u32 size = bh->b_size;
-
- while (size) {
- unsigned int order = fls(size >> PAGE_SHIFT)-1;
-
- if (bh->b_data)
- free_pages((unsigned long)bh->b_data, order);
+ struct idetape_bh *bh = tape->merge_bh;
- size &= (order-1);
- bh->b_data += (1 << order) * PAGE_SIZE;
- }
- prev_bh = bh;
- bh = bh->b_reqnext;
- kfree(prev_bh);
- }
+ kfree(bh->b_data);
+ kfree(bh);
}
static void ide_tape_handle_dsc(ide_drive_t *);
@@ -859,117 +821,50 @@ out:
}
/*
- * The function below uses __get_free_pages to allocate a data buffer of size
- * tape->buffer_size (or a bit more). We attempt to combine sequential pages as
- * much as possible.
- *
- * It returns a pointer to the newly allocated buffer, or NULL in case of
- * failure.
+ * It returns a pointer to the newly allocated buffer, or NULL in case
+ * of failure.
*/
static struct idetape_bh *ide_tape_kmalloc_buffer(idetape_tape_t *tape,
- int full, int clear)
-{
- struct idetape_bh *prev_bh, *bh, *merge_bh;
- int pages = tape->pages_per_buffer;
- unsigned int order, b_allocd;
- char *b_data = NULL;
-
- merge_bh = kmalloc(sizeof(struct idetape_bh), GFP_KERNEL);
- bh = merge_bh;
- if (bh == NULL)
- goto abort;
-
- order = fls(pages) - 1;
- bh->b_data = (char *) __get_free_pages(GFP_KERNEL, order);
- if (!bh->b_data)
- goto abort;
- b_allocd = (1 << order) * PAGE_SIZE;
- pages &= (order-1);
-
- if (clear)
- memset(bh->b_data, 0, b_allocd);
- bh->b_reqnext = NULL;
- bh->b_size = b_allocd;
- atomic_set(&bh->b_count, full ? bh->b_size : 0);
+ int full)
+{
+ struct idetape_bh *bh;
- while (pages) {
- order = fls(pages) - 1;
- b_data = (char *) __get_free_pages(GFP_KERNEL, order);
- if (!b_data)
- goto abort;
- b_allocd = (1 << order) * PAGE_SIZE;
-
- if (clear)
- memset(b_data, 0, b_allocd);
-
- /* newly allocated page frames below buffer header or ...*/
- if (bh->b_data == b_data + b_allocd) {
- bh->b_size += b_allocd;
- bh->b_data -= b_allocd;
- if (full)
- atomic_add(b_allocd, &bh->b_count);
- continue;
- }
- /* they are above the header */
- if (b_data == bh->b_data + bh->b_size) {
- bh->b_size += b_allocd;
- if (full)
- atomic_add(b_allocd, &bh->b_count);
- continue;
- }
- prev_bh = bh;
- bh = kmalloc(sizeof(struct idetape_bh), GFP_KERNEL);
- if (!bh) {
- free_pages((unsigned long) b_data, order);
- goto abort;
- }
- bh->b_reqnext = NULL;
- bh->b_data = b_data;
- bh->b_size = b_allocd;
- atomic_set(&bh->b_count, full ? bh->b_size : 0);
- prev_bh->b_reqnext = bh;
+ bh = kmalloc(sizeof(struct idetape_bh), GFP_KERNEL);
+ if (!bh)
+ return NULL;
- pages &= (order-1);
+ bh->b_data = kmalloc(tape->buffer_size, GFP_KERNEL);
+ if (!bh->b_data) {
+ kfree(bh);
+ return NULL;
}
- bh->b_size -= tape->excess_bh_size;
- if (full)
- atomic_sub(tape->excess_bh_size, &bh->b_count);
- return merge_bh;
-abort:
- ide_tape_kfree_buffer(tape);
- return NULL;
+ bh->b_size = tape->buffer_size;
+ atomic_set(&bh->b_count, full ? bh->b_size : 0);
+
+ return bh;
}
static int idetape_copy_stage_from_user(idetape_tape_t *tape,
const char __user *buf, int n)
{
struct idetape_bh *bh = tape->bh;
- int count;
int ret = 0;
- while (n) {
- if (bh == NULL) {
+ if (n) {
+ if (bh == NULL || n > bh->b_size - atomic_read(&bh->b_count)) {
printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
__func__);
return 1;
}
- count = min((unsigned int)
- (bh->b_size - atomic_read(&bh->b_count)),
- (unsigned int)n);
if (copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf,
- count))
+ n))
ret = 1;
- n -= count;
- atomic_add(count, &bh->b_count);
- buf += count;
- if (atomic_read(&bh->b_count) == bh->b_size) {
- bh = bh->b_reqnext;
- if (bh)
- atomic_set(&bh->b_count, 0);
- }
+ atomic_add(n, &bh->b_count);
+ if (atomic_read(&bh->b_count) == bh->b_size)
+ tape->bh = NULL;
}
- tape->bh = bh;
+
return ret;
}
@@ -977,30 +872,20 @@ static int idetape_copy_stage_to_user(idetape_tape_t *tape, char __user *buf,
int n)
{
struct idetape_bh *bh = tape->bh;
- int count;
int ret = 0;
- while (n) {
- if (bh == NULL) {
+ if (n) {
+ if (bh == NULL || n > tape->b_count) {
printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
__func__);
return 1;
}
- count = min(tape->b_count, n);
- if (copy_to_user(buf, tape->b_data, count))
+ if (copy_to_user(buf, tape->b_data, n))
ret = 1;
- n -= count;
- tape->b_data += count;
- tape->b_count -= count;
- buf += count;
- if (!tape->b_count) {
- bh = bh->b_reqnext;
- tape->bh = bh;
- if (bh) {
- tape->b_data = bh->b_data;
- tape->b_count = atomic_read(&bh->b_count);
- }
- }
+ tape->b_data += n;
+ tape->b_count -= n;
+ if (!tape->b_count)
+ tape->bh = NULL;
}
return ret;
}
@@ -1252,7 +1137,7 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
{
idetape_tape_t *tape = drive->driver_data;
- int blocks, min;
+ int blocks;
struct idetape_bh *bh;
if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {
@@ -1267,31 +1152,16 @@ static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
if (tape->merge_bh_size) {
blocks = tape->merge_bh_size / tape->blk_size;
if (tape->merge_bh_size % tape->blk_size) {
- unsigned int i;
-
+ unsigned int i = tape->blk_size -
+ tape->merge_bh_size % tape->blk_size;
blocks++;
- i = tape->blk_size - tape->merge_bh_size %
- tape->blk_size;
- bh = tape->bh->b_reqnext;
- while (bh) {
- atomic_set(&bh->b_count, 0);
- bh = bh->b_reqnext;
- }
bh = tape->bh;
- while (i) {
- if (bh == NULL) {
- printk(KERN_INFO "ide-tape: bug,"
- " bh NULL\n");
- break;
- }
- min = min(i, (unsigned int)(bh->b_size -
- atomic_read(&bh->b_count)));
+ if (bh) {
memset(bh->b_data + atomic_read(&bh->b_count),
- 0, min);
- atomic_add(min, &bh->b_count);
- i -= min;
- bh = bh->b_reqnext;
- }
+ 0, i);
+ atomic_add(i, &bh->b_count);
+ } else
+ printk(KERN_INFO "ide-tape: bug, bh NULL\n");
}
(void) idetape_add_chrdev_write_request(drive, blocks);
tape->merge_bh_size = 0;
@@ -1319,7 +1189,7 @@ static int idetape_init_read(ide_drive_t *drive)
" 0 now\n");
tape->merge_bh_size = 0;
}
- tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0, 0);
+ tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);
if (!tape->merge_bh)
return -ENOMEM;
tape->chrdev_dir = IDETAPE_DIR_READ;
@@ -1366,23 +1236,18 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
{
idetape_tape_t *tape = drive->driver_data;
- struct idetape_bh *bh;
+ struct idetape_bh *bh = tape->merge_bh;
int blocks;
while (bcount) {
unsigned int count;
- bh = tape->merge_bh;
count = min(tape->buffer_size, bcount);
bcount -= count;
blocks = count / tape->blk_size;
- while (count) {
- atomic_set(&bh->b_count,
- min(count, (unsigned int)bh->b_size));
- memset(bh->b_data, 0, atomic_read(&bh->b_count));
- count -= atomic_read(&bh->b_count);
- bh = bh->b_reqnext;
- }
+ atomic_set(&bh->b_count, count);
+ memset(bh->b_data, 0, atomic_read(&bh->b_count));
+
idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, blocks,
tape->merge_bh);
}
@@ -1594,7 +1459,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
"should be 0 now\n");
tape->merge_bh_size = 0;
}
- tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0, 0);
+ tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);
if (!tape->merge_bh)
return -ENOMEM;
tape->chrdev_dir = IDETAPE_DIR_WRITE;
@@ -1968,7 +1833,7 @@ static void idetape_write_release(ide_drive_t *drive, unsigned int minor)
idetape_tape_t *tape = drive->driver_data;
ide_tape_flush_merge_buffer(drive);
- tape->merge_bh = ide_tape_kmalloc_buffer(tape, 1, 0);
+ tape->merge_bh = ide_tape_kmalloc_buffer(tape, 1);
if (tape->merge_bh != NULL) {
idetape_pad_zeros(drive, tape->blk_size *
(tape->user_bs_factor - 1));
@@ -2199,11 +2064,6 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)
tape->buffer_size = *ctl * tape->blk_size;
}
buffer_size = tape->buffer_size;
- tape->pages_per_buffer = buffer_size / PAGE_SIZE;
- if (buffer_size % PAGE_SIZE) {
- tape->pages_per_buffer++;
- tape->excess_bh_size = PAGE_SIZE - buffer_size % PAGE_SIZE;
- }
/* select the "best" DSC read/write polling freq */
speed = max(*(u16 *)&tape->caps[14], *(u16 *)&tape->caps[8]);
--
1.6.0.2
---
drivers/ide/ide-atapi.c | 5 +----
drivers/ide/ide-tape.c | 39 ++++++++++++++++++++++-----------------
2 files changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 2b456c4..53e811d 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -372,10 +372,7 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
* ->pc_callback() might change rq->data_len for
* residual count, cache total length.
*/
- if (drive->media == ide_tape)
- done = ide_rq_bytes(rq); /* FIXME */
- else
- done = blk_rq_bytes(rq);
+ done = blk_rq_bytes(rq);
/* Command finished - Call the callback function */
uptodate = drive->pc_callback(drive, dsc);
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 3a7bd98..bd0e839 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -458,7 +458,7 @@ static int ide_tape_callback(ide_drive_t *drive, int dsc)
}
tape->first_frame += blocks;
- rq->current_nr_sectors -= blocks;
+ rq->data_len -= blocks * tape->blk_size;
if (pc->error) {
uptodate = 0;
@@ -799,10 +799,6 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
pc = (struct ide_atapi_pc *)rq->special;
rq->cmd[13] &= ~(REQ_IDETAPE_PC1);
rq->cmd[13] |= REQ_IDETAPE_PC2;
- if (pc->req_xfer) {
- ide_init_sg_cmd(&cmd, pc->req_xfer);
- ide_map_sg(drive, &cmd);
- }
goto out;
}
if (rq->cmd[13] & REQ_IDETAPE_PC2) {
@@ -812,6 +808,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
BUG();
out:
+ if (pc->req_xfer) {
+ ide_init_sg_cmd(&cmd, pc->req_xfer);
+ ide_map_sg(drive, &cmd);
+ }
if (rq_data_dir(rq))
cmd.tf_flags |= IDE_TFLAG_WRITE;
@@ -1061,32 +1061,37 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
struct idetape_bh *bh)
{
idetape_tape_t *tape = drive->driver_data;
+ size_t size = blocks * tape->blk_size;
struct request *rq;
- int ret, errors;
+ int ret;
debug_log(DBG_SENSE, "%s: cmd=%d\n", __func__, cmd);
+ BUG_ON(cmd != REQ_IDETAPE_READ && cmd != REQ_IDETAPE_WRITE);
rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
rq->cmd_type = REQ_TYPE_SPECIAL;
rq->cmd[13] = cmd;
rq->rq_disk = tape->disk;
- rq->special = (void *)bh;
rq->sector = tape->first_frame;
- rq->nr_sectors = blocks;
- rq->current_nr_sectors = blocks;
- blk_execute_rq(drive->queue, tape->disk, rq, 0);
- errors = rq->errors;
- ret = tape->blk_size * (blocks - rq->current_nr_sectors);
- blk_put_request(rq);
+ if (size) {
+ ret = blk_rq_map_kern(drive->queue, rq, bh->b_data, size,
+ __GFP_WAIT);
+ if (ret)
+ goto out_put;
+ }
- if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
- return 0;
+ blk_execute_rq(drive->queue, tape->disk, rq, 0);
+
+ size -= rq->data_len;
+ ret = size;
+ if (rq->errors == IDE_DRV_ERROR_GENERAL)
+ ret = -EIO;
if (tape->merge_bh)
idetape_init_merge_buffer(tape);
- if (errors == IDE_DRV_ERROR_GENERAL)
- return -EIO;
+out_put:
+ blk_put_request(rq);
return ret;
}
--
1.6.0.2
Impact: allow residual count implementation in ->pc_callback()
rq->data_len has two duties - carrying the number of input bytes on
issue and carrying residual count back to the issuer on completion.
ide-atapi completion callback ->pc_callback() is the right place to do
this but currently ide-atapi depends on rq->data_len carrying the
original request size after calling ->pc_callback() to complete the pc
request.
This patch makes ide_pc_intr() cache length to complete before calling
->pc_callback() so that it can modify rq->data_len as necessary.
Note: As using rq->data_len for two purposes can make cases like this
incorrect in subtle ways, future changes will introduce separate
field for residual count.
Signed-off-by: Tejun Heo <[email protected]>
Cc: Jens Axboe <[email protected]>
---
drivers/ide/ide-atapi.c | 19 +++++++++++--------
1 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index bc7dfcf..2b456c4 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -328,6 +328,7 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
/* No more interrupts */
if ((stat & ATA_DRQ) == 0) {
+ unsigned int done;
int uptodate;
debug_log("Packet command completed, %d bytes transferred\n",
@@ -367,6 +368,15 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
if ((pc->flags & PC_FLAG_WAIT_FOR_DSC) && (stat & ATA_DSC) == 0)
dsc = 1;
+ /*
+ * ->pc_callback() might change rq->data_len for
+ * residual count, cache total length.
+ */
+ if (drive->media == ide_tape)
+ done = ide_rq_bytes(rq); /* FIXME */
+ else
+ done = blk_rq_bytes(rq);
+
/* Command finished - Call the callback function */
uptodate = drive->pc_callback(drive, dsc);
@@ -375,20 +385,13 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
if (blk_special_request(rq)) {
rq->errors = 0;
- ide_complete_rq(drive, 0, blk_rq_bytes(rq));
+ ide_complete_rq(drive, 0, done);
} else {
- unsigned int done;
-
if (blk_fs_request(rq) == 0 && uptodate <= 0) {
if (rq->errors == 0)
rq->errors = -EIO;
}
- if (drive->media == ide_tape)
- done = ide_rq_bytes(rq); /* FIXME */
- else
- done = blk_rq_bytes(rq);
-
ide_complete_rq(drive, uptodate ? 0 : -EIO, done);
}
--
1.6.0.2
Impact: kill now unnecessary idetape_bh
With everything using standard mechanisms, there is no need for
idetape_bh anymore. Kill it and use tape->buf, cur and valid to
describe data buffer instead.
Changes worth mentioning are...
* idetape_queue_rq_tail() now always queue tape->buf and and adjusts
buffer state properly before completion.
* idetape_pad_zeros() clears the buffer only once.
Signed-off-by: Tejun Heo <[email protected]>
---
drivers/ide/ide-tape.c | 296 +++++++++++++-----------------------------------
1 files changed, 81 insertions(+), 215 deletions(-)
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index dd37fd7..e7f351b 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -131,12 +131,6 @@ enum {
IDETAPE_DIR_WRITE = (1 << 2),
};
-struct idetape_bh {
- u32 b_size;
- atomic_t b_count;
- char *b_data;
-};
-
/* Tape door status */
#define DOOR_UNLOCKED 0
#define DOOR_LOCKED 1
@@ -218,14 +212,12 @@ typedef struct ide_tape_obj {
/* Data buffer size chosen based on the tape's recommendation */
int buffer_size;
- /* merge buffer */
- struct idetape_bh *merge_bh;
- /* size of the merge buffer */
- int merge_bh_size;
- /* pointer to current buffer head within the merge buffer */
- struct idetape_bh *bh;
- char *b_data;
- int b_count;
+ /* Staging buffer of buffer_size bytes */
+ void *buf;
+ /* The read/write cursor */
+ void *cur;
+ /* The number of valid bytes in buf */
+ size_t valid;
/* Measures average tape speed */
unsigned long avg_time;
@@ -351,15 +343,6 @@ static void idetape_analyze_error(ide_drive_t *drive, u8 *sense)
}
}
-/* Free data buffers completely. */
-static void ide_tape_kfree_buffer(idetape_tape_t *tape)
-{
- struct idetape_bh *bh = tape->merge_bh;
-
- kfree(bh->b_data);
- kfree(bh);
-}
-
static void ide_tape_handle_dsc(ide_drive_t *);
static int ide_tape_callback(ide_drive_t *drive, int dsc)
@@ -605,7 +588,6 @@ static void ide_tape_create_rw_cmd(idetape_tape_t *tape,
struct ide_atapi_pc *pc, struct request *rq,
u8 opcode)
{
- struct idetape_bh *bh = (struct idetape_bh *)rq->special;
unsigned int length = rq->current_nr_sectors;
ide_init_pc(pc);
@@ -618,14 +600,11 @@ static void ide_tape_create_rw_cmd(idetape_tape_t *tape,
if (pc->req_xfer == tape->buffer_size)
pc->flags |= PC_FLAG_DMA_OK;
- if (opcode == READ_6) {
+ if (opcode == READ_6)
pc->c[0] = READ_6;
- atomic_set(&bh->b_count, 0);
- } else if (opcode == WRITE_6) {
+ else if (opcode == WRITE_6) {
pc->c[0] = WRITE_6;
pc->flags |= PC_FLAG_WRITING;
- pc->b_data = bh->b_data;
- pc->b_count = atomic_read(&bh->b_count);
}
memcpy(rq->cmd, pc->c, 12);
@@ -747,89 +726,6 @@ out:
}
/*
- * It returns a pointer to the newly allocated buffer, or NULL in case
- * of failure.
- */
-static struct idetape_bh *ide_tape_kmalloc_buffer(idetape_tape_t *tape,
- int full)
-{
- struct idetape_bh *bh;
-
- bh = kmalloc(sizeof(struct idetape_bh), GFP_KERNEL);
- if (!bh)
- return NULL;
-
- bh->b_data = kmalloc(tape->buffer_size, GFP_KERNEL);
- if (!bh->b_data) {
- kfree(bh);
- return NULL;
- }
-
- bh->b_size = tape->buffer_size;
- atomic_set(&bh->b_count, full ? bh->b_size : 0);
-
- return bh;
-}
-
-static int idetape_copy_stage_from_user(idetape_tape_t *tape,
- const char __user *buf, int n)
-{
- struct idetape_bh *bh = tape->bh;
- int ret = 0;
-
- if (n) {
- if (bh == NULL || n > bh->b_size - atomic_read(&bh->b_count)) {
- printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
- __func__);
- return 1;
- }
- if (copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf,
- n))
- ret = 1;
- atomic_add(n, &bh->b_count);
- if (atomic_read(&bh->b_count) == bh->b_size)
- tape->bh = NULL;
- }
-
- return ret;
-}
-
-static int idetape_copy_stage_to_user(idetape_tape_t *tape, char __user *buf,
- int n)
-{
- struct idetape_bh *bh = tape->bh;
- int ret = 0;
-
- if (n) {
- if (bh == NULL || n > tape->b_count) {
- printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
- __func__);
- return 1;
- }
- if (copy_to_user(buf, tape->b_data, n))
- ret = 1;
- tape->b_data += n;
- tape->b_count -= n;
- if (!tape->b_count)
- tape->bh = NULL;
- }
- return ret;
-}
-
-static void idetape_init_merge_buffer(idetape_tape_t *tape)
-{
- struct idetape_bh *bh = tape->merge_bh;
- tape->bh = tape->merge_bh;
-
- if (tape->chrdev_dir == IDETAPE_DIR_WRITE)
- atomic_set(&bh->b_count, 0);
- else {
- tape->b_data = bh->b_data;
- tape->b_count = atomic_read(&bh->b_count);
- }
-}
-
-/*
* Write a filemark if write_filemark=1. Flush the device buffers without
* writing a filemark otherwise.
*/
@@ -926,10 +822,10 @@ static void __ide_tape_discard_merge_buffer(ide_drive_t *drive)
return;
clear_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags);
- tape->merge_bh_size = 0;
- if (tape->merge_bh != NULL) {
- ide_tape_kfree_buffer(tape);
- tape->merge_bh = NULL;
+ tape->valid = 0;
+ if (tape->buf != NULL) {
+ kfree(tape->buf);
+ tape->buf = NULL;
}
tape->chrdev_dir = IDETAPE_DIR_NONE;
@@ -983,8 +879,7 @@ static void ide_tape_discard_merge_buffer(ide_drive_t *drive,
* Generate a read/write request for the block device interface and wait for it
* to be serviced.
*/
-static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
- struct idetape_bh *bh)
+static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks)
{
idetape_tape_t *tape = drive->driver_data;
size_t size = blocks * tape->blk_size;
@@ -1001,7 +896,7 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
rq->sector = tape->first_frame;
if (size) {
- ret = blk_rq_map_kern(drive->queue, rq, bh->b_data, size,
+ ret = blk_rq_map_kern(drive->queue, rq, tape->buf, size,
__GFP_WAIT);
if (ret)
goto out_put;
@@ -1009,17 +904,17 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
blk_execute_rq(drive->queue, tape->disk, rq, 0);
- /* calculate the number of transferred bytes and update bh */
+ /* calculate the number of transferred bytes and update buffer state */
size -= rq->data_len;
+ tape->cur = tape->buf;
if (cmd == REQ_IDETAPE_READ)
- atomic_add(size, &bh->b_count);
+ tape->valid = size;
+ else
+ tape->valid = 0;
ret = size;
if (rq->errors == IDE_DRV_ERROR_GENERAL)
ret = -EIO;
-
- if (tape->merge_bh)
- idetape_init_merge_buffer(tape);
out_put:
blk_put_request(rq);
return ret;
@@ -1061,49 +956,33 @@ static void idetape_create_space_cmd(struct ide_atapi_pc *pc, int count, u8 cmd)
/* Queue up a character device originated write request. */
static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
{
- idetape_tape_t *tape = drive->driver_data;
-
debug_log(DBG_CHRDEV, "Enter %s\n", __func__);
- return idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE,
- blocks, tape->merge_bh);
+ return idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, blocks);
}
static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
{
idetape_tape_t *tape = drive->driver_data;
int blocks;
- struct idetape_bh *bh;
if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {
printk(KERN_ERR "ide-tape: bug: Trying to empty merge buffer"
" but we are not writing.\n");
return;
}
- if (tape->merge_bh_size > tape->buffer_size) {
- printk(KERN_ERR "ide-tape: bug: merge_buffer too big\n");
- tape->merge_bh_size = tape->buffer_size;
- }
- if (tape->merge_bh_size) {
- blocks = tape->merge_bh_size / tape->blk_size;
- if (tape->merge_bh_size % tape->blk_size) {
- unsigned int i = tape->blk_size -
- tape->merge_bh_size % tape->blk_size;
+ if (tape->buf) {
+ blocks = tape->valid / tape->blk_size;
+ if (tape->valid % tape->blk_size) {
blocks++;
- bh = tape->bh;
- if (bh) {
- memset(bh->b_data + atomic_read(&bh->b_count),
- 0, i);
- atomic_add(i, &bh->b_count);
- } else
- printk(KERN_INFO "ide-tape: bug, bh NULL\n");
+ memset(tape->buf + tape->valid, 0,
+ tape->blk_size - tape->valid % tape->blk_size);
}
(void) idetape_add_chrdev_write_request(drive, blocks);
- tape->merge_bh_size = 0;
}
- if (tape->merge_bh != NULL) {
- ide_tape_kfree_buffer(tape);
- tape->merge_bh = NULL;
+ if (tape->buf != NULL) {
+ kfree(tape->buf);
+ tape->buf = NULL;
}
tape->chrdev_dir = IDETAPE_DIR_NONE;
}
@@ -1119,15 +998,15 @@ static int idetape_init_read(ide_drive_t *drive)
ide_tape_flush_merge_buffer(drive);
idetape_flush_tape_buffers(drive);
}
- if (tape->merge_bh || tape->merge_bh_size) {
- printk(KERN_ERR "ide-tape: merge_bh_size should be"
- " 0 now\n");
- tape->merge_bh_size = 0;
+ if (tape->buf || tape->valid) {
+ printk(KERN_ERR "ide-tape: valid should be 0 now\n");
+ tape->valid = 0;
}
- tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);
- if (!tape->merge_bh)
+ tape->buf = kmalloc(tape->buffer_size, GFP_KERNEL);
+ if (!tape->buf)
return -ENOMEM;
tape->chrdev_dir = IDETAPE_DIR_READ;
+ tape->cur = tape->buf;
/*
* Issue a read 0 command to ensure that DSC handshake is
@@ -1137,11 +1016,10 @@ static int idetape_init_read(ide_drive_t *drive)
*/
if (drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) {
bytes_read = idetape_queue_rw_tail(drive,
- REQ_IDETAPE_READ, 0,
- tape->merge_bh);
+ REQ_IDETAPE_READ, 0);
if (bytes_read < 0) {
- ide_tape_kfree_buffer(tape);
- tape->merge_bh = NULL;
+ kfree(tape->buf);
+ tape->buf = NULL;
tape->chrdev_dir = IDETAPE_DIR_NONE;
return bytes_read;
}
@@ -1154,8 +1032,6 @@ static int idetape_init_read(ide_drive_t *drive)
/* called from idetape_chrdev_read() to service a chrdev read request. */
static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
{
- idetape_tape_t *tape = drive->driver_data;
-
debug_log(DBG_PROCS, "Enter %s, %d blocks\n", __func__, blocks);
/* If we are at a filemark, return a read length of 0 */
@@ -1164,27 +1040,21 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
idetape_init_read(drive);
- return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,
- tape->merge_bh);
+ return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks);
}
static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
{
idetape_tape_t *tape = drive->driver_data;
- struct idetape_bh *bh = tape->merge_bh;
- int blocks;
+
+ memset(tape->buf, 0, tape->buffer_size);
while (bcount) {
- unsigned int count;
+ unsigned int count = min(tape->buffer_size, bcount);
- count = min(tape->buffer_size, bcount);
+ idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE,
+ count / tape->blk_size);
bcount -= count;
- blocks = count / tape->blk_size;
- atomic_set(&bh->b_count, count);
- memset(bh->b_data, 0, atomic_read(&bh->b_count));
-
- idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, blocks,
- tape->merge_bh);
}
}
@@ -1264,7 +1134,7 @@ static int idetape_space_over_filemarks(ide_drive_t *drive, short mt_op,
}
if (tape->chrdev_dir == IDETAPE_DIR_READ) {
- tape->merge_bh_size = 0;
+ tape->valid = 0;
if (test_and_clear_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags))
++count;
ide_tape_discard_merge_buffer(drive, 0);
@@ -1330,20 +1200,20 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
return rc;
if (count == 0)
return (0);
- if (tape->merge_bh_size) {
- actually_read = min((unsigned int)(tape->merge_bh_size),
- (unsigned int)count);
- if (idetape_copy_stage_to_user(tape, buf, actually_read))
+ if (tape->valid) {
+ actually_read = min_t(unsigned int, tape->valid, count);
+ if (copy_to_user(buf, tape->cur, actually_read))
ret = -EFAULT;
buf += actually_read;
- tape->merge_bh_size -= actually_read;
count -= actually_read;
+ tape->cur += actually_read;
+ tape->valid -= actually_read;
}
while (count >= tape->buffer_size) {
bytes_read = idetape_add_chrdev_read_request(drive, ctl);
if (bytes_read <= 0)
goto finish;
- if (idetape_copy_stage_to_user(tape, buf, bytes_read))
+ if (copy_to_user(buf, tape->cur, bytes_read))
ret = -EFAULT;
buf += bytes_read;
count -= bytes_read;
@@ -1354,10 +1224,11 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
if (bytes_read <= 0)
goto finish;
temp = min((unsigned long)count, (unsigned long)bytes_read);
- if (idetape_copy_stage_to_user(tape, buf, temp))
+ if (copy_to_user(buf, tape->cur, temp))
ret = -EFAULT;
actually_read += temp;
- tape->merge_bh_size = bytes_read-temp;
+ tape->cur += temp;
+ tape->valid -= temp;
}
finish:
if (!actually_read && test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags)) {
@@ -1389,16 +1260,15 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {
if (tape->chrdev_dir == IDETAPE_DIR_READ)
ide_tape_discard_merge_buffer(drive, 1);
- if (tape->merge_bh || tape->merge_bh_size) {
- printk(KERN_ERR "ide-tape: merge_bh_size "
- "should be 0 now\n");
- tape->merge_bh_size = 0;
+ if (tape->buf || tape->valid) {
+ printk(KERN_ERR "ide-tape: valid should be 0 now\n");
+ tape->valid = 0;
}
- tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);
- if (!tape->merge_bh)
+ tape->buf = kmalloc(tape->buffer_size, GFP_KERNEL);
+ if (!tape->buf)
return -ENOMEM;
tape->chrdev_dir = IDETAPE_DIR_WRITE;
- idetape_init_merge_buffer(tape);
+ tape->cur = tape->buf;
/*
* Issue a write 0 command to ensure that DSC handshake is
@@ -1408,11 +1278,10 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
*/
if (drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) {
ssize_t retval = idetape_queue_rw_tail(drive,
- REQ_IDETAPE_WRITE, 0,
- tape->merge_bh);
+ REQ_IDETAPE_WRITE, 0);
if (retval < 0) {
- ide_tape_kfree_buffer(tape);
- tape->merge_bh = NULL;
+ kfree(tape->buf);
+ tape->buf = NULL;
tape->chrdev_dir = IDETAPE_DIR_NONE;
return retval;
}
@@ -1420,23 +1289,19 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
}
if (count == 0)
return (0);
- if (tape->merge_bh_size) {
- if (tape->merge_bh_size >= tape->buffer_size) {
- printk(KERN_ERR "ide-tape: bug: merge buf too big\n");
- tape->merge_bh_size = 0;
- }
- actually_written = min((unsigned int)
- (tape->buffer_size - tape->merge_bh_size),
- (unsigned int)count);
- if (idetape_copy_stage_from_user(tape, buf, actually_written))
- ret = -EFAULT;
+ if (tape->valid < tape->buffer_size) {
+ actually_written = min_t(unsigned int,
+ tape->buffer_size - tape->valid,
+ count);
+ if (copy_from_user(tape->cur, buf, actually_written))
+ ret = -EFAULT;
buf += actually_written;
- tape->merge_bh_size += actually_written;
count -= actually_written;
+ tape->cur += actually_written;
+ tape->valid += actually_written;
- if (tape->merge_bh_size == tape->buffer_size) {
+ if (tape->valid == tape->buffer_size) {
ssize_t retval;
- tape->merge_bh_size = 0;
retval = idetape_add_chrdev_write_request(drive, ctl);
if (retval <= 0)
return (retval);
@@ -1444,7 +1309,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
}
while (count >= tape->buffer_size) {
ssize_t retval;
- if (idetape_copy_stage_from_user(tape, buf, tape->buffer_size))
+ if (copy_from_user(tape->cur, buf, tape->buffer_size))
ret = -EFAULT;
buf += tape->buffer_size;
count -= tape->buffer_size;
@@ -1455,9 +1320,10 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
}
if (count) {
actually_written += count;
- if (idetape_copy_stage_from_user(tape, buf, count))
+ if (copy_from_user(tape->cur, buf, count))
ret = -EFAULT;
- tape->merge_bh_size += count;
+ tape->cur += count;
+ tape->valid += count;
}
return ret ? ret : actually_written;
}
@@ -1620,7 +1486,7 @@ static int idetape_chrdev_ioctl(struct inode *inode, struct file *file,
idetape_flush_tape_buffers(drive);
}
if (cmd == MTIOCGET || cmd == MTIOCPOS) {
- block_offset = tape->merge_bh_size /
+ block_offset = tape->valid /
(tape->blk_size * tape->user_bs_factor);
position = idetape_read_position(drive);
if (position < 0)
@@ -1768,12 +1634,12 @@ static void idetape_write_release(ide_drive_t *drive, unsigned int minor)
idetape_tape_t *tape = drive->driver_data;
ide_tape_flush_merge_buffer(drive);
- tape->merge_bh = ide_tape_kmalloc_buffer(tape, 1);
- if (tape->merge_bh != NULL) {
+ tape->buf = kmalloc(tape->buffer_size, GFP_KERNEL);
+ if (tape->buf != NULL) {
idetape_pad_zeros(drive, tape->blk_size *
(tape->user_bs_factor - 1));
- ide_tape_kfree_buffer(tape);
- tape->merge_bh = NULL;
+ kfree(tape->buf);
+ tape->buf = NULL;
}
idetape_write_filemark(drive);
idetape_flush_tape_buffers(drive);
@@ -2039,7 +1905,7 @@ static void ide_tape_release(struct device *dev)
ide_drive_t *drive = tape->drive;
struct gendisk *g = tape->disk;
- BUG_ON(tape->merge_bh_size);
+ BUG_ON(tape->valid);
drive->dev_flags &= ~IDE_DFLAG_DSC_OVERLAP;
drive->driver_data = NULL;
--
1.6.0.2
Impact: cleanup
Read and write init paths are almost identical. Unify them into
idetape_init_rw().
Signed-off-by: Tejun Heo <[email protected]>
---
drivers/ide/ide-tape.c | 106 +++++++++++++++++++-----------------------------
1 files changed, 42 insertions(+), 64 deletions(-)
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index e7f351b..c013954 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -987,42 +987,46 @@ static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
tape->chrdev_dir = IDETAPE_DIR_NONE;
}
-static int idetape_init_read(ide_drive_t *drive)
+static int idetape_init_rw(ide_drive_t *drive, int dir)
{
idetape_tape_t *tape = drive->driver_data;
- int bytes_read;
- /* Initialize read operation */
- if (tape->chrdev_dir != IDETAPE_DIR_READ) {
- if (tape->chrdev_dir == IDETAPE_DIR_WRITE) {
- ide_tape_flush_merge_buffer(drive);
- idetape_flush_tape_buffers(drive);
- }
- if (tape->buf || tape->valid) {
- printk(KERN_ERR "ide-tape: valid should be 0 now\n");
- tape->valid = 0;
- }
- tape->buf = kmalloc(tape->buffer_size, GFP_KERNEL);
- if (!tape->buf)
- return -ENOMEM;
- tape->chrdev_dir = IDETAPE_DIR_READ;
- tape->cur = tape->buf;
+ if (tape->chrdev_dir == dir)
+ return 0;
- /*
- * Issue a read 0 command to ensure that DSC handshake is
- * switched from completion mode to buffer available mode.
- * No point in issuing this if DSC overlap isn't supported, some
- * drives (Seagate STT3401A) will return an error.
- */
- if (drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) {
- bytes_read = idetape_queue_rw_tail(drive,
- REQ_IDETAPE_READ, 0);
- if (bytes_read < 0) {
- kfree(tape->buf);
- tape->buf = NULL;
- tape->chrdev_dir = IDETAPE_DIR_NONE;
- return bytes_read;
- }
+ if (tape->chrdev_dir == IDETAPE_DIR_READ)
+ ide_tape_discard_merge_buffer(drive, 1);
+ else if (tape->chrdev_dir == IDETAPE_DIR_WRITE) {
+ ide_tape_flush_merge_buffer(drive);
+ idetape_flush_tape_buffers(drive);
+ }
+
+ if (tape->buf || tape->valid) {
+ printk(KERN_ERR "ide-tape: valid should be 0 now\n");
+ tape->valid = 0;
+ }
+
+ tape->buf = kmalloc(tape->buffer_size, GFP_KERNEL);
+ if (!tape->buf)
+ return -ENOMEM;
+ tape->chrdev_dir = dir;
+ tape->cur = tape->buf;
+
+ /*
+ * Issue a 0 rw command to ensure that DSC handshake is
+ * switched from completion mode to buffer available mode. No
+ * point in issuing this if DSC overlap isn't supported, some
+ * drives (Seagate STT3401A) will return an error.
+ */
+ if (drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) {
+ int rc;
+
+ rc = idetape_queue_rw_tail(drive, dir, 0);
+ if (rc < 0) {
+ kfree(tape->buf);
+ tape->buf = NULL;
+ tape->chrdev_dir = IDETAPE_DIR_NONE;
+ return rc;
}
}
@@ -1038,7 +1042,7 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
if (test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags))
return 0;
- idetape_init_read(drive);
+ idetape_init_rw(drive, IDETAPE_DIR_READ);
return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks);
}
@@ -1195,7 +1199,7 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
(count % tape->blk_size) == 0)
tape->user_bs_factor = count / tape->blk_size;
}
- rc = idetape_init_read(drive);
+ rc = idetape_init_rw(drive, IDETAPE_DIR_READ);
if (rc < 0)
return rc;
if (count == 0)
@@ -1249,6 +1253,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
ssize_t actually_written = 0;
ssize_t ret = 0;
u16 ctl = *(u16 *)&tape->caps[12];
+ int rc;
/* The drive is write protected. */
if (tape->write_prot)
@@ -1257,36 +1262,9 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
debug_log(DBG_CHRDEV, "Enter %s, count %Zd\n", __func__, count);
/* Initialize write operation */
- if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {
- if (tape->chrdev_dir == IDETAPE_DIR_READ)
- ide_tape_discard_merge_buffer(drive, 1);
- if (tape->buf || tape->valid) {
- printk(KERN_ERR "ide-tape: valid should be 0 now\n");
- tape->valid = 0;
- }
- tape->buf = kmalloc(tape->buffer_size, GFP_KERNEL);
- if (!tape->buf)
- return -ENOMEM;
- tape->chrdev_dir = IDETAPE_DIR_WRITE;
- tape->cur = tape->buf;
-
- /*
- * Issue a write 0 command to ensure that DSC handshake is
- * switched from completion mode to buffer available mode. No
- * point in issuing this if DSC overlap isn't supported, some
- * drives (Seagate STT3401A) will return an error.
- */
- if (drive->dev_flags & IDE_DFLAG_DSC_OVERLAP) {
- ssize_t retval = idetape_queue_rw_tail(drive,
- REQ_IDETAPE_WRITE, 0);
- if (retval < 0) {
- kfree(tape->buf);
- tape->buf = NULL;
- tape->chrdev_dir = IDETAPE_DIR_NONE;
- return retval;
- }
- }
- }
+ rc = idetape_init_rw(drive, IDETAPE_DIR_WRITE);
+ if (rc < 0)
+ return rc;
if (count == 0)
return (0);
if (tape->valid < tape->buffer_size) {
--
1.6.0.2
Impact: cleanup
Byte size is what most issue functions deal with, make
idetape_queue_rw_tail() and its wrappers take byte size instead of
sector counts. idetape_chrdev_read() and write() functions are
converted to use tape->buffer_size instead of ctl from tape->cap.
This cleans up code a little bit and will ease the next r/w
reimplementation.
Signed-off-by: Tejun Heo <[email protected]>
---
drivers/ide/ide-tape.c | 45 ++++++++++++++++++++-------------------------
1 files changed, 20 insertions(+), 25 deletions(-)
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index c013954..5fc3282 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -879,15 +879,15 @@ static void ide_tape_discard_merge_buffer(ide_drive_t *drive,
* Generate a read/write request for the block device interface and wait for it
* to be serviced.
*/
-static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks)
+static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int size)
{
idetape_tape_t *tape = drive->driver_data;
- size_t size = blocks * tape->blk_size;
struct request *rq;
int ret;
debug_log(DBG_SENSE, "%s: cmd=%d\n", __func__, cmd);
BUG_ON(cmd != REQ_IDETAPE_READ && cmd != REQ_IDETAPE_WRITE);
+ BUG_ON(size < 0 || size % tape->blk_size);
rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
rq->cmd_type = REQ_TYPE_SPECIAL;
@@ -954,17 +954,16 @@ static void idetape_create_space_cmd(struct ide_atapi_pc *pc, int count, u8 cmd)
}
/* Queue up a character device originated write request. */
-static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
+static int idetape_add_chrdev_write_request(ide_drive_t *drive, int size)
{
debug_log(DBG_CHRDEV, "Enter %s\n", __func__);
- return idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, blocks);
+ return idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, size);
}
static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
{
idetape_tape_t *tape = drive->driver_data;
- int blocks;
if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {
printk(KERN_ERR "ide-tape: bug: Trying to empty merge buffer"
@@ -972,15 +971,10 @@ static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
return;
}
if (tape->buf) {
- blocks = tape->valid / tape->blk_size;
- if (tape->valid % tape->blk_size) {
- blocks++;
- memset(tape->buf + tape->valid, 0,
- tape->blk_size - tape->valid % tape->blk_size);
- }
- (void) idetape_add_chrdev_write_request(drive, blocks);
- }
- if (tape->buf != NULL) {
+ size_t aligned = roundup(tape->valid, tape->blk_size);
+
+ memset(tape->cur + tape->valid, 0, aligned - tape->valid);
+ idetape_add_chrdev_write_request(drive, aligned);
kfree(tape->buf);
tape->buf = NULL;
}
@@ -1034,9 +1028,9 @@ static int idetape_init_rw(ide_drive_t *drive, int dir)
}
/* called from idetape_chrdev_read() to service a chrdev read request. */
-static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
+static int idetape_add_chrdev_read_request(ide_drive_t *drive, int size)
{
- debug_log(DBG_PROCS, "Enter %s, %d blocks\n", __func__, blocks);
+ debug_log(DBG_PROCS, "Enter %s, %d bytes\n", __func__, size);
/* If we are at a filemark, return a read length of 0 */
if (test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags))
@@ -1044,7 +1038,7 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
idetape_init_rw(drive, IDETAPE_DIR_READ);
- return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks);
+ return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, size);
}
static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
@@ -1056,8 +1050,7 @@ static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
while (bcount) {
unsigned int count = min(tape->buffer_size, bcount);
- idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE,
- count / tape->blk_size);
+ idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, count);
bcount -= count;
}
}
@@ -1189,7 +1182,6 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
ide_drive_t *drive = tape->drive;
ssize_t bytes_read, temp, actually_read = 0, rc;
ssize_t ret = 0;
- u16 ctl = *(u16 *)&tape->caps[12];
debug_log(DBG_CHRDEV, "Enter %s, count %Zd\n", __func__, count);
@@ -1214,7 +1206,8 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
tape->valid -= actually_read;
}
while (count >= tape->buffer_size) {
- bytes_read = idetape_add_chrdev_read_request(drive, ctl);
+ bytes_read = idetape_add_chrdev_read_request(drive,
+ tape->buffer_size);
if (bytes_read <= 0)
goto finish;
if (copy_to_user(buf, tape->cur, bytes_read))
@@ -1224,7 +1217,8 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
actually_read += bytes_read;
}
if (count) {
- bytes_read = idetape_add_chrdev_read_request(drive, ctl);
+ bytes_read = idetape_add_chrdev_read_request(drive,
+ tape->buffer_size);
if (bytes_read <= 0)
goto finish;
temp = min((unsigned long)count, (unsigned long)bytes_read);
@@ -1252,7 +1246,6 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
ide_drive_t *drive = tape->drive;
ssize_t actually_written = 0;
ssize_t ret = 0;
- u16 ctl = *(u16 *)&tape->caps[12];
int rc;
/* The drive is write protected. */
@@ -1280,7 +1273,8 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
if (tape->valid == tape->buffer_size) {
ssize_t retval;
- retval = idetape_add_chrdev_write_request(drive, ctl);
+ retval = idetape_add_chrdev_write_request(drive,
+ tape->buffer_size);
if (retval <= 0)
return (retval);
}
@@ -1291,7 +1285,8 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
ret = -EFAULT;
buf += tape->buffer_size;
count -= tape->buffer_size;
- retval = idetape_add_chrdev_write_request(drive, ctl);
+ retval = idetape_add_chrdev_write_request(drive,
+ tape->buffer_size);
actually_written += tape->buffer_size;
if (retval <= 0)
return (retval);
--
1.6.0.2
Impact: cleanup
idetape_chrdev_read/write() functions are unnecessarily complex when
everything can be handled in a single loop. Collapse
idetape_add_chrdev_read/write_request() into the rw functions and
simplify the implementation.
Signed-off-by: Tejun Heo <[email protected]>
---
drivers/ide/ide-tape.c | 149 ++++++++++++++++--------------------------------
1 files changed, 50 insertions(+), 99 deletions(-)
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 5fc3282..95e9131 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -953,14 +953,6 @@ static void idetape_create_space_cmd(struct ide_atapi_pc *pc, int count, u8 cmd)
pc->flags |= PC_FLAG_WAIT_FOR_DSC;
}
-/* Queue up a character device originated write request. */
-static int idetape_add_chrdev_write_request(ide_drive_t *drive, int size)
-{
- debug_log(DBG_CHRDEV, "Enter %s\n", __func__);
-
- return idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, size);
-}
-
static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
{
idetape_tape_t *tape = drive->driver_data;
@@ -974,7 +966,7 @@ static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
size_t aligned = roundup(tape->valid, tape->blk_size);
memset(tape->cur + tape->valid, 0, aligned - tape->valid);
- idetape_add_chrdev_write_request(drive, aligned);
+ idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, aligned);
kfree(tape->buf);
tape->buf = NULL;
}
@@ -1027,20 +1019,6 @@ static int idetape_init_rw(ide_drive_t *drive, int dir)
return 0;
}
-/* called from idetape_chrdev_read() to service a chrdev read request. */
-static int idetape_add_chrdev_read_request(ide_drive_t *drive, int size)
-{
- debug_log(DBG_PROCS, "Enter %s, %d bytes\n", __func__, size);
-
- /* If we are at a filemark, return a read length of 0 */
- if (test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags))
- return 0;
-
- idetape_init_rw(drive, IDETAPE_DIR_READ);
-
- return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, size);
-}
-
static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
{
idetape_tape_t *tape = drive->driver_data;
@@ -1180,8 +1158,9 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
{
struct ide_tape_obj *tape = file->private_data;
ide_drive_t *drive = tape->drive;
- ssize_t bytes_read, temp, actually_read = 0, rc;
+ size_t done = 0;
ssize_t ret = 0;
+ int rc;
debug_log(DBG_CHRDEV, "Enter %s, count %Zd\n", __func__, count);
@@ -1191,52 +1170,43 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
(count % tape->blk_size) == 0)
tape->user_bs_factor = count / tape->blk_size;
}
+
rc = idetape_init_rw(drive, IDETAPE_DIR_READ);
if (rc < 0)
return rc;
- if (count == 0)
- return (0);
- if (tape->valid) {
- actually_read = min_t(unsigned int, tape->valid, count);
- if (copy_to_user(buf, tape->cur, actually_read))
- ret = -EFAULT;
- buf += actually_read;
- count -= actually_read;
- tape->cur += actually_read;
- tape->valid -= actually_read;
- }
- while (count >= tape->buffer_size) {
- bytes_read = idetape_add_chrdev_read_request(drive,
- tape->buffer_size);
- if (bytes_read <= 0)
- goto finish;
- if (copy_to_user(buf, tape->cur, bytes_read))
- ret = -EFAULT;
- buf += bytes_read;
- count -= bytes_read;
- actually_read += bytes_read;
- }
- if (count) {
- bytes_read = idetape_add_chrdev_read_request(drive,
- tape->buffer_size);
- if (bytes_read <= 0)
- goto finish;
- temp = min((unsigned long)count, (unsigned long)bytes_read);
- if (copy_to_user(buf, tape->cur, temp))
+
+ while (done < count) {
+ size_t todo;
+
+ /* 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))
+ break;
+ /* read */
+ if (idetape_queue_rw_tail(drive, REQ_IDETAPE_READ,
+ tape->buffer_size) <= 0)
+ break;
+ }
+
+ /* copy out */
+ todo = min_t(size_t, count - done, tape->valid);
+ if (copy_to_user(buf + done, tape->cur, todo))
ret = -EFAULT;
- actually_read += temp;
- tape->cur += temp;
- tape->valid -= temp;
+
+ tape->cur += todo;
+ tape->valid -= todo;
+ done += todo;
}
-finish:
- if (!actually_read && test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags)) {
+
+ if (!done && test_bit(IDE_AFLAG_FILEMARK, &drive->atapi_flags)) {
debug_log(DBG_SENSE, "%s: spacing over filemark\n", tape->name);
idetape_space_over_filemarks(drive, MTFSF, 1);
return 0;
}
- return ret ? ret : actually_read;
+ return ret ? ret : done;
}
static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
@@ -1244,7 +1214,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
{
struct ide_tape_obj *tape = file->private_data;
ide_drive_t *drive = tape->drive;
- ssize_t actually_written = 0;
+ size_t done = 0;
ssize_t ret = 0;
int rc;
@@ -1258,47 +1228,28 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
rc = idetape_init_rw(drive, IDETAPE_DIR_WRITE);
if (rc < 0)
return rc;
- if (count == 0)
- return (0);
- if (tape->valid < tape->buffer_size) {
- actually_written = min_t(unsigned int,
- tape->buffer_size - tape->valid,
- count);
- if (copy_from_user(tape->cur, buf, actually_written))
- ret = -EFAULT;
- buf += actually_written;
- count -= actually_written;
- tape->cur += actually_written;
- tape->valid += actually_written;
-
- if (tape->valid == tape->buffer_size) {
- ssize_t retval;
- retval = idetape_add_chrdev_write_request(drive,
- tape->buffer_size);
- if (retval <= 0)
- return (retval);
- }
- }
- while (count >= tape->buffer_size) {
- ssize_t retval;
- if (copy_from_user(tape->cur, buf, tape->buffer_size))
- ret = -EFAULT;
- buf += tape->buffer_size;
- count -= tape->buffer_size;
- retval = idetape_add_chrdev_write_request(drive,
- tape->buffer_size);
- actually_written += tape->buffer_size;
- if (retval <= 0)
- return (retval);
- }
- if (count) {
- actually_written += count;
- if (copy_from_user(tape->cur, buf, count))
+
+ while (done < count) {
+ size_t todo;
+
+ /* flush if staging buffer is full */
+ if (tape->valid == tape->buffer_size &&
+ idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE,
+ tape->buffer_size) <= 0)
+ return rc;
+
+ /* copy in */
+ todo = min_t(size_t, count - done,
+ tape->buffer_size - tape->valid);
+ if (copy_from_user(tape->cur, buf + done, todo))
ret = -EFAULT;
- tape->cur += count;
- tape->valid += count;
+
+ tape->cur += todo;
+ tape->valid += todo;
+ done += todo;
}
- return ret ? ret : actually_written;
+
+ return ret ? ret : done;
}
static int idetape_write_filemark(ide_drive_t *drive)
--
1.6.0.2
Impact: remove fields code paths which are no longer necessary
Now that ide-tape uses standard mechanisms to transfer data, special
case handling for bh handling can be dropped from ide-atapi. Drop the
followings.
* pc->cur_pos, b_count, bh and b_data
* drive->pc_update_buffers() and pc_io_buffers().
Signed-off-by: Tejun Heo <[email protected]>
---
drivers/ide/ide-atapi.c | 15 +++------------
drivers/ide/ide-tape.c | 1 -
include/linux/ide.h | 12 ------------
3 files changed, 3 insertions(+), 25 deletions(-)
diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 53e811d..bbb8da1 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -317,9 +317,6 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
pc->flags |= PC_FLAG_DMA_ERROR;
} else {
pc->xferred = pc->req_xfer;
- if (drive->pc_update_buffers)
- drive->pc_update_buffers(drive, pc);
-
if (drive->media == ide_floppy)
ide_complete_rq(drive, 0, blk_rq_bytes(rq));
}
@@ -420,16 +417,11 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
return ide_do_reset(drive);
}
- if (drive->media == ide_tape && pc->bh)
- done = drive->pc_io_buffers(drive, pc, bcount, write);
- else {
- done = min_t(unsigned int, bcount, cmd->nleft);
- ide_pio_bytes(drive, cmd, write, done);
- }
+ done = min_t(unsigned int, bcount, cmd->nleft);
+ ide_pio_bytes(drive, cmd, write, done);
- /* Update the current position */
+ /* Update transferred byte count */
pc->xferred += done;
- pc->cur_pos += done;
bcount -= done;
@@ -610,7 +602,6 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
/* We haven't transferred any data yet */
pc->xferred = 0;
- pc->cur_pos = pc->buf;
tf_flags = IDE_TFLAG_OUT_DEVICE;
bcount = ((drive->media == ide_tape) ?
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 95e9131..557f073 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -593,7 +593,6 @@ static void ide_tape_create_rw_cmd(idetape_tape_t *tape,
ide_init_pc(pc);
put_unaligned(cpu_to_be32(length), (unsigned int *) &pc->c[1]);
pc->c[1] = 1;
- pc->bh = NULL;
pc->buf = NULL;
pc->buf_size = length * tape->blk_size;
pc->req_xfer = pc->buf_size;
diff --git a/include/linux/ide.h b/include/linux/ide.h
index 8d3bb27..996adae 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -397,11 +397,7 @@ struct ide_atapi_pc {
/* data buffer */
u8 *buf;
- /* current buffer position */
- u8 *cur_pos;
int buf_size;
- /* missing/available data on the current buffer */
- int b_count;
/* the corresponding request */
struct request *rq;
@@ -414,10 +410,6 @@ struct ide_atapi_pc {
*/
u8 pc_buf[IDE_PC_BUFFER_SIZE];
- /* idetape only */
- struct idetape_bh *bh;
- char *b_data;
-
unsigned long timeout;
};
@@ -630,10 +622,6 @@ struct ide_drive_s {
/* callback for packet commands */
int (*pc_callback)(struct ide_drive_s *, int);
- void (*pc_update_buffers)(struct ide_drive_s *, struct ide_atapi_pc *);
- int (*pc_io_buffers)(struct ide_drive_s *, struct ide_atapi_pc *,
- unsigned int, int);
-
ide_startstop_t (*irq_handler)(struct ide_drive_s *);
unsigned long atapi_flags;
--
1.6.0.2
Impact: remove code path which is no longer necessary
All IDE data transfers now use rq->bio. Simplify ide_map_sg()
accordingly.
Signed-off-by: Tejun Heo <[email protected]>
Cc: Jens Axboe <[email protected]>
---
drivers/ide/ide-io.c | 6 +-----
1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 9ad9bc1..6746171 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -244,11 +244,7 @@ void ide_map_sg(ide_drive_t *drive, struct ide_cmd *cmd)
struct scatterlist *sg = hwif->sg_table;
struct request *rq = cmd->rq;
- if (!rq->bio) {
- sg_init_one(sg, rq->data, rq->data_len);
- cmd->sg_nents = 1;
- } else
- cmd->sg_nents = blk_rq_map_sg(drive->queue, rq, sg);
+ cmd->sg_nents = blk_rq_map_sg(drive->queue, rq, sg);
}
EXPORT_SYMBOL_GPL(ide_map_sg);
--
1.6.0.2
Impact: use standard way to carry data area in request
ide-tape uses rq in an interesting way. For r/w requests, rq->special
is used to carry a private buffer management structure idetape_bh and
rq->nr_sectors and current_nr_sectors are initialized to the number of
idetape blocks which isn't necessary 512 bytes. Also,
rq->current_nr_sectors is used to report back the residual count in
units of idetape blocks.
This peculiarity taxes both block layer and ide. ide-atapi has
different paths and hooks to accomodate it and what a rq means becomes
quite confusing and making changes at the block layer becomes quite
difficult and error-prone.
This patch makes ide-tape use bio instead. With the previous patch,
ide-tape currently is using single contiguos buffer so replacing it is
easy. All that's necessary is doing blk_rq_map_kern() on issue, map
bio to sg in idetape_do_request() and use rq->data_len for residual
count instead of rq->current_nr_sectors.
This change also nicely removes the FIXME in ide_pc_intr() where
ide-tape rqs need to be completed using ide_rq_bytes() instead of
blk_rq_bytes() (although this didn't really matter as the request
didn't have bio).
Note that this patch only changes how the data areas are carried in
requests. Data transfer is still done using idetape_bh.
Signed-off-by: Tejun Heo <[email protected]>
Cc: Jens Axboe <[email protected]>
---
Sorry, wrote description to the wrong file. git tree updated as well.
drivers/ide/ide-atapi.c | 5 +----
drivers/ide/ide-tape.c | 39 ++++++++++++++++++++++-----------------
2 files changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 2b456c4..53e811d 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -372,10 +372,7 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
* ->pc_callback() might change rq->data_len for
* residual count, cache total length.
*/
- if (drive->media == ide_tape)
- done = ide_rq_bytes(rq); /* FIXME */
- else
- done = blk_rq_bytes(rq);
+ done = blk_rq_bytes(rq);
/* Command finished - Call the callback function */
uptodate = drive->pc_callback(drive, dsc);
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 3a7bd98..bd0e839 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -458,7 +458,7 @@ static int ide_tape_callback(ide_drive_t *drive, int dsc)
}
tape->first_frame += blocks;
- rq->current_nr_sectors -= blocks;
+ rq->data_len -= blocks * tape->blk_size;
if (pc->error) {
uptodate = 0;
@@ -799,10 +799,6 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
pc = (struct ide_atapi_pc *)rq->special;
rq->cmd[13] &= ~(REQ_IDETAPE_PC1);
rq->cmd[13] |= REQ_IDETAPE_PC2;
- if (pc->req_xfer) {
- ide_init_sg_cmd(&cmd, pc->req_xfer);
- ide_map_sg(drive, &cmd);
- }
goto out;
}
if (rq->cmd[13] & REQ_IDETAPE_PC2) {
@@ -812,6 +808,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
BUG();
out:
+ if (pc->req_xfer) {
+ ide_init_sg_cmd(&cmd, pc->req_xfer);
+ ide_map_sg(drive, &cmd);
+ }
if (rq_data_dir(rq))
cmd.tf_flags |= IDE_TFLAG_WRITE;
@@ -1061,32 +1061,37 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
struct idetape_bh *bh)
{
idetape_tape_t *tape = drive->driver_data;
+ size_t size = blocks * tape->blk_size;
struct request *rq;
- int ret, errors;
+ int ret;
debug_log(DBG_SENSE, "%s: cmd=%d\n", __func__, cmd);
+ BUG_ON(cmd != REQ_IDETAPE_READ && cmd != REQ_IDETAPE_WRITE);
rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
rq->cmd_type = REQ_TYPE_SPECIAL;
rq->cmd[13] = cmd;
rq->rq_disk = tape->disk;
- rq->special = (void *)bh;
rq->sector = tape->first_frame;
- rq->nr_sectors = blocks;
- rq->current_nr_sectors = blocks;
- blk_execute_rq(drive->queue, tape->disk, rq, 0);
- errors = rq->errors;
- ret = tape->blk_size * (blocks - rq->current_nr_sectors);
- blk_put_request(rq);
+ if (size) {
+ ret = blk_rq_map_kern(drive->queue, rq, bh->b_data, size,
+ __GFP_WAIT);
+ if (ret)
+ goto out_put;
+ }
- if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
- return 0;
+ blk_execute_rq(drive->queue, tape->disk, rq, 0);
+
+ size -= rq->data_len;
+ ret = size;
+ if (rq->errors == IDE_DRV_ERROR_GENERAL)
+ ret = -EIO;
if (tape->merge_bh)
idetape_init_merge_buffer(tape);
- if (errors == IDE_DRV_ERROR_GENERAL)
- return -EIO;
+out_put:
+ blk_put_request(rq);
return ret;
}
--
1.6.0.2
On Wed, Mar 25, 2009 at 3:17 PM, Tejun Heo <[email protected]> wrote:
> Impact: use standard way to transfer data
>
> Now that data area is represented with bio, there is no need to use
> custom data transfer methods. ?Drop idetape_io_buffers() and
> idetape_update_buffers(). ?pc->bh is set to null to tell ide-atapi to
> use standard data transfer mechanism and idetape_bh byte counts are
> updated by the issuer on completion using the residual count.
>
> Signed-off-by: Tejun Heo <[email protected]>
> ---
> ?drivers/ide/ide-tape.c | ? 84 +++--------------------------------------------
> ?1 files changed, 6 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index bd0e839..dd37fd7 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -292,65 +292,6 @@ static struct ide_tape_obj *ide_tape_chrdev_get(unsigned int i)
> ? ? ? ?return tape;
> ?}
>
> -static int idetape_input_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned int bcount)
> -{
> - ? ? ? struct idetape_bh *bh = pc->bh;
> - ? ? ? int count;
> -
> - ? ? ? if (bcount && bh) {
> - ? ? ? ? ? ? ? count = min(
> - ? ? ? ? ? ? ? ? ? ? ? (unsigned int)(bh->b_size - atomic_read(&bh->b_count)),
> - ? ? ? ? ? ? ? ? ? ? ? bcount);
> - ? ? ? ? ? ? ? drive->hwif->tp_ops->input_data(drive, NULL, bh->b_data +
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? atomic_read(&bh->b_count), count);
> - ? ? ? ? ? ? ? bcount -= count;
> - ? ? ? ? ? ? ? atomic_add(count, &bh->b_count);
> - ? ? ? ? ? ? ? if (atomic_read(&bh->b_count) == bh->b_size)
> - ? ? ? ? ? ? ? ? ? ? ? pc->bh = NULL;
> - ? ? ? }
> -
> - ? ? ? return bcount;
> -}
> -
> -static int idetape_output_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned int bcount)
> -{
> - ? ? ? struct idetape_bh *bh = pc->bh;
> - ? ? ? int count;
> -
> - ? ? ? if (bcount && bh) {
> - ? ? ? ? ? ? ? count = min((unsigned int)pc->b_count, (unsigned int)bcount);
> - ? ? ? ? ? ? ? drive->hwif->tp_ops->output_data(drive, NULL, pc->b_data, count);
> - ? ? ? ? ? ? ? bcount -= count;
> - ? ? ? ? ? ? ? pc->b_data += count;
> - ? ? ? ? ? ? ? pc->b_count -= count;
> - ? ? ? ? ? ? ? if (!pc->b_count)
> - ? ? ? ? ? ? ? ? ? ? ? pc->bh = NULL;
> - ? ? ? }
> -
> - ? ? ? return bcount;
> -}
> -
> -static void idetape_update_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc)
> -{
> - ? ? ? struct idetape_bh *bh = pc->bh;
> - ? ? ? unsigned int bcount = pc->xferred;
> -
> - ? ? ? if (pc->flags & PC_FLAG_WRITING)
> - ? ? ? ? ? ? ? return;
> - ? ? ? if (bcount) {
> - ? ? ? ? ? ? ? if (bh == NULL || bcount > bh->b_size) {
> - ? ? ? ? ? ? ? ? ? ? ? printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__);
> - ? ? ? ? ? ? ? ? ? ? ? return;
> - ? ? ? ? ? ? ? }
> - ? ? ? ? ? ? ? atomic_set(&bh->b_count, bcount);
> - ? ? ? ? ? ? ? if (atomic_read(&bh->b_count) == bh->b_size)
> - ? ? ? ? ? ? ? ? ? ? ? pc->bh = NULL;
> - ? ? ? }
> -}
> -
> ?/*
> ?* called on each failed packet command retry to analyze the request sense. We
> ?* currently do not utilize this information.
> @@ -368,12 +309,10 @@ static void idetape_analyze_error(ide_drive_t *drive, u8 *sense)
> ? ? ? ? ? ? ? ? pc->c[0], tape->sense_key, tape->asc, tape->ascq);
>
> ? ? ? ?/* Correct pc->xferred by asking the tape. ? ? ? */
> - ? ? ? if (pc->flags & PC_FLAG_DMA_ERROR) {
> + ? ? ? if (pc->flags & PC_FLAG_DMA_ERROR)
> ? ? ? ? ? ? ? ?pc->xferred = pc->req_xfer -
> ? ? ? ? ? ? ? ? ? ? ? ?tape->blk_size *
> ? ? ? ? ? ? ? ? ? ? ? ?get_unaligned_be32(&sense[3]);
> - ? ? ? ? ? ? ? idetape_update_buffers(drive, pc);
> - ? ? ? }
>
> ? ? ? ?/*
> ? ? ? ? * If error was the result of a zero-length read or write command,
> @@ -520,19 +459,6 @@ static void ide_tape_handle_dsc(ide_drive_t *drive)
> ? ? ? ?idetape_postpone_request(drive);
> ?}
>
> -static int ide_tape_io_buffers(ide_drive_t *drive, struct ide_atapi_pc *pc,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned int bcount, int write)
> -{
> - ? ? ? unsigned int bleft;
> -
> - ? ? ? if (write)
> - ? ? ? ? ? ? ? bleft = idetape_output_buffers(drive, pc, bcount);
> - ? ? ? else
> - ? ? ? ? ? ? ? bleft = idetape_input_buffers(drive, pc, bcount);
> -
> - ? ? ? return bcount - bleft;
> -}
> -
> ?/*
> ?* Packet Command Interface
> ?*
> @@ -685,7 +611,7 @@ static void ide_tape_create_rw_cmd(idetape_tape_t *tape,
> ? ? ? ?ide_init_pc(pc);
> ? ? ? ?put_unaligned(cpu_to_be32(length), (unsigned int *) &pc->c[1]);
> ? ? ? ?pc->c[1] = 1;
> - ? ? ? pc->bh = bh;
> + ? ? ? pc->bh = NULL;
> ? ? ? ?pc->buf = NULL;
> ? ? ? ?pc->buf_size = length * tape->blk_size;
> ? ? ? ?pc->req_xfer = pc->buf_size;
> @@ -1083,7 +1009,11 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
>
> ? ? ? ?blk_execute_rq(drive->queue, tape->disk, rq, 0);
>
> + ? ? ? /* calculate the number of transferred bytes and update bh */
> ? ? ? ?size -= rq->data_len;
> + ? ? ? if (cmd == REQ_IDETAPE_READ)
> + ? ? ? ? ? ? ? atomic_add(size, &bh->b_count);
> +
> ? ? ? ?ret = size;
> ? ? ? ?if (rq->errors == IDE_DRV_ERROR_GENERAL)
> ? ? ? ? ? ? ? ?ret = -EIO;
> @@ -2037,8 +1967,6 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)
> ? ? ? ?u16 *ctl = (u16 *)&tape->caps[12];
>
> ? ? ? ?drive->pc_callback ? ? ? = ide_tape_callback;
> - ? ? ? drive->pc_update_buffers = idetape_update_buffers;
> - ? ? ? drive->pc_io_buffers ? ? = ide_tape_io_buffers;
You might just as well remove those two ide_drive_t members since
they're used only by ide-tape.
--
Regards/Gruss,
Boris
>> ? ? ? ?drive->pc_callback ? ? ? = ide_tape_callback;
>> - ? ? ? drive->pc_update_buffers = idetape_update_buffers;
>> - ? ? ? drive->pc_io_buffers ? ? = ide_tape_io_buffers;
>
> You might just as well remove those two ide_drive_t members since
> they're used only by ide-tape.
Ah, nevermind. You've done it later in patch 9/10.
--
Regards/Gruss,
Boris
On Wed, Mar 25, 2009 at 7:17 AM, Tejun Heo <[email protected]> wrote:> Impact: simpler buffer allocation and handling, fix DMA transfers...> + atomic_set(&bh->b_count, bcount);> if (atomic_read(&bh->b_count) == bh->b_size)...
I'm failing to see why bh->b_count is an atomic_t.I always assumed tapes were exclusive access devicesand would be serialized at a higher level.
> /*> - * The function below uses __get_free_pages to allocate a data buffer of size> - * tape->buffer_size (or a bit more). We attempt to combine sequential pages as> - * much as possible.> - *> - * It returns a pointer to the newly allocated buffer, or NULL in case of> - * failure.> + * It returns a pointer to the newly allocated buffer, or NULL in case> + * of failure.> */> static struct idetape_bh *ide_tape_kmalloc_buffer(idetape_tape_t *tape,> - int full, int clear)> -{...> -abort:> - ide_tape_kfree_buffer(tape);> - return NULL;> + bh->b_size = tape->buffer_size;> + atomic_set(&bh->b_count, full ? bh->b_size : 0);
No one else could possibly be referencing bh->count at thispoint...I like that it's consistent though.
The use of atomic won't hurt correctness and this patch looks fine to me.Please add "Reviewed-by: Grant Grundler <[email protected]>"
thanks,grant
...> @@ -977,30 +872,20 @@ static int idetape_copy_stage_to_user(idetape_tape_t *tape, char __user *buf,> int n)> {> struct idetape_bh *bh = tape->bh;> - int count;> int ret = 0;>> - while (n) {> - if (bh == NULL) {> + if (n) {> + if (bh == NULL || n > tape->b_count) {> printk(KERN_ERR "ide-tape: bh == NULL in %s\n",> __func__);> return 1;> }> - count = min(tape->b_count, n);> - if (copy_to_user(buf, tape->b_data, count))> + if (copy_to_user(buf, tape->b_data, n))> ret = 1;> - n -= count;> - tape->b_data += count;> - tape->b_count -= count;> - buf += count;> - if (!tape->b_count) {> - bh = bh->b_reqnext;> - tape->bh = bh;> - if (bh) {> - tape->b_data = bh->b_data;> - tape->b_count = atomic_read(&bh->b_count);> - }> - }> + tape->b_data += n;> + tape->b_count -= n;> + if (!tape->b_count)> + tape->bh = NULL;> }> return ret;> }> @@ -1252,7 +1137,7 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)> static void ide_tape_flush_merge_buffer(ide_drive_t *drive)> {> idetape_tape_t *tape = drive->driver_data;> - int blocks, min;> + int blocks;> struct idetape_bh *bh;>> if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {> @@ -1267,31 +1152,16 @@ static void ide_tape_flush_merge_buffer(ide_drive_t *drive)> if (tape->merge_bh_size) {> blocks = tape->merge_bh_size / tape->blk_size;> if (tape->merge_bh_size % tape->blk_size) {> - unsigned int i;> -> + unsigned int i = tape->blk_size -> + tape->merge_bh_size % tape->blk_size;> blocks++;> - i = tape->blk_size - tape->merge_bh_size %> - tape->blk_size;> - bh = tape->bh->b_reqnext;> - while (bh) {> - atomic_set(&bh->b_count, 0);> - bh = bh->b_reqnext;> - }> bh = tape->bh;> - while (i) {> - if (bh == NULL) {> - printk(KERN_INFO "ide-tape: bug,"> - " bh NULL\n");> - break;> - }> - min = min(i, (unsigned int)(bh->b_size -> - atomic_read(&bh->b_count)));> + if (bh) {> memset(bh->b_data + atomic_read(&bh->b_count),> - 0, min);> - atomic_add(min, &bh->b_count);> - i -= min;> - bh = bh->b_reqnext;> - }> + 0, i);> + atomic_add(i, &bh->b_count);> + } else> + printk(KERN_INFO "ide-tape: bug, bh NULL\n");> }> (void) idetape_add_chrdev_write_request(drive, blocks);> tape->merge_bh_size = 0;> @@ -1319,7 +1189,7 @@ static int idetape_init_read(ide_drive_t *drive)> " 0 now\n");> tape->merge_bh_size = 0;> }> - tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0, 0);> + tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);> if (!tape->merge_bh)> return -ENOMEM;> tape->chrdev_dir = IDETAPE_DIR_READ;> @@ -1366,23 +1236,18 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)> static void idetape_pad_zeros(ide_drive_t *drive, int bcount)> {> idetape_tape_t *tape = drive->driver_data;> - struct idetape_bh *bh;> + struct idetape_bh *bh = tape->merge_bh;> int blocks;>> while (bcount) {> unsigned int count;>> - bh = tape->merge_bh;> count = min(tape->buffer_size, bcount);> bcount -= count;> blocks = count / tape->blk_size;> - while (count) {> - atomic_set(&bh->b_count,> - min(count, (unsigned int)bh->b_size));> - memset(bh->b_data, 0, atomic_read(&bh->b_count));> - count -= atomic_read(&bh->b_count);> - bh = bh->b_reqnext;> - }> + atomic_set(&bh->b_count, count);> + memset(bh->b_data, 0, atomic_read(&bh->b_count));> +> idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, blocks,> tape->merge_bh);> }> @@ -1594,7 +1459,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,> "should be 0 now\n");> tape->merge_bh_size = 0;> }> - tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0, 0);> + tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);> if (!tape->merge_bh)> return -ENOMEM;> tape->chrdev_dir = IDETAPE_DIR_WRITE;> @@ -1968,7 +1833,7 @@ static void idetape_write_release(ide_drive_t *drive, unsigned int minor)> idetape_tape_t *tape = drive->driver_data;>> ide_tape_flush_merge_buffer(drive);> - tape->merge_bh = ide_tape_kmalloc_buffer(tape, 1, 0);> + tape->merge_bh = ide_tape_kmalloc_buffer(tape, 1);> if (tape->merge_bh != NULL) {> idetape_pad_zeros(drive, tape->blk_size *> (tape->user_bs_factor - 1));> @@ -2199,11 +2064,6 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)> tape->buffer_size = *ctl * tape->blk_size;> }> buffer_size = tape->buffer_size;> - tape->pages_per_buffer = buffer_size / PAGE_SIZE;> - if (buffer_size % PAGE_SIZE) {> - tape->pages_per_buffer++;> - tape->excess_bh_size = PAGE_SIZE - buffer_size % PAGE_SIZE;> - }>> /* select the "best" DSC read/write polling freq */> speed = max(*(u16 *)&tape->caps[14], *(u16 *)&tape->caps[8]);> --> 1.6.0.2>> --> To unsubscribe from this list: send the line "unsubscribe linux-ide" in> the body of a message to [email protected]> More majordomo info at http://vger.kernel.org/majordomo-info.html>????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Hello, Grant.
Grant Grundler wrote:
> On Wed, Mar 25, 2009 at 7:17 AM, Tejun Heo <[email protected]> wrote:
>> Impact: simpler buffer allocation and handling, fix DMA transfers
> ...
>> + atomic_set(&bh->b_count, bcount);
>> if (atomic_read(&bh->b_count) == bh->b_size)
> ...
>
> I'm failing to see why bh->b_count is an atomic_t.
> I always assumed tapes were exclusive access devices
> and would be serialized at a higher level.
Beats me. I don't know. The code is generally pretty over-engineered
but, well, it's an ancient piece of code with (probably too) rich
history.
>> - ide_tape_kfree_buffer(tape);
>> - return NULL;
>> + bh->b_size = tape->buffer_size;
>> + atomic_set(&bh->b_count, full ? bh->b_size : 0);
>
> No one else could possibly be referencing bh->count at this
> point...I like that it's consistent though.
Yeah, this patch is just one of logical steps to remove bh, so it
doesn't make any other changes than described. The whole bh stuff
will be removed later in the series.
> The use of atomic won't hurt correctness and this patch looks fine to me.
> Please add "Reviewed-by: Grant Grundler <[email protected]>"
Thanks.
--
tejun
Hi,
On Wed, Mar 25, 2009 at 5:13 PM, Tejun Heo <[email protected]> wrote:
> Hello, Grant.
>
> Grant Grundler wrote:
>> On Wed, Mar 25, 2009 at 7:17 AM, Tejun Heo <[email protected]> wrote:
>>> Impact: simpler buffer allocation and handling, fix DMA transfers
>> ...
>>> + ? ? ? ? ? ? ? atomic_set(&bh->b_count, bcount);
>>> ? ? ? ? ? ? ? ?if (atomic_read(&bh->b_count) == bh->b_size)
>> ...
>>
>> I'm failing to see why bh->b_count is an atomic_t.
>> I always assumed tapes were exclusive access devices
>> and would be serialized at a higher level.
>
> Beats me. ?I don't know. ?The code is generally pretty over-engineered
> but, well, it's an ancient piece of code with (probably too) rich
> history.
Yep, actually the driver was even bigger and it implemented a bunch of fun stuff
like command pipelining and those atomics had to protect the pipeline from
simultaneous ->open()s through its chrdev interface, IIRC.
--
Regards/Gruss,
Boris
On Wed, Mar 25, 2009 at 11:17:43PM +0900, Tejun Heo wrote:
> Hi, Bartlomiej, Jens.
>
> This RFC patchset cleans up ide-tape. Note that the patchset
> currently is only compile tested, so it's likely to be broken, so the
> RFC status. Albert Lee is sending me some IDE tape drives, so I'll
> test these patches as soon as I receive them and repost with whatever
> necessary fixes.
By the way,
we may have another potential tester for ide-tape if we ask him nicely
enough :)
See http://bugzilla.kernel.org/show_bug.cgi?id=12874.
--
Regards/Gruss,
Boris.