2010-11-16 15:27:55

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH] 3c59x: fix build failure on !CONFIG_PCI

VORTEX_PCI() could return NULL so it needs to be casted before
accessing any member of struct pci_dev. This fixes following
build failure. Likewise VORTEX_EISA() was changed also.

CC [M] drivers/net/3c59x.o
drivers/net/3c59x.c: In function 'acpi_set_WOL':
drivers/net/3c59x.c:3211:39: warning: dereferencing 'void *' pointer
drivers/net/3c59x.c:3211:39: error: request for member 'current_state' in something not a structure or union
make[3]: *** [drivers/net/3c59x.o] Error 1
make[2]: *** [drivers/net/3c59x.o] Error 2
make[1]: *** [sub-make] Error 2
make: *** [all] Error 2

Signed-off-by: Namhyung Kim <[email protected]>
---
drivers/net/3c59x.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
index e1da258..0a92436 100644
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -699,7 +699,8 @@ DEFINE_WINDOW_IO(32)
#define DEVICE_PCI(dev) NULL
#endif

-#define VORTEX_PCI(vp) (((vp)->gendev) ? DEVICE_PCI((vp)->gendev) : NULL)
+#define VORTEX_PCI(vp) \
+ ((struct pci_dev *) (((vp)->gendev) ? DEVICE_PCI((vp)->gendev) : NULL))

#ifdef CONFIG_EISA
#define DEVICE_EISA(dev) (((dev)->bus == &eisa_bus_type) ? to_eisa_device((dev)) : NULL)
@@ -707,7 +708,8 @@ DEFINE_WINDOW_IO(32)
#define DEVICE_EISA(dev) NULL
#endif

-#define VORTEX_EISA(vp) (((vp)->gendev) ? DEVICE_EISA((vp)->gendev) : NULL)
+#define VORTEX_EISA(vp) \
+ ((struct eisa_device *) (((vp)->gendev) ? DEVICE_EISA((vp)->gendev) : NULL))

