Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: ledtrig-cpu: Limit to 4 CPUs
To:     Pavel Machek <pavel@ucw.cz>
Cc:     linux-leds@vger.kernel.org, dmurphy@ti.com,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20200919093833.GA14326@duo.ucw.cz>
 <27e19ac9-4bc0-2945-3985-6cd6bb5407df@gmail.com>
 <20200920183401.GA21494@duo.ucw.cz>
 <781dcb5e-7bad-f740-5914-778ec8a7306b@gmail.com> <20200921224212.GA13299@amd>
 <db0b2dca-b7d3-8d76-cc6c-b399c1fa9921@gmail.com>
 <c60858bd-9a9f-5537-9f96-2e44db0c0d9e@gmail.com> <20200925094038.GC20659@amd>
From:   Jacek Anaszewski <jacek.anaszewski@gmail.com>
Message-ID: <5496ac44-003e-5f2a-7faf-88b4a264dedf@gmail.com>
Date:   Sat, 26 Sep 2020 15:59:05 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20200925094038.GC20659@amd>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/25/20 11:40 AM, Pavel Machek wrote:
> Hi!
> 
>>>> So.. no, it is not causing kernel crashes or something. But it is
>>>> example of bad interface, and _that_ is causing problems. (And yes, if
>>>> I realized it is simply possible to limit it, maybe the BIN_ATTR
>>>> conversion would not be neccessary...)
>>>
>>> The limitation you proposed breaks the trigger on many plafforms.
>>
>> Actually it precludes its use.
>>
>> I still see the patch in your linux-next, so I reserve myself the
>> right to comment on your pull request.
> 
> You are free to comment on anything.
> 
> I believe probability someone uses that with more than 4 CPUs is <
> 5%. 

So you even didn't bother to check:

$ git grep "default-trigger = \"cpu[4-9]"
arch/arm/boot/dts/vexpress-v2m-rs1.dtsi: 
linux,default-trigger = "cpu4";
arch/arm/boot/dts/vexpress-v2m-rs1.dtsi: 
linux,default-trigger = "cpu5";
arch/arm/boot/dts/vexpress-v2m.dtsi: 
linux,default-trigger = "cpu4";
arch/arm/boot/dts/vexpress-v2m.dtsi: 
linux,default-trigger = "cpu5";

cpus are enumerated starting from 0, so there are more reasons for which
your patch is broken:

1. There are mainline users.
2. You claim that you limit trigger use to 4 cpus, while the number is
    actually 5, due to your condition:
	+		if (cpu > 4)
	+			continue;
3. For platforms exceeding the limit the number of triggers registered
    would not match the number all available cpus, for no obvious reason.
    Better solution would be to prevent use of the trigger entirely
    in such cases, which would need only to alter first instruction in
    ledtrig_cpu_init(), which currently is:

	BUILD_BUG_ON(CONFIG_NR_CPUS > 9999);

However I am not in favor of this approach since after removing
PAGE_LIMIT size on triggers file, the trigger doesn't longer cause
problems even with hypothetical 1000 cpus.

The correct approach would be to create new trigger with better
interface and then advise people switching to it.

> Probability that someone uses it with more than 100 CPUs is << 1%
> I'd say. Systems just don't have that many LEDs. I'll take the risk.
> 
> If I broke someone's real, existing setup, I'll raise the limit.

Is this professional approach - throw a potential bug at users and
check if it will hit them? :-) And for no reason - you're not fixing
anything.

> (With exception of uled setups. In such cases, I'll just laugh.)
> 
> If you know or can find out someone using it with more than 4 CPUs, I
> can obviously raise the limit before the merge.

-- 
Best regards,
Jacek Anaszewski