[PATCH 0/4] fujitsu_init() cleanup

[PATCH 0/4] fujitsu_init() cleanup

[Michał Kępień]

These patches should make fujitsu_init() a bit more palatable.  I
intentionally stopped after four patches, because they should do no harm
and the next steps require some discussion.

The problem I have with this driver is that it is generally oblivious to
the user's chosen backlight provider.  It was written before acpi_video
support for backlight control was automatically detected by the kernel
and as such it required a fix which was allegedly provided by
7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is
serving this functionality").  However, that fix was superficial as the
module still registered its vendor-specific ACPI driver for backlight
control.  That in turn caused SBLL/SBL2 to be called when brightness
hotkeys were pressed even when acpi_video was supposed to handle
brightness changes, which is exactly what that patch was hoping to
prevent.  Moreover, the module unconditionally exports backlight-related
sysfs attributes which enable userspace to change the brightness level,
which should also only be possible when vendor backlight control is
used.  Fast forward several years and on modern Fujitsu machines (e.g.
Lifebook E744), the kernel automatically detects that native backlight
control [1] should be used and SBLL becomes a noop [2].  This creates
confusion, because the driver claims that it can get/set LCD brightness
level via the platform device's sysfs attributes, but it actually
cannot.  It gets even more confusing on Skylake machines on which the
FUJ02B1 ACPI device is not present at all.

The proposal I put forward in regard to the above is to remove the three
backlight-related attributes (brightness_changed, lcd_level,
max_brightness) from the platform driver's sysfs tree.  I believe the
functions they implement should only be exposed through the backlight
device, because the latter is only created when the user explicitly
selects vendor backlight control or it is automatically selected by the
kernel (this should be the case for older machines).

That would leave us with the remaining three sysfs attributes of the
platform device, namely dock, lid and radios.  These all depend on the
FUJ02E3 ACPI device.  Which begs the question: shall we reassign them to
that ACPI device and drop the platform device altogether?  This would
logically be the correct thing to do (panasonic-laptop and toshiba_acpi
already assign extra sysfs attributes to ACPI nodes).  But I understand
that this would break an 8-year-old userspace interface as functions
previously exposed through /sys/devices/platform/fujitsu-laptop would be
moved to /sys/bus/acpi/devices/FUJ02E3:00.  If that is unacceptable, the
least we can (and should) do is to move platform device registration to
acpi_fujitsu_hotkey_add().  What the driver currently does may create
confusion in the future, because the platform device is registered
unconditionally while it clearly depends on FUJ02E3 being present.  I do
not know whether FUJ02E3 is present on all Fujitsu devices today without
exception, but I do know that if Fujitsu ever decides to drop that
device from its firmware, we would again (see above) expose a userspace
interface (dock, lid, radios) which simply will not be able to function
properly.

I am happy to provide patches for whatever solution gets chosen, but
first I would really appreciate if you (and anyone interested) could
comment on the issues I described above.  Thanks!

[1] This means the kernel does not handle backlight brightness changes
    on its own but instead delegates this task to userspace and only
    feeds it input events which indicate the user requested a brightness
    change.  For more information, see fbc9fe1b4f22 ("ACPI / video: Do
    not register backlight if win8 and native interface exists").

[2] It can be invoked, but it does not change screen brightness.

 drivers/platform/x86/fujitsu-laptop.c | 98 ++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 48 deletions(-)

-- 
2.11.0

[PATCH 1/4] platform/x86: fujitsu-laptop: simplify acpi_bus_register_driver() error handling

[Michał Kępień]

A separate variable is not needed to handle error codes returned by
acpi_bus_register_driver().  If the latter fails, just use the value it
returned as the value returned by fujitsu_init().

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index b725a907a91f..9f62d9e1b0de 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -1176,7 +1176,7 @@ MODULE_DEVICE_TABLE(acpi, fujitsu_ids);
 
 static int __init fujitsu_init(void)
 {
-	int ret, result, max_brightness;
+	int ret, max_brightness;
 
 	if (acpi_disabled)
 		return -ENODEV;
@@ -1191,11 +1191,9 @@ static int __init fujitsu_init(void)
 	fujitsu->keycode5 = KEY_RFKILL;
 	dmi_check_system(fujitsu_dmi_table);
 
-	result = acpi_bus_register_driver(&acpi_fujitsu_driver);
-	if (result < 0) {
-		ret = -ENODEV;
+	ret = acpi_bus_register_driver(&acpi_fujitsu_driver);
+	if (ret)
 		goto fail_acpi;
-	}
 
 	/* Register platform stuff */
 
@@ -1248,11 +1246,9 @@ static int __init fujitsu_init(void)
 		goto fail_hotkey;
 	}
 
-	result = acpi_bus_register_driver(&acpi_fujitsu_hotkey_driver);
-	if (result < 0) {
-		ret = -ENODEV;
+	ret = acpi_bus_register_driver(&acpi_fujitsu_hotkey_driver);
+	if (ret)
 		goto fail_hotkey1;
-	}
 
 	/* Sync backlight power status (needs FUJ02E3 device, hence deferred) */
 	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-- 
2.11.0

[PATCH 2/4] platform/x86: fujitsu-laptop: register backlight device in a separate function

[Michał Kępień]

Move code responsible for backlight device registration to a separate
function in order to simplify error handling and decrease indentation.
Simplify initialization of struct backlight_properties.  Use
KBUILD_MODNAME as device name to avoid repeating the same string literal
throughout the module.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 38 ++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 9f62d9e1b0de..df1cd0a0231d 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -1174,9 +1174,28 @@ static const struct acpi_device_id fujitsu_ids[] __used = {
 };
 MODULE_DEVICE_TABLE(acpi, fujitsu_ids);
 
+static int __init fujitsu_backlight_init(void)
+{
+	struct backlight_properties props = {
+		.brightness = fujitsu->brightness_level,
+		.max_brightness = fujitsu->max_brightness - 1,
+		.type = BACKLIGHT_PLATFORM
+	};
+	struct backlight_device *bd;
+
+	bd = backlight_device_register(KBUILD_MODNAME, NULL, NULL,
+				       &fujitsubl_ops, &props);
+	if (IS_ERR(bd))
+		return PTR_ERR(bd);
+
+	fujitsu->bl_device = bd;
+
+	return 0;
+}
+
 static int __init fujitsu_init(void)
 {
-	int ret, max_brightness;
+	int ret;
 
 	if (acpi_disabled)
 		return -ENODEV;
@@ -1216,22 +1235,9 @@ static int __init fujitsu_init(void)
 	/* Register backlight stuff */
 
 	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		struct backlight_properties props;
-
-		memset(&props, 0, sizeof(struct backlight_properties));
-		max_brightness = fujitsu->max_brightness;
-		props.type = BACKLIGHT_PLATFORM;
-		props.max_brightness = max_brightness - 1;
-		fujitsu->bl_device = backlight_device_register("fujitsu-laptop",
-							       NULL, NULL,
-							       &fujitsubl_ops,
-							       &props);
-		if (IS_ERR(fujitsu->bl_device)) {
-			ret = PTR_ERR(fujitsu->bl_device);
-			fujitsu->bl_device = NULL;
+		ret = fujitsu_backlight_init();
+		if (ret)
 			goto fail_sysfs_group;
-		}
-		fujitsu->bl_device->props.brightness = fujitsu->brightness_level;
 	}
 
 	ret = platform_driver_register(&fujitsupf_driver);
-- 
2.11.0

[PATCH 3/4] platform/x86: fujitsu-laptop: sync backlight power status in acpi_fujitsu_hotkey_add()

[Michał Kępień]

Registering an ACPI driver does not mean the device it handles has to
exist.  As the code which syncs backlight power status uses
call_fext_func(), it needs the FUJ02E3 ACPI device to be present, so
ensure that code is only run once the FUJ02E3 device is detected.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index df1cd0a0231d..6438bcce90d4 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -930,6 +930,14 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
 	/* Suspect this is a keymap of the application panel, print it */
 	pr_info("BTNI: [0x%x]\n", call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0));
 
+	/* Sync backlight power status */
+	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+		if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
+			fujitsu->bl_device->props.power = FB_BLANK_POWERDOWN;
+		else
+			fujitsu->bl_device->props.power = FB_BLANK_UNBLANK;
+	}
+
 #if IS_ENABLED(CONFIG_LEDS_CLASS)
 	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
 		result = led_classdev_register(&fujitsu->pf_device->dev,
@@ -1256,14 +1264,6 @@ static int __init fujitsu_init(void)
 	if (ret)
 		goto fail_hotkey1;
 
-	/* Sync backlight power status (needs FUJ02E3 device, hence deferred) */
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
-			fujitsu->bl_device->props.power = FB_BLANK_POWERDOWN;
-		else
-			fujitsu->bl_device->props.power = FB_BLANK_UNBLANK;
-	}
-
 	pr_info("driver " FUJITSU_DRIVER_VERSION " successfully loaded\n");
 
 	return 0;
-- 
2.11.0

[PATCH 4/4] platform/x86: fujitsu-laptop: cleanup error labels in fujitsu_init()

[Michał Kępień]

Error labels currently used in fujitsu_init() are really hard to follow:
some (fail_hotkey) indicate which operation has failed, others
(fail_sysfs_group) indicate where unrolling should start and the rest
(fail_platform_driver) is simply confusing.  Change them to follow the
pattern used throughout the rest of the module, i.e. make every label
indicate the first unrolling operation it leads to.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 6438bcce90d4..67bb14ed3723 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -1220,70 +1220,70 @@ static int __init fujitsu_init(void)
 
 	ret = acpi_bus_register_driver(&acpi_fujitsu_driver);
 	if (ret)
-		goto fail_acpi;
+		goto err_free_fujitsu;
 
 	/* Register platform stuff */
 
 	fujitsu->pf_device = platform_device_alloc("fujitsu-laptop", -1);
 	if (!fujitsu->pf_device) {
 		ret = -ENOMEM;
-		goto fail_platform_driver;
+		goto err_unregister_acpi;
 	}
 
 	ret = platform_device_add(fujitsu->pf_device);
 	if (ret)
-		goto fail_platform_device1;
+		goto err_put_platform_device;
 
 	ret =
 	    sysfs_create_group(&fujitsu->pf_device->dev.kobj,
 			       &fujitsupf_attribute_group);
 	if (ret)
-		goto fail_platform_device2;
+		goto err_del_platform_device;
 
 	/* Register backlight stuff */
 
 	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
 		ret = fujitsu_backlight_init();
 		if (ret)
-			goto fail_sysfs_group;
+			goto err_remove_sysfs_group;
 	}
 
 	ret = platform_driver_register(&fujitsupf_driver);
 	if (ret)
-		goto fail_backlight;
+		goto err_unregister_backlight;
 
 	/* Register hotkey driver */
 
 	fujitsu_hotkey = kzalloc(sizeof(struct fujitsu_hotkey_t), GFP_KERNEL);
 	if (!fujitsu_hotkey) {
 		ret = -ENOMEM;
-		goto fail_hotkey;
+		goto err_unregister_platform_driver;
 	}
 
 	ret = acpi_bus_register_driver(&acpi_fujitsu_hotkey_driver);
 	if (ret)
-		goto fail_hotkey1;
+		goto err_free_fujitsu_hotkey;
 
 	pr_info("driver " FUJITSU_DRIVER_VERSION " successfully loaded\n");
 
 	return 0;
 
-fail_hotkey1:
+err_free_fujitsu_hotkey:
 	kfree(fujitsu_hotkey);
-fail_hotkey:
+err_unregister_platform_driver:
 	platform_driver_unregister(&fujitsupf_driver);
-fail_backlight:
+err_unregister_backlight:
 	backlight_device_unregister(fujitsu->bl_device);
-fail_sysfs_group:
+err_remove_sysfs_group:
 	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
 			   &fujitsupf_attribute_group);
-fail_platform_device2:
+err_del_platform_device:
 	platform_device_del(fujitsu->pf_device);
-fail_platform_device1:
+err_put_platform_device:
 	platform_device_put(fujitsu->pf_device);
-fail_platform_driver:
+err_unregister_acpi:
 	acpi_bus_unregister_driver(&acpi_fujitsu_driver);
-fail_acpi:
+err_free_fujitsu:
 	kfree(fujitsu);
 
 	return ret;
-- 
2.11.0

Re: [PATCH 0/4] fujitsu_init() cleanup

[Jonathan Woithe]

On Fri, Jan 13, 2017 at 12:02:36PM +0100, Micha?? K??pie?? wrote:
> These patches should make fujitsu_init() a bit more palatable.  I
> intentionally stopped after four patches, because they should do no harm
> and the next steps require some discussion.

I will review these as soon as I can, which might have to be after LCA2017.
That is, there could be a delay of just over a week before I can get to
this.

> The problem I have with this driver is that it is generally oblivious to
> the user's chosen backlight provider.  It was written before acpi_video
> support for backlight control was automatically detected by the kernel
> and as such it required a fix which was allegedly provided by
> 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is
> serving this functionality").  However, that fix was superficial as the
> module still registered its vendor-specific ACPI driver for backlight
> control.  That in turn caused SBLL/SBL2 to be called when brightness
> hotkeys were pressed even when acpi_video was supposed to handle
> brightness changes, which is exactly what that patch was hoping to
> prevent.

I suspect this is going to get messy fairly quickly.  The problem that this
module was orginally written to solve was that on certain Fujitsu laptops
(the S7020 for example) the standard ACPI video functionality was not able
to control the backlight (even after acpi_video gained its automatic
capabilities).  The non-standard manipulations done by fujitsu-laptop were
required for these machines or else it was completely impossible for the
backlight to be controlled by software.  I would have to go digging through
some ancient notes to verifiy this, but I think there was also an issue
whereby the brightness buttons on these laptops didn't work without the
fujitsu-laptop brightness functions being called in response to the button
presses.  Or something like that - it was a long time ago.

With this in mind, I think what we're starting to see now is a conflict
between the requirements of newer machines (which do sane things with regard
to ACPI video and backlight, and therefore only really need the standard
approach offered by acpi_video) and the older hardware which simply doesn't
work with acpi_video or the standard backlight approach.

> Moreover, the module unconditionally exports backlight-related
> sysfs attributes which enable userspace to change the brightness level,
> which should also only be possible when vendor backlight control is
> used.  Fast forward several years and on modern Fujitsu machines (e.g.
> Lifebook E744), the kernel automatically detects that native backlight
> control [1] should be used and SBLL becomes a noop [2].  This creates
> confusion, because the driver claims that it can get/set LCD brightness
> level via the platform device's sysfs attributes, but it actually
> cannot.  It gets even more confusing on Skylake machines on which the
> FUJ02B1 ACPI device is not present at all.

Again, what I think this highlights is that the older hardware requires code
which turns out to break (or at least significantly complicate) things on
modern hardware.  There were some older fujitsu systems where vendor
backlight control was the only option.

That said, there is clearly merit in suppressing the backlight sysfs
attributes on systems where they are not functional.

> The proposal I put forward in regard to the above is to remove the three
> backlight-related attributes (brightness_changed, lcd_level,
> max_brightness) from the platform driver's sysfs tree.  I believe the
> functions they implement should only be exposed through the backlight
> device, because the latter is only created when the user explicitly
> selects vendor backlight control or it is automatically selected by the
> kernel (this should be the case for older machines).

I will need to do some more digging into the historical developments.  At
face value I'm not sure how machines like the S7020 would end up controlling
the backlight if these sysfs attributes were removed because on this machine
(at least last time I checked) the standard backlight/video drivers could
not effect brightness control on this hardware.  Or is there a side effect
that I have forgotten?

> That would leave us with the remaining three sysfs attributes of the
> platform device, namely dock, lid and radios.  These all depend on the
> FUJ02E3 ACPI device.  Which begs the question: shall we reassign them to
> that ACPI device and drop the platform device altogether?  This would
> logically be the correct thing to do (panasonic-laptop and toshiba_acpi
> already assign extra sysfs attributes to ACPI nodes).  But I understand
> that this would break an 8-year-old userspace interface as functions
> previously exposed through /sys/devices/platform/fujitsu-laptop would be
> moved to /sys/bus/acpi/devices/FUJ02E3:00.  If that is unacceptable, the
> least we can (and should) do is to move platform device registration to
> acpi_fujitsu_hotkey_add().  What the driver currently does may create
> confusion in the future, because the platform device is registered
> unconditionally while it clearly depends on FUJ02E3 being present.  I do
> not know whether FUJ02E3 is present on all Fujitsu devices today without
> exception, but I do know that if Fujitsu ever decides to drop that
> device from its firmware, we would again (see above) expose a userspace
> interface (dock, lid, radios) which simply will not be able to function
> properly.

There is a significant misalignment of names in the fujitsu-laptop driver
which is probably adding to the overall confusion.  Back in 2009 Alan
Jenkins proposed a series of patches to clean this up.  Unfortunately these
came at a bad time for me and I wasn't able to evaluate them on my hardware,
so they langished.  I have been meaning to resurrect those patches ever
since but have never quite had the time to do so.  Most recently I was
planning to do it over the New Year break, but something else came up which
prevented that. :-(

It might be worth glancing through these because the resulting renames in
particular definitely improved the clarity of the driver:

  Date: Thu, 17 Sep 2009
  From: Alan Jenkins
  Subject: [PATCH 1/4] fujitsu-laptop: renames and cleanup
           [PATCH 2/4] fujitsu-laptop: restructure initialisation
           [PATCH 3/4] fujitsu-laptop: autodetect lcd interface on unknown models
           [PATCH 4/4] fujitsu-laptop: simplify modalias (autoload)

Unfortunately I do not have access to recent Fujitsu hardware so I cannot
comment as to whether FUJ02E3 is ubiquitous.

Regarding the movement of sysfs attributes, it is my understanding that
breaking the userspace API in this way is frowned upon.  Personally I could
argue that given the relatively specialised nature of these attributes and
their consequential low rate of use in the wild, such a move might be
justifiable.  However, the kernel tends to hold userspace interfaces to be
perpetual so I can't see how such a change would get up in this case. 
Darren may like to comment on this.

> I am happy to provide patches for whatever solution gets chosen, but
> first I would really appreciate if you (and anyone interested) could
> comment on the issues I described above.

As mentioned I will need to dig up information from 2007/8 to identify the
precise issues encountered with hardware from that time.  I am fairly
certain that the old hardware required the custom backlight control
implementation or else it was simply not functional.  There was also an
issue that the hardware did not turn the backlight on coming out of
hibernation: it was necessary to arrange for this to be fixed by software. 
When the unified ACPI video and backlight drivers eventuated they did not
seem capable of controlling the backlight on this hardware, which is why the
fujitsu-laptop backlight control remained in place.

Perhaps we might have to consider splitting fujitsu-laptop such that it
provides two distinct and indepentent functions: the old backlight interface
which is loaded only for the older hardware which requires it, and support
for the other devices where needed.  The need for this obviously depends on
the requirements of the hardware.

I am in no way opposed to a clean up of the driver.  However, it must be
done in a way which preserves functionality on the older hardware. 
Disclaimer: I have an S7020 which is still in active service, and I don't
want to loose software control over the backlight.

Regards
  jonathan

Re: [PATCH 0/4] fujitsu_init() cleanup

[Michał Kępień]

> On Fri, Jan 13, 2017 at 12:02:36PM +0100, Micha?? K??pie?? wrote:
> > These patches should make fujitsu_init() a bit more palatable.  I
> > intentionally stopped after four patches, because they should do no harm
> > and the next steps require some discussion.
> 
> I will review these as soon as I can, which might have to be after LCA2017.
> That is, there could be a delay of just over a week before I can get to
> this.

No worries, take your time, there is no rush.

> > The problem I have with this driver is that it is generally oblivious to
> > the user's chosen backlight provider.  It was written before acpi_video
> > support for backlight control was automatically detected by the kernel
> > and as such it required a fix which was allegedly provided by
> > 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is
> > serving this functionality").  However, that fix was superficial as the
> > module still registered its vendor-specific ACPI driver for backlight
> > control.  That in turn caused SBLL/SBL2 to be called when brightness
> > hotkeys were pressed even when acpi_video was supposed to handle
> > brightness changes, which is exactly what that patch was hoping to
> > prevent.
> 
> I suspect this is going to get messy fairly quickly.  The problem that this
> module was orginally written to solve was that on certain Fujitsu laptops
> (the S7020 for example) the standard ACPI video functionality was not able
> to control the backlight (even after acpi_video gained its automatic
> capabilities).  The non-standard manipulations done by fujitsu-laptop were
> required for these machines or else it was completely impossible for the
> backlight to be controlled by software.

That was exactly my reasoning after digging through the git log, which
is good because it means we are on the same page here.

> I would have to go digging through
> some ancient notes to verifiy this, but I think there was also an issue
> whereby the brightness buttons on these laptops didn't work without the
> fujitsu-laptop brightness functions being called in response to the button
> presses.  Or something like that - it was a long time ago.

Well, that sounds very likely, because pressing a brightness-related key
usually causes firmware to notify the OS about a brightness change
request.  Without the OS part to receive and process these notifications
(and no support from acpi_video), I do not see how these keys would
work.

> With this in mind, I think what we're starting to see now is a conflict
> between the requirements of newer machines (which do sane things with regard
> to ACPI video and backlight, and therefore only really need the standard
> approach offered by acpi_video) and the older hardware which simply doesn't
> work with acpi_video or the standard backlight approach.
> 
> > Moreover, the module unconditionally exports backlight-related
> > sysfs attributes which enable userspace to change the brightness level,
> > which should also only be possible when vendor backlight control is
> > used.  Fast forward several years and on modern Fujitsu machines (e.g.
> > Lifebook E744), the kernel automatically detects that native backlight
> > control [1] should be used and SBLL becomes a noop [2].  This creates
> > confusion, because the driver claims that it can get/set LCD brightness
> > level via the platform device's sysfs attributes, but it actually
> > cannot.  It gets even more confusing on Skylake machines on which the
> > FUJ02B1 ACPI device is not present at all.
> 
> Again, what I think this highlights is that the older hardware requires code
> which turns out to break (or at least significantly complicate) things on
> modern hardware.

Personally, I do not feel the complication is that much of a problem.
All that needs to be done is a reevaluation of under what circumstances
each part of the driver should be loaded.  Specifically, the backlight
part must not be enabled unless acpi_backlight=vendor.  What I tried to
demonstrate in the cover letter is that backlight-related sysfs
attributes belonging to the platform device make such a change hard to
introduce.  Sure, we could add them conditionally, but that would only
complicate things even more, which is where my suggestion to remove them
altogether stems from - I believe the driver should only expose
backlight functions through the backlight device.

> There were some older fujitsu systems where vendor
> backlight control was the only option.
> 
> That said, there is clearly merit in suppressing the backlight sysfs
> attributes on systems where they are not functional.
> 
> > The proposal I put forward in regard to the above is to remove the three
> > backlight-related attributes (brightness_changed, lcd_level,
> > max_brightness) from the platform driver's sysfs tree.  I believe the
> > functions they implement should only be exposed through the backlight
> > device, because the latter is only created when the user explicitly
> > selects vendor backlight control or it is automatically selected by the
> > kernel (this should be the case for older machines).
> 
> I will need to do some more digging into the historical developments.  At
> face value I'm not sure how machines like the S7020 would end up controlling
> the backlight if these sysfs attributes were removed because on this machine
> (at least last time I checked) the standard backlight/video drivers could
> not effect brightness control on this hardware.  Or is there a side effect
> that I have forgotten?

Let us not confuse three separate things that fujitsu-laptop enables:

  * changing LCD brightness using the keyboard,
  * changing LCD brightness from userspace, using the backlight device,
  * changing LCD brightness from userspace, using sysfs attributes.

I have nothing against the first two, apart from the notion that they
should be more tightly coupled (i.e. one should not be enabled without
the other one and vice versa).

I am all against the last one.  IMHO it is a duplicate, misplaced
feature which only makes clean separation of components inside the
driver hard.

> > That would leave us with the remaining three sysfs attributes of the
> > platform device, namely dock, lid and radios.  These all depend on the
> > FUJ02E3 ACPI device.  Which begs the question: shall we reassign them to
> > that ACPI device and drop the platform device altogether?  This would
> > logically be the correct thing to do (panasonic-laptop and toshiba_acpi
> > already assign extra sysfs attributes to ACPI nodes).  But I understand
> > that this would break an 8-year-old userspace interface as functions
> > previously exposed through /sys/devices/platform/fujitsu-laptop would be
> > moved to /sys/bus/acpi/devices/FUJ02E3:00.  If that is unacceptable, the
> > least we can (and should) do is to move platform device registration to
> > acpi_fujitsu_hotkey_add().  What the driver currently does may create
> > confusion in the future, because the platform device is registered
> > unconditionally while it clearly depends on FUJ02E3 being present.  I do
> > not know whether FUJ02E3 is present on all Fujitsu devices today without
> > exception, but I do know that if Fujitsu ever decides to drop that
> > device from its firmware, we would again (see above) expose a userspace
> > interface (dock, lid, radios) which simply will not be able to function
> > properly.
> 
> There is a significant misalignment of names in the fujitsu-laptop driver
> which is probably adding to the overall confusion.  Back in 2009 Alan
> Jenkins proposed a series of patches to clean this up.  Unfortunately these
> came at a bad time for me and I wasn't able to evaluate them on my hardware,
> so they langished.  I have been meaning to resurrect those patches ever
> since but have never quite had the time to do so.  Most recently I was
> planning to do it over the New Year break, but something else came up which
> prevented that. :-(
> 
> It might be worth glancing through these because the resulting renames in
> particular definitely improved the clarity of the driver:
> 
>   Date: Thu, 17 Sep 2009
>   From: Alan Jenkins
>   Subject: [PATCH 1/4] fujitsu-laptop: renames and cleanup
>            [PATCH 2/4] fujitsu-laptop: restructure initialisation
>            [PATCH 3/4] fujitsu-laptop: autodetect lcd interface on unknown models
>            [PATCH 4/4] fujitsu-laptop: simplify modalias (autoload)

These subjects suggest that it makes little sense for me to duplicate
Alan's work.  I have a lot more cleanups on my todo list, but the more I
change, the less sense Alan's patches will make.  I guess the most
reasonable course of action would be for you to go through Alan's series
after all so that I will only need to send fixes for whatever he has
missed.  Feel free to suggest otherwise, though.

> Unfortunately I do not have access to recent Fujitsu hardware so I cannot
> comment as to whether FUJ02E3 is ubiquitous.

This is a secondary issue, I only raised it to emphasize that the driver
*might* be erring as we speak, but it is *definitely* not future-proof.

> Regarding the movement of sysfs attributes, it is my understanding that
> breaking the userspace API in this way is frowned upon.

This is my understanding as well, which is why this is only a proposal
and not a patch.

> Personally I could
> argue that given the relatively specialised nature of these attributes and
> their consequential low rate of use in the wild, such a move might be
> justifiable.  However, the kernel tends to hold userspace interfaces to be
> perpetual so I can't see how such a change would get up in this case. 
> Darren may like to comment on this.

Yes, this definitely needs input from subsystem maintainers.

Let me just emphasize once again that I believe backlight-related sysfs
attributes belonging to the platform driver are a misplaced feature.
Backlight-related features belong in backlight devices.  The only
situation I can think of that someone will get hurt by these getting
removed is that they have some custom script which uses the platform
device instead of the backlight device to control LCD brightness.
Removing these attributes has no effect on whether brightness-related
keys on the keyboard work or not.

Nevertheless, if the consensus is that these should stay where they are,
they need to be added conditionally, only when acpi_backlight=vendor.
Otherwise they simply do not work on modern hardware and cause
confusion.

> > I am happy to provide patches for whatever solution gets chosen, but
> > first I would really appreciate if you (and anyone interested) could
> > comment on the issues I described above.
> 
> As mentioned I will need to dig up information from 2007/8 to identify the
> precise issues encountered with hardware from that time.  I am fairly
> certain that the old hardware required the custom backlight control
> implementation or else it was simply not functional.  There was also an
> issue that the hardware did not turn the backlight on coming out of
> hibernation: it was necessary to arrange for this to be fixed by software. 
> When the unified ACPI video and backlight drivers eventuated they did not
> seem capable of controlling the backlight on this hardware, which is why the
> fujitsu-laptop backlight control remained in place.
> 
> Perhaps we might have to consider splitting fujitsu-laptop such that it
> provides two distinct and indepentent functions: the old backlight interface
> which is loaded only for the older hardware which requires it, and support
> for the other devices where needed.  The need for this obviously depends on
> the requirements of the hardware.

I also thought about this and this might be a nice idea.

The problem I have with the source code in its current state is that it
took me a day to understand that we are looking at two cleanly separable
ACPI drivers which are needlessly intertwined by a redundant platform
device.

> I am in no way opposed to a clean up of the driver.  However, it must be
> done in a way which preserves functionality on the older hardware. 

I could not agree more.

> Disclaimer: I have an S7020 which is still in active service, and I don't
> want to loose software control over the backlight.

Great, then we have someone to test the upcoming patches! ;)

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/4] fujitsu_init() cleanup

[Jonathan Woithe]

On Fri, Jan 13, 2017 at 02:19:15PM +0100, Micha?? K??pie?? wrote:
> > It might be worth glancing through these because the resulting renames in
> > particular definitely improved the clarity of the driver:
> > 
> >   Date: Thu, 17 Sep 2009
> >   From: Alan Jenkins
> >   Subject: [PATCH 1/4] fujitsu-laptop: renames and cleanup
> >            [PATCH 2/4] fujitsu-laptop: restructure initialisation
> >            [PATCH 3/4] fujitsu-laptop: autodetect lcd interface on unknown models
> >            [PATCH 4/4] fujitsu-laptop: simplify modalias (autoload)
> 
> These subjects suggest that it makes little sense for me to duplicate
> Alan's work.  I have a lot more cleanups on my todo list, but the more I
> change, the less sense Alan's patches will make.  I guess the most
> reasonable course of action would be for you to go through Alan's series
> after all so that I will only need to send fixes for whatever he has
> missed.  Feel free to suggest otherwise, though.

That certainly seems like a good way forward.  I am now back from travel and
will try to get onto these patches as soon as possible.  Hopefully I will
have something to report soon.

Regards
  jonathan

Re: [PATCH 0/4] fujitsu_init() cleanup

[Michał Kępień]

Darren, Andy,

Please drop this patch series for now.  I will send a rebased v2 after a
long overdue patch series from Alan Jenkins gets applied in a reworked
form.

However, your input is still essential for determining the future shape
of fujitsu-laptop.  I quoted the essential parts of my discussion with
Jonathan below.

> > > The problem I have with this driver is that it is generally oblivious to
> > > the user's chosen backlight provider.  It was written before acpi_video
> > > support for backlight control was automatically detected by the kernel 
> > > and as such it required a fix which was allegedly provided by          
> > > 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is    
> > > serving this functionality").  However, that fix was superficial as the
> > > module still registered its vendor-specific ACPI driver for backlight  
> > > control.  That in turn caused SBLL/SBL2 to be called when brightness   
> > > hotkeys were pressed even when acpi_video was supposed to handle       
> > > brightness changes, which is exactly what that patch was hoping to     
> > > prevent.  Moreover, the module unconditionally exports backlight-related
> > > sysfs attributes which enable userspace to change the brightness level,
> > > which should also only be possible when vendor backlight control is    
> > > used.  Fast forward several years and on modern Fujitsu machines (e.g. 
> > > Lifebook E744), the kernel automatically detects that native backlight 
> > > control [1] should be used and SBLL becomes a noop [2].  This creates  
> > > confusion, because the driver claims that it can get/set LCD brightness
> > > level via the platform device's sysfs attributes, but it actually      
> > > cannot.  It gets even more confusing on Skylake machines on which the  
> > > FUJ02B1 ACPI device is not present at all.                             

[]

> > > The proposal I put forward in regard to the above is to remove the three
> > > backlight-related attributes (brightness_changed, lcd_level,
> > > max_brightness) from the platform driver's sysfs tree.  I believe the
> > > functions they implement should only be exposed through the backlight
> > > device, because the latter is only created when the user explicitly
> > > selects vendor backlight control or it is automatically selected by the
> > > kernel (this should be the case for older machines).
> > 
> > I will need to do some more digging into the historical developments.  At
> > face value I'm not sure how machines like the S7020 would end up controlling
> > the backlight if these sysfs attributes were removed because on this machine
> > (at least last time I checked) the standard backlight/video drivers could
> > not effect brightness control on this hardware.  Or is there a side effect
> > that I have forgotten?
> 
> Let us not confuse three separate things that fujitsu-laptop enables:
> 
>   * changing LCD brightness using the keyboard,
>   * changing LCD brightness from userspace, using the backlight device,
>   * changing LCD brightness from userspace, using sysfs attributes.
> 
> I have nothing against the first two, apart from the notion that they
> should be more tightly coupled (i.e. one should not be enabled without
> the other one and vice versa).
> 
> I am all against the last one.  IMHO it is a duplicate, misplaced
> feature which only makes clean separation of components inside the
> driver hard.
> 
> > > That would leave us with the remaining three sysfs attributes of the
> > > platform device, namely dock, lid and radios.  These all depend on the
> > > FUJ02E3 ACPI device.  Which begs the question: shall we reassign them to
> > > that ACPI device and drop the platform device altogether?  This would
> > > logically be the correct thing to do (panasonic-laptop and toshiba_acpi
> > > already assign extra sysfs attributes to ACPI nodes).  But I understand
> > > that this would break an 8-year-old userspace interface as functions
> > > previously exposed through /sys/devices/platform/fujitsu-laptop would be
> > > moved to /sys/bus/acpi/devices/FUJ02E3:00.  If that is unacceptable, the
> > > least we can (and should) do is to move platform device registration to
> > > acpi_fujitsu_hotkey_add().  What the driver currently does may create
> > > confusion in the future, because the platform device is registered
> > > unconditionally while it clearly depends on FUJ02E3 being present.  I do
> > > not know whether FUJ02E3 is present on all Fujitsu devices today without
> > > exception, but I do know that if Fujitsu ever decides to drop that
> > > device from its firmware, we would again (see above) expose a userspace
> > > interface (dock, lid, radios) which simply will not be able to function
> > > properly.

