| Message ID | 20241014103810.4015718-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1,1/1] tg3: Increase buffer size for IRQ label | expand |
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>
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.
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).
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; };
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(-)