On Monday, September 24, 2012, Lan Tianyu wrote: > On 2012/9/24 21:18, Rafael J. Wysocki wrote: > > On Saturday, September 22, 2012, Rafael J. Wysocki wrote: > >> On Friday, September 21, 2012, Sarah Sharp wrote: > >>> Two weeks ago at Linux Plumbers Conference, I presented about the Intel > >>> Lynx Point USB port power off mechanism. This email is a report out of > >>> what was discussed, and a kick off point for further discussion. > > > > [...] > > > >>> ca9c9d0 usb : Add sysfs files to control port power. > >> > >> http://git.kernel.org/?p=linux/kernel/git/gregkh/usb.git;a=commit;h=ca9c9d0 > > > > I have reviewed the whole series (I suppose there's too late for adding > > Reviewed-by tags to it, but oh well ;-)) and the only patch I have some serious > > doubts about is the one above. In fact, I'd prefer it to be reverted for now. > > > > First off, if the power resources manipulated through those sysfs files > > are shared between ports (i.e. there are power domains), the interface isn't > > really well defined, because the advertised functionality (powering off/on a > > USB port) is not there in general. > USB port power off is defined in the usb spec that when the port's > PORT_POWER feature is removed, port power should be turn off. > But most hubs don't implement it. So the feature should be general and > not common. I don't mean this. Suppose that there are two ports on the hub, A and B, and there's only one power resource used to put A _and_ B into D3cold. Then, when you call acpi_bus_set_power(A, D3cold) on A alone, what really happens is that the power resource's reference counter is decremented and power isn't really removed from the port. To actually remove power from A you'd need to call acpi_bus_set_power(B, D3cold) too, but then it would be removed from _both_ A and B simultaneously. Your simple sysfs interface doesn't match this use case. > > Second, I'm not sure if there's any way for user space to figure out what > > ports are connected to what sockets visible to user space. If there is such > > a way, I wonder what it is, if not, the proposed interface is just plain > > dangerous. > ACPI _PLD method provides a lot of information to describe where the > port located in. But currently it is not exposed to user space. Well, precisely. Which means that the user would have to apply trial-and-error to figure out which sysfs file corresponds to which physical port on his/her machine. That doesn't sound really user friendly. > > Finally, it follows from my experience that interfaces of this kind often > > are sources of pain and grief for distro support folks who need to handle > > problems reported by users. I used to do that and I know what kind of pain > > that is. So, in my opinion it's better not to expose low-level functionality > > to user space directly, like in this case. > > You mean force power on and power off? There is a demand that if a usb > device was abnormal, user space driver or app could make it rework via > power off. Well, then implement it as a "hard reset" attribute on the device. Namely, if the device is attached to a power-manageable port, writing 1 (for example) to its "hard reset" attribute will cause the port to be power-cycled (as long as the port has its own power resource, that is). Thanks, Rafael -- 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
