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@xxxxxxxxxxxxx>
>>> Cc: Alexandra Winter <wintera@xxxxxxxxxxxxx>
>>> Cc: Wenjia Zhang <wenjia@xxxxxxxxxxxxx>
>>> Cc: linux-s390@xxxxxxxxxxxxxxx
>>> Cc: netdev@xxxxxxxxxxxxxxx
>>> Cc: Heiko Carstens <hca@xxxxxxxxxxxxx>
>>> Cc: Vasily Gorbik <gor@xxxxxxxxxxxxx>
>>> Cc: Alexander Gordeev <agordeev@xxxxxxxxxxxxx>
>>> Cc: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>
>>> Cc: Sven Schnelle <svens@xxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxx/
>>
>> 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.
--
~Randy
