RE: [PATCH net 2/3] r8152: fix remote wakeup
Oliver Neukum [mailto:oneu...@suse.com] [...] If the device does not support remote wakeup and the driver enables it, runtime power management will be switched off. That is the current state and it means that devices which don't support remote wakeup cannot do runtime power management at all. But the driver is correct. The only time a device that doesn't support remote wakeup can do runtime power managent is when no packets can be received that is while the interface is down. If you want to allow that you must not set needs_remote_wakeup in probe(), but you must set it in open() because it is necessary for runtime power management as I explained above. Sorry for the length of this mail, but I wanted to make sure I am absolutely clear this time. Thanks for you explanation. I focus on the wrong point. You mean this would cause the problem for runtime suspend. I would rethink my method and correct it. Best Regards, Hayes
RE: [PATCH net 2/3] r8152: fix remote wakeup
Hi, this is most likely wrong. Usbcore does check for a device's ability to do remote wakeup and will block a runtime suspend if it detects that a remote wakeup would be required but the device cannot deliver. (static int autosuspend_check()) So by removing the flag in the probe() method means that devices will suspend during operations without remote wakeup requested. Thus an incoming packet cannot wake them up. If you remove setting the flag on probe() you need to set it at open() [and reset on close()], as devices which cannot do remote wakeup must only be suspended when they are down. Hi, I don't think I understand your description clearly. My idea is that if the device doesn't support wakeup, we don't need set needs_remote_wakeup. We allow the device could be suspended and couldn't be waked up by incoming packet. The system could be waked up by other methods except by the device. I don't understand why I have to set it at open() and reset it at close(). And why must the device only be suspended when it is down? Best Regards, Hayes
Re: [PATCH net 2/3] r8152: fix remote wakeup
On Thu, 2015-07-23 at 15:09 +0800, Hayes Wang wrote: Set needs_remote_wakeup only when the device supports it. Signed-off-by: Hayes Wang hayesw...@realtek.com --- drivers/net/usb/r8152.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index e3a0110..eff1f25 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -4059,7 +4059,8 @@ static int rtl8152_probe(struct usb_interface *intf, break; } - intf-needs_remote_wakeup = 1; + if (udev-actconfig-desc.bmAttributes USB_CONFIG_ATT_WAKEUP) + intf-needs_remote_wakeup = 1; Hi, this is most likely wrong. Usbcore does check for a device's ability to do remote wakeup and will block a runtime suspend if it detects that a remote wakeup would be required but the device cannot deliver. (static int autosuspend_check()) So by removing the flag in the probe() method means that devices will suspend during operations without remote wakeup requested. Thus an incoming packet cannot wake them up. If you remove setting the flag on probe() you need to set it at open() [and reset on close()], as devices which cannot do remote wakeup must only be suspended when they are down. Regards Oliver -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net 2/3] r8152: fix remote wakeup
On Thu, 2015-07-23 at 09:55 +, Hayes Wang wrote:
Hi,
this is most likely wrong. Usbcore does check for a device's ability to
do remote wakeup and will block a runtime suspend if it detects that
a remote wakeup would be required but the device cannot deliver.
(static int autosuspend_check())
So by removing the flag in the probe() method means that devices will
suspend during operations without remote wakeup requested. Thus an
incoming packet cannot wake them up.
If you remove setting the flag on probe() you need to set it at open()
[and reset on close()], as devices which cannot do remote wakeup must
only be suspended when they are down.
Hi,
I don't think I understand your description clearly. My idea is that if the
Sorry, I will try to be clearer.
device doesn't support wakeup, we don't need set needs_remote_wakeup.
The algorithm for power saving needs remote wakeup.
We allow the device could be suspended and couldn't be waked up by
incoming packet. The system could be waked up by other methods except
by the device.
The system is not the problem. I was talking about device level power
management at runtime.
I don't understand why I have to set it at open() and reset it at close(). And
why must the device only be suspended when it is down?
OK,
the driver right now requests that runtime power management be done:
static struct usb_driver rtl8152_driver = {
.name = MODULENAME,
.id_table = rtl8152_table,
.probe =rtl8152_probe,
.disconnect = rtl8152_disconnect,
.suspend = rtl8152_suspend,
.resume = rtl8152_resume,
.reset_resume = rtl8152_resume,
.supports_autosuspend = 1,
^ the crucial flag
.disable_hub_initiated_lpm = 1,
};
It implements an advanced form of runtime power management which
requires remote wakeup.
For the device to work correctly it must not be suspended while
a - a setting is changed
b - a packet is transmitted
c - a packet is received
Cases a and b are covered by the driver on its own.
Case a see for example rtl8152_set_mac_address()
Case b is handled in rtl8152_start_xmit()
if (test_bit(SELECTIVE_SUSPEND, tp-flags)) {
set_bit(SCHEDULE_NAPI, tp-flags);
schedule_delayed_work(tp-schedule, 0);
} else {
usb_mark_last_busy(tp-udev);
The scheduled work will resume the device if necessary.
Case c is handled in rtl8152_resume()
if (netif_running(tp-netdev)) {
if (test_bit(SELECTIVE_SUSPEND, tp-flags)) {
rtl_runtime_suspend_enable(tp, false);
clear_bit(SELECTIVE_SUSPEND, tp-flags);
set_bit(WORK_ENABLE, tp-flags);
if (netif_carrier_ok(tp-netdev))
rtl_start_rx(tp);
But that code path only runs if remote wakeup is enabled.
So to operate with runtime power management is is necessary
that the device support remote wakeup and the driver enable it.
If the device does not support remote wakeup and the driver
enables it, runtime power management will be switched off.
That is the current state and it means that devices which
don't support remote wakeup cannot do runtime power management
at all. But the driver is correct.
The only time a device that doesn't support remote wakeup can
do runtime power managent is when no packets can be received
that is while the interface is down. If you want to allow that
you must not set needs_remote_wakeup in probe(), but you must
set it in open() because it is necessary for runtime power
management as I explained above.
Sorry for the length of this mail, but I wanted to make sure
I am absolutely clear this time.
HTH
Oliver
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
