From patchwork Tue Oct 31 23:31:46 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Darren Hart X-Patchwork-Id: 10035749 X-Patchwork-Delegate: dvhart@infradead.org Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 5F5DF602B5 for ; Tue, 31 Oct 2017 23:32:09 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 543DB27D4D for ; Tue, 31 Oct 2017 23:32:09 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 48E262884E; Tue, 31 Oct 2017 23:32:09 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B72ED27D4D for ; Tue, 31 Oct 2017 23:32:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932449AbdJaXby (ORCPT ); Tue, 31 Oct 2017 19:31:54 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:45803 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933278AbdJaXbv (ORCPT ); Tue, 31 Oct 2017 19:31:51 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Transfer-Encoding :Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Sender:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help: List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=t4YmkZjvw5kPjm2v/S8Xca/jTUkUb+GOx3OUmQdnxGQ=; b=i3JR1sGs5wwuMb8bXGjO89h1TY xXD8+06wplyvdP5zln0zacy5hyAI/4nlqmImpO0dz42Q1V0Fe/DeqyN9epzAT/D2DHdcwns+C8050 VVym+g4/o9ML1szxmekgT0ptFtzQiB/GVq7Es2/HIXeIq873cgxaH2j3EqZL8jQldH4SjzKaOxqOm qnd7mj0ubNBWqJHP/qXYadUcKa7jFswAheMB7gnoLqaF/xctKI21ckTp9vXQmalMLon1/81iRhmmE rVxLMpaBondOrj3SdXRPhRywiB/dzp12ii+OiZk/GZtuI10741Eae7DIiyGuJnUZ7j0vADvO7C840 QBdyTpdQ==; Received: from dvhart by bombadil.infradead.org with local (Exim 4.87 #1 (Red Hat Linux)) id 1e9g0Z-0002NP-Gi; Tue, 31 Oct 2017 23:31:47 +0000 Date: Tue, 31 Oct 2017 16:31:46 -0700 From: Darren Hart To: Mario.Limonciello@dell.com Cc: pali.rohar@gmail.com, andy.shevchenko@gmail.com, linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org, luto@kernel.org, quasisec@google.com, rjw@rjwysocki.net, mjg59@google.com, hch@lst.de, greg@kroah.com, gnomes@lxorguk.ukuu.org.uk Subject: Re: [PATCH v11 05/15] platform/x86: dell-wmi-descriptor: split WMI descriptor into it's own driver Message-ID: <20171031233146.GA5787@fury> References: <20171030114652.kopuffflshq2ates@pali> <27e2d166f2ce461fb025ebf06a284d01@ausx13mpc124.AMER.DELL.COM> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <27e2d166f2ce461fb025ebf06a284d01@ausx13mpc124.AMER.DELL.COM> User-Agent: Mutt/1.8.0 (2017-02-23) Sender: platform-driver-x86-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: platform-driver-x86@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Oct 30, 2017 at 01:32:57PM +0000, Mario.Limonciello@dell.com wrote: > > > > -----Original Message----- > > From: Pali Rohár [mailto:pali.rohar@gmail.com] > > Sent: Monday, October 30, 2017 6:47 AM > > To: Limonciello, Mario > > Cc: dvhart@infradead.org; Andy Shevchenko ; > > LKML ; platform-driver-x86@vger.kernel.org; Andy > > Lutomirski ; quasisec@google.com; rjw@rjwysocki.net; > > mjg59@google.com; hch@lst.de; Greg KH ; Alan Cox > > > > Subject: Re: [PATCH v11 05/15] platform/x86: dell-wmi-descriptor: split WMI > > descriptor into it's own driver > > > > On Friday 20 October 2017 12:40:20 Mario Limonciello wrote: > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.c > > b/drivers/platform/x86/dell-wmi-descriptor.c > > > new file mode 100644 > > > index 000000000000..3204c408e261 > > > --- /dev/null > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.c > > > > This dell-wmi-descriptor.c looks good now! > > > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.h > > b/drivers/platform/x86/dell-wmi-descriptor.h > > > new file mode 100644 > > > index 000000000000..5f7b69c2c83a > > > --- /dev/null > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.h > > > @@ -721,7 +652,9 @@ static int dell_wmi_events_set_enabled(bool enable) > > > static int dell_wmi_probe(struct wmi_device *wdev) > > > { > > > struct dell_wmi_priv *priv; > > > - int err; > > > + > > > + if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID)) > > > + return -ENODEV; > > > > > > priv = devm_kzalloc( > > > &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL); > > > @@ -729,9 +662,8 @@ static int dell_wmi_probe(struct wmi_device *wdev) > > > return -ENOMEM; > > > dev_set_drvdata(&wdev->dev, priv); > > > > > > - err = dell_wmi_check_descriptor_buffer(wdev); > > > - if (err) > > > - return err; > > > + if (!dell_wmi_get_interface_version(&priv->interface_version)) > > > + return -EPROBE_DEFER; > > > > But here is still a problem. You added check that > > DELL_WMI_DESCRIPTOR_GUID exists in APCI table, but it does not mean that > > probe method of dell-wmi-descriptor cannot fail. > > > > With PROBE_DEFER, dell_wmi_probe function would be called later again > > and again, even when probing dell-wmi-descriptor failed and so dell-wmi > > in this case cannot work. > > > > Yes it's possible that probe method can fail, but it depends on the reason for > failure if it will fail again later. For example if not enough memory, it may work > later. Or maybe user manually unbound from GUID, should continue to try until > it's bound again. > > So in short, I believe this is the correct behavior to adopt. In this case, I believe the synchronous request_module("dell-wmi-descriptor") would provide the desired result. The exit status doesn't even need to be checked. If that is successful, and the interface_version still returns false, then it can be considered an error and we can exit. If it fails, the interface_version will return false, and it is also an error. This can be easily added as a single patch on top of this series: Pali, I believe this addresses your concern? diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c index dcfa5de..964ca54 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -665,8 +665,9 @@ static int dell_wmi_probe(struct wmi_device *wdev) return -ENOMEM; dev_set_drvdata(&wdev->dev, priv); + request_module("dell-wmi-descriptor"); if (!dell_wmi_get_interface_version(&priv->interface_version)) - return -EPROBE_DEFER; + return -ENODEV; return dell_wmi_input_setup(wdev); }