Re: [PATCH 2.6.9-rc2 5/8] S2io: module loadable parameters

[Jeff Garzik <jgarzik@pobox.com>]

Major objections to this patch.

Comments:

1) Should use new module_param() not older, less-type-strict 
MODULE_PARM() in 2.6.x kernels

2) don't create individual module options when arrays are far better 
suited to the task.  The following technique simply bloats up the code 
and makes use of loops impossible:

> +static unsigned int tx_fifo_len_0 = DEFAULT_FIFO_LEN;
> +static unsigned int tx_fifo_len_1;
> +static unsigned int tx_fifo_len_2;
> +static unsigned int tx_fifo_len_3;
> +static unsigned int tx_fifo_len_4;
> +static unsigned int tx_fifo_len_5;
> +static unsigned int tx_fifo_len_6;
> +static unsigned int tx_fifo_len_7;

3) do not enable NETIF_F_SG without also enabling a checksum-offload/TSO 
mode of some sort

>  
> -       dev->features |= NETIF_F_SG | NETIF_F_IP_CSUM;
> +       dev->features |= NETIF_F_SG;
> +       if (cksum_offload_enable)
> +               dev->features |= NETIF_F_IP_CSUM;
>         if (sp->high_dma_flag == TRUE)
>                 dev->features |= NETIF_F_HIGHDMA;

4) I am unsure of the value of verify_load_parm().  Besides being huge, 
due to issue #2 (above), typically user module option limits should be 
documented, not necessarily tested.  Unix philosophy "root allowed to 
shoot themselves in the foot"