[Patch] Shut up warnings from files under drivers/

Previous thread: [PATCH] linux/types.h: always export 64bit aligned defines by Mike Frysinger on Saturday, January 26, 2008 - 5:23 am. (5 messages)

Next thread: Re: [PATCH 1/6] Core driver for WM97xx touchscreens by Dmitry Baryshkov on Saturday, January 26, 2008 - 5:58 am. (1 message)
To: Greg KH <gregkh@...>
Cc: LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 5:30 am

Fix two kind of defined-but-not-used warnings.

One is due to defination of MODULE_DEVICE_TABLE, the
other is due to __devexit_p. The solution is just to
add proper directives to protect those usages.

Compile tests passed.

Cc: Greg KH <gregkh@suse.de>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

---

diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
index 1f0b752..247bc16 100644
--- a/drivers/char/applicom.c
+++ b/drivers/char/applicom.c
@@ -65,6 +65,7 @@ static char *applicom_pci_devnames[] = {
"PCI2000PFB"
};

+#ifdef MODULE
static struct pci_device_id applicom_pci_tbl[] = {
{ PCI_VENDOR_ID_APPLICOM, PCI_DEVICE_ID_APPLICOM_PCIGENERIC,
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
@@ -74,6 +75,8 @@ static struct pci_device_id applicom_pci_tbl[] = {
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
{ 0 }
};
+#endif
+
MODULE_DEVICE_TABLE(pci, applicom_pci_tbl);

MODULE_AUTHOR("David Woodhouse & Applicom International");
diff --git a/drivers/char/synclink.c b/drivers/char/synclink.c
index 905d1f5..2009dc9 100644
--- a/drivers/char/synclink.c
+++ b/drivers/char/synclink.c
@@ -897,7 +897,9 @@ static char *driver_version = "$Revision: 4.38 $";

static int synclink_init_one (struct pci_dev *dev,
const struct pci_device_id *ent);
+#if defined(MODULE) || defined(CONFIG_HOTPLUG)
static void synclink_remove_one (struct pci_dev *dev);
+#endif

static struct pci_device_id synclink_pci_tbl[] = {
{ PCI_VENDOR_ID_MICROGATE, PCI_DEVICE_ID_MICROGATE_USC, PCI_ANY_ID, PCI_ANY_ID, },
@@ -8166,7 +8168,8 @@ static int __devinit synclink_init_one (struct pci_dev *dev,
return 0;
}

+#if defined(MODULE) || defined(CONFIG_HOTPLUG)
static void __devexit synclink_remove_one (struct pci_dev *dev)
{
}
-
+#endif
diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index 16413e5..635f651 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -2046,6 +2046,7 @@ static irqreturn_t hifn_inte...

To: WANG Cong <xiyou.wangcong@...>
Cc: Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 5:57 am

Hi WANG.

As replacement for the above I would prefer som kind of annotation
so we can drop the symbol at linker time.

where we have:
#ifdef MODULE
#define __moduseddata __section(.module.data)
#define __modusedconst __section(.module.rodata)
#define __modused __section(.module.text)
#else
#define __moduseddata __section(.discard.data)
#define __modusedconst __section(.discard.rodata)
#define __modused __section(.discard.text)
#endif

And we can then discard the symbols as we do for
__initdata today.

This function is properly annotated and __devexit_p() is used
so it should not generate a warning.

The root casue is that __devexit is defined to nothing in the
#ifdef CONFIG_HOTPLUG case - it should have been defined as
#define __devexit __used
if MODULE was not defined.

This is the better fix for these kind of warnings.
For the latter I have fixed this in kbuild.git.
So we are only left with the DEVICE_MOD_TABLE issue.

Sam
--

To: WANG Cong <xiyou.wangcong@...>
Cc: Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 6:21 am

Looking a bit closer the above is rubbish.
We have:
#if defined(MODULE) || defined(CONFIG_HOTPLUG)
#define __devexit_p(x) x
#else
#define __devexit_p(x) NULL

so the pointer to the function is used in
both cases.

Could you drop me the config that produces the warning
and the warning message.

Thanks,
Sam
--

To: Sam Ravnborg <sam@...>
Cc: WANG Cong <xiyou.wangcong@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 6:26 am

I think in the latter case, 'x' is not used, just

Of course. Attached.

Regards.

To: Sam Ravnborg <sam@...>
Cc: WANG Cong <xiyou.wangcong@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 6:15 am

Sure.

Such as this one:

drivers/char/applicom.c:68: warning: ‘applicom_pci_tbl’ defined but not used

Others are similar.

--

To: WANG Cong <xiyou.wangcong@...>
Cc: Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 5:55 am

Quite strange -- due to __devexit_p() and the __devexit marker, ifdefs
should not be needed.

I would look into why that isn't working as designed in these cases...

Jeff

--

To: Jeff Garzik <jeff@...>
Cc: WANG Cong <xiyou.wangcong@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 3:17 pm

I checked up on the synclink.c warning.
We have the following code:

static void synclink_remove_one (struct pci_dev *dev);

...

static struct pci_driver synclink_pci_driver = {
.remove = __devexit_p(synclink_remove_one),
};

...

static void __devexit synclink_remove_one (struct pci_dev *dev)
{
}

And I double checked the preprocessed source to check
that we applied the __attribute__((__used__)) to the function.

Investigating a bit more I realized that gcc looses the
__used__ attribution due to the prototype.
So there are two correct fixes:
a) move the function up so we do not need the forward
declaration
b) add a __devexit to the forward decalration too.

I strongly prefer the first version and this is the
correct fix for these cases.

Do we have a gcc bug here - I did not see a definitive answer in gcc docs?

Sam
--

To: Sam Ravnborg <sam@...>
Cc: Jeff Garzik <jeff@...>, WANG Cong <xiyou.wangcong@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Sunday, January 27, 2008 - 12:15 am

Here is the updated version.

---->

Fix defined-but-not-used warnings from files under drivers/,
such as:

drivers/char/applicom.c:68: warning: ‘applicom_pci_tbl’ defined but not used

Compile tests passed.

Cc: Jeff Garzik <jeff@garzik.org>
Cc: Greg KH <gregkh@suse.de>
Cc: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

---

diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
index 1f0b752..97171ad 100644
--- a/drivers/char/applicom.c
+++ b/drivers/char/applicom.c
@@ -65,7 +65,7 @@ static char *applicom_pci_devnames[] = {
"PCI2000PFB"
};

-static struct pci_device_id applicom_pci_tbl[] = {
+static struct pci_device_id applicom_pci_tbl[] __used = {
{ PCI_VENDOR_ID_APPLICOM, PCI_DEVICE_ID_APPLICOM_PCIGENERIC,
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
{ PCI_VENDOR_ID_APPLICOM, PCI_DEVICE_ID_APPLICOM_PCI2000IBS_CAN,
diff --git a/drivers/char/synclink.c b/drivers/char/synclink.c
index 905d1f5..ae1f565 100644
--- a/drivers/char/synclink.c
+++ b/drivers/char/synclink.c
@@ -897,7 +897,6 @@ static char *driver_version = "$Revision: 4.38 $";

static int synclink_init_one (struct pci_dev *dev,
const struct pci_device_id *ent);
-static void synclink_remove_one (struct pci_dev *dev);

static struct pci_device_id synclink_pci_tbl[] = {
{ PCI_VENDOR_ID_MICROGATE, PCI_DEVICE_ID_MICROGATE_USC, PCI_ANY_ID, PCI_ANY_ID, },
@@ -908,6 +907,10 @@ MODULE_DEVICE_TABLE(pci, synclink_pci_tbl);

MODULE_LICENSE("GPL");

+static void __devexit synclink_remove_one (struct pci_dev *dev)
+{
+}
+
static struct pci_driver synclink_pci_driver = {
.name = "synclink",
.id_table = synclink_pci_tbl,
@@ -8166,7 +8169,3 @@ static int __devinit synclink_init_one (struct pci_dev *dev,
return 0;
}

-static void __devexit synclink_remove_one (struct pci_dev *dev)
-{
-}
-
diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index dfbf24c..f33f82a 100644
--- a/driver...

To: WANG Cong <xiyou.wangcong@...>
Cc: Jeff Garzik <jeff@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Sunday, January 27, 2008 - 5:21 am

Hi WANG.

Thanks for chasing these annoying warnings - it is
good to keep the warning level low so we keep

It is preferable to have the forward declaration unconditional

Moving the aty_resume_chip() function down just above
atyfb_pci_resume() would put in in the same #if/#endif
block and you can kill the forward declaration too.
Did you try that out?

I could not spot any obvious reason why it should not work,
but I did not try it out.

Sam
--

To: Sam Ravnborg <sam@...>
Cc: WANG Cong <xiyou.wangcong@...>, Jeff Garzik <jeff@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Monday, January 28, 2008 - 1:49 am

Yes, it works.

Here's the updated patch, and I dropped the drivers/char/applicom.c
part as Jiri Slaby has better fix.

------->

Fix defined-but-not-used warnings from files under drivers/,
such as:

drivers/video/aty/atyfb_base.c:2713: warning: ‘aty_resume_chip’ defined but not used

Compile tests passed.

Cc: Jeff Garzik <jeff@garzik.org>
Cc: Greg KH <gregkh@suse.de>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Jiri Slaby <jirislaby@gmail.com>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

---

diff --git a/drivers/char/synclink.c b/drivers/char/synclink.c
index 905d1f5..ae1f565 100644
--- a/drivers/char/synclink.c
+++ b/drivers/char/synclink.c
@@ -897,7 +897,6 @@ static char *driver_version = "$Revision: 4.38 $";

static int synclink_init_one (struct pci_dev *dev,
const struct pci_device_id *ent);
-static void synclink_remove_one (struct pci_dev *dev);

static struct pci_device_id synclink_pci_tbl[] = {
{ PCI_VENDOR_ID_MICROGATE, PCI_DEVICE_ID_MICROGATE_USC, PCI_ANY_ID, PCI_ANY_ID, },
@@ -908,6 +907,10 @@ MODULE_DEVICE_TABLE(pci, synclink_pci_tbl);

MODULE_LICENSE("GPL");

+static void __devexit synclink_remove_one (struct pci_dev *dev)
+{
+}
+
static struct pci_driver synclink_pci_driver = {
.name = "synclink",
.id_table = synclink_pci_tbl,
@@ -8166,7 +8169,3 @@ static int __devinit synclink_init_one (struct pci_dev *dev,
return 0;
}

-static void __devexit synclink_remove_one (struct pci_dev *dev)
-{
-}
-
diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index dfbf24c..f33f82a 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -2730,7 +2730,7 @@ err_out_disable_pci_device:
return err;
}

-static void hifn_remove(struct pci_dev *pdev)
+static void __devexit hifn_remove(struct pci_dev *pdev)
{
int i;
struct hifn_device *dev;
diff --git a/drivers/telephony/ixj.c b/drivers/telephony/ixj.c
index 49cd979..1549c0b 100644
--- a/...

To: Andrew Morton <akpm@...>
Cc: <linux-kernel@...>, <dwmw2@...>, Jiri Slaby <jirislaby@...>, WANG Cong <xiyou.wangcong@...>
Date: Wednesday, January 30, 2008 - 7:13 am

Instead of testing hardcoded values, use pci_match_id to reference the
pci_device_id table. Sideways, it allows easy new additions to the table.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: WANG Cong <xiyou.wangcong@gmail.com>
---
drivers/char/applicom.c | 15 ++++-----------
1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
index b0bb71b..64bf71c 100644
--- a/drivers/char/applicom.c
+++ b/drivers/char/applicom.c
@@ -57,7 +57,6 @@
#define PCI_DEVICE_ID_APPLICOM_PCI2000IBS_CAN 0x0002
#define PCI_DEVICE_ID_APPLICOM_PCI2000PFB 0x0003
#endif
-#define MAX_PCI_DEVICE_NUM 3

static char *applicom_pci_devnames[] = {
"PCI board",
@@ -66,12 +65,9 @@ static char *applicom_pci_devnames[] = {
};

static struct pci_device_id applicom_pci_tbl[] = {
- { PCI_VENDOR_ID_APPLICOM, PCI_DEVICE_ID_APPLICOM_PCIGENERIC,
- PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
- { PCI_VENDOR_ID_APPLICOM, PCI_DEVICE_ID_APPLICOM_PCI2000IBS_CAN,
- PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
- { PCI_VENDOR_ID_APPLICOM, PCI_DEVICE_ID_APPLICOM_PCI2000PFB,
- PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
+ { PCI_VDEVICE(APPLICOM, PCI_DEVICE_ID_APPLICOM_PCIGENERIC) },
+ { PCI_VDEVICE(APPLICOM, PCI_DEVICE_ID_APPLICOM_PCI2000IBS_CAN) },
+ { PCI_VDEVICE(APPLICOM, PCI_DEVICE_ID_APPLICOM_PCI2000PFB) },
{ 0 }
};
MODULE_DEVICE_TABLE(pci, applicom_pci_tbl);
@@ -197,10 +193,7 @@ static int __init applicom_init(void)

while ( (dev = pci_get_class(PCI_CLASS_OTHERS << 16, dev))) {

- if (dev->vendor != PCI_VENDOR_ID_APPLICOM)
- continue;
-
- if (dev->device > MAX_PCI_DEVICE_NUM || dev->device == 0)
+ if (!pci_match_id(applicom_pci_tbl, dev));
continue;

if (pci_enable_device(dev))
--
1.5.3.8

--

To: Jiri Slaby <jirislaby@...>
Cc: <linux-kernel@...>, <dwmw2@...>, <jirislaby@...>, <xiyou.wangcong@...>
Date: Wednesday, January 30, 2008 - 7:10 pm

On Wed, 30 Jan 2008 12:13:15 +0100

The patch was carelessly prepared, was not runtine-tested and was not
passed through checkpatch, which detects this error. Please fix these
things, permanently.

It's hard to overemphasise how out-of-balance the economics are here. You
saved maybe thirty person-seconds by skipping the review and checkpatch
steps. But the cost (if this bug had gone into mainline) would be many
many thousands times higher than this.
--

To: Andrew Morton <akpm@...>
Cc: <linux-kernel@...>, <dwmw2@...>, <xiyou.wangcong@...>
Date: Thursday, January 31, 2008 - 5:26 am

Yes, you're right, I definitely need to change my workflow, this needn't happen.
--

To: Andrew Morton <akpm@...>
Cc: <linux-kernel@...>, <dwmw2@...>, Jiri Slaby <jirislaby@...>
Date: Wednesday, January 30, 2008 - 7:13 am

Use pci_resource_start instead of accessing pci_dev struct internals.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
---
drivers/char/applicom.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
index 1f0b752..b0bb71b 100644
--- a/drivers/char/applicom.c
+++ b/drivers/char/applicom.c
@@ -206,22 +206,23 @@ static int __init applicom_init(void)
if (pci_enable_device(dev))
return -EIO;

- RamIO = ioremap(dev->resource[0].start, LEN_RAM_IO);
+ RamIO = ioremap(pci_resource_start(dev, 0), LEN_RAM_IO);

if (!RamIO) {
printk(KERN_INFO "ac.o: Failed to ioremap PCI memory "
"space at 0x%llx\n",
- (unsigned long long)dev->resource[0].start);
+ (unsigned long long)pci_resource_start(dev, 0));
pci_disable_device(dev);
return -EIO;
}

printk(KERN_INFO "Applicom %s found at mem 0x%llx, irq %d\n",
applicom_pci_devnames[dev->device-1],
- (unsigned long long)dev->resource[0].start,
+ (unsigned long long)pci_resource_start(dev, 0),
dev->irq);

- boardno = ac_register_board(dev->resource[0].start, RamIO,0);
+ boardno = ac_register_board(pci_resource_start(dev, 0),
+ RamIO, 0);
if (!boardno) {
printk(KERN_INFO "ac.o: PCI Applicom device doesn't have correct signature.\n");
iounmap(RamIO);
--
1.5.3.8

--

To: WANG Cong <xiyou.wangcong@...>
Cc: Sam Ravnborg <sam@...>, Jeff Garzik <jeff@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Sunday, January 27, 2008 - 5:08 am

I have probably a better fix with pci_match_id() referencing this array.
--

To: Jiri Slaby <jirislaby@...>
Cc: WANG Cong <xiyou.wangcong@...>, Sam Ravnborg <sam@...>, Jeff Garzik <jeff@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>, David Woodhouse <dwmw2@...>
Date: Sunday, January 27, 2008 - 5:21 am

Hmm, checking it one more time persuades me that the best fix ever is to convert
it to probing. David, do you have the card?
--

To: Jiri Slaby <jirislaby@...>
Cc: WANG Cong <xiyou.wangcong@...>, Sam Ravnborg <sam@...>, Jeff Garzik <jeff@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>, David Woodhouse <dwmw2@...>
Date: Monday, January 28, 2008 - 1:20 am

OK. I will drop this part.

--

To: Sam Ravnborg <sam@...>
Cc: Jeff Garzik <jeff@...>, WANG Cong <xiyou.wangcong@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 10:50 pm

Yes. The only macro we used on synclink_remove_one is __devexit.
It's defination is:

#ifdef CONFIG_HOTPLUG
#define __devexit
#else
#define __devexit __exit
#endif

And __exit is:
#ifdef MODULE
#define __exit __attribute__ ((__section__(".exit.text"))) __cold
#else
#define __exit __attribute_used__ __attribute__ ((__section__(".exit.text"))) __cold
#endif

So only when !defined(MODULE) && !defined(CONFIG_HOTPLUG),
__attribute_used__ is applied. And this case is just that

Agreed. I will try a).

--

To: Jeff Garzik <jeff@...>
Cc: WANG Cong <xiyou.wangcong@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 3:30 pm

I have reported it now - follow:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34985

Sam
--

To: Jeff Garzik <jeff@...>
Cc: WANG Cong <xiyou.wangcong@...>, Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 6:18 am

For example, the defination of __devexit_p:

#if defined(MODULE) || defined(CONFIG_HOTPLUG)
#define __devexit_p(x) x
#else
#define __devexit_p(x) NULL
#endif

If !(defined(MODULE) || defined(CONFIG_HOTPLUG)), __devexit_p
is just a NULL pointer, thus 'x' may be unused although defined.

--

To: WANG Cong <xiyou.wangcong@...>
Cc: Greg KH <gregkh@...>, LKML <linux-kernel@...>, Andrew Morton <akpm@...>
Date: Saturday, January 26, 2008 - 7:44 am

...and that's the purpose of the __devexit marker on the code itself.

Jeff

--

Previous thread: [PATCH] linux/types.h: always export 64bit aligned defines by Mike Frysinger on Saturday, January 26, 2008 - 5:23 am. (5 messages)

Next thread: Re: [PATCH 1/6] Core driver for WM97xx touchscreens by Dmitry Baryshkov on Saturday, January 26, 2008 - 5:58 am. (1 message)