Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Jun 2023 21:37:09 +0200
From:   Simon Horman <simon.horman@corigine.com>
To:     Randy Dunlap <rdunlap@infradead.org>
Cc:     linux-kernel@vger.kernel.org,
        Alexandra Winter <wintera@linux.ibm.com>,
        Wenjia Zhang <wenjia@linux.ibm.com>,
        linux-s390@vger.kernel.org, netdev@vger.kernel.org,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>
Subject: Re: [PATCH] s390/net: lcs: use IS_ENABLED() for kconfig detection
Message-ID: <ZJNRZZCkGdvp+k34@corigine.com>
References: <20230615222152.13250-1-rdunlap@infradead.org>
 <ea55623d-d469-ddaf-92ce-3daf1d2d726f@infradead.org>
 <ZJMc3oS2nxORPASN@corigine.com>
 <66fd6106-16a5-d61a-4202-02ff99b84f76@infradead.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <66fd6106-16a5-d61a-4202-02ff99b84f76@infradead.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?BCCctOtXnUK6NZqtLiXEOuM3inF0J6MbvM3OgX5uMwAORqhVJhqLTKefOkra?=
 =?us-ascii?Q?e4iEOVEM3rwNTq5NOLQdzOVkO2fZ/RMLe59cQ44GlwPRW3yvLqn0xS/Rfb0M?=
 =?us-ascii?Q?ePjO9j9rC/QO2vuvaW7SxoMMFN0Vsx3pWcZj4RH/mr0NvRYL85IVjhAx9Xeb?=
 =?us-ascii?Q?KtA1x9OrYyTfdA8potFpPFdMitp+UEuVi6JKNzsfXNEX06Gg1/y5gV8gYBJ8?=
 =?us-ascii?Q?Vkqy9NnxQAufNNr9JNppwmarlUrG2rLTZKaqAo9OGbKS8NycoYyTZpT9JRWX?=
 =?us-ascii?Q?PiEwra/96SM9VN3QPTCKIJPeWukHjpssjWi9eGovSBuz94LL2bEPMHvcrqf2?=
 =?us-ascii?Q?IycIAUm/VsH6VyReTHoTFtHJFP4UzvkAyuIc36BnNFY49ibFwGvPQkzOicOE?=
 =?us-ascii?Q?gJmwUiwUhgO79+ZtEAZ2Y+REXUH/8yUSPsX9JfVv5Uv0SldkiF/sRacn8Aro?=
 =?us-ascii?Q?Fq6W5KjRZ+fT5dAK69nFpO4t8Xu9zL72f8Lk2SgZjmop0jzvjhsAdb+zDBfG?=
 =?us-ascii?Q?EAqoxiB41AD3QHjFSC2l1U7QIL5Lkn0eW5ld+HYGFJoYkeYtI5n5S+4wXRN8?=
 =?us-ascii?Q?myqGc0sfNYucFiXO+M7QA4FbEblchHSRehVPRgxioEuEmVcu3FwfWJJC7Cqx?=
 =?us-ascii?Q?3o0M/nN8V7fdnJqJlUQAlLUPsOfIFx+YK0sXXZKZ1sG/gGDkA9I+ceGV7ESv?=
 =?us-ascii?Q?UvURE/zEVVW8padWZbwfmVr1K7eeId1PbxthsWB5+ZS+Cix84zXEeN5DUljL?=
 =?us-ascii?Q?nGn9iV0GNqgUx5isDzXaMEzVBFKioa8iVgsipPgvaWXlujX9yXJ4rdFx9Ddg?=
 =?us-ascii?Q?LAM5LNpaF7iXViWnMgd0cbmdaFhxknny+f2QvcBGP9mSumLRt6H+uBHgrwX/?=
 =?us-ascii?Q?ZBfik3HEhutDodRrXQ3Z4k8FoDOqQi8Hg1f//Jv3DYtPV2YKnVJka32iZG64?=
 =?us-ascii?Q?jpfDcgsACgSl0Jtmf30XiO8VgNaH5k8o9c4oSEvK+vLcS5Ck2ch4EST9zmIY?=
 =?us-ascii?Q?awSY55Pj05V9KLj7H1iEJM/GAs2dj5INQCxiwV4bpoKbTKDCnJeIJZ5pE9zO?=
 =?us-ascii?Q?g3/7G2ZsBfxxain75Lgusrhdy/vD1beS3rQNRo0vfNHSZFMOsCrOCotn4ruO?=
 =?us-ascii?Q?bFRHhjLE7GedLYq14694KiLIHFMr8r11nZsBUNDjbDYNHgXQIxQS/NyfKaZ2?=
 =?us-ascii?Q?V25y/pm+rv4crPvLOovu9Bw7vC3ewFHOyU9MSnwBIMNGEyajCqjZELgitwEu?=
 =?us-ascii?Q?vKRlzcsSxz6SH4KUIB0aYg3Sn0JpOQOD17ql9vL5HPbE0Ur0duvuJ6OxJpxT?=
 =?us-ascii?Q?30hxGuxSMSZ5FOsqoMPv8iVittW2zP5PlfOieQVRq0e+mngVtrAWwbHq/wwd?=
 =?us-ascii?Q?giTOPW0NmRCn1KWLuETLpihpRxdYcGYQm01bW3YK2rYScj1AGT9qrBDcST+C?=
 =?us-ascii?Q?9i2/O/Z+wxZMkaGDFQ3SEOshOjcVG3Y1wv0gthYLdQ9yLHOhU/E/D5uRZ4dE?=
 =?us-ascii?Q?XNwN6xecfEGc0u80UQkVf6K8a0TwTH4v0M9lIpE7kluoZydA19IXseqyiSPi?=
 =?us-ascii?Q?zQQxdGtL5GlJzrtgVsyu6lAippLag1S6feVJY3K/w6qorkViCdiSumh2OZbK?=
 =?us-ascii?Q?uA=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 572a355e-2021-42db-1707-08db728eeae5
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jun 2023 19:37:15.7844
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 5gFH2VsMVQF6E0yfiPCdI7rp+WCgPF3pR4/derRmvKmlJw2E1fb0qptyGJnlp7S73flakEXKAvNPLI2J17vjO/y+2i8f3L1C8laAafjhFKI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR13MB4152
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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