From patchwork Wed Apr 22 15:28:29 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alan Stern X-Patchwork-Id: 6256711 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 42AB4BF4A6 for ; Wed, 22 Apr 2015 15:28:43 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 559B4201F2 for ; Wed, 22 Apr 2015 15:28:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4F3C320320 for ; Wed, 22 Apr 2015 15:28:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965639AbbDVP2c (ORCPT ); Wed, 22 Apr 2015 11:28:32 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:49881 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S965612AbbDVP2a (ORCPT ); Wed, 22 Apr 2015 11:28:30 -0400 Received: (qmail 17496 invoked by uid 2102); 22 Apr 2015 11:28:29 -0400 Received: from localhost (sendmail-bs@127.0.0.1) by localhost with SMTP; 22 Apr 2015 11:28:29 -0400 Date: Wed, 22 Apr 2015 11:28:29 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Tom Yan cc: linux-usb@vger.kernel.org, Subject: Re: default value of power/wakeup In-Reply-To: Message-ID: MIME-Version: 1.0 Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 22 Apr 2015, Tom Yan wrote: > On 21 April 2015 at 23:51, Alan Stern wrote: > > Anyway, you're suggesting that drivers should never override sysfs > > attribute values. But there doesn't seem to be any other way to > > implement the kernel's policy that wakeup should be enabled by default > > for all keyboard devices. > > I just doubt if there should be any of these kinds of kernel's policy, The kernel _has_ to have some policy about default values. It's unavoidable -- even if you say that the default value for everything will be 0, that's still a policy! > especially for non-critical attributes like wakeup. Obviously now udev > is put to a very embarassed position (supposedly it should be the one > managing these policy, but now the fact is the kernel took its job > from it). Also, from the case of my two receivers, we can see that it > also causes unnecessary inconsistency to user experience. The inconsistency is a bug. Not all keyboard drivers have implemented the wakeup policy. That can be fixed. Try applying the patch below and let me know what happens. Overriding udev is unfortunate and we should try to avoid it. But at the moment, I can't see any way to avoid it without making a lot of users unhappy (because their keyboards will no longer automatically be enabled for wakeup). > To me it's more of "driver's policy" than the kernel's. What's the difference? Drivers are part of the kernel, after all. > If it's not > trying to make people with same hardware capibilities share the same > experience on the same attributes, what's the meaning of these > policies then? Yes of course there might be a (great) chance that it > might make (many) people with certain hardware feel happier, but > objectively does it mean anything? Not to mention that not everyone > likes the policy. (Can anyone even confirm that the majority likes > wakeup to be enabled for keyboards by default?) If the kernel developers never did anything until they had first checked to see that a majority of users were in favor, they would never do anything at all. Do you think Microsoft checked with all their users before introducing Windows Vista or Windows 8? Obviously Linux is not like Windows, for many reasons, but in some respects their developments are similar. > IMHO it would be best that any general policies considered important > to be off-loaded to udev (as a udev rule?). Only when there's no > better way (like "communicate directly" with udev?) for the driver to > set necessary specific policies for itself, it goes back to this > not-so-good method. If we were to change the policy now, it would be a regression because it would make lots of keyboards stop being wakeup-enabled by default. Kernel developers are not allowed to cause regressions except in a few rare cases (such as those involving security problems). > > After all, only the driver knows whether or not the device it > > manages is a keyboard. > > I am not sure that I understand what does this mean practically to this issue. It means that the wakeup setting cannot be initialized correctly when the sysfs attribute is created, because the attribute is created before the kernel knows that the device is a keyboard (since only the driver knows this, and the driver doesn't get bound to the device until after the attribute is created). Alan Stern --- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Index: usb-4.0/drivers/hid/hid-logitech-dj.c =================================================================== --- usb-4.0.orig/drivers/hid/hid-logitech-dj.c +++ usb-4.0/drivers/hid/hid-logitech-dj.c @@ -990,6 +990,7 @@ static int logi_dj_probe(struct hid_devi const struct hid_device_id *id) { struct usb_interface *intf = to_usb_interface(hdev->dev.parent); + struct usb_device *udev = interface_to_usbdev(intf); struct dj_receiver_dev *djrcv_dev; int retval; @@ -1078,6 +1079,9 @@ static int logi_dj_probe(struct hid_devi goto logi_dj_recv_query_paired_devices_failed; } + /* Keyboards are enabled for wakeup by default */ + device_set_wakeup_enable(&udev->dev, 1); + return retval; logi_dj_recv_query_paired_devices_failed: