From patchwork Sat Sep 23 09:08:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jonathan Woithe X-Patchwork-Id: 9967421 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 B81D560353 for ; Sat, 23 Sep 2017 09:09:29 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A9A0E28C1A for ; Sat, 23 Sep 2017 09:09:29 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9B5BD28AEB; Sat, 23 Sep 2017 09:09:29 +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.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham 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 7E09828AEB for ; Sat, 23 Sep 2017 09:09:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751465AbdIWJJ0 (ORCPT ); Sat, 23 Sep 2017 05:09:26 -0400 Received: from server.atrad.com.au ([150.101.241.2]:49064 "EHLO server.atrad.com.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751149AbdIWJJX (ORCPT ); Sat, 23 Sep 2017 05:09:23 -0400 Received: from marvin.atrad.com.au (IDENT:1008@marvin.atrad.com.au [192.168.0.2]) by server.atrad.com.au (8.15.2/8.14.9) with ESMTPS id v8N98egZ019963 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sat, 23 Sep 2017 18:38:42 +0930 Date: Sat, 23 Sep 2017 18:38:40 +0930 From: Jonathan Woithe To: Darren Hart Cc: Ville Syrjala , platform-driver-x86@vger.kernel.org, Andy Shevchenko Subject: Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt Message-ID: <20170923090839.GC21630@marvin.atrad.com.au> References: <20170918200059.16279-1-ville.syrjala@linux.intel.com> <20170923000048.GC20327@fury> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170923000048.GC20327@fury> User-Agent: Mutt/1.6.1 (2016-04-27) X-MIMEDefang-action: accept X-Scanned-By: MIMEDefang 2.79 on 192.168.0.1 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 Hi Darren The more detailed follow-up, as promised. :-) Sorry for the delay. On Fri, Sep 22, 2017 at 05:00:48PM -0700, Darren Hart wrote: > On Mon, Sep 18, 2017 at 11:00:59PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > My Fujitsu-Siemens Lifebook S6120 doesn't have the FUJ02E3 device, > > but it does have FUJ02B1. That means we do register the backlight > > device (and it even seems to work), but the code will oops as soon > > as we try to set the backlight brightness because it's trying to > > I'm curious by what you mean with "it even seems to work". Since it > crashes when adjusting, what does it do that "works" ? As mentioned earlier, I have assumed that this means the backlight adjustment works correctly if the suggested patch has been applied. Having thought about this some more I am unconvinced: if fext is NULL then call_fext_func() (with the check added) can't have any effect. The backlight adjustment might still work, but it's not due to this code path. > > call call_fext_func() with a NULL device. Let's just skip those > > function calls when the FUJ02E3 device is not present. > > Interesting. We call call_fext_func from many locations using the > "device" argument, or the driver static "fext". > > This looks to me that we should be a bit more consistent here. Tidying this up further is the aim of Michal's ongoing cleanup work which was to lead in to the splitting of the driver into two separate modules (one for each ACPI device). However, this plan was predicated on the idea that all fujitsu laptops would have the FUJ02E3 device while some also included the FUJ02B1 backlight device. Ville's report about the S6120 demonstrates that this assumption is wrong, so the approach for splitting the driver needs to be revisited (and may turn out to be much trickier than first thought). Michal has indicated that he is giving it consideration. > Finally, it seems a proper fix would be to either not register the > backlight device if !fext or to check for !fext inside call_fext_func. According to my initial understanding of Ville's report, it suggests that his S6120 needs the FUJ02B1 backlight device registered even though there's no FUJ02E3 (fext) in his system. This means that not registering the backlight device in the absence of FUJ02E3 is no good: the S6120 needs the backlight device anyway. However, this understanding might be wrong, which means enforcing the requirement that FUJ02E3 exists before FUJ02B1 might be an option. Of course we have no way of knowing whether this might trip up some other variation of the hardware. Checking inside call_fext_func was the alternative approach that Michal suggested. Either would work and I would be happy for either. Due to the ongoing restructuring work that Michal's doing I thought that keeping the test local to the only current user of fext might make things easier, but I certainly have no firm preference either way. Since the proposed patch is known to work I figured we could run with that for the moment in order to address what is effectively a regression. The "proper fix" (whatever form that ends up taking) could then just be a part of Michel's future clean-up patch series. However, if you feel that going straight for a call_fext_func() fix is better then we could do that: Signed-off-by: Jonathan Woithe As Michal suggested, this approach does have the advantage of protecting against other unforeseen hardware arrangements in the future and therefore might be the better solution. Given the proven unpredictable nature of the hardware arrangements in this laptop series I would be more comfortable at this stage with one of the two proposed NULL device checks: either the one Ville suggested or the test in call_fext_func(). Making assumptions about precisely which devices might be present (which is what the "FUJ02E3 exists" test would do) is is what tripped us up in this case. Regards jonathan --- a/drivers/platform/x86/fujitsu-laptop.c 2017-09-23 18:24:47.258058706 +0930 +++ b/drivers/platform/x86/fujitsu-laptop.c 2017-09-23 18:27:24.938052251 +0930 @@ -153,6 +153,10 @@ static int call_fext_func(struct acpi_de unsigned long long value; acpi_status status; + if (!device) { + return 0; + } + status = acpi_evaluate_integer(device->handle, "FUNC", &arg_list, &value); if (ACPI_FAILURE(status)) {