linux-kernel - Re: [PATCH 13/18] ide: use ->tf_load in SELECT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200902162251.49271.bzolnier@gmail.com>
Date:	Mon, 16 Feb 2009 22:51:49 +0100
From:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
To:	Sergei Shtylyov <sshtylyov@...mvista.com>
Cc:	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 13/18] ide: use ->tf_load in SELECT_DRIVE()


On Monday 16 February 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >>> There should be no functional changes caused by this patch.
> >>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
> >>> Index: b/drivers/ide/ide-iops.c
> >>> ===================================================================
> >>> --- a/drivers/ide/ide-iops.c
> >>> +++ b/drivers/ide/ide-iops.c
> >>> @@ -88,11 +88,15 @@ void SELECT_DRIVE (ide_drive_t *drive)
> >>>  {
> >>>      ide_hwif_t *hwif = drive->hwif;
> >>>      const struct ide_port_ops *port_ops = hwif->port_ops;
> >>> +    ide_task_t task;
> >>>  
> >>>      if (port_ops && port_ops->selectproc)
> >>>          port_ops->selectproc(drive);
> >>>  
> >>> -    hwif->OUTB(drive->select.all, hwif->io_ports.device_addr);
> >>> +    memset(&task, 0, sizeof(task));
> >>> +    task.tf_flags = IDE_TFLAG_OUT_DEVICE;
> >>> +
> >>> +    drive->hwif->tf_load(drive, &task);
> >>
> >>    This actually doesn't seem like a bright idea to me, considering 
> >> that this gets called when starting every request. How will you look 
> >> at me adding the transport method for writing this register? :-)

Please check profiles first -- it might not be worth it. [1]

> > Convert SELECT_DRIVE() to use ->tf_load instead of ->OUTB.
> >
> >   OTOH, adding such a "backdoor" to the taskfile doesn't seem very 
> > consistent... well, I'm not excited about the whole idea conversion to 
> > tf_{load|read}() -- it's not clear what exactly this bought us.

This was explained some months ago already, so just to recall -- it was
a part of a bigger work removing duplicated code and allowing abstraction
of the ATA logic.

Anyway this is not set in a stone so if you have proposal of a better
approach please come forward with it.

>    We at least could have saved on memset() -- tf_load() method ignores 
> fields other than tf_flags anyway...

Unless it is huge performance win (unlikely) this is not a good idea
as it would be a maintainance nightmare.

->tf_load does only use cmd->tf_flags today but it might change one day
and nobody will remember to audit all users that they pass a valid cmd...

Thanks,
Bart

[1] coincidentally on the past Saturday I woke up with a bright idea
of doing some profiling of IDE code... I thought this would be a fun...

# readprofile -m System.map | grep ide_
    30 ide_intr                                   0.0714
     1 ide_map_sg                                 0.0112
     1 ide_complete_rq                            0.0152
    78 do_ide_request                             0.0643
     1 __ide_wait_stat                            0.0059
    51 ide_execute_command                        0.5100
     4 ide_set_handler                            0.0635
     7 ide_outb                                   1.1667
   308 ide_mm_inb                                44.0000
...

This was on ICH4 PATA controller...  Fun indeed, ain't it?

[ For non-ATA folks: it is _impossible_ for ide_mm_inb to be used
  on the above controller. ]

I still have to check what will happen if I would change the order
of following assignments in ide_tf_read():

...
        if (mmio) {
                tf_outb = ide_mm_outb;
                tf_inb  = ide_mm_inb;
        } else {
                tf_outb = ide_outb;
                tf_inb  = ide_inb;
        }
...

However I instead I went ahead and tried to run oprofile to get me some
more trushworthy results:

# opreport -l vmlinux
CPU: Pentium M (P6 core), speed 600 MHz (estimated)
Counted CPU_CLK_UNHALTED events (clocks processor is not halted, and not in a thermal trip) with a unit mask of 0x00 (No unit mask) count 100000
Segmentation fault

After this, testing some older kernel versions and assuring that user-space
packages are at their latest distro versions I had enough of fun...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/