Re: [PATCH 1/2] Input: DaVinci Keypad Driver

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: David Brownell <david-b@pacbell.net>
Cc: Miguel Aguilar <miguel.aguilar@ridgerun.com>,
	nsnehaprabha@ti.com,
	davinci-linux-open-source@linux.davincidsp.com,
	linux-input@vger.kernel.org, todd.fischer@ridgerun.com,
	diego.dompe@ridgerun.com, clark.becker@ridgerun.com,
	santiago.nunez@ridgerun.com, Greg KH <gregkh@suse.de>
Subject: Re: [PATCH 1/2] Input: DaVinci Keypad Driver
Date: Wed, 23 Sep 2009 11:07:45 -0700	[thread overview]
Message-ID: <20090923180745.GE13435@core.coreip.homeip.net> (raw)
In-Reply-To: <200909231051.05714.david-b@pacbell.net>

On Wed, Sep 23, 2009 at 10:51:05AM -0700, David Brownell wrote:
> On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> > >> __devexit?
> > > [MA] According to comments from David Brownell to the first version of 
> > > this patch the __exit should be used.
> > >
> > > " - Use platform_driver_probe() and __exit/__exit_p();
> > >    there's no point in keeping that code around in
> > >    typical configs, it'd just waste memory. "
> > 
> > I am afraid David is wrong here.
> 
> No, you're just pointing out a bug introduced in some
> unrelated code.

From the very version of the patch platform_driver_probe() only ensured
that probe() would not be called after module inialization is done, it
never made any guarantees about removing devices. Hmm, let's add Greg as
well here.

>  Such bugs happen when folk ignore the
> code bloat issues.  (And didn't Linus recently point
> out how such code bloat is becoming an issue?)
> 
> 
> > Even when we register driver with 
> > platform_driver_probe() we still have "unbind" attribute in sysfs which
> > may be used to unbind the device from driver. If code is __exit then
> > such attempts will cause oops.
> 
> That would be a bug in the unbind() code, which doesn't
> currently recognize that not every driver or bus supports
> hotplugging.  It should probably check for a null release()
> pointer in the driver, and politely fail in that case.
> 

NULL release (as well as NULL probe) don't mean what you think they do.

> That's just about the only place that a third party
> (neither the driver nor its hotplug-aware bus framework)
> will try to decouple device and driver ... and it's doing
> it wrong.  So it's unlikely such bugs will be elsewhere.
> 

No, it does exactly what it is supposed to do. You may argue that such
facility is not needed but this is different. I'd argue that sometimes
you do need to shut off a device even if it is not normally
hot-pluggrable. Plus most of the blat is coming from probe() functions,
remove()s are fairly small.

> It's *ALWAYS* been legit to have a NULL pointer in the
> remove() methods.

Which means "device does not need any special actions for unbinding",
not that device can not be unbound. The same with probe(): NULL does not
mean that device can not be bound but rather that it does not need any
specal processing during binding.

>  That's why the __exit_p() -- or for
> hotpluggable drivers, __devexit_p() -- macros exist:
> for that particular case.

Huh? They are here so that the linker/module loader can discard them if
they only referenced via driver pointers and we know they not going to
be called.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html