netdev - Re: [PATCH net-next v3 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

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] [day] [month] [year] [list]

Date:   Fri, 18 Jun 2021 20:28:42 +1200
From:   Michael Schmitz <schmitzmic@...il.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Finn Thain <fthain@...ux-m68k.org>,
        Linux/m68k <linux-m68k@...r.kernel.org>,
        ALeX Kazik <alex@...ik.de>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 2/2] net/8390: apne.c - add 100 Mbit support
 to apne.c driver

Hi Geert,

Am 18.06.2021 um 20:13 schrieb Geert Uytterhoeven:
>>
>> How does re-probing after a card change for a builtin driver work?
>> Changing the permission bits is a minor issue.
>
> Oh right, this driver predates the driver framework, and doesn't support
> PCMCIA hotplug.  So auto-unregister on removal doesn't work.
> Even using unbind/bind in sysfs won't work.
>
> So rmmod/modprobe is the only thing that has a chance to work...
>
>>>> The comment there says isa_type must be set as early as possible, so I'd
>>>> rather leave that alone, and add an 'else' clause here.
>>>>
>>>> This of course raise the question whether we ought to move the entire
>>>> isa_type handling into arch code instead - make it a generic
>>>> amiga_pcmcia_16bit option settable via sysfs. There may be other 16 bit
>>>> cards that require the same treatment, and duplicating PCMCIA mode
>>>> switching all over the place could be avoided. Opinions?
>>>
>>> Indeed.
>>
>> The only downside I can see is that setting isa_type needs to be done
>> ahead of modprobe, through sysfs. That might be a little error prone.
>>
>>> Still, can we autodetect in the driver?
>>
>> Guess we'll have to find out how the 16 bit cards behave if first poked
>> in 8 bit mode, attempting to force a reset of the 8390 chip, and
>> switching to 16 bit mode if this fails. That's normally done in
>> apne_probe1() which runs after init_pcmcia(), so we can't rely on the
>> result of a 8390 reset autoprobe to do the PCMCIA software reset there.
>>
>> The 8390 reset part does not rely on anything else in apne_probe1(), so
>> that code can be lifted out of apne_probe1() and run early in
>> apne_probe() (after the check for an inserted PCMCIA card). I'll try and
>> prepare a patch for Alex to test that method.

I just realized that might not work - ínit_pcmcia() enables the PCMCIA 
interface for us, so the early 8390 reset may not go through at all... 
The module parameter may have to stay as a fallback option at least.

>>
>>> I'm wondering how this is handled on PCs with PCMCIA, or if there
>>> really is something special about Amiga PCMCIA hardware...
>>
>> What's special about Amiga PCMCIA hardware is that the card reset isn't
>> connected for those 16 bit cards, so pcmcia_reset() does not work.
>
> I was mostly thinking about the difference between 8-bit and 16-bit
> accesses.

No idea. I've never even seen an 8 bit PCMCIA card - I have a few old 
16/32 bit ones around that were great for crashing my PowerBook, nothing 
else...

>>> And I'd really like to get rid of the CONFIG_APNE100MBIT option,
>>> i.e. always include the support, if possible.
>>
>> I can't see why that wouldn't be possible - the only downside is that we
>> force MULTI_ISA=1 always for Amiga, and lose the optimizations done for
>> MUTLI_ISA=0 in io_mm.h. Unless we autoprobe, we can use isa_type to
>> guard against running a software reset on 8 bit cards ...
>
> The latter sounds like a neat trick...

Yes, we can at least get rid of the APNE100MBIT option that way. I'll 
have to think about the autoprobe a bit more.

Cheers,

	Michael

> Gr{oetje,eeting}s,
>
>                         Geert
>