netdev - Re: [PATCH 9/9] igc: add support for ethtool.set_phys

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]

Date: Wed, 5 Jun 2024 13:03:57 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "Nelson, Shannon" <shannon.nelson@....com>, David Miller
	<davem@...emloft.net>, netdev <netdev@...r.kernel.org>, Jakub Kicinski
	<kuba@...nel.org>, Vitaly Lifshits <vitaly.lifshits@...el.com>, "Menachem
 Fogel" <menachem.fogel@...el.com>, Naama Meir <naamax.meir@...ux.intel.com>
Subject: Re: [PATCH 9/9] igc: add support for ethtool.set_phys_id



On 6/4/2024 6:14 PM, Andrew Lunn wrote:
> On Tue, Jun 04, 2024 at 02:12:31PM -0700, Jacob Keller wrote:
>>
>>
>> On 6/3/2024 5:12 PM, Nelson, Shannon wrote:
>>> On 6/3/2024 3:38 PM, Jacob Keller wrote:
>>>>
>>>> From: Vitaly Lifshits <vitaly.lifshits@...el.com>
>>>>
>>>> Add support for ethtool.set_phys_id callback to initiate LED blinking
>>>> and stopping them by the ethtool interface.
>>>> This is done by storing the initial LEDCTL register value and restoring
>>>> it when LED blinking is terminated.
>>>>
>>>> In addition, moved IGC_LEDCTL related defines from igc_leds.c to
>>>> igc_defines.h where they can be included by all of the igc module
>>>> files.
> 
> Sorry for the deep nesting. I missed the first post.
> 
> This seems like a very Intel specific solution to a very generic
> problem. The LED code added by Kurt Kanzenbach follows the generic
> netdev way of controlling LEDs. Any MAC or PHY driver with LED support
> should be capable of blinking. Maybe in hardware, maybe it needs
> software support.
> 
> So please write a generic ethtool helper which any MAC driver can use
> to make use of the generic sys class LEDs associated to it, not an
> Intel specific solution.
> 
>     Andrew


... Isn't that what the .set_phys_id ethtool callback is???

>  * @set_phys_id: Identify the physical devices, e.g. by flashing an LED
>  *      attached to it.  The implementation may update the indicator
>  *      asynchronously or synchronously, but in either case it must return
>  *      quickly.  It is initially called with the argument %ETHTOOL_ID_ACTIVE,
>  *      and must either activate asynchronous updates and return zero, return
>  *      a negative error or return a positive frequency for synchronous
>  *      indication (e.g. 1 for one on/off cycle per second).  If it returns
>  *      a frequency then it will be called again at intervals with the
>  *      argument %ETHTOOL_ID_ON or %ETHTOOL_ID_OFF and should set the state of
>  *      the indicator accordingly.  Finally, it is called with the argument
>  *      %ETHTOOL_ID_INACTIVE and must deactivate the indicator.  Returns a
>  *      negative error code or zero.

Maybe I'm misunderstanding here. Are you asking us to expose the LEDs
via some other interface and extend ethtool to use that interface to
blink LEDs?