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

Groups > linux.kernel > #1645285 > unrolled thread

[PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device Michał Kępień <kernel@kempniu.pl> - 2017-05-19 09:50 +0200
    Re: [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last  instantiated FUJ02E3 ACPI device Jonathan Woithe <jwoithe@just42.net> - 2017-05-22 01:30 +0200
      Re: [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last  instantiated FUJ02E3 ACPI device Michał Kępień <kernel@kempniu.pl> - 2017-05-23 23:50 +0200
        Re: [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last  instantiated FUJ02E3 ACPI device Jonathan Woithe <jwoithe@just42.net> - 2017-05-24 02:00 +0200

#1645285 — [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device

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.)  In a perfect world,
private data used by each of these drivers would be neatly encapsulated
in a structure specific to a given driver instance.  Sadly, firmware
present on some Fujitsu laptops makes that impossible by exposing
backlight power control (which is what the FUJ02B1 ACPI device should
take care of) through the FUJ02E3 ACPI device.  This means the backlight
driver needs a way to access an ACPI device it is not bound to.  When
the backlight driver is extracted into a separate module, it will not be
able to rely on a module-wide variable any more and such access will
happen through an API exposed by fujitsu-laptop.

For all known firmwares out in the wild, it seems that whenever the
FUJ02B1 ACPI device is present, it is always accompanied by a single
instance of the FUJ02E3 ACPI device.  We could independently grab an
ACPI handle to the FUJ02E3 ACPI device from the backlight driver, but
that would require using a hardcoded absolute path to that ACPI device,
which is subject to change.  It is easier to simply store a module-wide
pointer to the last (most likely only) FUJ02E3 ACPI device found, make
the aforementioned API use it and cover our bases by warning the user if
firmware exposes multiple FUJ02E3 ACPI devices.

Introducing this pointer in advance allows us to get rid of the
acpi_handle field of struct fujitsu_bl and also enables a bit more
step-by-step migration to a device-specific implementation of
call_fext_func().

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 3916f0ae59f3..21bd60afea75 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -155,6 +155,7 @@ struct fujitsu_laptop {
 };
 
 static struct fujitsu_laptop *fujitsu_laptop;
+static struct acpi_device *fext;
 
 #ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
 static u32 dbg_level = 0x03;
@@ -788,6 +789,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (!priv)
 		return -ENOMEM;
 
+	WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found.  Driver may not work as intended.");
+	fext = device;
+
 	fujitsu_laptop = priv;
 	fujitsu_laptop->acpi_handle = device->handle;
 	sprintf(acpi_device_name(device), "%s",
-- 
2.13.0

[toc] | [next] | [standalone]

#1646472 — Re: [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device

On Fri, May 19, 2017 at 09:44:45AM +0200, Micha?? K??pie?? wrote:
> It is easier to simply store a module-wide
> pointer to the last (most likely only) FUJ02E3 ACPI device found, make
> the aforementioned API use it and cover our bases by warning the user if
> firmware exposes multiple FUJ02E3 ACPI devices.
> :
> @@ -788,6 +789,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found.  Driver may not work as intended.");
> +	fext = device;
> +
>  	fujitsu_laptop = priv;
>  	fujitsu_laptop->acpi_handle = device->handle;
>  	sprintf(acpi_device_name(device), "%s",

I thought WARN_ONCE() printed the warning when it was encountered for the
first time and then suppressed it on all other occasions.  If this is true
then we'll get the warning even when there is only one FUJ02E3 in the
system.  Am I missing something?

Regards
  jonathan

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

#1648836 — Re: [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device

> On Fri, May 19, 2017 at 09:44:45AM +0200, Micha?? K??pie?? wrote:
> > It is easier to simply store a module-wide
> > pointer to the last (most likely only) FUJ02E3 ACPI device found, make
> > the aforementioned API use it and cover our bases by warning the user if
> > firmware exposes multiple FUJ02E3 ACPI devices.
> > :
> > @@ -788,6 +789,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> >  	if (!priv)
> >  		return -ENOMEM;
> >  
> > +	WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found.  Driver may not work as intended.");
> > +	fext = device;
> > +
> >  	fujitsu_laptop = priv;
> >  	fujitsu_laptop->acpi_handle = device->handle;
> >  	sprintf(acpi_device_name(device), "%s",
> 
> I thought WARN_ONCE() printed the warning when it was encountered for the
> first time and then suppressed it on all other occasions.

Correct.

> If this is true
> then we'll get the warning even when there is only one FUJ02E3 in the
> system.  Am I missing something?

Probably the fact that the first argument of the macro is a conditional
expression ("fext" is functionally equivalent to "fext != NULL" in this
case).  In other words, the warning will only ever be issued if
acpi_fujitsu_laptop_add() is called at least twice - note the assignment
on the line following WARN_ONCE().

-- 
Best regards,
Michał Kępień

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

#1649006 — Re: [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device

On Tue, May 23, 2017 at 11:47:06PM +0200, Micha?? K??pie?? wrote:
> > On Fri, May 19, 2017 at 09:44:45AM +0200, Micha?? K??pie?? wrote:
> > > It is easier to simply store a module-wide
> > > pointer to the last (most likely only) FUJ02E3 ACPI device found, make
> > > the aforementioned API use it and cover our bases by warning the user if
> > > firmware exposes multiple FUJ02E3 ACPI devices.
> > > :
> > > @@ -788,6 +789,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> > >  	if (!priv)
> > >  		return -ENOMEM;
> > >  
> > > +	WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found.  Driver may not work as intended.");
> > > +	fext = device;
> > > +
> > >  	fujitsu_laptop = priv;
> > >  	fujitsu_laptop->acpi_handle = device->handle;
> > >  	sprintf(acpi_device_name(device), "%s",
> > 
> > I thought WARN_ONCE() printed the warning when it was encountered for the
> > first time and then suppressed it on all other occasions.
> 
> Correct.
> 
> > If this is true
> > then we'll get the warning even when there is only one FUJ02E3 in the
> > system.  Am I missing something?
> 
> Probably the fact that the first argument of the macro is a conditional
> expression ...

Ah (having now had an opportunity to look at the source of WARN_ONCE()),
it's tested within WARN_ONCE.

> ("fext" is functionally equivalent to "fext != NULL" in this case).

Indeed, by virtue of the test done in WARN_ONCE().  Ok, that makes sense.

Regards
  jonathan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web