On 2012年11月14日 08:08, Rafael J. Wysocki wrote:
> On Tuesday, November 13, 2012 04:00:02 PM Lan Tianyu wrote:
>> This patch is to add runtime pm callback for usb port device.
>> Set/clear PORT_POWER feature in the resume/suspend callbak.
>> Add portnum for struct usb_port to record port number.
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
>
> This one looks reasonable to me. From the PM side
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
Hi Rafael and Alan:
This patch has a collaboration problem with pm qos. Since pm core would
pm_runtime_get_sync/put(port_dev) if pm qos flags was changed and port's
suspend call_back() clear PORT_POWER feature without any check. This
will cause PORT_POWER feather being cleared every time after pm qos
flags being changed.
I have an idea that add check in the port's runtime idle callback.
Check NO_POWER_OFF flag firstly. If set return. Second, for port without
device, suspend port directly and for port with device, increase child
device's runtime usage without resume and do barrier to ensure all
suspend process finish, at last check the child runtime status. If it
was suspended, suspend port and if do nothing.
static int usb_port_runtime_idle(struct device *dev)
{
struct usb_port *port_dev = to_usb_port(dev);
int retval = 0;
if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
== PM_QOS_FLAGS_ALL)
return 0;
if (!port_dev->child) {
retval = pm_runtime_suspend(&port_dev->dev);
if (!retval)
port_dev->power_on =false;
}
else {
pm_runtime_get_noresume(&port_dev->child->dev);
pm_runtime_barrier(&port_dev->child->dev);
if (port_dev->child->dev.power.runtime_status
== RPM_SUSPENDED) {
retval = pm_runtime_suspend(&port_dev->dev);
if (!retval)
port_dev->power_on =false;
}
pm_runtime_put_noidle(&port_dev->child->dev);
}
return retval;
}
I'd like to see your opinion :) thanks.
>
>> ---
>> drivers/usb/core/hub.c | 14 ++++++++++++++
>> drivers/usb/core/port.c | 39 +++++++++++++++++++++++++++++++++++++++
>> drivers/usb/core/usb.h | 2 ++
>> include/linux/usb.h | 2 ++
>> 4 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 60746aa..2cb414e 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -765,6 +765,20 @@ static void hub_tt_work(struct work_struct *work)
>> spin_unlock_irqrestore (&hub->tt.lock, flags);
>> }
>>
>> +int usb_hub_set_port_power(struct usb_device *hdev, int port1,
>> + bool set)
>> +{
>> + int ret;
>> +
>> + if (set)
>> + ret = set_port_feature(hdev, port1,
>> + USB_PORT_FEAT_POWER);
>> + else
>> + ret = clear_port_feature(hdev, port1,
>> + USB_PORT_FEAT_POWER);
>> + return ret;
>> +}
>> +
>> /**
>> * usb_hub_clear_tt_buffer - clear control/bulk TT state in high speed hub
>> * @urb: an URB associated with the failed or incomplete split transaction
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 6523a03..1cda766 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -53,9 +53,45 @@ static void usb_port_device_release(struct device *dev)
>> kfree(port_dev);
>> }
>>
>> +static int usb_port_runtime_resume(struct device *dev)
>> +{
>> + struct usb_port *port_dev = to_usb_port(dev);
>> + struct usb_device *hdev =
>> + to_usb_device(dev->parent->parent);
>> +
>> + return usb_hub_set_port_power(hdev, port_dev->portnum,
>> + true);
>> +}
>> +
>> +static int usb_port_runtime_suspend(struct device *dev)
>> +{
>> + struct usb_port *port_dev = to_usb_port(dev);
>> + struct usb_device *hdev =
>> + to_usb_device(dev->parent->parent);
>> +
>> + return usb_hub_set_port_power(hdev, port_dev->portnum,
>> + false);
>> +}
>> +
>> +static int usb_port_runtime_idle(struct device *dev)
>> +{
>> + struct usb_port *port_dev = to_usb_port(dev);
>> +
>> + return pm_runtime_suspend(&port_dev->dev);
>> +}
>> +
>> +static const struct dev_pm_ops usb_port_pm_ops = {
>> +#ifdef CONFIG_USB_SUSPEND
>> +.runtime_suspend = usb_port_runtime_suspend,
>> +.runtime_resume = usb_port_runtime_resume,
>> +.runtime_idle = usb_port_runtime_idle,
>> +#endif
>> +};
>> +
>> struct device_type usb_port_device_type = {
>> .name = "usb_port",
>> .release = usb_port_device_release,
>> + .pm = &usb_port_pm_ops,
>> };
>>
>> int usb_hub_create_port_device(struct device *intfdev,
>> @@ -63,6 +99,7 @@ int usb_hub_create_port_device(struct device *intfdev,
>> {
>> int retval;
>>
>> + port_dev->portnum = port1;
>> port_dev->dev.parent = intfdev;
>> port_dev->dev.groups = port_dev_group;
>> port_dev->dev.type = &usb_port_device_type;
>> @@ -72,6 +109,8 @@ int usb_hub_create_port_device(struct device *intfdev,
>> if (retval)
>> goto error_register;
>>
>> + pm_runtime_set_active(&port_dev->dev);
>> + pm_runtime_enable(&port_dev->dev);
>> usb_acpi_register_power_resources(&port_dev->dev);
>>
>> return 0;
>> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
>> index de7434b..47d77d4 100644
>> --- a/drivers/usb/core/usb.h
>> +++ b/drivers/usb/core/usb.h
>> @@ -62,6 +62,8 @@ extern void usb_major_cleanup(void);
>>
>> extern int usb_hub_create_port_device(struct device *intfdev,
>> int port1, struct usb_port *port_dev);
>> +extern int usb_hub_set_port_power(struct usb_device *hdev,
>> + int port1, bool set);
>>
>> #ifdef CONFIG_PM
>>
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index c1f1346..71510bf 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -578,12 +578,14 @@ struct usb_device {
>> * @dev: generic device interface
>> * @port_owner: port's owner
>> * @connect_type: port's connect type
>> + * @portnum: port index num based one
>> */
>> struct usb_port {
>> struct usb_device *child;
>> struct device dev;
>> struct dev_state *port_owner;
>> enum usb_port_connect_type connect_type;
>> + u8 portnum;
>> };
>> #define to_usb_port(_dev) \
>> container_of(_dev, struct usb_port, dev)
>>
--
Best regards
Tianyu Lan
--
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
