[PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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
call call_fext_func() with a NULL device. Let's just skip those
function calls when the FUJ02E3 device is not present.

Cc: Jonathan Woithe <jwoithe@just42.net>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/platform/x86/fujitsu-laptop.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 85de30f93a9c..56a8195096a2 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -254,10 +254,12 @@ static int bl_update_status(struct backlight_device *b)
 {
 	struct acpi_device *device = bl_get_data(b);
 
-	if (b->props.power == FB_BLANK_POWERDOWN)
-		call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
-	else
-		call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
+	if (fext) {
+		if (b->props.power == FB_BLANK_POWERDOWN)
+			call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
+		else
+			call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
+	}
 
 	return set_lcd_level(device, b->props.brightness);
 }
-- 
2.13.5

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Michał Kępień]

> 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
> call call_fext_func() with a NULL device. Let's just skip those
> function calls when the FUJ02E3 device is not present.

Oh, the irony...  I have literally just sat down to dust off the patch
series I have had queued for the past couple of months which makes the
driver for FUJ02B1 a separate module depending on the FUJ02E3 driver.
Then I saw your email and it is now clear that such a dependency is
bogus.  AFAIR, until now everyone involved was expecting the FUJ02E3
ACPI device to be present on all Fujitsu laptops.

I need to refresh my memory a bit and think about next steps.  I still
have one more cleanup series queued that is worth posting and I also
need to fix radio LED detection.  My spare time is still very limited,
but I will do my best to post something soon.

-- 
Best regards,
Michał Kępień

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Jonathan Woithe]

Hi Michal

On Tue, Sep 19, 2017 at 06:06:48AM +0200, Micha?? K??pie?? wrote:
> > 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
> > call call_fext_func() with a NULL device. Let's just skip those
> > function calls when the FUJ02E3 device is not present.
> 
> Oh, the irony...  I have literally just sat down to dust off the patch
> series I have had queued for the past couple of months which makes the
> driver for FUJ02B1 a separate module depending on the FUJ02E3 driver.
> Then I saw your email and it is now clear that such a dependency is
> bogus.  AFAIR, until now everyone involved was expecting the FUJ02E3
> ACPI device to be present on all Fujitsu laptops.

Yes, this was the general concensus (although admittedly it was based on a
relatively small sample of Fujitsu models).

> I need to refresh my memory a bit and think about next steps.  I still
> have one more cleanup series queued that is worth posting and I also
> need to fix radio LED detection.  My spare time is still very limited,
> but I will do my best to post something soon.

In the meantime, I think we ought to consider pushing the proposed patch in
order to restore functionality for the S6120 - as it stands the code oopses,
which isn't great.  Is there any reason not to do so?

Regards
  jonathan

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Michał Kępień]

> > I need to refresh my memory a bit and think about next steps.  I still
> > have one more cleanup series queued that is worth posting and I also
> > need to fix radio LED detection.  My spare time is still very limited,
> > but I will do my best to post something soon.
> 
> In the meantime, I think we ought to consider pushing the proposed patch in
> order to restore functionality for the S6120 - as it stands the code oopses,
> which isn't great.  Is there any reason not to do so?

It might only be worth considering to check whether device is NULL
inside call_fext_func() in order to prevent any further issues like this
in the future.  However, for now bl_update_status() is the only place in
fujitsu-laptop using the fext module-wide variable, so the patch is okay
as it is, too.

-- 
Best regards,
Michał Kępień

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Jonathan Woithe]

On Tue, Sep 19, 2017 at 06:42:24AM +0200, Micha?? K??pie?? wrote:
> > > I need to refresh my memory a bit and think about next steps.  I still
> > > have one more cleanup series queued that is worth posting and I also
> > > need to fix radio LED detection.  My spare time is still very limited,
> > > but I will do my best to post something soon.
> > 
> > In the meantime, I think we ought to consider pushing the proposed patch in
> > order to restore functionality for the S6120 - as it stands the code oopses,
> > which isn't great.  Is there any reason not to do so?
> 
> It might only be worth considering to check whether device is NULL
> inside call_fext_func() in order to prevent any further issues like this
> in the future.  However, for now bl_update_status() is the only place in
> fujitsu-laptop using the fext module-wide variable, so the patch is okay
> as it is, too.

I like the idea of addressing this locally in bl_update_status() as per the
suggested patch.  That may make further cleanup work easier since the module
wide variable was something which was to be possibly removed (although this
S6120 may force a rethink of these plans as you mentioned earlier).  Having
said that, guarding against further bugs like this is tempting.

