[PATCH net-next v1 1/1] tg3: Increase buffer size for IRQ label

[PATCH net-next v1 1/1] tg3: Increase buffer size for IRQ label

[Andy Shevchenko]

GCC is not happy with the current code, e.g.:

.../tg3.c:11313:37: error: ‘-txrx-’ directive output may be truncated writing 6 bytes into a region of size between 1 and 16 [-Werror=format-truncation=]
11313 |                                  "%s-txrx-%d", tp->dev->name, irq_num);
      |                                     ^~~~~~
.../tg3.c:11313:34: note: using the range [-2147483648, 2147483647] for directive argument
11313 |                                  "%s-txrx-%d", tp->dev->name, irq_num);

When `make W=1` is supplied, this prevents kernel building. Fix it by
increasing the buffer size for IRQ label and use sizeoF() instead of
hard coded constants.

While at it, move the respective buffer out from the structure as
it's used only in one caller. This also improves memory footprint
of struct tg3_napi.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 12 ++++++------
 drivers/net/ethernet/broadcom/tg3.h |  1 -
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 378815917741..b5a2f0e2855e 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -11299,6 +11299,7 @@ static void tg3_reset_task(struct work_struct *work)
 
 static int tg3_request_irq(struct tg3 *tp, int irq_num)
 {
+	char irq_lbl[IFNAMSIZ + 6 + 10]; /* name + "-txrx-" + %d */
 	irq_handler_t fn;
 	unsigned long flags;
 	char *name;
@@ -11307,20 +11308,19 @@ static int tg3_request_irq(struct tg3 *tp, int irq_num)
 	if (tp->irq_cnt == 1)
 		name = tp->dev->name;
 	else {
-		name = &tnapi->irq_lbl[0];
+		name = &irq_lbl[0];
 		if (tnapi->tx_buffers && tnapi->rx_rcb)
-			snprintf(name, IFNAMSIZ,
+			snprintf(name, sizeof(irq_lbl),
 				 "%s-txrx-%d", tp->dev->name, irq_num);
 		else if (tnapi->tx_buffers)
-			snprintf(name, IFNAMSIZ,
+			snprintf(name, sizeof(irq_lbl),
 				 "%s-tx-%d", tp->dev->name, irq_num);
 		else if (tnapi->rx_rcb)
-			snprintf(name, IFNAMSIZ,
+			snprintf(name, sizeof(irq_lbl),
 				 "%s-rx-%d", tp->dev->name, irq_num);
 		else
-			snprintf(name, IFNAMSIZ,
+			snprintf(name, sizeof(irq_lbl),
 				 "%s-%d", tp->dev->name, irq_num);
-		name[IFNAMSIZ-1] = 0;
 	}
 
 	if (tg3_flag(tp, USING_MSI) || tg3_flag(tp, USING_MSIX)) {
diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index cf1b2b123c7e..2c5d1eee8068 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -3033,7 +3033,6 @@ struct tg3_napi {
 	dma_addr_t			rx_rcb_mapping;
 	dma_addr_t			tx_desc_mapping;
 
-	char				irq_lbl[IFNAMSIZ];
 	unsigned int			irq_vec;
 };
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH net-next v1 1/1] tg3: Increase buffer size for IRQ label

[Simon Horman]

On Mon, Oct 14, 2024 at 01:38:10PM +0300, Andy Shevchenko wrote:
> GCC is not happy with the current code, e.g.:
> 
> .../tg3.c:11313:37: error: ‘-txrx-’ directive output may be truncated writing 6 bytes into a region of size between 1 and 16 [-Werror=format-truncation=]
> 11313 |                                  "%s-txrx-%d", tp->dev->name, irq_num);
>       |                                     ^~~~~~
> .../tg3.c:11313:34: note: using the range [-2147483648, 2147483647] for directive argument
> 11313 |                                  "%s-txrx-%d", tp->dev->name, irq_num);
> 
> When `make W=1` is supplied, this prevents kernel building. Fix it by
> increasing the buffer size for IRQ label and use sizeoF() instead of
> hard coded constants.
> 
> While at it, move the respective buffer out from the structure as
> it's used only in one caller. This also improves memory footprint
> of struct tg3_napi.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Very nice to see this addressed :)

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v1 1/1] tg3: Increase buffer size for IRQ label

[Jakub Kicinski]

On Mon, 14 Oct 2024 13:38:10 +0300 Andy Shevchenko wrote:
> While at it, move the respective buffer out from the structure as
> it's used only in one caller. This also improves memory footprint
> of struct tg3_napi.

It's passed to request_irq(), I thought request_irq() dups the string
but I can't see it now. So please include in the commit message a
reference to the function where the strdup (or such) of name happens 
in the request_irq() internals.
-- 
pw-bot: cr

Re: [PATCH net-next v1 1/1] tg3: Increase buffer size for IRQ label

[Andy Shevchenko]

On Tue, Oct 15, 2024 at 08:16:21AM -0700, Jakub Kicinski wrote:
> On Mon, 14 Oct 2024 13:38:10 +0300 Andy Shevchenko wrote:
> > While at it, move the respective buffer out from the structure as
> > it's used only in one caller. This also improves memory footprint
> > of struct tg3_napi.
> 
> It's passed to request_irq(), I thought request_irq() dups the string
> but I can't see it now. So please include in the commit message a
> reference to the function where the strdup (or such) of name happens 
> in the request_irq() internals.

Hmm... you are right, the name should be kept as long as device instance alive
(more precisely till calling free_irq() for the IRQ handler in question).

I will redo this part (currently I'm choosing between leaving the name in the
structure or using devm_kasprintf()for it, I'll check which one looks better
at the end).

-- 
With Best Regards,
Andy Shevchenko