On 1/16/25 2:47 PM, Andrew Lunn wrote:
It is not possible to immediately configure the LED mode, because the
interface to which the PHY and the LED is connected to might not be
attached to the PHY yet. The netdev_trig_notify() is called when the
PHY got attached to interface, extend netdev_trig_notify() to detect
the condition where the LED does have netdev trigger configured in DT
but the mode was not yet configured and configure the baseline mode
from the notifier. This can reuse most of set_device_name() except for
the rtnl_lock() which cannot be claimed in the notifier, so split the
relevant core code into set_device_name_locked() and call only the core
code.
Why cannot it be claimed? Because it has already been claimed?
Yes
If so,
please add an ASSERT_RTNL() in the locked function to document
this.
Added
Or is there a lock inversion here?
Not to my knowledge, the rtnl lock should always be locked first and the
trigger_data->lock mutex second.
-static int set_device_name(struct led_netdev_data *trigger_data,
- const char *name, size_t size)
+static void set_device_name_locked(struct led_netdev_data *trigger_data,
+ const char *name, size_t size)
{
- if (size >= IFNAMSIZ)
- return -EINVAL;
-
The code you cannot see in the context does:
memcpy(trigger_data->device_name, name, size);
If we don't have this size check, is it possible to overrun the
buffer?
Yes, good point, added.
It might be better to split this patch into two, one doing the
refactoring of this function, and include an explanation of the
locking and why it is safe not to include this size check.
Does this still apply with the ASSERT_RTNL() in place and the check in
the _locked() function reinstated ?
