From patchwork Mon May 13 15:01:30 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Herrmann X-Patchwork-Id: 2558871 X-Patchwork-Delegate: jikos@jikos.cz Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 93FE9DF2E5 for ; Mon, 13 May 2013 15:01:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751589Ab3EMPBd (ORCPT ); Mon, 13 May 2013 11:01:33 -0400 Received: from mail-ea0-f178.google.com ([209.85.215.178]:62771 "EHLO mail-ea0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751052Ab3EMPBc (ORCPT ); Mon, 13 May 2013 11:01:32 -0400 Received: by mail-ea0-f178.google.com with SMTP id q16so613697ead.23 for ; Mon, 13 May 2013 08:01:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:from:to:cc:subject:date:message-id:x-mailer; bh=nmya1H7NsxZl6EgDZm3MfnY4uxC52i6Zfdq5yCYDuWU=; b=VtWm1AHlERnQB3fGEtp1j0fTfmoCA8eoIEBsifLnO0SXJq5DqK9neJyCkyS/gMcI8X v+7YxafS9J8R1OnyRcS7cTdkMxMtfD1g9qZpnHsK2jQZveihiUSzMTu8Bm48Ahsim8u1 8oDRX7c4vw7h7/OhQFQ2JyFxbW4Jy5WtZkLTW43l6wqqnS1prqQpjmv/1Q38INtU5ikY oF2THApINMKYHjsgwAPA2pMSbsSSF2t73EAvZdLYwbeJP3jMPHxswNWHJXhrpu/HqgdD OYqliQt0+7yPyWL0Srl1kh2EU3b5g3mFzaOzk6pz1HmNtfSa9QjdHd3Qv1Ecl7MmYygJ IhqA== X-Received: by 10.14.38.198 with SMTP id a46mr79916157eeb.11.1368457290627; Mon, 13 May 2013 08:01:30 -0700 (PDT) Received: from localhost.localdomain (stgt-5f719e34.pool.mediaWays.net. [95.113.158.52]) by mx.google.com with ESMTPSA id y10sm23516785eev.3.2013.05.13.08.01.28 for (version=TLSv1.2 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 13 May 2013 08:01:29 -0700 (PDT) From: David Herrmann To: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org, David Herrmann , Jiri Kosina , Anton Vorontsov , David Woodhouse Subject: [PATCH] HID: input: return ENODATA if reading battery attrs fails Date: Mon, 13 May 2013 17:01:30 +0200 Message-Id: <1368457290-1734-1-git-send-email-dh.herrmann@gmail.com> X-Mailer: git-send-email 1.8.2.3 Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org power_supply core has the bad habit of calling our battery callbacks from within power_supply_register(). Furthermore, if the callbacks fail with an unhandled error code, it will skip any uevent that it might currently process. So if HID-core registers battery devices, an "add" uevent is generated and the battery callbacks are called. These will gracefully fail due to timeouts as they might still hold locks on event processing. One could argue that this should be fixed in power_supply core, but the least we can do is to signal ENODATA so power_supply core will just skip the property and continue with the uevent. This fixes a bug where "add" and "remove" uevents are skipped for battery devices. upower is unable to track these devices and currently needs to ignore them. This patch also overwrites any other error code. I cannot see any reason why we should forward protocol- or I/O-errors to the power_supply core. We handle these errors in hid_ll_driver later, anyway, so just skip them. power_supply core cannot do anything useful with them, anyway, and we avoid skipping important uevents and confusing user-space. Thanks a lot to Daniel Nicoletti for pushing and investigating on this. Cc: Jiri Kosina Cc: Anton Vorontsov Cc: David Woodhouse Reported-by: Daniel Nicoletti Signed-off-by: David Herrmann --- Hi I really dislike the way power_supply core calls into the drivers during the "add" uevent. If a driver holds an I/O mutex (or anything else), it might even deadlock in a very non-obvious way. Is there a reason why we need to pass _all_ battery properties along "add" and "remove" uevents? Isn't it enough to pass them with "change" uevents? This would guarantee that the power_supply callbacks are only called from user-context and "change" events. Anyway, I'd still like to see this patch applied so we have this annoying bug fixed. I'd be willing to change the power_supply core, too, if one of the maintainers agrees on the design (David? Anton?). And, @Daniel, can you check whether this actually fixes the bug? I don't own a device that reports battery-information, but it at least fixes the same bug for the hid-wiimote driver (tested by me). Cheers David drivers/hid/hid-input.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 945b815..c526a3c 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -354,10 +354,10 @@ static int hidinput_get_battery_property(struct power_supply *psy, dev->battery_report_type); if (ret != 2) { - if (ret >= 0) - ret = -EINVAL; + ret = -ENODATA; break; } + ret = 0; if (dev->battery_min < dev->battery_max && buf[1] >= dev->battery_min &&