Message-ID: <466EAE21.7090407@s5r6.in-berlin.de>
Date:	Tue, 12 Jun 2007 16:30:57 +0200
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	"Huang, Ying" <ying.huang@...el.com>
CC:	linux-kernel@...r.kernel.org, Greg KH <greg@...ah.com>
Subject: Re: [PATCH] driver core: multithreaded device matching with	dependency

Huang, Ying wrote:
> The summary of dependency rule is as follow:
> 
> 1. A flag as follow is added to struct device.
>      unsigned multithreaded_probe:1;
> If it is set, the devices sub-tree with this device as root will be
> probed parallelized with other devices sub-tree. If it is clear, the
> device belongs to the devices sub-tree of the parent of the device, and
> should be probed serially with other devices in the devices sub-tree.
> The root devices (without parent) is assumed to have this flag set (in
> spite of the actual value). With this flag, the PCI subsystem can be
> probed serially, while IEEE 1394 subsystem can be probed parallelized
> among different device node, but serially among different unit in a
> node.
> 
> 2. A field as follow is added to struct device.
>      struct device *depend;
> The device will not start probing unless the probing of the device
> pointed by depend has been finished. This is used to control some random
> dependency between devices.
> 
> 3. The probing of the device will not be started, unless the probing of
> the parent of the device has been finished.
> 
> I revised my patch to reflect the rule above, so resend it.

Looks good, except for two points, IMO:

  - I'd still like to see kerneldoc comments in your patch for each new
    API element.  You did so already for the new exported functions, but
    you didn't provide kerneldoc for the new members of struct device
    which are API elements.

    Of course, the whole struct device is /not/ quite well documented at
    the moment, but that doesn't mean that new extensions should make
    matters even worse.

  - Your rules 1 and 3 are simple and useful.  Rule 2 is in itself
    simple too, but the maintenance of the dependency tree which is
    built from the .depend pointers comes at a cost.  (Devices come and
    go all the time; the .depend pointers must always be valid.  How is
    this accomplished?)

    Consider to reduce your patch to rule 1 and 3, and throw out rule 2.
    If a subsystem has a more complicated requirement which cannot be
    satisfied by rule 1 and 3, it can either just restrict itself to
    dumb single-threaded device-driver matching, or it can pretend to be
    fully capable of parallel device-driver matching and ensure the
    necessary serialization by own internal mutexes.

There is so much unskilled labor in driver hacking land (I'm merely
speaking for myself of course) that something like the driver core
/really/ needs well-working, easy to understand, and well-documented APIs.

PS:  By a well-documented struct definition I mean:
   - Public members have a type and name which indicate the meaning of
     the member, and there is a comment on permissible or required
     write/read accesses to the member.
   - Private members are undocumented or even better yet explicitly
     marked as private.
If the access methods to public members turn out too difficult and
fragile, make the members private and provide documented public accessors.
-- 
Stefan Richter
-=====-=-=== -==- -==--
http://arcgraph.de/sr/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists