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

Groups > linux.kernel > #1645295 > unrolled thread

[PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-19 09:50 +0200
    [PATCH v2 4/8] platform/x86: fujitsu-laptop: allocate struct fujitsu_laptop in acpi_fujitsu_laptop_add() Michał Kępień <kernel@kempniu.pl> - 2017-05-19 09:50 +0200
    Re: [PATCH v2 0/8] fujitsu-laptop: use device-specific data instead  of module-wide globals Jonathan Woithe <jwoithe@just42.net> - 2017-05-22 01:30 +0200
      Re: [PATCH v2 0/8] fujitsu-laptop: use device-specific data instead  of module-wide globals Jonathan Woithe <jwoithe@just42.net> - 2017-05-24 02:10 +0200

#1645295 — [PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals

fujitsu-laptop registers two ACPI drivers that access each other's
module-wide structures.  To improve data encapsulation 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.

To avoid breaking a working feature (backlight power synchronization
upon module load), this series leaves the module-wide struct fujitsu_bl
in place.  It will be taken care of when the backlight driver is split
into a separate module.

As we agreed that grabbing a handle to an ACPI device using its absolute
path is not a truly elegant solution, this series uses a different
approach to call_fext_func() than v1.  By passing that function a
pointer to a struct acpi_device instead of an acpi_handle, all relevant
static functions of the module will now use the same type for their
first argument and the acpi_handle fields of both module-wide structures
are removed altogether.

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

As with v1, adhering to the "one logical change per patch" rule was
tricky.  If the changes introduced are illegible, I will be happy to
further explain and/or improve the series.  Using --color-words should
make reviewing much more manageable.

Changes from v1:

  - Drop patch 01/10 from v1, i.e. do not introduce fext_*() helper
    functions.

  - Drop patch 02/10 from v1 as the acpi_handle fields of both
    module-wide structures are removed altogether by other patches.

  - Replace patch 03/10 from v1 with patch 6/8, passing call_fext_func()
    a pointer to struct acpi_device instead of an acpi_handle.

  - Drop patch 04/10 from v1, thus deferring driver separation until the
    split into separate modules.  Consider patch 5/8 a partial spiritual
    successor ;)  More information is available in the commit message of
    that patch.

  - Add an additional check to patch 2/8 to avoid a NULL dereference
    which could happen due to patch 04/10 from v1 being dropped.

  - Do not store ACPI handles in private structures.  Instead, extract
    them directly from struct acpi_device pointers passed as function
    arguments.

  - Updated commit messages.

  - As the above might be a bit confusing, here is the patch number
    mapping from v1 to v2:

        v1    | v2
	------+--------
        01/10 | dropped
        02/10 | dropped
        03/10 | 6/8
        04/10 | 5/8
        05/10 | 1/8
        06/10 | 2/8
        07/10 | 3/8
        08/10 | 4/8
        09/10 | 7/8
        10/10 | 8/8

 drivers/platform/x86/fujitsu-laptop.c | 417 +++++++++++++++++-----------------
 1 file changed, 213 insertions(+), 204 deletions(-)

-- 
2.13.0

[toc] | [next] | [standalone]

#1645296 — [PATCH v2 4/8] platform/x86: fujitsu-laptop: allocate struct fujitsu_laptop in acpi_fujitsu_laptop_add()

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

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 1124070aad2d..3916f0ae59f3 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -776,6 +776,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 
 static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 {
+	struct fujitsu_laptop *priv;
 	int state = 0;
 	int error;
 	int i;
@@ -783,11 +784,16 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
+	priv = devm_kzalloc(&device->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	fujitsu_laptop = priv;
 	fujitsu_laptop->acpi_handle = device->handle;
 	sprintf(acpi_device_name(device), "%s",
 		ACPI_FUJITSU_LAPTOP_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
-	device->driver_data = fujitsu_laptop;
+	device->driver_data = priv;
 
 	/* kfifo */
 	spin_lock_init(&fujitsu_laptop->fifo_lock);
@@ -1014,22 +1020,14 @@ static int __init fujitsu_init(void)
 
 	/* Register laptop driver */
 
-	fujitsu_laptop = kzalloc(sizeof(struct fujitsu_laptop), GFP_KERNEL);
-	if (!fujitsu_laptop) {
-		ret = -ENOMEM;
-		goto err_unregister_platform_driver;
-	}
-
 	ret = acpi_bus_register_driver(&acpi_fujitsu_laptop_driver);
 	if (ret)
-		goto err_free_fujitsu_laptop;
+		goto err_unregister_platform_driver;
 
 	pr_info("driver " FUJITSU_DRIVER_VERSION " successfully loaded\n");
 
 	return 0;
 
-err_free_fujitsu_laptop:
-	kfree(fujitsu_laptop);
 err_unregister_platform_driver:
 	platform_driver_unregister(&fujitsu_pf_driver);
 err_unregister_acpi:
@@ -1042,8 +1040,6 @@ static void __exit fujitsu_cleanup(void)
 {
 	acpi_bus_unregister_driver(&acpi_fujitsu_laptop_driver);
 
-	kfree(fujitsu_laptop);
-
 	platform_driver_unregister(&fujitsu_pf_driver);
 
 	acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
-- 
2.13.0

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

#1646473 — Re: [PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals

Hi Michael

On Fri, May 19, 2017 at 09:44:40AM +0200, Micha?? K??pie?? wrote:
> fujitsu-laptop registers two ACPI drivers that access each other's
> module-wide structures.  To improve data encapsulation 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.
> :

I have had a quick look through this series.  To my eye it addresses the
outcome of our discussion over the last couple of weeks, and looks good.  I
had one query regarding patch 5/8, but that might just be a case of me not
knowing about a subtlety of WARN_ONCE().  In any case this isn't a major
issue and would be easily resolved if needed.

Once I get feedback on patch 5/8 (and after seeing any additional comments
from Darren et al) I can send through a reviewed-by.

Regards
  jonathan

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

#1649013 — Re: [PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals

Hi Michael

On Mon, May 22, 2017 at 08:53:23AM +0930, Jonathan Woithe wrote:
> On Fri, May 19, 2017 at 09:44:40AM +0200, Micha?? K??pie?? wrote:
> > fujitsu-laptop registers two ACPI drivers that access each other's
> > module-wide structures.  To improve data encapsulation 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.
> > :
> 
> I have had a quick look through this series.  To my eye it addresses the
> outcome of our discussion over the last couple of weeks, and looks good.  I
> had one query regarding patch 5/8, but that might just be a case of me not
> knowing about a subtlety of WARN_ONCE().  In any case this isn't a major
> issue and would be easily resolved if needed.
> 
> Once I get feedback on patch 5/8 (and after seeing any additional comments
> from Darren et al) I can send through a reviewed-by.

With the minor query in 5.8 sorted, I'm happy to proceed with this (subject
of course to any further comments from Darren or Andy).  While not an
endpoint in and of itslef, it is never-the-less an important step towards
the agreed goal to separate the fujitsu-laptop module into two drivers. 
This is motivated by the current use of two ACPI devices by the single
fujitsu-laptop module, which is inconsistent with the kernel's "one driver
per module" approach.

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

Regards
  jonathan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web