| Message ID | 1358777887-2656-8-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote: > Some usb devices can't be resumed correctly after power off. This > patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off() > for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage > count and then port will not be suspended. The device will not be powered off. Please linewrap comments at the proper level (git likes 72). > > Acked-by: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > drivers/usb/core/port.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > index 0c51d24..0334d91 100644 > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -18,11 +18,39 @@ > > #include <linux/slab.h> > #include <linux/pm_qos.h> > +#include <linux/module.h> > > #include "hub.h" > > static const struct attribute_group *port_dev_group[]; > > +/** > + * usb_device_control_power_off - Allow or prohibit power off device. > + * @udev: target usb device > + * @allow: choice of allow or prohibit > + * > + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target > + * usb device to be powered off in the kernel. The operations of setting > + * true and false should be couple. The default status is allowed. > + */ > +int usb_device_control_power_off(struct usb_device *udev, bool allow) Ick, again with the boolean variables. Please don't do this, just make two different functions: usb_device_allow_power_off() usb_device_forbid_power_off() that makes it much easier to understand when you see the function being called, right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote: > Some usb devices can't be resumed correctly after power off. This > patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off() > for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage > count and then port will not be suspended. The device will not be powered off. > > Acked-by: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > drivers/usb/core/port.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > index 0c51d24..0334d91 100644 > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -18,11 +18,39 @@ > > #include <linux/slab.h> > #include <linux/pm_qos.h> > +#include <linux/module.h> > > #include "hub.h" > > static const struct attribute_group *port_dev_group[]; > > +/** > + * usb_device_control_power_off - Allow or prohibit power off device. > + * @udev: target usb device > + * @allow: choice of allow or prohibit > + * > + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target > + * usb device to be powered off in the kernel. The operations of setting > + * true and false should be couple. The default status is allowed. > + */ > +int usb_device_control_power_off(struct usb_device *udev, bool allow) > +{ > + struct usb_port *port_dev; > + > + if (!udev->parent) > + return -EINVAL; > + > + port_dev = hdev_to_hub(udev->parent)->ports[udev->portnum - 1]; > + > + if (allow) > + pm_runtime_put_sync(&port_dev->dev); > + else > + pm_runtime_get_sync(&port_dev->dev); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(usb_device_control_power_off); Oh, I don't see any code calling this function, so why did you add it? Who needs it? Where is that code? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/1/22 5:33, Greg KH wrote: > On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote: >> Some usb devices can't be resumed correctly after power off. This >> patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off() >> for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage >> count and then port will not be suspended. The device will not be powered off. > > Please linewrap comments at the proper level (git likes 72). > >> >> Acked-by: Alan Stern <stern@rowland.harvard.edu> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> drivers/usb/core/port.c | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c >> index 0c51d24..0334d91 100644 >> --- a/drivers/usb/core/port.c >> +++ b/drivers/usb/core/port.c >> @@ -18,11 +18,39 @@ >> >> #include <linux/slab.h> >> #include <linux/pm_qos.h> >> +#include <linux/module.h> >> >> #include "hub.h" >> >> static const struct attribute_group *port_dev_group[]; >> >> +/** >> + * usb_device_control_power_off - Allow or prohibit power off device. >> + * @udev: target usb device >> + * @allow: choice of allow or prohibit >> + * >> + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target >> + * usb device to be powered off in the kernel. The operations of setting >> + * true and false should be couple. The default status is allowed. >> + */ >> +int usb_device_control_power_off(struct usb_device *udev, bool allow) > > Ick, again with the boolean variables. Please don't do this, just make > two different functions: > usb_device_allow_power_off() > usb_device_forbid_power_off() > that makes it much easier to understand when you see the function being > called, right? Ok. I will do it. > > thanks, > > greg k-h >
On 2013/1/22 5:33, Greg KH wrote: > On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote: >> Some usb devices can't be resumed correctly after power off. This >> patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off() >> for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage >> count and then port will not be suspended. The device will not be powered off. >> >> Acked-by: Alan Stern <stern@rowland.harvard.edu> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> drivers/usb/core/port.c | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c >> index 0c51d24..0334d91 100644 >> --- a/drivers/usb/core/port.c >> +++ b/drivers/usb/core/port.c >> @@ -18,11 +18,39 @@ >> >> #include <linux/slab.h> >> #include <linux/pm_qos.h> >> +#include <linux/module.h> >> >> #include "hub.h" >> >> static const struct attribute_group *port_dev_group[]; >> >> +/** >> + * usb_device_control_power_off - Allow or prohibit power off device. >> + * @udev: target usb device >> + * @allow: choice of allow or prohibit >> + * >> + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target >> + * usb device to be powered off in the kernel. The operations of setting >> + * true and false should be couple. The default status is allowed. >> + */ >> +int usb_device_control_power_off(struct usb_device *udev, bool allow) >> +{ >> + struct usb_port *port_dev; >> + >> + if (!udev->parent) >> + return -EINVAL; >> + >> + port_dev = hdev_to_hub(udev->parent)->ports[udev->portnum - 1]; >> + >> + if (allow) >> + pm_runtime_put_sync(&port_dev->dev); >> + else >> + pm_runtime_get_sync(&port_dev->dev); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(usb_device_control_power_off); > > Oh, I don't see any code calling this function, so why did you add it? > > Who needs it? Where is that code? This is provided for device driver. Some device may not be compatible with the power off mechanism and driver can use these function to forbid/allow it. But currently, we are not sure which kinds of device they are. So just provide new interfaces. > > thanks, > > greg k-h >
On Tue, Jan 22, 2013 at 10:23:12PM +0800, Lan Tianyu wrote: > On 2013/1/22 5:33, Greg KH wrote: > >On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote: > >>Some usb devices can't be resumed correctly after power off. This > >>patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off() > >>for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage > >>count and then port will not be suspended. The device will not be powered off. > >> > >>Acked-by: Alan Stern <stern@rowland.harvard.edu> > >>Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > >>--- > >> drivers/usb/core/port.c | 28 ++++++++++++++++++++++++++++ > >> 1 file changed, 28 insertions(+) > >> > >>diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > >>index 0c51d24..0334d91 100644 > >>--- a/drivers/usb/core/port.c > >>+++ b/drivers/usb/core/port.c > >>@@ -18,11 +18,39 @@ > >> > >> #include <linux/slab.h> > >> #include <linux/pm_qos.h> > >>+#include <linux/module.h> > >> > >> #include "hub.h" > >> > >> static const struct attribute_group *port_dev_group[]; > >> > >>+/** > >>+ * usb_device_control_power_off - Allow or prohibit power off device. > >>+ * @udev: target usb device > >>+ * @allow: choice of allow or prohibit > >>+ * > >>+ * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target > >>+ * usb device to be powered off in the kernel. The operations of setting > >>+ * true and false should be couple. The default status is allowed. > >>+ */ > >>+int usb_device_control_power_off(struct usb_device *udev, bool allow) > >>+{ > >>+ struct usb_port *port_dev; > >>+ > >>+ if (!udev->parent) > >>+ return -EINVAL; > >>+ > >>+ port_dev = hdev_to_hub(udev->parent)->ports[udev->portnum - 1]; > >>+ > >>+ if (allow) > >>+ pm_runtime_put_sync(&port_dev->dev); > >>+ else > >>+ pm_runtime_get_sync(&port_dev->dev); > >>+ > >>+ return 0; > >>+} > >>+EXPORT_SYMBOL_GPL(usb_device_control_power_off); > > > >Oh, I don't see any code calling this function, so why did you add it? > > > >Who needs it? Where is that code? > This is provided for device driver. Some device may not be > compatible with the power off mechanism and driver can use these > function to forbid/allow it. But currently, we are not sure which > kinds of device > they are. So just provide new interfaces. We don't add new interfaces to the kernel unless they have a user. If I were to see this in the tree, I would expect it to be removed because of that. So don't add it at all, only add it when it is needed. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 22 Jan 2013, Lan Tianyu wrote: > >> +/** > >> + * usb_device_control_power_off - Allow or prohibit power off device. > >> + * @udev: target usb device > >> + * @allow: choice of allow or prohibit > >> + * > >> + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target > >> + * usb device to be powered off in the kernel. The operations of setting > >> + * true and false should be couple. The default status is allowed. > >> + */ > >> +int usb_device_control_power_off(struct usb_device *udev, bool allow) > > > > Ick, again with the boolean variables. Please don't do this, just make > > two different functions: > > usb_device_allow_power_off() > > usb_device_forbid_power_off() > > that makes it much easier to understand when you see the function being > > called, right? > Ok. I will do it. Greg and Tianyu: You both missed the fact that Tianyu already did this. It is in the submitted patch. The two functions Greg asked for are defined as inline routines. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index 0c51d24..0334d91 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -18,11 +18,39 @@ #include <linux/slab.h> #include <linux/pm_qos.h> +#include <linux/module.h> #include "hub.h" static const struct attribute_group *port_dev_group[]; +/** + * usb_device_control_power_off - Allow or prohibit power off device. + * @udev: target usb device + * @allow: choice of allow or prohibit + * + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target + * usb device to be powered off in the kernel. The operations of setting + * true and false should be couple. The default status is allowed. + */ +int usb_device_control_power_off(struct usb_device *udev, bool allow) +{ + struct usb_port *port_dev; + + if (!udev->parent) + return -EINVAL; + + port_dev = hdev_to_hub(udev->parent)->ports[udev->portnum - 1]; + + if (allow) + pm_runtime_put_sync(&port_dev->dev); + else + pm_runtime_get_sync(&port_dev->dev); + + return 0; +} +EXPORT_SYMBOL_GPL(usb_device_control_power_off); + static ssize_t show_port_connect_type(struct device *dev, struct device_attribute *attr, char *buf) {