| 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: <f8f8d5fb-68c1-4fd1-9e0b-04c661c98f25@amd.com>
Date: Mon, 3 Jun 2024 17:12:31 -0700
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Jacob Keller <jacob.e.keller@...el.com>,
David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>,
Jakub Kicinski <kuba@...nel.org>
Cc: 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/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
>
> err_register:
>
> --
> 2.44.0.53.g0f9d4d28b7e6
>
>
Powered by blists - more mailing lists