| 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: <ZSa7Y9gwC8qCBv2r@nanopsycho>
Date: Wed, 11 Oct 2023 17:12:35 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Pawel Chmielewski <pawel.chmielewski@...el.com>
Cc: netdev@...r.kernel.org, intel-wired-lan@...ts.osuosl.org,
andrew@...n.ch, aelior@...vell.com, manishc@...vell.com,
horms@...nel.org, vladimir.oltean@....com, pabeni@...hat.com,
davem@...emloft.net, edumazet@...gle.com, jdamato@...tly.com,
d-tatianin@...dex-team.ru, kuba@...nel.org,
Paul Greenwalt <paul.greenwalt@...el.com>,
Jacob Keller <jacob.e.keller@...el.com>,
Przemek Kitszel <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH net-next v4 1/2] ethtool: Add forced speed to supported
link modes maps
Wed, Oct 11, 2023 at 03:13:47PM CEST, pawel.chmielewski@...el.com wrote:
>From: Paul Greenwalt <paul.greenwalt@...el.com>
>
>The need to map Ethtool forced speeds to Ethtool supported link modes is
>common among drivers. To support this, add a common structure for forced
>speed maps and a function to init them. This is solution was originally
>introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes
>maps on module init") for qede driver.
>
>ethtool_forced_speed_maps_init() should be called during driver init
>with an array of struct ethtool_forced_speed_map to populate the mapping.
>
>Definitions for maps themselves are left in the driver code, as the sets
>of supported link modes may vary between the devices.
>
>The qede driver was compile tested only.
>
>Suggested-by: Andrew Lunn <andrew@...n.ch>
>Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
>Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>Signed-off-by: Paul Greenwalt <paul.greenwalt@...el.com>
>Signed-off-by: Pawel Chmielewski <pawel.chmielewski@...el.com>
>---
> .../net/ethernet/qlogic/qede/qede_ethtool.c | 46 +++++--------------
> include/linux/ethtool.h | 27 +++++++++++
> net/ethtool/ioctl.c | 13 ++++++
Would be probably better to this this in 2 patches. 1) Introduce ethtool
infra, 2) convert qede to use it
> 3 files changed, 52 insertions(+), 34 deletions(-)
>
>diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
>index 95820cf1cd6c..d4f2cd13d308 100644
>--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
>+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
>@@ -201,21 +201,6 @@ static const char qede_tests_str_arr[QEDE_ETHTOOL_TEST_MAX][ETH_GSTRING_LEN] = {
>
> /* Forced speed capabilities maps */
>
>-struct qede_forced_speed_map {
>- u32 speed;
>- __ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
>-
>- const u32 *cap_arr;
>- u32 arr_size;
>-};
>-
>-#define QEDE_FORCED_SPEED_MAP(value) \
>-{ \
>- .speed = SPEED_##value, \
>- .cap_arr = qede_forced_speed_##value, \
>- .arr_size = ARRAY_SIZE(qede_forced_speed_##value), \
>-}
>-
> static const u32 qede_forced_speed_1000[] __initconst = {
> ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
>@@ -263,28 +248,21 @@ static const u32 qede_forced_speed_100000[] __initconst = {
> ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT,
> };
>
>-static struct qede_forced_speed_map qede_forced_speed_maps[] __ro_after_init = {
>- QEDE_FORCED_SPEED_MAP(1000),
>- QEDE_FORCED_SPEED_MAP(10000),
>- QEDE_FORCED_SPEED_MAP(20000),
>- QEDE_FORCED_SPEED_MAP(25000),
>- QEDE_FORCED_SPEED_MAP(40000),
>- QEDE_FORCED_SPEED_MAP(50000),
>- QEDE_FORCED_SPEED_MAP(100000),
>+static struct ethtool_forced_speed_map
>+ qede_forced_speed_maps[] __ro_after_init = {
>+ ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 1000),
This is confusing indentation. What about:
static struct ethtool_forced_speed_map
qede_forced_speed_maps[] __ro_after_init = {
ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 1000),
?
>+ ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 10000),
>+ ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 20000),
>+ ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 25000),
>+ ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 40000),
>+ ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 50000),
>+ ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 100000),
> };
>
> void __init qede_forced_speed_maps_init(void)
> {
>- struct qede_forced_speed_map *map;
>- u32 i;
>-
>- for (i = 0; i < ARRAY_SIZE(qede_forced_speed_maps); i++) {
>- map = qede_forced_speed_maps + i;
>-
>- linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps);
>- map->cap_arr = NULL;
>- map->arr_size = 0;
>- }
>+ ethtool_forced_speed_maps_init(qede_forced_speed_maps,
>+ ARRAY_SIZE(qede_forced_speed_maps));
> }
>
> /* Ethtool callbacks */
>@@ -564,8 +542,8 @@ static int qede_set_link_ksettings(struct net_device *dev,
> const struct ethtool_link_ksettings *cmd)
> {
> const struct ethtool_link_settings *base = &cmd->base;
>+ const struct ethtool_forced_speed_map *map;
> struct qede_dev *edev = netdev_priv(dev);
>- const struct qede_forced_speed_map *map;
> struct qed_link_output current_link;
> struct qed_link_params params;
> u32 i;
>diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>index 62b61527bcc4..68d682012e9d 100644
>--- a/include/linux/ethtool.h
>+++ b/include/linux/ethtool.h
>@@ -1052,4 +1052,31 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
> * next string.
> */
> extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...);
>+
>+/* Link mode to forced speed capabilities maps */
>+struct ethtool_forced_speed_map {
>+ u32 speed;
>+ __ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
>+
>+ const u32 *cap_arr;
>+ u32 arr_size;
>+};
>+
>+#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \
>+{ \
>+ .speed = SPEED_##value, \
>+ .cap_arr = prefix##_##value, \
>+ .arr_size = ARRAY_SIZE(prefix##_##value), \
>+}
>+
>+/**
>+ * ethtool_forced_speed_maps_init
>+ * @maps: Pointer to an array of Ethtool forced speed map
>+ * @size: Array size
>+ *
>+ * Initialize an array of Ethtool forced speed map to Ethtool link modes. This
>+ * should be called during driver module init.
>+ */
>+void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
>+ u32 size);
> #endif /* _LINUX_ETHTOOL_H */
>diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
Why you put this into ioctl.c?
Can't this be put into include/linux/linkmode.h as a static helper as
well?
>index 0b0ce4f81c01..34507691fc9d 100644
>--- a/net/ethtool/ioctl.c
>+++ b/net/ethtool/ioctl.c
>@@ -3388,3 +3388,16 @@ void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *flow)
> kfree(flow);
> }
> EXPORT_SYMBOL(ethtool_rx_flow_rule_destroy);
>+
>+void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
>+ u32 size)
>+{
>+ for (u32 i = 0; i < size; i++) {
>+ struct ethtool_forced_speed_map *map = &maps[i];
>+
>+ linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps);
>+ map->cap_arr = NULL;
>+ map->arr_size = 0;
>+ }
>+}
>+EXPORT_SYMBOL(ethtool_forced_speed_maps_init);
>--
>2.37.3
>
>
Powered by blists - more mailing lists