On Sat, 2008-03-15 at 09:19 -0500, James Bottomley wrote: > On Fri, 2008-03-14 at 22:58 +0100, Kay Sievers wrote: > > On Fri, 2008-03-14 at 16:20 -0500, James Bottomley wrote: > > > On Fri, 2008-03-14 at 12:15 -0500, James Bottomley wrote: > > > > On Thu, 2008-03-13 at 14:06 -0700, Greg KH wrote: > > > > > Here's a huge patch from Tony and Kay that converts the scsi layer to > > > > > use struct device instead of class_device. > > > > > > > > > > It doesn't seem like it could be split up any smaller due to the > > > > > interconectedness of the whole mess, if you have any suggestions > > > > > otherwise, it would be appreciated. > > > > > > > > > > If you want, I can take this through my tree as it does depend on a > > > > > previous IB patch to make that portion of the patch much smaller. > > > > > > > > > > After this, all of the class_device code is now finally gone from the > > > > > kernel! > > > > > > > > Actually, I have it built and running (actually 2.6.25-rc5-mc5 which > > > > includes all the changes in your tree). Amazingly it's pretty much > > > > fully functional, except ses which seems to have suffered a breakdown in > > > > the way its model works. I'll see if I can fix it up. > > > > > > OK, I looked at converting ses and enclosure, but it looks impossible > > > given the way you've set all of this up (and at the moment it's almost > > > non functional). > > > > > > What it wants is for the enclosure and component devices to be arranged > > > hierarchically under the enclosure class and then the device link of the > > > components to be populated if they're actually present (which they might > > > not be). > > > > > > Given the fact that you've chosen to have dev->parent point to the > > > device the original class_device->dev was pointing to, there's almost no > > > way left to express this relationship. We can do it by having > > > kobj->parent point to the enclosure and dev->parent point to the > > > enclosed device, but I can't encapsulate that information into a struct > > > device alone. > > > > > > >From an interface point of view, this dual use of the parent pointer > > > looks a bit nasty ... and you have some fun code to prove the point in > > > drivers/base/core.c:get_device_parent() > > > > > > How about just adding a struct device *peer; entry to struct device and > > > using it to express the former class relationship? That way we can do > > > all the parenting stuff correctly. > > > > > > Otherwise, I'm going to need something like a device_add_class_parent() > > > API that allows me to add these former class devices and set up the > > > kobject parent correctly. > > > > Unfortunately, the enclosure/enclusure-component/component-device > > relationship is not a tree. There is only a single "parent" for a > > device. The "device" link already expresses the parent device, and the > > class devices will show up as childs of the devices, where the "device" > > link pointed to. There can't be a second device which could be used as a > > parent. > > But that's precisely my point: it is a tree; it's not a multi rooted > tree either. Hmm, the enclosure components have two parents in this model, the enclosure itself _and_ the scsi device, so it's not a flat tree anymore. > Each enclosure has a fixed number of components (the bays > in the enclosure) that's a simple two level tree. I think it's not a second level, it's a second tree if you have two parents for one single node. The driver core only supports one flat device tree, and one parent relationship per device. > However, both the > enclosure and the components may point to devices in the regular tree > (bays only if the bay is actually populated). Sure, that's fine, but still, the component can have only one parent managed by the core, we have only one single device tree at /sys/devices/ and other relations should be expressed by the driver itself. > This was formerly representable in the class_device infrastructure, > because class devices were allowed to have parents. The "parent" field was something Greg added for trying to represent "input", and it failed, existing userspace logic could not handle it, and we immediately deprecated hierarchies in /sys/class. It was never supposed to be used with the "dev" field pointing to a different device at the same time. As documented in Documentation/, all this "device" link magic we are talking about, will only work in SYSFS_DEPRECATED=y mode anyway, which no recent distro uses anymore, and the enclosure stuff was added long after that. > Class devices are > supposed to represent interfaces to devices That's not true. Class devices are just devices, as documented in the Documentation/ directory. Class devices are _not_ supposed to be anything special, they just don't have drivers/binding in the kernel internals - no other difference. There is no such thing as an "interface" in the driver core or sysfs. Every device _is_ the interface itself, otherwise devices would have no functional sysfs attributes. It was one of the biggest mistakes to expose such a kernel implementation details as different entities to userspace, and one reason sysfs is that hard to understand. The guy who did all that just ran away leaving all this mess behind him. :) And yes, we are too slow cleaning all that up, but we will get there over time. > and not all interfaces are > fully flat. True, but the core is only one flat tree, the drivers need to express "reverse trees" themselves, just like the raid block devices do. > > A sysfs class device hierarchy, and at the same let the "device" link > > point to a different device is not supported. Existing userspace tools > > do not support that. > > > > We have a similar problem for raid block devices, which can't be > > expressed in a single device tree. The "reverse tree" is constructed by > > custom holders/ slave/ directories at the devices. > > > > I suggest to express the relationship of the enclosure components to > > the enclosure device by custom symlinks, instead of expecting a "device" > > link maintained by the core to build a "reverse tree". > > > > Would that work? > > There are still several problems with the symlink approach; not least of > which is that the component namespace isn't globally unique, it's only > unique to the enclosure: If you have two separate enclosures connected, > they're likely each going to have a component called slot1 (the name is > actually take from the enclosure). I also think we're going to have > difficulty going back from a component to an enclosure, which was pretty > much the whole point of the exercise but I need to get ses/enclosure > actually working to look at that. Hmm, I don't understand. The only thing that changed is that you don't have a core-managed "device" link at the enclosure component anymore, nothing else should be different. The namespace issue should be the same as it was before, right? We just need to create something like a "contains" link from the component to the scsi device, and a "enclosure" link at the scsi device back to the component, right? Thanks, Kay -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