Since the proposed fix addresses the problem I'm happy for it to be pushed
out as is:

Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

That will at least stop the oops in the interim while the remaining clean up
patch is prepared.  Addressing this problem permanently could be something
that's rolled into that clean up work if it is deemed significant enough.

Darren, Andy: can we push this fix?  It is probably something which should
go to -stable too.

Regards
  jonathan

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Ville Syrjälä]

On Tue, Sep 19, 2017 at 06:06:48AM +0200, Michał Kępień wrote:
> > 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
> > call call_fext_func() with a NULL device. Let's just skip those
> > function calls when the FUJ02E3 device is not present.
> 
> Oh, the irony...  I have literally just sat down to dust off the patch
> series I have had queued for the past couple of months which makes the
> driver for FUJ02B1 a separate module depending on the FUJ02E3 driver.
> Then I saw your email and it is now clear that such a dependency is
> bogus.  AFAIR, until now everyone involved was expecting the FUJ02E3
> ACPI device to be present on all Fujitsu laptops.

BTW I just double checked my other laptops; S6010 doesn't have FUJ02E3
either, but S7110 does have it. So I have machines that go both ways,
in case you need help with testing something.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Darren Hart]

On Mon, Sep 18, 2017 at 11:00:59PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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" ?

> 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.

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.

Jonathan, your thoughts?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Jonathan Woithe]

Hi Darren

A quick reply since I have to go somewhere in a second.  I'll follow up in
more detail later.

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ä <ville.syrjala@linux.intel.com>
> > 
> > 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" ?

I interpreted this to mean that things work with the patch in place.

jonathan

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Jonathan Woithe]

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ä <ville.syrjala@linux.intel.com>
> > 
> > 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:

--- 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)) {

Signed-off-by: Jonathan Woithe <jwoithe@just42.net>

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

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Ville Syrjälä]

On Sat, Sep 23, 2017 at 06:38:40PM +0930, Jonathan Woithe wrote:
> 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ä <ville.syrjala@linux.intel.com>
> > > 
> > > 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.

Yes.

> 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.

It still calls set_lcd_level() which I assume is the thing that makes it
work.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Jonathan Woithe]

On Mon, Sep 25, 2017 at 03:14:41PM +0300, Ville Syrjälä wrote:
> On Sat, Sep 23, 2017 at 06:38:40PM +0930, Jonathan Woithe wrote:
> > > 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.
> 
> Yes.

Great - thanks for confirming.

> > 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.
> 
> It still calls set_lcd_level() which I assume is the thing that makes it
> work.

Yes, of course.  Thanks, I was overthinking things. :-)

Darren: this confirms that the idea of only registering FUJ02B1 if FUJ02E3
exists is no good: the S6120 (and probably others) needs FUJ02B1 but doesn't
have a FUJ02E3.  This means that at least for the purposes of addressing
this regression, the null device check is the best option: either in
bl_update_status() (Ville's original patch) or in call_fext_func() (as
suggested by Michel, with a suggested patch in my post on the 24th).

Regards
  jonathan

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Michał Kępień]

> > 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.

This is intentional.  The plan was to extract the backlight part of
fujitsu-laptop into a separate module, fujitsu-backlight, which would
use a simple two-function API exposed by fujitsu-laptop to control LCD
backlight power.  Those functions would have to somehow access the
FUJ02E3 ACPI device while not being ACPI callbacks.  Thus, fext is
storing a pointer to the last (and most likely only) instantiated
FUJ02E3 ACPI device so that it can be used for setting LCD backlight
power from fujitsu-backlight.  See commit ca0d9eab0fb5 for a brief
discussion of why this solution was chosen.

> 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.

My draft patch series which splits off fujitsu-backlight includes the
NULL check for fext inside the functions exposed by fujitsu-laptop.
Sadly, I have not got round to submitting it yet.

Just to make sure we are all on the same page here, please note that
access to FUJ02E3 is only needed for controlling LCD backlight _power_,
not LCD backlight _brightness_.

Speaking of which, I just noticed that my S7020 can control its LCD
brightness just fine without fujitsu-laptop being loaded.  Heck, it even
works when booted with "noacpi".  It seems to me that on this model, LCD
brightness control works at the firmware level and an ACPI-based driver
is just another possible way of getting/setting LCD brightness level.
Jonathan, IIRC you have an S7020 as well, could you please test that?
You know better than me why this driver was needed in the first place.

