[PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Richard Knutsson]

From: Richard Knutsson <ricknu-0@student.ltu.se>

Fix compiler-error on dgrs.c when CONFIG_PCI not set.

Signed-off-by: Richard Knutsson <ricknu-0@student.ltu.se>

---

 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
 
 

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Herbert Xu]

Richard Knutsson <ricknu-0@student.ltu.se> 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 <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
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);

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Richard Knutsson]

Herbert Xu wrote:

>Richard Knutsson <ricknu-0@student.ltu.se> 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 <herbert@gondor.apana.org.au>
>
>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);

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Herbert Xu]

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~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Richard Knutsson]

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 <ricknu-0@student.ltu.se>
---

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);

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Herbert Xu]

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~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Richard Knutsson]

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,

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Herbert Xu]

Richard Knutsson <ricknu-0@student.ltu.se> 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~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Richard Knutsson]

Herbert Xu wrote:

>Richard Knutsson <ricknu-0@student.ltu.se> 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

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Herbert Xu]

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~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH -mm2] net: Fix compiler-error on dgrs.c when !CONFIG_PCI

[Richard Knutsson]

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