On Sat, 18 Jul 2009, Rory Filer wrote: > > But users will have to install your patch adding the module attribute. > > How is that any easier? > > Er... I thought the point of submitting these patch requests was to get the > changes incorporated into the kernel. So it is. And so would be the point of submitting a different patch which made use of the power/level attribute rather than adding a new attribute or module parameter. > > I don't understand this sentence. What do you mean by "a one-time > > configuration"? Regardless, the initial value is not going to change; > > it will always be "on". Too many USB devices behave badly for that to > > be changed. > > Agreed and we don't want to change it for those products, just for ours > and of those, just for the ones of ours which actually work with SS. Think about this more carefully. Do you really want to change the _initial_ value? Or would it be okay to wait and change the value when the sierra driver probes the device? After all, why should you care if the modem doesn't get autosuspended before the driver binds to it? Or consider another possibility: Would it be okay to leave the initial value set to "on" and have a userspace program automatically change the value to "auto" when it sees that a Sierra modem with good firmware has been registered? (And by the way, there's nothing special about _selective_ suspend. System-wide suspend works exactly the same way, as far as the device is concerned.) > But > we don't want to keep having to make the change every time the module > re-enumerates. Another poor choice of words... I don't know what you mean by "the module re-enumerates". Do you mean the driver's probe function gets called? Also, what change don't you want to make? Do you mean that you don't want the driver to change the power/level attribute? Why not? Surely you're not worried about the extra work this would entail for the probe routine. Changing a single variable's value isn't exactly going to cause the CPU to overheat and melt down. :-) > This is not an unreasonable requirement. If it were, then > .../power/level shouldn't even need to exist. As a matter of fact, drivers aren't supposed to change the power/level value at all. Userspace programs are supposed to handle it. For example, suppose you do patch sierra.c so that it automatically sets power/level to "auto" when it probes a modem capable of suspend/resume. Then what about users who _don't_ want their modems to suspend? > > Besides, I don't understand why you want to the change the value from > > "on" to "auto" anyway. I thought the problem was that a bunch of your > > modems can't suspend and resume, and hence you wanted to _prevent_ the > > value from being set to "auto" so that they would work okay. > > > Not all of our modems are shipped with firmware and configurations that > will permit Selective Suspend to work. A subset of our modems work quite > happily with SS. We want one driver to be able to work with _any_ Sierra > modem, regardless of that modem's capability. Some modems which today > don't work well with SS eventually will. In this case, it is more > user-friendly to simply turn on SS by "flipping a switch" which is how > it is implemented with this patch submission via the device attribute. (Don't you mean "module parameter" rather than "device attribute"?) Isn't that exactly what power/level already does? It allows users to turn on autosuspend by "flipping a switch", i.e., by writing "auto" to the power/level attribute file. > You suggested we consider the existing "per-device sysfs attribute for power > management"; although you didn't identify which one we guess that you meant > .../power/level. I was slightly ambiguous on purpose. The user can also prevent autosuspend by writing -1 to power/autosuspend. This is a minor point; you can assume I really did mean power/level. > Fair enough. We have considered it. Following from our > reasoning in the previous paragraph, we would want the default for this > attribute to remain "on"; for those modems in which we actually want to > have SS turned on, we'd like to have that attribute default to "auto". "Default" is not a good word to use. You want the value to end up being "auto". You don't care (or you shouldn't care) whether it ends up that way because that was its initial value or because some program or driver changed it from "on" to "auto". Let's go one step further. You don't even care if the value ends up being "auto" even for modems which don't support suspend -- all that really matters is that the system shouldn't try to autosuspend these modems. The driver can make sure this doesn't happen by failing autosuspend calls for modems with bad firmware. > Further, any actions needed to configure the change to "auto" should only > need to be done once, not every time the driver is loaded. I don't understand this fixation you have on loading the driver. When the driver is loaded has nothing to do with it; what matters is when the modem is probed by the driver. The same modem can be probed multiple times without unloading and reloading the driver (for example, by unplugging and replugging the modem). Conversely, there might be multiple modems present, and then the driver would have to probe each one even though it was loaded only once. > Plus, it should > only affect this attribute for the sierra driver, not for all drivers in the > entire system which have a power directory. Well, if the change is implemented inside the sierra driver then it certainly won't affect other drivers. :-) > We chose to use a redundant control such as the device attribute because Don't you mean "module parameter" instead of "device attribute"? > that enables us to retain control of whether autosuspend is enabled or not > within the driver. The whole system could be configured for SS, but if the > modem is not up to it, then it could be bad for the end-user. By our > requiring the end-user to switch it on in the driver, they must make the > conscious decision to turn it on _for our modem_. What happens if the user has more than one sierra modem, and some of them support suspend while others don't? Doesn't it make sense to give such users the ability to suspend the good modems without suspending the bad ones? A module parameter doesn't allow this. The power/level attribute does. And so does making sierra.c detect which modems have bad firmware. > I'll add that we have been working in good faith to accept the suggestions > we've been receiving and modify the sierra.c driver cleanly. We are not > adding a bug here. We are adding a design feature to the driver which we > feel is necessary based on our understanding of our own product. To play > devil's advocate, it is a clean use of available mechanisms which were > recommended to us by members of the Linux community and in available > documentation. So far, the only criticism I've detected is because it is > a redundant control. And that it doesn't really do what it should do. And that the driver should perhaps be capable of doing this automatically, with no need for an additional control. > What I was getting at was if there actually is a way to change the default > setting of .../power/level to "auto", it should affect just the sierra.c > driver and not all other drivers at the same time. If not, then this attribute > amounts to a global ON/OFF switch like the main breaker in an electrical > panel. We want our own breaker independent of the main one. > > One-time means exactly what it says - you configure your system (i.e. > embedded gadget) once and don't need to touch it again. OEMs like this > kind of convenience. Normally this sort of configuration is done in userspace by programs like udev or hal. It's not hard to write a udev rule that will set power/level to "auto" for all Sierra modems and nothing else. Then the "one-time configuration" simply amounts to a decision as to whether the rule should be installed. A more flexible rule might even be capable of setting power/level to "auto" only for Sierra modems with good firmware. > Anyway, thanks for taking the time to review and sorry that you found > our original description of this too vague. We're all having to spend a > lot of time on this and I'm sure we all have other things requiring our > attention! It's not at all unusual for proposed driver changes to go through this sort of protracted discussion. This is nothing compared to some! :-) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
