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

Groups > linux.kernel > #1633656 > unrolled thread

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

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

  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

#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] | [next] | [standalone]

#1634409

> 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

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

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

> 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

> 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

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

> 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

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

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]

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

On Tuesday, May 09, 2017 09:47:34 AM Darren Hart wrote:
> 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.

Well, can you point me to patch [04/10] then?

Or better resend the whole series with a CC to linux-acpi (which it should go
to to start with IMO).

Thanks,
Rafael

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

#1639478

> On Tuesday, May 09, 2017 09:47:34 AM Darren Hart wrote:
> > 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.
> 
> Well, can you point me to patch [04/10] then?

Here is a link:

    https://www.spinics.net/lists/platform-driver-x86/msg11412.html

However, please note that in light of what Darren wrote, this specific
patch is likely to be dropped from v2.  Thus, there may be no point in
reviewing it after all, though your feedback would certainly be
appreciated for future reference.

> Or better resend the whole series with a CC to linux-acpi (which it should go
> to to start with IMO).

I did not think of that as this ten-patch series mostly revolves around
data encapsulation.  However, I think it might be worthwhile to CC
linux-acpi for the series that will split fujitsu-laptop in two, shall
it ever be posted.

-- 
Best regards,
Michał Kępień

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

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

On Thursday, May 11, 2017 03:52:11 PM Michał Kępień wrote:
> > On Tuesday, May 09, 2017 09:47:34 AM Darren Hart wrote:
> > > 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.
> > 
> > Well, can you point me to patch [04/10] then?
> 
> Here is a link:
> 
>     https://www.spinics.net/lists/platform-driver-x86/msg11412.html

Thanks!

> However, please note that in light of what Darren wrote, this specific
> patch is likely to be dropped from v2.  Thus, there may be no point in
> reviewing it after all, though your feedback would certainly be
> appreciated for future reference.

OK

> > Or better resend the whole series with a CC to linux-acpi (which it should go
> > to to start with IMO).
> 
> I did not think of that as this ten-patch series mostly revolves around
> data encapsulation.  However, I think it might be worthwhile to CC
> linux-acpi for the series that will split fujitsu-laptop in two, shall
> it ever be posted.

OK, but as a rule of thumb, it is better to CC everything touching ACPI to
linux-acpi just to let people know what you're doing if nothing else.

And if there are ACPI-related questions down the road, the context is there
aleady, so it is generally easier to answer them then.

Thanks,
Rafael

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

#1639751

On Thu, May 11, 2017 at 04:37:30PM +0200, Rafael Wysocki wrote:
> On Thursday, May 11, 2017 03:52:11 PM Michał Kępień wrote:
> > > On Tuesday, May 09, 2017 09:47:34 AM Darren Hart wrote:
> > > > 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:
...
> > > > > 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.
> > > 
> > > Well, can you point me to patch [04/10] then?
> > 
> > Here is a link:
> > 
> >     https://www.spinics.net/lists/platform-driver-x86/msg11412.html
> 
> Thanks!
> 
> > However, please note that in light of what Darren wrote, this specific
> > patch is likely to be dropped from v2.  Thus, there may be no point in
> > reviewing it after all, though your feedback would certainly be
> > appreciated for future reference.
> 
> OK

Rafael's take on balancing one driver per device, versus a single driver with
interdependent ACPI devices, even if one of them doesn't show up in the device
hierarchy, as well as one driver calling into another one, all from his
experience with ACPI device drivers would be valuable, and could sway my advice
above.

> 
> > > Or better resend the whole series with a CC to linux-acpi (which it should go
> > > to to start with IMO).
> > 
> > I did not think of that as this ten-patch series mostly revolves around
> > data encapsulation.  However, I think it might be worthwhile to CC
> > linux-acpi for the series that will split fujitsu-laptop in two, shall
> > it ever be posted.
> 
> OK, but as a rule of thumb, it is better to CC everything touching ACPI to
> linux-acpi just to let people know what you're doing if nothing else.
> 
> And if there are ACPI-related questions down the road, the context is there
> aleady, so it is generally easier to answer them then.
> 

Agreed. Unfortunately, we don't have a good way to make this clear in
MAINTAINERS without enumerating every driver. I'll try to make sure the regular
contributors know this, but new folks will continue to miss it unless we can
find a better way to make it obvious.


-- 
Darren Hart
VMware Open Source Technology Center

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

#1639462

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

Indeed, thanks for a clear-headed opinion.  I got a bit carried away :)

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

Good point.

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

I like the latter option more.  Exporting call_fext_func() as it is
would mean enabling other modules to reimplement fujitsu-laptop's
features and we do not want that.

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

In light of the above, I still feel the split is worth going through
with.  The question is whether Jonathan feels the same :)

Jonathan, assuming the objective of splitting the module in two, allow
me to pick your brain a bit:

 1. Would you be okay with leaving "priv" as the variable name for
    device-specific data in both drivers?  If they are to be separated,
    "priv" would soon become unambiguous.  I do not have any strong
    feelings about this, though.

 2. Would you be okay with renaming "acpi_handle" to "handle"?  Darren
    seems to like this idea and in light of the above we would not have
    another ACPI handle inside struct fujitsu_bl any more.

 3. You mentioned earlier that you were not really fond of the fext_*()
    helper functions.  Would you like me to drop them and simply use
    call_fext_func() with five arguments everywhere?  Or should I keep
    the helper functions in v2?

Thanks,

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web