From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fout-b3-smtp.messagingengine.com (fout-b3-smtp.messagingengine.com [202.12.124.146]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3DD6335B64B for ; Fri, 20 Mar 2026 20:48:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=202.12.124.146 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774039718; cv=none; b=dMPueVfOF7/Gamq5vUfGC1fszgw/MvfUyxDQinvTi8FXuGxXI56YPcvc3+1yfWonrnEv8y2WflWB/uOb0ddzUrBbNoZ+Jen8Oxl5nq9ZcHnLGRhfj+1QE2kQD4iW01HzmeRSjtYUMfsqiWJxe3jjXE/3iM7PQzE5G+BH20LeZRQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774039718; c=relaxed/simple; bh=6iJKeRVvfZKtvhtpJrPt73DDKKzzdGV+KggsGp531mw=; h=MIME-Version:Date:From:To:Cc:Message-Id:In-Reply-To:References: Subject:Content-Type; b=JLpO6ydRL39QZppWbpSF8Wm7S1kzXG7XGhXzHSNNncz0S9bGzJ0j7SeCG0ACHNPLE5ywDw+VCx1bN4cY2jSdvhalMVNBIldU3ITjrL5g6jFuda0R6dzp8vdDrkJAf5hmQNS1/3Hu56iC1y8IhwaeaB7UdGUbWQj339U1ZeKaw48= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arndb.de; spf=pass smtp.mailfrom=arndb.de; dkim=pass (2048-bit key) header.d=arndb.de header.i=@arndb.de header.b=Esq0mgpv; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=VCZ/AHdp; arc=none smtp.client-ip=202.12.124.146 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arndb.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arndb.de Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=arndb.de header.i=@arndb.de header.b="Esq0mgpv"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="VCZ/AHdp" Received: from phl-compute-04.internal (phl-compute-04.internal [10.202.2.44]) by mailfout.stl.internal (Postfix) with ESMTP id D37501D000B2; Fri, 20 Mar 2026 16:48:34 -0400 (EDT) Received: from phl-imap-02 ([10.202.2.81]) by phl-compute-04.internal (MEProxy); Fri, 20 Mar 2026 16:48:35 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=arndb.de; h=cc :cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm3; t=1774039714; x=1774126114; bh=MR438byzDRVcyRIAzP4AWEVh8CmGgC39iq9wYYJZ1E8=; b= Esq0mgpv1o2fwCKdLQyDCSUnC0nd8bc2XeMLb7OWaYCz9Hc7t0js2gmCgmnspy4n DkRKgcdQO9fh264uVmWa9KukQ2aRXANjtGfcsz8f5g/RPmlTuGvxAVMiMhErKv8E VrsNcR1QXueHzgTtNkCmcLptynczYll8xaJ7rTlwBH33/t2K5hBsJX0KjKwP+ph5 nOiBCqBJLZF+ZjmxV8uAaVoe76O0RYPG5Og0a49e299H1X8WC/mXDnbNN6YkZv1I ACWTiLEv0UHsY3m37jMXSYk9rEmuNnWERxacPOrWnRfFtfL2omZIEglMWCmFdU+o KuYNynbJ3JgigK3eXsv5Yw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1774039714; x= 1774126114; bh=MR438byzDRVcyRIAzP4AWEVh8CmGgC39iq9wYYJZ1E8=; b=V CZ/AHdpsPj8Tcw2UrJoFF04ip4tlZ655w87p5Y4nBcodWG9rQBYoJUtpYpd82/gm TeQXea2G8hLq0l9DBOoh27js0OiXcwhKgfrV/NO5UBenyZYfCeSPVKaui7scTrjA bYvLVTmyGSLyAOEHRJE7tbomTlNWnOfZH1YEVKpZRtE39IjEJ+T4njiqLdXrK57V 8QSMNDXTf5mYIdgIvydpKZ9FFNMThBA4GTd4VvgHZ7gn1/NyHeb+zm6upsdLhs5W vz4JjKG2KZeqXKqrFPAXhxWAll8DktgR9QuGl7r8HNWMRAEXzMX2Zkbdec9vJS1Z Ji+pTX2eCjtvircfV1/sA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefgedrtddtgdefuddtleduucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu rghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf gurhepofggfffhvfevkfgjfhfutgfgsehtqhertdertdejnecuhfhrohhmpedftehrnhgu uceuvghrghhmrghnnhdfuceorghrnhgusegrrhhnuggsrdguvgeqnecuggftrfgrthhtvg hrnhepkedvuefhiedtueeijeevtdeiieejfeelvefffeelkeeiteejffdvkefgteeuhffg necuffhomhgrihhnpehkvghrnhgvlhdrohhrghenucevlhhushhtvghrufhiiigvpedtne curfgrrhgrmhepmhgrihhlfhhrohhmpegrrhhnugesrghrnhgusgdruggvpdhnsggprhgt phhtthhopedvtddpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtohepsghrvghtthdrtg hrvggvlhgvhiesrghmugdrtghomhdprhgtphhtthhopehlihhnuhigqdhnvghtqdgurhhi vhgvrhhssegrmhgurdgtohhmpdhrtghpthhtohepuggrvhgvmhesuggrvhgvmhhlohhfth drnhgvthdprhgtphhtthhopehlvghithgrohesuggvsghirghnrdhorhhgpdhrtghpthht ohepvggtrhgvvgdrgihilhhinhigsehgmhgrihhlrdgtohhmpdhrtghpthhtohepnhhitg hkrdguvghsrghulhhnihgvrhhsodhlkhhmlhesghhmrghilhdrtghomhdprhgtphhtthho pegvughumhgriigvthesghhoohhglhgvrdgtohhmpdhrtghpthhtohepjhhushhtihhnsh htihhtthesghhoohhglhgvrdgtohhmpdhrtghpthhtohepmhhorhgsohesghhoohhglhgv rdgtohhm X-ME-Proxy: Feedback-ID: i56a14606:Fastmail Received: by mailuser.phl.internal (Postfix, from userid 501) id 0F04A700069; Fri, 20 Mar 2026 16:48:34 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-ThreadId: Aywyuz1KiUOB Date: Fri, 20 Mar 2026 21:48:13 +0100 From: "Arnd Bergmann" To: "Edward Cree" , "Arnd Bergmann" , "Andrew Lunn" , "David S . Miller" , "Eric Dumazet" , "Jakub Kicinski" , "Paolo Abeni" , "Nathan Chancellor" , "Jeff Garzik" , "Ben Hutchings" Cc: "Nick Desaulniers" , "Bill Wendling" , "Justin Stitt" , "Brett Creeley" , "Breno Leitao" , "Kees Cook" , Netdev , linux-net-drivers@amd.com, linux-kernel@vger.kernel.org, llvm@lists.linux.dev Message-Id: <61de3aea-26b3-4c00-8598-3a137ebef1bc@app.fastmail.com> In-Reply-To: <90a03d3c-def3-4e34-89a0-6e952d3b0d7a@gmail.com> References: <20260320151924.3474821-1-arnd@kernel.org> <90a03d3c-def3-4e34-89a0-6e952d3b0d7a@gmail.com> Subject: Re: [PATCH] net: sfc: avoid format string warning Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On Fri, Mar 20, 2026, at 20:04, Edward Cree wrote: > On 20/03/2026 15:19, Arnd Bergmann wrote: >> From: Arnd Bergmann >>=20 >> Three sfc drivers contain the same *_fill_test() function that takes >> a format string argument that gets passed down to snprintf(), produci= ng >> a warning with clang-22: >>=20 >> drivers/net/ethernet/sfc/falcon/ethtool.c:227:60: error: diagnostic b= ehavior 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_for= mat, test_id); >> | = ^ >> drivers/net/ethernet/sfc/falcon/ethtool.c:210:13: note: 'ef4_fill_tes= t' declared here >>=20 >> Rework these to take a varargs based test_format that allows better >> type checking and avoids this warning. >>=20 >> Fixes: 3273c2e8c66a ("[netdrvr] sfc: sfc: Add self-test support") >> Signed-off-by: Arnd Bergmann > > I'm not convinced this is an improvement. The function takes *two* pr= intf > 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 t= akes > 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/eth= ernet/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_devi= ce *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 !=3D %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_devi= ce *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]; =20 @@ -222,11 +222,11 @@ static void __printf(7, 8) if (strings) { va_list arg; =20 - if (strchr(unit_format, '%')) + if (unit_id >=3D 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) } } =20 -#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) =20 @@ -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")); =20 return test_index; @@ -308,11 +308,11 @@ static int ef4_ethtool_fill_self_tests(struct ef4_= nic *efx, enum ef4_loopback_mode mode; =20 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"); =20 /* Event queues */ ef4_for_each_channel(channel, efx) { @@ -327,9 +327,9 @@ static int ef4_ethtool_fill_self_tests(struct ef4_ni= c *efx, } =20 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"); =20 if (efx->phy_op->run_tests !=3D NULL) { EF4_BUG_ON_PARANOID(efx->phy_op->test_name =3D=3D NULL); @@ -343,7 +343,7 @@ static int ef4_ethtool_fill_self_tests(struct ef4_ni= c *efx, break; =20 ef4_fill_test(n++, strings, data, &tests->phy_ext[i], - "phy", 0, "%s", name); + "phy", -1, "%s", name); } } =20 > I don't like splitting the specifiers/return type from the function na= me > 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 =E2=80=94 some put it on its own li= ne before > `static`, some would put it between `static` and `void`=E2=80=A6 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