[]

> > Regarding the movement of sysfs attributes, it is my understanding that
> > breaking the userspace API in this way is frowned upon.
> 
> This is my understanding as well, which is why this is only a proposal
> and not a patch.
> 
> > Personally I could
> > argue that given the relatively specialised nature of these attributes and
> > their consequential low rate of use in the wild, such a move might be
> > justifiable.  However, the kernel tends to hold userspace interfaces to be
> > perpetual so I can't see how such a change would get up in this case. 
> > Darren may like to comment on this.
> 
> Yes, this definitely needs input from subsystem maintainers.
> 
> Let me just emphasize once again that I believe backlight-related sysfs
> attributes belonging to the platform driver are a misplaced feature.
> Backlight-related features belong in backlight devices.  The only
> situation I can think of that someone will get hurt by these getting
> removed is that they have some custom script which uses the platform
> device instead of the backlight device to control LCD brightness.
> Removing these attributes has no effect on whether brightness-related
> keys on the keyboard work or not.
> 
> Nevertheless, if the consensus is that these should stay where they are,
> they need to be added conditionally, only when acpi_backlight=vendor.
> Otherwise they simply do not work on modern hardware and cause
> confusion.

In regard to the above, please let me know what you think about:

  - removing backlight-related attributes from the platform device,

  - removing the platform device altogether and attaching
    laptop-specific attributes to the ACPI device instead.

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/4] fujitsu_init() cleanup

[Andy Shevchenko]

On Fri, Feb 3, 2017 at 4:06 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> Darren, Andy,
>
> Please drop this patch series for now.  I will send a rebased v2 after a
> long overdue patch series from Alan Jenkins gets applied in a reworked
> form.

Noted.

Just for you information. Our repo now consists three branches: fixes,
for-next, and testing. testing is kinda for internal use, it might be
broken or be in a wrong state.
Better to use for-next as a base. For this cycle (which is my first as
a co-maintainer) I missed a proper workflow.
That's why I just rebased for-next. It will be not the case in the future.

Darren, consider above as a summary of sync mail for this week.

> However, your input is still essential for determining the future shape
> of fujitsu-laptop.  I quoted the essential parts of my discussion with
> Jonathan below.

I'm about to leave, I'm going to follow this later.
Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] fujitsu_init() cleanup

[Darren Hart]

On Fri, Feb 03, 2017 at 04:13:40PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 3, 2017 at 4:06 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> > Darren, Andy,
> >
> > Please drop this patch series for now.  I will send a rebased v2 after a
> > long overdue patch series from Alan Jenkins gets applied in a reworked
> > form.
> 
> Noted.
> 
> Just for you information. Our repo now consists three branches: fixes,
> for-next, and testing. testing is kinda for internal use, it might be
> broken or be in a wrong state.
> Better to use for-next as a base. For this cycle (which is my first as

In general, you should be able to use Linus' master as the base. Only if you
require patches already in for-next, should you use for-next.


-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 0/4] fujitsu_init() cleanup

[Michał Kępień]

> On Fri, Feb 03, 2017 at 04:13:40PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 3, 2017 at 4:06 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> > > Darren, Andy,
> > >
> > > Please drop this patch series for now.  I will send a rebased v2 after a
> > > long overdue patch series from Alan Jenkins gets applied in a reworked
> > > form.
> > 
> > Noted.
> > 
> > Just for you information. Our repo now consists three branches: fixes,
> > for-next, and testing. testing is kinda for internal use, it might be
> > broken or be in a wrong state.
> > Better to use for-next as a base. For this cycle (which is my first as
> 
> In general, you should be able to use Linus' master as the base. Only if you
> require patches already in for-next, should you use for-next.

Thanks, guys, this is all helpful information.  I have always assumed
testing was the correct base branch because my patches got pushed there
once they got accepted.

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/4] fujitsu_init() cleanup

[Andy Shevchenko]

On Sat, Feb 4, 2017 at 8:21 AM, Michał Kępień <kernel@kempniu.pl> wrote:
>> On Fri, Feb 03, 2017 at 04:13:40PM +0200, Andy Shevchenko wrote:
>> > On Fri, Feb 3, 2017 at 4:06 PM, Michał Kępień <kernel@kempniu.pl> wrote:
>> > > Darren, Andy,
>> > >
>> > > Please drop this patch series for now.  I will send a rebased v2 after a
>> > > long overdue patch series from Alan Jenkins gets applied in a reworked
>> > > form.
>> >
>> > Noted.
>> >
>> > Just for you information. Our repo now consists three branches: fixes,
>> > for-next, and testing. testing is kinda for internal use, it might be
>> > broken or be in a wrong state.
>> > Better to use for-next as a base. For this cycle (which is my first as
>>
>> In general, you should be able to use Linus' master as the base. Only if you
>> require patches already in for-next, should you use for-next.
>
> Thanks, guys, this is all helpful information.  I have always assumed
> testing was the correct base branch because my patches got pushed there
> once they got accepted.

Consider testing as a purgatory of the stuff.

P.S. Darren fixed your name in previous patches and thus one more
rebase happened in for-next.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] fujitsu_init() cleanup

[Michał Kępień]

> Darren, Andy,
> 
> Please drop this patch series for now.  I will send a rebased v2 after a
> long overdue patch series from Alan Jenkins gets applied in a reworked
> form.
> 
> However, your input is still essential for determining the future shape
> of fujitsu-laptop.  I quoted the essential parts of my discussion with
> Jonathan below.
> 
> > > > The problem I have with this driver is that it is generally oblivious to
> > > > the user's chosen backlight provider.  It was written before acpi_video
> > > > support for backlight control was automatically detected by the kernel 
> > > > and as such it required a fix which was allegedly provided by          
> > > > 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is    
> > > > serving this functionality").  However, that fix was superficial as the
> > > > module still registered its vendor-specific ACPI driver for backlight  
> > > > control.  That in turn caused SBLL/SBL2 to be called when brightness   
> > > > hotkeys were pressed even when acpi_video was supposed to handle       
> > > > brightness changes, which is exactly what that patch was hoping to     
> > > > prevent.  Moreover, the module unconditionally exports backlight-related
> > > > sysfs attributes which enable userspace to change the brightness level,
> > > > which should also only be possible when vendor backlight control is    
> > > > used.  Fast forward several years and on modern Fujitsu machines (e.g. 
> > > > Lifebook E744), the kernel automatically detects that native backlight 
> > > > control [1] should be used and SBLL becomes a noop [2].  This creates  
> > > > confusion, because the driver claims that it can get/set LCD brightness
> > > > level via the platform device's sysfs attributes, but it actually      
> > > > cannot.  It gets even more confusing on Skylake machines on which the  
> > > > FUJ02B1 ACPI device is not present at all.                             
> 
> []
> 
> > > > The proposal I put forward in regard to the above is to remove the three
> > > > backlight-related attributes (brightness_changed, lcd_level,
> > > > max_brightness) from the platform driver's sysfs tree.  I believe the
> > > > functions they implement should only be exposed through the backlight
> > > > device, because the latter is only created when the user explicitly
> > > > selects vendor backlight control or it is automatically selected by the
> > > > kernel (this should be the case for older machines).
> > > 
> > > I will need to do some more digging into the historical developments.  At
> > > face value I'm not sure how machines like the S7020 would end up controlling
> > > the backlight if these sysfs attributes were removed because on this machine
> > > (at least last time I checked) the standard backlight/video drivers could
> > > not effect brightness control on this hardware.  Or is there a side effect
> > > that I have forgotten?
> > 
> > Let us not confuse three separate things that fujitsu-laptop enables:
> > 
> >   * changing LCD brightness using the keyboard,
> >   * changing LCD brightness from userspace, using the backlight device,
> >   * changing LCD brightness from userspace, using sysfs attributes.
> > 
> > I have nothing against the first two, apart from the notion that they
> > should be more tightly coupled (i.e. one should not be enabled without
> > the other one and vice versa).
> > 
> > I am all against the last one.  IMHO it is a duplicate, misplaced
> > feature which only makes clean separation of components inside the
> > driver hard.
> > 
> > > > That would leave us with the remaining three sysfs attributes of the
> > > > platform device, namely dock, lid and radios.  These all depend on the
> > > > FUJ02E3 ACPI device.  Which begs the question: shall we reassign them to
> > > > that ACPI device and drop the platform device altogether?  This would
> > > > logically be the correct thing to do (panasonic-laptop and toshiba_acpi
> > > > already assign extra sysfs attributes to ACPI nodes).  But I understand
> > > > that this would break an 8-year-old userspace interface as functions
> > > > previously exposed through /sys/devices/platform/fujitsu-laptop would be
> > > > moved to /sys/bus/acpi/devices/FUJ02E3:00.  If that is unacceptable, the
> > > > least we can (and should) do is to move platform device registration to
> > > > acpi_fujitsu_hotkey_add().  What the driver currently does may create
> > > > confusion in the future, because the platform device is registered
> > > > unconditionally while it clearly depends on FUJ02E3 being present.  I do
> > > > not know whether FUJ02E3 is present on all Fujitsu devices today without
> > > > exception, but I do know that if Fujitsu ever decides to drop that
> > > > device from its firmware, we would again (see above) expose a userspace
> > > > interface (dock, lid, radios) which simply will not be able to function
> > > > properly.
> 
> []
> 
> > > Regarding the movement of sysfs attributes, it is my understanding that
> > > breaking the userspace API in this way is frowned upon.
> > 
> > This is my understanding as well, which is why this is only a proposal
> > and not a patch.
> > 
> > > Personally I could
> > > argue that given the relatively specialised nature of these attributes and
> > > their consequential low rate of use in the wild, such a move might be
> > > justifiable.  However, the kernel tends to hold userspace interfaces to be
> > > perpetual so I can't see how such a change would get up in this case. 
> > > Darren may like to comment on this.
> > 
> > Yes, this definitely needs input from subsystem maintainers.
> > 
> > Let me just emphasize once again that I believe backlight-related sysfs
> > attributes belonging to the platform driver are a misplaced feature.
> > Backlight-related features belong in backlight devices.  The only
> > situation I can think of that someone will get hurt by these getting
> > removed is that they have some custom script which uses the platform
> > device instead of the backlight device to control LCD brightness.
> > Removing these attributes has no effect on whether brightness-related
> > keys on the keyboard work or not.
> > 
> > Nevertheless, if the consensus is that these should stay where they are,
> > they need to be added conditionally, only when acpi_backlight=vendor.
> > Otherwise they simply do not work on modern hardware and cause
> > confusion.
> 
> In regard to the above, please let me know what you think about:
> 
>   - removing backlight-related attributes from the platform device,
> 
>   - removing the platform device altogether and attaching
>     laptop-specific attributes to the ACPI device instead.

