Hi,
This series aims to implement ethtool_puts() and send out a wave 1 of
conversions from ethtool_sprintf(). There's also a checkpatch patch
included to check for the cases listed below.
This was sparked from recent discussion here [1]
The conversions are used in cases where ethtool_sprintf() was being used
with just two arguments:
| ethtool_sprintf(&data, buffer[i].name);
or when it's used with format string: "%s"
| ethtool_sprintf(&data, "%s", buffer[i].name);
which both now become:
| ethtool_puts(&data, buffer[i].name);
The first case commonly triggers a -Wformat-security warning with Clang
due to potential problems with format flags present in the strings [3].
The second is just a bit weird with a plain-ol' "%s".
v2 (and newer) of this patch is targeted at linux-next so that we can
catch some of the patches I sent [2] using this "%s" pattern and replace
them before they hit mainline.
Changes found with Cocci [4] and grep [5].
[1]: https://lore.kernel.org/all/202310141935.B326C9E@keescook/
[2]: https://lore.kernel.org/all/?q=dfb%3Aethtool_sprintf+AND+f%3Ajustinstitt
[3]: https://lore.kernel.org/all/202310101528.9496539BE@keescook/
[4]: (script authored by Kees w/ modifications from Joe)
@replace_2_args@
expression BUF;
expression VAR;
@@
- ethtool_sprintf(BUF, VAR)
+ ethtool_puts(BUF, VAR)
@replace_3_args@
expression BUF;
expression VAR;
@@
- ethtool_sprintf(BUF, "%s", VAR)
+ ethtool_puts(BUF, VAR)
- ethtool_sprintf(&BUF, "%s", VAR)
+ ethtool_puts(&BUF, VAR)
[5]: $ rg "ethtool_sprintf\(\s*[^,)]+\s*,\s*[^,)]+\s*\)"
Signed-off-by: Justin Stitt <[email protected]>
---
Changes in v2:
- wrap lines better in replacement (thanks Joe, Kees)
- add --fix to checkpatch (thanks Joe)
- clean up checkpatch formatting (thanks Joe, et al.)
- rebase against next
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Justin Stitt (3):
ethtool: Implement ethtool_puts()
checkpatch: add ethtool_sprintf rules
treewide: Convert some ethtool_sprintf() to ethtool_puts()
drivers/net/dsa/lantiq_gswip.c | 2 +-
drivers/net/dsa/mt7530.c | 2 +-
drivers/net/dsa/qca/qca8k-common.c | 2 +-
drivers/net/dsa/realtek/rtl8365mb.c | 2 +-
drivers/net/dsa/realtek/rtl8366-core.c | 2 +-
drivers/net/dsa/vitesse-vsc73xx-core.c | 8 +--
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 4 +-
drivers/net/ethernet/brocade/bna/bnad_ethtool.c | 2 +-
drivers/net/ethernet/freescale/fec_main.c | 4 +-
.../net/ethernet/fungible/funeth/funeth_ethtool.c | 8 +--
drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c | 2 +-
.../net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c | 2 +-
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 65 +++++++++++-----------
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 6 +-
drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 3 +-
drivers/net/ethernet/intel/ice/ice_ethtool.c | 9 +--
drivers/net/ethernet/intel/idpf/idpf_ethtool.c | 2 +-
drivers/net/ethernet/intel/igb/igb_ethtool.c | 6 +-
drivers/net/ethernet/intel/igc/igc_ethtool.c | 6 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 5 +-
.../net/ethernet/microchip/sparx5/sparx5_ethtool.c | 2 +-
.../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 44 +++++++--------
drivers/net/ethernet/pensando/ionic/ionic_stats.c | 4 +-
drivers/net/ethernet/wangxun/libwx/wx_ethtool.c | 2 +-
drivers/net/hyperv/netvsc_drv.c | 4 +-
drivers/net/phy/nxp-tja11xx.c | 2 +-
drivers/net/phy/smsc.c | 2 +-
drivers/net/vmxnet3/vmxnet3_ethtool.c | 10 ++--
include/linux/ethtool.h | 34 +++++++----
net/ethtool/ioctl.c | 7 +++
scripts/checkpatch.pl | 19 +++++++
31 files changed, 149 insertions(+), 123 deletions(-)
---
base-commit: 2ef7141596eed0b4b45ef18b3626f428a6b0a822
change-id: 20231025-ethtool_puts_impl-a1479ffbc7e0
Best regards,
--
Justin Stitt <[email protected]>
This patch converts some basic cases of ethtool_sprintf() to
ethtool_puts().
The conversions are used in cases where ethtool_sprintf() was being used
with just two arguments:
| ethtool_sprintf(&data, buffer[i].name);
or when it's used with format string: "%s"
| ethtool_sprintf(&data, "%s", buffer[i].name);
which both now become:
| ethtool_puts(&data, buffer[i].name);
Signed-off-by: Justin Stitt <[email protected]>
---
drivers/net/dsa/lantiq_gswip.c | 2 +-
drivers/net/dsa/mt7530.c | 2 +-
drivers/net/dsa/qca/qca8k-common.c | 2 +-
drivers/net/dsa/realtek/rtl8365mb.c | 2 +-
drivers/net/dsa/realtek/rtl8366-core.c | 2 +-
drivers/net/dsa/vitesse-vsc73xx-core.c | 8 +--
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 4 +-
drivers/net/ethernet/brocade/bna/bnad_ethtool.c | 2 +-
drivers/net/ethernet/freescale/fec_main.c | 4 +-
.../net/ethernet/fungible/funeth/funeth_ethtool.c | 8 +--
drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c | 2 +-
.../net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c | 2 +-
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 65 +++++++++++-----------
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 6 +-
drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 3 +-
drivers/net/ethernet/intel/ice/ice_ethtool.c | 9 +--
drivers/net/ethernet/intel/idpf/idpf_ethtool.c | 2 +-
drivers/net/ethernet/intel/igb/igb_ethtool.c | 6 +-
drivers/net/ethernet/intel/igc/igc_ethtool.c | 6 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 5 +-
.../net/ethernet/microchip/sparx5/sparx5_ethtool.c | 2 +-
.../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 44 +++++++--------
drivers/net/ethernet/pensando/ionic/ionic_stats.c | 4 +-
drivers/net/ethernet/wangxun/libwx/wx_ethtool.c | 2 +-
drivers/net/hyperv/netvsc_drv.c | 4 +-
drivers/net/phy/nxp-tja11xx.c | 2 +-
drivers/net/phy/smsc.c | 2 +-
drivers/net/vmxnet3/vmxnet3_ethtool.c | 10 ++--
28 files changed, 100 insertions(+), 112 deletions(-)
diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 9c185c9f0963..05a017c9ef3d 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1759,7 +1759,7 @@ static void gswip_get_strings(struct dsa_switch *ds, int port, u32 stringset,
return;
for (i = 0; i < ARRAY_SIZE(gswip_rmon_cnt); i++)
- ethtool_sprintf(&data, "%s", gswip_rmon_cnt[i].name);
+ ethtool_puts(&data, gswip_rmon_cnt[i].name);
}
static u32 gswip_bcm_ram_entry_read(struct gswip_priv *priv, u32 table,
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index d27c6b70a2f6..391c4dbdff42 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -836,7 +836,7 @@ mt7530_get_strings(struct dsa_switch *ds, int port, u32 stringset,
return;
for (i = 0; i < ARRAY_SIZE(mt7530_mib); i++)
- ethtool_sprintf(&data, "%s", mt7530_mib[i].name);
+ ethtool_puts(&data, mt7530_mib[i].name);
}
static void
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
index 9243eff8918d..2358cd399c7e 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -487,7 +487,7 @@ void qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset,
return;
for (i = 0; i < priv->info->mib_count; i++)
- ethtool_sprintf(&data, "%s", ar8327_mib[i].name);
+ ethtool_puts(&data, ar8327_mib[i].name);
}
void qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 0875e4fc9f57..b072045eb154 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -1303,7 +1303,7 @@ static void rtl8365mb_get_strings(struct dsa_switch *ds, int port, u32 stringset
for (i = 0; i < RTL8365MB_MIB_END; i++) {
struct rtl8365mb_mib_counter *mib = &rtl8365mb_mib_counters[i];
- ethtool_sprintf(&data, "%s", mib->name);
+ ethtool_puts(&data, mib->name);
}
}
diff --git a/drivers/net/dsa/realtek/rtl8366-core.c b/drivers/net/dsa/realtek/rtl8366-core.c
index 82e267b8fddb..59f98d2c8769 100644
--- a/drivers/net/dsa/realtek/rtl8366-core.c
+++ b/drivers/net/dsa/realtek/rtl8366-core.c
@@ -401,7 +401,7 @@ void rtl8366_get_strings(struct dsa_switch *ds, int port, u32 stringset,
return;
for (i = 0; i < priv->num_mib_counters; i++)
- ethtool_sprintf(&data, "%s", priv->mib_counters[i].name);
+ ethtool_puts(&data, priv->mib_counters[i].name);
}
EXPORT_SYMBOL_GPL(rtl8366_get_strings);
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index e6f29e4e508c..dd50502e2122 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -949,7 +949,7 @@ static void vsc73xx_get_strings(struct dsa_switch *ds, int port, u32 stringset,
indices[5] = ((val >> 26) & 0x1f); /* TX counter 2 */
/* The first counters is the RX octets */
- ethtool_sprintf(&buf, "RxEtherStatsOctets");
+ ethtool_puts(&buf, "RxEtherStatsOctets");
/* Each port supports recording 3 RX counters and 3 TX counters,
* figure out what counters we use in this set-up and return the
@@ -959,15 +959,15 @@ static void vsc73xx_get_strings(struct dsa_switch *ds, int port, u32 stringset,
*/
for (i = 0; i < 3; i++) {
cnt = vsc73xx_find_counter(vsc, indices[i], false);
- ethtool_sprintf(&buf, "%s", cnt ? cnt->name : "");
+ ethtool_puts(&buf, cnt ? cnt->name : "");
}
/* TX stats begins with the number of TX octets */
- ethtool_sprintf(&buf, "TxEtherStatsOctets");
+ ethtool_puts(&buf, "TxEtherStatsOctets");
for (i = 3; i < 6; i++) {
cnt = vsc73xx_find_counter(vsc, indices[i], true);
- ethtool_sprintf(&buf, "%s", cnt ? cnt->name : "");
+ ethtool_puts(&buf, cnt ? cnt->name : "");
}
}
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index d671df4b76bc..e3ef081aa42b 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -299,13 +299,13 @@ static void ena_get_strings(struct ena_adapter *adapter,
for (i = 0; i < ENA_STATS_ARRAY_GLOBAL; i++) {
ena_stats = &ena_stats_global_strings[i];
- ethtool_sprintf(&data, ena_stats->name);
+ ethtool_puts(&data, ena_stats->name);
}
if (eni_stats_needed) {
for (i = 0; i < ENA_STATS_ARRAY_ENI(adapter); i++) {
ena_stats = &ena_stats_eni_strings[i];
- ethtool_sprintf(&data, ena_stats->name);
+ ethtool_puts(&data, ena_stats->name);
}
}
diff --git a/drivers/net/ethernet/brocade/bna/bnad_ethtool.c b/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
index df10edff5603..d1ad6c9f8140 100644
--- a/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
+++ b/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
@@ -608,7 +608,7 @@ bnad_get_strings(struct net_device *netdev, u32 stringset, u8 *string)
for (i = 0; i < BNAD_ETHTOOL_STATS_NUM; i++) {
BUG_ON(!(strlen(bnad_net_stats_strings[i]) < ETH_GSTRING_LEN));
- ethtool_sprintf(&string, bnad_net_stats_strings[i]);
+ ethtool_puts(&string, bnad_net_stats_strings[i]);
}
bmap = bna_tx_rid_mask(&bnad->bna);
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c3b7694a7485..bae9536de767 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2932,10 +2932,10 @@ static void fec_enet_get_strings(struct net_device *netdev,
switch (stringset) {
case ETH_SS_STATS:
for (i = 0; i < ARRAY_SIZE(fec_stats); i++) {
- ethtool_sprintf(&data, "%s", fec_stats[i].name);
+ ethtool_puts(&data, fec_stats[i].name);
}
for (i = 0; i < ARRAY_SIZE(fec_xdp_stat_strs); i++) {
- ethtool_sprintf(&data, "%s", fec_xdp_stat_strs[i]);
+ ethtool_puts(&data, fec_xdp_stat_strs[i]);
}
page_pool_ethtool_stats_get_strings(data);
diff --git a/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c b/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
index 31aa185f4d17..091c93bd7587 100644
--- a/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
+++ b/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
@@ -655,7 +655,7 @@ static void fun_get_strings(struct net_device *netdev, u32 sset, u8 *data)
i);
}
for (j = 0; j < ARRAY_SIZE(txq_stat_names); j++)
- ethtool_sprintf(&p, txq_stat_names[j]);
+ ethtool_puts(&p, txq_stat_names[j]);
for (i = 0; i < fp->num_xdpqs; i++) {
for (j = 0; j < ARRAY_SIZE(xdpq_stat_names); j++)
@@ -663,7 +663,7 @@ static void fun_get_strings(struct net_device *netdev, u32 sset, u8 *data)
xdpq_stat_names[j], i);
}
for (j = 0; j < ARRAY_SIZE(xdpq_stat_names); j++)
- ethtool_sprintf(&p, xdpq_stat_names[j]);
+ ethtool_puts(&p, xdpq_stat_names[j]);
for (i = 0; i < netdev->real_num_rx_queues; i++) {
for (j = 0; j < ARRAY_SIZE(rxq_stat_names); j++)
@@ -671,10 +671,10 @@ static void fun_get_strings(struct net_device *netdev, u32 sset, u8 *data)
i);
}
for (j = 0; j < ARRAY_SIZE(rxq_stat_names); j++)
- ethtool_sprintf(&p, rxq_stat_names[j]);
+ ethtool_puts(&p, rxq_stat_names[j]);
for (j = 0; j < ARRAY_SIZE(tls_stat_names); j++)
- ethtool_sprintf(&p, tls_stat_names[j]);
+ ethtool_puts(&p, tls_stat_names[j]);
break;
default:
break;
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c
index 8f391e2adcc0..bdb7afaabdd0 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c
@@ -678,7 +678,7 @@ static void hns_gmac_get_strings(u32 stringset, u8 *data)
return;
for (i = 0; i < ARRAY_SIZE(g_gmac_stats_string); i++)
- ethtool_sprintf(&buff, g_gmac_stats_string[i].desc);
+ ethtool_puts(&buff, g_gmac_stats_string[i].desc);
}
static int hns_gmac_get_sset_count(int stringset)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c
index fc26ffaae620..c58833eb4830 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c
@@ -752,7 +752,7 @@ static void hns_xgmac_get_strings(u32 stringset, u8 *data)
return;
for (i = 0; i < ARRAY_SIZE(g_xgmac_stats_string); i++)
- ethtool_sprintf(&buff, g_xgmac_stats_string[i].desc);
+ ethtool_puts(&buff, g_xgmac_stats_string[i].desc);
}
/**
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index b54f3706fb97..fe40cceb0f79 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
@@ -912,42 +912,41 @@ static void hns_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
if (stringset == ETH_SS_TEST) {
if (priv->ae_handle->phy_if != PHY_INTERFACE_MODE_XGMII)
- ethtool_sprintf(&buff,
- hns_nic_test_strs[MAC_INTERNALLOOP_MAC]);
- ethtool_sprintf(&buff,
- hns_nic_test_strs[MAC_INTERNALLOOP_SERDES]);
+ ethtool_puts(&buff,
+ hns_nic_test_strs[MAC_INTERNALLOOP_MAC]);
+ ethtool_puts(&buff, hns_nic_test_strs[MAC_INTERNALLOOP_SERDES]);
if ((netdev->phydev) && (!netdev->phydev->is_c45))
- ethtool_sprintf(&buff,
- hns_nic_test_strs[MAC_INTERNALLOOP_PHY]);
+ ethtool_puts(&buff,
+ hns_nic_test_strs[MAC_INTERNALLOOP_PHY]);
} else {
- ethtool_sprintf(&buff, "rx_packets");
- ethtool_sprintf(&buff, "tx_packets");
- ethtool_sprintf(&buff, "rx_bytes");
- ethtool_sprintf(&buff, "tx_bytes");
- ethtool_sprintf(&buff, "rx_errors");
- ethtool_sprintf(&buff, "tx_errors");
- ethtool_sprintf(&buff, "rx_dropped");
- ethtool_sprintf(&buff, "tx_dropped");
- ethtool_sprintf(&buff, "multicast");
- ethtool_sprintf(&buff, "collisions");
- ethtool_sprintf(&buff, "rx_over_errors");
- ethtool_sprintf(&buff, "rx_crc_errors");
- ethtool_sprintf(&buff, "rx_frame_errors");
- ethtool_sprintf(&buff, "rx_fifo_errors");
- ethtool_sprintf(&buff, "rx_missed_errors");
- ethtool_sprintf(&buff, "tx_aborted_errors");
- ethtool_sprintf(&buff, "tx_carrier_errors");
- ethtool_sprintf(&buff, "tx_fifo_errors");
- ethtool_sprintf(&buff, "tx_heartbeat_errors");
- ethtool_sprintf(&buff, "rx_length_errors");
- ethtool_sprintf(&buff, "tx_window_errors");
- ethtool_sprintf(&buff, "rx_compressed");
- ethtool_sprintf(&buff, "tx_compressed");
- ethtool_sprintf(&buff, "netdev_rx_dropped");
- ethtool_sprintf(&buff, "netdev_tx_dropped");
-
- ethtool_sprintf(&buff, "netdev_tx_timeout");
+ ethtool_puts(&buff, "rx_packets");
+ ethtool_puts(&buff, "tx_packets");
+ ethtool_puts(&buff, "rx_bytes");
+ ethtool_puts(&buff, "tx_bytes");
+ ethtool_puts(&buff, "rx_errors");
+ ethtool_puts(&buff, "tx_errors");
+ ethtool_puts(&buff, "rx_dropped");
+ ethtool_puts(&buff, "tx_dropped");
+ ethtool_puts(&buff, "multicast");
+ ethtool_puts(&buff, "collisions");
+ ethtool_puts(&buff, "rx_over_errors");
+ ethtool_puts(&buff, "rx_crc_errors");
+ ethtool_puts(&buff, "rx_frame_errors");
+ ethtool_puts(&buff, "rx_fifo_errors");
+ ethtool_puts(&buff, "rx_missed_errors");
+ ethtool_puts(&buff, "tx_aborted_errors");
+ ethtool_puts(&buff, "tx_carrier_errors");
+ ethtool_puts(&buff, "tx_fifo_errors");
+ ethtool_puts(&buff, "tx_heartbeat_errors");
+ ethtool_puts(&buff, "rx_length_errors");
+ ethtool_puts(&buff, "tx_window_errors");
+ ethtool_puts(&buff, "rx_compressed");
+ ethtool_puts(&buff, "tx_compressed");
+ ethtool_puts(&buff, "netdev_rx_dropped");
+ ethtool_puts(&buff, "netdev_tx_dropped");
+
+ ethtool_puts(&buff, "netdev_tx_timeout");
h->dev->ops->get_strings(h, stringset, buff);
}
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index fd7163128c4d..79c3e7968a85 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -2514,13 +2514,11 @@ static void i40e_get_priv_flag_strings(struct net_device *netdev, u8 *data)
u8 *p = data;
for (i = 0; i < I40E_PRIV_FLAGS_STR_LEN; i++)
- ethtool_sprintf(&p, "%s",
- i40e_gstrings_priv_flags[i].flag_string);
+ ethtool_puts(&p, i40e_gstrings_priv_flags[i].flag_string);
if (pf->hw.pf_id != 0)
return;
for (i = 0; i < I40E_GL_PRIV_FLAGS_STR_LEN; i++)
- ethtool_sprintf(&p, "%s",
- i40e_gl_gstrings_priv_flags[i].flag_string);
+ ethtool_puts(&p, i40e_gl_gstrings_priv_flags[i].flag_string);
}
static void i40e_get_strings(struct net_device *netdev, u32 stringset,
diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index 6f236d1a6444..75d433dc1974 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -396,8 +396,7 @@ static void iavf_get_priv_flag_strings(struct net_device *netdev, u8 *data)
unsigned int i;
for (i = 0; i < IAVF_PRIV_FLAGS_STR_LEN; i++)
- ethtool_sprintf(&data, "%s",
- iavf_gstrings_priv_flags[i].flag_string);
+ ethtool_puts(&data, iavf_gstrings_priv_flags[i].flag_string);
}
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 7870a3984547..e16c04924d10 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -1133,8 +1133,7 @@ __ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data,
switch (stringset) {
case ETH_SS_STATS:
for (i = 0; i < ICE_VSI_STATS_LEN; i++)
- ethtool_sprintf(&p, "%s",
- ice_gstrings_vsi_stats[i].stat_string);
+ ethtool_puts(&p, ice_gstrings_vsi_stats[i].stat_string);
if (ice_is_port_repr_netdev(netdev))
return;
@@ -1153,8 +1152,7 @@ __ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data,
return;
for (i = 0; i < ICE_PF_STATS_LEN; i++)
- ethtool_sprintf(&p, "%s",
- ice_gstrings_pf_stats[i].stat_string);
+ ethtool_puts(&p, ice_gstrings_pf_stats[i].stat_string);
for (i = 0; i < ICE_MAX_USER_PRIORITY; i++) {
ethtool_sprintf(&p, "tx_priority_%u_xon.nic", i);
@@ -1170,8 +1168,7 @@ __ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data,
break;
case ETH_SS_PRIV_FLAGS:
for (i = 0; i < ICE_PRIV_FLAG_ARRAY_SIZE; i++)
- ethtool_sprintf(&p, "%s",
- ice_gstrings_priv_flags[i].name);
+ ethtool_puts(&p, ice_gstrings_priv_flags[i].name);
break;
default:
break;
diff --git a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
index 52ea38669f85..bf58989a573e 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
@@ -532,7 +532,7 @@ static void idpf_add_stat_strings(u8 **p, const struct idpf_stats *stats,
unsigned int i;
for (i = 0; i < size; i++)
- ethtool_sprintf(p, "%s", stats[i].stat_string);
+ ethtool_puts(p, stats[i].stat_string);
}
/**
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 16d2a55d5e17..89dac7b215e5 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2356,11 +2356,9 @@ static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
break;
case ETH_SS_STATS:
for (i = 0; i < IGB_GLOBAL_STATS_LEN; i++)
- ethtool_sprintf(&p, "%s",
- igb_gstrings_stats[i].stat_string);
+ ethtool_puts(&p, igb_gstrings_stats[i].stat_string);
for (i = 0; i < IGB_NETDEV_STATS_LEN; i++)
- ethtool_sprintf(&p, "%s",
- igb_gstrings_net_stats[i].stat_string);
+ ethtool_puts(&p, igb_gstrings_net_stats[i].stat_string);
for (i = 0; i < adapter->num_tx_queues; i++) {
ethtool_sprintf(&p, "tx_queue_%u_packets", i);
ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 785eaa8e0ba8..2ed92bf34059 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -773,11 +773,9 @@ static void igc_ethtool_get_strings(struct net_device *netdev, u32 stringset,
break;
case ETH_SS_STATS:
for (i = 0; i < IGC_GLOBAL_STATS_LEN; i++)
- ethtool_sprintf(&p, "%s",
- igc_gstrings_stats[i].stat_string);
+ ethtool_puts(&p, igc_gstrings_stats[i].stat_string);
for (i = 0; i < IGC_NETDEV_STATS_LEN; i++)
- ethtool_sprintf(&p, "%s",
- igc_gstrings_net_stats[i].stat_string);
+ ethtool_puts(&p, igc_gstrings_net_stats[i].stat_string);
for (i = 0; i < adapter->num_tx_queues; i++) {
ethtool_sprintf(&p, "tx_queue_%u_packets", i);
ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 4dd897806fa5..dd722b0381e0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1413,12 +1413,11 @@ static void ixgbe_get_strings(struct net_device *netdev, u32 stringset,
switch (stringset) {
case ETH_SS_TEST:
for (i = 0; i < IXGBE_TEST_LEN; i++)
- ethtool_sprintf(&p, "%s", ixgbe_gstrings_test[i]);
+ ethtool_puts(&p, ixgbe_gstrings_test[i]);
break;
case ETH_SS_STATS:
for (i = 0; i < IXGBE_GLOBAL_STATS_LEN; i++)
- ethtool_sprintf(&p, "%s",
- ixgbe_gstrings_stats[i].stat_string);
+ ethtool_puts(&p, ixgbe_gstrings_stats[i].stat_string);
for (i = 0; i < netdev->num_tx_queues; i++) {
ethtool_sprintf(&p, "tx_queue_%u_packets", i);
ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_ethtool.c b/drivers/net/ethernet/microchip/sparx5/sparx5_ethtool.c
index 37d2584b48a7..a06dc5a9b355 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_ethtool.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_ethtool.c
@@ -1012,7 +1012,7 @@ static void sparx5_get_sset_strings(struct net_device *ndev, u32 sset, u8 *data)
return;
for (idx = 0; idx < sparx5->num_ethtool_stats; idx++)
- ethtool_sprintf(&data, "%s", sparx5->stats_layout[idx]);
+ ethtool_puts(&data, sparx5->stats_layout[idx]);
}
static void sparx5_get_sset_data(struct net_device *ndev,
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index e75cbb287625..1636ce61a3c0 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -800,7 +800,7 @@ static void nfp_get_self_test_strings(struct net_device *netdev, u8 *data)
for (i = 0; i < NFP_TEST_TOTAL_NUM; i++)
if (nfp_self_test[i].is_supported(netdev))
- ethtool_sprintf(&data, nfp_self_test[i].name);
+ ethtool_puts(&data, nfp_self_test[i].name);
}
static int nfp_get_self_test_count(struct net_device *netdev)
@@ -852,24 +852,24 @@ static u8 *nfp_vnic_get_sw_stats_strings(struct net_device *netdev, u8 *data)
ethtool_sprintf(&data, "rvec_%u_tx_busy", i);
}
- ethtool_sprintf(&data, "hw_rx_csum_ok");
- ethtool_sprintf(&data, "hw_rx_csum_inner_ok");
- ethtool_sprintf(&data, "hw_rx_csum_complete");
- ethtool_sprintf(&data, "hw_rx_csum_err");
- ethtool_sprintf(&data, "rx_replace_buf_alloc_fail");
- ethtool_sprintf(&data, "rx_tls_decrypted_packets");
- ethtool_sprintf(&data, "hw_tx_csum");
- ethtool_sprintf(&data, "hw_tx_inner_csum");
- ethtool_sprintf(&data, "tx_gather");
- ethtool_sprintf(&data, "tx_lso");
- ethtool_sprintf(&data, "tx_tls_encrypted_packets");
- ethtool_sprintf(&data, "tx_tls_ooo");
- ethtool_sprintf(&data, "tx_tls_drop_no_sync_data");
-
- ethtool_sprintf(&data, "hw_tls_no_space");
- ethtool_sprintf(&data, "rx_tls_resync_req_ok");
- ethtool_sprintf(&data, "rx_tls_resync_req_ign");
- ethtool_sprintf(&data, "rx_tls_resync_sent");
+ ethtool_puts(&data, "hw_rx_csum_ok");
+ ethtool_puts(&data, "hw_rx_csum_inner_ok");
+ ethtool_puts(&data, "hw_rx_csum_complete");
+ ethtool_puts(&data, "hw_rx_csum_err");
+ ethtool_puts(&data, "rx_replace_buf_alloc_fail");
+ ethtool_puts(&data, "rx_tls_decrypted_packets");
+ ethtool_puts(&data, "hw_tx_csum");
+ ethtool_puts(&data, "hw_tx_inner_csum");
+ ethtool_puts(&data, "tx_gather");
+ ethtool_puts(&data, "tx_lso");
+ ethtool_puts(&data, "tx_tls_encrypted_packets");
+ ethtool_puts(&data, "tx_tls_ooo");
+ ethtool_puts(&data, "tx_tls_drop_no_sync_data");
+
+ ethtool_puts(&data, "hw_tls_no_space");
+ ethtool_puts(&data, "rx_tls_resync_req_ok");
+ ethtool_puts(&data, "rx_tls_resync_req_ign");
+ ethtool_puts(&data, "rx_tls_resync_sent");
return data;
}
@@ -943,13 +943,13 @@ nfp_vnic_get_hw_stats_strings(u8 *data, unsigned int num_vecs, bool repr)
swap_off = repr * NN_ET_SWITCH_STATS_LEN;
for (i = 0; i < NN_ET_SWITCH_STATS_LEN; i++)
- ethtool_sprintf(&data, nfp_net_et_stats[i + swap_off].name);
+ ethtool_puts(&data, nfp_net_et_stats[i + swap_off].name);
for (i = NN_ET_SWITCH_STATS_LEN; i < NN_ET_SWITCH_STATS_LEN * 2; i++)
- ethtool_sprintf(&data, nfp_net_et_stats[i - swap_off].name);
+ ethtool_puts(&data, nfp_net_et_stats[i - swap_off].name);
for (i = NN_ET_SWITCH_STATS_LEN * 2; i < NN_ET_GLOBAL_STATS_LEN; i++)
- ethtool_sprintf(&data, nfp_net_et_stats[i].name);
+ ethtool_puts(&data, nfp_net_et_stats[i].name);
for (i = 0; i < num_vecs; i++) {
ethtool_sprintf(&data, "rxq_%u_pkts", i);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_stats.c b/drivers/net/ethernet/pensando/ionic/ionic_stats.c
index 9859a4432985..1f6022fb7679 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_stats.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_stats.c
@@ -258,10 +258,10 @@ static void ionic_sw_stats_get_strings(struct ionic_lif *lif, u8 **buf)
int i, q_num;
for (i = 0; i < IONIC_NUM_LIF_STATS; i++)
- ethtool_sprintf(buf, ionic_lif_stats_desc[i].name);
+ ethtool_puts(buf, ionic_lif_stats_desc[i].name);
for (i = 0; i < IONIC_NUM_PORT_STATS; i++)
- ethtool_sprintf(buf, ionic_port_stats_desc[i].name);
+ ethtool_puts(buf, ionic_port_stats_desc[i].name);
for (q_num = 0; q_num < MAX_Q(lif); q_num++)
ionic_sw_stats_get_tx_strings(lif, buf, q_num);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index ddc5f6d20b9c..6e9e5f01c152 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -75,7 +75,7 @@ void wx_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
switch (stringset) {
case ETH_SS_STATS:
for (i = 0; i < WX_GLOBAL_STATS_LEN; i++)
- ethtool_sprintf(&p, wx_gstrings_stats[i].stat_string);
+ ethtool_puts(&p, wx_gstrings_stats[i].stat_string);
for (i = 0; i < netdev->num_tx_queues; i++) {
ethtool_sprintf(&p, "tx_queue_%u_packets", i);
ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 3ba3c8fb28a5..cbd9405fc2f3 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1582,10 +1582,10 @@ static void netvsc_get_strings(struct net_device *dev, u32 stringset, u8 *data)
switch (stringset) {
case ETH_SS_STATS:
for (i = 0; i < ARRAY_SIZE(netvsc_stats); i++)
- ethtool_sprintf(&p, netvsc_stats[i].name);
+ ethtool_puts(&p, netvsc_stats[i].name);
for (i = 0; i < ARRAY_SIZE(vf_stats); i++)
- ethtool_sprintf(&p, vf_stats[i].name);
+ ethtool_puts(&p, vf_stats[i].name);
for (i = 0; i < nvdev->num_chn; i++) {
ethtool_sprintf(&p, "tx_queue_%u_packets", i);
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index a71399965142..2c263ae44b4f 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -415,7 +415,7 @@ static void tja11xx_get_strings(struct phy_device *phydev, u8 *data)
int i;
for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++)
- ethtool_sprintf(&data, "%s", tja11xx_hw_stats[i].string);
+ ethtool_puts(&data, tja11xx_hw_stats[i].string);
}
static void tja11xx_get_stats(struct phy_device *phydev,
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 1c7306a1af13..150aea7c9c36 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -508,7 +508,7 @@ static void smsc_get_strings(struct phy_device *phydev, u8 *data)
int i;
for (i = 0; i < ARRAY_SIZE(smsc_hw_stats); i++)
- ethtool_sprintf(&data, "%s", smsc_hw_stats[i].string);
+ ethtool_puts(&data, smsc_hw_stats[i].string);
}
static u64 smsc_get_stat(struct phy_device *phydev, int i)
diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c
index 98c22d7d87a2..8f5f202cde39 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
@@ -245,20 +245,20 @@ vmxnet3_get_strings(struct net_device *netdev, u32 stringset, u8 *buf)
for (j = 0; j < adapter->num_tx_queues; j++) {
for (i = 0; i < ARRAY_SIZE(vmxnet3_tq_dev_stats); i++)
- ethtool_sprintf(&buf, vmxnet3_tq_dev_stats[i].desc);
+ ethtool_puts(&buf, vmxnet3_tq_dev_stats[i].desc);
for (i = 0; i < ARRAY_SIZE(vmxnet3_tq_driver_stats); i++)
- ethtool_sprintf(&buf, vmxnet3_tq_driver_stats[i].desc);
+ ethtool_puts(&buf, vmxnet3_tq_driver_stats[i].desc);
}
for (j = 0; j < adapter->num_rx_queues; j++) {
for (i = 0; i < ARRAY_SIZE(vmxnet3_rq_dev_stats); i++)
- ethtool_sprintf(&buf, vmxnet3_rq_dev_stats[i].desc);
+ ethtool_puts(&buf, vmxnet3_rq_dev_stats[i].desc);
for (i = 0; i < ARRAY_SIZE(vmxnet3_rq_driver_stats); i++)
- ethtool_sprintf(&buf, vmxnet3_rq_driver_stats[i].desc);
+ ethtool_puts(&buf, vmxnet3_rq_driver_stats[i].desc);
}
for (i = 0; i < ARRAY_SIZE(vmxnet3_global_stats); i++)
- ethtool_sprintf(&buf, vmxnet3_global_stats[i].desc);
+ ethtool_puts(&buf, vmxnet3_global_stats[i].desc);
}
netdev_features_t vmxnet3_fix_features(struct net_device *netdev,
--
2.42.0.820.g83a721a137-goog
Use strscpy() to implement ethtool_puts().
Functionally the same as ethtool_sprintf() when it's used with two
arguments or with just "%s" format specifier.
Signed-off-by: Justin Stitt <[email protected]>
---
include/linux/ethtool.h | 34 +++++++++++++++++++++++-----------
net/ethtool/ioctl.c | 7 +++++++
2 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 226a36ed5aa1..7129dd2e227c 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1053,22 +1053,34 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
*/
extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...);
+/**
+ * ethtool_puts - Write string to ethtool string data
+ * @data: Pointer to start of string to update
+ * @str: String to write
+ *
+ * Write string to data. Update data to point at start of next
+ * string.
+ *
+ * Prefer this function to ethtool_sprintf() when given only
+ * two arguments or if @fmt is just "%s".
+ */
+extern void ethtool_puts(u8 **data, const char *str);
+
/* Link mode to forced speed capabilities maps */
struct ethtool_forced_speed_map {
- u32 speed;
+ u32 speed;
__ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
- const u32 *cap_arr;
- u32 arr_size;
+ 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), \
-}
+#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \
+ { \
+ .speed = SPEED_##value, .cap_arr = prefix##_##value, \
+ .arr_size = ARRAY_SIZE(prefix##_##value), \
+ }
-void
-ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size);
+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
index 0b0ce4f81c01..abdf05edf804 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1991,6 +1991,13 @@ __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...)
}
EXPORT_SYMBOL(ethtool_sprintf);
+void ethtool_puts(u8 **data, const char *str)
+{
+ strscpy(*data, str, ETH_GSTRING_LEN);
+ *data += ETH_GSTRING_LEN;
+}
+EXPORT_SYMBOL(ethtool_puts);
+
static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
{
struct ethtool_value id;
--
2.42.0.820.g83a721a137-goog
Add some warnings for using ethtool_sprintf() where a simple
ethtool_puts() would suffice.
The two cases are:
1) Use ethtool_sprintf() with just two arguments:
| ethtool_sprintf(&data, driver[i].name);
or
2) Use ethtool_sprintf() with a standalone "%s" fmt string:
| ethtool_sprintf(&data, "%s", driver[i].name);
The former may cause -Wformat-security warnings while the latter is just
not preferred. Both are safely in the category of warnings, not errors.
Signed-off-by: Justin Stitt <[email protected]>
---
scripts/checkpatch.pl | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25fdb7fda112..22f007131337 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7011,6 +7011,25 @@ sub process {
"Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr);
}
+# ethtool_sprintf uses that should likely be ethtool_puts
+ if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
+ if(WARN("ETHTOOL_SPRINTF",
+ "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/;
+ }
+ }
+
+ # use $rawline because $line loses %s via sanitization and thus we can't match against it.
+ if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) {
+ if(WARN("ETHTOOL_SPRINTF",
+ "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/;
+ }
+ }
+
+
# typecasts on min/max could be min_t/max_t
if ($perl_version_ok &&
defined $stat &&
--
2.42.0.820.g83a721a137-goog
Hi Justin,
On Thu, Oct 26, 2023 at 09:56:07PM +0000, Justin Stitt wrote:
> Use strscpy() to implement ethtool_puts().
>
> Functionally the same as ethtool_sprintf() when it's used with two
> arguments or with just "%s" format specifier.
>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> include/linux/ethtool.h | 34 +++++++++++++++++++++++-----------
> net/ethtool/ioctl.c | 7 +++++++
> 2 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 226a36ed5aa1..7129dd2e227c 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -1053,22 +1053,34 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
> */
> extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...);
>
> +/**
> + * ethtool_puts - Write string to ethtool string data
> + * @data: Pointer to start of string to update
> + * @str: String to write
> + *
> + * Write string to data. Update data to point at start of next
> + * string.
> + *
> + * Prefer this function to ethtool_sprintf() when given only
> + * two arguments or if @fmt is just "%s".
> + */
> +extern void ethtool_puts(u8 **data, const char *str);
> +
> /* Link mode to forced speed capabilities maps */
> struct ethtool_forced_speed_map {
> - u32 speed;
> + u32 speed;
> __ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
>
> - const u32 *cap_arr;
> - u32 arr_size;
> + 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), \
> -}
> +#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \
> + { \
> + .speed = SPEED_##value, .cap_arr = prefix##_##value, \
> + .arr_size = ARRAY_SIZE(prefix##_##value), \
> + }
>
> -void
> -ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size);
> +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
> + u32 size);
> #endif /* _LINUX_ETHTOOL_H */
Maybe this is due to an incorrect rebase conflict resolution, but you
shouldn't have touched any of the ethtool force speed maps.
Please wait for at least 24 hours to pass before posting a new version,
to allow for more comments to come in.
On Thu, Oct 26, 2023 at 3:02 PM Vladimir Oltean <[email protected]> wrote:
>
> Hi Justin,
>
> On Thu, Oct 26, 2023 at 09:56:07PM +0000, Justin Stitt wrote:
> > Use strscpy() to implement ethtool_puts().
> >
> > Functionally the same as ethtool_sprintf() when it's used with two
> > arguments or with just "%s" format specifier.
> >
> > Signed-off-by: Justin Stitt <[email protected]>
> > ---
> > include/linux/ethtool.h | 34 +++++++++++++++++++++++-----------
> > net/ethtool/ioctl.c | 7 +++++++
> > 2 files changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index 226a36ed5aa1..7129dd2e227c 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -1053,22 +1053,34 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
> > */
> > extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...);
> >
> > +/**
> > + * ethtool_puts - Write string to ethtool string data
> > + * @data: Pointer to start of string to update
> > + * @str: String to write
> > + *
> > + * Write string to data. Update data to point at start of next
> > + * string.
> > + *
> > + * Prefer this function to ethtool_sprintf() when given only
> > + * two arguments or if @fmt is just "%s".
> > + */
> > +extern void ethtool_puts(u8 **data, const char *str);
> > +
> > /* Link mode to forced speed capabilities maps */
> > struct ethtool_forced_speed_map {
> > - u32 speed;
> > + u32 speed;
> > __ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
> >
> > - const u32 *cap_arr;
> > - u32 arr_size;
> > + 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), \
> > -}
> > +#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \
> > + { \
> > + .speed = SPEED_##value, .cap_arr = prefix##_##value, \
> > + .arr_size = ARRAY_SIZE(prefix##_##value), \
> > + }
> >
> > -void
> > -ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size);
> > +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
> > + u32 size);
> > #endif /* _LINUX_ETHTOOL_H */
>
> Maybe this is due to an incorrect rebase conflict resolution, but you
> shouldn't have touched any of the ethtool force speed maps.
Ah, I did have a conflict and resolved by simply moving the hunks
out of each other's way. Trivial resolution.
Should I undo this? I want my patch against next since it's targeting
some stuff in-flight over there. BUT, I also want ethtool_puts() to be
directly below ethtool_sprintf() in the source code. What to do?
>
> Please wait for at least 24 hours to pass before posting a new version,
> to allow for more comments to come in.
Ok :)
Thanks
Justin
On Thu, Oct 26, 2023 at 3:09 PM Justin Stitt <[email protected]> wrote:
>
> On Thu, Oct 26, 2023 at 3:02 PM Vladimir Oltean <[email protected]> wrote:
> >
> > Hi Justin,
> >
> > On Thu, Oct 26, 2023 at 09:56:07PM +0000, Justin Stitt wrote:
> > > Use strscpy() to implement ethtool_puts().
> > >
> > > Functionally the same as ethtool_sprintf() when it's used with two
> > > arguments or with just "%s" format specifier.
> > >
> > > Signed-off-by: Justin Stitt <[email protected]>
> > > ---
> > > include/linux/ethtool.h | 34 +++++++++++++++++++++++-----------
> > > net/ethtool/ioctl.c | 7 +++++++
> > > 2 files changed, 30 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > > index 226a36ed5aa1..7129dd2e227c 100644
> > > --- a/include/linux/ethtool.h
> > > +++ b/include/linux/ethtool.h
> > > @@ -1053,22 +1053,34 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
> > > */
> > > extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...);
> > >
> > > +/**
> > > + * ethtool_puts - Write string to ethtool string data
> > > + * @data: Pointer to start of string to update
> > > + * @str: String to write
> > > + *
> > > + * Write string to data. Update data to point at start of next
> > > + * string.
> > > + *
> > > + * Prefer this function to ethtool_sprintf() when given only
> > > + * two arguments or if @fmt is just "%s".
> > > + */
> > > +extern void ethtool_puts(u8 **data, const char *str);
> > > +
> > > /* Link mode to forced speed capabilities maps */
> > > struct ethtool_forced_speed_map {
> > > - u32 speed;
> > > + u32 speed;
> > > __ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
> > >
> > > - const u32 *cap_arr;
> > > - u32 arr_size;
> > > + 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), \
> > > -}
> > > +#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \
> > > + { \
> > > + .speed = SPEED_##value, .cap_arr = prefix##_##value, \
> > > + .arr_size = ARRAY_SIZE(prefix##_##value), \
> > > + }
> > >
> > > -void
> > > -ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size);
> > > +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
> > > + u32 size);
> > > #endif /* _LINUX_ETHTOOL_H */
> >
> > Maybe this is due to an incorrect rebase conflict resolution, but you
> > shouldn't have touched any of the ethtool force speed maps.
>
> Ah, I did have a conflict and resolved by simply moving the hunks
> out of each other's way. Trivial resolution.
>
> Should I undo this? I want my patch against next since it's targeting
> some stuff in-flight over there. BUT, I also want ethtool_puts() to be
> directly below ethtool_sprintf() in the source code. What to do?
Oh, I just realized my auto formatter had a field day with that function.
I will rectify this in a new version after waiting 24hrs for comments to
trickle in as well.
>
> >
> > Please wait for at least 24 hours to pass before posting a new version,
> > to allow for more comments to come in.
>
> Ok :)
>
> Thanks
> Justin
Thanks
Justin
On Thu, Oct 26, 2023 at 09:56:08PM +0000, Justin Stitt wrote:
> Add some warnings for using ethtool_sprintf() where a simple
> ethtool_puts() would suffice.
>
> The two cases are:
>
> 1) Use ethtool_sprintf() with just two arguments:
> | ethtool_sprintf(&data, driver[i].name);
> or
> 2) Use ethtool_sprintf() with a standalone "%s" fmt string:
> | ethtool_sprintf(&data, "%s", driver[i].name);
>
> The former may cause -Wformat-security warnings while the latter is just
> not preferred. Both are safely in the category of warnings, not errors.
>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> scripts/checkpatch.pl | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 25fdb7fda112..22f007131337 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7011,6 +7011,25 @@ sub process {
> "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr);
> }
>
> +# ethtool_sprintf uses that should likely be ethtool_puts
> + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
> + if(WARN("ETHTOOL_SPRINTF",
> + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/;
> + }
> + }
> +
> + # use $rawline because $line loses %s via sanitization and thus we can't match against it.
> + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) {
> + if(WARN("ETHTOOL_SPRINTF",
> + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/;
> + }
> + }
> +
> +
> # typecasts on min/max could be min_t/max_t
> if ($perl_version_ok &&
> defined $stat &&
>
> --
> 2.42.0.820.g83a721a137-goog
>
I don't really know Perl, but does the indentation and coding style here
conform to any rules, or is it just free-form? The rest of the script
looks almost as you'd expect from C. This is unreadable to me.
On Thu, Oct 26, 2023 at 09:56:09PM +0000, Justin Stitt wrote:
> This patch converts some basic cases of ethtool_sprintf() to
> ethtool_puts().
>
> The conversions are used in cases where ethtool_sprintf() was being used
> with just two arguments:
> | ethtool_sprintf(&data, buffer[i].name);
> or when it's used with format string: "%s"
> | ethtool_sprintf(&data, "%s", buffer[i].name);
> which both now become:
> | ethtool_puts(&data, buffer[i].name);
>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> drivers/net/dsa/lantiq_gswip.c | 2 +-
> drivers/net/dsa/mt7530.c | 2 +-
> drivers/net/dsa/qca/qca8k-common.c | 2 +-
> drivers/net/dsa/realtek/rtl8365mb.c | 2 +-
> drivers/net/dsa/realtek/rtl8366-core.c | 2 +-
> drivers/net/dsa/vitesse-vsc73xx-core.c | 8 +--
> drivers/net/ethernet/amazon/ena/ena_ethtool.c | 4 +-
> drivers/net/ethernet/brocade/bna/bnad_ethtool.c | 2 +-
> drivers/net/ethernet/freescale/fec_main.c | 4 +-
> .../net/ethernet/fungible/funeth/funeth_ethtool.c | 8 +--
> drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c | 2 +-
> .../net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c | 2 +-
> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 65 +++++++++++-----------
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 6 +-
> drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 3 +-
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 9 +--
> drivers/net/ethernet/intel/idpf/idpf_ethtool.c | 2 +-
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 6 +-
> drivers/net/ethernet/intel/igc/igc_ethtool.c | 6 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 5 +-
> .../net/ethernet/microchip/sparx5/sparx5_ethtool.c | 2 +-
> .../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 44 +++++++--------
> drivers/net/ethernet/pensando/ionic/ionic_stats.c | 4 +-
> drivers/net/ethernet/wangxun/libwx/wx_ethtool.c | 2 +-
> drivers/net/hyperv/netvsc_drv.c | 4 +-
> drivers/net/phy/nxp-tja11xx.c | 2 +-
> drivers/net/phy/smsc.c | 2 +-
> drivers/net/vmxnet3/vmxnet3_ethtool.c | 10 ++--
> 28 files changed, 100 insertions(+), 112 deletions(-)
What's the "next" branch that you expect this to be applied through, and
why is the patch "treewide"? It only affects networking drivers (I see
nothing outside of drivers/net/) - so it's "net: Convert ..." and it
should go through the "net-next.git" tree. The patch should be formatted
as "PATCH net-next" not "PATCH next", to make this absolutely clear.
On Thu, Oct 26, 2023 at 03:11:28PM -0700, Justin Stitt wrote:
> On Thu, Oct 26, 2023 at 3:09 PM Justin Stitt <[email protected]> wrote:
> > On Thu, Oct 26, 2023 at 3:02 PM Vladimir Oltean <[email protected]> wrote:
> > > Maybe this is due to an incorrect rebase conflict resolution, but you
> > > shouldn't have touched any of the ethtool force speed maps.
> >
> > Ah, I did have a conflict and resolved by simply moving the hunks
> > out of each other's way. Trivial resolution.
> >
> > Should I undo this? I want my patch against next since it's targeting
> > some stuff in-flight over there. BUT, I also want ethtool_puts() to be
> > directly below ethtool_sprintf() in the source code. What to do?
>
> Oh, I just realized my auto formatter had a field day with that function.
> I will rectify this in a new version after waiting 24hrs for comments to
> trickle in as well.
Nothing other than ethtool_puts() should appear in the patch delta.
pw-bot: cr
On Thu, Oct 26, 2023 at 3:12 PM Vladimir Oltean <[email protected]> wrote:
>
> On Thu, Oct 26, 2023 at 09:56:08PM +0000, Justin Stitt wrote:
> > Add some warnings for using ethtool_sprintf() where a simple
> > ethtool_puts() would suffice.
> >
> > The two cases are:
> >
> > 1) Use ethtool_sprintf() with just two arguments:
> > | ethtool_sprintf(&data, driver[i].name);
> > or
> > 2) Use ethtool_sprintf() with a standalone "%s" fmt string:
> > | ethtool_sprintf(&data, "%s", driver[i].name);
> >
> > The former may cause -Wformat-security warnings while the latter is just
> > not preferred. Both are safely in the category of warnings, not errors.
> >
> > Signed-off-by: Justin Stitt <[email protected]>
> > ---
> > scripts/checkpatch.pl | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 25fdb7fda112..22f007131337 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -7011,6 +7011,25 @@ sub process {
> > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr);
> > }
> >
> > +# ethtool_sprintf uses that should likely be ethtool_puts
> > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
> > + if(WARN("ETHTOOL_SPRINTF",
> > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) &&
> > + $fix) {
> > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/;
> > + }
> > + }
> > +
> > + # use $rawline because $line loses %s via sanitization and thus we can't match against it.
> > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) {
> > + if(WARN("ETHTOOL_SPRINTF",
> > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) &&
> > + $fix) {
> > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/;
> > + }
> > + }
> > +
> > +
> > # typecasts on min/max could be min_t/max_t
> > if ($perl_version_ok &&
> > defined $stat &&
> >
> > --
> > 2.42.0.820.g83a721a137-goog
> >
>
> I don't really know Perl, but does the indentation and coding style here
> conform to any rules, or is it just free-form? The rest of the script
> looks almost as you'd expect from C. This is unreadable to me.
There was some discussion here [1] but AFAICT I need to use EMACS
or configure my vim in a very particular way to get the same formatting
But yeah, look around line 7000 -- lots of this pattern matching code is
pretty hard to read. Not sure there's much to be done as far as readability
is concerned.
[1]: https://lore.kernel.org/all/[email protected]/
Thanks
Justin
On Thu, Oct 26, 2023 at 03:09:59PM -0700, Justin Stitt wrote:
> Should I undo this? I want my patch against next since it's targeting
> some stuff in-flight over there. BUT, I also want ethtool_puts() to be
> directly below ethtool_sprintf() in the source code. What to do?
(removing everyone except the lists from CC, I don't want to go to email
arest because of spamming too many recipients)
What is the stuff in-flight in next that this is targeting?
And why would anything prevent you from putting ethtool_puts() directly
below ethtool_sprintf()?
On Thu, Oct 26, 2023 at 03:24:54PM -0700, Justin Stitt wrote:
> There was some discussion here [1] but AFAICT I need to use EMACS
> or configure my vim in a very particular way to get the same formatting
>
> But yeah, look around line 7000 -- lots of this pattern matching code is
> pretty hard to read. Not sure there's much to be done as far as readability
> is concerned.
>
> [1]: https://lore.kernel.org/all/[email protected]/
Hard to read because of pattern matching is one thing, but your
indentation is unlike anything else in this file. There are inner curly
brackets which are less indented than the outer curly brackets. I cannot
read/review this, sorry, I hope somebody else can.
On Thu, 2023-10-26 at 21:56 +0000, Justin Stitt wrote:
> Add some warnings for using ethtool_sprintf() where a simple
> ethtool_puts() would suffice.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7011,6 +7011,25 @@ sub process {
> "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr);
> }
>
> +# ethtool_sprintf uses that should likely be ethtool_puts
> + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
> + if(WARN("ETHTOOL_SPRINTF",
> + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/;
> + }
> + }
> +
> + # use $rawline because $line loses %s via sanitization and thus we can't match against it.
> + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) {
> + if(WARN("ETHTOOL_SPRINTF",
> + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/;
Thanks, but:
This fix wouldn't work if the first argument was itself a function
with multiple arguments.
And this is whitespace formatted incorrectly.
It:
o needs a space after if
o needs a space after the comma in the fix
o needs to use the appropriate amount and tabs for indentation
o needs alignment to open parentheses
o the backslashes are not required in the fix block
Do you want me to push what I wrote in the link below?
https://lore.kernel.org/lkml/[email protected]/
> + }
> + }
> +
> +
> # typecasts on min/max could be min_t/max_t
> if ($perl_version_ok &&
> defined $stat &&
>
> Changes in v2:
> - wrap lines better in replacement (thanks Joe, Kees)
> - add --fix to checkpatch (thanks Joe)
> - clean up checkpatch formatting (thanks Joe, et al.)
> - rebase against next
Please could you explain the rebase against next? As Vladimir pointed
out, all the patches are to drivers/net, so anything in flight should
be in net-next, merged by the netdev Maintainers.
Andrew
On Thu, Oct 26, 2023 at 5:25 PM Andrew Lunn <[email protected]> wrote:
>
> > Changes in v2:
> > - wrap lines better in replacement (thanks Joe, Kees)
> > - add --fix to checkpatch (thanks Joe)
> > - clean up checkpatch formatting (thanks Joe, et al.)
> > - rebase against next
>
> Please could you explain the rebase against next? As Vladimir pointed
> out, all the patches are to drivers/net, so anything in flight should
> be in net-next, merged by the netdev Maintainers.
OK, should v3 be against net-next? I opted for next as a catch-all.
>
> Andrew
Thanks
Justin
On Thu, Oct 26, 2023 at 3:25 PM Vladimir Oltean <[email protected]> wrote:
>
> On Thu, Oct 26, 2023 at 03:09:59PM -0700, Justin Stitt wrote:
> > Should I undo this? I want my patch against next since it's targeting
> > some stuff in-flight over there. BUT, I also want ethtool_puts() to be
> > directly below ethtool_sprintf() in the source code. What to do?
>
> (removing everyone except the lists from CC, I don't want to go to email
> arest because of spamming too many recipients)
>
> What is the stuff in-flight in next that this is targeting?
>
> And why would anything prevent you from putting ethtool_puts() directly
> below ethtool_sprintf()?
The in-flight stuff consists of patches I sent changing some strncpy() usage
to
ethtool_sprintf(&data, "%s", something[i].name);
We can see them here [1]. I went for this approach initially but then
discussion came up about introducing ethtool_puts() which now
made my patches (some accepted into next already) semi-outdated
and in need of another swap from sprintf->puts() -- hence this series.
As far as the rebase, I simply took my commits and placed them on
top of next/master and got merge conflicts when ethtool_puts()
was placed below ethtool_sprintf(). All I have to do is move the hunks
around but since I formatted the file it's appearing in the diff. v3 will
be a clean diff.
[1]: https://lore.kernel.org/all/?q=dfb:ethtool_sprintf%20AND%20f:justinstitt
Thanks
Justin
On Thu, Oct 26, 2023 at 3:39 PM Joe Perches <[email protected]> wrote:
>
> On Thu, 2023-10-26 at 21:56 +0000, Justin Stitt wrote:
> > Add some warnings for using ethtool_sprintf() where a simple
> > ethtool_puts() would suffice.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -7011,6 +7011,25 @@ sub process {
> > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr);
> > }
> >
> > +# ethtool_sprintf uses that should likely be ethtool_puts
> > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
> > + if(WARN("ETHTOOL_SPRINTF",
> > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) &&
> > + $fix) {
> > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/;
> > + }
> > + }
> > +
> > + # use $rawline because $line loses %s via sanitization and thus we can't match against it.
> > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) {
> > + if(WARN("ETHTOOL_SPRINTF",
> > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) &&
> > + $fix) {
> > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/;
>
> Thanks, but:
>
> This fix wouldn't work if the first argument was itself a function
> with multiple arguments.
>
> And this is whitespace formatted incorrectly.
>
> It:
>
> o needs a space after if
> o needs a space after the comma in the fix
> o needs to use the appropriate amount and tabs for indentation
> o needs alignment to open parentheses
> o the backslashes are not required in the fix block
>
> Do you want me to push what I wrote in the link below?
> https://lore.kernel.org/lkml/[email protected]/
Ah, I didn't see you provided a diff in previous thread.
Yeah you can push it but it's not really a standalone so perhaps I'll
just steal the diff and
wrap into v3?
>
> > + }
> > + }
> > +
> > +
> > # typecasts on min/max could be min_t/max_t
> > if ($perl_version_ok &&
> > defined $stat &&
> >
>
Thanks
Justin
On 2023-10-27 12:40, Justin Stitt wrote:
> Yeah you can push it but it's not really a standalone so perhaps I'll
> just steal the diff and
> wrap into v3?
Fine by me.
No need for my sign off.