Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1629573 > unrolled thread

[PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-04-24 15:40 +0200
    [PATCH 09/10] platform/x86: fujitsu-laptop: use device-specific data in LED-related code Michał Kępień <kernel@kempniu.pl> - 2017-04-24 15:40 +0200
    [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization Michał Kępień <kernel@kempniu.pl> - 2017-04-24 15:40 +0200
      Re: [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight  power synchronization Jonathan Woithe <jwoithe@just42.net> - 2017-05-01 15:40 +0200
        Re: [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight  power synchronization Darren Hart <dvhart@infradead.org> - 2017-05-01 18:20 +0200
    [PATCH 06/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_bl in acpi_fujitsu_bl_add() Michał Kępień <kernel@kempniu.pl> - 2017-04-24 15:40 +0200
    [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data Michał Kępień <kernel@kempniu.pl> - 2017-04-24 15:40 +0200
      Re: [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current  uses of device-specific data Jonathan Woithe <jwoithe@just42.net> - 2017-05-01 15:50 +0200
    Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Jonathan Woithe <jwoithe@just42.net> - 2017-05-01 15:10 +0200
      Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-02 15:30 +0200
        Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Jonathan Woithe <jwoithe@just42.net> - 2017-05-05 01:50 +0200
          Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Darren Hart <dvhart@infradead.org> - 2017-05-05 18:20 +0200
            Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-06 14:40 +0200
              Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-06 14:50 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of  module-wide globals Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-06 16:30 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of  module-wide globals Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-06 16:30 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Darren Hart <dvhart@infradead.org> - 2017-05-08 18:10 +0200
                  Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-09 11:40 +0200
                    Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Jonathan Woithe <jwoithe@just42.net> - 2017-05-09 14:20 +0200
                    Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Darren Hart <dvhart@infradead.org> - 2017-05-09 18:50 +0200
                      Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-05-09 23:40 +0200
                        Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-11 16:00 +0200
                          Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-05-11 16:50 +0200
                            Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Darren Hart <dvhart@infradead.org> - 2017-05-11 17:40 +0200
                      Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-11 15:50 +0200

Page 1 of 2  [1] 2  Next page →

#1629573 — [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

fujitsu-laptop registers two ACPI drivers.  Whenever an ACPI device with
a matching identifier is found by the ACPI bus, a new instance of the
relevant driver is bound to that ACPI device.  However, both ACPI
drivers registered by fujitsu-laptop access module-wide global data
structures, assuming neither ACPI driver will ever be instantiated more
than once.  While there are currently no indications of such issues
happening in the wild, it is theoretically possible for multiple
FUJ02B1/FUJ02E3 ACPI devices to be present in the firmware, which would
cause two instances of the relevant driver to simultaneously access
module-wide globals without any locking in place.  Also, modern Fujitsu
laptops ship without the FUJ02B1 ACPI device present in firmware,
causing memory to be needlessly allocated inside fujitsu_init().

To future-proof the module and lay the groundwork for separating the two
aforementioned ACPI drivers into separate modules, move away from
module-wide global data structures by using device-specific data
instead.

This patch series was tested on a Lifebook S7020 and a Lifebook E744.

I found it challenging to adhere to the "one logical change per patch"
rule while touching code commonly used by almost all other module code.
If the changes introduced are illegible, I will be happy to further
explain and/or improve the series.  Please also note that while the diff
stats for this series may seem daunting at first, using --color-words
should hopefully make reviewing much more manageable.

 drivers/platform/x86/fujitsu-laptop.c | 460 +++++++++++++++++-----------------
 1 file changed, 236 insertions(+), 224 deletions(-)

-- 
2.12.2

[toc] | [next] | [standalone]

#1629578 — [PATCH 09/10] platform/x86: fujitsu-laptop: use device-specific data in LED-related code

In order to perform their duties, all LED callbacks need a handle to the
FUJ02E3 ACPI device.  If that handle is to be fetched without accessing
module-wide data, it needs to be extracted from data that gets passed to
the LED callbacks as arguments.  However, LED core does not currently
support supplying driver-specific pointers to struct led_classdev
callbacks, so the latter will have to be implemented a bit differently
than backlight device callbacks and platform device attribute callbacks.

As the FUJ02E3 ACPI device is the parent device of all LED class devices
registered by fujitsu-laptop, we can get to the struct acpi_device
representing the former by following the parent link present inside the
struct device belonging to the struct led_classdev passed as an argument
to each LED callback.  Note that acpi_driver_data() is not used to
retrieve the ACPI handle.  Doing that would break cleanup upon module
removal, because the managed LED class device release callback,
devm_led_classdev_release(), would be called after the bus-level device
removal callback, acpi_device_remove(), which sets the driver_data
member of struct acpi_device to NULL and as devm_led_classdev_release()
calls led_classdev_unregister(), which in turn resets the LED's
brightness to LED_OFF, LED callbacks would not be able to retrieve the
ACPI handle needed to perform that operation.  To work around this, the
handle is retrieved directly from struct acpi_device as the latter is
guaranteed to be available upon brightness reset because parent device's
reference count is only decremented once brightness gets reset.

To get rid of module-wide structures defining LED class devices,
allocate them dynamically using devm_kzalloc() and initialize them in
acpi_fujitsu_laptop_leds_register().

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index f26abc41266e..77082e35f26a 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -635,6 +635,7 @@ static void fujitsu_laptop_platform_remove(void)
 static int logolamp_set(struct led_classdev *cdev,
 			enum led_brightness brightness)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	int poweron = FUNC_LED_ON, always = FUNC_LED_ON;
 	int ret;
 
@@ -644,136 +645,120 @@ static int logolamp_set(struct led_classdev *cdev,
 	if (brightness < LED_FULL)
 		always = FUNC_LED_OFF;
 
-	ret = fext_leds(fujitsu_laptop->handle, 0x1, LOGOLAMP_POWERON, poweron);
+	ret = fext_leds(handle, 0x1, LOGOLAMP_POWERON, poweron);
 	if (ret < 0)
 		return ret;
 
-	return fext_leds(fujitsu_laptop->handle, 0x1, LOGOLAMP_ALWAYS, always);
+	return fext_leds(handle, 0x1, LOGOLAMP_ALWAYS, always);
 }
 
 static enum led_brightness logolamp_get(struct led_classdev *cdev)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	int ret;
 
-	ret = fext_leds(fujitsu_laptop->handle, 0x2, LOGOLAMP_ALWAYS, 0x0);
+	ret = fext_leds(handle, 0x2, LOGOLAMP_ALWAYS, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_FULL;
 
-	ret = fext_leds(fujitsu_laptop->handle, 0x2, LOGOLAMP_POWERON, 0x0);
+	ret = fext_leds(handle, 0x2, LOGOLAMP_POWERON, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_HALF;
 
 	return LED_OFF;
 }
 
-static struct led_classdev logolamp_led = {
-	.name = "fujitsu::logolamp",
-	.brightness_set_blocking = logolamp_set,
-	.brightness_get = logolamp_get
-};
-
 static int kblamps_set(struct led_classdev *cdev,
 		       enum led_brightness brightness)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
+
 	if (brightness >= LED_FULL)
-		return fext_leds(fujitsu_laptop->handle, 0x1, KEYBOARD_LAMPS,
-				 FUNC_LED_ON);
+		return fext_leds(handle, 0x1, KEYBOARD_LAMPS, FUNC_LED_ON);
 	else
-		return fext_leds(fujitsu_laptop->handle, 0x1, KEYBOARD_LAMPS,
-				 FUNC_LED_OFF);
+		return fext_leds(handle, 0x1, KEYBOARD_LAMPS, FUNC_LED_OFF);
 }
 
 static enum led_brightness kblamps_get(struct led_classdev *cdev)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	enum led_brightness brightness = LED_OFF;
 
-	if (fext_leds(fujitsu_laptop->handle,
-		      0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
+	if (fext_leds(handle, 0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
 }
 
-static struct led_classdev kblamps_led = {
-	.name = "fujitsu::kblamps",
-	.brightness_set_blocking = kblamps_set,
-	.brightness_get = kblamps_get
-};
-
 static int radio_led_set(struct led_classdev *cdev,
 			 enum led_brightness brightness)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
+
 	if (brightness >= LED_FULL)
-		return fext_flags(fujitsu_laptop->handle, 0x5, RADIO_LED_ON,
-				  RADIO_LED_ON);
+		return fext_flags(handle, 0x5, RADIO_LED_ON, RADIO_LED_ON);
 	else
-		return fext_flags(fujitsu_laptop->handle, 0x5, RADIO_LED_ON,
-				  0x0);
+		return fext_flags(handle, 0x5, RADIO_LED_ON, 0x0);
 }
 
 static enum led_brightness radio_led_get(struct led_classdev *cdev)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	enum led_brightness brightness = LED_OFF;
 
-	if (fext_flags(fujitsu_laptop->handle, 0x4, 0x0, 0x0) & RADIO_LED_ON)
+	if (fext_flags(handle, 0x4, 0x0, 0x0) & RADIO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
 }
 
-static struct led_classdev radio_led = {
-	.name = "fujitsu::radio_led",
-	.brightness_set_blocking = radio_led_set,
-	.brightness_get = radio_led_get,
-	.default_trigger = "rfkill-any"
-};
-
 static int eco_led_set(struct led_classdev *cdev,
 		       enum led_brightness brightness)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	int curr;
 
-	curr = fext_leds(fujitsu_laptop->handle, 0x2, ECO_LED, 0x0);
+	curr = fext_leds(handle, 0x2, ECO_LED, 0x0);
 	if (brightness >= LED_FULL)
-		return fext_leds(fujitsu_laptop->handle, 0x1, ECO_LED,
-				 curr | ECO_LED_ON);
+		return fext_leds(handle, 0x1, ECO_LED, curr | ECO_LED_ON);
 	else
-		return fext_leds(fujitsu_laptop->handle, 0x1, ECO_LED,
-				 curr & ~ECO_LED_ON);
+		return fext_leds(handle, 0x1, ECO_LED, curr & ~ECO_LED_ON);
 }
 
 static enum led_brightness eco_led_get(struct led_classdev *cdev)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	enum led_brightness brightness = LED_OFF;
 
-	if (fext_leds(fujitsu_laptop->handle, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
+	if (fext_leds(handle, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
 }
 
-static struct led_classdev eco_led = {
-	.name = "fujitsu::eco_led",
-	.brightness_set_blocking = eco_led_set,
-	.brightness_get = eco_led_get
-};
-
 static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 {
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
+	struct led_classdev *led;
 	int result;
 
-	if (fext_leds(fujitsu_laptop->handle,
-		      0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
-		result = devm_led_classdev_register(&device->dev,
-						    &logolamp_led);
+	if (fext_leds(priv->handle, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
+		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
+		led->name = "fujitsu::logolamp";
+		led->brightness_set_blocking = logolamp_set;
+		led->brightness_get = logolamp_get;
+		result = devm_led_classdev_register(&device->dev, led);
 		if (result)
 			return result;
 	}
 
-	if ((fext_leds(fujitsu_laptop->handle,
-		       0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
-	    (fext_buttons(fujitsu_laptop->handle, 0x0, 0x0, 0x0) == 0x0)) {
-		result = devm_led_classdev_register(&device->dev, &kblamps_led);
+	if ((fext_leds(priv->handle, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
+	    (fext_buttons(priv->handle, 0x0, 0x0, 0x0) == 0x0)) {
+		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
+		led->name = "fujitsu::kblamps";
+		led->brightness_set_blocking = kblamps_set;
+		led->brightness_get = kblamps_get;
+		result = devm_led_classdev_register(&device->dev, led);
 		if (result)
 			return result;
 	}
@@ -784,8 +769,13 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	 * to also have an RF LED.  Therefore use bit 24 as an indicator
 	 * that an RF LED is present.
 	 */
-	if (fext_buttons(fujitsu_laptop->handle, 0x0, 0x0, 0x0) & BIT(24)) {
-		result = devm_led_classdev_register(&device->dev, &radio_led);
+	if (fext_buttons(priv->handle, 0x0, 0x0, 0x0) & BIT(24)) {
+		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
+		led->name = "fujitsu::radio_led";
+		led->brightness_set_blocking = radio_led_set;
+		led->brightness_get = radio_led_get;
+		led->default_trigger = "rfkill-any";
+		result = devm_led_classdev_register(&device->dev, led);
 		if (result)
 			return result;
 	}
@@ -795,10 +785,13 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	 * bit 14 seems to indicate presence of said led as well.
 	 * Confirm by testing the status.
 	 */
-	if ((fext_leds(fujitsu_laptop->handle, 0x0, 0x0, 0x0) & BIT(14)) &&
-	    (fext_leds(fujitsu_laptop->handle,
-		       0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
-		result = devm_led_classdev_register(&device->dev, &eco_led);
+	if ((fext_leds(priv->handle, 0x0, 0x0, 0x0) & BIT(14)) &&
+	    (fext_leds(priv->handle, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
+		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
+		led->name = "fujitsu::eco_led";
+		led->brightness_set_blocking = eco_led_set;
+		led->brightness_get = eco_led_get;
+		result = devm_led_classdev_register(&device->dev, led);
 		if (result)
 			return result;
 	}
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1629580 — [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization

fujitsu-laptop registers two ACPI drivers: one for ACPI device FUJ02B1
enabling backlight control and another for ACPI device FUJ02E3 which
handles various other stuff (hotkeys, LEDs, etc.)  So far, these two
drivers have been entangled by calls to fext_backlight() (previously
known as call_fext_func()) in the backlight part of the module which use
module-wide data managed by the other part of the module and accesses to
the backlight device from within acpi_fujitsu_laptop_add().  This
entaglement can be solved by storing an independently fetched ACPI
handle to the FUJ02E3 device inside the data structure managed by the
backlight part of the module.

Add a field to struct fujitsu_bl for storing a handle to the FUJ02E3
ACPI device.  Make fext_backlight() calls use that handle instead of the
one from struct fujitsu_laptop.  Move backlight power synchronization
from acpi_fujitsu_laptop_add() to fujitsu_backlight_register().

This makes the bl_device field of struct fujitsu_bl redundant, so remove
it.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index ea3210ee83ec..5f6b34a97348 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -131,9 +131,9 @@
 /* Device controlling the backlight and associated keys */
 struct fujitsu_bl {
 	acpi_handle handle;
+	acpi_handle fext_handle;
 	struct input_dev *input;
 	char phys[32];
-	struct backlight_device *bl_device;
 	unsigned int max_brightness;
 	unsigned int brightness_level;
 };
@@ -290,10 +290,12 @@ static int bl_get_brightness(struct backlight_device *b)
 
 static int bl_update_status(struct backlight_device *b)
 {
-	if (b->props.power == FB_BLANK_POWERDOWN)
-		fext_backlight(fujitsu_laptop->handle, 0x1, 0x4, 0x3);
-	else
-		fext_backlight(fujitsu_laptop->handle, 0x1, 0x4, 0x0);
+	if (fujitsu_bl->fext_handle) {
+		if (b->props.power == FB_BLANK_POWERDOWN)
+			fext_backlight(fujitsu_bl->fext_handle, 0x1, 0x4, 0x3);
+		else
+			fext_backlight(fujitsu_bl->fext_handle, 0x1, 0x4, 0x0);
+	}
 
 	return set_lcd_level(b->props.brightness);
 }
@@ -397,6 +399,7 @@ static int fujitsu_backlight_register(struct acpi_device *device)
 		.type = BACKLIGHT_PLATFORM
 	};
 	struct backlight_device *bd;
+	acpi_status status;
 
 	bd = devm_backlight_device_register(&device->dev, "fujitsu-laptop",
 					    &device->dev, NULL,
@@ -404,7 +407,10 @@ static int fujitsu_backlight_register(struct acpi_device *device)
 	if (IS_ERR(bd))
 		return PTR_ERR(bd);
 
-	fujitsu_bl->bl_device = bd;
+	status = acpi_get_handle(NULL, "\\_SB.FEXT", &fujitsu_bl->fext_handle);
+	if (ACPI_SUCCESS(status) &&
+	    fext_backlight(fujitsu_bl->fext_handle, 0x2, 0x4, 0x0) == 3)
+		bd->props.power = FB_BLANK_POWERDOWN;
 
 	return 0;
 }
@@ -861,15 +867,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	pr_info("BTNI: [0x%x]\n", fext_buttons(fujitsu_laptop->handle,
 					       0x0, 0x0, 0x0));
 
-	/* Sync backlight power status */
-	if (fujitsu_bl->bl_device &&
-	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		if (fext_backlight(fujitsu_laptop->handle, 0x2, 0x4, 0x0) == 3)
-			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
-		else
-			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
-	}
-
 	error = acpi_fujitsu_laptop_leds_register(device);
 	if (error)
 		goto err_free_fifo;
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1633667 — Re: [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization

On Mon, Apr 24, 2017 at 03:33:28PM +0200, Micha?? K??pie?? wrote:
> fujitsu-laptop registers two ACPI drivers: one for ACPI device FUJ02B1
> enabling backlight control and another for ACPI device FUJ02E3 which
> handles various other stuff (hotkeys, LEDs, etc.)  So far, these two
> drivers have been entangled by calls to fext_backlight() (previously
> known as call_fext_func()) in the backlight part of the module which use
> module-wide data managed by the other part of the module and accesses to
> the backlight device from within acpi_fujitsu_laptop_add().  This
> entaglement can be solved by storing an independently fetched ACPI
> handle to the FUJ02E3 device inside the data structure managed by the
> backlight part of the module.
> 
> Add a field to struct fujitsu_bl for storing a handle to the FUJ02E3
> ACPI device.  Make fext_backlight() calls use that handle instead of the
> one from struct fujitsu_laptop.  Move backlight power synchronization
> from acpi_fujitsu_laptop_add() to fujitsu_backlight_register().
> 
> This makes the bl_device field of struct fujitsu_bl redundant, so remove
> it.
> 
> Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/fujitsu-laptop.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index ea3210ee83ec..5f6b34a97348 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -131,9 +131,9 @@
>  /* Device controlling the backlight and associated keys */
>  struct fujitsu_bl {
>  	acpi_handle handle;
> +	acpi_handle fext_handle;

This is the extra "handle" field I eluded to in my comment about an earlier
part of this patch series.  The end result is two "handle" fields: one whose
job is obvious (fext_handle) and one whose name in no way reflects what it
might be used for (handle).  One could of course adopt the view that any
unqualified handle is a generic acpi handle, but I like the clarification
which comes with the additional suffix.  Perhaps "acpi_handle" is too
generic and I'm certainly not opposed to the use of something more specific. 
However, IMHO leaving it as "handle" seems like an unnecessary obfuscation
without much gain.

This change reinforces the shift away from ACPI drivers linked to specific
ACPI devices, and towards a focus on the driver's functionality (backlight
and "various other stuff").  With the evolution of the hardware I think this
makes sense.  While the "other stuff" still needs a driver, the backlight
component is not always needed.  The case for separation therefore makes
sense.

As a point of discussion though, since the backlight driver needs access to
both FUJ02B1 and FUJ02E3, should we consider rolling both ACPI drivers into
one?  Aside from conceptual neatness and perhaps a small runtime memory
footprint saving in the event that no backlight control functionality need
be provided by fujitsu-laptop, is there a whole lot to be gained through the
use of two separate drivers?

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1633731 — Re: [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization

On Mon, May 01, 2017 at 11:02:45PM +0930, Jonathan Woithe wrote:
> On Mon, Apr 24, 2017 at 03:33:28PM +0200, Micha?? K??pie?? wrote:
> > fujitsu-laptop registers two ACPI drivers: one for ACPI device FUJ02B1
> > enabling backlight control and another for ACPI device FUJ02E3 which
> > handles various other stuff (hotkeys, LEDs, etc.)  So far, these two
> > drivers have been entangled by calls to fext_backlight() (previously
> > known as call_fext_func()) in the backlight part of the module which use
> > module-wide data managed by the other part of the module and accesses to
> > the backlight device from within acpi_fujitsu_laptop_add().  This
> > entaglement can be solved by storing an independently fetched ACPI
> > handle to the FUJ02E3 device inside the data structure managed by the
> > backlight part of the module.
> > 
> > Add a field to struct fujitsu_bl for storing a handle to the FUJ02E3
> > ACPI device.  Make fext_backlight() calls use that handle instead of the
> > one from struct fujitsu_laptop.  Move backlight power synchronization
> > from acpi_fujitsu_laptop_add() to fujitsu_backlight_register().
> > 
> > This makes the bl_device field of struct fujitsu_bl redundant, so remove
> > it.
> > 
> > Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> > ---
> >  drivers/platform/x86/fujitsu-laptop.c | 27 ++++++++++++---------------
> >  1 file changed, 12 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index ea3210ee83ec..5f6b34a97348 100644
> > --- a/drivers/platform/x86/fujitsu-laptop.c
> > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > @@ -131,9 +131,9 @@
> >  /* Device controlling the backlight and associated keys */
> >  struct fujitsu_bl {
> >  	acpi_handle handle;
> > +	acpi_handle fext_handle;
> 
> This is the extra "handle" field I eluded to in my comment about an earlier
> part of this patch series.  The end result is two "handle" fields: one whose
> job is obvious (fext_handle) and one whose name in no way reflects what it
> might be used for (handle).  One could of course adopt the view that any
> unqualified handle is a generic acpi handle, but I like the clarification
> which comes with the additional suffix.  Perhaps "acpi_handle" is too
> generic and I'm certainly not opposed to the use of something more specific. 
> However, IMHO leaving it as "handle" seems like an unnecessary obfuscation
> without much gain.

Yeah, that's a fair criticism. Naming is hard. I can see the argument for
"handle" is for the system in general, "fext_handle" is for this specific subset
of functionality". The alternative appears to be "plt_handle", "fuj_handle",
"main_handle", or "hk_led_etc_handle ;-)" which honestly doesn't add any new
information or is just silly. So, if you want to prefix handle, go ahead and do
so, but I don't think it's a big deal.

> 
> This change reinforces the shift away from ACPI drivers linked to specific
> ACPI devices, and towards a focus on the driver's functionality (backlight
> and "various other stuff").  With the evolution of the hardware I think this
> makes sense.  While the "other stuff" still needs a driver, the backlight
> component is not always needed.  The case for separation therefore makes
> sense.
> 
> As a point of discussion though, since the backlight driver needs access to
> both FUJ02B1 and FUJ02E3, should we consider rolling both ACPI drivers into
> one?  Aside from conceptual neatness and perhaps a small runtime memory
> footprint saving in the event that no backlight control functionality need
> be provided by fujitsu-laptop, is there a whole lot to be gained through the
> use of two separate drivers?

This on the other hand is something we need to consider carefully. I'd like to
hear from Michal on this before I comment further.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1629581 — [PATCH 06/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_bl in acpi_fujitsu_bl_add()

Only allocate memory for struct fujitsu_bl when the FUJ02B1 ACPI device
is present.  Use devm_kzalloc() for allocating memory to simplify
cleanup.

Until all backlight-related code is modified to only use device-specific
data, the pointer to the allocated memory still has to be stored in a
module-wide variable.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 536b601c7067..780e11b43d27 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -417,6 +417,7 @@ static int fujitsu_backlight_register(struct acpi_device *device)
 
 static int acpi_fujitsu_bl_add(struct acpi_device *device)
 {
+	struct fujitsu_bl *priv;
 	int state = 0;
 	int error;
 
@@ -426,10 +427,15 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
+	priv = devm_kzalloc(&device->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	fujitsu_bl = priv;
 	fujitsu_bl->handle = device->handle;
 	sprintf(acpi_device_name(device), "%s", ACPI_FUJITSU_BL_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
-	device->driver_data = fujitsu_bl;
+	device->driver_data = priv;
 
 	error = acpi_fujitsu_bl_input_setup(device);
 	if (error)
@@ -1018,13 +1024,9 @@ static int __init fujitsu_init(void)
 	if (acpi_disabled)
 		return -ENODEV;
 
-	fujitsu_bl = kzalloc(sizeof(struct fujitsu_bl), GFP_KERNEL);
-	if (!fujitsu_bl)
-		return -ENOMEM;
-
 	ret = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
 	if (ret)
-		goto err_free_fujitsu_bl;
+		return ret;
 
 	/* Register platform stuff */
 
@@ -1054,8 +1056,6 @@ static int __init fujitsu_init(void)
 	platform_driver_unregister(&fujitsu_pf_driver);
 err_unregister_acpi:
 	acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
-err_free_fujitsu_bl:
-	kfree(fujitsu_bl);
 
 	return ret;
 }
@@ -1070,8 +1070,6 @@ static void __exit fujitsu_cleanup(void)
 
 	acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
 
-	kfree(fujitsu_bl);
-
 	pr_info("driver unloaded\n");
 }
 
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1629586 — [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data

In portions of the driver which use device-specific data, rename local
variables from fujitsu_bl and fujitsu_laptop to priv in order to clearly
distinguish these parts from code that uses module-wide data.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 5f6b34a97348..536b601c7067 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -369,26 +369,26 @@ static const struct key_entry keymap_backlight[] = {
 
 static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
 {
-	struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
+	struct fujitsu_bl *priv = acpi_driver_data(device);
 	int ret;
 
-	fujitsu_bl->input = devm_input_allocate_device(&device->dev);
-	if (!fujitsu_bl->input)
+	priv->input = devm_input_allocate_device(&device->dev);
+	if (!priv->input)
 		return -ENOMEM;
 
-	snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys),
-		 "%s/video/input0", acpi_device_hid(device));
+	snprintf(priv->phys, sizeof(priv->phys), "%s/video/input0",
+		 acpi_device_hid(device));
 
-	fujitsu_bl->input->name = acpi_device_name(device);
-	fujitsu_bl->input->phys = fujitsu_bl->phys;
-	fujitsu_bl->input->id.bustype = BUS_HOST;
-	fujitsu_bl->input->id.product = 0x06;
+	priv->input->name = acpi_device_name(device);
+	priv->input->phys = priv->phys;
+	priv->input->id.bustype = BUS_HOST;
+	priv->input->id.product = 0x06;
 
-	ret = sparse_keymap_setup(fujitsu_bl->input, keymap_backlight, NULL);
+	ret = sparse_keymap_setup(priv->input, keymap_backlight, NULL);
 	if (ret)
 		return ret;
 
-	return input_register_device(fujitsu_bl->input);
+	return input_register_device(priv->input);
 }
 
 static int fujitsu_backlight_register(struct acpi_device *device)
@@ -566,27 +566,27 @@ static const struct dmi_system_id fujitsu_laptop_dmi_table[] = {
 
 static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
 {
-	struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
 	int ret;
 
-	fujitsu_laptop->input = devm_input_allocate_device(&device->dev);
-	if (!fujitsu_laptop->input)
+	priv->input = devm_input_allocate_device(&device->dev);
+	if (!priv->input)
 		return -ENOMEM;
 
-	snprintf(fujitsu_laptop->phys, sizeof(fujitsu_laptop->phys),
-		 "%s/video/input0", acpi_device_hid(device));
+	snprintf(priv->phys, sizeof(priv->phys), "%s/video/input0",
+		 acpi_device_hid(device));
 
-	fujitsu_laptop->input->name = acpi_device_name(device);
-	fujitsu_laptop->input->phys = fujitsu_laptop->phys;
-	fujitsu_laptop->input->id.bustype = BUS_HOST;
-	fujitsu_laptop->input->id.product = 0x06;
+	priv->input->name = acpi_device_name(device);
+	priv->input->phys = priv->phys;
+	priv->input->id.bustype = BUS_HOST;
+	priv->input->id.product = 0x06;
 
 	dmi_check_system(fujitsu_laptop_dmi_table);
-	ret = sparse_keymap_setup(fujitsu_laptop->input, keymap, NULL);
+	ret = sparse_keymap_setup(priv->input, keymap, NULL);
 	if (ret)
 		return ret;
 
-	return input_register_device(fujitsu_laptop->input);
+	return input_register_device(priv->input);
 }
 
 static int fujitsu_laptop_platform_add(void)
@@ -885,11 +885,11 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 
 static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 {
-	struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
 
 	fujitsu_laptop_platform_remove();
 
-	kfifo_free(&fujitsu_laptop->fifo);
+	kfifo_free(&priv->fifo);
 
 	return 0;
 }
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1633673 — Re: [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data

On Mon, Apr 24, 2017 at 03:33:29PM +0200, Micha?? K??pie?? wrote:
> In portions of the driver which use device-specific data, rename local
> variables from fujitsu_bl and fujitsu_laptop to priv in order to clearly
> distinguish these parts from code that uses module-wide data.
> 
> Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/fujitsu-laptop.c | 48 +++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 5f6b34a97348..536b601c7067 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -369,26 +369,26 @@ static const struct key_entry keymap_backlight[] = {
>  
>  static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
>  {
> -	struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
> +	struct fujitsu_bl *priv = acpi_driver_data(device);

[cut]

>  static int fujitsu_backlight_register(struct acpi_device *device)
> @@ -566,27 +566,27 @@ static const struct dmi_system_id fujitsu_laptop_dmi_table[] = {
>  
>  static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
>  {
> -	struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
> +	struct fujitsu_laptop *priv = acpi_driver_data(device);
>  	int ret;

Distinguishing between local and global use like this makes sense, but I
feel we should stick with a slightly more descriptive name than "priv". 
Without any qualification, "priv" could refer to private device-specific
data from either the fujitsu_bl or fujitsu_laptop drivers.  From the source
it is far from obvious which is being accessed in a given function.  If we
implemented only a single ACPI device driver then this would be largely a
moot point, but as there are two within the one module the loss of the
description could make it harder to follow the code later on.

Could we use "bl_priv" and "laptop_priv" for example, so as to provide a
clue within the source code as to what exactly is being referenced? 
Obviously it doesn't provide any compile time type checking, but it's better
than nothing.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1633656 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

Hi Michael

On Mon, Apr 24, 2017 at 03:33:24PM +0200, Micha?? K??pie?? wrote:
> fujitsu-laptop registers two ACPI drivers.  Whenever an ACPI device with
> a matching identifier is found by the ACPI bus, a new instance of the
> relevant driver is bound to that ACPI device.  However, both ACPI
> drivers registered by fujitsu-laptop access module-wide global data
> structures, assuming neither ACPI driver will ever be instantiated more
> than once.  While there are currently no indications of such issues
> happening in the wild, it is theoretically possible for multiple
> FUJ02B1/FUJ02E3 ACPI devices to be present in the firmware, which would
> cause two instances of the relevant driver to simultaneously access
> module-wide globals without any locking in place.  Also, modern Fujitsu
> laptops ship without the FUJ02B1 ACPI device present in firmware,
> causing memory to be needlessly allocated inside fujitsu_init().
> 
> To future-proof the module and lay the groundwork for separating the two
> aforementioned ACPI drivers into separate modules, move away from
> module-wide global data structures by using device-specific data
> instead.

Apologies for the delay in getting this first set of feedback to you.  It's
a combination of the extent of the patch set and a very busy week.

This patch set represents another worthwhile clean up of the fujitsu-laptop
driver.  While I sincerely doubt any laptop vendor will place more than one
FUJ02B1 (or FUJ02E3) in a single machine, removing the dependency on global
variables makes the driver self contained and more consistent.  I have some
points of clarification which I will post as follow ups to the respective
patchs.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1634409 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

> Hi Michael
> 
> On Mon, Apr 24, 2017 at 03:33:24PM +0200, Micha?? K??pie?? wrote:
> > fujitsu-laptop registers two ACPI drivers.  Whenever an ACPI device with
> > a matching identifier is found by the ACPI bus, a new instance of the
> > relevant driver is bound to that ACPI device.  However, both ACPI
> > drivers registered by fujitsu-laptop access module-wide global data
> > structures, assuming neither ACPI driver will ever be instantiated more
> > than once.  While there are currently no indications of such issues
> > happening in the wild, it is theoretically possible for multiple
> > FUJ02B1/FUJ02E3 ACPI devices to be present in the firmware, which would
> > cause two instances of the relevant driver to simultaneously access
> > module-wide globals without any locking in place.  Also, modern Fujitsu
> > laptops ship without the FUJ02B1 ACPI device present in firmware,
> > causing memory to be needlessly allocated inside fujitsu_init().
> > 
> > To future-proof the module and lay the groundwork for separating the two
> > aforementioned ACPI drivers into separate modules, move away from
> > module-wide global data structures by using device-specific data
> > instead.
> 
> Apologies for the delay in getting this first set of feedback to you.  It's
> a combination of the extent of the patch set and a very busy week.
> 
> This patch set represents another worthwhile clean up of the fujitsu-laptop
> driver.  While I sincerely doubt any laptop vendor will place more than one
> FUJ02B1 (or FUJ02E3) in a single machine, removing the dependency on global
> variables makes the driver self contained and more consistent.  I have some
> points of clarification which I will post as follow ups to the respective
> patchs.

Jonathan,

Thanks for the review.  My hidden agenda, which, in retrospect,
I probably should have included in the cover letter, follows.

In order to avoid accessing global structures from call_fext_func(), we
need to pass it an ACPI handle to FUJ02E3.  This decreases code
readability in two ways: by increasing the function's parameter count
from an already challenging four to an even worse five and by causing
line breaks to be inserted (due to the 80-column line rule) in places
they were previously not necessary in.

To counter this growing obfuscation, patches 01/10, 02/10 and 05/10 (all
called out in your review) work in tandem to ensure that all uses of
call_fext_func() remain legible _and_ fit in one line.  All three of
these patches are needed to prevent line breaks from being inserted
(granted, that is an arbitrary objective), because call_fext_func()
needs to get the ACPI handle somehow and the latter is stored in a field
of a device-specific structure.  Thus, for all call sites, these patches
shorten:

  - (01/10) name of the called function,
  - (02/10) name of the field holding the ACPI handle,
  - (05/10) name of the variable denoting device-specific data.

In other words, these patches are the only sane approach I could come up
with to ensure that, in the end, _all_ uses of call_fext_func() neatly
fit into a single line, thus ensuring reasonable readability even when
taking the added parameter (ACPI handle) into consideration.

I have pasted some examples at the end of this message of what a few
call_fext_func() call sites look like after adding the ACPI handle
parameter and fixing the code to make checkpatch happy, both with ("new
style") and without ("old style") the above three patches applied.  As
you can see, compound conditional expressions benefit the most from the
changes I suggested.

Separating fext_backlight() from the other functions of call_fext_func()
also has the added benefit of only exposing that specific function from
fujitsu-laptop to fujitsu-backlight (where fujitsu-backlight is the
backlight part of the current module), shall the module be split into
two.

And thus we come back to the question of "to split or not to split".
The three options we have are:

  - one module, two drivers: current, suboptimal, state of affairs,

  - two modules, one driver in each: the original cleanup approach I
    have been targeting in all of my patch series for fujitsu-laptop,

  - one module, one driver handling both ACPI devices: the new approach
    you suggested in your review.

I have not considered the last option until now as I deemed it
unacceptable in light of the kernel's philosophy in this regard.
However, such an approach might not be bad in and of itself, because:

  - FUJ02B1 is not fully standalone as it needs FUJ02E3 on some models,

  - FUJ02E3 is present in all models we know of, while FUJ02B1 seems to
    be phased out in newer models,

  - userspace is unlikely to care which input device each hotkey event
    comes from,

  - the memory footprint of both drivers is negligible, considering that
    both are only loaded on machines with hundreds of MB of RAM.

So we could perhaps make fujitsu-laptop register _one_ ACPI driver,
which binds to the FUJ02E3 device and only deals with backlight when the
FUJ02B1 device is present and the vendor interface is either
automatically selected by the kernel or explicitly requested by the
user.  We would then have a single device-specific structure ("priv"
would not be ambiguous any more) holding two ACPI handles ("fjex_handle"
and "fext_handle"?) and all the other fields from both struct fujitsu_bl
and struct fujitsu_laptop.  Please note that I have not played with this
idea in code yet and perhaps handling the added complexity will make the
driver more, not less, convoluted.

Darren, does the above sound more like a viable plan or rather a pipe
dream?  Answering Jonathan's question, there is no added benefit from
splitting fujitsu-laptop into two separate modules, it is only about
following the "one module, one driver" philosophy.  Any answer to this
question puts the variable naming discussion on a specific track, so
perhaps this is the first dilemma that we should sort out.

------------------------------------------------------------------------
old style:
	if ((call_fext_func(fujitsu_laptop->acpi_handle, FUNC_LEDS,
			    0x0, 0x0, 0x0) & BIT(14)) &&
	    (call_fext_func(fujitsu_laptop->acpi_handle, FUNC_LEDS,
	    		    0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
new style:
	if ((fext_leds(priv->handle, 0x0, 0x0, 0x0) & BIT(14)) &&
	    (fext_leds(priv->handle, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
------------------------------------------------------------------------
old style:
	if ((fujitsu_laptop->flags_supported & BIT(26)) &&
	    (call_fext_func(fujitsu_laptop->acpi_handle, FUNC_FLAGS,
	    	            0x1, 0x0, 0x0) & BIT(26)))
new style:
	if ((priv->flags_supported & BIT(26)) &&
	    (fext_flags(priv->handle, 0x1, 0x0, 0x0) & BIT(26)))
------------------------------------------------------------------------
old style:
	if (fujitsu_laptop->fext_handle) {
		if (b->props.power == FB_BLANK_POWERDOWN)
			call_fext_func(fujitsu_bl->fext_handle, FUNC_BACKLIGHT,
				       0x1, 0x4, 0x3);
		else
			call_fext_func(fujitsu_bl->fext_handle, FUNC_BACKLIGHT,
				       0x1, 0x4, 0x0);
	}
new style:
	if (priv->fext_handle) {
		if (b->props.power == FB_BLANK_POWERDOWN)
			fext_backlight(priv->fext_handle, 0x1, 0x4, 0x3);
		else
			fext_backlight(priv->fext_handle, 0x1, 0x4, 0x0);
	}
------------------------------------------------------------------------
old style:
	if (brightness >= LED_FULL)
		return call_fext_func(handle, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
				      FUNC_LED_ON);
	else
		return call_fext_func(handle, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
				      FUNC_LED_OFF);
new style:
	if (brightness >= LED_FULL)
		return fext_leds(handle, 0x1, KEYBOARD_LAMPS, FUNC_LED_ON);
	else
		return fext_leds(handle, 0x1, KEYBOARD_LAMPS, FUNC_LED_OFF);
------------------------------------------------------------------------
old style:
	if (call_fext_func(handle, FUNC_LEDS, 0x2, KEYBOARD_LAMPS,
			   0x0) == FUNC_LED_ON)
		brightness = LED_FULL;
new style:
	if (fext_leds(handle, 0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
		brightness = LED_FULL;
------------------------------------------------------------------------
old style:
	if ((call_fext_func(fujitsu_laptop->acpi_handle, FUNC_LEDS,
			    0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
	    (call_fext_func(fujitsu_laptop->acpi_handle, FUNC_LEDS,
	    		    0x0, 0x0, 0x0) == 0x0)) {
new style:
	if ((fext_leds(priv->handle, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
	    (fext_buttons(priv->handle, 0x0, 0x0, 0x0) == 0x0)) {
------------------------------------------------------------------------
old style:
	while (call_fext_func(fujitsu_laptop->acpi_handle, FUNC_BUTTONS,
			      0x1, 0x0, 0x0) != 0 &&
	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
new style:
	while (fext_buttons(priv->handle, 0x1, 0x0, 0x0) != 0 &&
	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
------------------------------------------------------------------------

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1636091 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

Hi Michael

On Tue, May 02, 2017 at 03:21:44PM +0200, Micha?? K??pie?? wrote:
> In order to avoid accessing global structures from call_fext_func(), we
> need to pass it an ACPI handle to FUJ02E3.  This decreases code
> readability in two ways: by increasing the function's parameter count
> from an already challenging four to an even worse five and by causing
> line breaks to be inserted (due to the 80-column line rule) in places
> they were previously not necessary in.
> 
> To counter this growing obfuscation, patches 01/10, 02/10 and 05/10 (all
> called out in your review) work in tandem to ensure that all uses of
> call_fext_func() remain legible _and_ fit in one line.  All three of
> these patches are needed to prevent line breaks from being inserted
> (granted, that is an arbitrary objective), because call_fext_func()
> needs to get the ACPI handle somehow and the latter is stored in a field
> of a device-specific structure. ...

Thanks for the explanation of your rationale behind patchs 1, 2 and 5.  In
short, they are (at the lowest level) cosmetic aimed at the adherence to the
80-column guideline, but for the reasons you outlined this is not
necessarily a bad thing.

> And thus we come back to the question of "to split or not to split".
> The three options we have are:
> 
>   - one module, two drivers: current, suboptimal, state of affairs,
> 
>   - two modules, one driver in each: the original cleanup approach I
>     have been targeting in all of my patch series for fujitsu-laptop,
> 
>   - one module, one driver handling both ACPI devices: the new approach
>     you suggested in your review.
> 
> I have not considered the last option until now as I deemed it
> unacceptable in light of the kernel's philosophy in this regard.
> However, such an approach might not be bad in and of itself, because:
> 
>   - FUJ02B1 is not fully standalone as it needs FUJ02E3 on some models,

This to me is a fairly strong indication that migrating to the "one module
one driver" approach is worthwhile considering.  If we do split we will end
up with two modules interacting with FUJ02E3, at least on some hardware. 
Conceptually it makes more sense to me that all interaction with FUJ02E3 is
instigated from one module/driver as it will make it easier to ensure that
minipulations of FUJ02E3 for one task don't have unintended side effects for
others.

>   - FUJ02E3 is present in all models we know of, while FUJ02B1 seems to
>     be phased out in newer models,

Agreed.  Furthermore, if FUJ02E3 is phased out it is reasonable to expect
that any platform driver required by the resulting hardware would be so
different to fujitsu-laptop that a new driver would be needed anyway.

>   - userspace is unlikely to care which input device each hotkey event
>     comes from,

Agreed.

>   - the memory footprint of both drivers is negligible, considering that
>     both are only loaded on machines with hundreds of MB of RAM.

Agreed.

> So we could perhaps make fujitsu-laptop register _one_ ACPI driver,
> which binds to the FUJ02E3 device and only deals with backlight when the
> FUJ02B1 device is present and the vendor interface is either
> automatically selected by the kernel or explicitly requested by the
> user.  We would then have a single device-specific structure ("priv"
> would not be ambiguous any more) holding two ACPI handles ("fjex_handle"
> and "fext_handle"?) and all the other fields from both struct fujitsu_bl
> and struct fujitsu_laptop.

Yes, these are the kinds of benefits I was thinking about.

> Please note that I have not played with this idea in code yet and perhaps
> handling the added complexity will make the driver more, not less,
> convoluted.

I understand.  Since FUJ02B1 is only relevant to the backlight I can't see
how the above approach would result in a signficant increase in complexity,
but like you I haven't had a close look at the implications.

> Darren, does the above sound more like a viable plan or rather a pipe
> dream?  Answering Jonathan's question, there is no added benefit from
> splitting fujitsu-laptop into two separate modules, it is only about
> following the "one module, one driver" philosophy.  Any answer to this
> question puts the variable naming discussion on a specific track, so
> perhaps this is the first dilemma that we should sort out.

I agree.  We should resolve the question of the split/no-split option first
since the answer does influence many of the other pending questions. 

Darren: I would therefore be interested in your take on the three options
(as summarised by Michael) so we can determine a way forward.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1636473 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

On Fri, May 05, 2017 at 09:10:58AM +0930, Jonathan Woithe wrote:
> Hi Michael
> 
> On Tue, May 02, 2017 at 03:21:44PM +0200, Micha?? K??pie?? wrote:
> > In order to avoid accessing global structures from call_fext_func(), we
> > need to pass it an ACPI handle to FUJ02E3.  This decreases code
> > readability in two ways: by increasing the function's parameter count
> > from an already challenging four to an even worse five and by causing
> > line breaks to be inserted (due to the 80-column line rule) in places
> > they were previously not necessary in.
> > 
> > To counter this growing obfuscation, patches 01/10, 02/10 and 05/10 (all
> > called out in your review) work in tandem to ensure that all uses of
> > call_fext_func() remain legible _and_ fit in one line.  All three of
> > these patches are needed to prevent line breaks from being inserted
> > (granted, that is an arbitrary objective), because call_fext_func()
> > needs to get the ACPI handle somehow and the latter is stored in a field
> > of a device-specific structure. ...
> 
> Thanks for the explanation of your rationale behind patchs 1, 2 and 5.  In
> short, they are (at the lowest level) cosmetic aimed at the adherence to the
> 80-column guideline, but for the reasons you outlined this is not
> necessarily a bad thing.
> 
> > And thus we come back to the question of "to split or not to split".
> > The three options we have are:
> > 
> >   - one module, two drivers: current, suboptimal, state of affairs,
> > 
> >   - two modules, one driver in each: the original cleanup approach I
> >     have been targeting in all of my patch series for fujitsu-laptop,
> > 
> >   - one module, one driver handling both ACPI devices: the new approach
> >     you suggested in your review.
> > 
> > I have not considered the last option until now as I deemed it
> > unacceptable in light of the kernel's philosophy in this regard.
> > However, such an approach might not be bad in and of itself, because:
> > 
> >   - FUJ02B1 is not fully standalone as it needs FUJ02E3 on some models,
> 
> This to me is a fairly strong indication that migrating to the "one module
> one driver" approach is worthwhile considering.  If we do split we will end
> up with two modules interacting with FUJ02E3, at least on some hardware. 
> Conceptually it makes more sense to me that all interaction with FUJ02E3 is
> instigated from one module/driver as it will make it easier to ensure that
> minipulations of FUJ02E3 for one task don't have unintended side effects for
> others.
> 
> >   - FUJ02E3 is present in all models we know of, while FUJ02B1 seems to
> >     be phased out in newer models,
> 
> Agreed.  Furthermore, if FUJ02E3 is phased out it is reasonable to expect
> that any platform driver required by the resulting hardware would be so
> different to fujitsu-laptop that a new driver would be needed anyway.

*cough* thinkpad_acpi *cough*

> 
> >   - userspace is unlikely to care which input device each hotkey event
> >     comes from,
> 
> Agreed.
> 
> >   - the memory footprint of both drivers is negligible, considering that
> >     both are only loaded on machines with hundreds of MB of RAM.
> 
> Agreed.
> 
> > So we could perhaps make fujitsu-laptop register _one_ ACPI driver,
> > which binds to the FUJ02E3 device and only deals with backlight when the
> > FUJ02B1 device is present and the vendor interface is either
> > automatically selected by the kernel or explicitly requested by the
> > user.  We would then have a single device-specific structure ("priv"
> > would not be ambiguous any more) holding two ACPI handles ("fjex_handle"
> > and "fext_handle"?) and all the other fields from both struct fujitsu_bl
> > and struct fujitsu_laptop.
> 
> Yes, these are the kinds of benefits I was thinking about.
> 
> > Please note that I have not played with this idea in code yet and perhaps
> > handling the added complexity will make the driver more, not less,
> > convoluted.
> 
> I understand.  Since FUJ02B1 is only relevant to the backlight I can't see
> how the above approach would result in a signficant increase in complexity,
> but like you I haven't had a close look at the implications.
> 
> > Darren, does the above sound more like a viable plan or rather a pipe
> > dream?  Answering Jonathan's question, there is no added benefit from
> > splitting fujitsu-laptop into two separate modules, it is only about
> > following the "one module, one driver" philosophy.  Any answer to this
> > question puts the variable naming discussion on a specific track, so
> > perhaps this is the first dilemma that we should sort out.
> 
> I agree.  We should resolve the question of the split/no-split option first
> since the answer does influence many of the other pending questions. 
> 
> Darren: I would therefore be interested in your take on the three options
> (as summarised by Michael) so we can determine a way forward.

+Rafael for his insight from an ACPI driver model perspective.

Unfortunately, this is a fairly subjective area of driver design. We have
competing goals:

a) Driver coupling
   Module load order dependencies and such is to be avoided whenever possible.
   Drivers should be as independent as possible from one another.

b) Single function
   Can't think of a better name for this right now. But this is Michal's point
   about one driver per device. As we add more devices, we risk growing the
   driver until it carries a lot of legacy baggage and is more and more
   difficult to maintain. thinkpad_acpi is the prime example of this.

In an ideal world, Single Function drivers are preferred, but if we end up have
to perform unnatural acts to keep the drivers separated, the advantages can be
lost. So it all hinges on how much Driver Coupling would exist in the separate
driver approach.

We'll accept either with supporting evidence for why it's the better choice. My
preference, under ideal conditions, would be for separate drivers, separate
modules, one per device.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1636893 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

> On Fri, May 05, 2017 at 09:10:58AM +0930, Jonathan Woithe wrote:
> > Hi Michael
> > 
> > On Tue, May 02, 2017 at 03:21:44PM +0200, Micha?? K??pie?? wrote:
> > > In order to avoid accessing global structures from call_fext_func(), we
> > > need to pass it an ACPI handle to FUJ02E3.  This decreases code
> > > readability in two ways: by increasing the function's parameter count
> > > from an already challenging four to an even worse five and by causing
> > > line breaks to be inserted (due to the 80-column line rule) in places
> > > they were previously not necessary in.
> > > 
> > > To counter this growing obfuscation, patches 01/10, 02/10 and 05/10 (all
> > > called out in your review) work in tandem to ensure that all uses of
> > > call_fext_func() remain legible _and_ fit in one line.  All three of
> > > these patches are needed to prevent line breaks from being inserted
> > > (granted, that is an arbitrary objective), because call_fext_func()
> > > needs to get the ACPI handle somehow and the latter is stored in a field
> > > of a device-specific structure. ...
> > 
> > Thanks for the explanation of your rationale behind patchs 1, 2 and 5.  In
> > short, they are (at the lowest level) cosmetic aimed at the adherence to the
> > 80-column guideline, but for the reasons you outlined this is not
> > necessarily a bad thing.
> > 
> > > And thus we come back to the question of "to split or not to split".
> > > The three options we have are:
> > > 
> > >   - one module, two drivers: current, suboptimal, state of affairs,
> > > 
> > >   - two modules, one driver in each: the original cleanup approach I
> > >     have been targeting in all of my patch series for fujitsu-laptop,
> > > 
> > >   - one module, one driver handling both ACPI devices: the new approach
> > >     you suggested in your review.
> > > 
> > > I have not considered the last option until now as I deemed it
> > > unacceptable in light of the kernel's philosophy in this regard.
> > > However, such an approach might not be bad in and of itself, because:
> > > 
> > >   - FUJ02B1 is not fully standalone as it needs FUJ02E3 on some models,
> > 
> > This to me is a fairly strong indication that migrating to the "one module
> > one driver" approach is worthwhile considering.  If we do split we will end
> > up with two modules interacting with FUJ02E3, at least on some hardware. 
> > Conceptually it makes more sense to me that all interaction with FUJ02E3 is
> > instigated from one module/driver as it will make it easier to ensure that
> > minipulations of FUJ02E3 for one task don't have unintended side effects for
> > others.
> > 
> > >   - FUJ02E3 is present in all models we know of, while FUJ02B1 seems to
> > >     be phased out in newer models,
> > 
> > Agreed.  Furthermore, if FUJ02E3 is phased out it is reasonable to expect
> > that any platform driver required by the resulting hardware would be so
> > different to fujitsu-laptop that a new driver would be needed anyway.
> 
> *cough* thinkpad_acpi *cough*
> 
> > 
> > >   - userspace is unlikely to care which input device each hotkey event
> > >     comes from,
> > 
> > Agreed.
> > 
> > >   - the memory footprint of both drivers is negligible, considering that
> > >     both are only loaded on machines with hundreds of MB of RAM.
> > 
> > Agreed.
> > 
> > > So we could perhaps make fujitsu-laptop register _one_ ACPI driver,
> > > which binds to the FUJ02E3 device and only deals with backlight when the
> > > FUJ02B1 device is present and the vendor interface is either
> > > automatically selected by the kernel or explicitly requested by the
> > > user.  We would then have a single device-specific structure ("priv"
> > > would not be ambiguous any more) holding two ACPI handles ("fjex_handle"
> > > and "fext_handle"?) and all the other fields from both struct fujitsu_bl
> > > and struct fujitsu_laptop.
> > 
> > Yes, these are the kinds of benefits I was thinking about.
> > 
> > > Please note that I have not played with this idea in code yet and perhaps
> > > handling the added complexity will make the driver more, not less,
> > > convoluted.
> > 
> > I understand.  Since FUJ02B1 is only relevant to the backlight I can't see
> > how the above approach would result in a signficant increase in complexity,
> > but like you I haven't had a close look at the implications.
> > 
> > > Darren, does the above sound more like a viable plan or rather a pipe
> > > dream?  Answering Jonathan's question, there is no added benefit from
> > > splitting fujitsu-laptop into two separate modules, it is only about
> > > following the "one module, one driver" philosophy.  Any answer to this
> > > question puts the variable naming discussion on a specific track, so
> > > perhaps this is the first dilemma that we should sort out.
> > 
> > I agree.  We should resolve the question of the split/no-split option first
> > since the answer does influence many of the other pending questions. 
> > 
> > Darren: I would therefore be interested in your take on the three options
> > (as summarised by Michael) so we can determine a way forward.
> 
> +Rafael for his insight from an ACPI driver model perspective.
> 
> Unfortunately, this is a fairly subjective area of driver design. We have
> competing goals:
> 
> a) Driver coupling
>    Module load order dependencies and such is to be avoided whenever possible.
>    Drivers should be as independent as possible from one another.
> 
> b) Single function
>    Can't think of a better name for this right now. But this is Michal's point
>    about one driver per device. As we add more devices, we risk growing the
>    driver until it carries a lot of legacy baggage and is more and more
>    difficult to maintain. thinkpad_acpi is the prime example of this.
> 
> In an ideal world, Single Function drivers are preferred, but if we end up have
> to perform unnatural acts to keep the drivers separated, the advantages can be
> lost. So it all hinges on how much Driver Coupling would exist in the separate
> driver approach.

Just to make sure we are all on the same page here, choosing the "two
separate modules, each with one driver for one ACPI device" approach
would mean ending up with two modules:

  - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
    everything _except_ backlight,

  - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
    backlight and depending on fujitsu-laptop.

We would need to export one function from fujitsu-laptop, namely
fext_backlight().  I understand this would require creating a separate
header file which would then be included in fujitsu-backlight.

fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
called.  This method is marked as Serialized, which AFAIU means we do
not need a separate lock in kernel code because all calls to this method
are implicitly serialized by firmware itself.

I do not see anything "unnatural" in this approach, but I would love to
be corrected if I am wrong.

> We'll accept either with supporting evidence for why it's the better choice. My
> preference, under ideal conditions, would be for separate drivers, separate
> modules, one per device.

Putting my two cents in, that would be my choice, too.  Among other
issues, if we choose the "one module, one driver handling two ACPI
devices" approach, only FUJ02E3 will be bound to a driver from the
kernel's perspective, while FUJ02B1 will not, even though it actually
_will_ be handled by the same driver as FUJ02E3.  Sounds ugly to me, but
I would also really like to hear Rafael's opinion.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1636895 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

> Just to make sure we are all on the same page here, choosing the "two
> separate modules, each with one driver for one ACPI device" approach
> would mean ending up with two modules:
> 
>   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
>     everything _except_ backlight,
> 
>   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
>     backlight and depending on fujitsu-laptop.
> 
> We would need to export one function from fujitsu-laptop, namely
> fext_backlight().  I understand this would require creating a separate
> header file which would then be included in fujitsu-backlight.
> 
> fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> called.  This method is marked as Serialized, which AFAIU means we do
> not need a separate lock in kernel code because all calls to this method
> are implicitly serialized by firmware itself.
> 
> I do not see anything "unnatural" in this approach, but I would love to
> be corrected if I am wrong.

To be fair, one thing that may be "unnatural" with this approach is that
even though fujitsu-backlight would depend on fujitsu-laptop, it would
still have to get a handle to FUJ02E3 using:

    acpi_get_handle(NULL, "\\_SB.FEXT", ...)
    
because call_fext_func() - and thus fext_backlight() - needs to be
passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
from the perspective of the ACPI device hierarchy.  Unless there is a
better way of implementing this, in which case I am open to suggestions.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1636914 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

On Sat, May 6, 2017 at 5:21 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, May 6, 2017 at 3:45 PM, Michał Kępień <kernel@kempniu.pl> wrote:
>
>> To be fair, one thing that may be "unnatural" with this approach is that
>> even though fujitsu-backlight would depend on fujitsu-laptop, it would
>> still have to get a handle to FUJ02E3 using:
>>
>>     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
>>
>> because call_fext_func() - and thus fext_backlight() - needs to be
>> passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
>> fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
>> from the perspective of the ACPI device hierarchy.  Unless there is a
>> better way of implementing this, in which case I am open to suggestions.
>
> There are two areas to check with:
> 1. Remote graph node
> https://lwn.net/Articles/718184/
> 2. Component framework (only works in case when all devices are
> mandatory to have).

Ah, and third one is MFD framework.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1636915 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

On Sat, May 6, 2017 at 3:45 PM, Michał Kępień <kernel@kempniu.pl> wrote:

> To be fair, one thing that may be "unnatural" with this approach is that
> even though fujitsu-backlight would depend on fujitsu-laptop, it would
> still have to get a handle to FUJ02E3 using:
>
>     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
>
> because call_fext_func() - and thus fext_backlight() - needs to be
> passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> from the perspective of the ACPI device hierarchy.  Unless there is a
> better way of implementing this, in which case I am open to suggestions.

There are two areas to check with:
1. Remote graph node
https://lwn.net/Articles/718184/
2. Component framework (only works in case when all devices are
mandatory to have).

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1637545 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
> > Just to make sure we are all on the same page here, choosing the "two
> > separate modules, each with one driver for one ACPI device" approach
> > would mean ending up with two modules:
> > 
> >   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
> >     everything _except_ backlight,
> > 
> >   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
> >     backlight and depending on fujitsu-laptop.
> > 
> > We would need to export one function from fujitsu-laptop, namely
> > fext_backlight().  I understand this would require creating a separate
> > header file which would then be included in fujitsu-backlight.
> > 
> > fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> > called.  This method is marked as Serialized, which AFAIU means we do
> > not need a separate lock in kernel code because all calls to this method
> > are implicitly serialized by firmware itself.
> > 
> > I do not see anything "unnatural" in this approach, but I would love to
> > be corrected if I am wrong.
> 
> To be fair, one thing that may be "unnatural" with this approach is that
> even though fujitsu-backlight would depend on fujitsu-laptop, it would
> still have to get a handle to FUJ02E3 using:
> 
>     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
>     
> because call_fext_func() - and thus fext_backlight() - needs to be
> passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> from the perspective of the ACPI device hierarchy.  Unless there is a
> better way of implementing this, in which case I am open to suggestions.

At a high level, I would consider the handle to be private data which should be
encapsulated in fujitsu_laptop. Or... where is FEXT in the ACPI hierarchy
relative to FUJ02E3?

Assuming FEXT is below FUJ02E3, the we appear to be making an assumption that
there is only one FUJ02E3 on the system. While I think this is perfectly
reasonable, it does contradict the argumentation from some of the other patches
in this series.

If FEXT is not below fujitsu laptop... then it is a shared function which either
one of them can own and serialize (or not if fw indeed handles that).

Either way, the owning driver should abstract away the private data and present
an interface the other can use with only the "public" information.

I suggest investigating the various mechanisms Andy pointed at and revisiting
this after that.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1638012 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

> On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
> > > Just to make sure we are all on the same page here, choosing the "two
> > > separate modules, each with one driver for one ACPI device" approach
> > > would mean ending up with two modules:
> > > 
> > >   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
> > >     everything _except_ backlight,
> > > 
> > >   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
> > >     backlight and depending on fujitsu-laptop.
> > > 
> > > We would need to export one function from fujitsu-laptop, namely
> > > fext_backlight().  I understand this would require creating a separate
> > > header file which would then be included in fujitsu-backlight.
> > > 
> > > fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> > > called.  This method is marked as Serialized, which AFAIU means we do
> > > not need a separate lock in kernel code because all calls to this method
> > > are implicitly serialized by firmware itself.
> > > 
> > > I do not see anything "unnatural" in this approach, but I would love to
> > > be corrected if I am wrong.
> > 
> > To be fair, one thing that may be "unnatural" with this approach is that
> > even though fujitsu-backlight would depend on fujitsu-laptop, it would
> > still have to get a handle to FUJ02E3 using:
> > 
> >     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
> >     
> > because call_fext_func() - and thus fext_backlight() - needs to be
> > passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> > fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> > from the perspective of the ACPI device hierarchy.  Unless there is a
> > better way of implementing this, in which case I am open to suggestions.
> 
> At a high level, I would consider the handle to be private data which should be
> encapsulated in fujitsu_laptop. Or... where is FEXT in the ACPI hierarchy
> relative to FUJ02E3?

FEXT *is* FUJ02E3:

Device (FEXT)
{
    Name (_HID, "FUJ02E3")  // _HID: Hardware ID
    ...
    Method (FUNC, 4, Serialized)
    {
        ...
    }
    ...
}

See also below.

> Assuming FEXT is below FUJ02E3, the we appear to be making an assumption that
> there is only one FUJ02E3 on the system. While I think this is perfectly
> reasonable, it does contradict the argumentation from some of the other patches
> in this series.

Exactly.  The whole purpose of this patch series is to stop using
module-wide data.  We have a different situation here than in the case
of e.g. dell-smbios, which coordinates access to a module-wide buffer it
allocates.  

> If FEXT is not below fujitsu laptop... then it is a shared function which either
> one of them can own and serialize (or not if fw indeed handles that).
> 
> Either way, the owning driver should abstract away the private data and present
> an interface the other can use with only the "public" information.

I feel the problem at hand needs a fresh explanation.  I will be as
concise as possible.

We are considering two ACPI devices present on Fujitsu laptops:

  - FJEX:
      * path: \_SB_.PCI0.LPCB.FJEX
      * HID: FUJ02B1
      * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
      * handles: backlight level (LCD brightness)

  - FEXT:
      * path: \_SB_.FEXT
      * HID: FUJ02E3
      * methods invoked by kernel: FUNC
      * handles: hotkey, LEDs, platform attributes, backlight power
                                                    ^^^^^^^^^^^^^^^

The problem is that if we split the ACPI drivers for those two devices
into separate modules, the FJEX driver will need to access the FUNC
method of device FEXT, handled by another driver in another module.

One way of solving this cleanly is to store a handle to the most
recently found FEXT instance (there should always be at most one anyway)
in a module-wide variable inside the FEXT driver, but that defeats the
purpose of this series.

Another solution is proposed by patch 04/10 of this series: make the
FJEX driver independently grab a handle to FEXT using the absolute ACPI
path to the latter.  It feels unnatural (AFAICT only one driver outside
drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
us to drop all module-wide data.

Finally, perhaps the approach I took in my patch series is simply too
zealous.  Maybe the simplest solution is to just keep using module-wide
data, but then we are left with a single module with two intertwined
ACPI drivers inside that need to be registered in the correct order.  It
feels a bit brittle.

> I suggest investigating the various mechanisms Andy pointed at and revisiting
> this after that.

For now, these yield no immediate silver bullets for me.  But I will dig
a bit deeper and report back.  Meanwhile, if anyone feels like sharing
their thoughts after reading the summary I wrote above, I am all ears.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1638080 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

On Tue, May 09, 2017 at 11:35:24AM +0200, Micha?? K??pie?? wrote:
> > If FEXT is not below fujitsu laptop... then it is a shared function which either
> > one of them can own and serialize (or not if fw indeed handles that).
> > 
> > Either way, the owning driver should abstract away the private data and present
> > an interface the other can use with only the "public" information.
> 
> I feel the problem at hand needs a fresh explanation.  I will be as
> concise as possible.
> 
> We are considering two ACPI devices present on Fujitsu laptops:
> 
>   - FJEX:
>       * path: \_SB_.PCI0.LPCB.FJEX
>       * HID: FUJ02B1
>       * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
>       * handles: backlight level (LCD brightness)
> 
>   - FEXT:
>       * path: \_SB_.FEXT
>       * HID: FUJ02E3
>       * methods invoked by kernel: FUNC
>       * handles: hotkey, LEDs, platform attributes, backlight power
>                                                     ^^^^^^^^^^^^^^^
> 
> The problem is that if we split the ACPI drivers for those two devices
> into separate modules, the FJEX driver will need to access the FUNC
> method of device FEXT, handled by another driver in another module.
> 
> One way of solving this cleanly is to store a handle to the most
> recently found FEXT instance (there should always be at most one anyway)
> in a module-wide variable inside the FEXT driver, but that defeats the
> purpose of this series.
> 
> Another solution is proposed by patch 04/10 of this series: make the
> FJEX driver independently grab a handle to FEXT using the absolute ACPI
> path to the latter.  It feels unnatural (AFAICT only one driver outside
> drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
> us to drop all module-wide data.
> 
> Finally, perhaps the approach I took in my patch series is simply too
> zealous.  Maybe the simplest solution is to just keep using module-wide
> data, but then we are left with a single module with two intertwined
> ACPI drivers inside that need to be registered in the correct order.  It
> feels a bit brittle.

I think this revised summary is a good description of the situation.

> > I suggest investigating the various mechanisms Andy pointed at and revisiting
> > this after that.
> 
> For now, these yield no immediate silver bullets for me.  But I will dig
> a bit deeper and report back.  Meanwhile, if anyone feels like sharing
> their thoughts after reading the summary I wrote above, I am all ears.

I appreciate you taking the time to continue pondering the situation - the
best approach is certainly not immediately obvious.  From where I sit I
suspect that whichever solution we eventually adopt there are going to be
wrinkles.  The trick is to identify the approach which has the fewest rough
edges and where the resulting code is the easiest to follow in future.  At
this point in time I cannot come up with an argument which definitively
supports one particular option over the others.  It may come down to
personal taste.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1638281 — Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

On Tue, May 09, 2017 at 11:35:24AM +0200, Michał Kępień wrote:
> > On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
> > > > Just to make sure we are all on the same page here, choosing the "two
> > > > separate modules, each with one driver for one ACPI device" approach
> > > > would mean ending up with two modules:
> > > > 
> > > >   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
> > > >     everything _except_ backlight,
> > > > 
> > > >   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
> > > >     backlight and depending on fujitsu-laptop.
> > > > 
> > > > We would need to export one function from fujitsu-laptop, namely
> > > > fext_backlight().  I understand this would require creating a separate
> > > > header file which would then be included in fujitsu-backlight.
> > > > 
> > > > fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> > > > called.  This method is marked as Serialized, which AFAIU means we do
> > > > not need a separate lock in kernel code because all calls to this method
> > > > are implicitly serialized by firmware itself.
> > > > 
> > > > I do not see anything "unnatural" in this approach, but I would love to
> > > > be corrected if I am wrong.
> > > 
> > > To be fair, one thing that may be "unnatural" with this approach is that
> > > even though fujitsu-backlight would depend on fujitsu-laptop, it would
> > > still have to get a handle to FUJ02E3 using:
> > > 
> > >     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
> > >     
> > > because call_fext_func() - and thus fext_backlight() - needs to be
> > > passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> > > fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> > > from the perspective of the ACPI device hierarchy.  Unless there is a
> > > better way of implementing this, in which case I am open to suggestions.
> > 
> > At a high level, I would consider the handle to be private data which should be
> > encapsulated in fujitsu_laptop. Or... where is FEXT in the ACPI hierarchy
> > relative to FUJ02E3?
> 
> FEXT *is* FUJ02E3:
> 
> Device (FEXT)
> {
>     Name (_HID, "FUJ02E3")  // _HID: Hardware ID
>     ...
>     Method (FUNC, 4, Serialized)
>     {
>         ...
>     }
>     ...
> }
> 
> See also below.
> 
> > Assuming FEXT is below FUJ02E3, the we appear to be making an assumption that
> > there is only one FUJ02E3 on the system. While I think this is perfectly
> > reasonable, it does contradict the argumentation from some of the other patches
> > in this series.
> 
> Exactly.  The whole purpose of this patch series is to stop using
> module-wide data.  We have a different situation here than in the case
> of e.g. dell-smbios, which coordinates access to a module-wide buffer it
> allocates.  
> 
> > If FEXT is not below fujitsu laptop... then it is a shared function which either
> > one of them can own and serialize (or not if fw indeed handles that).
> > 
> > Either way, the owning driver should abstract away the private data and present
> > an interface the other can use with only the "public" information.
> 
> I feel the problem at hand needs a fresh explanation.  I will be as
> concise as possible.
> 
> We are considering two ACPI devices present on Fujitsu laptops:
> 
>   - FJEX:
>       * path: \_SB_.PCI0.LPCB.FJEX
>       * HID: FUJ02B1
>       * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
>       * handles: backlight level (LCD brightness)
> 
>   - FEXT:
>       * path: \_SB_.FEXT
>       * HID: FUJ02E3
>       * methods invoked by kernel: FUNC
>       * handles: hotkey, LEDs, platform attributes, backlight power
>                                                     ^^^^^^^^^^^^^^^

This is very concise and describes the problem clearly, thank you!

> 
> The problem is that if we split the ACPI drivers for those two devices
> into separate modules, the FJEX driver will need to access the FUNC
> method of device FEXT, handled by another driver in another module.
> 
> One way of solving this cleanly is to store a handle to the most
> recently found FEXT instance (there should always be at most one anyway)
> in a module-wide variable inside the FEXT driver, but that defeats the
> purpose of this series.
> 
> Another solution is proposed by patch 04/10 of this series: make the
> FJEX driver independently grab a handle to FEXT using the absolute ACPI
> path to the latter.  It feels unnatural (AFAICT only one driver outside
> drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
> us to drop all module-wide data.

Rafael's take on this would be useful.

> 
> Finally, perhaps the approach I took in my patch series is simply too
> zealous.  Maybe the simplest solution is to just keep using module-wide
> data, but then we are left with a single module with two intertwined
> ACPI drivers inside that need to be registered in the correct order.  It
> feels a bit brittle.

Perhaps so (overly zealous). Regarding the globals, let's be clear on the
motivation. We want to follow good sw engineering practice, use data
encapsulation, etc. However, using an explicit path to an ACPI device to avoid
having a static file-level global doesn't really improve encapsulation in any
way - it just shifts the blame :-)

Another reason to eliminate globals is to allow one driver to handle multiple
devices - all device-specific data must be bound to the device, not the driver.
In our case, there literally cannot be more than one _SB.FEXT. While there could
theoretically be more than one FUJ02E3, I think we all agree that is highly
improbable - and if it did happen, the explicit ACPI path approach would also be
broken.

The motivation to divide the drivers was to provide functional encapsulation,
accurately represent the system in the device tree, and to improve readability
and maintainability of the driver code. So long as we can keep coupling to a
minimum, I still think this makes sense.

So - static global variable for a driver with exactly one device that needs
offer services to another driver... not really all that horrible.

You could accomplish this by making call_fext_func() not static and calling it
from fujitsu-backlight. Or, you could further restrict it by exporting a
fujitsu_backlight_power() function which wraps call_fext_func() providing a
specific interface for fujitsu-backlight. This makes the ownership very explicit
and ensures the usage doesn't grow without explicit changes to fujitsu-laptop.

That is probably the most practical solution IFF we still feel it is worth
splitting the driver into two separate modules. We need to develop a more robust
and objective decision making process on module granularity (when to split, when
to keep together). Will continue to give this more thought.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web