Darren, Andy,

As six weeks have passed since I originally asked for your comments, I
hope another gentle reminder is okay.  Your advice is needed before I
can post further cleanups for fujitsu-laptop.

Thanks,

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/4] fujitsu_init() cleanup

[Darren Hart]

Greg, question for you below, see "GregKH"...

On Mon, Feb 27, 2017 at 08:19:09AM +0100, Michał Kępień wrote:
> > Darren, Andy,
> > 
> > Please drop this patch series for now.  I will send a rebased v2 after a
> > long overdue patch series from Alan Jenkins gets applied in a reworked
> > form.
> > 
> > However, your input is still essential for determining the future shape
> > of fujitsu-laptop.  I quoted the essential parts of my discussion with
> > Jonathan below.

Very sorry for the delay, I've lost context on this, let me see if I can pick it
up with your summary below...

> > 
> > > > > The problem I have with this driver is that it is generally oblivious to
> > > > > the user's chosen backlight provider.  It was written before acpi_video
> > > > > support for backlight control was automatically detected by the kernel 
> > > > > and as such it required a fix which was allegedly provided by          
> > > > > 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is    
> > > > > serving this functionality").  However, that fix was superficial as the
> > > > > module still registered its vendor-specific ACPI driver for backlight  
> > > > > control.  That in turn caused SBLL/SBL2 to be called when brightness   
> > > > > hotkeys were pressed even when acpi_video was supposed to handle       
> > > > > brightness changes, which is exactly what that patch was hoping to     
> > > > > prevent.  Moreover, the module unconditionally exports backlight-related
> > > > > sysfs attributes which enable userspace to change the brightness level,
> > > > > which should also only be possible when vendor backlight control is    
> > > > > used.  Fast forward several years and on modern Fujitsu machines (e.g. 
> > > > > Lifebook E744), the kernel automatically detects that native backlight 
> > > > > control [1] should be used and SBLL becomes a noop [2].  This creates  
> > > > > confusion, because the driver claims that it can get/set LCD brightness
> > > > > level via the platform device's sysfs attributes, but it actually      
> > > > > cannot.  It gets even more confusing on Skylake machines on which the  
> > > > > FUJ02B1 ACPI device is not present at all.                           

Right, evolved.... time to take a step back and clean it up.

> > > > > The proposal I put forward in regard to the above is to remove the three
> > > > > backlight-related attributes (brightness_changed, lcd_level,
> > > > > max_brightness) from the platform driver's sysfs tree.  I believe the
> > > > > functions they implement should only be exposed through the backlight
> > > > > device, because the latter is only created when the user explicitly
> > > > > selects vendor backlight control or it is automatically selected by the
> > > > > kernel (this should be the case for older machines).

This seems to be a good approach as in combination with the discussion below re
the ACPI device, it eliminates the sysfs attributes from the platform device
completely and makes the driver more consistent with others in the subsystem.

> > > > I will need to do some more digging into the historical developments.  At
> > > > face value I'm not sure how machines like the S7020 would end up controlling
> > > > the backlight if these sysfs attributes were removed because on this machine
> > > > (at least last time I checked) the standard backlight/video drivers could
> > > > not effect brightness control on this hardware.  Or is there a side effect
> > > > that I have forgotten?
> > > 
> > > Let us not confuse three separate things that fujitsu-laptop enables:
> > > 
> > >   * changing LCD brightness using the keyboard,
> > >   * changing LCD brightness from userspace, using the backlight device,
> > >   * changing LCD brightness from userspace, using sysfs attributes.
> > > 
> > > I have nothing against the first two, apart from the notion that they
> > > should be more tightly coupled (i.e. one should not be enabled without
> > > the other one and vice versa).
> > > 
> > > I am all against the last one.  IMHO it is a duplicate, misplaced
> > > feature which only makes clean separation of components inside the
> > > driver hard.
> > > 
> > > > > That would leave us with the remaining three sysfs attributes of the
> > > > > platform device, namely dock, lid and radios.  These all depend on the
> > > > > FUJ02E3 ACPI device.  Which begs the question: shall we reassign them to
> > > > > that ACPI device and drop the platform device altogether?  This would
> > > > > logically be the correct thing to do (panasonic-laptop and toshiba_acpi
> > > > > already assign extra sysfs attributes to ACPI nodes).  But I understand
> > > > > that this would break an 8-year-old userspace interface as functions
> > > > > previously exposed through /sys/devices/platform/fujitsu-laptop would be
> > > > > moved to /sys/bus/acpi/devices/FUJ02E3:00.  If that is unacceptable, the

We can change the device sysfs attributes using versioning. We should of course
do our due diligence to discover if there are any users of this sysfs interface,
and if so, if they have strong objections to changing. But, if someone screams,
we can always revert.

GregKH - Can you please confirm the above? Moving an attribute is different than
the format and contents, which is what I explicitly documented in
Documentation/admin-guide/sysfs-rules.rst (last section).

> > > > > least we can (and should) do is to move platform device registration to
> > > > > acpi_fujitsu_hotkey_add().  What the driver currently does may create
> > > > > confusion in the future, because the platform device is registered
> > > > > unconditionally while it clearly depends on FUJ02E3 being present.  I do
> > > > > not know whether FUJ02E3 is present on all Fujitsu devices today without
> > > > > exception, but I do know that if Fujitsu ever decides to drop that
> > > > > device from its firmware, we would again (see above) expose a userspace
> > > > > interface (dock, lid, radios) which simply will not be able to function
> > > > > properly.
> > 
> > []
> > 
> > > > Regarding the movement of sysfs attributes, it is my understanding that
> > > > breaking the userspace API in this way is frowned upon.

Yes, but... It is less of a concern for a specific device or platform, than it
is for a common kernel feature. I believe there is good argumentation above for
why this driver and it's attributes need to be cleaned up.

> > > 
> > > This is my understanding as well, which is why this is only a proposal
> > > and not a patch.
> > > 
> > > > Personally I could
> > > > argue that given the relatively specialised nature of these attributes and
> > > > their consequential low rate of use in the wild, such a move might be
> > > > justifiable.  However, the kernel tends to hold userspace interfaces to be
> > > > perpetual so I can't see how such a change would get up in this case. 
> > > > Darren may like to comment on this.
> > > 
> > > Yes, this definitely needs input from subsystem maintainers.
> > > 
> > > Let me just emphasize once again that I believe backlight-related sysfs
> > > attributes belonging to the platform driver are a misplaced feature.
> > > Backlight-related features belong in backlight devices.  The only
> > > situation I can think of that someone will get hurt by these getting
> > > removed is that they have some custom script which uses the platform
> > > device instead of the backlight device to control LCD brightness.
> > > Removing these attributes has no effect on whether brightness-related
> > > keys on the keyboard work or not.
> > > 
> > > Nevertheless, if the consensus is that these should stay where they are,
> > > they need to be added conditionally, only when acpi_backlight=vendor.
> > > Otherwise they simply do not work on modern hardware and cause
> > > confusion.
> > 
> > In regard to the above, please let me know what you think about:
> > 
> >   - removing backlight-related attributes from the platform device,
> > 
> >   - removing the platform device altogether and attaching
> >     laptop-specific attributes to the ACPI device instead.

Unless GregKH objects, I would prefer to see the second option.

> 
> Darren, Andy,
> 
> As six weeks have passed since I originally asked for your comments, I
> hope another gentle reminder is okay.  Your advice is needed before I
> can post further cleanups for fujitsu-laptop.

My sincere apologies. Please always feel free to nag if more than 2 weeks go by.
6 weeks is totally unacceptable.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 0/4] fujitsu_init() cleanup

[Greg Kroah-Hartman]

On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> Greg, question for you below, see "GregKH"...
> 
> On Mon, Feb 27, 2017 at 08:19:09AM +0100, Michał Kępień wrote:
> > > Darren, Andy,
> > > 
> > > Please drop this patch series for now.  I will send a rebased v2 after a
> > > long overdue patch series from Alan Jenkins gets applied in a reworked
> > > form.
> > > 
> > > However, your input is still essential for determining the future shape
> > > of fujitsu-laptop.  I quoted the essential parts of my discussion with
> > > Jonathan below.
> 
> Very sorry for the delay, I've lost context on this, let me see if I can pick it
> up with your summary below...
> 
> > > 
> > > > > > The problem I have with this driver is that it is generally oblivious to
> > > > > > the user's chosen backlight provider.  It was written before acpi_video
> > > > > > support for backlight control was automatically detected by the kernel 
> > > > > > and as such it required a fix which was allegedly provided by          
> > > > > > 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is    
> > > > > > serving this functionality").  However, that fix was superficial as the
> > > > > > module still registered its vendor-specific ACPI driver for backlight  
> > > > > > control.  That in turn caused SBLL/SBL2 to be called when brightness   
> > > > > > hotkeys were pressed even when acpi_video was supposed to handle       
> > > > > > brightness changes, which is exactly what that patch was hoping to     
> > > > > > prevent.  Moreover, the module unconditionally exports backlight-related
> > > > > > sysfs attributes which enable userspace to change the brightness level,
> > > > > > which should also only be possible when vendor backlight control is    
> > > > > > used.  Fast forward several years and on modern Fujitsu machines (e.g. 
> > > > > > Lifebook E744), the kernel automatically detects that native backlight 
> > > > > > control [1] should be used and SBLL becomes a noop [2].  This creates  
> > > > > > confusion, because the driver claims that it can get/set LCD brightness
> > > > > > level via the platform device's sysfs attributes, but it actually      
> > > > > > cannot.  It gets even more confusing on Skylake machines on which the  
> > > > > > FUJ02B1 ACPI device is not present at all.                           
> 
> Right, evolved.... time to take a step back and clean it up.
> 
> > > > > > The proposal I put forward in regard to the above is to remove the three
> > > > > > backlight-related attributes (brightness_changed, lcd_level,
> > > > > > max_brightness) from the platform driver's sysfs tree.  I believe the
> > > > > > functions they implement should only be exposed through the backlight
> > > > > > device, because the latter is only created when the user explicitly
> > > > > > selects vendor backlight control or it is automatically selected by the
> > > > > > kernel (this should be the case for older machines).
> 
> This seems to be a good approach as in combination with the discussion below re
> the ACPI device, it eliminates the sysfs attributes from the platform device
> completely and makes the driver more consistent with others in the subsystem.
> 
> > > > > I will need to do some more digging into the historical developments.  At
> > > > > face value I'm not sure how machines like the S7020 would end up controlling
> > > > > the backlight if these sysfs attributes were removed because on this machine
> > > > > (at least last time I checked) the standard backlight/video drivers could
> > > > > not effect brightness control on this hardware.  Or is there a side effect
> > > > > that I have forgotten?
> > > > 
> > > > Let us not confuse three separate things that fujitsu-laptop enables:
> > > > 
> > > >   * changing LCD brightness using the keyboard,
> > > >   * changing LCD brightness from userspace, using the backlight device,
> > > >   * changing LCD brightness from userspace, using sysfs attributes.
> > > > 
> > > > I have nothing against the first two, apart from the notion that they
> > > > should be more tightly coupled (i.e. one should not be enabled without
> > > > the other one and vice versa).
> > > > 
> > > > I am all against the last one.  IMHO it is a duplicate, misplaced
> > > > feature which only makes clean separation of components inside the
> > > > driver hard.
> > > > 
> > > > > > That would leave us with the remaining three sysfs attributes of the
> > > > > > platform device, namely dock, lid and radios.  These all depend on the
> > > > > > FUJ02E3 ACPI device.  Which begs the question: shall we reassign them to
> > > > > > that ACPI device and drop the platform device altogether?  This would
> > > > > > logically be the correct thing to do (panasonic-laptop and toshiba_acpi
> > > > > > already assign extra sysfs attributes to ACPI nodes).  But I understand
> > > > > > that this would break an 8-year-old userspace interface as functions
> > > > > > previously exposed through /sys/devices/platform/fujitsu-laptop would be
> > > > > > moved to /sys/bus/acpi/devices/FUJ02E3:00.  If that is unacceptable, the
> 
> We can change the device sysfs attributes using versioning. We should of course
> do our due diligence to discover if there are any users of this sysfs interface,
> and if so, if they have strong objections to changing. But, if someone screams,
> we can always revert.
> 
> GregKH - Can you please confirm the above? Moving an attribute is different than
> the format and contents, which is what I explicitly documented in
> Documentation/admin-guide/sysfs-rules.rst (last section).

