Message-ID: <20170228080704.GA32366@kroah.com>
Date:   Tue, 28 Feb 2017 09:07:04 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Darren Hart <dvhart@...radead.org>
Cc:     Michał Kępień <kernel@...pniu.pl>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Jonathan Woithe <jwoithe@...t42.net>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/4] fujitsu_init() cleanup

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

Powered by blists - more mailing lists