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

Groups > linux.kernel > #1573109 > 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-03 15:10 +0100
    Re: [PATCH 0/4] fujitsu_init() cleanup Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-03 15:20 +0100
      Re: [PATCH 0/4] fujitsu_init() cleanup Darren Hart <dvhart@infradead.org> - 2017-02-04 01:50 +0100
        Re: [PATCH 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-02-04 07:30 +0100
          Re: [PATCH 0/4] fujitsu_init() cleanup Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-04 15:20 +0100

#1573109 — 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.

-- 
Best regards,
Michał Kępień

[toc] | [next] | [standalone]

#1573113

On Fri, Feb 3, 2017 at 4:06 PM, Michał Kępień <kernel@kempniu.pl> 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.

Noted.

Just for you information. Our repo now consists three branches: fixes,
for-next, and testing. testing is kinda for internal use, it might be
broken or be in a wrong state.
Better to use for-next as a base. For this cycle (which is my first as
a co-maintainer) I missed a proper workflow.
That's why I just rebased for-next. It will be not the case in the future.

Darren, consider above as a summary of sync mail for this week.

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

I'm about to leave, I'm going to follow this later.
Thanks!

-- 
With Best Regards,
Andy Shevchenko

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

#1573550

On Fri, Feb 03, 2017 at 04:13:40PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 3, 2017 at 4:06 PM, Michał Kępień <kernel@kempniu.pl> 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.
> 
> Noted.
> 
> Just for you information. Our repo now consists three branches: fixes,
> for-next, and testing. testing is kinda for internal use, it might be
> broken or be in a wrong state.
> Better to use for-next as a base. For this cycle (which is my first as

In general, you should be able to use Linus' master as the base. Only if you
require patches already in for-next, should you use for-next.


-- 
Darren Hart
Intel Open Source Technology Center

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

#1573606

> On Fri, Feb 03, 2017 at 04:13:40PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 3, 2017 at 4:06 PM, Michał Kępień <kernel@kempniu.pl> 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.
> > 
> > Noted.
> > 
> > Just for you information. Our repo now consists three branches: fixes,
> > for-next, and testing. testing is kinda for internal use, it might be
> > broken or be in a wrong state.
> > Better to use for-next as a base. For this cycle (which is my first as
> 
> In general, you should be able to use Linus' master as the base. Only if you
> require patches already in for-next, should you use for-next.

Thanks, guys, this is all helpful information.  I have always assumed
testing was the correct base branch because my patches got pushed there
once they got accepted.

-- 
Best regards,
Michał Kępień

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

#1573673

On Sat, Feb 4, 2017 at 8:21 AM, Michał Kępień <kernel@kempniu.pl> wrote:
>> On Fri, Feb 03, 2017 at 04:13:40PM +0200, Andy Shevchenko wrote:
>> > On Fri, Feb 3, 2017 at 4:06 PM, Michał Kępień <kernel@kempniu.pl> 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.
>> >
>> > Noted.
>> >
>> > Just for you information. Our repo now consists three branches: fixes,
>> > for-next, and testing. testing is kinda for internal use, it might be
>> > broken or be in a wrong state.
>> > Better to use for-next as a base. For this cycle (which is my first as
>>
>> In general, you should be able to use Linus' master as the base. Only if you
>> require patches already in for-next, should you use for-next.
>
> Thanks, guys, this is all helpful information.  I have always assumed
> testing was the correct base branch because my patches got pushed there
> once they got accepted.

Consider testing as a purgatory of the stuff.

P.S. Darren fixed your name in previous patches and thus one more
rebase happened in for-next.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web