On 9/26/06, Greg KH <[email protected]> wrote:
On Tue, Sep 26, 2006 at 10:01:06AM -0400, Dmitry Torokhov wrote:
> On 9/26/06, Greg KH <[email protected]> wrote:
> >On Tue, Sep 26, 2006 at 09:20:17AM -0400, Dmitry Torokhov wrote:
> >> On 9/26/06, Greg KH <[email protected]> wrote:
> >> Why do you feel the need to change internal kernel structures
> >> (ever-expanding struct device to accomodate everything that is in
> >> struct class_device) when it should be possible to simply adjust sysfs
> >> representation of the kernel tree (moving class devices into
> >> /sys/device/.. part of the tree) to udev's liking and leave the rest
> >> of the kernel alone. You have seen the patch, only minor changes in
> >> driver/base/class.c are needed to accomplish the move.
> >
> >Think about suspend. We want a single device tree so that the class
> >gets called when a device is about to be suspended so that it could shut
> >down the network queue in a common way, before the physical device is
> >called.
>
> Why can't the device itself manage it? If you want to stub out the
> common parts just create a function like netdev_suspend and call it at
> appropriate time.
Because you would then need to add that function call to _every_ network
device driver. This way, we do not need to do that as the class gets
called in the proper place before the device driver does.
In short, it makes it much simpler for the device driver writer, as it
is one less thing for them to get wrong
Ok, you convinced me here, we might want to add start/pause methods to
the class devices.
(you can implement it once in
the input core, and have it work for all input devices, no need to touch
every input driver too.)
I don't need it for input ;)
> >It's also needed if we want to have a single device tree in general.
> >class_device was the wrong thing and is really just a duplicate of
> >struct device in the first place (the driver core code implementing it
> >is pretty much just a cut and paste job.)
>
> They complement each other. They are different and need different
> methods to operate.
Not they are not. They really are just the same thing.
I do not think so. struct device manages real hardware and converts
data flow into in-kernel format. class device represent abstractions
the kernel presents to userspace. They normally have a dev_t number
associated with them and a specific (or several) protocol/interface
for accessing them. You are trying to mix it all together. Why does a
PCI device need a dev_t? Or i8042? They don't. Do they need multiple
interfaces? No, they don't. Does a network device needs to know about
multiple power management levels? No, it does not.
I am pretty sure there is a way of getting class devices into suspend
without merging these 2 abstractions. In fact, why can't we have all
class devices stopped first and then suspend all "real" devices? Then
we'd have 2 lists, one for class devices and another for devices.
> > The fact that we were
> >arbritrary marking it different has caused problems (look at the mess
> >that input causes to the class_device code, that's just not nice).
> >
>
> The only mess is that you refused to deepen the classification (i.e.
> have sub-classes). If input could be a parent class and
> mice/event/js/ts would grow from it it won't be such a mess.
But that is what we are now allowing you to do with devices. The whole
sub-class stuff was tried and failed.
Define failed? You just said you did not like it and ended up with
current code where individual class devices override in-kernel
interface (methods, attributes) specified by the class itself. In the
end we had to require updated version of udev anyway ;(
But in the end, it would almost
work with input devices, but then why not just make it a real device, so
you can use whatever heirachy you want to. I would think that you would
welcome this change.
> Alternatively we could go with input vs input_intf classes if flat
> classification is a must. Anyway, I don't think we want to break udev
> again.
Flat classification is not a must at all, and with these changes, you
don't need it. As an example, here's what the input tree looks like
when changed over:
$ tree /sys/class/input/
/sys/class/input/
|-- event0 -> ../../devices/platform/pcspkr/input0/event0
|-- event1 -> ../../devices/platform/i8042/serio4/input1/event1
|-- event2 -> ../../devices/platform/i8042/serio3/input2/event2
|-- input0 -> ../../devices/platform/pcspkr/input0
|-- input1 -> ../../devices/platform/i8042/serio4/input1
|-- input2 -> ../../devices/platform/i8042/serio3/input2
|-- mice -> ../../devices/virtual/input/mice
|-- mouse0 -> ../../devices/platform/i8042/serio3/input2/mouse0
`-- ts0 -> ../../devices/platform/i8042/serio3/input2/ts0
If you want, you can move any of these input devices anywhere in the
heirchay that you wish to do so. A symlink will be automatically
created in /sys/class/input so that userspace tools like udev can find
all of the input devices (which is something that is needed), but there
is no more rigid heirachy being imposed on any one. You are free to
move them at will, and no userspace tools will break as they will be
following the symlink instead.
The classification is still flat. You are allowing to move and stack
devices within /sys/devices/... subtree but the /sys/class/... is
flat.
> >Kay also has a long list of the reasons why, I think he's posted it here
> >before. Kay, care to send that list again?
> >
>
> Kay did send it and I agree with all his reasons as to why we need the
> move.
Great, they why are you objecting to these driver core patches?
I do not agree with implementation that's why I object to the patches.
> However I do not agree with your implementation.
Which implementation? The one I did for the class subsystem? Ok,
that's fine, your patch is still in my queue to look at, I'm not
ignoring it at all (had a bunch of "real life" work to get through this
last week and weekend, sorry, am still catching up.)
Don't worry, I'm not going to be pushing any input subsystem changes
wihout going through you first :)
These driver core patches are merely the needed functions for us to do
those input and other subsystem changes, they should not affect anything
of yours at all.
I am not concerned with you making changes to input system, I question
the need of such sweeping changes throughout all subsystems. If my
patch (plus possibly adding 2nd list for class devices for
suspend/resume) is sufficient then we do not need these core patches
in mainline and don't need to change subsystems.
> >> I really disappointed that there was no discussion/review of the
> >> implementation at all.
> >
> >There has not been any real implementation yet, only a few patches added
> >to the core that add a few extra functionality to struct device to allow
> >class_device to move that way.
>
> If there was no real discussion why you requesting these changes to be
> pulled in the mainline?
I didn't think these patches were controversial at all. They have been
in -mm for a few months now and work just fine.
I do not think being in -mm is enough justification for changes to the
driver core design. they do not appear to break stuff, here I agree,
but it does not mean that the design is proper.
Is there anything specfic in these patches that you object to?
Yes, everyting involving merging calss_device and device is a bit premature IMO.
Becides
the virtual thing (I tried it with a flat /sys/devices/virtual/ tree,
and it was a mess, I like the extra directory for classification, but in
the end, it doesn't matter, we can change it with no problem, as no
userspace tool will break if you move devices around the /sys/devices/
tree.)
--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