Moving an attribute to a different device structure is usually a bad
idea, if the userspace tool counting on it to be present in a specific
place breaks.

And I really don't like putting random device attributes on "parent"
devices.  I know Dmitry Torokhov disagrees about this though, his
subsystem does like doing that.  But whatever you do, you should at
least try to be consistent.

But, as you can't be consistent here, don't break userspace please, I'd
recommend just leaving it alone for now.

thanks,

greg k-h

Re: [PATCH 0/4] fujitsu_init() cleanup

[Michał Kępień]

> On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> > Greg, question for you below, see "GregKH"...
> > 
> > On Mon, Feb 27, 2017 at 08:19:09AM +0100, Michał Kępień wrote:
> > > > Darren, Andy,
> > > > 
> > > > Please drop this patch series for now.  I will send a rebased v2 after a
> > > > long overdue patch series from Alan Jenkins gets applied in a reworked
> > > > form.
> > > > 
> > > > However, your input is still essential for determining the future shape
> > > > of fujitsu-laptop.  I quoted the essential parts of my discussion with
> > > > Jonathan below.
> > 
> > Very sorry for the delay, I've lost context on this, let me see if I can pick it
> > up with your summary below...
> > 
> > > > 
> > > > > > > The problem I have with this driver is that it is generally oblivious to
> > > > > > > the user's chosen backlight provider.  It was written before acpi_video
> > > > > > > support for backlight control was automatically detected by the kernel 
> > > > > > > and as such it required a fix which was allegedly provided by          
> > > > > > > 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is    
> > > > > > > serving this functionality").  However, that fix was superficial as the
> > > > > > > module still registered its vendor-specific ACPI driver for backlight  
> > > > > > > control.  That in turn caused SBLL/SBL2 to be called when brightness   
> > > > > > > hotkeys were pressed even when acpi_video was supposed to handle       
> > > > > > > brightness changes, which is exactly what that patch was hoping to     
> > > > > > > prevent.  Moreover, the module unconditionally exports backlight-related
> > > > > > > sysfs attributes which enable userspace to change the brightness level,
> > > > > > > which should also only be possible when vendor backlight control is    
> > > > > > > used.  Fast forward several years and on modern Fujitsu machines (e.g. 
> > > > > > > Lifebook E744), the kernel automatically detects that native backlight 
> > > > > > > control [1] should be used and SBLL becomes a noop [2].  This creates  
> > > > > > > confusion, because the driver claims that it can get/set LCD brightness
> > > > > > > level via the platform device's sysfs attributes, but it actually      
> > > > > > > cannot.  It gets even more confusing on Skylake machines on which the  
> > > > > > > FUJ02B1 ACPI device is not present at all.                           
> > 
> > Right, evolved.... time to take a step back and clean it up.
> > 
> > > > > > > The proposal I put forward in regard to the above is to remove the three
> > > > > > > backlight-related attributes (brightness_changed, lcd_level,
> > > > > > > max_brightness) from the platform driver's sysfs tree.  I believe the
> > > > > > > functions they implement should only be exposed through the backlight
> > > > > > > device, because the latter is only created when the user explicitly
> > > > > > > selects vendor backlight control or it is automatically selected by the
> > > > > > > kernel (this should be the case for older machines).
> > 
> > This seems to be a good approach as in combination with the discussion below re
> > the ACPI device, it eliminates the sysfs attributes from the platform device
> > completely and makes the driver more consistent with others in the subsystem.
> > 
> > > > > > I will need to do some more digging into the historical developments.  At
> > > > > > face value I'm not sure how machines like the S7020 would end up controlling
> > > > > > the backlight if these sysfs attributes were removed because on this machine
> > > > > > (at least last time I checked) the standard backlight/video drivers could
> > > > > > not effect brightness control on this hardware.  Or is there a side effect
> > > > > > that I have forgotten?
> > > > > 
> > > > > Let us not confuse three separate things that fujitsu-laptop enables:
> > > > > 
> > > > >   * changing LCD brightness using the keyboard,
> > > > >   * changing LCD brightness from userspace, using the backlight device,
> > > > >   * changing LCD brightness from userspace, using sysfs attributes.
> > > > > 
> > > > > I have nothing against the first two, apart from the notion that they
> > > > > should be more tightly coupled (i.e. one should not be enabled without
> > > > > the other one and vice versa).
> > > > > 
> > > > > I am all against the last one.  IMHO it is a duplicate, misplaced
> > > > > feature which only makes clean separation of components inside the
> > > > > driver hard.
> > > > > 
> > > > > > > That would leave us with the remaining three sysfs attributes of the
> > > > > > > platform device, namely dock, lid and radios.  These all depend on the
> > > > > > > FUJ02E3 ACPI device.  Which begs the question: shall we reassign them to
> > > > > > > that ACPI device and drop the platform device altogether?  This would
> > > > > > > logically be the correct thing to do (panasonic-laptop and toshiba_acpi
> > > > > > > already assign extra sysfs attributes to ACPI nodes).  But I understand
> > > > > > > that this would break an 8-year-old userspace interface as functions
> > > > > > > previously exposed through /sys/devices/platform/fujitsu-laptop would be
> > > > > > > moved to /sys/bus/acpi/devices/FUJ02E3:00.  If that is unacceptable, the
> > 
> > We can change the device sysfs attributes using versioning. We should of course
> > do our due diligence to discover if there are any users of this sysfs interface,
> > and if so, if they have strong objections to changing. But, if someone screams,
> > we can always revert.
> > 
> > GregKH - Can you please confirm the above? Moving an attribute is different than
> > the format and contents, which is what I explicitly documented in
> > Documentation/admin-guide/sysfs-rules.rst (last section).
> 
> Moving an attribute to a different device structure is usually a bad
> idea, if the userspace tool counting on it to be present in a specific
> place breaks.

Yes, I am familiar with that premise.  Here is the thing though: I am
unaware of any userspace tool which uses these attributes.  Though,
obviously, that does not mean such tools do not exist.

> And I really don't like putting random device attributes on "parent"
> devices.  I know Dmitry Torokhov disagrees about this though, his
> subsystem does like doing that.  But whatever you do, you should at
> least try to be consistent.

This is tricky, too.  The x86 platform driver subsystem currently mixes
both approaches, i.e. some drivers register a separate platform device
while others just attach sysfs attributes directly to ACPI device nodes.

> But, as you can't be consistent here, don't break userspace please, I'd
> recommend just leaving it alone for now.

Darren, in the light of the above I will be awaiting your final call on
this before posting any further patches touching this area.  My number
one priority was to drop the broken backlight-related attributes,
because leaving the other attributes where they currently are does not
prevent achieving a clean split between the two drivers registered by
fujitsu-laptop, which is the ultimate objective of all these cleanups.

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/4] fujitsu_init() cleanup

[Jonathan Woithe]

On Tue, Feb 28, 2017 at 09:07:04AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> > > > > > > The problem I have with this driver is that it is generally oblivious to
> > > > > > > the user's chosen backlight provider.  It was written before acpi_video
> > > > > > > support for backlight control was automatically detected by the kernel 
> > > > > > > and as such it required a fix which was allegedly provided by          
> > > > > > > 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is    
> > > > > > > serving this functionality").  However, that fix was superficial as the
> > > > > > > module still registered its vendor-specific ACPI driver for backlight  
> > > > > > > control.  That in turn caused SBLL/SBL2 to be called when brightness   
> > > > > > > hotkeys were pressed even when acpi_video was supposed to handle       
> > > > > > > brightness changes, which is exactly what that patch was hoping to     
> > > > > > > prevent.  Moreover, the module unconditionally exports backlight-related
> > > > > > > sysfs attributes which enable userspace to change the brightness level,
> > > > > > > which should also only be possible when vendor backlight control is    
> > > > > > > used.  Fast forward several years and on modern Fujitsu machines (e.g. 
> > > > > > > Lifebook E744), the kernel automatically detects that native backlight 
> > > > > > > control [1] should be used and SBLL becomes a noop [2].  This creates  
> > > > > > > confusion, because the driver claims that it can get/set LCD brightness
> > > > > > > level via the platform device's sysfs attributes, but it actually      
> > > > > > > cannot.  It gets even more confusing on Skylake machines on which the  
> > > > > > > FUJ02B1 ACPI device is not present at all.                           
> > 
> > Right, evolved.... time to take a step back and clean it up.
> > 
> > > > > > > The proposal I put forward in regard to the above is to remove the three
> > > > > > > backlight-related attributes (brightness_changed, lcd_level,
> > > > > > > max_brightness) from the platform driver's sysfs tree.  I believe the
> > > > > > > functions they implement should only be exposed through the backlight
> > > > > > > device, because the latter is only created when the user explicitly
> > > > > > > selects vendor backlight control or it is automatically selected by the
> > > > > > > kernel (this should be the case for older machines).
> > 
> > This seems to be a good approach as in combination with the discussion below re
> > the ACPI device, it eliminates the sysfs attributes from the platform device
> > completely and makes the driver more consistent with others in the subsystem.

