netdev - Re: [PATCH v4 9/9] net-next: New ax88796 platform driver for Amiga X-Surf 100 Zorro board (m68k)

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]

Date:   Sat, 9 Jun 2018 12:28:23 +1200
From:   Michael Schmitz <schmitzmic@...il.com>
To:     Michael Karcher <michael.karcher@...berlin.de>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Finn Thain <fthain@...egraphics.com.au>,
        Florian Fainelli <f.fainelli@...il.com>,
        Linux/m68k <linux-m68k@...r.kernel.org>,
        Michael Karcher <kernel@...rcher.dialup.fu-berlin.de>
Subject: Re: [PATCH v4 9/9] net-next: New ax88796 platform driver for Amiga
 X-Surf 100 Zorro board (m68k)

Hi Michael,

Am 08.06.2018 um 21:28 schrieb Michael Karcher:
> Let me add my 2 cents as main author of that code:
...
>
> actually, the the block_input / block_output functions were the reason I
> included the lib8390.c file. Except for xs100_write / xs100_read, they are
> a verbatim copy from ax88796.c I'm not that enthusiastic about that idea
> anymore, but did not get around to improve it. I added a customization
> point to ax88796 for a custom block_input / block_output, because the 8390
> core already provides that customization point. What I really need is a
> customization point just for the 8390-remote-DMA-via-MMIO functions (i.e.
> xs100_write, xs100_read) instead of the whole block transfer core that
> also sets up the remote DMA engine and tries to re-initialize the card in
> case of unexplained problems.

OK, so essentially change

         if (ei_local->word16) {
                 ioread16_rep(nic_base + NE_DATAPORT, buf, count >> 1);
                 if (count & 0x01)
                         buf[count-1] = ei_inb(nic_base + NE_DATAPORT);

         } else {
                 ioread8_rep(nic_base + NE_DATAPORT, buf, count);
         }

to something like

         if (ax->block_read) {
		ax->block_read(dev, buf, count);
	} else if (ei_local->word16) {
                 ioread16_rep(nic_base + NE_DATAPORT, buf, count >> 1);
                 if (count & 0x01)
                         buf[count-1] = ei_inb(nic_base + NE_DATAPORT);

         } else {
                 ioread8_rep(nic_base + NE_DATAPORT, buf, count);
         }

and populate ax->block_read() and ax_block_write() from platform data, 
instead of substituting ax_block_input() / ax_block_output() wholesale?

I must be missing something here.

> This should get rid of
>  - xs100_block_output
>  - xs100_block_input (which has the calls to ax_reset_8390 and
> ax_NS8390_init)
>  - ax_reset_8390
>  - and thus the include of lib8390.c (which should be included only by
> ax88796.c, not by xsurf100.c)

I've got an (untested) patch that just exports ax_NS8390_init() via the 
ax_device struct, and gets rid of the lib8390.c include that way, but 
the above solution would be a lot cleaner. Adds one test for 
ax->block_read on the critical path but we already have the test for 
ei_local->word16 there. May need rearranging the tests so the majority 
of ax88796 users isn't impacted.

Anyway, your code, your call.

Cheers,

	Michael


>
> Regards,
>   Michael Karcher
>