Re: ledtrig-cpu: Limit to 4 CPUs

From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
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>
Subject: Re: ledtrig-cpu: Limit to 4 CPUs
Date: Tue, 22 Sep 2020 00:14:15 +0200	[thread overview]
Message-ID: <781dcb5e-7bad-f740-5914-778ec8a7306b@gmail.com> (raw)
In-Reply-To: <20200920183401.GA21494@duo.ucw.cz>

On 9/20/20 8:34 PM, Pavel Machek wrote:
> Hi!
> 
>>>     *
>>>     * It can be bound to any LED just like other triggers using either a
>>>     * board file or via sysfs interface.
>>>     *
>>>     * An API named ledtrig_cpu is exported for any user, who want to add CPU
>>> - * activity indication in their code
>>> + * activity indication in their code.
>>>     *
>>>     * Copyright 2011 Linus Walleij <linus.walleij@linaro.org>
>>>     * Copyright 2011 - 2012 Bryan Wu <bryan.wu@canonical.com>
>>> @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
>>>    	for_each_possible_cpu(cpu) {
>>>    		struct led_trigger_cpu *trig = &per_cpu(cpu_trig, cpu);
>>> +		if (cpu > 4)
>>
>> NACK. The workaround for this trigger was implemented for a reason -
>> to make it working on platforms with arbitrary number of logical cpus.
>> I've got 8, so I am discriminated now. Not saying, that it precludes
>> trigger registration with no single line of warning.
> 
> Can I get details of your setup?

I don't use this trigger, but I can imagine that someone does.

> What CPU type that is, and how are you mapping CPU activity to LEDs?

The type of CPU is irrelevant here. What is important is the fact
that with this trigger it is possible to visually monitor CPU core
online state. Probably it would be good to ask the author of that
trigger about his use case.

We've had also another patch in 2017 adding "cpu" trigger to
ledtrig-cpu.c that expressed number of online cores in a function of
brightness.

The commit message said:

"This is particularly useful on tiny linux boards with more CPU cores
than LED pins."

So apparently there are still users thereof.

As I've checked it now it has a bug, as it assumes max brightness to be
always LED_FULL, so that will need a fix.

I have spoken up, because I don't get the reason for your patch.
This driver was reworked year ago to remove PAGE_SIZE limit,
and I even applied it to my for-next tree, but that was at
the time of handling maintainership to yourself, and you
seem to not have picked that commit.

Was that intentional? We had even Greg's ack [0].

[0] https://lkml.org/lkml/2019/9/30/397

-- 
Best regards,
Jacek Anaszewski