Ville, could you please test your S6120 for the above as well?  Please
try unloading/blacklisting fujitsu-laptop and see whether LCD backlight
control still works.

Finally, it seems the S6120 also has a row of "application panel"
hotkeys above the keyboard, but lacks the FUJ02E3 ACPI device, so I am
wondering whether these hotkeys work in Linux.  Ville, do they?  In
fact, it would be super helpful if you could post acpidumps from all
three models you have access to (you mentioned S6120, S6010 and S7110).
IMHO the biggest issue we have had while refactoring fujitsu-laptop is a
narrow set of known/tested models.

-- 
Best regards,
Michał Kępień

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Jonathan Woithe]

Hi Michal

On Tue, Sep 26, 2017 at 06:49:08AM +0200, Micha?? K??pie?? wrote:
> > 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.
> 
> My draft patch series which splits off fujitsu-backlight includes the
> NULL check for fext inside the functions exposed by fujitsu-laptop.
> Sadly, I have not got round to submitting it yet.

It's probably similar to the patch I proposed earlier this week as an
alterative to the one posted by Ville.  I think we should push at least one
of these fixes out as soon as possible because we do currently have a
regression as a result of the oversight in the current code.  This would
then allow the draft patch series to be completed when you have the time,
rather than rushing it through now.

> Speaking of which, I just noticed that my S7020 can control its LCD
> brightness just fine without fujitsu-laptop being loaded.  Heck, it even
> works when booted with "noacpi".  It seems to me that on this model, LCD
> brightness control works at the firmware level and an ACPI-based driver
> is just another possible way of getting/setting LCD brightness level.
> Jonathan, IIRC you have an S7020 as well, could you please test that?
> You know better than me why this driver was needed in the first place.

You're really testing my memory now. :-)

My recollection is that back in the day the brightness buttons did work
without fujitsu-laptop loaded, so your report here doesn't suprise me. 
However, the fujitsu-laptop driver was required to make it possible for
*software* to control the LCD brightness and power.  Like you, I think I
concluded at the time that the hardware buttons worked at the firmware level
and the OS had no opportunity to participate in the activity.  There may
have also been a suspend-to-ram issue with LCD power which the driver helped
with, but I would have to go back through my notes from the time to see if
this was really the case.

To summarise, the driver was originally written to provide a way for Linux
to interact with the LCD so the brightness and power could be controlled by
Linux, and so Linux could find out what the LCD state was.

Regards
  jonathan

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Darren Hart]

On Mon, Sep 18, 2017 at 11:00:59PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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
> call call_fext_func() with a NULL device. Let's just skip those
> function calls when the FUJ02E3 device is not present.
> 
> Cc: Jonathan Woithe <jwoithe@just42.net>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for all the discussion on this patch. While there are a couple approaches
we can take, it is clear further clean up and refactoring is in order. To fix
the Ooops now, I will apply this patch to fixes and Cc Stable.

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Jonathan Woithe]

Hi Darren

On Tue, Sep 26, 2017 at 11:56:55PM -0700, Darren Hart wrote:
> On Mon, Sep 18, 2017 at 11:00:59PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 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
> > call call_fext_func() with a NULL device. Let's just skip those
> > function calls when the FUJ02E3 device is not present.
> > 
> > Cc: Jonathan Woithe <jwoithe@just42.net>
> > Cc: Darren Hart <dvhart@infradead.org>
> > Cc: Andy Shevchenko <andy@infradead.org>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Thanks for all the discussion on this patch. While there are a couple approaches
> we can take, it is clear further clean up and refactoring is in order. To fix
> the Ooops now, I will apply this patch to fixes and Cc Stable.

Thanks Darren.

jonathan

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Ville Syrjälä]

