Messages in this thread | | | Date | Mon, 07 Feb 2005 14:13:22 +0900 | From | Tejun Heo <> | Subject | Re: [PATCH 2.6.11-rc2 04/09] ide: convert REQ_DRIVE_TASK to REQ_DRIVE_TASKFILE |
| |
Bartlomiej Zolnierkiewicz wrote: > I put some more thought into this change... details below... > > On Sat, 5 Feb 2005 11:15:56 +0900 (KST), Tejun Heo <tj@home-tj.org> wrote: > > >>@@ -705,24 +705,17 @@ static int idedisk_issue_flush(request_q >> { >> ide_drive_t *drive = q->queuedata; >> struct request *rq; >>+ ide_task_t args; >> int ret; > > > ide_task_t task please >
Okay.
> >>@@ -730,8 +723,9 @@ static int idedisk_issue_flush(request_q >> * if we failed and caller wants error offset, get it >> */ >> if (ret && error_sector) >>- *error_sector = ide_get_error_location(drive, rq->cmd); >>+ *error_sector = ide_get_error_location(drive, &args); >> >>+ rq->special = NULL; /* just in case */ > > > In what case? >
As the request is allocated and freed by the generic block layer, I was kinda worrying about cases where the request outlives blk_put_request() and somehow somebody accesses ->special. Probably unnecessary but dangling pointers pointing stack area really scares me.
> >>@@ -55,22 +55,19 @@ >> #include <asm/io.h> >> #include <asm/bitops.h> >> >>-static void ide_fill_flush_cmd(ide_drive_t *drive, struct request *rq) >>+void ide_init_flush_task(ide_drive_t *drive, ide_task_t *args) > > > ide_task_t *task > > >>@@ -80,7 +77,9 @@ static void ide_fill_flush_cmd(ide_drive >> static struct request *ide_queue_flush_cmd(ide_drive_t *drive, >> struct request *rq, int post) >> { >>- struct request *flush_rq = &HWGROUP(drive)->wrq; >>+ ide_hwgroup_t *hwgroup = drive->hwif->hwgroup; >>+ struct request *flush_rq = &hwgroup->flush_rq; >>+ ide_task_t *args = &hwgroup->flush_args; > > > ide_task_t *task > > >>@@ -221,41 +223,37 @@ static void ide_complete_pm_request (ide >> /* >> * FIXME: probably move this somewhere else, name is bad too :) >> */ >>-u64 ide_get_error_location(ide_drive_t *drive, char *args) >>+u64 ide_get_error_location(ide_drive_t *drive, ide_task_t *args) > > > ide_task_t *task > > >> { >> u32 high, low; >> u8 hcyl, lcyl, sect; >>- u64 sector; >> >>- high = 0; >>- hcyl = args[5]; >>- lcyl = args[4]; >>- sect = args[3]; >>- >>- if (ide_id_has_flush_cache_ext(drive->id)) { >>- low = (hcyl << 16) | (lcyl << 8) | sect; >>- HWIF(drive)->OUTB(drive->ctl|0x80, IDE_CONTROL_REG); >>- high = ide_read_24(drive); >>- } else { >>- u8 cur = HWIF(drive)->INB(IDE_SELECT_REG); >>- if (cur & 0x40) >>- low = (hcyl << 16) | (lcyl << 8) | sect; >>- else { >>- low = hcyl * drive->head * drive->sect; >>- low += lcyl * drive->sect; >>- low += sect - 1; >>- } >>- } >>+ if (ide_id_has_flush_cache_ext(drive->id) && >>+ (drive->capacity64 >= (1UL << 28))) > > > Please just use if (drive->addressing), it is simpler and still correct. > Since we are now using ide_task_t 'high' will be 0 when > ide_id_has_flush_cache() == 0 and drive->addressing == 1 > (such combination is unlikely but...). Also thanks to this change > ide_get_error_location() becomes a really *generic* helper and > can be later used by other code. >
Sure.
> >>@@ -1201,9 +1224,14 @@ extern ide_startstop_t ide_do_reset (ide >> extern void ide_init_drive_cmd (struct request *rq); >> >> /* >>+ * This function initializes @task to WIN_FLUSH_CACHE[_EXT] command. >>+ */ >>+void ide_init_flush_task(ide_drive_t *drive, ide_task_t *args); > > > comment is wrong and not needed, > > void ide_init_flush_task(ide_drive_t *, ide_task_t *); > > should be enough >
Okay.
> > There is one problem left with this change - FLUSH_CACHE_{EXT} > command handling becomes slower for drive's supporting LBA48 > (also next patches make ide_{task,cmd}_ioctl() slower). Why is so? > See do_rw_taskfile(), HOB registers are written/read unconditionally > if (drive->addressing == 1). This can be fixed by i.e. adding > 'unsigned long flags' to ide_task_t and IDE_TASK_LBA48 flag. > BTW this fix is needed also to implement LBA48 optimization for > read/write requests (not writing HOB registers when not needed). > > IMHO there are some things worth mentioning in the patch description, > do_rw_taskfile() vs execute_drive_cmd()+ide_cmd() details: > some registers are written now in different order and timeout is bumped > (these changes shouldn't make any harm but I'm paranoid :). >
Yeah, sure.
Thanks.
-- tejun
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |