Re: [PATCH net v11 3/3] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Michael,

On Mon, Nov 15, 2021 at 12:40 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Add module parameter, IO mode autoprobe and PCMCIA reset code
> required to support 100 Mbit PCMCIA ethernet cards on Amiga.
>
> 10 Mbit and 100 Mbit mode are supported by the same module.
> Use the core PCMCIA cftable parser to detect 16 bit cards,
> and automatically enable 16 bit ISA IO access for those cards
> by changing isa_type at runtime. The user must select PCCARD
> and PCMCIA in the kernel config to make the necessary support
> modules available
>
> Code to reset the PCMCIA hardware required for 16 bit cards is
> also added to the driver probe.
>
> An optional module parameter switches Amiga ISA IO accessors
> to 8 or 16 bit access in case autoprobe fails.
>
> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
> Kazik <alex@kazik.de>.
>
> CC: netdev@vger.kernel.org
> Link: https://lore.kernel.org/r/1622958877-2026-1-git-send-email-schmitzmic@gmail.com
> Tested-by: Alex Kazik <alex@kazik.de>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Thanks for your patch!

> --- a/drivers/net/ethernet/8390/apne.c
> +++ b/drivers/net/ethernet/8390/apne.c
> @@ -119,6 +119,48 @@ static u32 apne_msg_enable;
>  module_param_named(msg_enable, apne_msg_enable, uint, 0444);
>  MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
>
> +static int apne_100_mbit = -1;
> +module_param_named(100_mbit, apne_100_mbit, int, 0444);
> +MODULE_PARM_DESC(100_mbit, "Enable 100 Mbit support");
> +
> +#if IS_ENABLED(CONFIG_PCMCIA)

What if CONFIG_PCMIA=m, and CONFIG_APNE=y?

> +static int pcmcia_is_16bit(void)
> +{
> +       u_char cftuple[258];
> +       int cftuple_len;
> +       tuple_t cftable_tuple;
> +       cistpl_cftable_entry_t cftable_entry;
> +
> +       cftuple_len = pcmcia_copy_tuple(CISTPL_CFTABLE_ENTRY, cftuple, 256);
> +       if (cftuple_len < 3)
> +               return 0;
> +#ifdef DEBUG
> +       else
> +               print_hex_dump(KERN_WARNING, "cftable: ", DUMP_PREFIX_NONE, 8,
> +                              sizeof(char), cftuple, cftuple_len, false);
> +#endif
> +
> +       /* build tuple_t struct and call pcmcia_parse_tuple */
> +       cftable_tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
> +       cftable_tuple.TupleCode = CISTPL_CFTABLE_ENTRY;
> +       cftable_tuple.TupleData = &cftuple[2];
> +       cftable_tuple.TupleDataLen = cftuple_len - 2;
> +       cftable_tuple.TupleDataMax = cftuple_len - 2;
> +
> +       if (pcmcia_parse_tuple(&cftable_tuple, (cisparse_t *)&cftable_entry))

Can't you avoid the cast, by changing the type of cftable_entry?
Perhaps you don't want to do that, to avoid abusing it below, but
perhaps you can use container_of() instead of the cast?

> +               return 0;
> +
> +#ifdef DEBUG
> +       pr_info("IO flags: %x\n", cftable_entry.io.flags);
> +#endif
> +
> +       if (cftable_entry.io.flags & CISTPL_IO_16BIT)
> +               return 1;
> +
> +       return 0;
> +}
> +#endif
> +
>  static struct net_device * __init apne_probe(void)
>  {
>         struct net_device *dev;
> @@ -140,6 +182,13 @@ static struct net_device * __init apne_probe(void)
>
>         pr_info("Looking for PCMCIA ethernet card : ");
>
> +       if (apne_100_mbit == 1)
> +               isa_type = ISA_TYPE_AG16;
> +       else if (apne_100_mbit == 0)
> +               isa_type = ISA_TYPE_AG;
> +       else
> +               pr_cont(" (autoprobing 16 bit mode) ");
> +
>         /* check if a card is inserted */
>         if (!(PCMCIA_INSERTED)) {
>                 pr_cont("NO PCMCIA card inserted\n");
> @@ -167,6 +216,14 @@ static struct net_device * __init apne_probe(void)
>
>         pr_cont("ethernet PCMCIA card inserted\n");
>
> +#if IS_ENABLED(CONFIG_PCMCIA)
> +       if (apne_100_mbit < 0 && pcmcia_is_16bit()) {
> +               pr_info("16-bit PCMCIA card detected!\n");
> +               isa_type = ISA_TYPE_AG16;
> +               apne_100_mbit = 1;
> +       }

I think you should reset apne_100_mbit to zero if apne_100_mbit < 0
&& !pcmcia_is_16bit(), so rmmod + switching card + modprobe
has a chance to work.

> +#endif
> +
>         if (!init_pcmcia()) {
>                 /* XXX: shouldn't we re-enable irq here? */
>                 free_netdev(dev);


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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