[PATCH ethtool 0/2] Fix condition for showing MDI-X status

[PATCH ethtool 0/2] Fix condition for showing MDI-X status

[bage]

From: Bastian Germann <bage@linutronix.de>

Currently there is a filter for showing the MDI-X info. It is only
presented for twisted pair ports, which suppresses the info, e.g., for MII.
I found that issue running ethtool on a br53 switch port.

Despite the enum names, I cannot find documentation on the MDIX fields only
being valid for twisted pair ports -- if they are present, they should be
valid. But maybe I am mistaken.

Additionally, fix a duplicate condition.

Bastian Germann (2):
  netlink: settings: Correct duplicate condition
  netlink: settings: Drop port filter for MDI-X

 netlink/settings.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.30.2

[PATCH ethtool 1/2] netlink: settings: Correct duplicate condition

[bage]

From: Bastian Germann <bage@linutronix.de>

tb's fields ETHTOOL_A_LINKINFO_TP_MDIX and ETHTOOL_A_LINKINFO_TP_MDIX_CTRL
are used in this case. The condition is duplicate for the former. Fix that.

Signed-off-by: Bastian Germann <bage@linutronix.de>
---
 netlink/settings.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/netlink/settings.c b/netlink/settings.c
index 6d10a07..c4f5d61 100644
--- a/netlink/settings.c
+++ b/netlink/settings.c
@@ -560,7 +560,7 @@ int linkinfo_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 		print_enum(names_transceiver, ARRAY_SIZE(names_transceiver),
 			   val, "Transceiver");
 	}