/* The action to take with a media selection timer tick.
Note that we deviate from the 3Com order by checking 10base2 before AUI.
--
1.7.0.4


2010-11-16 17:14:38

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH] 3c59x: fix build failure on !CONFIG_PCI

On Wed, 17 Nov 2010 00:27:51 +0900 Namhyung Kim wrote:

> VORTEX_PCI() could return NULL so it needs to be casted before
> accessing any member of struct pci_dev. This fixes following
> build failure. Likewise VORTEX_EISA() was changed also.
>
> CC [M] drivers/net/3c59x.o
> drivers/net/3c59x.c: In function 'acpi_set_WOL':
> drivers/net/3c59x.c:3211:39: warning: dereferencing 'void *' pointer
> drivers/net/3c59x.c:3211:39: error: request for member 'current_state' in something not a structure or union
> make[3]: *** [drivers/net/3c59x.o] Error 1
> make[2]: *** [drivers/net/3c59x.o] Error 2
> make[1]: *** [sub-make] Error 2
> make: *** [all] Error 2
>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> drivers/net/3c59x.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
> index e1da258..0a92436 100644
> --- a/drivers/net/3c59x.c
> +++ b/drivers/net/3c59x.c
> @@ -699,7 +699,8 @@ DEFINE_WINDOW_IO(32)
> #define DEVICE_PCI(dev) NULL
> #endif
>
> -#define VORTEX_PCI(vp) (((vp)->gendev) ? DEVICE_PCI((vp)->gendev) : NULL)
> +#define VORTEX_PCI(vp) \
> + ((struct pci_dev *) (((vp)->gendev) ? DEVICE_PCI((vp)->gendev) : NULL))
>
> #ifdef CONFIG_EISA
> #define DEVICE_EISA(dev) (((dev)->bus == &eisa_bus_type) ? to_eisa_device((dev)) : NULL)
> @@ -707,7 +708,8 @@ DEFINE_WINDOW_IO(32)
> #define DEVICE_EISA(dev) NULL
> #endif
>
> -#define VORTEX_EISA(vp) (((vp)->gendev) ? DEVICE_EISA((vp)->gendev) : NULL)
> +#define VORTEX_EISA(vp) \
> + ((struct eisa_device *) (((vp)->gendev) ? DEVICE_EISA((vp)->gendev) : NULL))
>
> /* The action to take with a media selection timer tick.
> Note that we deviate from the 3Com order by checking 10base2 before AUI.
> --

Hi,

Interesting patch. I have reported this build error and looked
into fixing it, but did not come up with this solution.

Looking at it more: if CONFIG_PCI is not enabled, DEVICE_PCI() is NULL.
That makes VORTEX_PCI() (with or without your patch) have a value of NULL.

Is the line with the reported syntax error (3211) executed in
function acpi_set_WOL() ? If so, let's assume that vp->enable_wol is true.
Then what happens on line 3211 (or 3213 after your patch)?

if (VORTEX_PCI(vp)->current_state < PCI_D3hot)
return;

or if I am really confuzed this morning, please tell me how it works.

thanks,
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

2010-11-17 02:22:22

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH] 3c59x: fix build failure on !CONFIG_PCI

2010-11-16 (화), 09:14 -0800, Randy Dunlap:
> Hi,
>

Hi, Randy


> Interesting patch. I have reported this build error and looked
> into fixing it, but did not come up with this solution.
>
> Looking at it more: if CONFIG_PCI is not enabled, DEVICE_PCI() is NULL.
> That makes VORTEX_PCI() (with or without your patch) have a value of NULL.
>
> Is the line with the reported syntax error (3211) executed in
> function acpi_set_WOL() ? If so, let's assume that vp->enable_wol is true.
> Then what happens on line 3211 (or 3213 after your patch)?
>
> if (VORTEX_PCI(vp)->current_state < PCI_D3hot)
> return;
>
> or if I am really confuzed this morning, please tell me how it works.
>

At first glance, I've noticed that the code above could make a NULL
dereference so I added NULL check prior to the line.

But after reading more code I realized that other pci-functions called
in acpi_set_WOL() would not work with NULL pci_dev pointer also. And I
found all callers of the acpi_set_WOL() already checked NULL pointer
before the call. Finally I could remove the NULL check and leave the
code as is. That's how it works. :)

Thanks.

--
Regards,
Namhyung Kim

2010-11-17 15:59:49

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH] 3c59x: fix build failure on !CONFIG_PCI

On 11/16/10 18:22, Namhyung Kim wrote:
> 2010-11-16 (화), 09:14 -0800, Randy Dunlap:
>> Hi,
>>
>
> Hi, Randy
>
>
>> Interesting patch. I have reported this build error and looked
>> into fixing it, but did not come up with this solution.
>>
>> Looking at it more: if CONFIG_PCI is not enabled, DEVICE_PCI() is NULL.
>> That makes VORTEX_PCI() (with or without your patch) have a value of NULL.
>>
>> Is the line with the reported syntax error (3211) executed in
>> function acpi_set_WOL() ? If so, let's assume that vp->enable_wol is true.
>> Then what happens on line 3211 (or 3213 after your patch)?
>>
>> if (VORTEX_PCI(vp)->current_state < PCI_D3hot)
>> return;
>>
>> or if I am really confuzed this morning, please tell me how it works.
>>
>
> At first glance, I've noticed that the code above could make a NULL
> dereference so I added NULL check prior to the line.
>
> But after reading more code I realized that other pci-functions called
> in acpi_set_WOL() would not work with NULL pci_dev pointer also. And I
> found all callers of the acpi_set_WOL() already checked NULL pointer
> before the call. Finally I could remove the NULL check and leave the
> code as is. That's how it works. :)

I see. and concur. Thanks for the explanation.

--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

2010-11-18 18:47:36

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] 3c59x: fix build failure on !CONFIG_PCI

From: Randy Dunlap <[email protected]>
Date: Wed, 17 Nov 2010 07:58:36 -0800

> On 11/16/10 18:22, Namhyung Kim wrote:
>> 2010-11-16 (ȭ), 09:14 -0800, Randy Dunlap:
>>> Hi,
>>>
>>
>> Hi, Randy
>>
>>
>>> Interesting patch. I have reported this build error and looked
>>> into fixing it, but did not come up with this solution.
>>>
>>> Looking at it more: if CONFIG_PCI is not enabled, DEVICE_PCI() is NULL.
>>> That makes VORTEX_PCI() (with or without your patch) have a value of NULL.
>>>
>>> Is the line with the reported syntax error (3211) executed in
>>> function acpi_set_WOL() ? If so, let's assume that vp->enable_wol is true.
>>> Then what happens on line 3211 (or 3213 after your patch)?
>>>
>>> if (VORTEX_PCI(vp)->current_state < PCI_D3hot)
>>> return;
>>>
>>> or if I am really confuzed this morning, please tell me how it works.
>>>
>>
>> At first glance, I've noticed that the code above could make a NULL
>> dereference so I added NULL check prior to the line.
>>
>> But after reading more code I realized that other pci-functions called
>> in acpi_set_WOL() would not work with NULL pci_dev pointer also. And I
>> found all callers of the acpi_set_WOL() already checked NULL pointer
>> before the call. Finally I could remove the NULL check and leave the
>> code as is. That's how it works. :)
>
> I see. and concur. Thanks for the explanation.

Looks good to me too, applied, thanks!