Messages in this thread Patch in this message | | | Date | Sun, 6 Feb 2005 19:34:59 +0100 | From | Bartlomiej Zolnierkiewicz <> | Subject | Re: [PATCH 2.6.11-rc2 04/09] ide: convert REQ_DRIVE_TASK to REQ_DRIVE_TASKFILE |
| |
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
> @@ -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?
> @@ -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.
> @@ -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
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 :).
Thanks, Bartlomiej - 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/
| |