I have now had a chance to revisit the historical background, especially as
it relates to the three sysfs attributes mentioned (brightness_changed,
lcd_level, max_brightness).  It seems to me that these are most likely left
overs from a very early version of fujitsu-laptop, originally out-of-tree,
merged to mainline by myself and others.  Like Michael I doubt there is any
userspace utility which makes use of these.  Checking the scripts which I
personally use on my S7020 I see that I only ever use the attributes
provided under/sys/devices/virtual/backlight/fujitsu-laptop when controlling
the backlight.  Like Michael said, it's impossible to completely rule out
the possiblity of an obscure Fujitsu-only utility somewhere in the universe
which makes use of the /sys/devices/platform/fujitsu-laptop/ backlight
attributes but I think the probability of such a thing existing is
vanishingly small.  Even if there was, there is a mostly trivial mapping
from old to new (lcd_level -> brightness, max_brightness -> max_brightness). 
Only brightness_changed isn't replicated but I'm sure something could be
hooked if necessary.

What this means is that from my perspective it is highly unlikely that
removing the three sysfs attributes as proposed by Michael
(brightness_changed, lcd_level, max_brightness) will break userspace in a
practical sense.  Instead, it will improve the structure of the
fujitsu-laptop driver and make it more consistent with other platform
drivers.  Putting all this together, I have no objections to the removal as
proposed by Michael: there appears to be far more gains than losses.

> > > > > > > That would leave us with the remaining three sysfs attributes of the
> > > > > > > platform device, namely dock, lid and radios.  These all depend on the
> > > > > > > FUJ02E3 ACPI device.  Which begs the question: shall we reassign them to
> > > > > > > that ACPI device and drop the platform device altogether?  This would
> > > > > > > logically be the correct thing to do (panasonic-laptop and toshiba_acpi
> > > > > > > already assign extra sysfs attributes to ACPI nodes).  But I understand
> > > > > > > that this would break an 8-year-old userspace interface as functions
> > > > > > > previously exposed through /sys/devices/platform/fujitsu-laptop would be
> > > > > > > moved to /sys/bus/acpi/devices/FUJ02E3:00.  If that is unacceptable, the
> > 
> > We can change the device sysfs attributes using versioning. We should of course
> > do our due diligence to discover if there are any users of this sysfs interface,
> > and if so, if they have strong objections to changing. But, if someone screams,
> > we can always revert.
> > 
> > GregKH - Can you please confirm the above? Moving an attribute is different than
> > the format and contents, which is what I explicitly documented in
> > Documentation/admin-guide/sysfs-rules.rst (last section).
> 
> Moving an attribute to a different device structure is usually a bad
> idea, if the userspace tool counting on it to be present in a specific
> place breaks.

This isn't really a move in the true sense of the word.  The equivalent
standard backlight attributes have existed in
/sys/devices/virtual/backlight/fujitsu-laptop/ probably since the very
beginning and I suspect these are the ones people would be using since they
hook into the standard backlight subsystem.  Michael's proposal is to remove
the brightness_changed, lcd_level and max_brightness from
/sys/devices/platform/fujitsu-laptop on the basis that the backlight
equivalents are what people should be using and most likely are.  The
attributes provided by fujitsu-laptop via the backlight subsystem would
continue to exist (assuming I've understood Michael's proposal correctly).

> And I really don't like putting random device attributes on "parent"
> devices.  I know Dmitry Torokhov disagrees about this though, his
> subsystem does like doing that.  But whatever you do, you should at
> least try to be consistent.
> 
> But, as you can't be consistent here, don't break userspace please, I'd
> recommend just leaving it alone for now.

Ultimately I'm happy to defer to the direction given by Darren, GregKH et
al.  For what it's worth, from my point of view I would be happy for the
deletions to occur for the reasons stated above.  If however the need to
retain these old (most likely unused) sysfs attributes is never-the-less
immutable then they have obviously to stay.

Regards
  jonathan

Re: [PATCH 0/4] fujitsu_init() cleanup

[Jonathan Woithe]

On Tue, Feb 28, 2017 at 09:33:28AM +0100, Micha?? K??pie?? wrote:
> GregKH wrote:
> > On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> > > GregKH - Can you please confirm the above? Moving an attribute is different than
> > > the format and contents, which is what I explicitly documented in
> > > Documentation/admin-guide/sysfs-rules.rst (last section).
> > 
> > Moving an attribute to a different device structure is usually a bad
> > idea, if the userspace tool counting on it to be present in a specific
> > place breaks.
> 
> Yes, I am familiar with that premise.  Here is the thing though: I am
> unaware of any userspace tool which uses these attributes.  Though,
> obviously, that does not mean such tools do not exist.

For what it's worth I too am unaware of any utilities which use the
/sys/devices/platform/fujitsu-laptop/ attributes associated with the
backlight - this after using the S7020 since 2005.  I would be surprised if
any existed since they would have to be specifically for the Fujitsu
hardware.  If writing any utility to control the backlight the logical thing
to do would be to use the standard backlight attributes in
/sys/devices/virtual/backlight/fujitsu-laptop/.

> > But, as you can't be consistent here, don't break userspace please, I'd
> > recommend just leaving it alone for now.
> 
> Darren, in the light of the above I will be awaiting your final call on
> this before posting any further patches touching this area.  My number
> one priority was to drop the broken backlight-related attributes,
> because leaving the other attributes where they currently are does not
> prevent achieving a clean split between the two drivers registered by
> fujitsu-laptop, which is the ultimate objective of all these cleanups.

As I explained in my response to GregKH earlier and having investigated this
in more detail, I have no objection to the removal of the non-standard
backlight-related sysfs attributes (brightness_changed, lcd_level and
max_brightness).  They are almost certainly unused and their removal will
allow a significant cleanup of fujitsu-laptop.  Obviously however there are
competing viewpoints which take a bigger picture view so I will defer to
Darren's judgement.

Regards
  jonathan

Re: [PATCH 0/4] fujitsu_init() cleanup

[Greg Kroah-Hartman]

On Tue, Feb 28, 2017 at 09:44:26PM +1030, Jonathan Woithe wrote:
> On Tue, Feb 28, 2017 at 09:07:04AM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> > > > > > > > The problem I have with this driver is that it is generally oblivious to
> > > > > > > > the user's chosen backlight provider.  It was written before acpi_video
> > > > > > > > support for backlight control was automatically detected by the kernel 
> > > > > > > > and as such it required a fix which was allegedly provided by          
> > > > > > > > 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if video.ko is    
> > > > > > > > serving this functionality").  However, that fix was superficial as the
> > > > > > > > module still registered its vendor-specific ACPI driver for backlight  
> > > > > > > > control.  That in turn caused SBLL/SBL2 to be called when brightness   
> > > > > > > > hotkeys were pressed even when acpi_video was supposed to handle       
> > > > > > > > brightness changes, which is exactly what that patch was hoping to     
> > > > > > > > prevent.  Moreover, the module unconditionally exports backlight-related
> > > > > > > > sysfs attributes which enable userspace to change the brightness level,
> > > > > > > > which should also only be possible when vendor backlight control is    
> > > > > > > > used.  Fast forward several years and on modern Fujitsu machines (e.g. 
> > > > > > > > Lifebook E744), the kernel automatically detects that native backlight 
> > > > > > > > control [1] should be used and SBLL becomes a noop [2].  This creates  
> > > > > > > > confusion, because the driver claims that it can get/set LCD brightness
> > > > > > > > level via the platform device's sysfs attributes, but it actually      
> > > > > > > > cannot.  It gets even more confusing on Skylake machines on which the  
> > > > > > > > FUJ02B1 ACPI device is not present at all.                           
> > > 
> > > Right, evolved.... time to take a step back and clean it up.
> > > 
> > > > > > > > The proposal I put forward in regard to the above is to remove the three
> > > > > > > > backlight-related attributes (brightness_changed, lcd_level,
> > > > > > > > max_brightness) from the platform driver's sysfs tree.  I believe the
> > > > > > > > functions they implement should only be exposed through the backlight
> > > > > > > > device, because the latter is only created when the user explicitly
> > > > > > > > selects vendor backlight control or it is automatically selected by the
> > > > > > > > kernel (this should be the case for older machines).
> > > 
> > > This seems to be a good approach as in combination with the discussion below re
> > > the ACPI device, it eliminates the sysfs attributes from the platform device
> > > completely and makes the driver more consistent with others in the subsystem.
> 
> I have now had a chance to revisit the historical background, especially as
> it relates to the three sysfs attributes mentioned (brightness_changed,
> lcd_level, max_brightness).  It seems to me that these are most likely left
> overs from a very early version of fujitsu-laptop, originally out-of-tree,
> merged to mainline by myself and others.  Like Michael I doubt there is any
> userspace utility which makes use of these.  Checking the scripts which I
> personally use on my S7020 I see that I only ever use the attributes
> provided under/sys/devices/virtual/backlight/fujitsu-laptop when controlling
> the backlight.  Like Michael said, it's impossible to completely rule out
> the possiblity of an obscure Fujitsu-only utility somewhere in the universe
> which makes use of the /sys/devices/platform/fujitsu-laptop/ backlight
> attributes but I think the probability of such a thing existing is
> vanishingly small.  Even if there was, there is a mostly trivial mapping
> from old to new (lcd_level -> brightness, max_brightness -> max_brightness). 
> Only brightness_changed isn't replicated but I'm sure something could be
> hooked if necessary.
> 
> What this means is that from my perspective it is highly unlikely that
> removing the three sysfs attributes as proposed by Michael
> (brightness_changed, lcd_level, max_brightness) will break userspace in a
> practical sense.  Instead, it will improve the structure of the
> fujitsu-laptop driver and make it more consistent with other platform
> drivers.  Putting all this together, I have no objections to the removal as
> proposed by Michael: there appears to be far more gains than losses.

As the person who wrote the original driver, for Fujitsu, I can almost
guarantee that there is no specific "fujitsu utility" that uses that
platform driver.  We just used the "normal" backlight/led interface
instead, so all should be fine there.

thanks,

greg k-h

Re: [PATCH 0/4] fujitsu_init() cleanup

[Michał Kępień]

> On Tue, Feb 28, 2017 at 09:33:28AM +0100, Micha?? K??pie?? wrote:
> > GregKH wrote:
> > > On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> > > > GregKH - Can you please confirm the above? Moving an attribute is different than
> > > > the format and contents, which is what I explicitly documented in
> > > > Documentation/admin-guide/sysfs-rules.rst (last section).
> > > 
> > > Moving an attribute to a different device structure is usually a bad
> > > idea, if the userspace tool counting on it to be present in a specific
> > > place breaks.
> > 
> > Yes, I am familiar with that premise.  Here is the thing though: I am
> > unaware of any userspace tool which uses these attributes.  Though,
> > obviously, that does not mean such tools do not exist.
> 
> For what it's worth I too am unaware of any utilities which use the
> /sys/devices/platform/fujitsu-laptop/ attributes associated with the
> backlight - this after using the S7020 since 2005.  I would be surprised if
> any existed since they would have to be specifically for the Fujitsu
> hardware.  If writing any utility to control the backlight the logical thing
> to do would be to use the standard backlight attributes in
> /sys/devices/virtual/backlight/fujitsu-laptop/.
> 
> > > But, as you can't be consistent here, don't break userspace please, I'd
> > > recommend just leaving it alone for now.
> > 
> > Darren, in the light of the above I will be awaiting your final call on
> > this before posting any further patches touching this area.  My number
> > one priority was to drop the broken backlight-related attributes,
> > because leaving the other attributes where they currently are does not
> > prevent achieving a clean split between the two drivers registered by
> > fujitsu-laptop, which is the ultimate objective of all these cleanups.
> 
> As I explained in my response to GregKH earlier and having investigated this
> in more detail, I have no objection to the removal of the non-standard
> backlight-related sysfs attributes (brightness_changed, lcd_level and
> max_brightness).  They are almost certainly unused and their removal will
> allow a significant cleanup of fujitsu-laptop.  Obviously however there are
> competing viewpoints which take a bigger picture view so I will defer to
> Darren's judgement.

