From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f196.google.com ([209.85.223.196]:53875 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752196AbdKJJ2a (ORCPT ); Fri, 10 Nov 2017 04:28:30 -0500 Received: by mail-io0-f196.google.com with SMTP id 189so12912880iow.10 for ; Fri, 10 Nov 2017 01:28:30 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1510154134-1248-1-git-send-email-ulf.hansson@linaro.org> From: Ulf Hansson Date: Fri, 10 Nov 2017 10:28:29 +0100 Message-ID: Subject: Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag To: "Rafael J. Wysocki" Cc: Geert Uytterhoeven , "Rafael J . Wysocki" , Linux PM list , Kevin Hilman , Viresh Kumar , Geert Uytterhoeven , Simon Horman , Niklas Soderlund , Linux-Renesas Content-Type: text/plain; charset="UTF-8" Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: On 9 November 2017 at 17:31, Rafael J. Wysocki wrote: > On Thu, Nov 9, 2017 at 3:41 PM, Geert Uytterhoeven wrote: >> Hi Ulf, >> >> On Thu, Nov 9, 2017 at 3:28 PM, Ulf Hansson wrote: >>> [...] >>> >>>>>> The Ethernet driver can still call device_set_wakeup_enable(... , false) >>>>>> to control this. If WoL is disabled by the user (or deemed not usable, see >>>>>> below), it already does so. >>>>> >>>>> I don't think that API is intended to be used like that and I wonder >>>>> if it even works as expected. >>>>> >>>>> Quoting the doc: >>>>> "If device wakeup mechanisms are enabled or disabled directly by >>>>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do >>>>> during a system sleep transition. Device drivers, however, are not expected to >>>>> call :c:func:`device_set_wakeup_enable()` directly in any case." >>>>> >>>>> Rafael, can you comment on this? >>>> >>>> There are ca. 100 callers in drivers. >>> >>> Yeah, but those doesn't normally use it to toggle the setting, but >>> instead only to set a default value during ->probe() or whatever >>> initialization code that runs. >>> >>> I think that makes a big difference, doesn't it? >> >> The few Ethernet drivers I looked at change the state in their >> ethtool_ops.set_wol() callback, not during probe. >> This is to be configured from userspace using ethtool. > > Which is the case I was talking about. > > Since changing the WoL setting via ethtool is expected to cause the > sysfs knob to reflect its status, using device_set_wakeup_enable() in > there is not actually confusing. > > The same applies to setting the default from ->probe(). > > It will be confusing in all of the other cases, though. Agreed, so using it to enable/disable wakeups during suspend is a bad idea. I will re-spin my series, addressing your review comments. Kind regards Uffe