On Wed, Oct 18, 2017 at 06:10:58PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 26, 2017 at 06:49:08AM +0200, Michał Kępień wrote:
> > > > 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.
> > 
> > This is intentional.  The plan was to extract the backlight part of
> > fujitsu-laptop into a separate module, fujitsu-backlight, which would
> > use a simple two-function API exposed by fujitsu-laptop to control LCD
> > backlight power.  Those functions would have to somehow access the
> > FUJ02E3 ACPI device while not being ACPI callbacks.  Thus, fext is
> > storing a pointer to the last (and most likely only) instantiated
> > FUJ02E3 ACPI device so that it can be used for setting LCD backlight
> > power from fujitsu-backlight.  See commit ca0d9eab0fb5 for a brief
> > discussion of why this solution was chosen.
> > 
> > > 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.
> > 
> > My draft patch series which splits off fujitsu-backlight includes the
> > NULL check for fext inside the functions exposed by fujitsu-laptop.
> > Sadly, I have not got round to submitting it yet.
> > 
> > Just to make sure we are all on the same page here, please note that
> > access to FUJ02E3 is only needed for controlling LCD backlight _power_,
> > not LCD backlight _brightness_.
> > 
> > Speaking of which, I just noticed that my S7020 can control its LCD
> > brightness just fine without fujitsu-laptop being loaded.  Heck, it even
> > works when booted with "noacpi".  It seems to me that on this model, LCD
> > brightness control works at the firmware level and an ACPI-based driver
> > is just another possible way of getting/setting LCD brightness level.
> > Jonathan, IIRC you have an S7020 as well, could you please test that?
> > You know better than me why this driver was needed in the first place.
> > 
> > Ville, could you please test your S6120 for the above as well?  Please
> > try unloading/blacklisting fujitsu-laptop and see whether LCD backlight
> > control still works.
> 
> My recollection is that the Fn+brightness works even without the
> driver. But I'll try to retest that tonight since I'm not 100% sure.

Indeed that is the case.

> 
> > 
> > Finally, it seems the S6120 also has a row of "application panel"
> > hotkeys above the keyboard, but lacks the FUJ02E3 ACPI device, so I am
> > wondering whether these hotkeys work in Linux.  Ville, do they?
> 
> They work via the apanel driver. Well, there's buttons 1-4 and Enter.
> Buttons 1-4 work but the Enter doesn't, but that could be just an
> oversight in the apanel driver. That's at least how the S6010 works.
> I'll have to retest the S6120 to make sure it behaves the same way.
> At least it has the same set of buttons on it.

Looks like S6120 works the same way, via the apanel driver.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Michał Kępień]

> > Finally, it seems the S6120 also has a row of "application panel"
> > hotkeys above the keyboard, but lacks the FUJ02E3 ACPI device, so I am
> > wondering whether these hotkeys work in Linux.  Ville, do they?
> 
> They work via the apanel driver.

Oops, then it looks like we have two drivers which poke the same
hardware at the same time using different methods: apanel uses SMBus,
fujitsu-laptop uses ACPI (the former is inferior as it resorts to
polling, which causes key presses to be reported with a delay).  I just
loaded both drivers on my Fujitsu Lifebook S7020 and it results in
duplicate events being passed to userspace.  We should probably prevent
this, but I am not quite sure how to achieve that.

Darren, Andy, do you have any thoughts on this?

> Well, there's buttons 1-4 and Enter.
> Buttons 1-4 work but the Enter doesn't, but that could be just an
> oversight in the apanel driver.

No, this is the way it is supposed to work.  Or at least fujitsu-laptop
does not react to the Enter key being pressed either.

> That's at least how the S6010 works.
> I'll have to retest the S6120 to make sure it behaves the same way.
> At least it has the same set of buttons on it.
> 
> > In
> > fact, it would be super helpful if you could post acpidumps from all
> > three models you have access to (you mentioned S6120, S6010 and S7110).
> > IMHO the biggest issue we have had while refactoring fujitsu-laptop is a
> > narrow set of known/tested models.
> 
> I've attached acpidumps and dmidecode outputs for all three machines.

Thank you!  This helps a lot and will definitely help preventing similar
issues in the future.

-- 
Best regards,
Michał Kępień

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Ville Syrjälä]

On Wed, Oct 25, 2017 at 06:51:31AM +0200, Michał Kępień wrote:
> > > Finally, it seems the S6120 also has a row of "application panel"
> > > hotkeys above the keyboard, but lacks the FUJ02E3 ACPI device, so I am
> > > wondering whether these hotkeys work in Linux.  Ville, do they?
> > 
> > They work via the apanel driver.
> 
> Oops, then it looks like we have two drivers which poke the same
> hardware at the same time using different methods: apanel uses SMBus,
> fujitsu-laptop uses ACPI (the former is inferior as it resorts to
> polling, which causes key presses to be reported with a delay).  I just
> loaded both drivers on my Fujitsu Lifebook S7020 and it results in
> duplicate events being passed to userspace.  We should probably prevent
> this, but I am not quite sure how to achieve that.

Maybe in apanel probe?

#if ENABLED(FUJITSU_LAPTOP)
	if (ACPI thing available)
		fail;
#endif

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Jonathan Woithe]

