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

From: David Brownell <david-b@pacbell.net>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
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 12:29:07 -0700	[thread overview]
Message-ID: <200909231229.07403.david-b@pacbell.net> (raw)
In-Reply-To: <20090923180745.GE13435@core.coreip.homeip.net>

On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> 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.

I don't see your point.  If the driver doesn't support
any unbinding mechanism -- a remove() method, converse
of probe() -- then it can't safely be unbound.

And thus, if any code is presuming that *every* driver
can be unbound, it's wrong.

As I said:  bug in other code.

Looking at this a bit more, it seems like there will need
to be some "can this bus remove this driver" check, since
the struct device.remove method is now managed at the bus
level.  Easy enough to do instead of the null check that
I mentioned below.  Provide it for platform bus, and the
main potential trouble spots will be resolved.

> >  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?)

Though to be fair, it's *always* been an issue for anyone
working with embedded Linux systems.  And it's long been
a frustration that too few "mainline" developers didn't
accept such issues.

> > > 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.

I could say the same things about how you think, of course,
and probably with just as little accuracy.  Let's not be
needlessly confrontational.

> > 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. 

Erm, once a driver is freely poking around in the hardware,
and is managing IRQs and other asynch events ... exactly
how do you expect to decouple it from the hardware without
a remove()?  Drivers that *don't* respond asynchronously to
hardware events are an extreme minority; it's not worth
even considering them as the design center.

As a policy choice, there's only one sane one:  unbinding
the driver always requires an active ack from the driver.

Even if that's a trivial/NOP remove() function ... instead
of one which cleanly tears down the hardware event stream;
the driver's handling thereof; and whatever resources the
driver claimed, including links to upper level code in the
operating system (like char/block device nodes and many
other things).

> Plus most of the blat is coming from probe() functions, 
> remove()s are fairly small.

Not always.  Of course, there's still that nasty issue
with code that probe() and remove() need to share ... for
now there's no clean way to have such code be removable.
(Like an "__init_or_exit" section annotation.)  Drivers
have been stuck putting such code in the .text segement,
and thus causing additional driver bloat.

> > 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.

I don't know where such a model comes from.  I'll just
say that in quite a few years of driver development I
have *never* come across hardware where "doesn't need
any attention during driver unbind" makes sense.

Recall that any driver has both lower (to hardware) and
upper (to rest of OS) links, and maybe more (to other
drivers) ... and it's a design goal in Linux to have such
links be explicit, so the drivers are more reusable.  So
requiring some hook -- remove() -- before attempting
to break the links internal to the driver model seems
like a fairly obvious requirement.

> >  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.

Finish that thought.  What "exit" section code fits those
categories?  Driver remove() methods ... and not much else.
And for that matter, what else goes into exit sections,
other than such code and module hooks to call it?

That discarding of exit sections has been around in Linux
for an extremely long time, at least on architectures which
lean more towards embedded systems than bloat-is-OK servers.
It's applied to driver remove methods, and to the code which
invokes them via rmmod.

Note that your comments all seem to presume that such
discarding is either useless or should be phased out for
some reason ... but you're not quite making *that*
argument.

- Dave

--
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