| Message ID | 20250309214238.66155-1-deller@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: tulip: avoid unused variable warning | expand |
On Sun, Mar 09, 2025 at 10:42:38PM +0100, deller@kernel.org wrote: > From: Helge Deller <deller@gmx.de> > > When compiling with W=1 and CONFIG_TULIP_MWI=n one gets this warning: > drivers/net/ethernet/dec/tulip/tulip_core.c: In function ‘tulip_init_one’: > drivers/net/ethernet/dec/tulip/tulip_core.c:1309:22: warning: variable ‘force_csr0’ set but not used > > Avoid it by annotating the variable __maybe_unused, which seems to be > the easiest solution. > > Signed-off-by: Helge Deller <deller@gmx.de> Hi Helge, A few thoughts on this: Firstly, thanks for your patch, which I agree addresses the problem you have described. However, AFAIK, this is a rather old driver and I'm not sure that addressing somewhat cosmetic problems are worth the churn they cause: maybe it's best to leave it be. But if we do want to fix this problem, I do wonder if the following solution, which leverages IS_ENABLED, is somehow nicer as it slightly reduces the amount of conditionally compiled code, thus increasing compile test coverage. diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c index 27e01d780cd0..75eac18ff246 100644 --- a/drivers/net/ethernet/dec/tulip/tulip_core.c +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c @@ -1177,7 +1177,6 @@ static void set_rx_mode(struct net_device *dev) iowrite32(csr6, ioaddr + CSR6); } -#ifdef CONFIG_TULIP_MWI static void tulip_mwi_config(struct pci_dev *pdev, struct net_device *dev) { struct tulip_private *tp = netdev_priv(dev); @@ -1251,7 +1250,6 @@ static void tulip_mwi_config(struct pci_dev *pdev, struct net_device *dev) netdev_dbg(dev, "MWI config cacheline=%d, csr0=%08x\n", cache, csr0); } -#endif /* * Chips that have the MRM/reserved bit quirk and the burst quirk. That @@ -1463,10 +1461,9 @@ static int tulip_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) INIT_WORK(&tp->media_work, tulip_tbl[tp->chip_id].media_task); -#ifdef CONFIG_TULIP_MWI - if (!force_csr0 && (tp->flags & HAS_PCI_MWI)) + if (IS_ENABLED(CONFIG_TULIP_MWI) && !force_csr0 && + (tp->flags & HAS_PCI_MWI)) tulip_mwi_config (pdev, dev); -#endif /* Stop the chip's Tx and Rx processes. */ tulip_stop_rxtx(tp);
On 3/12/25 14:14, Simon Horman wrote: > On Sun, Mar 09, 2025 at 10:42:38PM +0100, deller@kernel.org wrote: >> From: Helge Deller <deller@gmx.de> >> >> When compiling with W=1 and CONFIG_TULIP_MWI=n one gets this warning: >> drivers/net/ethernet/dec/tulip/tulip_core.c: In function ‘tulip_init_one’: >> drivers/net/ethernet/dec/tulip/tulip_core.c:1309:22: warning: variable ‘force_csr0’ set but not used >> >> Avoid it by annotating the variable __maybe_unused, which seems to be >> the easiest solution. >> >> Signed-off-by: Helge Deller <deller@gmx.de> > > Hi Helge, > > A few thoughts on this: Hi Simon, Thanks for following up on this! > Firstly, thanks for your patch, which I agree addresses the problem you > have described. > > However, AFAIK, this is a rather old driver and I'm not sure that > addressing somewhat cosmetic problems are worth the churn they cause: > maybe it's best to leave it be. Well, the only reason why I sent this patch is, because some people are interested to get a Linux kernel build without any warnings when "W=1" option is enabled. This code in the tulip driver is one of the last 10 places in the kernel where I see a warning at all, so I think it's worth fixing it, although it's just cosmetic. > But if we do want to fix this problem, I do wonder if the following > solution, which leverages IS_ENABLED, is somehow nicer as > it slightly reduces the amount of conditionally compiled code, > thus increasing compile test coverage. Full Ack from my side! I wanted to keep my patch small, but your proposed patch is the better one. I did not compile-test it, but if it builds you may add my: Acked-by: Helge Deller <deller@gmx.de> Helge > diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c > index 27e01d780cd0..75eac18ff246 100644 > --- a/drivers/net/ethernet/dec/tulip/tulip_core.c > +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c > @@ -1177,7 +1177,6 @@ static void set_rx_mode(struct net_device *dev) > iowrite32(csr6, ioaddr + CSR6); > } > > -#ifdef CONFIG_TULIP_MWI > static void tulip_mwi_config(struct pci_dev *pdev, struct net_device *dev) > { > struct tulip_private *tp = netdev_priv(dev); > @@ -1251,7 +1250,6 @@ static void tulip_mwi_config(struct pci_dev *pdev, struct net_device *dev) > netdev_dbg(dev, "MWI config cacheline=%d, csr0=%08x\n", > cache, csr0); > } > -#endif > > /* > * Chips that have the MRM/reserved bit quirk and the burst quirk. That > @@ -1463,10 +1461,9 @@ static int tulip_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > > INIT_WORK(&tp->media_work, tulip_tbl[tp->chip_id].media_task); > > -#ifdef CONFIG_TULIP_MWI > - if (!force_csr0 && (tp->flags & HAS_PCI_MWI)) > + if (IS_ENABLED(CONFIG_TULIP_MWI) && !force_csr0 && > + (tp->flags & HAS_PCI_MWI)) > tulip_mwi_config (pdev, dev); > -#endif > > /* Stop the chip's Tx and Rx processes. */ > tulip_stop_rxtx(tp); >
On Wed, Mar 12, 2025 at 03:08:35PM +0100, Helge Deller wrote: > On 3/12/25 14:14, Simon Horman wrote: > > On Sun, Mar 09, 2025 at 10:42:38PM +0100, deller@kernel.org wrote: > > > From: Helge Deller <deller@gmx.de> > > > > > > When compiling with W=1 and CONFIG_TULIP_MWI=n one gets this warning: > > > drivers/net/ethernet/dec/tulip/tulip_core.c: In function ‘tulip_init_one’: > > > drivers/net/ethernet/dec/tulip/tulip_core.c:1309:22: warning: variable ‘force_csr0’ set but not used > > > > > > Avoid it by annotating the variable __maybe_unused, which seems to be > > > the easiest solution. > > > > > > Signed-off-by: Helge Deller <deller@gmx.de> > > > > Hi Helge, > > > > A few thoughts on this: > > Hi Simon, > > Thanks for following up on this! > > > Firstly, thanks for your patch, which I agree addresses the problem you > > have described. > > > > However, AFAIK, this is a rather old driver and I'm not sure that > > addressing somewhat cosmetic problems are worth the churn they cause: > > maybe it's best to leave it be. > > Well, the only reason why I sent this patch is, because some people > are interested to get a Linux kernel build without any warnings when "W=1" > option is enabled. > This code in the tulip driver is one of the last 10 places in the kernel where > I see a warning at all, so I think it's worth fixing it, although it's just > cosmetic. > > > > But if we do want to fix this problem, I do wonder if the following > > solution, which leverages IS_ENABLED, is somehow nicer as > > it slightly reduces the amount of conditionally compiled code, > > thus increasing compile test coverage. > > Full Ack from my side! > I wanted to keep my patch small, but your proposed patch is the better one. > > I did not compile-test it, but if it builds you may add my: > Acked-by: Helge Deller <deller@gmx.de> Thanks, I posted v2 here: https://lore.kernel.org/netdev/20250313-tulip-w1-v2-1-2ac0d3d909f9@kernel.org/T/#u
diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c index 27e01d780cd0..1ab65deb280c 100644 --- a/drivers/net/ethernet/dec/tulip/tulip_core.c +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c @@ -1306,7 +1306,7 @@ static int tulip_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) const char *chip_name = tulip_tbl[chip_idx].chip_name; unsigned int eeprom_missing = 0; u8 addr[ETH_ALEN] __aligned(2); - unsigned int force_csr0 = 0; + unsigned int force_csr0 __maybe_unused = 0; board_idx++;