diff mbox series

s390/net: lcs: use IS_ENABLED() for kconfig detection

Message ID 20230615222152.13250-1-rdunlap@infradead.org
State New
Headers
Series s390/net: lcs: use IS_ENABLED() for kconfig detection |

Commit Message

Randy Dunlap June 15, 2023, 10:21 p.m. UTC 
  When CONFIG_ETHERNET=m or CONFIG_FDDI=m, lcs.s has build errors or
warnings:

../drivers/s390/net/lcs.c:40:2: error: #error Cannot compile lcs.c without some net devices switched on.
   40 | #error Cannot compile lcs.c without some net devices switched on.
../drivers/s390/net/lcs.c: In function 'lcs_startlan_auto':
../drivers/s390/net/lcs.c:1601:13: warning: unused variable 'rc' [-Wunused-variable]
 1601 |         int rc;

Solve this by using IS_ENABLED(CONFIG_symbol) instead of ifdef
CONFIG_symbol. The latter only works for builtin (=y) values
while IS_ENABLED() works for builtin or modular values.

Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Alexandra Winter <wintera@linux.ibm.com>
Cc: Wenjia Zhang <wenjia@linux.ibm.com>
Cc: linux-s390@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
---
 drivers/s390/net/lcs.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org June 16, 2023, 8:10 a.m. UTC | #1 
Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 15 Jun 2023 15:21:52 -0700 you wrote:
> When CONFIG_ETHERNET=m or CONFIG_FDDI=m, lcs.s has build errors or
> warnings:
> 
> ../drivers/s390/net/lcs.c:40:2: error: #error Cannot compile lcs.c without some net devices switched on.
>    40 | #error Cannot compile lcs.c without some net devices switched on.
> ../drivers/s390/net/lcs.c: In function 'lcs_startlan_auto':
> ../drivers/s390/net/lcs.c:1601:13: warning: unused variable 'rc' [-Wunused-variable]
>  1601 |         int rc;
> 
> [...]

Here is the summary with links:
  - s390/net: lcs: use IS_ENABLED() for kconfig detection
    https://git.kernel.org/netdev/net-next/c/128272336120

You are awesome, thank you!
Alexandra Winter June 19, 2023, 10:04 a.m. UTC | #2 
On 16.06.23 10:10, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This patch was applied to netdev/net-next.git (main)
> by David S. Miller <davem@davemloft.net>:
> 
> On Thu, 15 Jun 2023 15:21:52 -0700 you wrote:
>> When CONFIG_ETHERNET=m or CONFIG_FDDI=m, lcs.s has build errors or
>> warnings:
>>
 
IIUC, CONFIG_ETHERNET is bool.
I reproduced this with CONFIG_ETHERNET=n and CONFIG_FDDI=m,
and verified that it does compile with your patch.

Thank you Randy for correcting this.

>> ../drivers/s390/net/lcs.c:40:2: error: #error Cannot compile lcs.c without some net devices switched on.
>>    40 | #error Cannot compile lcs.c without some net devices switched on.
>> ../drivers/s390/net/lcs.c: In function 'lcs_startlan_auto':
>> ../drivers/s390/net/lcs.c:1601:13: warning: unused variable 'rc' [-Wunused-variable]
>>  1601 |         int rc;
>>
>> [...]
> 
> Here is the summary with links:
>   - s390/net: lcs: use IS_ENABLED() for kconfig detection
>     https://git.kernel.org/netdev/net-next/c/128272336120
> 
> You are awesome, thank you!
Randy Dunlap June 19, 2023, 3:18 p.m. UTC | #3 
On 6/19/23 03:04, Alexandra Winter wrote:
> 
> 
> On 16.06.23 10:10, patchwork-bot+netdevbpf@kernel.org wrote:
>> Hello:
>>
>> This patch was applied to netdev/net-next.git (main)
>> by David S. Miller <davem@davemloft.net>:
>>
>> On Thu, 15 Jun 2023 15:21:52 -0700 you wrote:
>>> When CONFIG_ETHERNET=m or CONFIG_FDDI=m, lcs.s has build errors or
>>> warnings:
>>>
>  
> IIUC, CONFIG_ETHERNET is bool.

