Re: [PATCH] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger

From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Akinobu Mita <akinobu.mita@gmail.com>,
	linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
	Pavel Machek <pavel@ucw.cz>, Dan Murphy <dmurphy@ti.com>
Subject: Re: [PATCH] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger
Date: Mon, 2 Sep 2019 20:47:02 +0200	[thread overview]
Message-ID: <1cd468a3-b6e5-a93b-739a-f30288318356@gmail.com> (raw)
In-Reply-To: <20190902181207.GA18577@kroah.com>

On 9/2/19 8:12 PM, Greg KH wrote:
> On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
>> Hi Akinobu,
>>
>> Thank you for the patch.
>>
>> I have one nit below but in general it looks good to me.
>> I've tested it with 2000 mtd triggers (~14kB file size)
>> and it worked flawlessly.
>>
>> Still, I would like to have ack from Greg for it.
>>
>> Adding Greg on Cc.
>>
>> On 8/29/19 4:49 PM, Akinobu Mita wrote:
>>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
>>> However, the size of this file is limited to PAGE_SIZE because of the
>>> limitation for sysfs attribute.
>>>
>>> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
>>> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
>>> and which trigger is currently activated.
>>>
>>> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
>>> the PAGE_SIZE limitation.
> 
> But this is NOT a binary file.  A sysfs binary file is used for when the
> kernel passes data to or from hardware without any parsing of the data
> by the kernel.
> 
> You are not doing that here, you are abusing the "one value per file"
> rule of sysfs so much that you are forced to work around the limitation
> it put in place on purpose to keep you from doing stuff like this.
> 
> Please fix this "correctly" by creating a new api that works properly
> and just live with the fact that this file will never work correctly and
> move everyone to use the new api instead.
> 
> Don't keep on abusing the interface by workarounds like this, it is not
> ok.

In the message [0] you pledged to give us exception for that, provided
it will be properly documented in the code. I suppose you now object
because the patch does not meet that condition.

Provided that will be fixed, can we count on your ack for the
implementation of the solution you proposed? :-)

[0] https://lore.kernel.org/lkml/20190329102606.GB7286@kroah.com/

-- 
Best regards,
Jacek Anaszewski