Re: [PATCH] drivers/net/others

[Andrzej Krzysztofowicz]

"Paul Gortmaker wrote:"
> Some hopefully useful/constructive feedback:
> 
> Andrzej Krzysztofowicz wrote:
> > 
> > +static char version[]
> > +#ifdef MODULE
> > +       __initdata
> > +#else
> > +       __devinitdata
> > +#endif
> > +       = KERN_INFO RTL8139_DRIVER_NAME "\n";
> 
> This doesn't look right. If defined(MODULE) then __initdata
> is a no-op (see linux/init.h).  You probably just want:

Currently it is no-op.
Hopefully it would change some day. I've seen a patch pointer somewhere.

Maybe you are right that potential only remove of one string from loaded
module code is not worth the change.

> Generally we should aim to reduce the number of #ifdef MODULE, rather
> than add more.  If the driver load paths look the same regardless
> of whether built in or modular then driver maintenance is easier.
> (Ok, removing existing #ifdef MODULE is a 2.5 thing, but we should
> avoid adding more in 2.4.x)

IMO there's no difference whoether there's 3 or 4 of them.
There's no point in adding an new #ifdef only if there's none currently
(IMHO).

> We can probably do something better with cases like these too:
> 
> > +#ifdef MODULE
> > +       am79c961_banner();
> > +#endif /* MODULE */
> 
> I think the days of kitchen sink kernels with 20 drivers all compiled in 
> are over, and so we should just do the version printk/banner unconditionally. 

Jeff, your opinion here ?

> This way, if you have unused drivers built into your image, at least you 
> will have a way of knowing it.  (People who don't use modules are clearly 
> building their own kernels and don't want any unused drivers accidentally
> glued into their image).

> Other options for dealing with printing driver version info include:
> 
> (1) to replace the printk(...) with e.g. module_banner(...) and have the
> conditional stuff hidden in how module_banner() is defined in module.h 
> 
> (2) have sys_create_module or sys_init_module print out the 
> MODULE_DESCRIPTION and (optionally?) MODULE_AUTHOR for all modules
> thus removing code replication from each module. (This assumes that
> the modinfo section is, and will remain with modules in the future).

> I personally like the sounds of (2) a lot.  Of course we would have to
> make sure all modules had a useful MODULE_DESCRIPTION.
> 
> I'd avoid making patches like this:
> 
> > -#endif
> > +#endif /* CONFIG_ISAPNP || CONFIG_ISAPNP_MODULE */

OK.

> ...they fall into the category of "patching for the sake of patching"
> (which is not good in 2.4.x) and you end up inflicting your style on 
> the original author(s) who may not like it (especially if the corresponding
> #ifdef is only one line up...).  One could argue that the printk(version)
> vs. printk("%s\n", version) changes fall into the same category...
> 
> Also might want to avoid changes like this:
> 
> > -       if (ei_debug  &&  version_printed++ == 0)
> > +       if (version_printed++ == 0)
> >                 printk(version);

It was intentional. I follor Jeff's suggestion of printing the version banner
rules, i.e.
- print version unconditionally for modules
- print version only if hardware has been detected for built-in.

Having different behaviour of different modules here is very bad IMO.

> that are actually changing the original author's intention. In this case
> (ne.c) the compiled output remains unchanged, and I'm okay with the

Unfinished.

> change in intention since we now have the "quiet" boot argument anyway.
> 
> Also, if you are changing version strings like this:
> 
>  static char version[] __initdata =
> -    "at1700.c:v1.15 4/7/98  Donald\ Becker([e-mail])\n";
> +    KERN_INFO "at1700.c:v1.15 4/7/98  Donald Becker ([e-mail])\n";
> 
> to add KERN_xxx tags, then you could also:
> 	 s/[e-mail]/[e-mail]/

OK. Thanks for the suggestion.

> and I'm sure Donald would thank you for it.
> 
> Finally, breaking your patch into logical chunks / separate e-mails would 
> also ensure that your work has a better chance of getting used - e.g.

Final version will be splitted out with Cc: to the appropriate maintainers.

>  [PATCH] add KERN_INFO to version tags of net drivers
>  [PATCH] missing __[dev]initdata in net drivers
>  [PATCH] add MODULE_PARM_DESC to various net drivers

IMO, no chance. They are too close to be separated in many places.
I would rather find out which of the changes are bad/unacceptable.

Thanks for your comments.

Andrzej

-- 
=======================================================================
  Andrzej M. Krzysztofowicz               [e-mail]
  phone [phone]
Faculty of Applied Phys. & Math.,   Technical University of Gdansk