Messages in this thread | | | From | Andrzej Krzysztofowicz <> | Subject | Re: [PATCH] drivers/net/others | Date | Fri, 25 May 2001 13:38:19 +0200 (MET DST) |
| |
"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(becker@cesdis.gsfc.nasa.gov)\n"; > + KERN_INFO "at1700.c:v1.15 4/7/98 Donald Becker (becker@cesdis.gsfc.nasa.gov)\n"; > > to add KERN_xxx tags, then you could also: > s/becker@cesdis.gsfc.nasa.gov/becker@scyld.com/
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 ankry@mif.pg.gda.pl phone (48)(58) 347 14 61 Faculty of Applied Phys. & Math., Technical University of Gdansk
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |