From: Richard Knutsson <[email protected]>
Fix compiler-error on dgrs.c when CONFIG_PCI not set.
Signed-off-by: Richard Knutsson <[email protected]>
---
dgrs.c | 2 ++
1 files changed, 2 insertions(+)
diff -Narup a/drivers/net/dgrs.c b/drivers/net/dgrs.c
--- a/drivers/net/dgrs.c 2005-11-19 20:17:51.000000000 +0100
+++ b/drivers/net/dgrs.c 2005-11-19 20:29:52.000000000 +0100
@@ -1458,6 +1458,8 @@ static struct pci_driver dgrs_pci_driver
.probe = dgrs_pci_probe,
.remove = __devexit_p(dgrs_pci_remove),
};
+#else
+static struct pci_driver dgrs_pci_driver = {};
#endif
Richard Knutsson <[email protected]> wrote:
>
> diff -Narup a/drivers/net/dgrs.c b/drivers/net/dgrs.c
> --- a/drivers/net/dgrs.c 2005-11-19 20:17:51.000000000 +0100
> +++ b/drivers/net/dgrs.c 2005-11-19 20:29:52.000000000 +0100
> @@ -1458,6 +1458,8 @@ static struct pci_driver dgrs_pci_driver
> .probe = dgrs_pci_probe,
> .remove = __devexit_p(dgrs_pci_remove),
> };
> +#else
> +static struct pci_driver dgrs_pci_driver = {};
> #endif
How about something like this?
[NETDRV] dgrs: Fix compiler-error on dgrs.c when !CONFIG_PCI
The previous patch on this driver removed the #ifdef CONFIG_PCI
around the pci_register_driver call in order to avoid a warning
about an unused variable. This produces an error since the PCI
driver object is undefined if CONFIG_PCI is not set.
Instead of doing that, we should simply make sure that the
cardcount variable is always used.
The following patch does this by making inline function wrappers
to do the actual registration/deregistration.
This problem was reported by Richard Knutsson.
Signed-off-by: Herbert Xu <[email protected]>
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/drivers/net/dgrs.c b/drivers/net/dgrs.c
--- a/drivers/net/dgrs.c
+++ b/drivers/net/dgrs.c
@@ -1458,6 +1458,25 @@ static struct pci_driver dgrs_pci_driver
.probe = dgrs_pci_probe,
.remove = __devexit_p(dgrs_pci_remove),
};
+
+static inline int dgrs_register_pci(void)
+{
+ return pci_register_driver(&dgrs_pci_driver);
+}
+
+static inline void dgrs_unregister_pci(void)
+{
+ pci_unregister_driver(&dgrs_pci_driver);
+}
+#else
+static inline int dgrs_register_pci(void)
+{
+ return 0;
+}
+
+static inline int dgrs_unregister_pci(void)
+{
+}
#endif
@@ -1520,6 +1539,25 @@ static struct eisa_driver dgrs_eisa_driv
.remove = __devexit_p(dgrs_eisa_remove),
}
};
+
+static inline int dgrs_register_eisa(void)
+{
+ return eisa_driver_register(&dgrs_eisa_driver);
+}
+
+static inline void dgrs_unregister_eisa(void)
+{
+ eisa_driver_unregister(&dgrs_eisa_driver);
+}
+#else
+static inline int dgrs_register_eisa(void)
+{
+ return 0;
+}
+
+static inline void dgrs_unregister_eisa(void)
+{
+}
#endif
/*
@@ -1590,25 +1628,21 @@ static int __init dgrs_init_module (void
/*
* Find and configure all the cards
*/
-#ifdef CONFIG_EISA
- cardcount = eisa_driver_register(&dgrs_eisa_driver);
+ cardcount = dgrs_register_eisa();
if (cardcount < 0)
return cardcount;
-#endif
- cardcount = pci_register_driver(&dgrs_pci_driver);
- if (cardcount)
+ cardcount = dgrs_register_pci();
+ if (cardcount < 0) {
+ dgrs_unregister_eisa();
return cardcount;
+ }
return 0;
}
static void __exit dgrs_cleanup_module (void)
{
-#ifdef CONFIG_EISA
- eisa_driver_unregister (&dgrs_eisa_driver);
-#endif
-#ifdef CONFIG_PCI
- pci_unregister_driver (&dgrs_pci_driver);
-#endif
+ dgrs_unregister_pci();
+ dgrs_unregister_eisa();
}
module_init(dgrs_init_module);
Herbert Xu wrote:
>Richard Knutsson <[email protected]> wrote:
>
>
>>diff -Narup a/drivers/net/dgrs.c b/drivers/net/dgrs.c
>>--- a/drivers/net/dgrs.c 2005-11-19 20:17:51.000000000 +0100
>>+++ b/drivers/net/dgrs.c 2005-11-19 20:29:52.000000000 +0100
>>@@ -1458,6 +1458,8 @@ static struct pci_driver dgrs_pci_driver
>> .probe = dgrs_pci_probe,
>> .remove = __devexit_p(dgrs_pci_remove),
>>};
>>+#else
>>+static struct pci_driver dgrs_pci_driver = {};
>>#endif
>>
>>
>
>How about something like this?
>
>[NETDRV] dgrs: Fix compiler-error on dgrs.c when !CONFIG_PCI
>
>
[NETDRV], is that "standard" formating? If so, is there any more? Thanks :)
>The previous patch on this driver removed the #ifdef CONFIG_PCI
>around the pci_register_driver call in order to avoid a warning
>about an unused variable. This produces an error since the PCI
>driver object is undefined if CONFIG_PCI is not set.
>
>
Well, not really. The removal of CONFIG_PCI was just a final touch by
Andrew Morton. The only real change were from having two variables, for
eisa and pci, to having one common.
>Instead of doing that, we should simply make sure that the
>cardcount variable is always used.
>
>
Is it not? The problem were that dgrs_pci_driver was not defined if
!CONFIG_PCI, and because the pci_*-functions have empty shells if
!CONFIG_PCI, it is quite nice to use them without #ifdef.
So the simplest solution (as I saw it) was to define an empty
dgrs_pci_driver when CONFIG_PCI is not set.
On the design perspective, I would argue that struct's is more
appropriate within #ifdef's then functions.
>The following patch does this by making inline function wrappers
>to do the actual registration/deregistration.
>
>
I must say, I like that eisa gets addressed in the same manner as pci.
But is it not more appropriate to implement empty eisa-functions if
!CONFIG_EISA in linux/eisa.h (or equal)?
>This problem was reported by Richard Knutsson.
>
>Signed-off-by: Herbert Xu <[email protected]>
>
>Cheers,
>
>
Thank you for the review,
Richard Knutsson
PS
You realize you have not tushed the cardcount-"problem"? ;)
DS
> diff --git a/drivers/net/dgrs.c b/drivers/net/dgrs.c
> --- a/drivers/net/dgrs.c
> +++ b/drivers/net/dgrs.c
> @@ -1458,6 +1458,25 @@ static struct pci_driver dgrs_pci_driver
> .probe = dgrs_pci_probe,
> .remove = __devexit_p(dgrs_pci_remove),
> };
> +
> +static inline int dgrs_register_pci(void)
> +{
> + return pci_register_driver(&dgrs_pci_driver);
> +}
> +
> +static inline void dgrs_unregister_pci(void)
> +{
> + pci_unregister_driver(&dgrs_pci_driver);
> +}
> +#else
> +static inline int dgrs_register_pci(void)
> +{
> + return 0;
> +}
> +
> +static inline int dgrs_unregister_pci(void)
> +{
> +}
> #endif
>
>
> @@ -1520,6 +1539,25 @@ static struct eisa_driver dgrs_eisa_driv
> .remove = __devexit_p(dgrs_eisa_remove),
> }
> };
> +
> +static inline int dgrs_register_eisa(void)
> +{
> + return eisa_driver_register(&dgrs_eisa_driver);
> +}
> +
> +static inline void dgrs_unregister_eisa(void)
> +{
> + eisa_driver_unregister(&dgrs_eisa_driver);
> +}
> +#else
> +static inline int dgrs_register_eisa(void)
> +{
> + return 0;
> +}
> +
> +static inline void dgrs_unregister_eisa(void)
> +{
> +}
> #endif
>
> /*
> @@ -1590,25 +1628,21 @@ static int __init dgrs_init_module (void
> /*
> * Find and configure all the cards
> */
> -#ifdef CONFIG_EISA
> - cardcount = eisa_driver_register(&dgrs_eisa_driver);
> + cardcount = dgrs_register_eisa();
> if (cardcount < 0)
> return cardcount;
> -#endif
> - cardcount = pci_register_driver(&dgrs_pci_driver);
> - if (cardcount)
> + cardcount = dgrs_register_pci();
> + if (cardcount < 0) {
Are you sure it should be "cardcount < 0" and not "cardcount"?
> + dgrs_unregister_eisa();
Why change the behaviour off this driver?
> return cardcount;
> + }
> return 0;
> }
>
> static void __exit dgrs_cleanup_module (void)
> {
> -#ifdef CONFIG_EISA
> - eisa_driver_unregister (&dgrs_eisa_driver);
> -#endif
> -#ifdef CONFIG_PCI
> - pci_unregister_driver (&dgrs_pci_driver);
> -#endif
> + dgrs_unregister_pci();
> + dgrs_unregister_eisa();
> }
>
> module_init(dgrs_init_module);
On Sun, Nov 20, 2005 at 04:35:46PM +0100, Richard Knutsson wrote:
>
> >-#ifdef CONFIG_EISA
> >- cardcount = eisa_driver_register(&dgrs_eisa_driver);
> >+ cardcount = dgrs_register_eisa();
> > if (cardcount < 0)
> > return cardcount;
> >-#endif
> >- cardcount = pci_register_driver(&dgrs_pci_driver);
> >- if (cardcount)
> >+ cardcount = dgrs_register_pci();
> >+ if (cardcount < 0) {
> Are you sure it should be "cardcount < 0" and not "cardcount"?
Yes if cardcount is >= 0 then the registration was successful.
> >+ dgrs_unregister_eisa();
> Why change the behaviour off this driver?
Because the driver was buggy. When this function returns a non-zero
value, it must return the system to its original state.
That means if the EISA driver has already been registered then it must
be unregistered.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu wrote:
>On Sun, Nov 20, 2005 at 04:35:46PM +0100, Richard Knutsson wrote:
>
>
>>>-#ifdef CONFIG_EISA
>>>- cardcount = eisa_driver_register(&dgrs_eisa_driver);
>>>+ cardcount = dgrs_register_eisa();
>>> if (cardcount < 0)
>>> return cardcount;
>>>-#endif
>>>- cardcount = pci_register_driver(&dgrs_pci_driver);
>>>- if (cardcount)
>>>+ cardcount = dgrs_register_pci();
>>>+ if (cardcount < 0) {
>>>
>>>
>>Are you sure it should be "cardcount < 0" and not "cardcount"?
>>
>>
>
>Yes if cardcount is >= 0 then the registration was successful.
>
>
>
>>>+ dgrs_unregister_eisa();
>>>
>>>
>>Why change the behaviour off this driver?
>>
>>
>
>Because the driver was buggy. When this function returns a non-zero
>value, it must return the system to its original state.
>
>That means if the EISA driver has already been registered then it must
>be unregistered.
>
>Cheers,
>
>
What do you think about this patch? Will you sign it? It is no longer an
error-warning-fix but a bug-fix (and some cleanup).
I "took" you implementation of dgrs_(un)register_eisa(), especially
since eisa needed to be unregistered if pci succeeds (I take you word
for it to be so).
(BTW, this patch is compiled with CONFIG_PCI set, CONFIG_EISA set and
both set without errors/warnings for dgrs.o.)
This patch requirer the
"net-fix-compiler-error-on-dgrsc-when-config_pci.patch" (added to the
-mm tree after 2.6.15-rc1-mm2):
--- devel/drivers/net/dgrs.c~net-fix-compiler-error-on-dgrsc-when-config_pci 2005-11-19 18:00:34.000000000 -0800
+++ devel-akpm/drivers/net/dgrs.c 2005-11-19 18:00:34.000000000 -0800
@@ -1458,6 +1458,8 @@ static struct pci_driver dgrs_pci_driver
.probe = dgrs_pci_probe,
.remove = __devexit_p(dgrs_pci_remove),
};
+#else
+static struct pci_driver dgrs_pci_driver = {};
#endif
Signed-off-by: Richard Knutsson <[email protected]>
---
diff -Narup a/drivers/net/dgrs.c b/drivers/net/dgrs.c
--- a/drivers/net/dgrs.c 2005-11-21 11:25:29.000000000 +0100
+++ b/drivers/net/dgrs.c 2005-11-21 11:38:33.000000000 +0100
@@ -1522,6 +1522,26 @@ static struct eisa_driver dgrs_eisa_driv
.remove = __devexit_p(dgrs_eisa_remove),
}
};
+
+
+static inline int dgrs_register_eisa(void)
+{
+ return eisa_driver_register(&dgrs_eisa_driver);
+}
+
+static inline void dgrs_unregister_eisa(void)
+{
+ eisa_driver_unregister(&dgrs_eisa_driver);
+}
+
+#else
+
+static inline int dgrs_register_eisa(void)
+{
+ return 0;
+}
+
+static inline void dgrs_unregister_eisa(void) {}
#endif
/*
@@ -1551,7 +1571,7 @@ MODULE_PARM_DESC(nicmode, "Digi RightSwi
static int __init dgrs_init_module (void)
{
int i;
- int cardcount = 0;
+ int cardcount;
/*
* Command line variable overrides
@@ -1592,25 +1612,23 @@ static int __init dgrs_init_module (void
/*
* Find and configure all the cards
*/
-#ifdef CONFIG_EISA
- cardcount = eisa_driver_register(&dgrs_eisa_driver);
+
+ cardcount = dgrs_register_eisa();
if (cardcount < 0)
return cardcount;
-#endif
+
cardcount = pci_register_driver(&dgrs_pci_driver);
- if (cardcount)
+ if (cardcount < 0) {
+ dgrs_unregister_eisa();
return cardcount;
+ }
return 0;
}
static void __exit dgrs_cleanup_module (void)
{
-#ifdef CONFIG_EISA
- eisa_driver_unregister (&dgrs_eisa_driver);
-#endif
-#ifdef CONFIG_PCI
+ dgrs_unregister_eisa();
pci_unregister_driver (&dgrs_pci_driver);
-#endif
}
module_init(dgrs_init_module);
On Mon, Nov 21, 2005 at 01:52:49PM +0100, Richard Knutsson wrote:
>
> What do you think about this patch? Will you sign it? It is no longer an
> error-warning-fix but a bug-fix (and some cleanup).
> I "took" you implementation of dgrs_(un)register_eisa(), especially
> since eisa needed to be unregistered if pci succeeds (I take you word
> for it to be so).
> (BTW, this patch is compiled with CONFIG_PCI set, CONFIG_EISA set and
> both set without errors/warnings for dgrs.o.)
>
> This patch requirer the
> "net-fix-compiler-error-on-dgrsc-when-config_pci.patch" (added to the
> -mm tree after 2.6.15-rc1-mm2):
>
> ---
> devel/drivers/net/dgrs.c~net-fix-compiler-error-on-dgrsc-when-config_pci
> 2005-11-19 18:00:34.000000000 -0800
> +++ devel-akpm/drivers/net/dgrs.c 2005-11-19 18:00:34.000000000 -0800
> @@ -1458,6 +1458,8 @@ static struct pci_driver dgrs_pci_driver
> .probe = dgrs_pci_probe,
> .remove = __devexit_p(dgrs_pci_remove),
> };
> +#else
> +static struct pci_driver dgrs_pci_driver = {};
> #endif
I don't see the point. We shouldn't have this structure at all
if CONFIG_PCI is not set.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu wrote:
>On Mon, Nov 21, 2005 at 01:52:49PM +0100, Richard Knutsson wrote:
>
>
>>This patch requirer the
>>"net-fix-compiler-error-on-dgrsc-when-config_pci.patch" (added to the
>>-mm tree after 2.6.15-rc1-mm2):
>>
>>---
>>devel/drivers/net/dgrs.c~net-fix-compiler-error-on-dgrsc-when-config_pci
>>2005-11-19 18:00:34.000000000 -0800
>>+++ devel-akpm/drivers/net/dgrs.c 2005-11-19 18:00:34.000000000 -0800
>>@@ -1458,6 +1458,8 @@ static struct pci_driver dgrs_pci_driver
>> .probe = dgrs_pci_probe,
>> .remove = __devexit_p(dgrs_pci_remove),
>>};
>>+#else
>>+static struct pci_driver dgrs_pci_driver = {};
>>#endif
>>
>>
>
>I don't see the point. We shouldn't have this structure at all
>if CONFIG_PCI is not set.
>
>Cheers,
>
We need it even if pci_register_driver() and pci_register_driver() is
empty shells. And instead of removing #endif CONFIG_PCI for
dgrs_pci_driver, which needs dgrs_pci_tbl, dgrs_pci_probe() and
dgrs_pci_remove() (and so on), I added the empty dgrs_pci_driver-structure.
cu,
Richard Knutsson <[email protected]> wrote:
>
> We need it even if pci_register_driver() and pci_register_driver() is
> empty shells. And instead of removing #endif CONFIG_PCI for
I don't think so. Please see my previous patch where pci_register_driver
is not called at all if CONFIG_PCI is not defined.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu wrote:
>Richard Knutsson <[email protected]> wrote:
>
>
>>We need it even if pci_register_driver() and pci_register_driver() is
>>empty shells. And instead of removing #endif CONFIG_PCI for
>>
>>
>
>I don't think so. Please see my previous patch where pci_register_driver
>is not called at all if CONFIG_PCI is not defined.
>
>Cheers,
>
>
Yes, I know you patch don't need it (thought you commented the patch vs.
dgrs.c, not vs. you patch). But to do that, you have to introduce a new
dgrs-specific pci-layer to compensate. If you don't want to have an
empty struct, is it not nicer/easier to just #ifdef CONFIG_PCI the
pci_*-functions? (instead of two inline functions for every used
pci_*-function?)
Am thinking of removing the #ifdef CONFIG_PCI's in other files, to
"clean up" the code, but that would introduce this problem again, don't
you think it is more readable to make an empty struct when !CONFIG_PCI,
then making new pci_*-functions specific to the driver?
cu
On Tue, Nov 22, 2005 at 12:20:30AM +0100, Richard Knutsson wrote:
>
> Am thinking of removing the #ifdef CONFIG_PCI's in other files, to
> "clean up" the code, but that would introduce this problem again, don't
> you think it is more readable to make an empty struct when !CONFIG_PCI,
> then making new pci_*-functions specific to the driver?
I think my version is more readable. However, it's really up to Jeff
to decide.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu wrote:
>On Mon, Nov 21, 2005 at 01:52:49PM +0100, Richard Knutsson wrote:
>
>
>>This patch requirer the
>>"net-fix-compiler-error-on-dgrsc-when-config_pci.patch" (added to the
>>-mm tree after 2.6.15-rc1-mm2):
>>
>>---
>>devel/drivers/net/dgrs.c~net-fix-compiler-error-on-dgrsc-when-config_pci
>>2005-11-19 18:00:34.000000000 -0800
>>+++ devel-akpm/drivers/net/dgrs.c 2005-11-19 18:00:34.000000000 -0800
>>@@ -1458,6 +1458,8 @@ static struct pci_driver dgrs_pci_driver
>> .probe = dgrs_pci_probe,
>> .remove = __devexit_p(dgrs_pci_remove),
>>};
>>+#else
>>+static struct pci_driver dgrs_pci_driver = {};
>>#endif
>>
>>
>
>I don't see the point. We shouldn't have this structure at all
>if CONFIG_PCI is not set.
>
>Cheers,
>
>
Opps, misread your mail. Sorry.
But in that case, why shall we have any pci_*-function in the first
place when !CONFIG_PCI? As it was before, they were contained within
#ifdef CONFIG_PCI's.
You said your patch were easier to read, please elaborate. Yes, in the
dgrs_init_module() (no arguments there), but you introduces new
functions (who need to be checked out if you read the code for the first
time) and is really #ifdef's a good idea to change function behavior?
Isn't better to change the input? (I know, linux/pci.h does it, but at
least that is in a .h-file with inline functions containing at most
"return 0;").
Btw, you said Jeff should decide. Why not Rich Richardson who is the author?
Apologize for the ranting.
As I said before, thinking of deleting CONFIG_PCI's containing
pci_*-functions and if so, need a valid plan for it (because of the
pci_driver-struct). If there is any no-no in "my" way, please point it
out to spare me/you/lkml the patches. If not for this, I would've let
this rest and leave it to Jeff (or Rich ;) ).
cu,
Richard