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

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

[Nagamani PV]

When HiperSockets interfaces on the same VCHID span different OS
families, reads of the sysfs attributes bridge_role and bridge_state
fail with -EPERM if bridge port ownership belongs to another OS family.

As a result, userspace tools such as 'lszdev -ii' cannot retrieve
bridge_role and bridge_state, even though firmware returns valid bridge
port data for QUERY_BRIDGE_PORTS requests.

The firmware reports IPA_RC_SBP_IQD_OS_MISMATCH (0x0010) to indicate
that bridge port ownership belongs to a different OS family. For
QUERY_BRIDGE_PORTS operations, firmware still returns valid bridge port
data (role=none, state=inactive) together with a primary return code of
0x0000 (success).

Allow QUERY_BRIDGE_PORTS requests to return the bridge port data
provided by the firmware despite OS_MISMATCH. To make the OS family
mismatch visible to userspace, represent the firmware-reported role
"none" as "none (OS family mismatch)" while preserving the reported
bridge_state.

The behavior for non-QUERY bridge port commands is unchanged; SET
operations continue to return -EPERM when another OS family owns the
bridge port.

This restores readability of bridge_role and bridge_state.

Fixes: 1b05cf6285c1 ("qeth: Include error message for "OS Mismatch"")
Cc: stable@vger.kernel.org
Suggested-by: Halil Pasic <pasic@linux.ibm.com>
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
---
 drivers/s390/net/qeth_l2.h      |  3 ++-
 drivers/s390/net/qeth_l2_main.c | 26 ++++++++++++++++++++++----
 drivers/s390/net/qeth_l2_sys.c  |  7 ++++++-
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/net/qeth_l2.h b/drivers/s390/net/qeth_l2.h
index 7c646e2fed7e..f94975e970ca 100644
--- a/drivers/s390/net/qeth_l2.h
+++ b/drivers/s390/net/qeth_l2.h
@@ -13,7 +13,8 @@ extern const struct attribute_group *qeth_l2_attr_groups[];
 
 int qeth_bridgeport_query_ports(struct qeth_card *card,
 				enum qeth_sbp_roles *role,
-				enum qeth_sbp_states *state);
+				enum qeth_sbp_states *state,
+				bool *os_mismatch);
 int qeth_bridgeport_setrole(struct qeth_card *card, enum qeth_sbp_roles role);
 int qeth_bridgeport_an_set(struct qeth_card *card, int enable);
 
diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index a9e7d1d637a2..2935c2ecc314 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))
@@ -1545,6 +1545,7 @@ struct _qeth_sbp_cbctl {
 		struct {
 			enum qeth_sbp_roles *role;
 			enum qeth_sbp_states *state;
+			bool *os_mismatch;
 		} qports;
 	} data;
 };
@@ -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;
 
@@ -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;
 }
 
@@ -1748,13 +1761,17 @@ static int qeth_bridgeport_query_ports_cb(struct qeth_card *card,
  * @card:			   qeth_card structure pointer.
  * @role:   Role of the port: 0-none, 1-primary, 2-secondary.
  * @state:  State of the port: 0-inactive, 1-standby, 2-active.
+ * @os_mismatch: if non-NULL, set to true when firmware reports
+ *		 OS family mismatch.
  *
  * Returns negative errno-compatible error indication or 0 on success.
  *
- * 'role' and 'state' are not updated in case of hardware operation failure.
+ * 'role', 'state' and 'os_mismatch' are not updated in case of
+ * hardware operation failure.
  */
 int qeth_bridgeport_query_ports(struct qeth_card *card,
-	enum qeth_sbp_roles *role, enum qeth_sbp_states *state)
+	enum qeth_sbp_roles *role, enum qeth_sbp_states *state,
+	bool *os_mismatch)
 {
 	struct qeth_cmd_buffer *iob;
 	struct _qeth_sbp_cbctl cbctl = {
@@ -1762,6 +1779,7 @@ int qeth_bridgeport_query_ports(struct qeth_card *card,
 			.qports = {
 				.role = role,
 				.state = state,
+				.os_mismatch = os_mismatch,
 			},
 		},
 	};
diff --git a/drivers/s390/net/qeth_l2_sys.c b/drivers/s390/net/qeth_l2_sys.c
index 7f592f912517..7101be62eb1d 100644
--- a/drivers/s390/net/qeth_l2_sys.c
+++ b/drivers/s390/net/qeth_l2_sys.c
@@ -15,6 +15,7 @@ static ssize_t qeth_bridge_port_role_state_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 	enum qeth_sbp_states state = QETH_SBP_STATE_INACTIVE;
+	bool os_mismatch = false;
 	int rc = 0;
 	char *word;
 
@@ -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);
 	}
-- 
2.53.0

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

[sashiko-bot]

> When HiperSockets interfaces on the same VCHID span different OS
> families, reads of the sysfs attributes bridge_role and bridge_state
> fail with -EPERM if bridge port ownership belongs to another OS family.
> 
> As a result, userspace tools such as 'lszdev -ii' cannot retrieve
> bridge_role and bridge_state, even though firmware returns valid bridge
> port data for QUERY_BRIDGE_PORTS requests.
> 
> The firmware reports IPA_RC_SBP_IQD_OS_MISMATCH (0x0010) to indicate
> that bridge port ownership belongs to a different OS family. For
> QUERY_BRIDGE_PORTS operations, firmware still returns valid bridge port
> data (role=none, state=inactive) together with a primary return code of
> 0x0000 (success).
> 
> Allow QUERY_BRIDGE_PORTS requests to return the bridge port data
> [ ... ]
> Fixes: 1b05cf6285c1 ("qeth: Include error message for "OS Mismatch"")
> Cc: stable@vger.kernel.org
> Suggested-by: Halil Pasic <pasic@linux.ibm.com>
> Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
> Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901155344.3561483-1-nagamani@linux.ibm.com?part=1

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

[netdev-bot+sashiko]

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