lkml.org 
[lkml]   [2005]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2005-03-22 14:10    [W:0.979 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site