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

Geert Uytterhoeven
2021-11-17T14:43:52Z

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