-	if (tb[ETHTOOL_A_LINKINFO_TP_MDIX] && tb[ETHTOOL_A_LINKINFO_TP_MDIX] &&
+	if (tb[ETHTOOL_A_LINKINFO_TP_MDIX] && tb[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL] &&
 	    port == PORT_TP) {
 		uint8_t mdix = mnl_attr_get_u8(tb[ETHTOOL_A_LINKINFO_TP_MDIX]);
 		uint8_t mdix_ctrl =
-- 
2.30.2

[PATCH ethtool 2/2] netlink: settings: Drop port filter for MDI-X

[bage]

From: Bastian Germann <bage@linutronix.de>

The port == PORT_TP condition on printing linkinfo's MDI-X field prevents
ethtool from printing that info even if it is present and valid, e.g. with
the port being MII and still having that info.

Signed-off-by: Bastian Germann <bage@linutronix.de>
---
 netlink/settings.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/netlink/settings.c b/netlink/settings.c
index c4f5d61..4da251b 100644
--- a/netlink/settings.c
+++ b/netlink/settings.c
@@ -560,8 +560,7 @@ int linkinfo_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 		print_enum(names_transceiver, ARRAY_SIZE(names_transceiver),
 			   val, "Transceiver");
 	}
-	if (tb[ETHTOOL_A_LINKINFO_TP_MDIX] && tb[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL] &&
-	    port == PORT_TP) {
+	if (tb[ETHTOOL_A_LINKINFO_TP_MDIX] && tb[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL]) {
 		uint8_t mdix = mnl_attr_get_u8(tb[ETHTOOL_A_LINKINFO_TP_MDIX]);
 		uint8_t mdix_ctrl =
 			mnl_attr_get_u8(tb[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL]);
-- 
2.30.2

Re: [PATCH ethtool 2/2] netlink: settings: Drop port filter for MDI-X

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]

On Wed, Oct 27, 2021 at 08:11:40PM +0200, bage@linutronix.de wrote:
> From: Bastian Germann <bage@linutronix.de>
> 
> The port == PORT_TP condition on printing linkinfo's MDI-X field prevents
> ethtool from printing that info even if it is present and valid, e.g. with
> the port being MII and still having that info.
> 
> Signed-off-by: Bastian Germann <bage@linutronix.de>
> ---
>  netlink/settings.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/netlink/settings.c b/netlink/settings.c
> index c4f5d61..4da251b 100644
> --- a/netlink/settings.c
> +++ b/netlink/settings.c
> @@ -560,8 +560,7 @@ int linkinfo_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  		print_enum(names_transceiver, ARRAY_SIZE(names_transceiver),
>  			   val, "Transceiver");
>  	}
> -	if (tb[ETHTOOL_A_LINKINFO_TP_MDIX] && tb[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL] &&
> -	    port == PORT_TP) {
> +	if (tb[ETHTOOL_A_LINKINFO_TP_MDIX] && tb[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL]) {
>  		uint8_t mdix = mnl_attr_get_u8(tb[ETHTOOL_A_LINKINFO_TP_MDIX]);
>  		uint8_t mdix_ctrl =
>  			mnl_attr_get_u8(tb[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL]);

It's a bit more complicated, I'm afraid. With current kernel code, both
ETHTOOL_A_LINKINFO_TP_MDIX and ETHTOOL_A_LINKINFO_TP_MDIX_CTRL will be
always present in kernel reply so that we would always show something.
Also, the same condition is also used in ioctl code path and we should
try to keep the output consistent between ioctl and netlink.

How about replacing "port == PORT_TP" with

  (port == TP || mdix != ETH_TP_MDI_INVALID || mdix_ctrl != ETH_TP_MDI_INVALID)

in both code path and probably moving the check into dump_mdix()?

We could (and perhaps should) also modify kernel code to omit
ETHTOOL_A_LINKINFO_TP_MDIX or ETHTOOL_A_LINKINFO_TP_MDIX_CTRL if the
value is ETH_TP_MDI_INVALID but ethtool would still have to check for
both options (absence and ETH_TP_MDI_INVALID value) to preserve
compatibility with older kernels.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ethtool 1/2] netlink: settings: Correct duplicate condition

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

On Wed, Oct 27, 2021 at 08:11:39PM +0200, bage@linutronix.de wrote:
> From: Bastian Germann <bage@linutronix.de>
> 
> tb's fields ETHTOOL_A_LINKINFO_TP_MDIX and ETHTOOL_A_LINKINFO_TP_MDIX_CTRL
> are used in this case. The condition is duplicate for the former. Fix that.
> 
> Signed-off-by: Bastian Germann <bage@linutronix.de>
> ---
>  netlink/settings.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/netlink/settings.c b/netlink/settings.c
> index 6d10a07..c4f5d61 100644
> --- a/netlink/settings.c
> +++ b/netlink/settings.c
> @@ -560,7 +560,7 @@ int linkinfo_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  		print_enum(names_transceiver, ARRAY_SIZE(names_transceiver),
>  			   val, "Transceiver");
>  	}
> -	if (tb[ETHTOOL_A_LINKINFO_TP_MDIX] && tb[ETHTOOL_A_LINKINFO_TP_MDIX] &&
> +	if (tb[ETHTOOL_A_LINKINFO_TP_MDIX] && tb[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL] &&
>  	    port == PORT_TP) {
>  		uint8_t mdix = mnl_attr_get_u8(tb[ETHTOOL_A_LINKINFO_TP_MDIX]);
>  		uint8_t mdix_ctrl =

Fixes: 10cc3ea337d1 ("netlink: partial netlink handler for gset (no option)")
Acked-by: Michal Kubecek <mkubecek@suse.cz>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ethtool 2/2] netlink: settings: Drop port filter for MDI-X

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

On Wed, Oct 27, 2021 at 09:59:23PM +0200, Michal Kubecek wrote:
> How about replacing "port == PORT_TP" with
> 
>   (port == TP || mdix != ETH_TP_MDI_INVALID || mdix_ctrl != ETH_TP_MDI_INVALID)
> 
> in both code path and probably moving the check into dump_mdix()?

Looking at the code again, we cannot move the check into dump_mdix()
easily as decision if print_banner(nlctx) should be called depends on
its result.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ethtool 0/2] Fix condition for showing MDI-X status

[Andrew Lunn]

Hi Bastian

> I found that issue running ethtool on a br53 switch port.

Is this on the user ports? With copper PHYs? Did you try fixing the
driver so it actually sets TP?

> Despite the enum names, I cannot find documentation on the MDIX fields only
> being valid for twisted pair ports -- if they are present, they should be
> valid. But maybe I am mistaken.

I'm not sure that is true. I've never seen an SFP module that can swap
around the two fibres if they happen to be the wrong way around. And
it makes no sense for BNC based cheapernet, if that actually still
exists anywhere.

       Andrew