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

Groups > linux.kernel > #1588528 > unrolled thread

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

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 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-02-27 08:30 +0100
    Re: [PATCH 0/4] fujitsu_init() cleanup Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-02-28 09:10 +0100
      Re: [PATCH 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-02-28 09:40 +0100
        Re: [PATCH 0/4] fujitsu_init() cleanup Jonathan Woithe <jwoithe@just42.net> - 2017-02-28 13:20 +0100
          Re: [PATCH 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-02-28 14:40 +0100
            Re: [PATCH 0/4] fujitsu_init() cleanup Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-02-28 15:10 +0100
              Re: [PATCH 0/4] fujitsu_init() cleanup Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-28 16:00 +0100
              Re: [PATCH 0/4] fujitsu_init() cleanup Darren Hart <dvhart@infradead.org> - 2017-03-01 04:30 +0100
      Re: [PATCH 0/4] fujitsu_init() cleanup Jonathan Woithe <jwoithe@just42.net> - 2017-02-28 12:30 +0100
        Re: [PATCH 0/4] fujitsu_init() cleanup Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-02-28 13:40 +0100
    Re: [PATCH 0/4] fujitsu_init() cleanup Darren Hart <dvhart@infradead.org> - 2017-02-28 09:20 +0100

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

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

Darren, Andy,

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

Thanks,

-- 
Best regards,
Michał Kępień

[toc] | [next] | [standalone]

#1589265

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

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

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

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

thanks,

greg k-h

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

#1589278

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

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

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

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

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

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

-- 
Best regards,
Michał Kępień

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

#1589399

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

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

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

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

Regards
  jonathan

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

#1589465

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

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

Current custom attributes attached to the platform device are:

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

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

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

In other words, my proposal was to change this:

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

into this:

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

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

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

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

-- 
Best regards,
Michał Kępień

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

#1589483

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

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

thanks,

greg k-h

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

#1589530

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

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

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

-- 
With Best Regards,
Andy Shevchenko

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

#1589963

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

Yes, please kill these.

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

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

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

Thank you everyone for the time to clarify.

-- 
Darren Hart
Intel Open Source Technology Center

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

#1589370

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

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

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

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

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

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

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

Regards
  jonathan

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

#1589412

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

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

thanks,

greg k-h

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

#1589273

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web