Re: [PATCH net] s390/qeth: allow bridgeport queries despite OS_MISMATCH

[netdev-bot+sashiko@kernel.org]

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

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

- [High] qeth_bridgeport_query_ports_cb() now returns success on
  IPA_RC_SBP_IQD_OS_MISMATCH and therefore executes…
- [Medium] The new OS_MISMATCH exception in
  qeth_bridgeport_query_ports_cb()…
- [Medium] qeth_bridge_port_role_state_show() can now emit "none (OS
  family mismatch)\n" for the read-write attribute bridge_role…

--

s390/qeth: allow bridgeport queries despite OS_MISMATCH

This treats IPA_RC_SBP_IQD_OS_MISMATCH as success for
QUERY_BRIDGE_PORTS so that bridge_role and bridge_state stay readable
when another OS family owns the bridge port, and passes a new
os_mismatch out-parameter up to sysfs so the role can be printed as
"none (OS family mismatch)".  SET operations keep returning -EPERM.

A few questions about the new code below.

> diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
> index a9e7d1d637a20..2935c2ecc314b 100644
> --- a/drivers/s390/net/qeth_l2_main.c
> +++ b/drivers/s390/net/qeth_l2_main.c
> @@ -1158,7 +1158,7 @@ static void qeth_l2_setup_bridgeport_attrs(struct qeth_card *card)
>  		qeth_bridgeport_setrole(card, card->options.sbp.role);
>  		/* Let the callback function refresh the stored role value. */
>  		qeth_bridgeport_query_ports(card, &card->options.sbp.role,
> -					    NULL);
> +					    NULL, NULL);
>  	}
>  	if (card->options.sbp.hostnotification) {
>  		if (qeth_bridgeport_an_set(card, 1))

[ ... ]

> @@ -1721,10 +1722,19 @@ static int qeth_bridgeport_query_ports_cb(struct qeth_card *card,
>  	struct qeth_ipa_cmd *cmd = (struct qeth_ipa_cmd *) data;
>  	struct _qeth_sbp_cbctl *cbctl = (struct _qeth_sbp_cbctl *)reply->param;
>  	struct qeth_sbp_port_data *qports;
> +	u16 sbp_rc;
>  	int rc;
>  
>  	QETH_CARD_TEXT(card, 2, "brqprtcb");
> -	rc = qeth_bridgeport_makerc(card, cmd);
> +	sbp_rc = cmd->data.sbp.hdr.return_code;
> +
> +	/* on OS family mismatch, query still returns valid port data;
> +	 * treat as success
> +	 */
> +	if (sbp_rc == IPA_RC_SBP_IQD_OS_MISMATCH && !cmd->hdr.return_code)
> +		rc = 0;
> +	else
> +		rc = qeth_bridgeport_makerc(card, cmd);
>  	if (rc)
>  		return rc;

[Severity: Medium]
Should this test also be qualified with IS_IQD(card)?

qeth_bridgeport_makerc() decodes the very same
cmd->data.sbp.hdr.return_code field in a card-type dependent way:

drivers/s390/net/qeth_l2_main.c:qeth_bridgeport_makerc() {
	if ((IS_IQD(card) && ipa_rc == IPA_RC_SUCCESS) ||
	    (!IS_IQD(card) && ipa_rc == sbp_rc)) {
		switch (sbp_rc) {
		...
		case IPA_RC_SBP_OSA_OS_MISMATCH:
		case IPA_RC_SBP_IQD_OS_MISMATCH:
			rc = -EPERM;
	...
	} else {
		switch (ipa_rc) {
		...
		default:
			rc = -EIO;
	}
}

and the two families use different numbering in qeth_core_mpc.h:

	IPA_RC_SBP_IQD_OS_MISMATCH	= 0x0010,
	IPA_RC_SBP_OSA_OS_MISMATCH	= 0x2B10,

On an OSA (non-IQD) card, a reply carrying ipa_rc == 0 and sbp_rc ==
0x0010 is not an OS mismatch in the OSA numbering, yet the new shortcut
turns it into success, parses port_data as valid role/state and reports
"none (OS family mismatch)" to userspace, where makerc() would have
returned -EIO through its else branch.

The opposite direction is a genuine OSA mismatch (ipa_rc == sbp_rc ==
0x2B10), which is not whitelisted here, so bridge_role and bridge_state
reads on OSA bridgeports keep failing with -EPERM.

Related, the new kernel-doc for the out-parameter says

 * @os_mismatch: if non-NULL, set to true when firmware reports
 *		 OS family mismatch.

which reads more generic than an IQD-only implementation.  Would it make
sense to either extend the check to IPA_RC_SBP_OSA_OS_MISMATCH with the
OSA ipa_rc == sbp_rc convention, or document it as IQD-only?

> @@ -1740,6 +1750,9 @@ static int qeth_bridgeport_query_ports_cb(struct qeth_card *card,
>  		if (cbctl->data.qports.state)
>  			*cbctl->data.qports.state = qports->entry[0].state;
>  	}
> +	if (cbctl->data.qports.os_mismatch)
> +		*cbctl->data.qports.os_mismatch =
> +			(sbp_rc == IPA_RC_SBP_IQD_OS_MISMATCH);
>  	return 0;
>  }

[Severity: High]
Can this discard the user's configured bridge port role?

Because the callback no longer returns early with -EPERM on OS mismatch,
it now falls through to the role store just above:

drivers/s390/net/qeth_l2_main.c:qeth_bridgeport_query_ports_cb() {
	if (qports->num_entries > 0) {
		if (cbctl->data.qports.role)
			*cbctl->data.qports.role = qports->entry[0].role;
	...
}

Both callers pass the persistent configuration field as that
out-parameter, qeth_l2_setup_bridgeport_attrs() with
&card->options.sbp.role and qeth_bridge_port_role_state_show() likewise,
so the firmware placeholder role "none" described in the commit message
gets written into card->options.sbp.role.

That has a few follow-on effects.  A read-only sysfs read of bridge_role
mutates stored configuration.  Re-application of the role is then gated
off for good:

drivers/s390/net/qeth_l2_main.c:qeth_l2_setup_bridgeport_attrs() {
	if (!card->options.sbp.reflect_promisc &&
	    card->options.sbp.role != QETH_SBP_ROLE_NONE) {
		qeth_bridgeport_setrole(card, card->options.sbp.role);
	...
}

so once the field has been zeroed, no later set-online or recovery
retries qeth_bridgeport_setrole(), even after the other OS family
releases the port.

The same field also feeds the mutual-exclusion predicate:

drivers/s390/net/qeth_l2.h:qeth_bridgeport_is_in_use() {
	return card->options.sbp.role ||
	       card->options.sbp.reflect_promisc ||
	       card->options.sbp.hostnotification;
}

Zeroing sbp.role flips that to false and unblocks the VNICC set/get
paths, qeth_l2_bridge_getlink(), the learning-sync path, and makes
bridge_reflect_promisc writable while a bridge role is configured.

A sequence that reaches this: write "primary" to bridge_role while
offline (stored without a firmware command), then set the device online;
qeth_bridgeport_setrole() still fails with -EPERM under mismatch, and the
following qeth_bridgeport_query_ports(card, &card->options.sbp.role,
NULL, NULL) now succeeds and overwrites the field with "none".  A single
read of bridge_role while online reproduces the same clobbering.

Would it work to publish the placeholder role only into the local sysfs
output and leave card->options.sbp.role untouched in the mismatch case?

> diff --git a/drivers/s390/net/qeth_l2_sys.c b/drivers/s390/net/qeth_l2_sys.c
> index 7f592f9125172..7101be62eb1d5 100644
> --- a/drivers/s390/net/qeth_l2_sys.c
> +++ b/drivers/s390/net/qeth_l2_sys.c
> @@ -25,7 +26,7 @@ static ssize_t qeth_bridge_port_role_state_show(struct device *dev,
>  	if (qeth_card_hw_is_reachable(card) &&
>  					card->options.sbp.supported_funcs)
>  		rc = qeth_bridgeport_query_ports(card,
> -			&card->options.sbp.role, &state);
> +			&card->options.sbp.role, &state, &os_mismatch);
>  	if (!rc) {
>  		if (show_state)
>  			switch (state) {

[ ... ]

> @@ -52,6 +53,10 @@ static ssize_t qeth_bridge_port_role_state_show(struct device *dev,
>  		if (rc)
>  			QETH_CARD_TEXT_(card, 2, "SBP%02x:%02x",
>  				card->options.sbp.role, state);
> +		else if (!show_state &&
> +			 card->options.sbp.role == QETH_SBP_ROLE_NONE &&
> +			 os_mismatch)
> +			rc = sysfs_emit(buf, "%s (OS family mismatch)\n", word);
>  		else
>  			rc = sysfs_emit(buf, "%s\n", word);
>  	}

[Severity: Medium]
Can the value emitted here be written back to the same attribute?

bridge_role is read/write:

drivers/s390/net/qeth_l2_sys.c:
static DEVICE_ATTR(bridge_role, 0644, qeth_bridge_port_role_show,
		   qeth_bridge_port_role_store);

and the store side accepts only the bare tokens:

drivers/s390/net/qeth_l2_sys.c:qeth_bridge_port_role_store() {
	if (sysfs_streq(buf, "primary"))
		role = QETH_SBP_ROLE_PRIMARY;
	else if (sysfs_streq(buf, "secondary"))
		role = QETH_SBP_ROLE_SECONDARY;
	else if (sysfs_streq(buf, "none"))
		role = QETH_SBP_ROLE_NONE;
	else
		return -EINVAL;
	...
}

sysfs_streq() tolerates a trailing newline but not the
" (OS family mismatch)" suffix, so a read-modify-write or a
save-and-restore of bridge_role by the zdev tooling named in the commit
message (chzdev save/restore) would get -EINVAL, and parsers that exact
match "none" no longer recognize the role string.  This is emitted while
the card is reachable and sbp is supported, that is exactly when store()
is otherwise usable.

There is no Documentation/ABI entry for this attribute, so store() is the
only in-tree description of the value space.  Should the mismatch
indication go somewhere other than the value of a writable attribute, or
should store() learn to accept the decorated form?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901155344.3561483-1-nagamani%40linux.ibm.com