lkml.org 
[lkml]   [2001]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] drivers/net/others
    Date
    "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/

    \
     
     \ /
      Last update: 2005-03-22 12:53    [W:0.033 / U:29.828 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site