On Wed, Oct 25, 2017 at 06:51:31AM +0200, Micha?? K??pie?? wrote:
> > > Finally, it seems the S6120 also has a row of "application panel"
> > > hotkeys above the keyboard, but lacks the FUJ02E3 ACPI device, so I am
> > > wondering whether these hotkeys work in Linux.  Ville, do they?
> > 
> > They work via the apanel driver.
> 
> Oops, then it looks like we have two drivers which poke the same
> hardware at the same time using different methods: apanel uses SMBus,
> fujitsu-laptop uses ACPI (the former is inferior as it resorts to
> polling, which causes key presses to be reported with a delay).  I just
> loaded both drivers on my Fujitsu Lifebook S7020 and it results in
> duplicate events being passed to userspace.  We should probably prevent
> this, but I am not quite sure how to achieve that.

That's a bit tricky.  Evidently some models need the SMBus approach because
they lack the FUJ02E3 device.  However, on systems with the FUJ02E3 the
ACPI method is clearly desirable.  Should apanel disable itself on systems
where an FUJ02E3 is present (although I'm not sure how precisely to arrange
for this to happen)?  Then again, this would only be feasible if the ACPI
method can handle all application panel variations which are seen across the
various models.

Regards
  jonathan

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Michał Kępień]

> > 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
> > call call_fext_func() with a NULL device. Let's just skip those
> > function calls when the FUJ02E3 device is not present.
> 
> Oh, the irony...  I have literally just sat down to dust off the patch
> series I have had queued for the past couple of months which makes the
> driver for FUJ02B1 a separate module depending on the FUJ02E3 driver.
> Then I saw your email and it is now clear that such a dependency is
> bogus.  AFAIR, until now everyone involved was expecting the FUJ02E3
> ACPI device to be present on all Fujitsu laptops.
> 
> I need to refresh my memory a bit and think about next steps. 

This comment of mine seems a bit too hotheaded in retrospect.  The fact
that we cannot assume that FUJ02E3 always exists when FUJ02B1 exists
does not prevent us from exposing a function from a module centered
around the former to the module centered around the latter.  That
function would just need to correctly handle a missing FUJ02E3 ACPI
device.  Thus, I am still supportive of splitting the module into two.

> I still
> have one more cleanup series queued that is worth posting and I also
> need to fix radio LED detection.  My spare time is still very limited,
> but I will do my best to post something soon.

The aforementioned patch series is still queued on my disk, but needs a
bit more polishing before I can post it.  This is just a "keepalive"
message that I have not given up on it.

-- 
Best regards,
Michał Kępień

Re: [PATCH] platform/x86: fujitsu-laptop: Don't oops when FUJ02E3 is not presnt

[Michał Kępień]

> > > > Finally, it seems the S6120 also has a row of "application panel"
> > > > hotkeys above the keyboard, but lacks the FUJ02E3 ACPI device, so I am
> > > > wondering whether these hotkeys work in Linux.  Ville, do they?
> > > 
> > > They work via the apanel driver.
> > 
> > Oops, then it looks like we have two drivers which poke the same
> > hardware at the same time using different methods: apanel uses SMBus,
> > fujitsu-laptop uses ACPI (the former is inferior as it resorts to
> > polling, which causes key presses to be reported with a delay).  I just
> > loaded both drivers on my Fujitsu Lifebook S7020 and it results in
> > duplicate events being passed to userspace.  We should probably prevent
> > this, but I am not quite sure how to achieve that.
> 
> That's a bit tricky.  Evidently some models need the SMBus approach because
> they lack the FUJ02E3 device.  However, on systems with the FUJ02E3 the
> ACPI method is clearly desirable.  Should apanel disable itself on systems
> where an FUJ02E3 is present (although I'm not sure how precisely to arrange
> for this to happen)?  Then again, this would only be feasible if the ACPI
> method can handle all application panel variations which are seen across the
> various models.

Darren, Andy,

This is still an outstanding issue that I do not quite know how to deal
with properly (if there is a proper way at all).  How do you think this
should be handled?

There are two drivers in different subsystems that are stepping on each
other's toes: input/misc/apanel.c and platform/x86/fujitsu-laptop.c.
Testing confirmed that:

  - duplicate input events are generated when both drivers are loaded
    (or statically compiled in) and a hotkey is pressed,

  - when both drivers are compiled as modules, they are both
    automatically loaded upon boot.

One driver should be preferred over the other (fujitsu-laptop over
apanel), but using the preferred driver is only possible when a specific
ACPI device is present.

Any thoughts and/or hints how to handle this would be appreciated.

-- 
Best regards,
Michał Kępień