netdev - Re: [PATCH net-next] r8169: add LED support for RTL8125/RTL8126

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <042741a7-9fce-404c-ae33-3a5abf22a186@gmail.com>
Date: Tue, 13 Feb 2024 08:19:36 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Realtek linux nic maintainers <nic_swsd@...ltek.com>,
 Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
 David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] r8169: add LED support for RTL8125/RTL8126

On 13.02.2024 04:11, Andrew Lunn wrote:
>> +static int rtl8125_get_led_reg(int index)
>> +{
>> +	static const int led_regs[] = { LEDSEL0, LEDSEL1, LEDSEL2, LEDSEL3 };
>> +
>> +	return led_regs[index];
>> +}
>> +
>> +int rtl8125_set_led_mode(struct rtl8169_private *tp, int index, u16 mode)
>> +{
>> +	int reg = rtl8125_get_led_reg(index);
>> +	struct device *dev = tp_to_dev(tp);
>> +	int ret;
>> +	u16 val;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	mutex_lock(&tp->led_lock);
>> +	val = RTL_R16(tp, reg) & ~LEDSEL_MASK_8125;
>> +	RTL_W16(tp, reg, val | mode);
>> +	mutex_unlock(&tp->led_lock);
> 
> I'm wondering if this mutex is actually needed. Each LED has its own
> register. So you don't need to worry about setting two different LEDs
> in parallel. Its just a question of, can the LED core act on one LED
> in parallel? I don't know the answer to this, but it does use delayed
> work for some things, and that should not run in parallel.
> 
This is applicable for non-atomic set_brightness ops.
For sysfs changes of the hw-controlled mode, the call chain is:
netdev_led_attr_store()
  set_baseline_state()
    rtl8125_led_hw_control_set()
      rtl8125_set_led_mode()
        mutex_lock()

So I think we need an own serialization.

> Maybe you can look into this and see if its really needed. Otherwise,
> lets keep it, it does no real harm.
> 
>      Andrew

Heiner