linux-kernel - Re: [PATCH 2/3] Revert "Input: hyperv-keyboard

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241209171623.GA29631@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Date: Mon, 9 Dec 2024 09:16:23 -0800
From: Saurabh Singh Sengar <ssengar@...ux.microsoft.com>
To: Erni Sri Satya Vennela <ernis@...ux.microsoft.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
	"Srivatsa S. Bhat" <srivatsa@...il.mit.edu>, kys@...rosoft.com,
	haiyangz@...rosoft.com, wei.liu@...nel.org, decui@...rosoft.com,
	jikos@...nel.org, bentiss@...nel.org, linux-hyperv@...r.kernel.org,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	ernis@...rosoft.com, rafael@...nel.org, pavel@....cz,
	lenb@...nel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 2/3] Revert "Input: hyperv-keyboard - register as a
 wakeup source"

On Fri, Nov 08, 2024 at 02:47:41AM -0800, Erni Sri Satya Vennela wrote:
> On Thu, Oct 17, 2024 at 06:44:38AM -0700, Erni Sri Satya Vennela wrote:
> > On Fri, Oct 04, 2024 at 01:14:10AM -0700, Dmitry Torokhov wrote:
> > > On Tue, Sep 24, 2024 at 03:28:51AM +0000, Srivatsa S. Bhat wrote:
> > > > [+linux-pm, Rafael, Len, Pavel]
> > > > 
> > > > On Thu, Sep 12, 2024 at 02:27:49PM -0700, Erni Sri Satya Vennela wrote:
> > > > > This reverts commit 62238f3aadc9bc56da70100e19ec61b9f8d72a5f.
> > > > > 
> > > > > Remove keyboard as wakeup source since Suspend-to-Idle feature
> > > > > is disabled.
> > > > > 
> > > > > Signed-off-by: Erni Sri Satya Vennela <ernis@...ux.microsoft.com>
> > > > > ---
> > > > >  drivers/input/serio/hyperv-keyboard.c | 12 ------------
> > > > >  1 file changed, 12 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
> > > > > index 31d9dacd2fd1..b42c546636bf 100644
> > > > > --- a/drivers/input/serio/hyperv-keyboard.c
> > > > > +++ b/drivers/input/serio/hyperv-keyboard.c
> > > > > @@ -162,15 +162,6 @@ static void hv_kbd_on_receive(struct hv_device *hv_dev,
> > > > >  			serio_interrupt(kbd_dev->hv_serio, scan_code, 0);
> > > > >  		}
> > > > >  		spin_unlock_irqrestore(&kbd_dev->lock, flags);
> > > > > -
> > > > > -		/*
> > > > > -		 * Only trigger a wakeup on key down, otherwise
> > > > > -		 * "echo freeze > /sys/power/state" can't really enter the
> > > > > -		 * state because the Enter-UP can trigger a wakeup at once.
> > > > > -		 */
> > > > > -		if (!(info & IS_BREAK))
> > > > > -			pm_wakeup_hard_event(&hv_dev->device);
> > > > > -
> > > > >  		break;
> > > > >  
> > > > >  	default:
> > > > > @@ -356,9 +347,6 @@ static int hv_kbd_probe(struct hv_device *hv_dev,
> > > > >  		goto err_close_vmbus;
> > > > >  
> > > > >  	serio_register_port(kbd_dev->hv_serio);
> > > > > -
> > > > > -	device_init_wakeup(&hv_dev->device, true);
> > > 
> > > If you do not want the keyboard to be a wakeup source by default maybe
> > > change this to:
> > > 
> > > 	device_set_wakeup_capable(&hv_dev->device, true);
> > > 
> > > and leave the rest of the driver alone?
> > > 
> > > Same for the HID change.
> > > 
> > > Thanks.
> > >
> > device_set_wakeup_capable() sets the @dev's power.can_wakeup flag and
> > adds wakeup-related attributes in sysfs.
> > 
> > Could you please help me understand why explicitly calling this function 
> > can be helpful in our use case?
> > 
> > > -- 
> > > Dmitry
> Just following up on this patch. Could you please help me understand the
> reason for the change?


Vennela,

There is a difference between "wakeup source registration" and "wakeup capable".
For this there are two flags defined in power management framework:
 1. power.wakeup
 2. power.can_wakeup
 
More details on these flags can be read here: 
https://www.kernel.org/doc/html/v6.12/driver-api/pm/devices.html

'device_init_wakeup(dev, true)' sets both; ie it registers the device as a wakeup
source and marks it as wakeup capable too.

In our case, the device is "wakeup capable" but we do not want to
"register it as a wakeup source". 'device_set_wakeup_capable(dev, true)' is more
appropriate because this marks the device as wakeup capable but doesn't register
it as a wakeup source knowingly.

I understand that Dimitry suggests not to revert the entire patch but to replace
'device_init_wakeup' with 'device_set_wakeup_capable', to mark the device as
capable of wakeup but knowingly skipping the registering part.

Requesting Dimitry to correct me if there is any misinterpretation.

While fixing this in next version, please fix the kernel bot warings as well
reported for 1/3 patch of this series.

- Saurabh