Okay, so it looks like I did the best I could to explain my intentions
and some confusion crept in anyway, sorry about that.  Let me try again,
I will be as concise as I can.

Current custom attributes attached to the platform device are:

    /sys/bus/platform/devices/fujitsu-laptop
    `- brightness_changed [*]
    `- dock
    `- lcd_level [*]
    `- lid
    `- max_brightness [*]
    `- radios

AFAICT, all four of us agree that attributes marked with an asterisk [*]
should be removed from the platform device because they are exposed by
the backlight device anyway and do not work correctly under certain
circumstances.

However, Darren's question to Greg did not apply to these attributes.
It was about the other three attributes: dock, lid and radios.

In other words, my proposal was to change this:

    /sys/bus/platform/devices/fujitsu-laptop
    `- dock
    `- lid
    `- radios

into this:

    /sys/bus/acpi/devices/FUJ02E3:00
    `- dock
    `- lid
    `- radios

That would enable us to completely rip the platform driver and platform
device out of fujitsu-laptop, cutting the driver down by ca. 40 lines.

Greg objected to that, arguing that there might be userspace
applications using these attributes under their current path.  I do not
know of such an application.  My question to Darren was thus: do we move
dock, lid and radios and rip the platform driver and platform device out
of fujitsu-laptop or should these attributes rather stay where they are?

Sorry that laying this all out this takes so much space, but this is
exactly why these cleanups are happening.

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/4] fujitsu_init() cleanup

[Greg Kroah-Hartman]

On Tue, Feb 28, 2017 at 02:24:40PM +0100, Michał Kępień wrote:
> > On Tue, Feb 28, 2017 at 09:33:28AM +0100, Micha?? K??pie?? wrote:
> > > GregKH wrote:
> > > > On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> > > > > GregKH - Can you please confirm the above? Moving an attribute is different than
> > > > > the format and contents, which is what I explicitly documented in
> > > > > Documentation/admin-guide/sysfs-rules.rst (last section).
> > > > 
> > > > Moving an attribute to a different device structure is usually a bad
> > > > idea, if the userspace tool counting on it to be present in a specific
> > > > place breaks.
> > > 
> > > Yes, I am familiar with that premise.  Here is the thing though: I am
> > > unaware of any userspace tool which uses these attributes.  Though,
> > > obviously, that does not mean such tools do not exist.
> > 
> > For what it's worth I too am unaware of any utilities which use the
> > /sys/devices/platform/fujitsu-laptop/ attributes associated with the
> > backlight - this after using the S7020 since 2005.  I would be surprised if
> > any existed since they would have to be specifically for the Fujitsu
> > hardware.  If writing any utility to control the backlight the logical thing
> > to do would be to use the standard backlight attributes in
> > /sys/devices/virtual/backlight/fujitsu-laptop/.
> > 
> > > > But, as you can't be consistent here, don't break userspace please, I'd
> > > > recommend just leaving it alone for now.
> > > 
> > > Darren, in the light of the above I will be awaiting your final call on
> > > this before posting any further patches touching this area.  My number
> > > one priority was to drop the broken backlight-related attributes,
> > > because leaving the other attributes where they currently are does not
> > > prevent achieving a clean split between the two drivers registered by
> > > fujitsu-laptop, which is the ultimate objective of all these cleanups.
> > 
> > As I explained in my response to GregKH earlier and having investigated this
> > in more detail, I have no objection to the removal of the non-standard
> > backlight-related sysfs attributes (brightness_changed, lcd_level and
> > max_brightness).  They are almost certainly unused and their removal will
> > allow a significant cleanup of fujitsu-laptop.  Obviously however there are
> > competing viewpoints which take a bigger picture view so I will defer to
> > Darren's judgement.
> 
> Okay, so it looks like I did the best I could to explain my intentions
> and some confusion crept in anyway, sorry about that.  Let me try again,
> I will be as concise as I can.
> 
> Current custom attributes attached to the platform device are:
> 
>     /sys/bus/platform/devices/fujitsu-laptop
>     `- brightness_changed [*]
>     `- dock
>     `- lcd_level [*]
>     `- lid
>     `- max_brightness [*]
>     `- radios
> 
> AFAICT, all four of us agree that attributes marked with an asterisk [*]
> should be removed from the platform device because they are exposed by
> the backlight device anyway and do not work correctly under certain
> circumstances.
> 
> However, Darren's question to Greg did not apply to these attributes.
> It was about the other three attributes: dock, lid and radios.
> 
> In other words, my proposal was to change this:
> 
>     /sys/bus/platform/devices/fujitsu-laptop
>     `- dock
>     `- lid
>     `- radios
> 
> into this:
> 
>     /sys/bus/acpi/devices/FUJ02E3:00
>     `- dock
>     `- lid
>     `- radios
> 
> That would enable us to completely rip the platform driver and platform
> device out of fujitsu-laptop, cutting the driver down by ca. 40 lines.
> 
> Greg objected to that, arguing that there might be userspace
> applications using these attributes under their current path.  I do not
> know of such an application.  My question to Darren was thus: do we move
> dock, lid and radios and rip the platform driver and platform device out
> of fujitsu-laptop or should these attributes rather stay where they are?

I would say just to leave them as-is, there's no harm in those 40 lines,
and you really don't want to break someone's existing setup for no good
reason.

thanks,

greg k-h

Re: [PATCH 0/4] fujitsu_init() cleanup

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 4:01 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Feb 28, 2017 at 02:24:40PM +0100, Michał Kępień wrote:
>> > On Tue, Feb 28, 2017 at 09:33:28AM +0100, Micha?? K??pie?? wrote:
>> > > GregKH wrote:

>> Greg objected to that, arguing that there might be userspace
>> applications using these attributes under their current path.  I do not
>> know of such an application.  My question to Darren was thus: do we move
>> dock, lid and radios and rip the platform driver and platform device out
>> of fujitsu-laptop or should these attributes rather stay where they are?
>
> I would say just to leave them as-is, there's no harm in those 40 lines,
> and you really don't want to break someone's existing setup for no good
> reason.

Michał, from design point of view you can split it to two parts if
*makes sense* / *needed*: core and platform driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] fujitsu_init() cleanup

[Darren Hart]

On Tue, Feb 28, 2017 at 03:01:52PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 28, 2017 at 02:24:40PM +0100, Michał Kępień wrote:
> > > On Tue, Feb 28, 2017 at 09:33:28AM +0100, Micha?? K??pie?? wrote:
> > > > GregKH wrote:
> > > > > On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> > > > > > GregKH - Can you please confirm the above? Moving an attribute is different than
> > > > > > the format and contents, which is what I explicitly documented in
> > > > > > Documentation/admin-guide/sysfs-rules.rst (last section).
> > > > > 
> > > > > Moving an attribute to a different device structure is usually a bad
> > > > > idea, if the userspace tool counting on it to be present in a specific
> > > > > place breaks.
> > > > 
> > > > Yes, I am familiar with that premise.  Here is the thing though: I am
> > > > unaware of any userspace tool which uses these attributes.  Though,
> > > > obviously, that does not mean such tools do not exist.
> > > 
> > > For what it's worth I too am unaware of any utilities which use the
> > > /sys/devices/platform/fujitsu-laptop/ attributes associated with the
> > > backlight - this after using the S7020 since 2005.  I would be surprised if
> > > any existed since they would have to be specifically for the Fujitsu
> > > hardware.  If writing any utility to control the backlight the logical thing
> > > to do would be to use the standard backlight attributes in
> > > /sys/devices/virtual/backlight/fujitsu-laptop/.
> > > 
> > > > > But, as you can't be consistent here, don't break userspace please, I'd
> > > > > recommend just leaving it alone for now.
> > > > 
> > > > Darren, in the light of the above I will be awaiting your final call on
> > > > this before posting any further patches touching this area.  My number
> > > > one priority was to drop the broken backlight-related attributes,
> > > > because leaving the other attributes where they currently are does not
> > > > prevent achieving a clean split between the two drivers registered by
> > > > fujitsu-laptop, which is the ultimate objective of all these cleanups.
> > > 
> > > As I explained in my response to GregKH earlier and having investigated this
> > > in more detail, I have no objection to the removal of the non-standard
> > > backlight-related sysfs attributes (brightness_changed, lcd_level and
> > > max_brightness).  They are almost certainly unused and their removal will
> > > allow a significant cleanup of fujitsu-laptop.  Obviously however there are
> > > competing viewpoints which take a bigger picture view so I will defer to
> > > Darren's judgement.
> > 
> > Okay, so it looks like I did the best I could to explain my intentions
> > and some confusion crept in anyway, sorry about that.  Let me try again,
> > I will be as concise as I can.
> > 
> > Current custom attributes attached to the platform device are:
> > 
> >     /sys/bus/platform/devices/fujitsu-laptop
> >     `- brightness_changed [*]
> >     `- dock
> >     `- lcd_level [*]
> >     `- lid
> >     `- max_brightness [*]
> >     `- radios
> > 
> > AFAICT, all four of us agree that attributes marked with an asterisk [*]
> > should be removed from the platform device because they are exposed by
> > the backlight device anyway and do not work correctly under certain
> > circumstances.

Yes, please kill these.

> > 
> > However, Darren's question to Greg did not apply to these attributes.
> > It was about the other three attributes: dock, lid and radios.
> > 
> > In other words, my proposal was to change this:
> > 
> >     /sys/bus/platform/devices/fujitsu-laptop
> >     `- dock
> >     `- lid
> >     `- radios
> > 
> > into this:
> > 
> >     /sys/bus/acpi/devices/FUJ02E3:00
> >     `- dock
> >     `- lid
> >     `- radios
> > 
> > That would enable us to completely rip the platform driver and platform
> > device out of fujitsu-laptop, cutting the driver down by ca. 40 lines.
> > 
> > Greg objected to that, arguing that there might be userspace
> > applications using these attributes under their current path.  I do not
> > know of such an application.  My question to Darren was thus: do we move
> > dock, lid and radios and rip the platform driver and platform device out
> > of fujitsu-laptop or should these attributes rather stay where they are?
> 
> I would say just to leave them as-is, there's no harm in those 40 lines,
> and you really don't want to break someone's existing setup for no good
> reason.

In a clean room I'd definitely opt for the move to eliminate the extra
infrastructure of the platform driver - and binding to a HID is a superior
enumeration mechanism.

However, changing this interface doesn't fix any bugs and it breaks a user space
interface. Greg made it clear this commitment is stronger than I had positioned
it for sysfs attributes. Please leave the platform driver code for dock, lid,
and radios in place.

Thank you everyone for the time to clarify.

-- 
Darren Hart
Intel Open Source Technology Center