linux-kernel - Re: [PATCH 01/10] floppy: cleanup: expand macro FDCS

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200226081808.GA1589@1wt.eu>
Date:   Wed, 26 Feb 2020 09:18:08 +0100
From:   Willy Tarreau <w@....eu>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Denis Efremov <efremov@...ux.com>, Jens Axboe <axboe@...nel.dk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-block <linux-block@...r.kernel.org>
Subject: Re: [PATCH 01/10] floppy: cleanup: expand macro FDCS

On Tue, Feb 25, 2020 at 10:08:51AM -0800, Linus Torvalds wrote:
> We should at least try moving those bits to the floppy.c file and
> remove it from the header file.
> 
> For example, doing a Debian code search on "FDPATCHES" doesn't find
> any user space hits. Searching for "FD_STATUS" gets a lot of hits, but
> thos all seem to be because it's a symbol used by user space programs,
> ("file descriptor status"), not because those hits actually used the
> fdreg.h header file.
> 
> So we can remove at least the FD_IOPORT mess from the header file, I bet.

OK so I think this time I managed to get it done after two failed attempts.

I've sent in response to this thread 6 new patches to the series just for
validation (11 to 16), I'll spam relevant people when resending the whole
if we agree on the principle already.

First, still no single byte change in the output code:
  willy@...p:master$ diff -u floppy-{before,after}.s
  --- floppy-before.s     2020-02-26 08:59:04.185152450 +0100
  +++ floppy-after.s      2020-02-26 08:58:58.253156733 +0100
  @@ -1,5 +1,5 @@

  -floppy-before.o:     file format elf64-x86-64
  +floppy-after.o:     file format elf64-x86-64

   Disassembly of section .text:

Second, I could kill FD_IOPORT entirely. The FD_* macros are now
just the registers offsets. I've added two local functions fdc_inb()
and fdc_outb() which take an fdc and the register, and remap this
to fd_inb() and fd_outb() so that we don't need to fiddle anymore
with "fdc". I had one attempt at propagating that cleanup (base+reg
instead of port) to various archs, it was OK but didn't bring any
visible value in my opinion.

Third, I renamed "fdc" to "current_fdc" and carefully replaced all
"fdc" instances which didn't build with "current_fdc". This revealed
that at many places we iterate over current_fdc just because it was
the required name for the register macro (which used to derive from
FD_IOPORT). So at this point I'm still seeing a lot of possible
cleanups which will produce different binary output but will be quite
more reviewable. The common pattern in floppy.c is :

    for (current_fdc = 0; current_fdc < N_FDC; current_fdc++) {
        do_something(current_fdc);
    }
    current_fdc = 0;

  or:

    for (i = 0; i < N_FDC; i++) {
        current_fdc = i;
        do_something(current_fdc);
    }
    current_fdc = 0;

These ones can safely be cleaned up.

I also thought that once done we could have a "current_fdc" being a
struct floppy_fdc_state* instead of an int and directly point to the
correct fdc_state. This way we'll regain a lot of readability in the
code.

Please just tell me what you think and if I should repost a whole
series and/or continue the cleanup.

Thanks,
Willy