From patchwork Mon Sep 18 16:35:35 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Johan Hovold X-Patchwork-Id: 141497 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp2844777vqi; Mon, 18 Sep 2023 10:57:15 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHR36FNr04hRGdzrAqG3HZ1i2GkoL3jkEg+xnKr8umdLQQe2aYM6Flz+AyIMA4EOYXU/qut X-Received: by 2002:a17:90b:3842:b0:262:d661:2963 with SMTP id nl2-20020a17090b384200b00262d6612963mr7384758pjb.39.1695059835673; Mon, 18 Sep 2023 10:57:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695059835; cv=none; d=google.com; s=arc-20160816; b=wvaqIUp5TIZAg9/TpJk6jFbrTgUT+nNRRK3YOf0JM32p17nM1ivVT42oPb8oqapHSS PtWbTrexXOwSebddo1WcHyu5ro5hYI3ppksDHSuyBXjDRX4M3jBbkhduhIbgXONzdvoO mtanbdxwTUgTHPikdrL+xyna1LW7Jo/m107UtfEDseZM+4KhIhFy3KiA/IfEELu4mqLc qkbWr7HOm/MBjikTc6ZqHtCXJ+qbQWQyrl9X2GpwBafA4squ8hBg7g5IauYKdR5u+dHb y81GO8AQs4/BxEMXHieHPsWn9f4zViKEUL8gWzz+XSkSu/lSMnv8KB5NoM4euSaHA4Em 1EZA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=akYYEYwYdtAIy+vsWoynoDll2mKtUlfTrsdRlhA14xc=; fh=RHO8AkmR0iOeVFvHpRKuhuAYG8Z/b+/FIzCxzchGVJ0=; b=X5i5JvlBL/t+2hDBFm3VfWMUWT3zTiwG84Ui0lazQJd3piOfWz9lzrRm0VuByVQyXH Dr7kjYCW4pjPeBylY6yl//VUTPatO/OQgJt5p0CKovCokaPKOu7krQtDoglBkweZDMK8 b49zW2jSmvDHkRTyRg8OGYvOKjOVh60joYOofYkmGyzKr3eReDDUpcCr3N5QwcN8+Taj tv3FRWP23FTlUYVThlA7jCQ3A4NYbsw3mNoRUA/8Vv51g2AuAmvxGP8YzGl44pvEOHrS jbyhKIi3yGA9cQAT0/IED4pG73anb3uiOdnTBz0HVCptk6md70w+oVQG2IxuQ7CFZi6V b9gQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=uZqcUn6f; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from groat.vger.email (groat.vger.email. [23.128.96.35]) by mx.google.com with ESMTPS id mh7-20020a17090b4ac700b0027198fe5672si8817542pjb.129.2023.09.18.10.57.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Sep 2023 10:57:15 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=uZqcUn6f; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id 74D0F80965B8; Mon, 18 Sep 2023 10:01:06 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231424AbjIRRA4 (ORCPT + 25 others); Mon, 18 Sep 2023 13:00:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39606 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231414AbjIRRAn (ORCPT ); Mon, 18 Sep 2023 13:00:43 -0400 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E92273C05; Mon, 18 Sep 2023 09:35:43 -0700 (PDT) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 51EBCC433CB; Mon, 18 Sep 2023 16:35:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1695054943; bh=uBg0YwMFSNtf20J3F2FF6kMJ6k/t9J1KNWcs8W4I5o8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=uZqcUn6fqIsV6acXBorvM40I2y5nOoXsq0RstNC6b8rLOvTTmZBRCxdxrRneA4nug qrySrpUfmmF0o6+yQs/rPDHvLErLg/yNAry1Q8UpU1sOb0YaGkQEelADBYf5GuvC6V owQBZb1B6n2e+zYUOuQw3dr6vbhJcUrXwnKmUg1cvYY6660cKT187dlK7gCqUNaWrE QrPMrjhZ4vAtMq0hgRrITikTir9iJzq3mi+uQpIGmjypRgaGU2wudVl5ONVPO5Kp2z 7rGviHjMrbrKG/2/4WBFUJmyEcf4B2wExJbq+n9DzUinmta3N2gUGzz4e/KhfW8Me3 ijL9OWQIs94Sg== Received: from johan by xi.lan with local (Exim 4.96) (envelope-from ) id 1qiHE2-0002xw-2i; Mon, 18 Sep 2023 18:35:54 +0200 From: Johan Hovold To: Jiri Kosina , Benjamin Tissoires Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, Johan Hovold , Douglas Anderson , Maxime Ripard Subject: [PATCH RESEND] HID: i2c-hid: fix handling of unpopulated devices Date: Mon, 18 Sep 2023 18:35:35 +0200 Message-ID: <20230918163535.11388-1-johan+linaro@kernel.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: References: MIME-Version: 1.0 X-Spam-Status: No, score=-1.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Mon, 18 Sep 2023 10:01:06 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777399062234042314 X-GMAIL-MSGID: 1777399062234042314 A recent commit reordered probe so that the interrupt line is now requested before making sure that the device exists. This breaks machines like the Lenovo ThinkPad X13s which rely on the HID driver to probe second-source devices and only register the variant that is actually populated. Specifically, the interrupt line may now already be (temporarily) claimed when doing asynchronous probing of the touchpad: genirq: Flags mismatch irq 191. 00082008 (hid-over-i2c) vs. 00082008 (hid-over-i2c) i2c_hid_of 21-0015: Could not register for hid-over-i2c interrupt, irq = 191, ret = -16 i2c_hid_of: probe of 21-0015 failed with error -16 Fix this by restoring the old behaviour of first making sure the device exists before requesting the interrupt line. Note that something like this should probably be implemented also for "panel followers", whose actual probe is currently effectively deferred until the DRM panel is probed (e.g. by powering down the device after making sure it exists and only then register it as a follower). Fixes: 675cd877c952 ("HID: i2c-hid: Rearrange probe() to power things up later") Cc: Douglas Anderson Cc: Maxime Ripard Signed-off-by: Johan Hovold --- This one still hasn't shown up in lore so resending as a reply to Doug's reply for completeness. Johan drivers/hid/i2c-hid/i2c-hid-core.c | 142 ++++++++++++++++------------- 1 file changed, 80 insertions(+), 62 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 9601c0605fd9..e66c058a4b00 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -998,45 +998,29 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid) return hid_driver_reset_resume(hid); } -/** - * __do_i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device. - * @ihid: The ihid object created during probe. - * - * This function is called at probe time. - * - * The initial power on is where we do some basic validation that the device - * exists, where we fetch the HID descriptor, and where we create the actual - * HID devices. - * - * Return: 0 or error code. +/* + * Check that the device exists and parse the HID descriptor. */ -static int __do_i2c_hid_core_initial_power_up(struct i2c_hid *ihid) +static int __i2c_hid_core_probe(struct i2c_hid *ihid) { struct i2c_client *client = ihid->client; struct hid_device *hid = ihid->hid; int ret; - ret = i2c_hid_core_power_up(ihid); - if (ret) - return ret; - /* Make sure there is something at this address */ ret = i2c_smbus_read_byte(client); if (ret < 0) { i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret); - ret = -ENXIO; - goto err; + return -ENXIO; } ret = i2c_hid_fetch_hid_descriptor(ihid); if (ret < 0) { dev_err(&client->dev, "Failed to fetch the HID Descriptor\n"); - goto err; + return ret; } - enable_irq(client->irq); - hid->version = le16_to_cpu(ihid->hdesc.bcdVersion); hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID); hid->product = le16_to_cpu(ihid->hdesc.wProductID); @@ -1050,17 +1034,49 @@ static int __do_i2c_hid_core_initial_power_up(struct i2c_hid *ihid) ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product); + return 0; +} + +static int i2c_hid_core_register_hid(struct i2c_hid *ihid) +{ + struct i2c_client *client = ihid->client; + struct hid_device *hid = ihid->hid; + int ret; + + enable_irq(client->irq); + ret = hid_add_device(hid); if (ret) { if (ret != -ENODEV) hid_err(client, "can't add hid device: %d\n", ret); - goto err; + disable_irq(client->irq); + return ret; } return 0; +} + +static int i2c_hid_core_probe_panel_follower(struct i2c_hid *ihid) +{ + int ret; -err: + ret = i2c_hid_core_power_up(ihid); + if (ret) + return ret; + + ret = __i2c_hid_core_probe(ihid); + if (ret) + goto err_power_down; + + ret = i2c_hid_core_register_hid(ihid); + if (ret) + goto err_power_down; + + return 0; + +err_power_down: i2c_hid_core_power_down(ihid); + return ret; } @@ -1077,7 +1093,7 @@ static void ihid_core_panel_prepare_work(struct work_struct *work) * steps. */ if (!hid->version) - ret = __do_i2c_hid_core_initial_power_up(ihid); + ret = i2c_hid_core_probe_panel_follower(ihid); else ret = i2c_hid_core_resume(ihid); @@ -1156,30 +1172,6 @@ static int i2c_hid_core_register_panel_follower(struct i2c_hid *ihid) return 0; } -static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid) -{ - /* - * If we're a panel follower, we'll register and do our initial power - * up when the panel turns on; otherwise we do it right away. - */ - if (drm_is_panel_follower(&ihid->client->dev)) - return i2c_hid_core_register_panel_follower(ihid); - else - return __do_i2c_hid_core_initial_power_up(ihid); -} - -static void i2c_hid_core_final_power_down(struct i2c_hid *ihid) -{ - /* - * If we're a follower, the act of unfollowing will cause us to be - * powered down. Otherwise we need to manually do it. - */ - if (ihid->is_panel_follower) - drm_panel_remove_follower(&ihid->panel_follower); - else - i2c_hid_core_suspend(ihid, true); -} - int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, u16 hid_descriptor_address, u32 quirks) { @@ -1224,14 +1216,10 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, return ret; device_enable_async_suspend(&client->dev); - ret = i2c_hid_init_irq(client); - if (ret < 0) - goto err_buffers_allocated; - hid = hid_allocate_device(); if (IS_ERR(hid)) { ret = PTR_ERR(hid); - goto err_irq; + goto err_free_buffers; } ihid->hid = hid; @@ -1242,19 +1230,42 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, hid->bus = BUS_I2C; hid->initial_quirks = quirks; - ret = i2c_hid_core_initial_power_up(ihid); + /* Power on and probe unless device is a panel follower. */ + if (!drm_is_panel_follower(&ihid->client->dev)) { + ret = i2c_hid_core_power_up(ihid); + if (ret < 0) + goto err_destroy_device; + + ret = __i2c_hid_core_probe(ihid); + if (ret < 0) + goto err_power_down; + } + + ret = i2c_hid_init_irq(client); + if (ret < 0) + goto err_power_down; + + /* + * If we're a panel follower, we'll register when the panel turns on; + * otherwise we do it right away. + */ + if (drm_is_panel_follower(&ihid->client->dev)) + ret = i2c_hid_core_register_panel_follower(ihid); + else + ret = i2c_hid_core_register_hid(ihid); if (ret) - goto err_mem_free; + goto err_free_irq; return 0; -err_mem_free: - hid_destroy_device(hid); - -err_irq: +err_free_irq: free_irq(client->irq, ihid); - -err_buffers_allocated: +err_power_down: + if (!drm_is_panel_follower(&ihid->client->dev)) + i2c_hid_core_power_down(ihid); +err_destroy_device: + hid_destroy_device(hid); +err_free_buffers: i2c_hid_free_buffers(ihid); return ret; @@ -1266,7 +1277,14 @@ void i2c_hid_core_remove(struct i2c_client *client) struct i2c_hid *ihid = i2c_get_clientdata(client); struct hid_device *hid; - i2c_hid_core_final_power_down(ihid); + /* + * If we're a follower, the act of unfollowing will cause us to be + * powered down. Otherwise we need to manually do it. + */ + if (ihid->is_panel_follower) + drm_panel_remove_follower(&ihid->panel_follower); + else + i2c_hid_core_suspend(ihid, true); hid = ihid->hid; hid_destroy_device(hid);