duh, yes. Thanks.

> I reproduced this with CONFIG_ETHERNET=n and CONFIG_FDDI=m,
> and verified that it does compile with your patch.
> 
> Thank you Randy for correcting this.
> 
>>> ../drivers/s390/net/lcs.c:40:2: error: #error Cannot compile lcs.c without some net devices switched on.
>>>    40 | #error Cannot compile lcs.c without some net devices switched on.
>>> ../drivers/s390/net/lcs.c: In function 'lcs_startlan_auto':
>>> ../drivers/s390/net/lcs.c:1601:13: warning: unused variable 'rc' [-Wunused-variable]
>>>  1601 |         int rc;
>>>
>>> [...]
>>
>> Here is the summary with links:
>>   - s390/net: lcs: use IS_ENABLED() for kconfig detection
>>     https://git.kernel.org/netdev/net-next/c/128272336120
>>
>> You are awesome, thank you!
>
Randy Dunlap June 21, 2023, 2:35 a.m. UTC | #4 
Hi,

On 6/15/23 15:21, Randy Dunlap wrote:
> When CONFIG_ETHERNET=m or CONFIG_FDDI=m, lcs.s has build errors or
> warnings:
> 
> ../drivers/s390/net/lcs.c:40:2: error: #error Cannot compile lcs.c without some net devices switched on.
>    40 | #error Cannot compile lcs.c without some net devices switched on.
> ../drivers/s390/net/lcs.c: In function 'lcs_startlan_auto':
> ../drivers/s390/net/lcs.c:1601:13: warning: unused variable 'rc' [-Wunused-variable]
>  1601 |         int rc;
> 
> Solve this by using IS_ENABLED(CONFIG_symbol) instead of ifdef
> CONFIG_symbol. The latter only works for builtin (=y) values
> while IS_ENABLED() works for builtin or modular values.
> 
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: Alexandra Winter <wintera@linux.ibm.com>
> Cc: Wenjia Zhang <wenjia@linux.ibm.com>
> Cc: linux-s390@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> ---
>  drivers/s390/net/lcs.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff -- a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
> --- a/drivers/s390/net/lcs.c
> +++ b/drivers/s390/net/lcs.c
> @@ -36,7 +36,7 @@
>  #include "lcs.h"
>  
>  
> -#if !defined(CONFIG_ETHERNET) && !defined(CONFIG_FDDI)
> +#if !IS_ENABLED(CONFIG_ETHERNET) && !IS_ENABLED(CONFIG_FDDI)
>  #error Cannot compile lcs.c without some net devices switched on.
>  #endif
>  
> @@ -1601,14 +1601,14 @@ lcs_startlan_auto(struct lcs_card *card)
>  	int rc;
>  
>  	LCS_DBF_TEXT(2, trace, "strtauto");
> -#ifdef CONFIG_ETHERNET
> +#if IS_ENABLED(CONFIG_ETHERNET)
>  	card->lan_type = LCS_FRAME_TYPE_ENET;
>  	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
>  	if (rc == 0)
>  		return 0;
>  
>  #endif
> -#ifdef CONFIG_FDDI
> +#if IS_ENABLED(CONFIG_FDDI)
>  	card->lan_type = LCS_FRAME_TYPE_FDDI;
>  	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
>  	if (rc == 0)
> @@ -2139,13 +2139,13 @@ lcs_new_device(struct ccwgroup_device *c
>  		goto netdev_out;
>  	}
>  	switch (card->lan_type) {
> -#ifdef CONFIG_ETHERNET
> +#if IS_ENABLED(CONFIG_ETHERNET)
>  	case LCS_FRAME_TYPE_ENET:
>  		card->lan_type_trans = eth_type_trans;
>  		dev = alloc_etherdev(0);
>  		break;
>  #endif
> -#ifdef CONFIG_FDDI
> +#if IS_ENABLED(CONFIG_FDDI)
>  	case LCS_FRAME_TYPE_FDDI:
>  		card->lan_type_trans = fddi_type_trans;
>  		dev = alloc_fddidev(0);


kernel test robot reports build errors from this patch when
ETHERNET=y, FDDI=m, LCS=y:

  https://lore.kernel.org/all/202306202129.pl0AqK8G-lkp@intel.com/

Since the code before my patch expected (supported) FDDI=y only
(by checking for CONFIG_FDDI only and not checking for CONFIG_FDDI_MODULE),
the best solution that I can see is to enforce that expectation in
drivers/s390/net/Kconfig:

diff -- a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
--- a/drivers/s390/net/Kconfig
+++ b/drivers/s390/net/Kconfig
@@ -5,7 +5,7 @@ menu "S/390 network device drivers"
 config LCS
 	def_tristate m
 	prompt "Lan Channel Station Interface"
-	depends on CCW && NETDEVICES && (ETHERNET || FDDI)
+	depends on CCW && NETDEVICES && (ETHERNET || FDDI = y)
 	help
 	  Select this option if you want to use LCS networking on IBM System z.
 	  This device driver supports FDDI (IEEE 802.7) and Ethernet.

What do people think of that change?
Any other ideas/suggestions?

thanks.
Simon Horman June 21, 2023, 3:53 p.m. UTC | #5 
On Tue, Jun 20, 2023 at 07:35:17PM -0700, Randy Dunlap wrote:
> Hi,
> 
> On 6/15/23 15:21, Randy Dunlap wrote:
> > When CONFIG_ETHERNET=m or CONFIG_FDDI=m, lcs.s has build errors or
> > warnings:
> > 
> > ../drivers/s390/net/lcs.c:40:2: error: #error Cannot compile lcs.c without some net devices switched on.
> >    40 | #error Cannot compile lcs.c without some net devices switched on.
> > ../drivers/s390/net/lcs.c: In function 'lcs_startlan_auto':
> > ../drivers/s390/net/lcs.c:1601:13: warning: unused variable 'rc' [-Wunused-variable]
> >  1601 |         int rc;
> > 
> > Solve this by using IS_ENABLED(CONFIG_symbol) instead of ifdef
> > CONFIG_symbol. The latter only works for builtin (=y) values
> > while IS_ENABLED() works for builtin or modular values.
> > 
> > Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Alexandra Winter <wintera@linux.ibm.com>
> > Cc: Wenjia Zhang <wenjia@linux.ibm.com>
> > Cc: linux-s390@vger.kernel.org
> > Cc: netdev@vger.kernel.org
> > Cc: Heiko Carstens <hca@linux.ibm.com>
> > Cc: Vasily Gorbik <gor@linux.ibm.com>
> > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> > Cc: Sven Schnelle <svens@linux.ibm.com>
> > ---
> >  drivers/s390/net/lcs.c |   10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff -- a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
> > --- a/drivers/s390/net/lcs.c
> > +++ b/drivers/s390/net/lcs.c
> > @@ -36,7 +36,7 @@
> >  #include "lcs.h"
> >  
> >  
> > -#if !defined(CONFIG_ETHERNET) && !defined(CONFIG_FDDI)
> > +#if !IS_ENABLED(CONFIG_ETHERNET) && !IS_ENABLED(CONFIG_FDDI)
> >  #error Cannot compile lcs.c without some net devices switched on.
> >  #endif
> >  
> > @@ -1601,14 +1601,14 @@ lcs_startlan_auto(struct lcs_card *card)
> >  	int rc;
> >  
> >  	LCS_DBF_TEXT(2, trace, "strtauto");
> > -#ifdef CONFIG_ETHERNET
> > +#if IS_ENABLED(CONFIG_ETHERNET)
> >  	card->lan_type = LCS_FRAME_TYPE_ENET;
> >  	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
> >  	if (rc == 0)
> >  		return 0;
> >  
> >  #endif
> > -#ifdef CONFIG_FDDI
> > +#if IS_ENABLED(CONFIG_FDDI)
> >  	card->lan_type = LCS_FRAME_TYPE_FDDI;
> >  	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
> >  	if (rc == 0)
> > @@ -2139,13 +2139,13 @@ lcs_new_device(struct ccwgroup_device *c
> >  		goto netdev_out;
> >  	}
> >  	switch (card->lan_type) {
> > -#ifdef CONFIG_ETHERNET
> > +#if IS_ENABLED(CONFIG_ETHERNET)
> >  	case LCS_FRAME_TYPE_ENET:
> >  		card->lan_type_trans = eth_type_trans;
> >  		dev = alloc_etherdev(0);
> >  		break;
> >  #endif
> > -#ifdef CONFIG_FDDI
> > +#if IS_ENABLED(CONFIG_FDDI)
> >  	case LCS_FRAME_TYPE_FDDI:
> >  		card->lan_type_trans = fddi_type_trans;
> >  		dev = alloc_fddidev(0);
> 
> 
> kernel test robot reports build errors from this patch when
> ETHERNET=y, FDDI=m, LCS=y:
> 
>   https://lore.kernel.org/all/202306202129.pl0AqK8G-lkp@intel.com/
> 
> Since the code before my patch expected (supported) FDDI=y only
> (by checking for CONFIG_FDDI only and not checking for CONFIG_FDDI_MODULE),
> the best solution that I can see is to enforce that expectation in
> drivers/s390/net/Kconfig:
> 
> diff -- a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
> --- a/drivers/s390/net/Kconfig
> +++ b/drivers/s390/net/Kconfig
> @@ -5,7 +5,7 @@ menu "S/390 network device drivers"
>  config LCS
>  	def_tristate m
>  	prompt "Lan Channel Station Interface"
> -	depends on CCW && NETDEVICES && (ETHERNET || FDDI)
> +	depends on CCW && NETDEVICES && (ETHERNET || FDDI = y)

Hi Randy,

Unfortunately I don't think this helps.
In the config given at the link above, ETHERNET is y.
And the error regarding fddi_type_trans and alloc_fddidev being undefined
seems to occur regardless of your change.

I did have better luck with this.

diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
index 9c67b97faba2..303220251495 100644
--- a/drivers/s390/net/Kconfig
+++ b/drivers/s390/net/Kconfig
@@ -6,6 +6,7 @@ config LCS
        def_tristate m
        prompt "Lan Channel Station Interface"
        depends on CCW && NETDEVICES && (ETHERNET || FDDI)
+       depends on FDDI=y || FDDI=n
        help
          Select this option if you want to use LCS networking on IBM System z.
          This device driver supports FDDI (IEEE 802.7) and Ethernet.

I am assuming that LCS=m and FDDI=m can't work at runtime
because there is no guarantee that FDDI is loaded before LCS.
But I could well be wrong here.

>  	help
>  	  Select this option if you want to use LCS networking on IBM System z.
>  	  This device driver supports FDDI (IEEE 802.7) and Ethernet.
> 
> What do people think of that change?
> Any other ideas/suggestions?
> 
> thanks.
> -- 
> ~Randy
>
Randy Dunlap June 21, 2023, 6:08 p.m. UTC | #6 
Hi Simon,


On 6/21/23 08:53, Simon Horman wrote:
> On Tue, Jun 20, 2023 at 07:35:17PM -0700, Randy Dunlap wrote:
>> Hi,
>>
>> On 6/15/23 15:21, Randy Dunlap wrote:
>>> When CONFIG_ETHERNET=m or CONFIG_FDDI=m, lcs.s has build errors or
>>> warnings:
>>>
>>> ../drivers/s390/net/lcs.c:40:2: error: #error Cannot compile lcs.c without some net devices switched on.
>>>    40 | #error Cannot compile lcs.c without some net devices switched on.
>>> ../drivers/s390/net/lcs.c: In function 'lcs_startlan_auto':
>>> ../drivers/s390/net/lcs.c:1601:13: warning: unused variable 'rc' [-Wunused-variable]
>>>  1601 |         int rc;
>>>
>>> Solve this by using IS_ENABLED(CONFIG_symbol) instead of ifdef
>>> CONFIG_symbol. The latter only works for builtin (=y) values
>>> while IS_ENABLED() works for builtin or modular values.
>>>
>>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>>> Cc: Alexandra Winter <wintera@linux.ibm.com>
>>> Cc: Wenjia Zhang <wenjia@linux.ibm.com>
>>> Cc: linux-s390@vger.kernel.org
>>> Cc: netdev@vger.kernel.org
>>> Cc: Heiko Carstens <hca@linux.ibm.com>
>>> Cc: Vasily Gorbik <gor@linux.ibm.com>
>>> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
>>> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
>>> Cc: Sven Schnelle <svens@linux.ibm.com>
>>> ---
>>>  drivers/s390/net/lcs.c |   10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff -- a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
>>> --- a/drivers/s390/net/lcs.c
>>> +++ b/drivers/s390/net/lcs.c
>>> @@ -36,7 +36,7 @@
>>>  #include "lcs.h"
>>>  
>>>  
>>> -#if !defined(CONFIG_ETHERNET) && !defined(CONFIG_FDDI)
>>> +#if !IS_ENABLED(CONFIG_ETHERNET) && !IS_ENABLED(CONFIG_FDDI)
>>>  #error Cannot compile lcs.c without some net devices switched on.
>>>  #endif
>>>  
>>> @@ -1601,14 +1601,14 @@ lcs_startlan_auto(struct lcs_card *card)
>>>  	int rc;
>>>  
>>>  	LCS_DBF_TEXT(2, trace, "strtauto");
>>> -#ifdef CONFIG_ETHERNET
>>> +#if IS_ENABLED(CONFIG_ETHERNET)
>>>  	card->lan_type = LCS_FRAME_TYPE_ENET;
>>>  	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
>>>  	if (rc == 0)
>>>  		return 0;
>>>  
>>>  #endif
>>> -#ifdef CONFIG_FDDI
>>> +#if IS_ENABLED(CONFIG_FDDI)
>>>  	card->lan_type = LCS_FRAME_TYPE_FDDI;
>>>  	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
>>>  	if (rc == 0)
>>> @@ -2139,13 +2139,13 @@ lcs_new_device(struct ccwgroup_device *c
>>>  		goto netdev_out;
>>>  	}
>>>  	switch (card->lan_type) {
>>> -#ifdef CONFIG_ETHERNET
>>> +#if IS_ENABLED(CONFIG_ETHERNET)
>>>  	case LCS_FRAME_TYPE_ENET:
>>>  		card->lan_type_trans = eth_type_trans;
>>>  		dev = alloc_etherdev(0);
>>>  		break;
>>>  #endif
>>> -#ifdef CONFIG_FDDI
>>> +#if IS_ENABLED(CONFIG_FDDI)
>>>  	case LCS_FRAME_TYPE_FDDI:
>>>  		card->lan_type_trans = fddi_type_trans;
>>>  		dev = alloc_fddidev(0);
>>
>>
>> kernel test robot reports build errors from this patch when
>> ETHERNET=y, FDDI=m, LCS=y:
>>
>>   https://lore.kernel.org/all/202306202129.pl0AqK8G-lkp@intel.com/
>>
>> Since the code before my patch expected (supported) FDDI=y only
>> (by checking for CONFIG_FDDI only and not checking for CONFIG_FDDI_MODULE),
>> the best solution that I can see is to enforce that expectation in
>> drivers/s390/net/Kconfig:
>>
>> diff -- a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
>> --- a/drivers/s390/net/Kconfig
>> +++ b/drivers/s390/net/Kconfig
>> @@ -5,7 +5,7 @@ menu "S/390 network device drivers"
>>  config LCS
>>  	def_tristate m
>>  	prompt "Lan Channel Station Interface"
>> -	depends on CCW && NETDEVICES && (ETHERNET || FDDI)
>> +	depends on CCW && NETDEVICES && (ETHERNET || FDDI = y)
> 
> Hi Randy,
> 
> Unfortunately I don't think this helps.
> In the config given at the link above, ETHERNET is y.
> And the error regarding fddi_type_trans and alloc_fddidev being undefined
> seems to occur regardless of your change.

Hmph, somehow I missed that. :(

> I did have better luck with this.
> 
> diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
> index 9c67b97faba2..303220251495 100644
> --- a/drivers/s390/net/Kconfig
> +++ b/drivers/s390/net/Kconfig
> @@ -6,6 +6,7 @@ config LCS
>         def_tristate m
>         prompt "Lan Channel Station Interface"
>         depends on CCW && NETDEVICES && (ETHERNET || FDDI)
> +       depends on FDDI=y || FDDI=n
>         help
>           Select this option if you want to use LCS networking on IBM System z.
>           This device driver supports FDDI (IEEE 802.7) and Ethernet.
> 
> I am assuming that LCS=m and FDDI=m can't work at runtime
> because there is no guarantee that FDDI is loaded before LCS.
> But I could well be wrong here.

There's probably some way to make that work, but I don't know.

I think that your patch is acceptable.
I would prefer to also add to the help text that if FDDI is used,
it must be builtin (=y).

>>  	help
>>  	  Select this option if you want to use LCS networking on IBM System z.
>>  	  This device driver supports FDDI (IEEE 802.7) and Ethernet.
>>
>> What do people think of that change?
>> Any other ideas/suggestions?
>>
>> thanks.
>> -- 
>> ~Randy
>>

Thanks for your help.
Simon Horman June 21, 2023, 7:37 p.m. UTC | #7 
On Wed, Jun 21, 2023 at 11:08:05AM -0700, Randy Dunlap wrote:
> Hi Simon,
> 
> 
> On 6/21/23 08:53, Simon Horman wrote:
> > On Tue, Jun 20, 2023 at 07:35:17PM -0700, Randy Dunlap wrote:
> >> Hi,
> >>
> >> On 6/15/23 15:21, Randy Dunlap wrote:
> >>> When CONFIG_ETHERNET=m or CONFIG_FDDI=m, lcs.s has build errors or
> >>> warnings:
> >>>
> >>> ../drivers/s390/net/lcs.c:40:2: error: #error Cannot compile lcs.c without some net devices switched on.
> >>>    40 | #error Cannot compile lcs.c without some net devices switched on.
> >>> ../drivers/s390/net/lcs.c: In function 'lcs_startlan_auto':
> >>> ../drivers/s390/net/lcs.c:1601:13: warning: unused variable 'rc' [-Wunused-variable]
> >>>  1601 |         int rc;
> >>>
> >>> Solve this by using IS_ENABLED(CONFIG_symbol) instead of ifdef
> >>> CONFIG_symbol. The latter only works for builtin (=y) values
> >>> while IS_ENABLED() works for builtin or modular values.
> >>>
> >>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> >>> Cc: Alexandra Winter <wintera@linux.ibm.com>
> >>> Cc: Wenjia Zhang <wenjia@linux.ibm.com>
> >>> Cc: linux-s390@vger.kernel.org
> >>> Cc: netdev@vger.kernel.org
> >>> Cc: Heiko Carstens <hca@linux.ibm.com>
> >>> Cc: Vasily Gorbik <gor@linux.ibm.com>
> >>> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> >>> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> >>> Cc: Sven Schnelle <svens@linux.ibm.com>
> >>> ---
> >>>  drivers/s390/net/lcs.c |   10 +++++-----
> >>>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff -- a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
> >>> --- a/drivers/s390/net/lcs.c
> >>> +++ b/drivers/s390/net/lcs.c
> >>> @@ -36,7 +36,7 @@
> >>>  #include "lcs.h"
> >>>  
> >>>  
> >>> -#if !defined(CONFIG_ETHERNET) && !defined(CONFIG_FDDI)
> >>> +#if !IS_ENABLED(CONFIG_ETHERNET) && !IS_ENABLED(CONFIG_FDDI)
> >>>  #error Cannot compile lcs.c without some net devices switched on.
> >>>  #endif
> >>>  
> >>> @@ -1601,14 +1601,14 @@ lcs_startlan_auto(struct lcs_card *card)
> >>>  	int rc;
> >>>  
> >>>  	LCS_DBF_TEXT(2, trace, "strtauto");
> >>> -#ifdef CONFIG_ETHERNET
> >>> +#if IS_ENABLED(CONFIG_ETHERNET)
> >>>  	card->lan_type = LCS_FRAME_TYPE_ENET;
> >>>  	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
> >>>  	if (rc == 0)
> >>>  		return 0;
> >>>  
> >>>  #endif
> >>> -#ifdef CONFIG_FDDI
> >>> +#if IS_ENABLED(CONFIG_FDDI)
> >>>  	card->lan_type = LCS_FRAME_TYPE_FDDI;
> >>>  	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
> >>>  	if (rc == 0)
> >>> @@ -2139,13 +2139,13 @@ lcs_new_device(struct ccwgroup_device *c
> >>>  		goto netdev_out;
> >>>  	}
> >>>  	switch (card->lan_type) {
> >>> -#ifdef CONFIG_ETHERNET
> >>> +#if IS_ENABLED(CONFIG_ETHERNET)
> >>>  	case LCS_FRAME_TYPE_ENET:
> >>>  		card->lan_type_trans = eth_type_trans;
> >>>  		dev = alloc_etherdev(0);
> >>>  		break;
> >>>  #endif
> >>> -#ifdef CONFIG_FDDI
> >>> +#if IS_ENABLED(CONFIG_FDDI)
> >>>  	case LCS_FRAME_TYPE_FDDI:
> >>>  		card->lan_type_trans = fddi_type_trans;
> >>>  		dev = alloc_fddidev(0);
> >>
> >>
> >> kernel test robot reports build errors from this patch when
> >> ETHERNET=y, FDDI=m, LCS=y:
> >>
> >>   https://lore.kernel.org/all/202306202129.pl0AqK8G-lkp@intel.com/
> >>
> >> Since the code before my patch expected (supported) FDDI=y only
> >> (by checking for CONFIG_FDDI only and not checking for CONFIG_FDDI_MODULE),
> >> the best solution that I can see is to enforce that expectation in
> >> drivers/s390/net/Kconfig:
> >>
> >> diff -- a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
> >> --- a/drivers/s390/net/Kconfig
> >> +++ b/drivers/s390/net/Kconfig
> >> @@ -5,7 +5,7 @@ menu "S/390 network device drivers"
> >>  config LCS
> >>  	def_tristate m
> >>  	prompt "Lan Channel Station Interface"
> >> -	depends on CCW && NETDEVICES && (ETHERNET || FDDI)
> >> +	depends on CCW && NETDEVICES && (ETHERNET || FDDI = y)
> > 
> > Hi Randy,
> > 
> > Unfortunately I don't think this helps.
> > In the config given at the link above, ETHERNET is y.
> > And the error regarding fddi_type_trans and alloc_fddidev being undefined
> > seems to occur regardless of your change.
> 
> Hmph, somehow I missed that. :(
> 
> > I did have better luck with this.
> > 
> > diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
> > index 9c67b97faba2..303220251495 100644
> > --- a/drivers/s390/net/Kconfig
> > +++ b/drivers/s390/net/Kconfig
> > @@ -6,6 +6,7 @@ config LCS
> >         def_tristate m
> >         prompt "Lan Channel Station Interface"
> >         depends on CCW && NETDEVICES && (ETHERNET || FDDI)
> > +       depends on FDDI=y || FDDI=n
> >         help
> >           Select this option if you want to use LCS networking on IBM System z.
> >           This device driver supports FDDI (IEEE 802.7) and Ethernet.
> > 
> > I am assuming that LCS=m and FDDI=m can't work at runtime
> > because there is no guarantee that FDDI is loaded before LCS.
> > But I could well be wrong here.
> 
> There's probably some way to make that work, but I don't know.
> 
> I think that your patch is acceptable.
> I would prefer to also add to the help text that if FDDI is used,
> it must be builtin (=y).

Thanks Randy,

Feel free to take the snippet above and work it into a proper patch.
Else I can take a shot at it.

> >>  	help
> >>  	  Select this option if you want to use LCS networking on IBM System z.
> >>  	  This device driver supports FDDI (IEEE 802.7) and Ethernet.
> >>
> >> What do people think of that change?
> >> Any other ideas/suggestions?
> >>
> >> thanks.
> >> -- 
> >> ~Randy
> >>
> 
> Thanks for your help.
> -- 
> ~Randy
Randy Dunlap June 21, 2023, 9:19 p.m. UTC | #8 
On 6/21/23 12:37, Simon Horman wrote:
> On Wed, Jun 21, 2023 at 11:08:05AM -0700, Randy Dunlap wrote:
>> Hi Simon,
>>

>>> I did have better luck with this.
>>>
>>> diff --git a/drivers/s390/net/Kconfig b/drivers/s390/net/Kconfig
>>> index 9c67b97faba2..303220251495 100644
>>> --- a/drivers/s390/net/Kconfig
>>> +++ b/drivers/s390/net/Kconfig
>>> @@ -6,6 +6,7 @@ config LCS
>>>         def_tristate m
>>>         prompt "Lan Channel Station Interface"
>>>         depends on CCW && NETDEVICES && (ETHERNET || FDDI)
>>> +       depends on FDDI=y || FDDI=n
>>>         help
>>>           Select this option if you want to use LCS networking on IBM System z.
>>>           This device driver supports FDDI (IEEE 802.7) and Ethernet.
>>>
>>> I am assuming that LCS=m and FDDI=m can't work at runtime
>>> because there is no guarantee that FDDI is loaded before LCS.
>>> But I could well be wrong here.
>>
>> There's probably some way to make that work, but I don't know.
>>
>> I think that your patch is acceptable.
>> I would prefer to also add to the help text that if FDDI is used,
>> it must be builtin (=y).
> 
> Thanks Randy,
> 
> Feel free to take the snippet above and work it into a proper patch.
> Else I can take a shot at it.
> 

OK, I'll send that. Thanks.
diff mbox series

Patch

diff -- a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -36,7 +36,7 @@ 
 #include "lcs.h"
 
 
-#if !defined(CONFIG_ETHERNET) && !defined(CONFIG_FDDI)
+#if !IS_ENABLED(CONFIG_ETHERNET) && !IS_ENABLED(CONFIG_FDDI)
 #error Cannot compile lcs.c without some net devices switched on.
 #endif
 
@@ -1601,14 +1601,14 @@  lcs_startlan_auto(struct lcs_card *card)
 	int rc;
 
 	LCS_DBF_TEXT(2, trace, "strtauto");
-#ifdef CONFIG_ETHERNET
+#if IS_ENABLED(CONFIG_ETHERNET)
 	card->lan_type = LCS_FRAME_TYPE_ENET;
 	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
 	if (rc == 0)
 		return 0;
 
 #endif
-#ifdef CONFIG_FDDI
+#if IS_ENABLED(CONFIG_FDDI)
 	card->lan_type = LCS_FRAME_TYPE_FDDI;
 	rc = lcs_send_startlan(card, LCS_INITIATOR_TCPIP);
 	if (rc == 0)
@@ -2139,13 +2139,13 @@  lcs_new_device(struct ccwgroup_device *c
 		goto netdev_out;
 	}
 	switch (card->lan_type) {
-#ifdef CONFIG_ETHERNET
+#if IS_ENABLED(CONFIG_ETHERNET)
 	case LCS_FRAME_TYPE_ENET:
 		card->lan_type_trans = eth_type_trans;
 		dev = alloc_etherdev(0);
 		break;
 #endif
-#ifdef CONFIG_FDDI
+#if IS_ENABLED(CONFIG_FDDI)
 	case LCS_FRAME_TYPE_FDDI:
 		card->lan_type_trans = fddi_type_trans;
 		dev = alloc_fddidev(0);