| 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
| ||
|
Message-ID: <dc0cc2ca-d3f4-4387-88bd-b54ea6896e0f@intel.com>
Date: Tue, 4 Jun 2024 14:12:31 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: "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>
CC: 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/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.
>>
>> Co-developed-by: Menachem Fogel <menachem.fogel@...el.com>
>> Signed-off-by: Menachem Fogel <menachem.fogel@...el.com>
>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@...el.com>
>> Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>> ---
>> drivers/net/ethernet/intel/igc/igc_defines.h | 22 +++++++++++++++++++
>> drivers/net/ethernet/intel/igc/igc_ethtool.c | 32 ++++++++++++++++++++++++++++
>> drivers/net/ethernet/intel/igc/igc_hw.h | 2 ++
>> drivers/net/ethernet/intel/igc/igc_leds.c | 21 +-----------------
>> drivers/net/ethernet/intel/igc/igc_main.c | 2 ++
>> 5 files changed, 59 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
>> index 5f92b3c7c3d4..664d49f10427 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
>> @@ -686,4 +686,26 @@
>> #define IGC_LTRMAXV_LSNP_REQ 0x00008000 /* LTR Snoop Requirement */
>> #define IGC_LTRMAXV_SCALE_SHIFT 10
>>
>> +/* LED ctrl defines */
>> +#define IGC_NUM_LEDS 3
>> +
>> +#define IGC_LEDCTL_GLOBAL_BLINK_MODE BIT(5)
>> +#define IGC_LEDCTL_LED0_MODE_SHIFT 0
>> +#define IGC_LEDCTL_LED0_MODE_MASK GENMASK(3, 0)
>> +#define IGC_LEDCTL_LED0_BLINK BIT(7)
>> +#define IGC_LEDCTL_LED1_MODE_SHIFT 8
>> +#define IGC_LEDCTL_LED1_MODE_MASK GENMASK(11, 8)
>> +#define IGC_LEDCTL_LED1_BLINK BIT(15)
>> +#define IGC_LEDCTL_LED2_MODE_SHIFT 16
>> +#define IGC_LEDCTL_LED2_MODE_MASK GENMASK(19, 16)
>> +#define IGC_LEDCTL_LED2_BLINK BIT(23)
>> +
>> +#define IGC_LEDCTL_MODE_ON 0x00
>> +#define IGC_LEDCTL_MODE_OFF 0x01
>> +#define IGC_LEDCTL_MODE_LINK_10 0x05
>> +#define IGC_LEDCTL_MODE_LINK_100 0x06
>> +#define IGC_LEDCTL_MODE_LINK_1000 0x07
>> +#define IGC_LEDCTL_MODE_LINK_2500 0x08
>> +#define IGC_LEDCTL_MODE_ACTIVITY 0x0b
>> +
>> #endif /* _IGC_DEFINES_H_ */
>> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> index f2c4f1966bb0..82ece5f95f1e 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> @@ -1975,6 +1975,37 @@ static void igc_ethtool_diag_test(struct net_device *netdev,
>> msleep_interruptible(4 * 1000);
>> }
>>
>> +static int igc_ethtool_set_phys_id(struct net_device *netdev,
>> + enum ethtool_phys_id_state state)
>> +{
>> + struct igc_adapter *adapter = netdev_priv(netdev);
>> + struct igc_hw *hw = &adapter->hw;
>> + u32 ledctl;
>> +
>> + switch (state) {
>> + case ETHTOOL_ID_ACTIVE:
>> + ledctl = rd32(IGC_LEDCTL);
>> +
>> + /* initiate LED1 blinking */
>> + ledctl &= ~(IGC_LEDCTL_GLOBAL_BLINK_MODE |
>> + IGC_LEDCTL_LED1_MODE_MASK |
>> + IGC_LEDCTL_LED2_MODE_MASK);
>> + ledctl |= IGC_LEDCTL_LED1_BLINK;
>> + wr32(IGC_LEDCTL, ledctl);
>> + break;
>> +
>> + case ETHTOOL_ID_INACTIVE:
>> + /* restore LEDCTL default value */
>> + wr32(IGC_LEDCTL, hw->mac.ledctl_default);
>> + break;
>> +
>> + default:
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static const struct ethtool_ops igc_ethtool_ops = {
>> .supported_coalesce_params = ETHTOOL_COALESCE_USECS,
>> .get_drvinfo = igc_ethtool_get_drvinfo,
>> @@ -2013,6 +2044,7 @@ static const struct ethtool_ops igc_ethtool_ops = {
>> .get_link_ksettings = igc_ethtool_get_link_ksettings,
>> .set_link_ksettings = igc_ethtool_set_link_ksettings,
>> .self_test = igc_ethtool_diag_test,
>> + .set_phys_id = igc_ethtool_set_phys_id,
>> };
>>
>> void igc_ethtool_set_ops(struct net_device *netdev)
>> diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h
>> index e1c572e0d4ef..45b68695bdb7 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_hw.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_hw.h
>> @@ -95,6 +95,8 @@ struct igc_mac_info {
>> bool autoneg;
>> bool autoneg_failed;
>> bool get_link_status;
>> +
>> + u32 ledctl_default;
>> };
>>
>> struct igc_nvm_operations {
>> diff --git a/drivers/net/ethernet/intel/igc/igc_leds.c b/drivers/net/ethernet/intel/igc/igc_leds.c
>> index 3929b25b6ae6..e5eeef240802 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_leds.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_leds.c
>> @@ -8,26 +8,7 @@
>> #include <uapi/linux/uleds.h>
>>
>> #include "igc.h"
>> -
>> -#define IGC_NUM_LEDS 3
>> -
>> -#define IGC_LEDCTL_LED0_MODE_SHIFT 0
>> -#define IGC_LEDCTL_LED0_MODE_MASK GENMASK(3, 0)
>> -#define IGC_LEDCTL_LED0_BLINK BIT(7)
>> -#define IGC_LEDCTL_LED1_MODE_SHIFT 8
>> -#define IGC_LEDCTL_LED1_MODE_MASK GENMASK(11, 8)
>> -#define IGC_LEDCTL_LED1_BLINK BIT(15)
>> -#define IGC_LEDCTL_LED2_MODE_SHIFT 16
>> -#define IGC_LEDCTL_LED2_MODE_MASK GENMASK(19, 16)
>> -#define IGC_LEDCTL_LED2_BLINK BIT(23)
>> -
>> -#define IGC_LEDCTL_MODE_ON 0x00
>> -#define IGC_LEDCTL_MODE_OFF 0x01
>> -#define IGC_LEDCTL_MODE_LINK_10 0x05
>> -#define IGC_LEDCTL_MODE_LINK_100 0x06
>> -#define IGC_LEDCTL_MODE_LINK_1000 0x07
>> -#define IGC_LEDCTL_MODE_LINK_2500 0x08
>> -#define IGC_LEDCTL_MODE_ACTIVITY 0x0b
>> +#include "igc_defines.h"
>>
>> #define IGC_SUPPORTED_MODES \
>> (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK_1000) | \
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 12f004f46082..d0db302aa3eb 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -7070,6 +7070,8 @@ static int igc_probe(struct pci_dev *pdev,
>> goto err_register;
>> }
>>
>> + hw->mac.ledctl_default = rd32(IGC_LEDCTL);
>> +
>> return 0;
>
> Is this the only time the driver should read the register? Are there
> any other reasons/times that the LED register value might change while
> the driver is loaded that shouldn't get lost?
>
> If someone leaves the LED blinking then unloads the driver, is the LED
> left blinking? Should igc_remove() restore the default value?
>
> Thanks,
> sln
>
Good questions. Seems like to me that it would make more sense to
initialize IGC_LEDCTL during probe to a known value, and possibly to
ensure its cleared back to the normal state on driver remove.
@Vitaly, any thoughts?
Powered by blists - more mailing lists