Re: ledtrig-cpu: Limit to 4 CPUs

From: Jacek Anaszewski
Date: Tue Sep 22 2020 - 16:41:43 EST


Hi Pavel,

On 9/22/20 12:42 AM, Pavel Machek wrote:
Hi!

Can I get details of your setup?

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

Well, if someone exists, we can increase the limit, or convince them
to change their setup.

Linux is used in many commercial projects and each such change generates
a cost, so this is not a sheer matter of convincing someone.

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.

It is relevant -- cpu trigger never worked on x86. I had patch fixing
it, but got pushback.

You mean literally x86 (32-bit)? Because I checked yesterday on my
x86_64 and it worked just fine, i.e. changing cpu online state
generated events on all userspace LEDs I registered for each cpuN
trigger.

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

I checked, and I believe the commit is in:

Indeed, I blindly sought the changeset in git log for ledtrig-cpu.c
file.

#ifdef CONFIG_LEDS_TRIGGERS
static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write,
0);
static struct bin_attribute *led_trigger_bin_attrs[] = {

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.

--
Best regards,
Jacek Anaszewski