Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966875AbcKLVOm (ORCPT ); Sat, 12 Nov 2016 16:14:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57386 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966699AbcKLVOl (ORCPT ); Sat, 12 Nov 2016 16:14:41 -0500 Subject: Re: PM regression with LED changes in next-20161109 To: Jacek Anaszewski , Pavel Machek References: <20161109192301.GS26979@atomide.com> <28234714-3994-6747-9cf8-1ff0b3257f7a@gmail.com>

<5bd5333e-0dbb-6333-0a48-ca4d3a990f9c@samsung.com> <20161110162925.GA28832@amd> <20161110175537.GF27724@atomide.com> <20161110202910.GE28832@amd> <80b645e7-c3fa-8001-d9b1-c3c8c40394fd@gmail.com> <20161111120114.GA1076@amd>

<4c31faef-144d-289c-0e32-83e76aff6178@gmail.com> <3eb60c78-d891-27e5-6b7b-a54a5b547a1c@redhat.com> Cc: Tony Lindgren , Jacek Anaszewski , linux-leds@vger.kernel.org, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Darren Hart From: Hans de Goede Message-ID: Date: Sat, 12 Nov 2016 22:14:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Sat, 12 Nov 2016 21:14:40 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3670 Lines: 94 Hi, On 12-11-16 20:14, Jacek Anaszewski wrote: >>>> Why a dedicated file? Are we going to mirror brightness here >>>> wrt r/w (show/store) behavior ? If not userspace now needs >>>> 2 open fds which is not really nice. If we are and we are >>>> not going to use poll for something else on brightness itself >>>> then why not just poll directly on brightness ? >>> >>> My main concern is that reporting only hw brightness changes >>> wouldn't be consistent with general brightness file purpose. >>> One could expect that brightness changes made by triggers >>> should be also reported. >> >> Ok, I agree that not notifying poll() while an actual >> read() would result in a different value is not really good >> semantics. >> >> I don't like to call it hw_brightness_change though, as >> mentioned before I believe that if we were to start with >> a clean slate we would make the brightness file's read/write >> behavior more a mirror of itself. >> >> So I would like to propose creating a new read-write >> user_brightness file. >> >> The write behavior would be 100% identical to the brightness >> file (in code terms it will call the same store function). >> >> The the read behavior otoh will be different: it will shows >> the last brightness as set by the user, this would show the >> read behavior we really want of brightness: show the real >> brightness when not blinking / triggers are active and show >> the brightness used when on when blinking / triggers are active. >> >> We could then add poll support on this new user_brightness >> file, thus avoiding the problem with the extra cpu-load on >> notifications on blinking / triggers. > > I agree that user_brightness allows to solve the issues you raised > about inconsistent write and read brightness' semantics > (which is not that painful IMHO). > > Reporting non-user brightness changes on user_brightness file > doesn't sound reasonable though. The changes I'm interested in are user brightness changes they are just not done through sysfs, but through a hardwired hotkey, they are however very much done by the user. > Also, how would we read the > brightness set by the firmware? We'd have to read brightness > file, so still two files would have to be opened which is > a second drawback of this approach. No, look carefully at the definition of the read behavior I plan to put in the ABI doc: "Reading this file will return the actual led brightness when not blinking and no triggers are active; reading this file will return the brightness used when the led is on when blinking or triggers are active." So for e.g. the backlit keyboard case reading this single file will return the actual brightness of the backlight, since this does not involve blinking or triggers. Basically the idea is that the user_brightness file will have the semantics which IMHO the brightness file itself should have had from the beginning, but which we can't change now due to ABI reasons. > Having no difference in this area between the two approaches > I'm still in favour of the read-only file for notifying > brightness changes procured by hardware. That brings back the needing 2 fds problem; and does not solve userspace not being able to reliably read the led on brightness when blinking or using triggers. And this also has the issue that one is doing poll() on one fd to detect changes on another fd, which is completely unheard of in any kernel API, so I still vote NACK for the entire idea of having a different file purely for notifying changes. The way unix defines poll to work means that the poll() and read() must be on the same fd, anything else does not make sense. Regards, Hans