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@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).
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
