Re: [PATCH] net: sfc: avoid format string warning

["Arnd Bergmann" <arnd@arndb.de>]

On Fri, Mar 20, 2026, at 20:04, Edward Cree wrote:
> On 20/03/2026 15:19, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> Three sfc drivers contain the same *_fill_test() function that takes
>> a format string argument that gets passed down to snprintf(), producing
>> a warning with clang-22:
>> 
>> drivers/net/ethernet/sfc/falcon/ethtool.c:227:60: error: diagnostic behavior may be improved by adding
>>       the 'format(printf, 7, 8)' attribute to the declaration of 'ef4_fill_test' [-Werror,-Wmissing-format-attribute]
>>   210 |                 snprintf(test_str, sizeof(test_str), test_format, test_id);
>>       |                                                                          ^
>> drivers/net/ethernet/sfc/falcon/ethtool.c:210:13: note: 'ef4_fill_test' declared here
>> 
>> Rework these to take a varargs based test_format that allows better
>> type checking and avoids this warning.
>> 
>> Fixes: 3273c2e8c66a ("[netdrvr] sfc: sfc: Add self-test support")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> I'm not convinced this is an improvement.  The function takes *two* printf
>  format strings (unit_format and test_format), and this only annotates and
>  varargs-ifies one of them (and doesn't harden the unit_format, which has
>  the considerably more worrying-looking "if it has a % in, assume it takes
>  precisely one int argument").

Right, I took a bit of a shortcut there. How about changing it
so the other format string is just a name that gets joined with the
number?

diff --git a/drivers/net/ethernet/sfc/falcon/ethtool.c b/drivers/net/ethernet/sfc/falcon/ethtool.c
index 1ae8acb8fdc8..a0064eadff41 100644
--- a/drivers/net/ethernet/sfc/falcon/ethtool.c
+++ b/drivers/net/ethernet/sfc/falcon/ethtool.c
@@ -200,7 +200,7 @@ static void ef4_ethtool_set_msglevel(struct net_device *net_dev, u32 msg_enable)
  * @strings:		Ethtool strings, or %NULL
  * @data:		Ethtool test results, or %NULL
  * @test:		Pointer to test result (used only if data != %NULL)
- * @unit_format:	Unit name format (e.g. "chan\%d")
+ * @unit_name:		Unit name format (e.g. "chan\%d")
  * @unit_id:		Unit id (e.g. 0 for "chan0")
  * @test_format:	Test name format (e.g. "loopback.\%s.tx.sent")
  * @test_id:		Test id (e.g. "PHYXS" for "loopback.PHYXS.tx_sent")
@@ -208,9 +208,9 @@ static void ef4_ethtool_set_msglevel(struct net_device *net_dev, u32 msg_enable)
  * Fill in an individual self-test entry.
  */
 static void __printf(7, 8)
- ef4_fill_test(unsigned int test_index, u8 *strings, u64 *data,
-			  int *test, const char *unit_format, int unit_id,
-			  const char *test_format, ...)
+ef4_fill_test(unsigned int test_index, u8 *strings, u64 *data,
+	      int *test, const char *unit_name, int unit_id,
+	      const char *test_format, ...)
 {
 	char unit_str[ETH_GSTRING_LEN], test_str[ETH_GSTRING_LEN];
 
@@ -222,11 +222,11 @@ static void __printf(7, 8)
 	if (strings) {
 		va_list arg;
 
-		if (strchr(unit_format, '%'))
+		if (unit_id >= 0)
 			snprintf(unit_str, sizeof(unit_str),
-				 unit_format, unit_id);
+				 "%s%d", unit_name, unit_id);
 		else
-			strcpy(unit_str, unit_format);
+			strcpy(unit_str, unit_name);
 		va_start(arg, test_format);
 		vsnprintf(test_str, sizeof(test_str), test_format, arg);
 		va_end(arg);
@@ -236,9 +236,9 @@ static void __printf(7, 8)
 	}
 }
 
-#define EF4_CHANNEL_NAME(_channel) "chan%d", _channel->channel
-#define EF4_TX_QUEUE_NAME(_tx_queue) "txq%d", _tx_queue->queue
-#define EF4_RX_QUEUE_NAME(_rx_queue) "rxq%d", _rx_queue->queue
+#define EF4_CHANNEL_NAME(_channel) "chan", _channel->channel
+#define EF4_TX_QUEUE_NAME(_tx_queue) "txq", _tx_queue->queue
+#define EF4_RX_QUEUE_NAME(_rx_queue) "rxq", _rx_queue->queue
 #define EF4_LOOPBACK_NAME(_mode, _counter)			\
 	"loopback.%s." _counter, STRING_TABLE_LOOKUP(_mode, ef4_loopback_mode)
 
@@ -276,11 +276,11 @@ static int ef4_fill_loopback_test(struct ef4_nic *efx,
 	}
 	ef4_fill_test(test_index++, strings, data,
 		      &lb_tests->rx_good,
-		      "rx", 0,
+		      "rx", -1,
 		      EF4_LOOPBACK_NAME(mode, "rx_good"));
 	ef4_fill_test(test_index++, strings, data,
 		      &lb_tests->rx_bad,
-		      "rx", 0,
+		      "rx", -1,
 		      EF4_LOOPBACK_NAME(mode, "rx_bad"));
 
 	return test_index;
@@ -308,11 +308,11 @@ static int ef4_ethtool_fill_self_tests(struct ef4_nic *efx,
 	enum ef4_loopback_mode mode;
 
 	ef4_fill_test(n++, strings, data, &tests->phy_alive,
-		      "phy", 0, "alive");
+		      "phy", -1, "alive");
 	ef4_fill_test(n++, strings, data, &tests->nvram,
-		      "core", 0, "nvram");
+		      "core", -1, "nvram");
 	ef4_fill_test(n++, strings, data, &tests->interrupt,
-		      "core", 0, "interrupt");
+		      "core", -1, "interrupt");
 
 	/* Event queues */
 	ef4_for_each_channel(channel, efx) {
@@ -327,9 +327,9 @@ static int ef4_ethtool_fill_self_tests(struct ef4_nic *efx,
 	}
 
 	ef4_fill_test(n++, strings, data, &tests->memory,
-		      "core", 0, "memory");
+		      "core", -1, "memory");
 	ef4_fill_test(n++, strings, data, &tests->registers,
-		      "core", 0, "registers");
+		      "core", -1, "registers");
 
 	if (efx->phy_op->run_tests != NULL) {
 		EF4_BUG_ON_PARANOID(efx->phy_op->test_name == NULL);
@@ -343,7 +343,7 @@ static int ef4_ethtool_fill_self_tests(struct ef4_nic *efx,
 				break;
 
 			ef4_fill_test(n++, strings, data, &tests->phy_ext[i],
-				      "phy", 0, "%s", name);
+				      "phy", -1, "%s", name);
 		}
 	}
 

> I don't like splitting the specifiers/return type from the function name
>  like this.  It's not Linux style[1].
> (From a quick poke around the tree, there seems to be less consensus on
>  where the __printf attribute goes — some put it on its own line before
>  `static`, some would put it between `static` and `void`… sigh.)
>
> [1]: https://lore.kernel.org/all/1054519757.161606@palladium.transmeta.com/T/#u

There are not a lot of good options here, and splitting the line seems
better than an overlong line to me. I don't really have a strong opinion
on where the __printf attribute should go either, but I do see that
after the return type is probably the least common, so I'll change
that. How about having the specifiers on one line and
the type in front of the name? that seems faily common.

static __printf(7, 8)
void ef4_fill_test(unsigned int test_index, u8 *strings, u64 *data,

    Arnd