Re: [PATCH] enclosure: add support for enclosure services
On Wed 2008-02-13 09:45:02, Kristen Carlson Accardi wrote: On Tue, 12 Feb 2008 13:28:15 -0600 James Bottomley [EMAIL PROTECTED] wrote: On Tue, 2008-02-12 at 11:07 -0800, Kristen Carlson Accardi wrote: I understand what you are trying to do - I guess I just doubt the value you've added by doing this. I think that there's going to be so much customization that system vendors will want to add, that they are going to wind up adding a custom library regardless, so standardising those few things won't buy us anything. It depends ... if you actually have a use for the customisations, yes. If you just want the basics of who (what's in the enclousure), what (activity) and where (locate) then I think it solves your problem almost entirely. So, entirely as a straw horse, tell me what else your enclosures provide that I haven't listed in the four points. The SES standards too provide a huge range of things that no-one ever seems to implement (temperature, power, fan speeds etc). I think the users of enclosures fall int these categories 85% just want to know where their device actually is (i.e. that sdc is in enclosure slot 5) 50% like watching the activity lights 30% want to be able to have a visual locate function 20% want a visual failure indication (the other 80% rely on some OS notification instead) When you add up the overlapping needs, you get about 90% of people happy with the basics that the enclosure services provide. Could there be more ... sure; should there be more ... I don't think so ... that's what value add the user libraries can provide. James I don't think I'm arguing whether or not your solution may work, what I am arguing is really a more philosophical point. Not can we do it this way, but should we do it way. I am of the opinion that Hw abstraction is still kernel's job. That's why we have leds exported in sysfs... let vendors have their libraries, but lets put the 'everyone does these' stuff in kernel. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
--- On Tue, 2/12/08, James Bottomley [EMAIL PROTECTED] wrote: I apologize for taking so long to review this patch. I obviously agree wholeheartedly with Luben. The problem I ran into while trying to design an enclosure management interface for the SATA devices is that there is all this vendor defined stuff. For example, for the AHCI LED protocol, the only defined LED is 'activity'. For LED2 and LED3 it is up to hardware vendors to define these. For SGPIO there's all kinds of ways for hw vendors to customize. I felt that it was going to be a maintainance nightmare to have to keep track of various vendors enclosure implementations in the ahci driver, and that it'd be better to just have user space libraries take care of that. Plus, that way a vendor doesn't have to get a patch into the kernel to get their new spiffy wizzy bang blinky lights working (think of how long it takes something to even get into a vendor kernel, which is what these guys care about...). So I'm still not sold on having an enclosure abstraction in the kernel - at least for the SATA controllers. Correct me if I'm wrong, but didn't the original AHCI enclosure patch expose activity LEDs via sysfs? I'm not saying there aren't a lot of non standard pieces that need to be activated by direct commands or other user activated protocol. I am saying there are a lot of standard pieces that we could do with showing in a uniform manner. Which is already the case without the SES kernel bloat. Case in point, the excellent user-space application lsscsi would clearly show which device is SES. And the excellent user-space application sg_ses could control an SES device. The pieces I think are absolutely standard are 1. Actual enclosure presence (is this device in an enclosure) lsscsi 2. Activity LED, this seems to be a feature of every enclosure. So that means that it needs a kernel representation? If this indeed were the case, for every feature of every type of device (not only SCSI) then the kernel itself would be insurmountably big. I also think the following are reasonably standard (based on the fact that most enclosure standards recommend but don't require this): 3. Locate LED (for locating the device). Even if you only have an activity LED, this is usually done by flashing the activity LED in a well defined pattern. 4. Fault. this is the least standardised of the lot, but does seem to be present in about every enclosure implementation. All I've done is standardise these four pieces ... the services actually take into account that it might not be possible to do certain of these (like fault). And none of this means that it needs a kernel representation. 1. You're not standardizing any known, in-practice, kernel representation, that is already in practice and thusly needs a kernel representation. 2. The kernel itself is not using nor needing this representation in order to function properly (the kernel). Leaving control of SES devices to user-space makes both the kernel and the vendors happy. All the kernel needs to do is expose the SES device to user-space as it currently does. It makes it so much easier both to vendors and to the kernel to stay out of unnecessary representations. Vendors may choose to distribute their own applications to control their hardware, as long as the kernel exposes an SES device and provides functionality, as opposed to policy of any kind. Luben - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
--- On Tue, 2/12/08, James Bottomley [EMAIL PROTECTED] wrote: I understand what you are trying to do - I guess I just doubt the value you've added by doing this. I think that there's going to be so much customization that system vendors will want to add, that they are going to wind up adding a custom library regardless, so standardising those few things won't buy us anything. It depends ... if you actually have a use for the customisations, yes. If you just want the basics of who (what's in the enclousure), what (activity) and where (locate) then I think it solves your problem almost entirely. If the kernel doesn't solve it entirely, then it is better off without the kernel bloat. In fact vendors would distribute their own user-space application(s) to provide a consistent and complete solution(s) of their product(s) to their customer(s). This could also be achieved via sg_ses, which can also control custom vendor pages if the encodings are known by the customer (which they would be if they purchased the product). So, entirely as a straw horse, tell me what else your enclosures provide that I haven't listed in the four points. You shouldn't insist on this. It should already be clear to you this direction isn't the vendor's preference. The SES standards too provide a huge range of things that no-one ever seems to implement (temperature, power, fan speeds etc). Vendors do use temperature, power and fan speeds and in fact more features, some of them completely custom for their product, since they end up writing the SES device server of the enclosure themselves (for their product). This kind of control and representation is better left to user-space. The kernel neither needs to represent it nor know about it, since it is itself not using nor controlling it. I think the users of enclosures fall int these categories This statement (above) really means that the numbers below are but merely the speculation of one person. 85% just want to know where their device actually is (i.e. that sdc is in enclosure slot 5) 50% like watching the activity lights 30% want to be able to have a visual locate function 20% want a visual failure indication (the other 80% rely on some OS notification instead) When you add up the overlapping needs, you get about 90% of people happy with the basics that the enclosure services provide. 90% doesn't make it a necessary requirement for the kernel to have this, as well as the fact that the kernel is neither needing this to function properly, nor using it. Could there be more ... sure; should there be more ... I don't think so ... that's what value add the user libraries can provide. should there be more ... I don't think so This decision isn't yours to make. In fact such a decision is never made by one person only. This decision is made by 1) the industry, 2) the customers and 3) vendors by the pricing of their product(s). Luben - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
The keep-it-in-user-space arguments seem fairly compelling to me. Especially as we've pushed whole i/o subsystems out to user space (iscsi, stgt, talked about fcoe, a lot of dm control, etc). The functionality seems to align with Doug's sg/lsscsi utility chain as well. Granted, the new utility would have to be designed in such as way that it can incorporate vendor hardware handlers. But, if James has a somewhat common implementation already for a kernel implementation, I'm sure that can be the starting point for lsscsi. So, the main question I believe is being asked is: - Do we need to represent this via the object/sysfs tree or can an outside utility be depended upon to show it ? Note that I am not supporting: Vendors may choose to distribute their own applications. For this to become truly useful, there needs to be a common tool/method that presents common features in a common manner, regardless of whether it is in kernel or not. -- james s Luben Tuikov wrote: Which is already the case without the SES kernel bloat. Case in point, the excellent user-space application lsscsi would clearly show which device is SES. And the excellent user-space application sg_ses could control an SES device. The pieces I think are absolutely standard are 1. Actual enclosure presence (is this device in an enclosure) lsscsi 2. Activity LED, this seems to be a feature of every enclosure. So that means that it needs a kernel representation? If this indeed were the case, for every feature of every type of device (not only SCSI) then the kernel itself would be insurmountably big. I also think the following are reasonably standard (based on the fact that most enclosure standards recommend but don't require this): 3. Locate LED (for locating the device). Even if you only have an activity LED, this is usually done by flashing the activity LED in a well defined pattern. 4. Fault. this is the least standardised of the lot, but does seem to be present in about every enclosure implementation. All I've done is standardise these four pieces ... the services actually take into account that it might not be possible to do certain of these (like fault). And none of this means that it needs a kernel representation. 1. You're not standardizing any known, in-practice, kernel representation, that is already in practice and thusly needs a kernel representation. 2. The kernel itself is not using nor needing this representation in order to function properly (the kernel). Leaving control of SES devices to user-space makes both the kernel and the vendors happy. All the kernel needs to do is expose the SES device to user-space as it currently does. It makes it so much easier both to vendors and to the kernel to stay out of unnecessary representations. Vendors may choose to distribute their own applications to control their hardware, as long as the kernel exposes an SES device and provides functionality, as opposed to policy of any kind. Luben - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
James Bottomley wrote: I don't disagree with that, but the fact is that there isn't such a tool. It's also a fact that the enterprise is reasonably unhappy with the lack of an enclosure management infrastructure, since it's something they got on all the other unix systems. I don't disagree. I think a minimal infrastructure in-kernel does just about everything the enterprise wants ... and since it's stateless, they can always use direct connect tools in addition. However, I'm happy to be proven wrong ... anyone on this thread is welcome to come up with a userland enclosure infrastructure. Once it does everything the in-kernel one does (which is really about the minimal possible set), I'll be glad to erase the in-kernel one. yeah, but... putting something new in, only to pull it later, is a bad paradigm for adding new mgmt interfaces. Believe me, I've felt users pain in the reverse flow : driver-specific stuff that then has to migrate to upstream interfaces, complicated by different pull points by different distros. You can migrate a management interface, but can you really remove/pull one out ? Isn't it better to let the lack of an interface give motivation to create the right interface, once the right way is determined - which is what I thought we were discussing ?or is this simply that there is no motivation until something exists, that people don't like, thus they become motivated ? -- james s - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Wed, 2008-02-13 at 09:08 -0500, James Smart wrote: The keep-it-in-user-space arguments seem fairly compelling to me. Especially as we've pushed whole i/o subsystems out to user space (iscsi, stgt, talked about fcoe, a lot of dm control, etc). And to me too. The functionality seems to align with Doug's sg/lsscsi utility chain as well. Granted, the new utility would have to be designed in such as way that it can incorporate vendor hardware handlers. But, if James has a somewhat common implementation already for a kernel implementation, I'm sure that can be the starting point for lsscsi. So, the main question I believe is being asked is: - Do we need to represent this via the object/sysfs tree or can an outside utility be depended upon to show it ? Note that I am not supporting: Vendors may choose to distribute their own applications. For this to become truly useful, there needs to be a common tool/method that presents common features in a common manner, regardless of whether it is in kernel or not. I don't disagree with that, but the fact is that there isn't such a tool. It's also a fact that the enterprise is reasonably unhappy with the lack of an enclosure management infrastructure, since it's something they got on all the other unix systems. I think a minimal infrastructure in-kernel does just about everything the enterprise wants ... and since it's stateless, they can always use direct connect tools in addition. However, I'm happy to be proven wrong ... anyone on this thread is welcome to come up with a userland enclosure infrastructure. Once it does everything the in-kernel one does (which is really about the minimal possible set), I'll be glad to erase the in-kernel one. James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
James Bottomley wrote: ... I wouldn't have bothered except that I could see ad-hoc in-kernel sysfs solutions beginning to appear. If this is true, and if no one quickly volunteers to do the utility, then I agree with what you are doing. -- james s - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Wed, 2008-02-13 at 11:22 -0500, James Smart wrote: James Bottomley wrote: I don't disagree with that, but the fact is that there isn't such a tool. It's also a fact that the enterprise is reasonably unhappy with the lack of an enclosure management infrastructure, since it's something they got on all the other unix systems. I don't disagree. I think a minimal infrastructure in-kernel does just about everything the enterprise wants ... and since it's stateless, they can always use direct connect tools in addition. However, I'm happy to be proven wrong ... anyone on this thread is welcome to come up with a userland enclosure infrastructure. Once it does everything the in-kernel one does (which is really about the minimal possible set), I'll be glad to erase the in-kernel one. yeah, but... putting something new in, only to pull it later, is a bad paradigm for adding new mgmt interfaces. Believe me, I've felt users pain in the reverse flow : driver-specific stuff that then has to migrate to upstream interfaces, complicated by different pull points by different distros. You can migrate a management interface, but can you really remove/pull one out ? That depends on the result. I agree that migration will be a pain, so I suppose I set the bar a bit low; the user tool needs to be a bit more compelling; plus I'll manage the interface transition ... if there is one; there's no such tool yet. Isn't it better to let the lack of an interface give motivation to create the right interface, once the right way is determined - which is what I thought we were discussing ?or is this simply that there is no motivation until something exists, that people don't like, thus they become motivated ? Well ... I did learn the latter from Andrew, so I thought I'd try it. It's certainly true that the enclosure problem has been an issue for over a decade, so there doesn't seem to be anything motivating anyone to solve it. I wouldn't have bothered except that I could see ad-hoc in-kernel sysfs solutions beginning to appear. At least this way they can all present a unified interface. James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Wed, 2008-02-13 at 09:45 -0800, Kristen Carlson Accardi wrote: I don't think I'm arguing whether or not your solution may work, what I am arguing is really a more philosophical point. Not can we do it this way, but should we do it way. I am of the opinion that management belongs in userspace. I also am of the opinion that if you can successfully accomplish something in user space, you should. I also believe that even if you provide this basic interface, all system vendors are going to provide libraries on top of that to customize it, so you've not added much value to just a simple message passing interface. I'm not necessarily arguing against that. However, what you're providing is slightly more than just a userspace tap into the enclosure. You're adding a file to display and control the enclosure state (sw_activity). This constitutes an ad-hoc sysfs interface. I'm not telling you not to do it, but I am pleading that if we have to have all these sysfs interfaces, lets at least do it in a uniform way. Enclosures are such nasty beasts, that even the job of getting a tap into them is problematic, so if we have a different tap infrastructure for every different enclosure type and connection it's still going to be pretty unmanageable to a userspace interface. So, I'm happy to defer to Jeff's judgement call here - I just want to do what's right for our customers and get an enclosure management interface for SATA exposed, preferrably in time for the 2.6.26 merge window. If he prefers your design, I'll disagree, but commit to his decision and try to get this to work for SATA. If he'd rather see something along the lines of what I proposed, then since it is 100% self contained in the SATA subsystem, it shouldn't impact whatever you want to do in the SCSI subsystem. James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Tue, 12 Feb 2008 13:28:15 -0600 James Bottomley [EMAIL PROTECTED] wrote: On Tue, 2008-02-12 at 11:07 -0800, Kristen Carlson Accardi wrote: I understand what you are trying to do - I guess I just doubt the value you've added by doing this. I think that there's going to be so much customization that system vendors will want to add, that they are going to wind up adding a custom library regardless, so standardising those few things won't buy us anything. It depends ... if you actually have a use for the customisations, yes. If you just want the basics of who (what's in the enclousure), what (activity) and where (locate) then I think it solves your problem almost entirely. So, entirely as a straw horse, tell me what else your enclosures provide that I haven't listed in the four points. The SES standards too provide a huge range of things that no-one ever seems to implement (temperature, power, fan speeds etc). I think the users of enclosures fall int these categories 85% just want to know where their device actually is (i.e. that sdc is in enclosure slot 5) 50% like watching the activity lights 30% want to be able to have a visual locate function 20% want a visual failure indication (the other 80% rely on some OS notification instead) When you add up the overlapping needs, you get about 90% of people happy with the basics that the enclosure services provide. Could there be more ... sure; should there be more ... I don't think so ... that's what value add the user libraries can provide. James I don't think I'm arguing whether or not your solution may work, what I am arguing is really a more philosophical point. Not can we do it this way, but should we do it way. I am of the opinion that management belongs in userspace. I also am of the opinion that if you can successfully accomplish something in user space, you should. I also believe that even if you provide this basic interface, all system vendors are going to provide libraries on top of that to customize it, so you've not added much value to just a simple message passing interface. So, I'm happy to defer to Jeff's judgement call here - I just want to do what's right for our customers and get an enclosure management interface for SATA exposed, preferrably in time for the 2.6.26 merge window. If he prefers your design, I'll disagree, but commit to his decision and try to get this to work for SATA. If he'd rather see something along the lines of what I proposed, then since it is 100% self contained in the SATA subsystem, it shouldn't impact whatever you want to do in the SCSI subsystem. Jeff? - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Tue, 2008-02-12 at 11:07 -0800, Kristen Carlson Accardi wrote: I understand what you are trying to do - I guess I just doubt the value you've added by doing this. I think that there's going to be so much customization that system vendors will want to add, that they are going to wind up adding a custom library regardless, so standardising those few things won't buy us anything. It depends ... if you actually have a use for the customisations, yes. If you just want the basics of who (what's in the enclousure), what (activity) and where (locate) then I think it solves your problem almost entirely. So, entirely as a straw horse, tell me what else your enclosures provide that I haven't listed in the four points. The SES standards too provide a huge range of things that no-one ever seems to implement (temperature, power, fan speeds etc). I think the users of enclosures fall int these categories 85% just want to know where their device actually is (i.e. that sdc is in enclosure slot 5) 50% like watching the activity lights 30% want to be able to have a visual locate function 20% want a visual failure indication (the other 80% rely on some OS notification instead) When you add up the overlapping needs, you get about 90% of people happy with the basics that the enclosure services provide. Could there be more ... sure; should there be more ... I don't think so ... that's what value add the user libraries can provide. James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Tue, 12 Feb 2008 12:45:35 -0600 James Bottomley [EMAIL PROTECTED] wrote: On Tue, 2008-02-12 at 10:22 -0800, Kristen Carlson Accardi wrote: I apologize for taking so long to review this patch. I obviously agree wholeheartedly with Luben. The problem I ran into while trying to design an enclosure management interface for the SATA devices is that there is all this vendor defined stuff. For example, for the AHCI LED protocol, the only defined LED is 'activity'. For LED2 and LED3 it is up to hardware vendors to define these. For SGPIO there's all kinds of ways for hw vendors to customize. I felt that it was going to be a maintainance nightmare to have to keep track of various vendors enclosure implementations in the ahci driver, and that it'd be better to just have user space libraries take care of that. Plus, that way a vendor doesn't have to get a patch into the kernel to get their new spiffy wizzy bang blinky lights working (think of how long it takes something to even get into a vendor kernel, which is what these guys care about...). So I'm still not sold on having an enclosure abstraction in the kernel - at least for the SATA controllers. Correct me if I'm wrong, but didn't the original AHCI enclosure patch expose activity LEDs via sysfs? You are sort of wrong. we exposed a sysfs entry to enable sofware controlled activity LED, then the driver was responsible for turning it on and off. (blech, I know, but some vendors want this feature). I'm not saying there aren't a lot of non standard pieces that need to be activated by direct commands or other user activated protocol. I am saying there are a lot of standard pieces that we could do with showing in a uniform manner. The pieces I think are absolutely standard are 1. Actual enclosure presence (is this device in an enclosure) 2. Activity LED, this seems to be a feature of every enclosure. I also think the following are reasonably standard (based on the fact that most enclosure standards recommend but don't require this): 3. Locate LED (for locating the device). Even if you only have an activity LED, this is usually done by flashing the activity LED in a well defined pattern. 4. Fault. this is the least standardised of the lot, but does seem to be present in about every enclosure implementation. All I've done is standardise these four pieces ... the services actually take into account that it might not be possible to do certain of these (like fault). James I understand what you are trying to do - I guess I just doubt the value you've added by doing this. I think that there's going to be so much customization that system vendors will want to add, that they are going to wind up adding a custom library regardless, so standardising those few things won't buy us anything. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Tue, 2008-02-12 at 10:22 -0800, Kristen Carlson Accardi wrote: I apologize for taking so long to review this patch. I obviously agree wholeheartedly with Luben. The problem I ran into while trying to design an enclosure management interface for the SATA devices is that there is all this vendor defined stuff. For example, for the AHCI LED protocol, the only defined LED is 'activity'. For LED2 and LED3 it is up to hardware vendors to define these. For SGPIO there's all kinds of ways for hw vendors to customize. I felt that it was going to be a maintainance nightmare to have to keep track of various vendors enclosure implementations in the ahci driver, and that it'd be better to just have user space libraries take care of that. Plus, that way a vendor doesn't have to get a patch into the kernel to get their new spiffy wizzy bang blinky lights working (think of how long it takes something to even get into a vendor kernel, which is what these guys care about...). So I'm still not sold on having an enclosure abstraction in the kernel - at least for the SATA controllers. Correct me if I'm wrong, but didn't the original AHCI enclosure patch expose activity LEDs via sysfs? I'm not saying there aren't a lot of non standard pieces that need to be activated by direct commands or other user activated protocol. I am saying there are a lot of standard pieces that we could do with showing in a uniform manner. The pieces I think are absolutely standard are 1. Actual enclosure presence (is this device in an enclosure) 2. Activity LED, this seems to be a feature of every enclosure. I also think the following are reasonably standard (based on the fact that most enclosure standards recommend but don't require this): 3. Locate LED (for locating the device). Even if you only have an activity LED, this is usually done by flashing the activity LED in a well defined pattern. 4. Fault. this is the least standardised of the lot, but does seem to be present in about every enclosure implementation. All I've done is standardise these four pieces ... the services actually take into account that it might not be possible to do certain of these (like fault). James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Mon, 4 Feb 2008 18:01:36 -0800 (PST) Luben Tuikov [EMAIL PROTECTED] wrote: --- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: The enclosure misc device is really just a library providing sysfs support for physical enclosure devices and their components. Who is the target audience/user of those facilities? a) The kernel itself needing to read/write SES pages? That depends on the enclosure integration, but right at the moment, it doesn't Yes, I didn't suspect so. b) A user space application using sysfs to read/write SES pages? Not an application so much as a user. The idea of sysfs is to allow users to get and set the information in addition to applications. Exactly the same argument stands for a user-space application with a user-space library. This is the classical case of where it is better to do this in user-space as opposed to the kernel. The kernel provides capability to access the SES device. The user space application and library provide interpretation and control. Thus if the enclosure were upgraded, one doesn't need to upgrade their kernel in order to utilize the new capabilities of the SES device. Plus upgrading a user-space application is a lot easier than the kernel (and no reboot necessary). Consider another thing: vendors would really like unprecedented access to the SES device in the enclosure so as your ses/enclosure code keeps state it would get out of sync when vendor user-space enclosure applications access (and modify) the SES device's pages. You can test this yourself: submit a patch that removes SES /dev/sgX support; advertise your ses/class solution and watch the fun. At the moment SES device management is done via an application (user-space) and a user-space library used by the application and /dev/sgX to send SCSI commands to the SES device. I must have missed that when I was looking for implementations; what's the URL? I'm not aware of any GPLed ones. That doesn't necessarily mean that the best course of action is to bloat the kernel. You can move your ses/enclosure stuff to a user space application library and thus start a GPLed one. But, if we have non-scsi enclosures to integrate, that makes it harder for a user application because it has to know all the implementations. So does the kernel. And as I pointed out above, it is a lot easier to upgrade a user-space application and library than it is to upgrade a new kernel and having to reboot the computer to run the new kernel. A sysfs framework on the other hand is a universal known thing for the user applications. So would a user-space ses library, a la libses.so. One could have a very good argument to not bloat the kernel with this but leave it to a user-space application and a library to do all this and communicate with the SES device via the kernel's /dev/sgX. The same thing goes for other esoteric SCSI infrastructure pieces like cd changers. On the whole, given that ATA is asking for enclosure management in kernel, it makes sense to consolidate the infrastructure and a ses ULD is a very good test bed. What is wrong with exporting the SES device as /dev/sgX and having a user-space application and library to do all this? Luben Hi, I apologize for taking so long to review this patch. I obviously agree wholeheartedly with Luben. The problem I ran into while trying to design an enclosure management interface for the SATA devices is that there is all this vendor defined stuff. For example, for the AHCI LED protocol, the only defined LED is 'activity'. For LED2 and LED3 it is up to hardware vendors to define these. For SGPIO there's all kinds of ways for hw vendors to customize. I felt that it was going to be a maintainance nightmare to have to keep track of various vendors enclosure implementations in the ahci driver, and that it'd be better to just have user space libraries take care of that. Plus, that way a vendor doesn't have to get a patch into the kernel to get their new spiffy wizzy bang blinky lights working (think of how long it takes something to even get into a vendor kernel, which is what these guys care about...). So I'm still not sold on having an enclosure abstraction in the kernel - at least for the SATA controllers. Kristen - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
--- On Tue, 2/12/08, Kristen Carlson Accardi [EMAIL PROTECTED] wrote: Hi, I apologize for taking so long to review this patch. I obviously agree wholeheartedly with Luben. The problem I ran into while trying to design an enclosure management interface for the SATA devices is that there is all this vendor defined stuff. For example, for the AHCI LED protocol, the only defined LED is 'activity'. For LED2 and LED3 it is up to hardware vendors to define these. For SGPIO there's all kinds of ways for hw vendors to customize. I felt that it was going to be a maintainance nightmare to have to keep track of various vendors enclosure implementations in the ahci driver, and that it'd be better to just have user space libraries take care of that. Plus, that way a vendor doesn't have to get a patch into the kernel to get their new spiffy wizzy bang blinky lights working (think of how long it takes something to even get into a vendor kernel, which is what these guys care about...). So I'm still not sold on having an enclosure abstraction in the kernel - at least for the SATA controllers. And I agree wholeheartedly with Kristen. Luben - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Mon, 2008-02-04 at 21:35 -0800, Luben Tuikov wrote: I guess the same could be said for STGT and SCST, right? You mean both of their kernel pieces are modular? That's correct. No, you know very well what I mean. By the same logic you're preaching to include your solution part of the kernel, you can also apply to SCST. Ah, but it's not ... the current patch is merely exporting an interface. The debate in STGT vs SCST is not whether to export an interface but where to draw the line. You could also argue in the same vein that sd is redundant because a filesystem could talk directly to the device via /dev/sgX (in fact OSD based filesystems already do this). The argument is true, but misses the bigger picture that the interfaces exported by sd are more portable (apply to non-SCSI block devices) and easier to use. Yes, for which the transport layer, implements the scsi device node for the SES device. It doesn't really matter if the SCSI commands sent to the SES device go over SGPIO or FC or SAS or Bluetooth or I2C, etc, the transport layer can implement that and present the /dev/sgX node. But it does matter if the enclosure device doesn't speak SCSI. Enclosure management isn't as simple as you're portraying it here. The enclosure management device speaks either SES or SAF-TE. The transport protocol to access it could be SGPIO or I2C or... Look, just read the spec; SGPIO is a bus for driving enclosures ... it doesn't require SES or SAF-TE or even any SCSI protocol. SGPIO isn't a SCSI protocol ... it's a general purpose serial bus protocol. It's pretty simple and register based, but it might (or might not) be accessible via a SCSI bridge. I see. You've just discovered SGPIO -- good for you. At any rate, I told you already that what is needed is not what you've provided but a _device node_ exported by the kernel, either a processor or enclosure type. Wrong ... we don't export non-SCSI devices as SCSI (with the single and rather annoying exception of ATA via SAT). James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
--- On Tue, 2/5/08, James Bottomley [EMAIL PROTECTED] wrote: I guess the same could be said for STGT and SCST, right? You mean both of their kernel pieces are modular? That's correct. No, you know very well what I mean. By the same logic you're preaching to include your solution part of the kernel, you can also apply to SCST. Ah, but it's not ... the current patch is merely exporting an interface. The debate in STGT vs SCST is not whether to export an interface but where to draw the line. draw the line -- I see. BTW, what is wrong with exporting the interface? What is wrong if both implementations are in the kernel and then let the users and distros decide which one they like best and use more? It'll not be the fist time this has happened in the kernel. Both are actively maintained. It seems highly arbitrary to say: X is in the kernel, Y is not. If you want Y, just forget about it and fix X. Give people choice at config time. This is off topic anyway. You could also argue in the same vein that sd is redundant because a filesystem could talk directly to the device via /dev/sgX (in fact OSD based filesystems already do this). Yes, I've mentioned this thing before on this list. Oh, maybe 3 years ago. This is why I had wanted for transport protocols to export ... (oh, let's not get this off topic). (Apparently it takes 3 years...) The argument is true, but misses the bigger picture that the interfaces exported by sd are more portable (apply to non-SCSI block devices) and easier to use. It isn't quite the same thing. It's like comparing apples to oranges. Yes, for which the transport layer, implements the scsi device node for the SES device. It doesn't really matter if the SCSI commands sent to the SES device go over SGPIO or FC or SAS or Bluetooth or I2C, etc, the transport layer can implement that and present the /dev/sgX node. But it does matter if the enclosure device doesn't speak SCSI. Enclosure management isn't as simple as you're portraying it here. The enclosure management device speaks either SES or SAF-TE. The transport protocol to access it could be SGPIO or I2C or... Look, just read the spec; SGPIO is a bus for driving enclosures ... I thought Serial General Purpose Input Output (SGPIO) was a method to serialize general purpose IO signals. it doesn't require SES or SAF-TE or even any SCSI protocol. That's true. And this is why I mentioned a couple of emails ago to simply export a sgpio device node *IF* this is what is needed. Of course devices that use SGPIO abstract it away for their functional purpose, e.g. enclosures, LED, etc, and provide a more general way to control it -- highly hardware specific on one side. Your abstraction currently deals with SES devices and I'd rather leave that to user-space. Alternatively, which I presume is what you're thinking, a HW specific core would be using your abstraction to provide some unified access to raw features, and that unified access isn't defined anywhere, and would likely not be. Alternatively that unified access is things like SES and SAF-TE, which is what vendors prefer to export, or they prefer to drive this directly via other means. That is, I fail to see the kernel bloat, for things that aren't necessary in the kernel. If you want your abstraction to fly, it first needs a common usage model to abstract, and the latter is missing _from the kernel_. Unless I don't know the details and you've been asked to implement this for a single vendor's HW solution. SGPIO isn't a SCSI protocol ... it's a general purpose serial bus protocol. It's pretty simple and register based, but it might (or might not) be accessible via a SCSI bridge. I see. You've just discovered SGPIO -- good for you. At any rate, I told you already that what is needed is not what you've provided but a _device node_ exported by the kernel, either a processor or enclosure type. Wrong ... we don't export non-SCSI devices as SCSI (with the single and rather annoying exception of ATA via SAT). I didn't say you should do that. I had already mentioned that vendors export such controls as either enclosure or processor type devices, and this is why I told you that that is what needs to be exported, which incidentally is a device node of that type. Without a common usage model already in the kernel to abstract (e.g. sd for block device, since you brought that up) your abstraction seems redundant and arbitrary. Your kernel code already uses READ DIAGNOSTIC, etc, and I'd rather leave that to user-space. Luben - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Tue, 2008-02-05 at 11:33 -0800, Luben Tuikov wrote: Wrong ... we don't export non-SCSI devices as SCSI (with the single and rather annoying exception of ATA via SAT). I didn't say you should do that. I had already mentioned that vendors export such controls as either enclosure or processor type devices, and this is why I told you that that is what needs to be exported, which incidentally is a device node of that type. Without a common usage model already in the kernel to abstract (e.g. sd for block device, since you brought that up) your abstraction seems redundant and arbitrary. Exactly, so the first patch in this series (a while ago now) was a common usage model abstraction of enclosures, and the second was an implementation in terms of SES. I will do one in terms of SGPIO as well ... assuming I ever find a SGPIO enclosure ... Your kernel code already uses READ DIAGNOSTIC, etc, and I'd rather leave that to user-space. You can do it in user space as well. It's just a bit difficult to get information out of a SES enclosure without using it, and getting some of the information is a requirement of the abstraction. James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
--- On Tue, 2/5/08, James Bottomley [EMAIL PROTECTED] wrote: Wrong ... we don't export non-SCSI devices as SCSI (with the single and rather annoying exception of ATA via SAT). I didn't say you should do that. I had already mentioned that vendors export such controls as either enclosure or processor type devices, and this is why I told you that that is what needs to be exported, which incidentally is a device node of that type. Without a common usage model already in the kernel to abstract (e.g. sd for block device, since you brought that up) your abstraction seems redundant and arbitrary. Exactly, so the first patch in this series (a while ago ^^^ See last paragraph. now) was a common usage model abstraction of enclosures, and the second was an implementation in terms of SES. I will do one in terms of SGPIO as well ... assuming I ever find a SGPIO enclosure ... The vendor would've abstracted that away most commonly using SES. Your kernel code already uses READ DIAGNOSTIC, etc, and I'd rather leave that to user-space. You can do it in user space as well. It's just a bit difficult to get information out of a SES enclosure without using it, and getting some of the information is a requirement of the abstraction. You missed my point. Your abstraction is redundant and arbitrary -- it is not based on any known, in-practice, usage model, already in place that needs a better, common way of doing XYZ, and therefore needs an abstraction. Luben - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Sun, 03 Feb 2008 18:16:51 -0600
James Bottomley [EMAIL PROTECTED] wrote:
From: James Bottomley [EMAIL PROTECTED]
Date: Sun, 3 Feb 2008 15:40:56 -0600
Subject: [SCSI] enclosure: add support for enclosure services
The enclosure misc device is really just a library providing sysfs
support for physical enclosure devices and their components.
Thanks for sending it out for review.
+struct enclosure_device *enclosure_find(struct device *dev)
+{
+ struct enclosure_device *edev = NULL;
+
+ mutex_lock(container_list_lock);
+ list_for_each_entry(edev, container_list, node) {
+ if (edev-cdev.dev == dev) {
+ mutex_unlock(container_list_lock);
+ return edev;
+ }
+ }
+ mutex_unlock(container_list_lock);
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(enclosure_find);
This looks a little odd. We don't take a ref on the object after looking
it up, so what prevents some other thread of control from freeing or
otherwise altering the returned object while the caller is playing with it?
+/**
+ * enclosure_for_each_device - calls a function for each enclosure
+ * @fn: the function to call
+ * @data:the data to pass to each call
+ *
+ * Loops over all the enclosures calling the function.
+ *
+ * Note, this function uses a mutex which will be held across calls to
+ * @fn, so it must have user context, and @fn should not sleep or
Probably non atomic context would be more accurate.
fn() actually _can_ sleep.
+ * otherwise cause the mutex to be held for indefinite periods
+ */
+int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
+ void *data)
+{
+ int error = 0;
+ struct enclosure_device *edev;
+
+ mutex_lock(container_list_lock);
+ list_for_each_entry(edev, container_list, node) {
+ error = fn(edev, data);
+ if (error)
+ break;
+ }
+ mutex_unlock(container_list_lock);
+
+ return error;
+}
+EXPORT_SYMBOL_GPL(enclosure_for_each_device);
+
+/**
+ * enclosure_register - register device as an enclosure
+ *
+ * @dev: device containing the enclosure
+ * @components: number of components in the enclosure
+ *
+ * This sets up the device for being an enclosure. Note that @dev does
+ * not have to be a dedicated enclosure device. It may be some other type
+ * of device that additionally responds to enclosure services
+ */
+struct enclosure_device *
+enclosure_register(struct device *dev, const char *name, int components,
+struct enclosure_component_callbacks *cb)
+{
+ struct enclosure_device *edev =
+ kzalloc(sizeof(struct enclosure_device) +
+ sizeof(struct enclosure_component)*components,
+ GFP_KERNEL);
+ int err, i;
+
+ if (!edev)
+ return ERR_PTR(-ENOMEM);
+
+ if (!cb) {
+ kfree(edev);
+ return ERR_PTR(-EINVAL);
+ }
It would be less fuss if this were to test cb before doing the kzalloc().
Can cb==NULL actually and legitimately happen?
+ edev-components = components;
+
+ edev-cdev.class = enclosure_class;
+ edev-cdev.dev = get_device(dev);
+ edev-cb = cb;
+ snprintf(edev-cdev.class_id, BUS_ID_SIZE, %s, name);
+ err = class_device_register(edev-cdev);
+ if (err)
+ goto err;
+
+ for (i = 0; i components; i++)
+ edev-component[i].number = -1;
+
+ mutex_lock(container_list_lock);
+ list_add_tail(edev-node, container_list);
+ mutex_unlock(container_list_lock);
+
+ return edev;
+
+ err:
+ put_device(edev-cdev.dev);
+ kfree(edev);
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(enclosure_register);
+
+static struct enclosure_component_callbacks enclosure_null_callbacks;
+
+/**
+ * enclosure_unregister - remove an enclosure
+ *
+ * @edev:the registered enclosure to remove;
+ */
+void enclosure_unregister(struct enclosure_device *edev)
+{
+ int i;
+
+ if (!edev)
+ return;
Is this legal?
+ mutex_lock(container_list_lock);
+ list_del(edev-node);
+ mutex_unlock(container_list_lock);
See, right now, someone who found this enclosure_device via
enclosure_find() could still be playing with it?
+ for (i = 0; i edev-components; i++)
+ if (edev-component[i].number != -1)
+ class_device_unregister(edev-component[i].cdev);
+
+ /* prevent any callbacks into service user */
+ edev-cb = enclosure_null_callbacks;
+ class_device_unregister(edev-cdev);
+}
+EXPORT_SYMBOL_GPL(enclosure_unregister);
+
+/**
+ * enclosure_component_register - add a particular component to an enclosure
+ * @edev:the enclosure to add the component
+ * @num: the device number
+ * @type:the type of component
Re: [PATCH] enclosure: add support for enclosure services
On Tue, 2008-02-05 at 16:12 -0800, Andrew Morton wrote:
On Sun, 03 Feb 2008 18:16:51 -0600
James Bottomley [EMAIL PROTECTED] wrote:
From: James Bottomley [EMAIL PROTECTED]
Date: Sun, 3 Feb 2008 15:40:56 -0600
Subject: [SCSI] enclosure: add support for enclosure services
The enclosure misc device is really just a library providing sysfs
support for physical enclosure devices and their components.
Thanks for sending it out for review.
+struct enclosure_device *enclosure_find(struct device *dev)
+{
+ struct enclosure_device *edev = NULL;
+
+ mutex_lock(container_list_lock);
+ list_for_each_entry(edev, container_list, node) {
+ if (edev-cdev.dev == dev) {
+ mutex_unlock(container_list_lock);
+ return edev;
+ }
+ }
+ mutex_unlock(container_list_lock);
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(enclosure_find);
This looks a little odd. We don't take a ref on the object after looking
it up, so what prevents some other thread of control from freeing or
otherwise altering the returned object while the caller is playing with it?
The use case is for enclosure destruction, so the free should never
happen, but I take the point; I've added a class_device_get().
+/**
+ * enclosure_for_each_device - calls a function for each enclosure
+ * @fn:the function to call
+ * @data: the data to pass to each call
+ *
+ * Loops over all the enclosures calling the function.
+ *
+ * Note, this function uses a mutex which will be held across calls to
+ * @fn, so it must have user context, and @fn should not sleep or
Probably non atomic context would be more accurate.
fn() actually _can_ sleep.
should to me means you don't have to do this but ought to. I'll add a
may (but should not).
+ if (!cb) {
+ kfree(edev);
+ return ERR_PTR(-EINVAL);
+ }
It would be less fuss if this were to test cb before doing the kzalloc().
Can cb==NULL actually and legitimately happen?
Not really ... I'll make it a BUG_ON.
+void enclosure_unregister(struct enclosure_device *edev)
+{
+ int i;
+
+ if (!edev)
+ return;
Is this legal?
No ... it'll oops on the null deref later ... I'll remove this.
+ mutex_lock(container_list_lock);
+ list_del(edev-node);
+ mutex_unlock(container_list_lock);
See, right now, someone who found this enclosure_device via
enclosure_find() could still be playing with it?
Yes, fixed.
+ if (!edev || number = edev-components)
+ return ERR_PTR(-EINVAL);
Is !edev possible and legitimate?
It shouldn't be, no ... I can remove it.
+ snprintf(cdev-class_id, BUS_ID_SIZE, %d, number);
%u :)
Nitpicker!
+ return snprintf(buf, 40, %d\n, edev-components);
+}
40?
I just followed precedence ;-P
There doesn't seem to be a define for this maximum length, so 40 is the
most commonly picked constant.
+static char *enclosure_type [] = {
+ [ENCLOSURE_COMPONENT_DEVICE] = device,
+ [ENCLOSURE_COMPONENT_ARRAY_DEVICE] = array device,
+};
One could play with const here, if sufficiently keen.
One will try to summon up the enthusiasm.
+static ssize_t set_component_fault(struct class_device *cdev, const char
*buf,
+ size_t count)
+{
+ struct enclosure_device *edev = to_enclosure_device(cdev-parent);
+ struct enclosure_component *ecomp = to_enclosure_component(cdev);
+ int val = simple_strtoul(buf, NULL, 0);
hrm, we do this conversion about 1e99 times in the kernel and we have to go
and pass three args where only one was needed. katoi()?
Yes ... I'll add it to the todo list.
+ for (i = 0; enclosure_status[i]; i++) {
+ if (strncmp(buf, enclosure_status[i],
+ strlen(enclosure_status[i])) == 0
+ buf[strlen(enclosure_status[i])] == '\n')
+ break;
+ }
So if an application does
write(fd, foo, 3)
it won't work? Thye have to do
write(fd, foo\n, 4)
?
No ... it's designed for echo; however, I'll add a check for '\0' which
will catch the write case.
+#define to_enclosure_device(x) container_of((x), struct enclosure_device,
cdev)
+#define to_enclosure_component(x) container_of((x), struct
enclosure_component, cdev)
These could be C functions...
OK ... I was just following precedence again, but I can make them
inlines.
Nice looking driver.
Thanks,
James
---
Here's the incremental diff.
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 42e6e43..6fcb0e9 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -39,7 +39,8 @@ static struct class enclosure_component_class;
*
* Looks through the list of registered enclosures to see
* if it can find a match for a device. Returns NULL if no
- * enclosure is found.
+ * enclosure is found.
Re: [PATCH] enclosure: add support for enclosure services
--- On Sun, 2/3/08, James Bottomley [EMAIL PROTECTED] wrote: The enclosure misc device is really just a library providing sysfs support for physical enclosure devices and their components. Who is the target audience/user of those facilities? a) The kernel itself needing to read/write SES pages? b) A user space application using sysfs to read/write SES pages? At the moment SES device management is done via an application (user-space) and a user-space library used by the application and /dev/sgX to send SCSI commands to the SES device. One could have a very good argument to not bloat the kernel with this but leave it to a user-space application and a library to do all this and communicate with the SES device via the kernel's /dev/sgX. Luben - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Mon, 2008-02-04 at 16:32 -0800, Luben Tuikov wrote: --- On Sun, 2/3/08, James Bottomley [EMAIL PROTECTED] wrote: The enclosure misc device is really just a library providing sysfs support for physical enclosure devices and their components. Who is the target audience/user of those facilities? a) The kernel itself needing to read/write SES pages? That depends on the enclosure integration, but right at the moment, it doesn't b) A user space application using sysfs to read/write SES pages? Not an application so much as a user. The idea of sysfs is to allow users to get and set the information in addition to applications. At the moment SES device management is done via an application (user-space) and a user-space library used by the application and /dev/sgX to send SCSI commands to the SES device. I must have missed that when I was looking for implementations; what's the URL? But, if we have non-scsi enclosures to integrate, that makes it harder for a user application because it has to know all the implementations. A sysfs framework on the other hand is a universal known thing for the user applications. One could have a very good argument to not bloat the kernel with this but leave it to a user-space application and a library to do all this and communicate with the SES device via the kernel's /dev/sgX. The same thing goes for other esoteric SCSI infrastructure pieces like cd changers. On the whole, given that ATA is asking for enclosure management in kernel, it makes sense to consolidate the infrastructure and a ses ULD is a very good test bed. James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
--- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: The enclosure misc device is really just a library providing sysfs support for physical enclosure devices and their components. Who is the target audience/user of those facilities? a) The kernel itself needing to read/write SES pages? That depends on the enclosure integration, but right at the moment, it doesn't Yes, I didn't suspect so. b) A user space application using sysfs to read/write SES pages? Not an application so much as a user. The idea of sysfs is to allow users to get and set the information in addition to applications. Exactly the same argument stands for a user-space application with a user-space library. This is the classical case of where it is better to do this in user-space as opposed to the kernel. The kernel provides capability to access the SES device. The user space application and library provide interpretation and control. Thus if the enclosure were upgraded, one doesn't need to upgrade their kernel in order to utilize the new capabilities of the SES device. Plus upgrading a user-space application is a lot easier than the kernel (and no reboot necessary). Consider another thing: vendors would really like unprecedented access to the SES device in the enclosure so as your ses/enclosure code keeps state it would get out of sync when vendor user-space enclosure applications access (and modify) the SES device's pages. You can test this yourself: submit a patch that removes SES /dev/sgX support; advertise your ses/class solution and watch the fun. At the moment SES device management is done via an application (user-space) and a user-space library used by the application and /dev/sgX to send SCSI commands to the SES device. I must have missed that when I was looking for implementations; what's the URL? I'm not aware of any GPLed ones. That doesn't necessarily mean that the best course of action is to bloat the kernel. You can move your ses/enclosure stuff to a user space application library and thus start a GPLed one. But, if we have non-scsi enclosures to integrate, that makes it harder for a user application because it has to know all the implementations. So does the kernel. And as I pointed out above, it is a lot easier to upgrade a user-space application and library than it is to upgrade a new kernel and having to reboot the computer to run the new kernel. A sysfs framework on the other hand is a universal known thing for the user applications. So would a user-space ses library, a la libses.so. One could have a very good argument to not bloat the kernel with this but leave it to a user-space application and a library to do all this and communicate with the SES device via the kernel's /dev/sgX. The same thing goes for other esoteric SCSI infrastructure pieces like cd changers. On the whole, given that ATA is asking for enclosure management in kernel, it makes sense to consolidate the infrastructure and a ses ULD is a very good test bed. What is wrong with exporting the SES device as /dev/sgX and having a user-space application and library to do all this? Luben - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
On Mon, 2008-02-04 at 18:01 -0800, Luben Tuikov wrote: --- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: The enclosure misc device is really just a library providing sysfs support for physical enclosure devices and their components. Who is the target audience/user of those facilities? a) The kernel itself needing to read/write SES pages? That depends on the enclosure integration, but right at the moment, it doesn't Yes, I didn't suspect so. b) A user space application using sysfs to read/write SES pages? Not an application so much as a user. The idea of sysfs is to allow users to get and set the information in addition to applications. Exactly the same argument stands for a user-space application with a user-space library. This is the classical case of where it is better to do this in user-space as opposed to the kernel. The kernel provides capability to access the SES device. The user space application and library provide interpretation and control. Thus if the enclosure were upgraded, one doesn't need to upgrade their kernel in order to utilize the new capabilities of the SES device. Plus upgrading a user-space application is a lot easier than the kernel (and no reboot necessary). The implementation is modular, so it's remove and insert ... Consider another thing: vendors would really like unprecedented access to the SES device in the enclosure so as your ses/enclosure code keeps state it would get out of sync when vendor user-space enclosure applications access (and modify) the SES device's pages. The state model doesn't assume nothing else will alter the state. You can test this yourself: submit a patch that removes SES /dev/sgX support; advertise your ses/class solution and watch the fun. At the moment SES device management is done via an application (user-space) and a user-space library used by the application and /dev/sgX to send SCSI commands to the SES device. I must have missed that when I was looking for implementations; what's the URL? I'm not aware of any GPLed ones. That doesn't necessarily mean that the best course of action is to bloat the kernel. You can move your ses/enclosure stuff to a user space application library and thus start a GPLed one. Certainly ... patches welcome. But, if we have non-scsi enclosures to integrate, that makes it harder for a user application because it has to know all the implementations. So does the kernel. And as I pointed out above, it is a lot easier to upgrade a user-space application and library than it is to upgrade a new kernel and having to reboot the computer to run the new kernel. No, think again ... it's easy for SES based enclosures because they have a SCSI transport. We have no transport for SGPIO based enclosures nor for any of the other more esoteric ones. That's not to say it can't be done, but it does mean that it can't be completely userspace. A sysfs framework on the other hand is a universal known thing for the user applications. So would a user-space ses library, a la libses.so. One could have a very good argument to not bloat the kernel with this but leave it to a user-space application and a library to do all this and communicate with the SES device via the kernel's /dev/sgX. The same thing goes for other esoteric SCSI infrastructure pieces like cd changers. On the whole, given that ATA is asking for enclosure management in kernel, it makes sense to consolidate the infrastructure and a ses ULD is a very good test bed. What is wrong with exporting the SES device as /dev/sgX and having a user-space application and library to do all this? How do you transport the enclosure commands over /dev/sgX? Only SES has SCSI command encapsulation ... the rest won't even be SCSI targets ... James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] enclosure: add support for enclosure services
--- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: On Mon, 2008-02-04 at 18:01 -0800, Luben Tuikov wrote: --- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: The enclosure misc device is really just a library providing sysfs support for physical enclosure devices and their components. Who is the target audience/user of those facilities? a) The kernel itself needing to read/write SES pages? That depends on the enclosure integration, but right at the moment, it doesn't Yes, I didn't suspect so. b) A user space application using sysfs to read/write SES pages? Not an application so much as a user. The idea of sysfs is to allow users to get and set the information in addition to applications. Exactly the same argument stands for a user-space application with a user-space library. This is the classical case of where it is better to do this in user-space as opposed to the kernel. The kernel provides capability to access the SES device. The user space application and library provide interpretation and control. Thus if the enclosure were upgraded, one doesn't need to upgrade their kernel in order to utilize the new capabilities of the SES device. Plus upgrading a user-space application is a lot easier than the kernel (and no reboot necessary). The implementation is modular, so it's remove and insert ... I guess the same could be said for STGT and SCST, right? LOL, no seriously, this is unnecessary kernel bloat, or rather at the wrong place (see below). Consider another thing: vendors would really like unprecedented access to the SES device in the enclosure so as your ses/enclosure code keeps state it would get out of sync when vendor user-space enclosure applications access (and modify) the SES device's pages. The state model doesn't assume nothing else will alter the state. But it would be trivial exercise to show that an inconsistent state can be had by modifying pages of the SES device directly from userspace bypassing your implementation. You can test this yourself: submit a patch that removes SES /dev/sgX support; advertise your ses/class solution and watch the fun. At the moment SES device management is done via an application (user-space) and a user-space library used by the application and /dev/sgX to send SCSI commands to the SES device. I must have missed that when I was looking for implementations; what's the URL? I'm not aware of any GPLed ones. That doesn't necessarily mean that the best course of action is to bloat the kernel. You can move your ses/enclosure stuff to a user space application library and thus start a GPLed one. Certainly ... patches welcome. I've non at the moment, plus I don't think you'd be the point of contact for a user-space SES library. Unless of course you've already started something up on sourceforge. Really, such an effort already exists: it is called sg_ses(8). But, if we have non-scsi enclosures to integrate, that makes it harder for a user application because it has to know all the implementations. So does the kernel. And as I pointed out above, it is a lot easier to upgrade a user-space application and library than it is to upgrade a new kernel and having to reboot the computer to run the new kernel. No, think again ... it's easy for SES based enclosures because they have a SCSI transport. We have no transport for SGPIO based enclosures nor for any of the other more esoteric ones. Yes, for which the transport layer, implements the scsi device node for the SES device. It doesn't really matter if the SCSI commands sent to the SES device go over SGPIO or FC or SAS or Bluetooth or I2C, etc, the transport layer can implement that and present the /dev/sgX node. Case in point: the protocol FW running on the ASIC provides this capability so really the LLDD would only see a the pure SCSI SES or processor device and register that with the kernel. At which point no new kernel bloat is required. Your code doesn't quite do that at the moment as it actually goes further in to read and present SES pages. Ideally it would simply provide capability for transport layers to register a SCSI device of type SES, or processor. Architecturally, the LLDD/transport layer would register the SGPIO device on one end with the SGPIO layer and on the other end as a SCSI SES/processpr device. After that sg_ses(8) or sglib, fits the bill for user space applications. That's not to say it can't be done, but it does mean that it can't be completely userspace. See previous paragraph. A sysfs framework on the other hand is a universal known thing for the user applications. So would a user-space ses library, a la libses.so. One could have a very good argument to not bloat the kernel with
Re: [PATCH] enclosure: add support for enclosure services
On Mon, 2008-02-04 at 19:28 -0800, Luben Tuikov wrote: --- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: On Mon, 2008-02-04 at 18:01 -0800, Luben Tuikov wrote: --- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: The enclosure misc device is really just a library providing sysfs support for physical enclosure devices and their components. Who is the target audience/user of those facilities? a) The kernel itself needing to read/write SES pages? That depends on the enclosure integration, but right at the moment, it doesn't Yes, I didn't suspect so. b) A user space application using sysfs to read/write SES pages? Not an application so much as a user. The idea of sysfs is to allow users to get and set the information in addition to applications. Exactly the same argument stands for a user-space application with a user-space library. This is the classical case of where it is better to do this in user-space as opposed to the kernel. The kernel provides capability to access the SES device. The user space application and library provide interpretation and control. Thus if the enclosure were upgraded, one doesn't need to upgrade their kernel in order to utilize the new capabilities of the SES device. Plus upgrading a user-space application is a lot easier than the kernel (and no reboot necessary). The implementation is modular, so it's remove and insert ... I guess the same could be said for STGT and SCST, right? You mean both of their kernel pieces are modular? That's correct. LOL, no seriously, this is unnecessary kernel bloat, or rather at the wrong place (see below). Consider another thing: vendors would really like unprecedented access to the SES device in the enclosure so as your ses/enclosure code keeps state it would get out of sync when vendor user-space enclosure applications access (and modify) the SES device's pages. The state model doesn't assume nothing else will alter the state. But it would be trivial exercise to show that an inconsistent state can be had by modifying pages of the SES device directly from userspace bypassing your implementation. I don't think so ... if you actually look at the code, you'll see it doesn't really have persistent state for the enclosure. You can test this yourself: submit a patch that removes SES /dev/sgX support; advertise your ses/class solution and watch the fun. At the moment SES device management is done via an application (user-space) and a user-space library used by the application and /dev/sgX to send SCSI commands to the SES device. I must have missed that when I was looking for implementations; what's the URL? I'm not aware of any GPLed ones. That doesn't necessarily mean that the best course of action is to bloat the kernel. You can move your ses/enclosure stuff to a user space application library and thus start a GPLed one. Certainly ... patches welcome. I've non at the moment, plus I don't think you'd be the point of contact for a user-space SES library. Unless of course you've already started something up on sourceforge. Really, such an effort already exists: it is called sg_ses(8). But, if we have non-scsi enclosures to integrate, that makes it harder for a user application because it has to know all the implementations. So does the kernel. And as I pointed out above, it is a lot easier to upgrade a user-space application and library than it is to upgrade a new kernel and having to reboot the computer to run the new kernel. No, think again ... it's easy for SES based enclosures because they have a SCSI transport. We have no transport for SGPIO based enclosures nor for any of the other more esoteric ones. Yes, for which the transport layer, implements the scsi device node for the SES device. It doesn't really matter if the SCSI commands sent to the SES device go over SGPIO or FC or SAS or Bluetooth or I2C, etc, the transport layer can implement that and present the /dev/sgX node. But it does matter if the enclosure device doesn't speak SCSI. SGPIO isn't a SCSI protocol ... it's a general purpose serial bus protocol. It's pretty simple and register based, but it might (or might not) be accessible via a SCSI bridge. Case in point: the protocol FW running on the ASIC provides this capability so really the LLDD would only see a the pure SCSI SES or processor device and register that with the kernel. At which point no new kernel bloat is required. Your code doesn't quite do that at the moment as it actually goes further in to read and present SES pages. Ideally it would simply provide capability for transport layers to register a
Re: [PATCH] enclosure: add support for enclosure services
--- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: --- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: On Mon, 2008-02-04 at 18:01 -0800, Luben Tuikov wrote: --- On Mon, 2/4/08, James Bottomley [EMAIL PROTECTED] wrote: The enclosure misc device is really just a library providing sysfs support for physical enclosure devices and their components. Who is the target audience/user of those facilities? a) The kernel itself needing to read/write SES pages? That depends on the enclosure integration, but right at the moment, it doesn't Yes, I didn't suspect so. b) A user space application using sysfs to read/write SES pages? Not an application so much as a user. The idea of sysfs is to allow users to get and set the information in addition to applications. Exactly the same argument stands for a user-space application with a user-space library. This is the classical case of where it is better to do this in user-space as opposed to the kernel. The kernel provides capability to access the SES device. The user space application and library provide interpretation and control. Thus if the enclosure were upgraded, one doesn't need to upgrade their kernel in order to utilize the new capabilities of the SES device. Plus upgrading a user-space application is a lot easier than the kernel (and no reboot necessary). The implementation is modular, so it's remove and insert ... I guess the same could be said for STGT and SCST, right? You mean both of their kernel pieces are modular? That's correct. No, you know very well what I mean. By the same logic you're preaching to include your solution part of the kernel, you can also apply to SCST. LOL, no seriously, this is unnecessary kernel bloat, or rather at the wrong place (see below). Consider another thing: vendors would really like unprecedented access to the SES device in the enclosure so as your ses/enclosure code keeps state it would get out of sync when vendor user-space enclosure applications access (and modify) the SES device's pages. The state model doesn't assume nothing else will alter the state. But it would be trivial exercise to show that an inconsistent state can be had by modifying pages of the SES device directly from userspace bypassing your implementation. I don't think so ... if you actually look at the code, you'll see it doesn't really have persistent state for the enclosure. You can test this yourself: submit a patch that removes SES /dev/sgX support; advertise your ses/class solution and watch the fun. At the moment SES device management is done via an application (user-space) and a user-space library used by the application and /dev/sgX to send SCSI commands to the SES device. I must have missed that when I was looking for implementations; what's the URL? I'm not aware of any GPLed ones. That doesn't necessarily mean that the best course of action is to bloat the kernel. You can move your ses/enclosure stuff to a user space application library and thus start a GPLed one. Certainly ... patches welcome. I've non at the moment, plus I don't think you'd be the point of contact for a user-space SES library. Unless of course you've already started something up on sourceforge. Really, such an effort already exists: it is called sg_ses(8). But, if we have non-scsi enclosures to integrate, that makes it harder for a user application because it has to know all the implementations. So does the kernel. And as I pointed out above, it is a lot easier to upgrade a user-space application and library than it is to upgrade a new kernel and having to reboot the computer to run the new kernel. No, think again ... it's easy for SES based enclosures because they have a SCSI transport. We have no transport for SGPIO based enclosures nor for any of the other more esoteric ones. Yes, for which the transport layer, implements the scsi device node for the SES device. It doesn't really matter if the SCSI commands sent to the SES device go over SGPIO or FC or SAS or Bluetooth or I2C, etc, the transport layer can implement that and present the /dev/sgX node. But it does matter if the enclosure device doesn't speak SCSI. Enclosure management isn't as simple as you're portraying it here. The enclosure management device speaks either SES or SAF-TE. The transport protocol to access it could be SGPIO or I2C or... SGPIO isn't a SCSI protocol ... it's a general purpose serial bus
[PATCH] enclosure: add support for enclosure services
The enclosure misc device is really just a library providing sysfs
support for physical enclosure devices and their components.
Signed-off-by: James Bottomley [EMAIL PROTECTED]
---
See the additional ses patch for SCSI enclosure services users of this.
---
drivers/misc/Kconfig | 10 +
drivers/misc/Makefile |1 +
drivers/misc/enclosure.c | 449 +
include/linux/enclosure.h | 120
4 files changed, 580 insertions(+), 0 deletions(-)
create mode 100644 drivers/misc/enclosure.c
create mode 100644 include/linux/enclosure.h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b5e67c0..c6e5c09 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -11,6 +11,7 @@ menuconfig MISC_DEVICES
If you say N, all options in this submenu will be skipped and
disabled.
+
if MISC_DEVICES
config IBM_ASM
@@ -232,4 +233,13 @@ config ATMEL_SSC
If unsure, say N.
+config ENCLOSURE_SERVICES
+ tristate Enclosure Services
+ default n
+ help
+ Provides support for intelligent enclosures (bays which
+ contain storage devices). You also need either a host
+ driver (SCSI/ATA) which supports enclosures
+ or a SCSI enclosure device (SES) to use these services.
+
endif # MISC_DEVICES
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 87f2685..de9f1f5 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP) += sony-laptop.o
obj-$(CONFIG_THINKPAD_ACPI)+= thinkpad_acpi.o
obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
+obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
\ No newline at end of file
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
new file mode 100644
index 000..e4683cd
--- /dev/null
+++ b/drivers/misc/enclosure.c
@@ -0,0 +1,449 @@
+/*
+ * Enclosure Services
+ *
+ * Copyright (C) 2008 James Bottomley [EMAIL PROTECTED]
+ *
+**-
+**
+** This program is free software; you can redistribute it and/or
+** modify it under the terms of the GNU General Public License
+** version 2 as published by the Free Software Foundation.
+**
+** This program is distributed in the hope that it will be useful,
+** but WITHOUT ANY WARRANTY; without even the implied warranty of
+** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+** GNU General Public License for more details.
+**
+** You should have received a copy of the GNU General Public License
+** along with this program; if not, write to the Free Software
+** Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+**
+**-
+*/
+#include linux/device.h
+#include linux/enclosure.h
+#include linux/err.h
+#include linux/list.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/mutex.h
+
+static LIST_HEAD(container_list);
+static DEFINE_MUTEX(container_list_lock);
+static struct class enclosure_class;
+static struct class enclosure_component_class;
+
+/**
+ * enclosure_find - find an enclosure given a device
+ * @dev: the device to find for
+ *
+ * Looks through the list of registered enclosures to see
+ * if it can find a match for a device. Returns NULL if no
+ * enclosure is found.
+ */
+struct enclosure_device *enclosure_find(struct device *dev)
+{
+ struct enclosure_device *edev = NULL;
+
+ mutex_lock(container_list_lock);
+ list_for_each_entry(edev, container_list, node) {
+ if (edev-cdev.dev == dev) {
+ mutex_unlock(container_list_lock);
+ return edev;
+ }
+ }
+ mutex_unlock(container_list_lock);
+
+ return NULL;
+}
+EXPORT_SYMBOL(enclosure_find);
+
+/**
+ * enclosure_for_each_device - calls a function for each enclosure
+ * @fn:the function to call
+ * @data: the data to pass to each call
+ *
+ * Loops over all the enclosures calling the function.
+ *
+ * Note, this function uses a mutex which will be held across calls to
+ * @fn, so it must have user context, and @fn should not sleep or
+ * otherwise cause the mutex to be held for indefinite periods
+ */
+int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
+ void *data)
+{
+ int error = 0;
+ struct enclosure_device *edev;
+
+ mutex_lock(container_list_lock);
+ list_for_each_entry(edev, container_list, node) {
+ error = fn(edev, data);
+ if (error)
+ break;
+ }
+ mutex_unlock(container_list_lock);
+
+ return error;
+}
+EXPORT_SYMBOL_GPL(enclosure_for_each_device);
+
+/**
+ * enclosure_register - register device as an enclosure
+ *
+ * @dev: device
Re: [PATCH] enclosure: add support for enclosure services
Hi James.
Nitpicking only.
Sam
The enclosure misc device is really just a library providing sysfs
support for physical enclosure devices and their components.
Signed-off-by: James Bottomley [EMAIL PROTECTED]
---
See the additional ses patch for SCSI enclosure services users of this.
---
drivers/misc/Kconfig | 10 +
drivers/misc/Makefile |1 +
drivers/misc/enclosure.c | 449
+
include/linux/enclosure.h | 120
4 files changed, 580 insertions(+), 0 deletions(-)
create mode 100644 drivers/misc/enclosure.c
create mode 100644 include/linux/enclosure.h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b5e67c0..c6e5c09 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -11,6 +11,7 @@ menuconfig MISC_DEVICES
If you say N, all options in this submenu will be skipped and
disabled.
+
if MISC_DEVICES
Unrelated change.
config IBM_ASM
@@ -232,4 +233,13 @@ config ATMEL_SSC
If unsure, say N.
+config ENCLOSURE_SERVICES
+ tristate Enclosure Services
+ default n
Not needed. n is default.
+ help
+ Provides support for intelligent enclosures (bays which
+ contain storage devices). You also need either a host
+ driver (SCSI/ATA) which supports enclosures
+ or a SCSI enclosure device (SES) to use these services.
+
endif # MISC_DEVICES
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 87f2685..de9f1f5 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP) += sony-laptop.o
obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o
obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
+obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
\ No newline at end of file
Can we get one..
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
new file mode 100644
index 000..e4683cd
--- /dev/null
+++ b/drivers/misc/enclosure.c
@@ -0,0 +1,449 @@
+/*
+ * Enclosure Services
+ *
+ * Copyright (C) 2008 James Bottomley [EMAIL PROTECTED]
+ *
+**-
+**
+** This program is free software; you can redistribute it and/or
+** modify it under the terms of the GNU General Public License
+** version 2 as published by the Free Software Foundation.
+**
+** This program is distributed in the hope that it will be useful,
+** but WITHOUT ANY WARRANTY; without even the implied warranty of
+** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+** GNU General Public License for more details.
+**
+** You should have received a copy of the GNU General Public License
+** along with this program; if not, write to the Free Software
+** Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+**
+**-
+*/
+#include linux/device.h
+#include linux/enclosure.h
+#include linux/err.h
+#include linux/list.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/mutex.h
+
+static LIST_HEAD(container_list);
+static DEFINE_MUTEX(container_list_lock);
+static struct class enclosure_class;
+static struct class enclosure_component_class;
+
+/**
+ * enclosure_find - find an enclosure given a device
+ * @dev: the device to find for
+ *
+ * Looks through the list of registered enclosures to see
+ * if it can find a match for a device. Returns NULL if no
+ * enclosure is found.
+ */
+struct enclosure_device *enclosure_find(struct device *dev)
+{
+ struct enclosure_device *edev = NULL;
+
+ mutex_lock(container_list_lock);
+ list_for_each_entry(edev, container_list, node) {
+ if (edev-cdev.dev == dev) {
+ mutex_unlock(container_list_lock);
+ return edev;
+ }
+ }
+ mutex_unlock(container_list_lock);
+
+ return NULL;
+}
+EXPORT_SYMBOL(enclosure_find);
No GPL - but the other of your EXPORT's are GPL.
+
+/**
+ * enclosure_for_each_device - calls a function for each enclosure
+ * @fn: the function to call
+ * @data:the data to pass to each call
+ *
+ * Loops over all the enclosures calling the function.
+ *
+ * Note, this function uses a mutex which will be held across calls to
+ * @fn, so it must have user context, and @fn should not sleep or
+ * otherwise cause the mutex to be held for indefinite periods
+ */
+int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
+ void *data)
+{
+ int error = 0;
+ struct enclosure_device *edev;
+
+ mutex_lock(container_list_lock);
+ list_for_each_entry(edev, container_list, node) {
+ error = fn(edev, data);
+ if (error)
+
Re: [PATCH] enclosure: add support for enclosure services
On Sun, 2008-02-03 at 23:03 +0100, Sam Ravnborg wrote:
Hi James.
Nitpicking only.
Sam
Thanks for the review.
+
if MISC_DEVICES
Unrelated change.
Yes, removed it.
+config ENCLOSURE_SERVICES
+ tristate Enclosure Services
+ default n
Not needed. n is default.
It is now ... plus there are a few other entries in this file with
default n.
+obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
\ No newline at end of file
Can we get one..
added
+EXPORT_SYMBOL(enclosure_find);
No GPL - but the other of your EXPORT's are GPL.
Done ... Greg gets annoyed about these.
+struct enclosure_device *
+enclosure_register(struct device *dev, const char *name, int components,
+ struct enclosure_component_callbacks *cb)
style purists would request you to put return type and function
name on the same line...
I tend to prefer the function name always at the beginning of the line.
But even style purists have to agree it's better than trying to futilely
squash all the arguments on separate lines because the return complex
type is quite long.
+ struct enclosure_device *edev =
+ kzalloc(sizeof(struct enclosure_device) +
+ sizeof(struct enclosure_component)*components,
+ GFP_KERNEL);
+ int err, i;
+
+ if (!edev)
+ return ERR_PTR(-ENOMEM);
+
+ if (!cb)
+ return ERR_PTR(-EINVAL);
We leak memory here - we never free edev.
We do indeed .. fixed.
+struct enclosure_component *
+enclosure_component_register(struct enclosure_device *edev,
+unsigned int number,
+enum enclosure_component_type type,
+const char *name)
+{
Missing kernel-doc for this and the following exports.
Yes, added.
James
---
From: James Bottomley [EMAIL PROTECTED]
Date: Sun, 3 Feb 2008 15:40:56 -0600
Subject: [SCSI] enclosure: add support for enclosure services
The enclosure misc device is really just a library providing sysfs
support for physical enclosure devices and their components.
Signed-off-by: James Bottomley [EMAIL PROTECTED]
---
drivers/misc/Kconfig |9 +
drivers/misc/Makefile |1 +
drivers/misc/enclosure.c | 486 +
include/linux/enclosure.h | 120 +++
4 files changed, 616 insertions(+), 0 deletions(-)
create mode 100644 drivers/misc/enclosure.c
create mode 100644 include/linux/enclosure.h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b5e67c0..ca68480 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -232,4 +232,13 @@ config ATMEL_SSC
If unsure, say N.
+config ENCLOSURE_SERVICES
+ tristate Enclosure Services
+ default n
+ help
+ Provides support for intelligent enclosures (bays which
+ contain storage devices). You also need either a host
+ driver (SCSI/ATA) which supports enclosures
+ or a SCSI enclosure device (SES) to use these services.
+
endif # MISC_DEVICES
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 87f2685..639c755 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP) += sony-laptop.o
obj-$(CONFIG_THINKPAD_ACPI)+= thinkpad_acpi.o
obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
+obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
new file mode 100644
index 000..42e6e43
--- /dev/null
+++ b/drivers/misc/enclosure.c
@@ -0,0 +1,486 @@
+/*
+ * Enclosure Services
+ *
+ * Copyright (C) 2008 James Bottomley [EMAIL PROTECTED]
+ *
+**-
+**
+** This program is free software; you can redistribute it and/or
+** modify it under the terms of the GNU General Public License
+** version 2 as published by the Free Software Foundation.
+**
+** This program is distributed in the hope that it will be useful,
+** but WITHOUT ANY WARRANTY; without even the implied warranty of
+** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+** GNU General Public License for more details.
+**
+** You should have received a copy of the GNU General Public License
+** along with this program; if not, write to the Free Software
+** Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+**
+**-
+*/
+#include linux/device.h
+#include linux/enclosure.h
+#include linux/err.h
+#include linux/list.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/mutex.h
+
+static LIST_HEAD(container_list);
+static DEFINE_MUTEX(container_list_lock);
+static struct class enclosure_class;
+static struct class enclosure_component_class;
+
+/**
+ * enclosure_find - find an enclosure given a device
+ * @dev:
