Re: [PATCH] platform_driver_register: warn if probe is in .init.text

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

Hello,

On Wed, Jan 27, 2010 at 05:14:03PM -0800, Greg KH wrote:
> On Tue, Jan 26, 2010 at 09:47:41AM +0100, Uwe Kleine-König wrote:
> > On Mon, Jan 25, 2010 at 06:09:01AM +0900, OGAWA Hirofumi wrote:
> > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > > > +int platform_driver_register(struct platform_driver *drv)
> > > > +{
> > > > +	int ret = __platform_driver_register(drv);
> > > > +
> > > > +#if defined(CONFIG_HOTPLUG)
> > > > +	/*
> > > > +	 * drivers that are registered by platform_driver_register
> > > > +	 * should not have their probe function in .init.text.  The
> > > > +	 * reason is that a probe can happen after .init.text is
> > > > +	 * discarded which then results in an oops.  The alternatives
> > > > +	 * are using .devinit.text for the probe function or "register"
> > > > +	 * with platform_driver_probe.
> > > > +	 */
> > > > +	if (drv->probe && kernel_init_text_address((unsigned long)drv->probe))
> > > > +		pr_warning("oops-warning: probe function of platform driver %s"
> > > > +			       " lives in .init.text\n", drv->driver.name);
> > > > +#else
> > > > +	/*
> > > > +	 * without HOTPLUG probe functions can be discarded after the driver is
> > > > +	 * loaded.
> > > > +	 * There is a little chance for false positives, namely if the driver is
> > > > +	 * registered after the .init sections are discarded.
> > > > +	 */
> > > > +	if (drv->probe && !kernel_init_text_address((unsigned long)drv->probe))
> > > > +		pr_info("probably the probe function of platform driver %s can"
> > > > +				" be moved to .init.text\n", drv->driver.name);
> > > > +#endif
> > > > +	return ret;
> > > > +}
> > > 
> > > Um..., can't we extend modpost or such one for this? I think the static
> > > analysis is better (assume the changing ->probe dynamically is really
> > > rare).
> > I like the idea and will look later into modpost if this can be done
> > there.
> 
> That would be nice to do instead, as we already do checks like this
> today, and might make more sense.
> 
> And could you do it for all probe functions, and not just the platform
> devices?  Don't all busses have this same problem?
I think so, yes.  And I made some changes to modpost to detect those.
Tested on two defconfigs (ARCH=arm) it yields three hits, all valid.

I send the series as reply to this mail or you can get it via git, see
below.

The first six patches should not change behaviour, only do some cleanup
and preparation for the last patch.

The most interesting patch is "make symbol white list a per mismatch
type variable".  It allows a white list per section mismatch type and
so allows to say:

	*driver (in .data) might reference to .devinit.text but not
	.init.text

That's what the last patch does.

Best regards
Uwe

The following changes since commit 499a2673713c85734a54c37dd90b4b729de399c4:
  Linus Torvalds (1):
        Merge branch 'for-linus' of git://git.kernel.org/.../dtor/input

are available in the git repository at:

  git://git.pengutronix.de/git/ukl/linux-2.6.git modpost

Uwe Kleine-König (7):
      modpost: members of *driver structs should not point to __init functions
      modpost: define ALL_XXX{IN,EX}IT_SECTIONS
      modpost: give most mismatch constants a better name
      modpost: pass around const struct sectioncheck * instead of enum mismatch
      modpost: remove now unused NO_MISMATCH constant
      modpost: make symbol white list a per mismatch type variable
      modpost: don't allow *driver to reference .init.*

 scripts/mod/modpost.c |  152 ++++++++++++++++++++++++++++---------------------
 1 files changed, 88 insertions(+), 64 deletions(-)

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |