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

Message-ID: <CAMuHMdV5Yd2w+maSn-dQ=NOrVyVc8JjV38miKRc-pvnzBcKSig@mail.gmail.com>
Date:   Fri, 18 Jun 2021 10:13:22 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Michael Schmitz <schmitzmic@...il.com>
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 Michael,

On Fri, Jun 18, 2021 at 10:06 AM Michael Schmitz <schmitzmic@...il.com> wrote:
> Am 18.06.2021 um 19:16 schrieb Geert Uytterhoeven:
> >>>> +#ifdef CONFIG_APNE100MBIT
> >>>> +    if (apne_100_mbit)
> >>>> +            isa_type = ISA_TYPE_AG16;
> >>>> +#endif
> >>>> +
> >>> I think isa_type has to be assigned unconditionally otherwise it can't be
> >>> reset for 10 mbit cards. Therefore, the AMIGAHW_PRESENT(PCMCIA) logic in
> >>> arch/m68k/kernel/setup_mm.c probably should move here.
> >>
> >> Good catch! I am uncertain though as to whether replacing a 100 Mbit
> >> card by a 10 Mbit one at run time is a common use case (or even
> >> possible, given constraints of the Amiga PCMCIA interface?), but it
> >> ought to work even if rarely used.
> >
> > Given it's PCMCIA, I guess that's a possibility.
> > Furthermore, always setting isa_type means the user can recover from
> > a mistake by unloading the module, and modprobe'ing again with the
> > correct parameter.
> > For the builtin-case, that needs a s/0444/0644/ change, though.
>
> 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'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.

> Whether the software reset workaround hurts for 8 bit cards is something
> I don't know and cannot test. But
>
> > 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...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds