| 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: <20221122164757.428409-1-alexandr.lobakin@intel.com>
Date: Tue, 22 Nov 2022 17:47:57 +0100
From: Alexander Lobakin <alexandr.lobakin@...el.com>
To: Simon Horman <simon.horman@...igine.com>
Cc: Alexander Lobakin <alexandr.lobakin@...el.com>,
David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
oss-drivers@...igine.com, Yu Xiao <yu.xiao@...igine.com>,
Louis Peens <louis.peens@...igine.com>
Subject: Re: [PATCH net-next] nfp: ethtool: support reporting link modes
From: Simon Horman <simon.horman@...igine.com>
Date: Mon, 21 Nov 2022 12:20:45 +0100
> From: Yu Xiao <yu.xiao@...igine.com>
>
> Add support for reporting link modes,
> including `Supported link modes` and `Advertised link modes`,
> via ethtool $DEV.
>
> A new command `SPCODE_READ_MEDIA` is added to read info from
> management firmware. Also, the mapping table `nfp_eth_media_table`
> associates the link modes between NFP and kernel. Both of them
> help to support this ability.
>
> Signed-off-by: Yu Xiao <yu.xiao@...igine.com>
> Reviewed-by: Louis Peens <louis.peens@...igine.com>
> Signed-off-by: Simon Horman <simon.horman@...igine.com>
> ---
> drivers/net/ethernet/netronome/nfp/nfp_main.h | 1 +
> .../ethernet/netronome/nfp/nfp_net_ethtool.c | 56 ++++++++++++++++++
> .../ethernet/netronome/nfp/nfpcore/nfp_nsp.c | 17 ++++++
> .../ethernet/netronome/nfp/nfpcore/nfp_nsp.h | 59 +++++++++++++++++++
> .../netronome/nfp/nfpcore/nfp_nsp_eth.c | 27 +++++++++
> 5 files changed, 160 insertions(+)
[...]
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> index 0058ba2b3505..be434b01b724 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> @@ -293,6 +293,59 @@ nfp_net_set_fec_link_mode(struct nfp_eth_table_port *eth_port,
> }
> }
>
> +static const u8 nfp_eth_media_table[] = {
I wouldn't use u8 for a bit number, 256 bits is not difficult to
overflow. u16 would be safe here.
> + [NFP_MEDIA_1000BASE_CX] = ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
> + [NFP_MEDIA_1000BASE_KX] = ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
> + [NFP_MEDIA_10GBASE_KX4] = ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
> + [NFP_MEDIA_10GBASE_KR] = ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
> + [NFP_MEDIA_10GBASE_CX4] = ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
> + [NFP_MEDIA_10GBASE_CR] = ETHTOOL_LINK_MODE_10000baseCR_Full_BIT,
> + [NFP_MEDIA_10GBASE_SR] = ETHTOOL_LINK_MODE_10000baseSR_Full_BIT,
> + [NFP_MEDIA_10GBASE_ER] = ETHTOOL_LINK_MODE_10000baseER_Full_BIT,
> + [NFP_MEDIA_25GBASE_KR] = ETHTOOL_LINK_MODE_25000baseKR_Full_BIT,
> + [NFP_MEDIA_25GBASE_KR_S] = ETHTOOL_LINK_MODE_25000baseKR_Full_BIT,
> + [NFP_MEDIA_25GBASE_CR] = ETHTOOL_LINK_MODE_25000baseCR_Full_BIT,
> + [NFP_MEDIA_25GBASE_CR_S] = ETHTOOL_LINK_MODE_25000baseCR_Full_BIT,
> + [NFP_MEDIA_25GBASE_SR] = ETHTOOL_LINK_MODE_25000baseSR_Full_BIT,
> + [NFP_MEDIA_40GBASE_CR4] = ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT,
> + [NFP_MEDIA_40GBASE_KR4] = ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT,
> + [NFP_MEDIA_40GBASE_SR4] = ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT,
> + [NFP_MEDIA_40GBASE_LR4] = ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT,
> + [NFP_MEDIA_50GBASE_KR] = ETHTOOL_LINK_MODE_50000baseKR_Full_BIT,
> + [NFP_MEDIA_50GBASE_SR] = ETHTOOL_LINK_MODE_50000baseSR_Full_BIT,
> + [NFP_MEDIA_50GBASE_CR] = ETHTOOL_LINK_MODE_50000baseCR_Full_BIT,
> + [NFP_MEDIA_50GBASE_LR] = ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
> + [NFP_MEDIA_50GBASE_ER] = ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
> + [NFP_MEDIA_50GBASE_FR] = ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
> + [NFP_MEDIA_100GBASE_KR4] = ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT,
> + [NFP_MEDIA_100GBASE_SR4] = ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT,
> + [NFP_MEDIA_100GBASE_CR4] = ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT,
> + [NFP_MEDIA_100GBASE_KP4] = ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT,
> + [NFP_MEDIA_100GBASE_CR10] = ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT,
Wouldn't it look fancier with one tab alignment? :>
> +};
> +
> +static void nfp_add_media_link_mode(struct nfp_port *port,
> + struct nfp_eth_table_port *eth_port,
> + struct ethtool_link_ksettings *cmd)
> +{
> + struct nfp_eth_media_buf ethm = {.eth_index = eth_port->eth_index};
I'm not sure it's okay to do one-line struct initialization. Up to
you I guess, but looks a bit confusing to me, in comparison to the
initialization two functions below.
> + struct nfp_cpp *cpp = port->app->cpp;
> + u8 i;
Same here. And there's no point in using types shorter than u32 on
the stack. Just go with u32. And also,
> +
> + if (nfp_eth_read_media(cpp, ðm))
> + return;
> +
> + for (i = 0; i < NFP_MEDIA_LINK_MODES_NUMBER; ++i) {
Since we're on -std=gnu11 for a bunch of releases already, all new
loops are expected to go with the iterator declarations inside them.
E.g.
for (u32 i = 0; i < ...
> + if (test_bit(i, ethm.supported_modes))
> + __set_bit(nfp_eth_media_table[i],
> + cmd->link_modes.supported);
> +
> + if (test_bit(i, ethm.advertised_modes))
> + __set_bit(nfp_eth_media_table[i],
> + cmd->link_modes.advertising);
> + }
> +}
> +
> /**
> * nfp_net_get_link_ksettings - Get Link Speed settings
> * @netdev: network interface device structure
> @@ -311,6 +364,8 @@ nfp_net_get_link_ksettings(struct net_device *netdev,
> u16 sts;
>
> /* Init to unknowns */
> + ethtool_link_ksettings_zero_link_mode(cmd, supported);
> + ethtool_link_ksettings_zero_link_mode(cmd, advertising);
> ethtool_link_ksettings_add_link_mode(cmd, supported, FIBRE);
> cmd->base.port = PORT_OTHER;
> cmd->base.speed = SPEED_UNKNOWN;
> @@ -321,6 +376,7 @@ nfp_net_get_link_ksettings(struct net_device *netdev,
> if (eth_port) {
> ethtool_link_ksettings_add_link_mode(cmd, supported, Pause);
> ethtool_link_ksettings_add_link_mode(cmd, advertising, Pause);
> + nfp_add_media_link_mode(port, eth_port, cmd);
> if (eth_port->supp_aneg) {
> ethtool_link_ksettings_add_link_mode(cmd, supported, Autoneg);
> if (eth_port->aneg == NFP_ANEG_AUTO) {
> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c
> index 730fea214b8a..7136bc48530b 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c
> @@ -100,6 +100,7 @@ enum nfp_nsp_cmd {
> SPCODE_FW_LOADED = 19, /* Is application firmware loaded */
> SPCODE_VERSIONS = 21, /* Report FW versions */
> SPCODE_READ_SFF_EEPROM = 22, /* Read module EEPROM */
> + SPCODE_READ_MEDIA = 23, /* Get either the supported or advertised media for a port */
> };
>
> struct nfp_nsp_dma_buf {
> @@ -1100,4 +1101,20 @@ int nfp_nsp_read_module_eeprom(struct nfp_nsp *state, int eth_index,
> kfree(buf);
>
> return ret;
> +};
> +
> +int nfp_nsp_read_media(struct nfp_nsp *state, void *buf, unsigned int size)
> +{
> + struct nfp_nsp_command_buf_arg media = {
> + {
> + .code = SPCODE_READ_MEDIA,
> + .option = size,
> + },
> + .in_buf = buf,
> + .in_size = size,
> + .out_buf = buf,
> + .out_size = size,
> + };
> +
> + return nfp_nsp_command_buf(state, &media);
> }
> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
> index 992d72ac98d3..c5630fe88a66 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
> @@ -65,6 +65,11 @@ static inline bool nfp_nsp_has_read_module_eeprom(struct nfp_nsp *state)
> return nfp_nsp_get_abi_ver_minor(state) > 28;
> }
>
> +static inline bool nfp_nsp_has_read_media(struct nfp_nsp *state)
> +{
> + return nfp_nsp_get_abi_ver_minor(state) > 33;
> +}
> +
> enum nfp_eth_interface {
> NFP_INTERFACE_NONE = 0,
> NFP_INTERFACE_SFP = 1,
> @@ -97,6 +102,47 @@ enum nfp_eth_fec {
> NFP_FEC_DISABLED_BIT,
> };
>
> +/* link modes about RJ45 haven't been used, so there's no mapping to them */
> +enum nfp_ethtool_link_mode_list {
> + NFP_MEDIA_W0_RJ45_10M,
> + NFP_MEDIA_W0_RJ45_10M_HD,
> + NFP_MEDIA_W0_RJ45_100M,
> + NFP_MEDIA_W0_RJ45_100M_HD,
> + NFP_MEDIA_W0_RJ45_1G,
> + NFP_MEDIA_W0_RJ45_2P5G,
> + NFP_MEDIA_W0_RJ45_5G,
> + NFP_MEDIA_W0_RJ45_10G,
> + NFP_MEDIA_1000BASE_CX,
> + NFP_MEDIA_1000BASE_KX,
> + NFP_MEDIA_10GBASE_KX4,
> + NFP_MEDIA_10GBASE_KR,
> + NFP_MEDIA_10GBASE_CX4,
> + NFP_MEDIA_10GBASE_CR,
> + NFP_MEDIA_10GBASE_SR,
> + NFP_MEDIA_10GBASE_ER,
> + NFP_MEDIA_25GBASE_KR,
> + NFP_MEDIA_25GBASE_KR_S,
> + NFP_MEDIA_25GBASE_CR,
> + NFP_MEDIA_25GBASE_CR_S,
> + NFP_MEDIA_25GBASE_SR,
> + NFP_MEDIA_40GBASE_CR4,
> + NFP_MEDIA_40GBASE_KR4,
> + NFP_MEDIA_40GBASE_SR4,
> + NFP_MEDIA_40GBASE_LR4,
> + NFP_MEDIA_50GBASE_KR,
> + NFP_MEDIA_50GBASE_SR,
> + NFP_MEDIA_50GBASE_CR,
> + NFP_MEDIA_50GBASE_LR,
> + NFP_MEDIA_50GBASE_ER,
> + NFP_MEDIA_50GBASE_FR,
> + NFP_MEDIA_100GBASE_KR4,
> + NFP_MEDIA_100GBASE_SR4,
> + NFP_MEDIA_100GBASE_CR4,
> + NFP_MEDIA_100GBASE_KP4,
> + NFP_MEDIA_100GBASE_CR10,
> + NFP_MEDIA_LINK_MODES_NUMBER
> +};
> +
> #define NFP_FEC_AUTO BIT(NFP_FEC_AUTO_BIT)
> #define NFP_FEC_BASER BIT(NFP_FEC_BASER_BIT)
> #define NFP_FEC_REED_SOLOMON BIT(NFP_FEC_REED_SOLOMON_BIT)
> @@ -256,6 +302,19 @@ enum nfp_nsp_sensor_id {
> int nfp_hwmon_read_sensor(struct nfp_cpp *cpp, enum nfp_nsp_sensor_id id,
> long *val);
>
> +/* The maximum number of link modes can be added */
> +#define NFP_NSP_MAX_MODE_SIZE 128
> +
> +struct nfp_eth_media_buf {
> + u8 eth_index;
> + u8 reserved[7];
> + DECLARE_BITMAP(supported_modes, NFP_NSP_MAX_MODE_SIZE);
> + DECLARE_BITMAP(advertised_modes, NFP_NSP_MAX_MODE_SIZE);
Jakub said that already, but yeah, object shared with the hardware
must have explicit fixed size and endianness, bitmaps are pure Linux
API and have nothing of these two.
I think here you'd want to use __le64.
> +};
> +
> +int nfp_nsp_read_media(struct nfp_nsp *state, void *buf, unsigned int size);
> +int nfp_eth_read_media(struct nfp_cpp *cpp, struct nfp_eth_media_buf *ethm);
> +
> #define NFP_NSP_VERSION_BUFSZ 1024 /* reasonable size, not in the ABI */
>
> enum nfp_nsp_versions {
> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> index bb64efec4c46..0996fefd6cd9 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> @@ -647,3 +647,30 @@ int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
> return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> lanes, NSP_ETH_CTRL_SET_LANES);
> }
> +
> +int nfp_eth_read_media(struct nfp_cpp *cpp, struct nfp_eth_media_buf *ethm)
> +{
> + struct nfp_nsp *nsp;
> + int err;
> +
> + nsp = nfp_nsp_open(cpp);
> + if (IS_ERR(nsp)) {
> + err = PTR_ERR(nsp);
> + nfp_err(cpp, "Failed to access the NSP: %d\n", err);
> + return err;
nfp_err(cpp, "Failed to access the NSP: -%pe\n, nsp);
return PTR_ERR(nsp);
%pe prints the name if SYMBOLIC_ERRNAME is enabled, and the error
code in dec otherwise.
> + }
> +
> + if (!nfp_nsp_has_read_media(nsp)) {
> + nfp_warn(cpp, "Reading media link modes not supported. Please update flash\n");
> + err = -EOPNOTSUPP;
> + goto exit_close_nsp;
> + }
> +
> + err = nfp_nsp_read_media(nsp, ethm, sizeof(*ethm));
> + if (err)
> + nfp_err(cpp, "Reading link modes failed %d\n", err);
Same here. Also, you don't have a ':' in between 'failed' and the
code format literal.
> +
> +exit_close_nsp:
> + nfp_nsp_close(nsp);
> + return err;
> +}
> --
> 2.30.2
Thanks,
Olek
Powered by blists - more mailing lists