Re: [PATCH net v3] net/iucv: require IUCV for AF_IUCV sockets

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] net/iucv/Kconfig: replacing `depends on S390` with `depends on
  IUCV` for AFIUCV reverts, without disclosure, the design intent of the…

--

net/iucv: require IUCV for AF_IUCV sockets

This changes the AFIUCV Kconfig symbol from "depends on S390" to
"depends on IUCV" so that AFIUCV can no longer be enabled when the IUCV
core is disabled. It replaces the v2 approach of always descending into
net/iucv from net/Makefile.

Previous posting referenced in the patch:
https://lore.kernel.org/all/20260815135456.53684-1-pengpeng@xxxxxxxxxxx/

> diff --git a/net/iucv/Kconfig b/net/iucv/Kconfig
> index 5cfddc9c6498..2686d795cc2f 100644
> --- a/net/iucv/Kconfig
> +++ b/net/iucv/Kconfig
> @@ -9,7 +9,7 @@ config IUCV
>  	  communication link between VM guests.
>  
>  config AFIUCV
> -	depends on S390
> +	depends on IUCV

[Severity: Medium]
Is the stated rationale accurate here?  The commit message says:

  "Independently entering the directory is not sufficient: AF_IUCV also
   relies on the IUCV implementation."

Looking at afiucv_init() in net/iucv/af_iucv.c, the only reference to
the IUCV core is compile-time guarded:

	if (machine_is_vm() && IS_ENABLED(CONFIG_IUCV)) {
		...
		pr_iucv = &iucv_if;
	} else {
		memset(&iucv_userid, 0, sizeof(iucv_userid));
		pr_iucv = NULL;
	}

and iucv_sock_alloc() treats a NULL provider as a supported mode:

	if (pr_iucv)
		iucv->transport = AF_IUCV_TRANS_IUCV;
	else
		iucv->transport = AF_IUCV_TRANS_HIPER;

The other iucv_* users in af_iucv.c are iucv_path_alloc() and
iucv_path_free(), which are static inline wrappers around kzalloc_obj()
and kfree() in include/net/iucv/iucv.h.  Does af_iucv.o then have any
link-time dependency on the IUCV core at all with CONFIG_IUCV=n?

The reason af_iucv.o is not built in that configuration appears to be
only the traversal rule in net/Makefile:

	obj-$(CONFIG_IUCV)		+= iucv/

Also, c69748d1c9b5, named in the Fixes: tag, deliberately changed
"depends on IUCV" to "depends on S390" together with
"def_tristate m if QETH_L3 || IUCV" so that AF_IUCV could be enabled for
the HiperSockets transport alone.  Since this patch makes the AFIUCV
symbol invisible for every CONFIG_IUCV=n configuration, is the word
"Restore" describing the right direction, and could the commit message
say explicitly that the HiperSockets-only configuration is being dropped
rather than fixed?

One further consequence worth spelling out: iucv_init() in
net/iucv/iucv.c bails out early on non-z/VM systems:

	if (!machine_is_vm()) {
		rc = -EPROTONOSUPPORT;
		goto out;
	}

so on an LPAR, does a modular IUCV still work as the provider that
AFIUCV=m now depends on, or does this effectively force CONFIG_IUCV=y
for HiperSockets AF_IUCV users?

>  	def_tristate m if QETH_L3 || IUCV
>  	prompt "AF_IUCV Socket support (S390 - z/VM and HiperSockets transport)"
>  	help

[Severity: Low]
With "depends on IUCV" in place, IUCV is always y or m whenever AFIUCV
is visible, so the QETH_L3 term of

	def_tristate m if QETH_L3 || IUCV

can never decide the default any more.  Should the default clause be
reduced to just "def_tristate m"?

Similarly, the prompt and the help text below still present z/VM and
HiperSockets as separable transports:

	  Select this option if you want to use AF_IUCV socket applications
	  based on z/VM inter-user communication vehicle or based on
	  HiperSockets.

Do these need updating too, now that the HiperSockets-only case can no
longer be configured?  The IS_ENABLED(CONFIG_IUCV) test in
afiucv_init() also becomes permanently true after this change.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260920034503.17322-1-hppiscas%40163.com